Automated Test: group-dm-user-addition-post #320
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the group management system to support explicit member management via pagination. It introduces dedicated API endpoints for adding and removing members, replaces username-based operations with direct member CRUD actions, and updates the administrative UI with a paginated member list and actions. Changes
Sequence DiagramsequenceDiagram
participant User as Admin User
participant Client as Browser/Client
participant AdminCtrl as Admin<br/>Controller
participant GroupModel as Group Model
participant GroupCtrl as Groups<br/>Controller
participant API as API Endpoint
rect rgba(100, 150, 200, 0.5)
Note over User,API: Member Management Flow
User->>Client: Click "Next" or "Add Members"
Client->>AdminCtrl: Trigger pagination/add action
AdminCtrl->>GroupModel: findMembers() or addMembers(usernames)
GroupModel->>API: GET/PUT /groups/:name/members.json
API->>GroupCtrl: Process pagination or member addition
GroupCtrl->>API: Query/update members with limit, offset
API-->>GroupModel: Return members + pagination metadata
GroupModel->>AdminCtrl: Update controller state (user_count, offset, limit)
AdminCtrl->>Client: Re-render member list with pagination
Client->>User: Display paginated members or new additions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 6
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)
6-19:⚠️ Potential issue | 🟡 MinorRemove the stray closing
</div>inside the table row.There’s no matching opening
<div>in this block, which yields invalid HTML and can disrupt table layout.🧹 Proposed fix
- </div>
🤖 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) incorrectly
adds 1 after Math.floor, causing an extra page when user_count is an exact
multiple of limit; change the calculation to compute ceil(user_count / limit) or
use Math.floor((user_count + limit - 1) / limit) so 20 users with limit 10
yields 2 pages; update the totalPages function (property("limit","user_count"))
to return 0 when user_count is 0 and otherwise the correct ceiling division so
downstream logic (e.g., showingLast) no longer sees an extra empty page.
- Around line 29-38: The next method allows offset to equal user_count when
user_count % limit === 0; change the offset clamp logic in the next function so
it sets offset to the last valid page start instead of user_count: compute a
maxOffset based on group.get("user_count") and group.get("limit") (e.g. last
page start = (Math.ceil(user_count / limit) - 1) * limit or
floor((user_count-1)/limit)*limit), then use Math.min(group.get("offset") +
group.get("limit"), maxOffset) before group.set("offset") and call
group.findMembers().
In `@app/assets/javascripts/admin/templates/group_member.hbs`:
- Line 1: Replace the anchor used for the removal action with a button to match
project convention and restore keyboard accessibility: in the template where
{{avatar member imageSize="small"}} {{member.username}} and the conditional
{{`#unless` automatic}} is rendered, change the <a class='remove' {{action
"removeMember" member}}> element to a <button type="button" class="remove"
{{action "removeMember" member}}> (keeping the {{fa-icon "times"}} inside) so
the removeMember action is invoked from an accessible, focusable control.
In `@app/assets/javascripts/discourse/models/group.js`:
- Around line 81-84: The destroy method contains an extraneous semicolon after
the if-block closing brace which creates a stray empty statement; in the destroy
function (method name: destroy) remove the semicolon immediately following the
if (!this.get('id')) { return } so the if statement is properly terminated and
the subsequent return Discourse.ajax(...) executes as intended.
In `@app/controllers/groups_controller.rb`:
- Around line 22-35: Clamp the pagination params before querying: replace the
current direct uses of params in computing limit and offset so limit is bounded
to a maximum (e.g., [(params[:limit] || 60).to_i, 100].min) and offset is
non-negative (e.g., [params[:offset].to_i, 0].max); then use these clamped limit
and offset values in the call to
group.users.order(:username_lower).limit(limit).offset(offset) so
serialize_data(members, GroupUserSerializer) gets a safe, bounded result.
In `@spec/controllers/admin/groups_controller_spec.rb`:
- Around line 114-117: The spec is calling the wrong HTTP verb for the
remove_member action; update the test in groups_controller_spec.rb to use the
DELETE verb instead of PUT (change the xhr :put, :remove_member call to xhr
:delete, :remove_member with the same group_id and user_id) so it matches the
controller's delete behavior and still asserts response.status == 422.
🧹 Nitpick comments (7)
spec/controllers/admin/groups_controller_spec.rb (2)
52-58: Hardcoded ID=1 assumes a specific automatic group exists.This test relies on
Group.find(1)returning an automatic group. Consider usingFabricate(:group, automatic: true)for test isolation, similar to the.destroycontext.♻️ Suggested improvement
it "ignore name change on automatic group" do - xhr :put, :update, id: 1, name: "WAT", visible: "true" + group = Fabricate(:group, automatic: true) + original_name = group.name + xhr :put, :update, id: group.id, name: "WAT", visible: "true" response.should be_success - group = Group.find(1) + group.reload - group.name.should_not == "WAT" + group.name.should == original_name group.visible.should == true end
93-96: Same hardcoded ID concern for automatic group test.Consider fabricating an automatic group instead of relying on ID=1.
app/controllers/admin/groups_controller.rb (2)
71-75: Silent failure when username is not found.Invalid usernames are silently ignored. Consider collecting failures and returning feedback to the admin so they know which usernames were not added.
♻️ Suggested improvement with error feedback
+ not_found = [] usernames.split(",").each do |username| if user = User.find_by_username(username) group.add(user) + else + not_found << username end end if group.save - render json: success_json + render json: success_json.merge(not_found: not_found) else
72-72: Wrap assignment in parentheses for clarity.RuboCop flags the assignment-in-condition. While this is a valid Ruby pattern, wrapping in parentheses makes the intent explicit.
♻️ Proposed fix
- if user = User.find_by_username(username) + if (user = User.find_by_username(username))app/assets/javascripts/discourse/models/group.js (2)
59-60: Missing semicolon afteraddMembersfunction.The closing of the
then()callback is missing a semicolon for consistency with the rest of the codebase.♻️ Proposed fix
}).then(function() { // reload member list self.findMembers(); - }) + }); },
40-60: Consider propagating errors from member operations.Both
removeMemberandaddMemberscallfindMembers()in the success callback but don't handle or propagate errors from the AJAX call. The caller has no way to know if the operation failed.♻️ Example with error propagation
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() { self.findMembers(); }).catch(function(error) { // Allow caller to handle errors throw error; }); },app/assets/javascripts/admin/controllers/admin-group.js.es6 (1)
61-65: Consider clearing the usernames input after a successful add.
This avoids accidental duplicate submissions and matches the TODO.If `addMembers` is not promise-based, I can adapt this to the correct callback pattern—just point me to the model API.🧹 Possible follow-up
- this.get("model").addMembers(this.get("usernames")); + var self = this; + this.get("model").addMembers(this.get("usernames")).then(function() { + self.set("usernames", null); + });
| 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.
Fix totalPages off-by-one when user_count is a multiple of limit.
With Math.floor(...)+1, 20 users at limit 10 yields 3 pages (should be 2). This produces an extra empty page and incorrect showingLast.
🐛 Proposed fix
- return Math.floor(this.get("user_count") / this.get("limit")) + 1;
+ return Math.ceil(this.get("user_count") / this.get("limit"));📝 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.
| 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"), | |
| totalPages: function() { | |
| if (this.get("user_count") == 0) { return 0; } | |
| 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) incorrectly adds 1 after
Math.floor, causing an extra page when user_count is an exact multiple of limit;
change the calculation to compute ceil(user_count / limit) or use
Math.floor((user_count + limit - 1) / limit) so 20 users with limit 10 yields 2
pages; update the totalPages function (property("limit","user_count")) to return
0 when user_count is 0 and otherwise the correct ceiling division so downstream
logic (e.g., showingLast) no longer sees an extra empty page.
| next: function() { | ||
| if (this.get("showingLast")) { return; } | ||
|
|
||
| var group = this.get("model"), | ||
| offset = Math.min(group.get("offset") + group.get("limit"), group.get("user_count")); | ||
|
|
||
| group.set("offset", offset); | ||
|
|
||
| return group.findMembers(); | ||
| }, |
There was a problem hiding this comment.
Clamp offset to the last valid page, not to user_count.
When user_count is exactly divisible by limit, this allows offset == user_count, yielding an empty page.
🐛 Proposed fix
- var group = this.get("model"),
- offset = Math.min(group.get("offset") + group.get("limit"), group.get("user_count"));
+ var group = this.get("model"),
+ maxOffset = Math.max(group.get("user_count") - group.get("limit"), 0),
+ offset = Math.min(group.get("offset") + group.get("limit"), maxOffset);📝 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.
| next: function() { | |
| if (this.get("showingLast")) { return; } | |
| var group = this.get("model"), | |
| offset = Math.min(group.get("offset") + group.get("limit"), group.get("user_count")); | |
| group.set("offset", offset); | |
| return group.findMembers(); | |
| }, | |
| next: function() { | |
| if (this.get("showingLast")) { return; } | |
| var group = this.get("model"), | |
| maxOffset = Math.max(group.get("user_count") - group.get("limit"), 0), | |
| offset = Math.min(group.get("offset") + group.get("limit"), maxOffset); | |
| group.set("offset", offset); | |
| return group.findMembers(); | |
| }, |
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/controllers/admin-group.js.es6` around lines 29
- 38, The next method allows offset to equal user_count when user_count % limit
=== 0; change the offset clamp logic in the next function so it sets offset to
the last valid page start instead of user_count: compute a maxOffset based on
group.get("user_count") and group.get("limit") (e.g. last page start =
(Math.ceil(user_count / limit) - 1) * limit or
floor((user_count-1)/limit)*limit), then use Math.min(group.get("offset") +
group.get("limit"), maxOffset) before group.set("offset") and call
group.findMembers().
| @@ -0,0 +1 @@ | |||
| {{avatar member imageSize="small"}} {{member.username}} {{#unless automatic}}<a class='remove' {{action "removeMember" member}}>{{fa-icon "times"}}</a>{{/unless}} | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and view its content
find . -name "group_member.hbs" -type fRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 145
🏁 Script executed:
# Search for similar .remove or remove action patterns in templates
find . -name "*.hbs" -type f -path "*/templates/*" | head -20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1205
🏁 Script executed:
# Check for CSS styling for .remove class
rg "\.remove\s*\{" --type css --type scssRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 116
🏁 Script executed:
# Search for button or <a> patterns in other handlebars templates to understand project conventions
rg "class=['\"]remove['\"]" --type hbsRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 306
🏁 Script executed:
# Read the actual file content
cat -n ./app/assets/javascripts/admin/templates/group_member.hbsRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 257
🏁 Script executed:
# Search for .remove styling in CSS files
find . -name "*.css" -type f | xargs rg "\.remove" 2>/dev/null | head -20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 70
🏁 Script executed:
# Search for how buttons are implemented in other admin templates
rg "<button" app/assets/javascripts/admin/templates/ -A 2 | head -40Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 3806
🏁 Script executed:
# Search for how other interactive actions are implemented in templates
rg "action.*remove" app/assets/javascripts/admin/templates/ -B 2 -A 2Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 306
🏁 Script executed:
# Check how fa-icon is used in other templates to understand the helper
rg "fa-icon" app/assets/javascripts/admin/templates/ | head -10Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 2141
🏁 Script executed:
# Look for similar delete/remove patterns with <a> in other templates
rg "<a[^>]*remove" app/assets/javascripts/admin/templates/ -A 1Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 306
🏁 Script executed:
# Check if there are accessibility attributes (aria-label, role) on other <a> tags without href
rg "<a[^>]*(aria-label|role)[^>]*>" app/assets/javascripts/admin/templates/ | head -10Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 70
Use a <button> instead of <a> for the remove action.
An <a> without href is not keyboard-focusable. The codebase consistently uses <button> for interactive actions—change this to match the project convention and improve accessibility.
♿ Proposed fix
-{{avatar member imageSize="small"}} {{member.username}} {{`#unless` automatic}}<a class='remove' {{action "removeMember" member}}>{{fa-icon "times"}}</a>{{/unless}}
+{{avatar member imageSize="small"}} {{member.username}}
+{{`#unless` automatic}}
+ <button type="button"
+ class="remove"
+ {{action "removeMember" member}}
+ aria-label="{{i18n 'admin_js.admin.groups.delete'}}"
+ title="{{i18n 'admin_js.admin.groups.delete'}}">
+ {{fa-icon "times"}}
+ </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.
| {{avatar member imageSize="small"}} {{member.username}} {{#unless automatic}}<a class='remove' {{action "removeMember" member}}>{{fa-icon "times"}}</a>{{/unless}} | |
| {{avatar member imageSize="small"}} {{member.username}} | |
| {{`#unless` automatic}} | |
| <button type="button" | |
| class="remove" | |
| {{action "removeMember" member}} | |
| aria-label="{{i18n 'admin_js.admin.groups.delete'}}" | |
| title="{{i18n 'admin_js.admin.groups.delete'}}"> | |
| {{fa-icon "times"}} | |
| </button> | |
| {{/unless}} |
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/templates/group_member.hbs` at line 1, Replace
the anchor used for the removal action with a button to match project convention
and restore keyboard accessibility: in the template where {{avatar member
imageSize="small"}} {{member.username}} and the conditional {{`#unless`
automatic}} is rendered, change the <a class='remove' {{action "removeMember"
member}}> element to a <button type="button" class="remove" {{action
"removeMember" member}}> (keeping the {{fa-icon "times"}} inside) so the
removeMember action is invoked from an accessible, focusable control.
| destroy: function(){ | ||
| if (!this.get('id')) { return }; | ||
| return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"}); | ||
| }, |
There was a problem hiding this comment.
Syntax error: extraneous semicolon after closing brace.
Line 82 has a semicolon immediately after the closing brace of the if statement, which is a syntax error or no-op empty statement.
🐛 Proposed fix
destroy: function(){
- if (!this.get('id')) { return };
+ if (!this.get('id')) { return; }
return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"});
},📝 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.
| destroy: function(){ | |
| if (!this.get('id')) { return }; | |
| return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"}); | |
| }, | |
| destroy: function(){ | |
| 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 81 - 84, The
destroy method contains an extraneous semicolon after the if-block closing brace
which creates a stray empty statement; in the destroy function (method name:
destroy) remove the semicolon immediately following the if (!this.get('id')) {
return } so the if statement is properly terminated and the subsequent return
Discourse.ajax(...) executes as intended.
| 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) | ||
|
|
||
| render json: { | ||
| members: serialize_data(members, GroupUserSerializer), | ||
| meta: { | ||
| total: total, | ||
| limit: limit, | ||
| offset: offset | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app/controllers/groups_controller.rb | head -60Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1525
🏁 Script executed:
# Check for strong_params in the file
rg "permit|strong_params|parameters" app/controllers/groups_controller.rb
# Check ApplicationController and parent classes
find app -name "application_controller.rb" -o -name "*_controller.rb" | head -5Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 292
🏁 Script executed:
# Check ApplicationController for before_action or param validation
find app/controllers -name "application_controller.rb" -exec cat -n {} \;
# Also check if there are any concerns/mixins that handle pagination
rg "pagination|limit|offset" app/controllers/application_controller.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 16094
🏁 Script executed:
# Check for other limit/offset patterns in controllers
rg "\.limit\(.*params" app/controllers/ -A 1 -B 1 | head -40
# Also search for offset patterns
rg "\.offset\(.*params" app/controllers/ -A 1 -B 1 | head -40Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 70
🏁 Script executed:
# Let's search more broadly for limit/offset usage
rg "limit.*params|offset.*params" app/controllers/ --type rb
# Also check if there's any validation in model or concern
rg "validate.*limit|validate.*offset" app/models/ --type rb
# Check for concerns that might handle pagination
find app -name "*pagination*" -o -name "*paginat*"Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 141
🏁 Script executed:
# Search for limit/offset patterns more broadly
rg "limit.*params|offset.*params" app/controllers/
# Check if Rails behavior documentation is known - let's test it
rg "\.limit\(" app/controllers/ | head -20
rg "\.offset\(" app/controllers/ | head -20Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 2072
🏁 Script executed:
# Verify the exact pattern from posts_controller
grep -n "limit.*params\|offset.*params" app/controllers/posts_controller.rb | head -10Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 291
🏁 Script executed:
# Let's see the context around posts_controller pagination to confirm it's the same endpoint type
sed -n '295,320p' app/controllers/posts_controller.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 911
Add bounds checking to pagination parameters to prevent invalid SQL queries.
The limit and offset parameters are taken directly from user input without validation. Other endpoints in this codebase (e.g., posts_controller.rb) properly clamp these values. Unclamped values can cause SQL errors or inefficient queries. Apply bounds checking similar to the established pattern elsewhere in the codebase: offset = [params[:offset].to_i, 0].max and limit = [(params[:limit] || 60).to_i, 100].min.
🔧 Suggested bounds checking
- limit = (params[:limit] || 50).to_i
- offset = params[:offset].to_i
+ limit = [(params[:limit] || 50).to_i, 100].min
+ offset = [params[:offset].to_i, 0].max🤖 Prompt for AI Agents
In `@app/controllers/groups_controller.rb` around lines 22 - 35, Clamp the
pagination params before querying: replace the current direct uses of params in
computing limit and offset so limit is bounded to a maximum (e.g.,
[(params[:limit] || 60).to_i, 100].min) and offset is non-negative (e.g.,
[params[:offset].to_i, 0].max); then use these clamped limit and offset values
in the call to group.users.order(:username_lower).limit(limit).offset(offset) so
serialize_data(members, GroupUserSerializer) gets a safe, bounded result.
| it "cannot remove members from automatic groups" do | ||
| xhr :put, :remove_member, group_id: 1, user_id: 42 | ||
| response.status.should == 422 | ||
| end |
There was a problem hiding this comment.
HTTP method inconsistency: test uses PUT but implementation expects DELETE.
Line 115 uses xhr :put, :remove_member but the actual controller action at line 125 is called with xhr :delete, :remove_member. This test will likely fail or test the wrong behavior.
🐛 Proposed 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.
| it "cannot remove members from automatic groups" do | |
| xhr :put, :remove_member, group_id: 1, user_id: 42 | |
| response.status.should == 422 | |
| end | |
| it "cannot remove members from automatic groups" do | |
| xhr :delete, :remove_member, group_id: 1, user_id: 42 | |
| response.status.should == 422 | |
| end |
🤖 Prompt for AI Agents
In `@spec/controllers/admin/groups_controller_spec.rb` around lines 114 - 117, The
spec is calling the wrong HTTP verb for the remove_member action; update the
test in groups_controller_spec.rb to use the DELETE verb instead of PUT (change
the xhr :put, :remove_member call to xhr :delete, :remove_member with the same
group_id and user_id) so it matches the controller's delete behavior and still
asserts response.status == 422.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Style