FIX: proper handling of group memberships#8
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe pull request refactors group management from a usernames-based model to a pagination-aware, RESTful architecture. Changes include controller action reorganization (create/update/destroy), new member add/remove endpoints, pagination-aware member fetching in the frontend and backend, template restructuring with inline member controls, and updated routing and localization. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/assets/javascripts/discourse/templates/group/members.hbs (2)
19-19: Malformed HTML: orphaned closing</div>tag.Line 19 has a
</div>with no corresponding opening tag. This will cause DOM structure issues. It should likely be removed.🔧 Proposed fix
</tr> - </div> {{/each}}
1-6: Guard condition and malformed HTML require fixes.Line 1 checks
{{#if model}}but line 6 iterates overmembers. SincefindMembers()on line 11 ofgroup-members.js.es6is asynchronous and doesn't block route rendering,memberswill be undefined when the template first renders. The table structure will render with headers but no rows until the AJAX call completes. Change the guard to{{#if members}}or{{#if members.length}}for consistency.Additionally, line 20 contains an orphaned closing
</div>tag with no corresponding opening tag—remove it to fix the HTML structure.
🤖 Fix all issues with AI agents
In @app/assets/javascripts/admin/controllers/admin-group.js.es6:
- Around line 51-59: The removeMember method currently calls
this.get("model").removeMember(member) without handling its promise; update
removeMember to handle the returned promise from model.removeMember(member) by
attaching success and failure handlers (e.g., .then(...) and .catch(...) or
using async/await) so that on success you proceed as before and on failure you
surface an error to the user (use bootbox.alert or I18n.t-based message) and
optionally log the error; reference the removeMember function and the
model.removeMember call when making the change.
- Around line 11-14: The totalPages computed property has an off-by-one bug:
replace the current floor+1 logic in totalPages with a proper ceiling-based
calculation (use Math.ceil(this.get("user_count") / this.get("limit"))) and keep
the zero-check; also guard against a zero or falsy limit by returning 0 when
this.get("limit") is not positive. This change should be applied inside the
totalPages property implementation to correctly handle exact multiples of limit.
In @app/assets/javascripts/admin/templates/group.hbs:
- Line 29: The button that triggers the addMembers action is inside a form and
lacks an explicit type, so it defaults to submitting the form; update the button
element that invokes the "addMembers" action (the <button {{action
"addMembers"}} ...> in the template) to include type="button" to prevent form
submission and ensure it only fires the addMembers action.
- Line 49: The delete button currently lacks a type attribute so clicking it
submits the surrounding form instead of invoking the "destroy" action; update
the button element that uses {{action "destroy"}} and class 'btn btn-danger' to
include type="button" so it does not act as a submit control and continues to
trigger the destroy action as intended.
In @app/assets/javascripts/discourse/models/group.js:
- Around line 40-49: The removeMember function currently calls
this.findMembers() inside the .then handler but does not return its promise, so
callers of removeMember can resolve before the member reload completes; update
the .then handler in removeMember (the Discourse.ajax(...).then(...) block) to
return self.findMembers() (i.e., replace the fire-and-forget call with a
returned promise) so the outer promise chain waits for the member list refresh
to finish.
- Around line 51-60: The addMembers function doesn't return the promise from
self.findMembers(), breaking proper chaining, and it's missing a trailing
semicolon; update addMembers (in the group.js model) so the .then callback
returns self.findMembers() and add the missing semicolon after the .then block
to fix promise chaining and syntax.
- Around line 77-84: The save method lacks an id guard and the destroy method
contains an extraneous semicolon; update save (function save) to return early if
this.get('id') is falsy (mirroring destroy's guard) to avoid calling
/admin/groups/undefined, and remove the stray semicolon after the return in
destroy (change "if (!this.get('id')) { return };" to "if (!this.get('id')) {
return }") so the syntax is clean.
In @app/controllers/admin/groups_controller.rb:
- Line 8: The WHERE clause on groups uses the raw search string as "%#{search}%"
which treats %, _, and \ as SQL LIKE wildcards; fix by escaping those characters
using ActiveRecord's sanitize_sql_like (e.g., call
ActiveRecord::Base.sanitize_sql_like or model.sanitize_sql_like on the search)
and then interpolate the escaped value into the ILIKE pattern used in the groups
= groups.where("name ILIKE ?", "%#{escaped}%") expression so searches match
literally.
In @app/controllers/groups_controller.rb:
- Around line 22-23: The user-controlled limit has no upper bound; clamp it to a
safe maximum by defining a MAX_LIMIT constant (e.g., 1000) and replacing the
current assignment of limit to coerce and clamp params[:limit] into the allowed
range (ensure it remains an integer >= 1 and <= MAX_LIMIT). Update the line that
assigns limit (currently using params[:limit] and .to_i) to use the clamp logic
(or equivalent min/max checks) so large requests are rejected/truncated; leave
offset handling unchanged.
🧹 Nitpick comments (11)
app/assets/stylesheets/common/admin/admin_base.scss (1)
471-476: Hardcoded colors break theme consistency.Using
#333and#aaadirectly bypasses the theme system used throughout this file. These colors won't adapt to dark themes or custom color schemes, potentially causing accessibility and readability issues.Consider using theme variables or utility functions:
♻️ Suggested refactor using theme variables
.next, .previous { - color: #333 !important; + color: $primary !important; &.disabled { - color: #aaa !important; + color: scale-color($primary, $lightness: 50%) !important; } }app/assets/javascripts/discourse/routes/group-members.js.es6 (1)
8-12: Unhandled promise fromfindMembers()may silently fail.
model.findMembers()returns a promise that isn't awaited or chained with error handling. If the API call fails, users receive no feedback and the members list remains empty. Consider adding a.catch()handler or using Ember's error handling patterns.♻️ Suggested improvement
setupController: function(controller, model) { this.controllerFor('group').set('showing', 'members'); controller.set("model", model); - model.findMembers(); + model.findMembers().catch(function() { + // Handle error - could set an error state on controller + controller.set("membersLoadError", true); + }); }app/assets/javascripts/discourse/models/group.js (2)
20-38: Consider adding error handling for the AJAX request.The
findMembersmethod lacks error handling. If the AJAX request fails, the promise rejection will propagate unhandled. Consider adding a.catch()handler or returning a rejected promise with a meaningful error.Also, the early return on line 21 returns
undefinedwhile the success path returns a resolved promise. For consistency, consider returning a resolved promise:- if (Em.isEmpty(this.get('name'))) { return ; } + if (Em.isEmpty(this.get('name'))) { return Ember.RSVP.resolve(); }
70-75: Potential null reference if response format is unexpected.If the server response doesn't contain
basic_group, line 73 will throwCannot read property 'id' of undefined. Consider adding defensive checks or error handling.♻️ Suggested fix
create: function(){ var self = this; return Discourse.ajax("/admin/groups", { type: "POST", data: this.asJSON() }).then(function(resp) { - self.set('id', resp.basic_group.id); + if (resp && resp.basic_group) { + self.set('id', resp.basic_group.id); + } }); },app/assets/javascripts/admin/routes/admin_group_route.js (1)
11-15: Consider handling thefindMembers()promise result.The
findMembers()call on line 13 is fire-and-forget. While this may be intentional for async loading, any errors will go unhandled. Consider adding error handling to provide user feedback if member loading fails.setupController: function(controller, model) { controller.set("model", model); - model.findMembers(); + model.findMembers().catch(function(error) { + // Handle error - e.g., show notification + console.error("Failed to load group members:", error); + }); }spec/controllers/admin/groups_controller_spec.rb (2)
51-58: Tests rely on magic numberid: 1for automatic group.The tests assume that group with
id: 1is an automatic group (likely seeded). While this may work in the test environment, it's fragile. Consider fabricating an explicit automatic group for clarity and reliability.Suggested improvement
it "ignore name change on automatic group" do - xhr :put, :update, id: 1, name: "WAT", visible: "true" + auto_group = Fabricate(:group, automatic: true) + xhr :put, :update, id: auto_group.id, name: "WAT", visible: "true" response.should be_success - group = Group.find(1) + group = Group.find(auto_group.id) group.name.should_not == "WAT" group.visible.should == true end
93-96: Same magic number pattern for automatic group tests.Lines 94 and 115 also rely on
group_id: 1being an automatic group. Consider using a fabricated automatic group for consistency.app/assets/javascripts/admin/controllers/admin-group.js.es6 (2)
6-9: Use strict equality (===) for comparisons.Using
==for comparison can lead to unexpected type coercion. Consider using===for clarity and safety.Proposed fix
currentPage: function() { - if (this.get("user_count") == 0) { return 0; } + if (this.get("user_count") === 0) { return 0; } return Math.floor(this.get("offset") / this.get("limit")) + 1; }.property("limit", "offset", "user_count"),
61-65: Address the TODO: Clear the input after adding members.The comment indicates the input should be cleared after adding members. This would improve UX by preventing duplicate submissions.
Would you like me to suggest an implementation that clears
usernamesafter successfully adding members?app/controllers/admin/groups_controller.rb (2)
71-75: Silent skip when username is not found.Non-existent usernames are silently ignored. This may be intentional, but consider logging or returning feedback about which usernames were not found so admins can correct typos.
72-72: Wrap assignment in parentheses to clarify intent.RuboCop flags assignment-in-condition. Adding parentheses signals the assignment is intentional.
Proposed fix
- if user = User.find_by_username(username) + if (user = User.find_by_username(username))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/assets/javascripts/admin/controllers/admin-group.js.es6app/assets/javascripts/admin/routes/admin_group_route.jsapp/assets/javascripts/admin/templates/group.hbsapp/assets/javascripts/admin/templates/group_member.hbsapp/assets/javascripts/admin/views/group-member.js.es6app/assets/javascripts/discourse/models/group.jsapp/assets/javascripts/discourse/routes/group-members.js.es6app/assets/javascripts/discourse/templates/components/admin-group-selector.hbsapp/assets/javascripts/discourse/templates/group/members.hbsapp/assets/javascripts/discourse/templates/user-selector-autocomplete.raw.hbsapp/assets/stylesheets/common/admin/admin_base.scssapp/controllers/admin/groups_controller.rbapp/controllers/groups_controller.rbconfig/locales/client.en.ymlconfig/routes.rbspec/controllers/admin/groups_controller_spec.rb
💤 Files with no reviewable changes (1)
- app/assets/javascripts/discourse/templates/components/admin-group-selector.hbs
🧰 Additional context used
🧬 Code graph analysis (4)
app/controllers/groups_controller.rb (1)
app/controllers/application_controller.rb (1)
serialize_data(191-200)
spec/controllers/admin/groups_controller_spec.rb (1)
app/models/group.rb (2)
add(272-274)name(161-163)
config/routes.rb (1)
lib/discourse_hub.rb (1)
put(27-29)
app/controllers/admin/groups_controller.rb (3)
app/controllers/application_controller.rb (3)
render_serialized(206-208)render_json_error(301-303)success_json(305-307)app/models/group.rb (5)
name(161-163)refresh_automatic_groups!(146-153)usernames(244-266)usernames(268-270)add(272-274)app/models/user.rb (1)
find_by_username(175-177)
🪛 RuboCop (1.82.1)
spec/controllers/admin/groups_controller_spec.rb
[convention] 14-15: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
[convention] 31-32: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
[convention] 35-36: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
[convention] 46-47: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
[convention] 50-51: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
[convention] 59-60: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
[convention] 77-78: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
[convention] 81-82: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
[convention] 88-89: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
[convention] 92-93: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
[convention] 109-110: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
[convention] 113-114: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
[convention] 131-132: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
app/controllers/admin/groups_controller.rb
[warning] 6-6: Use == if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.
(Lint/AssignmentInCondition)
[warning] 72-72: Use == if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.
(Lint/AssignmentInCondition)
🔇 Additional comments (12)
app/assets/stylesheets/common/admin/admin_base.scss (1)
164-182: LGTM! Good extraction of shared form styling.Consolidating the
.form-horizontalstyles for both.groupsand.badgesinto a single rule block follows DRY principles and improves maintainability.app/assets/javascripts/discourse/templates/user-selector-autocomplete.raw.hbs (1)
5-19: LGTM!The formatting and line-wrapping adjustments improve readability without changing template behavior.
config/locales/client.en.yml (1)
1623-1633: LGTM!The new localization strings for group member management are well-structured and follow Rails i18n conventions with proper placeholder syntax (
%{username},%{group}).app/controllers/groups_controller.rb (1)
25-35: Pagination implementation looks good.The structured JSON response with
membersdata andmetacontaining pagination info (total,limit,offset) follows RESTful conventions and enables proper client-side pagination.app/assets/javascripts/discourse/models/group.js (2)
10-12: LGTM!The pagination properties (
limit,offset,user_count) are properly initialized with sensible defaults.
62-68: LGTM!The
asJSONmethod returns a clean flat object with proper boolean coercion forvisible.app/assets/javascripts/admin/templates/group_member.hbs (1)
1-1: LGTM!The template correctly renders member avatar and username, with a conditional remove link that's hidden for automatic group members. The action binding for
removeMemberis properly wired.app/assets/javascripts/admin/views/group-member.js.es6 (1)
1-4: LGTM!Clean and minimal view definition that correctly binds to the
group_membertemplate with appropriate class names.config/routes.rb (1)
45-51: LGTM!The new member management routes are correctly placed within the
groupsresource block, creating RESTful endpoints at/admin/groups/:group_id/members. The HTTP verbs (DELETE for remove, PUT for add) align with the frontend AJAX calls ingroup.js.spec/controllers/admin/groups_controller_spec.rb (1)
119-130: Good test coverage for member removal.The test correctly sets up a group with a member, then verifies removal. The use of
:deleteHTTP method aligns with RESTful conventions for theremove_memberaction.app/controllers/admin/groups_controller.rb (2)
22-32: LGTM on create action.Clean implementation with proper error handling via
render_json_error.
34-47: Good protection for automatic group renaming.The check on line 40 correctly prevents renaming automatic groups while still allowing other attribute updates.
| totalPages: function() { | ||
| if (this.get("user_count") == 0) { return 0; } | ||
| return Math.floor(this.get("user_count") / this.get("limit")) + 1; | ||
| }.property("limit", "user_count"), |
There was a problem hiding this comment.
Off-by-one error in totalPages calculation.
When user_count is an exact multiple of limit, this formula adds an extra page. For example, with user_count=10 and limit=5, the result is Math.floor(10/5) + 1 = 3, but there are only 2 pages.
Proposed fix
totalPages: function() {
if (this.get("user_count") == 0) { return 0; }
- return Math.floor(this.get("user_count") / this.get("limit")) + 1;
+ return Math.ceil(this.get("user_count") / this.get("limit"));
}.property("limit", "user_count"),🤖 Prompt for AI Agents
In @app/assets/javascripts/admin/controllers/admin-group.js.es6 around lines 11
- 14, The totalPages computed property has an off-by-one bug: replace the
current floor+1 logic in totalPages with a proper ceiling-based calculation (use
Math.ceil(this.get("user_count") / this.get("limit"))) and keep the zero-check;
also guard against a zero or falsy limit by returning 0 when this.get("limit")
is not positive. This change should be applied inside the totalPages property
implementation to correctly handle exact multiples of limit.
| removeMember: function(member) { | ||
| var self = this, | ||
| message = I18n.t("admin.groups.delete_member_confirm", { username: member.get("username"), group: this.get("name") }); | ||
| return bootbox.confirm(message, I18n.t("no_value"), I18n.t("yes_value"), function(confirm) { | ||
| if (confirm) { | ||
| self.get("model").removeMember(member); | ||
| } | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Missing error handling for removeMember operation.
The model.removeMember(member) call likely returns a promise, but errors are not caught or displayed to the user. If the server request fails, the user won't know.
Suggested improvement
removeMember: function(member) {
var self = this,
message = I18n.t("admin.groups.delete_member_confirm", { username: member.get("username"), group: this.get("name") });
return bootbox.confirm(message, I18n.t("no_value"), I18n.t("yes_value"), function(confirm) {
if (confirm) {
- self.get("model").removeMember(member);
+ self.get("model").removeMember(member).catch(function(e) {
+ var message = $.parseJSON(e.responseText).errors;
+ bootbox.alert(message);
+ });
}
});
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| removeMember: function(member) { | |
| var self = this, | |
| message = I18n.t("admin.groups.delete_member_confirm", { username: member.get("username"), group: this.get("name") }); | |
| return bootbox.confirm(message, I18n.t("no_value"), I18n.t("yes_value"), function(confirm) { | |
| if (confirm) { | |
| self.get("model").removeMember(member); | |
| } | |
| }); | |
| }, | |
| removeMember: function(member) { | |
| var self = this, | |
| message = I18n.t("admin.groups.delete_member_confirm", { username: member.get("username"), group: this.get("name") }); | |
| return bootbox.confirm(message, I18n.t("no_value"), I18n.t("yes_value"), function(confirm) { | |
| if (confirm) { | |
| self.get("model").removeMember(member).catch(function(e) { | |
| var message = $.parseJSON(e.responseText).errors; | |
| bootbox.alert(message); | |
| }); | |
| } | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In @app/assets/javascripts/admin/controllers/admin-group.js.es6 around lines 51
- 59, The removeMember method currently calls
this.get("model").removeMember(member) without handling its promise; update
removeMember to handle the returned promise from model.removeMember(member) by
attaching success and failure handlers (e.g., .then(...) and .catch(...) or
using async/await) so that on success you proceed as before and on failure you
surface an error to the user (use bootbox.alert or I18n.t-based message) and
optionally log the error; reference the removeMember function and the
model.removeMember call when making the change.
| <div> | ||
| <label for="user-selector">{{i18n 'admin.groups.add_members'}}</label> | ||
| {{user-selector usernames=usernames placeholderKey="admin.groups.selector_placeholder" id="user-selector"}} | ||
| <button {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button> |
There was a problem hiding this comment.
Add type="button" to prevent unintended form submission.
The "add members" button is inside a <form> element. Without an explicit type="button", it defaults to type="submit", which will submit the form instead of just triggering the addMembers action.
Proposed fix
- <button {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button>
+ <button type="button" {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button> | |
| <button type="button" {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button> |
🤖 Prompt for AI Agents
In @app/assets/javascripts/admin/templates/group.hbs at line 29, The button that
triggers the addMembers action is inside a form and lacks an explicit type, so
it defaults to submitting the form; update the button element that invokes the
"addMembers" action (the <button {{action "addMembers"}} ...> in the template)
to include type="button" to prevent form submission and ensure it only fires the
addMembers action.
| <div class='buttons'> | ||
| <button {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn btn-primary'>{{i18n 'admin.customize.save'}}</button> | ||
| {{#unless automatic}} | ||
| <button {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button> |
There was a problem hiding this comment.
Add type="button" to the delete button.
Same issue as the add button—without type="button", clicking delete will submit the form rather than triggering the destroy action.
Proposed fix
- <button {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>
+ <button type="button" {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button> | |
| <button type="button" {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button> |
🤖 Prompt for AI Agents
In @app/assets/javascripts/admin/templates/group.hbs at line 49, The delete
button currently lacks a type attribute so clicking it submits the surrounding
form instead of invoking the "destroy" action; update the button element that
uses {{action "destroy"}} and class 'btn btn-danger' to include type="button" so
it does not act as a submit control and continues to trigger the destroy action
as intended.
| removeMember: function(member) { | ||
| var self = this; | ||
| return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', { | ||
| type: "DELETE", | ||
| data: { user_id: member.get("id") } | ||
| }).then(function() { | ||
| // reload member list | ||
| self.findMembers(); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Promise chain doesn't wait for member reload.
The findMembers() call result is not returned, so the outer promise resolves before the member list refresh completes. Callers waiting on removeMember() may see stale data.
♻️ Suggested fix
removeMember: function(member) {
var self = this;
return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
type: "DELETE",
data: { user_id: member.get("id") }
}).then(function() {
// reload member list
- self.findMembers();
+ return self.findMembers();
});
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| removeMember: function(member) { | |
| var self = this; | |
| return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', { | |
| type: "DELETE", | |
| data: { user_id: member.get("id") } | |
| }).then(function() { | |
| // reload member list | |
| self.findMembers(); | |
| }); | |
| }, | |
| removeMember: function(member) { | |
| var self = this; | |
| return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', { | |
| type: "DELETE", | |
| data: { user_id: member.get("id") } | |
| }).then(function() { | |
| // reload member list | |
| return self.findMembers(); | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In @app/assets/javascripts/discourse/models/group.js around lines 40 - 49, The
removeMember function currently calls this.findMembers() inside the .then
handler but does not return its promise, so callers of removeMember can resolve
before the member reload completes; update the .then handler in removeMember
(the Discourse.ajax(...).then(...) block) to return self.findMembers() (i.e.,
replace the fire-and-forget call with a returned promise) so the outer promise
chain waits for the member list refresh to finish.
| addMembers: function(usernames) { | ||
| var self = this; | ||
| return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', { | ||
| type: "PUT", | ||
| data: { usernames: usernames } | ||
| }).then(function() { | ||
| // reload member list | ||
| self.findMembers(); | ||
| }) | ||
| }, |
There was a problem hiding this comment.
Same promise chain issue and missing semicolon.
Similar to removeMember, the findMembers() result should be returned to properly chain the promise. Also, there's a missing semicolon on line 59.
♻️ Suggested fix
addMembers: function(usernames) {
var self = this;
return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
type: "PUT",
data: { usernames: usernames }
}).then(function() {
// reload member list
- self.findMembers();
- })
+ return self.findMembers();
+ });
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| addMembers: function(usernames) { | |
| var self = this; | |
| return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', { | |
| type: "PUT", | |
| data: { usernames: usernames } | |
| }).then(function() { | |
| // reload member list | |
| self.findMembers(); | |
| }) | |
| }, | |
| addMembers: function(usernames) { | |
| var self = this; | |
| return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', { | |
| type: "PUT", | |
| data: { usernames: usernames } | |
| }).then(function() { | |
| // reload member list | |
| return self.findMembers(); | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In @app/assets/javascripts/discourse/models/group.js around lines 51 - 60, The
addMembers function doesn't return the promise from self.findMembers(), breaking
proper chaining, and it's missing a trailing semicolon; update addMembers (in
the group.js model) so the .then callback returns self.findMembers() and add the
missing semicolon after the .then block to fix promise chaining and syntax.
| save: function(){ | ||
| return Discourse.ajax("/admin/groups/" + this.get('id'), { type: "PUT", data: this.asJSON() }); | ||
| }, | ||
|
|
||
| destroy: function(){ | ||
| if (!this.get('id')) { return }; | ||
| return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"}); | ||
| }, |
There was a problem hiding this comment.
Missing id guard in save and syntax issue in destroy.
save()doesn't guard against missingid, which could result in a request to/admin/groups/undefined.- Line 82 has an errant semicolon after
return:{ return };— the semicolon after the closing brace serves no purpose.
♻️ Suggested fix
save: function(){
+ if (!this.get('id')) { return; }
return Discourse.ajax("/admin/groups/" + this.get('id'), { type: "PUT", data: this.asJSON() });
},
destroy: function(){
- if (!this.get('id')) { return };
+ if (!this.get('id')) { return; }
return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"});
},🤖 Prompt for AI Agents
In @app/assets/javascripts/discourse/models/group.js around lines 77 - 84, The
save method lacks an id guard and the destroy method contains an extraneous
semicolon; update save (function save) to return early if this.get('id') is
falsy (mirroring destroy's guard) to avoid calling /admin/groups/undefined, and
remove the stray semicolon after the return in destroy (change "if
(!this.get('id')) { return };" to "if (!this.get('id')) { return }") so the
syntax is clean.
| if search = params[:search] | ||
| search = search.to_s | ||
| groups = groups.where("name ilike ?", "%#{search}%") | ||
| groups = groups.where("name ILIKE ?", "%#{search}%") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "sanitize_sql_like" --type rubyRepository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 67
🏁 Script executed:
# Check the controller file to see the actual code context
cat -n app/controllers/admin/groups_controller.rb | head -30Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 965
🏁 Script executed:
# Check if there's a Gemfile to understand Rails version
cat Gemfile 2>/dev/null | grep -i rails || echo "Gemfile not found in root"Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 1004
🏁 Script executed:
# Search for any ILIKE usage patterns in the codebase
rg -n "ILIKE" --type ruby -B 2 -A 2Repository: AI-Code-Review-Evals/coderabbit-discourse
Length of output: 2965
🌐 Web query:
sanitize_sql_like Rails helper method
💡 Result:
sanitize_sql_like is an ActiveRecord helper that escapes characters that are special in SQL LIKE patterns so a user-provided string can be safely used inside a LIKE/ILIKE pattern.
- Location/signature: ActiveRecord::Sanitization::ClassMethods#sanitize_sql_like(string, escape_character = "\").
Escape LIKE wildcard characters in search pattern.
Special characters in search (%, _, ) are not escaped and will be interpreted as LIKE wildcards, causing unexpected matches. A search for % would match all groups, and _ would match any single character.
Proposed fix
- groups = groups.where("name ILIKE ?", "%#{search}%")
+ groups = groups.where("name ILIKE ?", "%#{sanitize_sql_like(search)}%")🤖 Prompt for AI Agents
In @app/controllers/admin/groups_controller.rb at line 8, The WHERE clause on
groups uses the raw search string as "%#{search}%" which treats %, _, and \ as
SQL LIKE wildcards; fix by escaping those characters using ActiveRecord's
sanitize_sql_like (e.g., call ActiveRecord::Base.sanitize_sql_like or
model.sanitize_sql_like on the search) and then interpolate the escaped value
into the ILIKE pattern used in the groups = groups.where("name ILIKE ?",
"%#{escaped}%") expression so searches match literally.
| limit = (params[:limit] || 50).to_i | ||
| offset = params[:offset].to_i |
There was a problem hiding this comment.
Consider capping the limit parameter to prevent abuse.
The limit parameter is user-controlled with no upper bound. A client could request an arbitrarily large limit (e.g., limit=1000000), potentially causing performance degradation or memory issues.
🔧 Proposed fix
- limit = (params[:limit] || 50).to_i
- offset = params[:offset].to_i
+ limit = [(params[:limit] || 50).to_i, 200].min
+ offset = [params[:offset].to_i, 0].max📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| limit = (params[:limit] || 50).to_i | |
| offset = params[:offset].to_i | |
| limit = [(params[:limit] || 50).to_i, 200].min | |
| offset = [params[:offset].to_i, 0].max |
🤖 Prompt for AI Agents
In @app/controllers/groups_controller.rb around lines 22 - 23, The
user-controlled limit has no upper bound; clamp it to a safe maximum by defining
a MAX_LIMIT constant (e.g., 1000) and replacing the current assignment of limit
to coerce and clamp params[:limit] into the allowed range (ensure it remains an
integer >= 1 and <= MAX_LIMIT). Update the line that assigns limit (currently
using params[:limit] and .to_i) to use the clamp logic (or equivalent min/max
checks) so large requests are rejected/truncated; leave offset handling
unchanged.
2 participants
Test 8
Summary by CodeRabbit
Release Notes
New Features
Style
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#8