Automated Test: group-dm-user-addition-post #328
Conversation
📝 WalkthroughWalkthroughThis PR refactors group member management by shifting pagination logic from controllers to the Discourse.Group model, introducing new admin UI components for paginated member listing, and adding dedicated API endpoints for member operations (add/remove). The backend consolidates CRUD actions and the frontend integrates member management controls into a cohesive form. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant UI as Admin UI
participant Model as Group Model
participant Server as Admin API
Admin->>UI: Click "Next" pagination button
UI->>Model: update offset, trigger next action
Model->>Server: findMembers(limit, offset)
Server->>Model: return paginated members + metadata
Model->>UI: update members, currentPage, totalPages
UI->>Admin: render updated member list
Admin->>UI: Enter usernames & click "Add Members"
UI->>Model: addMembers(usernames)
Model->>Server: PUT /admin/groups/{id}/members
Server->>Model: members added successfully
Model->>Server: findMembers (reload)
Server->>Model: return updated member list
Model->>UI: update members display
UI->>Admin: show updated group with new members
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/assets/javascripts/discourse/templates/group/members.hbs (1)
18-19:⚠️ Potential issue | 🟡 MinorStray
</div>closing tag without a matching opening<div>.Line 19 has a
</div>that doesn't correspond to any opening<div>. This appears pre-existing but will produce invalid HTML inside the<table>. Consider removing it.Proposed fix
</tr> - </div> {{/each}}
🤖 Fix all issues with AI agents
In `@app/assets/javascripts/admin/controllers/admin-group.js.es6`:
- Around line 11-14: The totalPages computed property
(totalPages.property("limit", "user_count")) incorrectly uses
Math.floor(this.get("user_count") / this.get("limit")) + 1 causing an extra page
when user_count is an exact multiple of limit; change the computation to use
Math.ceil(this.get("user_count") / this.get("limit")) and keep the zero-check
for user_count so totalPages returns 0 when user_count is 0.
In `@app/assets/javascripts/admin/templates/group.hbs`:
- Line 29: The Add button in the group.hbs template currently defaults to
type="submit" and will submit surrounding forms; update the <button> with the
action "addMembers" to explicitly include type="button" so clicking it won't
trigger form submission, leaving the class 'btn add', fa-icon "plus", and i18n
'admin.groups.add' unchanged.
- Around line 46-51: The two button elements that invoke the Ember actions
"save" and "destroy" default to type="submit" and will trigger native form
submission; update both buttons (the one with class 'btn btn-primary' invoking
the "save" action and the conditional 'btn btn-danger' invoking the "destroy"
action) to include type="button" so clicking them only runs the Ember actions
and prevents page reload/navigation.
In `@app/assets/javascripts/discourse/models/group.js`:
- Around line 40-60: Both removeMember and addMembers call self.findMembers()
inside their .then() callbacks but fail to return that promise, so callers
resolve before the member list finishes reloading; update the .then() handlers
in removeMember and addMembers to return self.findMembers() (i.e., change the
callback to return the promise from findMembers) so the promise chain waits for
the member-list refresh.
In `@app/assets/javascripts/discourse/routes/group-members.js.es6`:
- Around line 8-12: setupController currently calls model.findMembers() but does
not handle its returned promise; attach error handling by either returning the
promise from setupController (so the router can handle rejections) or chaining a
.catch(...) on model.findMembers() to handle/log failures. Specifically, in
setupController (which sets group showing and controller.model), change the call
to model.findMembers() to either return model.findMembers() or to call
model.findMembers().catch(error => { /* handle or log e.g.,
controller.set('loadError', error) or this.onError(error) */ }) so rejections
are not left unhandled.
In `@app/controllers/admin/groups_controller.rb`:
- Around line 6-9: The LIKE wildcards in the user-provided search string are not
escaped, so replace the raw interpolation with an escaped version using
ActiveRecord's sanitize_sql_like; e.g. convert params[:search] to a string, call
ActiveRecord::Base.sanitize_sql_like(search) (or sanitize_sql_like(search) if
available) to escape %/_ then use groups.where("name ILIKE ?", "%#{escaped}%")
so the search treats literal % and _ correctly.
In `@app/controllers/groups_controller.rb`:
- Around line 22-26: limit is unbounded and offset can be negative; clamp both
before querying: coerce params[:limit] to an integer, cap it to a safe maximum
(e.g., MAX_LIMIT constant or 100) and ensure it is at least 1, and coerce
params[:offset] to an integer and clamp it to a minimum of 0; then use these
sanitized values in the
group.users.order(:username_lower).limit(limit).offset(offset) query and keep
total = group.users.count unchanged.
In `@spec/controllers/admin/groups_controller_spec.rb`:
- Around line 112-130: The test for the remove_member action is using the wrong
HTTP verb (xhr :put) so it hits add_members; change the test that calls
remove_member to use DELETE (xhr :delete) so it exercises
GroupsController#remove_member and matches the route for "members" =>
"groups#remove_member"; update the spec example titled "cannot remove members
from automatic groups" to call xhr :delete, :remove_member with the same params.
🧹 Nitpick comments (8)
app/assets/stylesheets/common/admin/admin_base.scss (1)
471-476: Hardcoded colors break theming/dark mode support.
#333and#aaawon't adapt to dark themes — text could become invisible against a dark background. Every other color reference in this file uses SCSS variables ($primary,$secondary, etc.) or helper functions. These should follow the same pattern.♻️ Suggested fix using theme-aware variables
.next, .previous { - color: `#333` !important; + color: $primary !important; &.disabled { - color: `#aaa` !important; + color: scale-color($primary, $lightness: 50%) !important; } }app/assets/javascripts/discourse/models/group.js (2)
81-84: Extraneous semicolon afterifblock.Line 82:
if (!this.get('id')) { return };— the trailing semicolon after}is an empty statement. It's harmless but syntactically unusual. Also,returnwithout a value inside a function that otherwise returns a promise means callers ofdestroy()may getundefinedinstead of a promise whenidis falsy.Proposed cleanup
destroy: function(){ - if (!this.get('id')) { return }; + if (!this.get('id')) { return; } return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"}); },
20-38: Offset clamping allowsoffset === user_count, which yields empty results.Line 23 clamps offset with
Math.min(user_count, ...). When offset equalsuser_count(e.g., navigating past the last page), the API request will start at the boundary and return zero members. If this is intentional (the UI prevents it), this is fine — just noting the edge case.spec/controllers/admin/groups_controller_spec.rb (1)
13-32: RuboCop: Extra empty lines at block body boundaries throughout the file.Static analysis flagged numerous
Layout/EmptyLinesAroundBlockBodyviolations (extra blank lines at the beginning/end of context blocks). Consider removing the extra blank lines for consistency if the project enforces this rule.Also applies to: 34-47, 49-60, 62-78, 80-89, 91-110
app/controllers/admin/groups_controller.rb (2)
65-82: Silent skip when usernames don't exist — consider returning feedback.When a username in the comma-separated list doesn't match any
User, it is silently ignored (line 72-74). The caller receives a success response with no indication of which users were not found. This could confuse admins who mistype a username.Consider collecting unmatched usernames and returning them in the response, or at minimum logging them.
71-75: RuboCop: Assignment in condition — wrap in parentheses to signal intent.Line 72 uses
if user = User.find_by_username(username)which RuboCop flags. Wrapping the assignment in parentheses makes intent explicit.Proposed fix
- if user = User.find_by_username(username) + if (user = User.find_by_username(username))app/assets/javascripts/admin/controllers/admin-group.js.es6 (2)
6-9:currentPageformula is correct but uses loose equality.The
== 0check at line 7 uses loose equality, which also matchesnull,undefined, and"". While this is a safe fallback here (returns 0 for unloaded data), prefer=== 0for explicitness if only the numeric zero case is intended.
61-65: TODO: Input should be cleared after adding members.The TODO at line 62 notes that the
usernamesinput isn't cleared after a successful add. Consider resetting it in the.then()callback ofaddMembersto prevent double-submission.Do you want me to generate the fix to clear the input after a successful add?
| 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.
Bug: totalPages formula is off-by-one for exact multiples.
Math.floor(user_count / limit) + 1 returns one extra page when user_count is evenly divisible by limit. For example, 100 users with limit 50 → Math.floor(2) + 1 = 3 pages instead of the correct 2.
Use Math.ceil instead:
🐛 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 (totalPages.property("limit",
"user_count")) incorrectly uses Math.floor(this.get("user_count") /
this.get("limit")) + 1 causing an extra page when user_count is an exact
multiple of limit; change the computation to use
Math.ceil(this.get("user_count") / this.get("limit")) and keep the zero-check
for user_count so totalPages returns 0 when user_count is 0.
| <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 button also needs type="button" to avoid form submission.
Same issue as the Save/Delete buttons — this <button> defaults to type="submit".
🐛 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 Add button
in the group.hbs template currently defaults to type="submit" and will submit
surrounding forms; update the <button> with the action "addMembers" to
explicitly include type="button" so clicking it won't trigger form submission,
leaving the class 'btn add', fa-icon "plus", and i18n 'admin.groups.add'
unchanged.
| <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> | ||
| {{/unless}} | ||
| </div> |
There was a problem hiding this comment.
Buttons inside <form> will trigger native form submission.
The <button> elements at lines 47 and 49 default to type="submit" since no type attribute is specified. Clicking them will trigger the browser's native form submission (page navigation/reload) in addition to the Ember {{action}}. This will likely cause the page to reload before the Ember action completes.
Add type="button" to both buttons to prevent this.
🐛 Proposed fix
- <button {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn btn-primary'>{{i18n 'admin.customize.save'}}</button>
+ <button type="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>
+ <button type="button" {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>
{{/unless}}📝 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.
| <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> | |
| {{/unless}} | |
| </div> | |
| <div class='buttons'> | |
| <button type="button" {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn btn-primary'>{{i18n 'admin.customize.save'}}</button> | |
| {{`#unless` automatic}} | |
| <button type="button" {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button> | |
| {{/unless}} | |
| </div> |
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/templates/group.hbs` around lines 46 - 51, The
two button elements that invoke the Ember actions "save" and "destroy" default
to type="submit" and will trigger native form submission; update both buttons
(the one with class 'btn btn-primary' invoking the "save" action and the
conditional 'btn btn-danger' invoking the "destroy" action) to include
type="button" so clicking them only runs the Ember actions and prevents page
reload/navigation.
| 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(); | ||
| }); | ||
| }, | ||
|
|
||
| asJSON: function() { | ||
| return { group: { | ||
| name: this.get('name'), | ||
| alias_level: this.get('alias_level'), | ||
| visible: !!this.get('visible'), | ||
| usernames: this.get('usernames') } }; | ||
| 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.
findMembers() promise not returned from .then() — callers can't await the member-list refresh.
In both removeMember and addMembers, the .then() callback calls self.findMembers() but doesn't return it. This means the promise returned to callers resolves before the member list has actually reloaded, leading to potential UI timing issues.
Proposed 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();
});
},
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.
| 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(); | |
| }); | |
| }, | |
| asJSON: function() { | |
| return { group: { | |
| name: this.get('name'), | |
| alias_level: this.get('alias_level'), | |
| visible: !!this.get('visible'), | |
| usernames: this.get('usernames') } }; | |
| 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(); | |
| }) | |
| }, | |
| 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(); | |
| }); | |
| }, | |
| 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 40 - 60, Both
removeMember and addMembers call self.findMembers() inside their .then()
callbacks but fail to return that promise, so callers resolve before the member
list finishes reloading; update the .then() handlers in removeMember and
addMembers to return self.findMembers() (i.e., change the callback to return the
promise from findMembers) so the promise chain waits for the member-list
refresh.
| setupController: function(controller, model) { | ||
| this.controllerFor('group').set('showing', 'members'); | ||
| controller.set("model", model); | ||
| model.findMembers(); | ||
| } |
There was a problem hiding this comment.
Unhandled promise from findMembers().
model.findMembers() returns a promise whose rejection is never caught. If the AJAX call fails (network error, 4xx/5xx), this will produce an unhandled promise rejection. Consider adding a .catch() or returning the promise so the router's error handling can intercept it.
Proposed fix
setupController: function(controller, model) {
this.controllerFor('group').set('showing', 'members');
controller.set("model", model);
- model.findMembers();
+ model.findMembers().catch(function() {
+ // optionally surface an error to the user
+ });
}📝 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.
| setupController: function(controller, model) { | |
| this.controllerFor('group').set('showing', 'members'); | |
| controller.set("model", model); | |
| model.findMembers(); | |
| } | |
| setupController: function(controller, model) { | |
| this.controllerFor('group').set('showing', 'members'); | |
| controller.set("model", model); | |
| model.findMembers().catch(function() { | |
| // optionally surface an error to the user | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/routes/group-members.js.es6` around lines 8
- 12, setupController currently calls model.findMembers() but does not handle
its returned promise; attach error handling by either returning the promise from
setupController (so the router can handle rejections) or chaining a .catch(...)
on model.findMembers() to handle/log failures. Specifically, in setupController
(which sets group showing and controller.model), change the call to
model.findMembers() to either return model.findMembers() or to call
model.findMembers().catch(error => { /* handle or log e.g.,
controller.set('loadError', error) or this.onError(error) */ }) so rejections
are not left unhandled.
| if search = params[:search] | ||
| search = search.to_s | ||
| groups = groups.where("name ilike ?", "%#{search}%") | ||
| groups = groups.where("name ILIKE ?", "%#{search}%") | ||
| end |
There was a problem hiding this comment.
LIKE wildcards (%, _) in search are not escaped.
A search term like % or _ will act as a LIKE wildcard, matching more broadly than expected. This is admin-only so the risk is low, but it's a correctness gap.
🛡️ Proposed fix — sanitize LIKE wildcards
if search = params[:search]
search = search.to_s
- groups = groups.where("name ILIKE ?", "%#{search}%")
+ groups = groups.where("name ILIKE ?", "%#{ActiveRecord::Base.sanitize_sql_like(search)}%")
end📝 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.
| if search = params[:search] | |
| search = search.to_s | |
| groups = groups.where("name ilike ?", "%#{search}%") | |
| groups = groups.where("name ILIKE ?", "%#{search}%") | |
| end | |
| if search = params[:search] | |
| search = search.to_s | |
| groups = groups.where("name ILIKE ?", "%#{ActiveRecord::Base.sanitize_sql_like(search)}%") | |
| end |
🧰 Tools
🪛 RuboCop (1.84.0)
[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)
🤖 Prompt for AI Agents
In `@app/controllers/admin/groups_controller.rb` around lines 6 - 9, The LIKE
wildcards in the user-provided search string are not escaped, so replace the raw
interpolation with an escaped version using ActiveRecord's sanitize_sql_like;
e.g. convert params[:search] to a string, call
ActiveRecord::Base.sanitize_sql_like(search) (or sanitize_sql_like(search) if
available) to escape %/_ then use groups.where("name ILIKE ?", "%#{escaped}%")
so the search treats literal % and _ correctly.
| limit = (params[:limit] || 50).to_i | ||
| offset = params[:offset].to_i | ||
|
|
||
| total = group.users.count | ||
| members = group.users.order(:username_lower).limit(limit).offset(offset) |
There was a problem hiding this comment.
Cap limit and clamp offset to prevent abuse and negative values.
limit is unbounded — a client can pass limit=1000000 and dump the entire user list in one request. Also, a negative offset (e.g., offset=-5) passes .to_i and reaches ActiveRecord, which may produce unexpected behavior depending on the database adapter.
🛡️ 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 | |
| total = group.users.count | |
| members = group.users.order(:username_lower).limit(limit).offset(offset) | |
| limit = [(params[:limit] || 50).to_i, 200].min | |
| offset = [params[:offset].to_i, 0].max | |
| total = group.users.count | |
| members = group.users.order(:username_lower).limit(limit).offset(offset) |
🤖 Prompt for AI Agents
In `@app/controllers/groups_controller.rb` around lines 22 - 26, limit is
unbounded and offset can be negative; clamp both before querying: coerce
params[:limit] to an integer, cap it to a safe maximum (e.g., MAX_LIMIT constant
or 100) and ensure it is at least 1, and coerce params[:offset] to an integer
and clamp it to a minimum of 0; then use these sanitized values in the
group.users.order(:username_lower).limit(limit).offset(offset) query and keep
total = group.users.count unchanged.
| context ".remove_member" do | ||
|
|
||
| it "cannot remove members from automatic groups" do | ||
| xhr :put, :remove_member, group_id: 1, user_id: 42 | ||
| response.status.should == 422 | ||
| end | ||
|
|
||
| it "is able to remove a member" do | ||
| group = Fabricate(:group) | ||
| user = Fabricate(:user) | ||
| group.add(user) | ||
| group.save | ||
|
|
||
| xhr :delete, :remove_member, group_id: group.id, user_id: user.id | ||
|
|
||
| response.should be_success | ||
| group.reload | ||
| group.users.count.should == 0 | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find routes configuration
fd "routes\.rb" --type f | head -20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 104
🏁 Script executed:
# Look for remove_member in routes
rg "remove_member" --type rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 114
🏁 Script executed:
# Check the admin groups controller for remove_member action
fd "groups_controller\.rb" --type f | grep -E "admin|app/controllers" | head -5Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 167
🏁 Script executed:
# Check the routes configuration for remove_member
rg "remove_member" config/routes.rb -A 2 -B 2Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 275
🏁 Script executed:
# Check the admin groups controller for remove_member
cat app/controllers/admin/groups_controller.rb | head -200Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 2348
🏁 Script executed:
# Get file size first to determine how to read it
wc -l app/controllers/admin/groups_controller.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 134
Inconsistent HTTP method for remove_member test.
Line 115 uses xhr :put, :remove_member but the route configuration shows delete "members" => "groups#remove_member". The DELETE method is required; the PUT request will hit the wrong route (add_members). Update line 115 to use DELETE:
Fix
it "cannot remove members from automatic groups" do
- xhr :put, :remove_member, group_id: 1, user_id: 42
+ xhr :delete, :remove_member, group_id: 1, user_id: 42
response.status.should == 422
end📝 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.
| context ".remove_member" do | |
| it "cannot remove members from automatic groups" do | |
| xhr :put, :remove_member, group_id: 1, user_id: 42 | |
| response.status.should == 422 | |
| end | |
| it "is able to remove a member" do | |
| group = Fabricate(:group) | |
| user = Fabricate(:user) | |
| group.add(user) | |
| group.save | |
| xhr :delete, :remove_member, group_id: group.id, user_id: user.id | |
| response.should be_success | |
| group.reload | |
| group.users.count.should == 0 | |
| end | |
| context ".remove_member" do | |
| it "cannot remove members from automatic groups" do | |
| xhr :delete, :remove_member, group_id: 1, user_id: 42 | |
| response.status.should == 422 | |
| end | |
| it "is able to remove a member" do | |
| group = Fabricate(:group) | |
| user = Fabricate(:user) | |
| group.add(user) | |
| group.save | |
| xhr :delete, :remove_member, group_id: group.id, user_id: user.id | |
| response.should be_success | |
| group.reload | |
| group.users.count.should == 0 | |
| end |
🧰 Tools
🪛 RuboCop (1.84.0)
[convention] 113-114: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
🤖 Prompt for AI Agents
In `@spec/controllers/admin/groups_controller_spec.rb` around lines 112 - 130, The
test for the remove_member action is using the wrong HTTP verb (xhr :put) so it
hits add_members; change the test that calls remove_member to use DELETE (xhr
:delete) so it exercises GroupsController#remove_member and matches the route
for "members" => "groups#remove_member"; update the spec example titled "cannot
remove members from automatic groups" to call xhr :delete, :remove_member with
the same params.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
New Features
Style