Automated Test: group-dm-user-addition-post #312
Conversation
📝 WalkthroughWalkthroughThis refactor restructures group membership management by introducing client-side pagination, separating member operations from group editing, and replacing GET users endpoint with PUT/DELETE members operations. The admin interface now provides form-based member add/remove flows with confirmation dialogs and pagination controls. Changes
Sequence DiagramsequenceDiagram
participant Admin as Admin UI
participant Controller as AdminGroupsController
participant Model as Group Model
participant API as Backend API
participant Store as Data Store
Admin->>Controller: trigger addMembers action with usernames
Controller->>Model: call addMembers(usernames)
Model->>API: PUT /admin/groups/{id}/members
API->>Store: update group membership
Store-->>API: success response
API-->>Model: return response
Model->>Model: call findMembers()
Model->>API: GET /groups/{name}/members.json
API->>Store: fetch members with pagination
Store-->>API: return paginated members
API-->>Model: return members, total, limit, offset
Model-->>Admin: update members list
Admin->>Controller: trigger removeMember with member
Controller->>Model: call removeMember(member)
Model->>API: DELETE /admin/groups/{id}/members
API->>Store: remove member from group
Store-->>API: success response
API-->>Model: return response
Model->>Model: call findMembers()
Model->>API: GET /groups/{name}/members.json
API-->>Model: return updated members
Model-->>Admin: refresh members list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 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)
1-6:⚠️ Potential issue | 🟡 MinorGuard condition checks
modelbut iteration usesmembers— potential mismatch.Line 1 guards on
{{#ifmodel}}, but line 6 iterates overmembers. Ifmembersis populated independently frommodel, the guard won't protect against an empty/undefinedmemberscollection. Consider aligning the guard:Proposed fix
-{{`#if` model}} +{{`#if` members}}
19-19:⚠️ Potential issue | 🟡 MinorStray
</div>closing tag inside a<table>— invalid HTML.There is no matching
<div>opening tag within the{{#each}}loop. This</div>is orphaned inside the table structure and should be removed.Proposed fix
</tr> - </div> {{/each}}
🤖 Fix all issues with AI agents
In `@app/assets/javascripts/admin/controllers/admin-group.js.es6`:
- Around line 29-38: The next action currently sets offset to Math.min(offset +
limit, user_count) which can equal user_count and produce an empty page; change
the clamp to use the start of the last page by computing lastStart = Math.max(0,
group.get("user_count") - group.get("limit")) and set offset =
Math.min(group.get("offset") + group.get("limit"), lastStart) before calling
group.findMembers(); update the logic inside the next function (refer to next,
group.get("offset"), group.get("limit"), group.get("user_count"), and
findMembers) accordingly.
- Around line 11-14: The computed property totalPages incorrectly uses
Math.floor(...)+1 causing an off-by-one when user_count is divisible by limit;
replace the current formula in totalPages with Math.ceil(this.get("user_count")
/ this.get("limit")) while preserving the zero check (if user_count == 0 return
0) so showingLast and pagination logic reflect the correct number of pages.
In `@app/assets/javascripts/admin/templates/group.hbs`:
- Around line 1-53: The wrapper <form> causes native submit on Enter; either
replace the top-level <form> with a <div> to avoid browser reloads, or add a
submit handler that prevents default (e.g. wire the template to an Ember action
on submit that calls event.preventDefault() and delegates to save/addMembers as
needed). Locate the form tag in this template and update it; if adding the
handler, ensure it integrates with existing actions like "save", "addMembers"
and "destroy" so Enter doesn't trigger a page reload.
In `@app/assets/javascripts/discourse/models/group.js`:
- Around line 77-79: The save() method issues a PUT to "/admin/groups/" +
this.get('id') without checking for a missing id, which can result in requests
to "/admin/groups/undefined"; add the same guard used in destroy() (check
this.get('id') exists) and return a rejected promise or throw/early-return when
id is falsy, or alternatively document that create() must be called first;
update the save() implementation to validate this.get('id') before calling
Discourse.ajax so it behaves consistently with destroy().
- Around line 81-84: The destroy function currently returns undefined when
this.get('id') is falsy, causing callers like group.destroy().then(...) to
throw; update destroy (the method named destroy in the group model) to return a
resolved promise when no id is present (e.g., return a resolved
Promise/Discourse.Promise/Ember.RSVP.resolve()) so it always returns a thenable,
and remove the stray semicolon after the closing brace to eliminate the empty
statement; this mirrors the safe behavior used by findMembers.
- Around line 20-38: Change the early-return in findMembers to return a resolved
promise (use Ember.RSVP.resolve()) rather than undefined so callers can safely
chain .then(); also tighten the offset calculation in findMembers so it cannot
equal user_count (e.g., clamp offset to at most Math.max(0,
this.get("user_count") - this.get("limit")) instead of
Math.min(this.get("user_count"), ...)) to avoid requesting from exactly the
total count.
In `@app/controllers/admin/groups_controller.rb`:
- Around line 65-82: In add_members: trim whitespace from each entry returned by
usernames.split(",") (e.g., call strip on each username) before looking up users
with User.find_by_username so " bob" matches "bob"; collect usernames that fail
to resolve and, after attempting to add all valid users via group.add, return a
helpful response listing missing usernames (or include them in the error path)
instead of silently skipping them, while preserving existing group.save and
render json success/error behavior.
In `@app/controllers/groups_controller.rb`:
- Around line 22-23: The limit param is currently unbounded; add a maximum cap
and sanitize it before use by defining a MAX_LIMIT constant (e.g., MAX_LIMIT =
100) in GroupsController and replacing the existing limit assignment to parse
the param, enforce a minimum (>=1) and clamp it to MAX_LIMIT (use Integer
parsing or to_i then .clamp(1, MAX_LIMIT)). Update the variable referenced as
limit (and optionally ensure offset is non-negative) so requests cannot request
arbitrarily large result sets.
In `@spec/controllers/admin/groups_controller_spec.rb`:
- Around line 114-117: The test "cannot remove members from automatic groups" is
sending the wrong HTTP verb; change the request in that example to use the
DELETE verb instead of PUT so it exercises the actual route — update the test
that calls remove_member (the xhr :put, :remove_member, group_id: 1, user_id: 42
line) to use xhr :delete with the same parameters so the spec validates the
DELETE "members" => "groups#remove_member" route.
🧹 Nitpick comments (8)
spec/controllers/admin/groups_controller_spec.rb (1)
13-132: RuboCop: extra empty lines at block body boundaries throughout the spec.Static analysis flags ~13 instances of
Layout/EmptyLinesAroundBlockBodyviolations (extra blank lines at the start/end of context blocks). Consider removing them for consistency if your project enforces this convention.app/assets/stylesheets/common/admin/admin_base.scss (1)
462-480: Hardcoded colors#333and#aaabreak theme consistency.The rest of this file consistently uses theme variables (
$primary,$secondary,scale-color-diff(), etc.) for colors. The hardcoded hex values on.next/.previousand.disabledwill not adapt to dark themes or custom color schemes.Suggested fix 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/admin/templates/group.hbs (1)
16-18: Pagination uses<a>tags withouthref, which hurts keyboard accessibility.The
<a>elements for previous/next lackhrefattributes, so they won't be focusable via keyboard by default. Consider addinghref="#"or using<button>elements instead for proper keyboard navigation and semantic correctness.app/assets/javascripts/admin/controllers/admin-group.js.es6 (3)
6-14: Use strict equality (===) instead of loose equality (==).Lines 7 and 12 use
==for comparinguser_countto0. While functionally equivalent here,===is the idiomatic JavaScript practice and avoids subtle type-coercion pitfalls.Suggested 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"), totalPages: function() { - if (this.get("user_count") == 0) { return 0; } + if (this.get("user_count") === 0) { return 0; } return Math.ceil(this.get("user_count") / this.get("limit")); }.property("limit", "user_count"),
51-59:removeMemberhas no error handling — failures are silently swallowed.If
model.removeMember(member)rejects, the user sees no feedback. Consider chaining a.catch()/ error callback to display an alert.
61-65: TODO: input should be cleared after adding members.The TODO on line 62 notes that the usernames input is not cleared after a successful add. This could lead to duplicate submissions.
Would you like me to propose a fix that clears
usernamesafteraddMemberssucceeds?app/controllers/admin/groups_controller.rb (1)
6-8: RuboCop: wrap assignment-in-condition in parentheses.Lines 6 and 72 use assignment inside
ifconditions. While idiomatic Ruby, RuboCop flags it. Wrapping in parentheses signals intent and silences the warning.Proposed fix for line 6
- if search = params[:search] + if (search = params[:search])Proposed fix for line 72
- if user = User.find_by_username(username) + if (user = User.find_by_username(username.strip))app/assets/javascripts/discourse/models/group.js (1)
51-60: Missing semicolon after the closing})on line 59.Line 48 (
removeMember) ends with});but line 59 (addMembers) ends with})— inconsistent within the same file.Fix
}).then(function() { // reload member list self.findMembers(); - }) + });
| 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 is off-by-one when user_count is evenly divisible by limit.
Math.floor(user_count / limit) + 1 yields one extra page whenever user_count is a multiple of limit (e.g., 50 users with limit 50 → reports 2 pages instead of 1). This causes showingLast to be false when actually on the last page, letting the user paginate past the real end into an empty result.
Fix: use `Math.ceil`
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"),📝 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 computed property totalPages incorrectly uses Math.floor(...)+1
causing an off-by-one when user_count is divisible by limit; replace the current
formula in totalPages with Math.ceil(this.get("user_count") / this.get("limit"))
while preserving the zero check (if user_count == 0 return 0) so showingLast and
pagination logic reflect the correct number of pages.
| 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.
next action can set offset to user_count, producing an empty page.
Line 33 clamps to user_count, but if offset + limit >= user_count, the fetch will return zero results. The upper bound should be the start of the last page.
Suggested fix
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"));
+ offset = Math.min(group.get("offset") + group.get("limit"),
+ Math.max(group.get("user_count") - group.get("limit"), 0));
group.set("offset", offset);
return group.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.
| 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"), | |
| offset = Math.min(group.get("offset") + group.get("limit"), | |
| Math.max(group.get("user_count") - group.get("limit"), 0)); | |
| 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 action currently sets offset to Math.min(offset + limit,
user_count) which can equal user_count and produce an empty page; change the
clamp to use the start of the last page by computing lastStart = Math.max(0,
group.get("user_count") - group.get("limit")) and set offset =
Math.min(group.get("offset") + group.get("limit"), lastStart) before calling
group.findMembers(); update the logic inside the next function (refer to next,
group.get("offset"), group.get("limit"), group.get("user_count"), and
findMembers) accordingly.
| <form class="form-horizontal"> | ||
|
|
||
| <div class="control-group"> | ||
| <label class="control-label">{{i18n 'admin.groups.group_members'}}</label> | ||
| <div class="controls"> | ||
| {{user-selector usernames=usernames id="group-users" placeholderKey="admin.groups.selector_placeholder" tabindex="1" disabled=automatic}} | ||
| <div> | ||
| {{#if automatic}} | ||
| <h3>{{name}}</h3> | ||
| {{else}} | ||
| <label for="name">{{i18n 'admin.groups.name'}}</label> | ||
| {{text-field name="name" value=name placeholderKey="admin.groups.name_placeholder"}} | ||
| {{/if}} | ||
| </div> | ||
| </div> | ||
| <div class="control-group"> | ||
| <div class="controls"> | ||
| {{input type="checkbox" checked=visible}} {{i18n 'groups.visible'}} | ||
|
|
||
| {{#if id}} | ||
| <div> | ||
| <label>{{i18n 'admin.groups.group_members'}} ({{user_count}})</label> | ||
| <div> | ||
| <a {{bind-attr class=":previous showingFirst:disabled"}} {{action "previous"}}>{{fa-icon "fast-backward"}}</a> | ||
| {{currentPage}}/{{totalPages}} | ||
| <a {{bind-attr class=":next showingLast:disabled"}} {{action "next"}}>{{fa-icon "fast-forward"}}</a> | ||
| </div> | ||
| <div class="ac-wrap clearfix"> | ||
| {{each member in members itemView="group-member"}} | ||
| </div> | ||
| </div> | ||
|
|
||
| {{#unless automatic}} | ||
| <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> | ||
| </div> | ||
| {{/unless}} | ||
| {{/if}} | ||
|
|
||
| <div> | ||
| <label> | ||
| {{input type="checkbox" checked=visible}} | ||
| {{i18n 'groups.visible'}} | ||
| </label> | ||
| </div> | ||
| </div> | ||
| <div class="control-group"> | ||
| <label class="control-label">{{i18n 'groups.alias_levels.title'}}</label> | ||
| <div class="controls"> | ||
| {{combo-box valueAttribute="value" value=alias_level content=aliasLevelOptions}} | ||
|
|
||
| <div> | ||
| <label for="alias">{{i18n 'groups.alias_levels.title'}}</label> | ||
| {{combo-box name="alias" valueAttribute="value" value=alias_level content=aliasLevelOptions}} | ||
| </div> | ||
| </div> | ||
| <div class='controls'> | ||
| <button {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn'>{{i18n 'admin.customize.save'}}</button> | ||
| {{#unless automatic}} | ||
| <button {{action "destroy"}} class='btn btn-danger'><i class="fa fa-trash-o"></i>{{i18n 'admin.customize.delete'}}</button> | ||
| {{/unless}} | ||
| </div> | ||
|
|
||
| <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> | ||
|
|
||
| </form> |
There was a problem hiding this comment.
Native form submission on Enter key will cause a page reload.
Wrapping everything in <form> without a submit handler means pressing Enter while focused on the name text field or user-selector will trigger a browser-native form submission, reloading the page. Either add an Ember submit action that prevents default, or switch to a <div>.
Option A: handle submit on the form
-<form class="form-horizontal">
+<form class="form-horizontal" {{action "save" on="submit"}}>Option B: use a div instead
-<form class="form-horizontal">
+<div class="form-horizontal">
...
-</form>
+</div>📝 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.
| <form class="form-horizontal"> | |
| <div class="control-group"> | |
| <label class="control-label">{{i18n 'admin.groups.group_members'}}</label> | |
| <div class="controls"> | |
| {{user-selector usernames=usernames id="group-users" placeholderKey="admin.groups.selector_placeholder" tabindex="1" disabled=automatic}} | |
| <div> | |
| {{#if automatic}} | |
| <h3>{{name}}</h3> | |
| {{else}} | |
| <label for="name">{{i18n 'admin.groups.name'}}</label> | |
| {{text-field name="name" value=name placeholderKey="admin.groups.name_placeholder"}} | |
| {{/if}} | |
| </div> | |
| </div> | |
| <div class="control-group"> | |
| <div class="controls"> | |
| {{input type="checkbox" checked=visible}} {{i18n 'groups.visible'}} | |
| {{#if id}} | |
| <div> | |
| <label>{{i18n 'admin.groups.group_members'}} ({{user_count}})</label> | |
| <div> | |
| <a {{bind-attr class=":previous showingFirst:disabled"}} {{action "previous"}}>{{fa-icon "fast-backward"}}</a> | |
| {{currentPage}}/{{totalPages}} | |
| <a {{bind-attr class=":next showingLast:disabled"}} {{action "next"}}>{{fa-icon "fast-forward"}}</a> | |
| </div> | |
| <div class="ac-wrap clearfix"> | |
| {{each member in members itemView="group-member"}} | |
| </div> | |
| </div> | |
| {{#unless automatic}} | |
| <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> | |
| </div> | |
| {{/unless}} | |
| {{/if}} | |
| <div> | |
| <label> | |
| {{input type="checkbox" checked=visible}} | |
| {{i18n 'groups.visible'}} | |
| </label> | |
| </div> | |
| </div> | |
| <div class="control-group"> | |
| <label class="control-label">{{i18n 'groups.alias_levels.title'}}</label> | |
| <div class="controls"> | |
| {{combo-box valueAttribute="value" value=alias_level content=aliasLevelOptions}} | |
| <div> | |
| <label for="alias">{{i18n 'groups.alias_levels.title'}}</label> | |
| {{combo-box name="alias" valueAttribute="value" value=alias_level content=aliasLevelOptions}} | |
| </div> | |
| </div> | |
| <div class='controls'> | |
| <button {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn'>{{i18n 'admin.customize.save'}}</button> | |
| {{#unless automatic}} | |
| <button {{action "destroy"}} class='btn btn-danger'><i class="fa fa-trash-o"></i>{{i18n 'admin.customize.delete'}}</button> | |
| {{/unless}} | |
| </div> | |
| <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> | |
| </form> | |
| <form class="form-horizontal" {{action "save" on="submit"}}> | |
| <div> | |
| {{`#if` automatic}} | |
| <h3>{{name}}</h3> | |
| {{else}} | |
| <label for="name">{{i18n 'admin.groups.name'}}</label> | |
| {{text-field name="name" value=name placeholderKey="admin.groups.name_placeholder"}} | |
| {{/if}} | |
| </div> | |
| {{`#if` id}} | |
| <div> | |
| <label>{{i18n 'admin.groups.group_members'}} ({{user_count}})</label> | |
| <div> | |
| <a {{bind-attr class=":previous showingFirst:disabled"}} {{action "previous"}}>{{fa-icon "fast-backward"}}</a> | |
| {{currentPage}}/{{totalPages}} | |
| <a {{bind-attr class=":next showingLast:disabled"}} {{action "next"}}>{{fa-icon "fast-forward"}}</a> | |
| </div> | |
| <div class="ac-wrap clearfix"> | |
| {{each member in members itemView="group-member"}} | |
| </div> | |
| </div> | |
| {{`#unless` automatic}} | |
| <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> | |
| </div> | |
| {{/unless}} | |
| {{/if}} | |
| <div> | |
| <label> | |
| {{input type="checkbox" checked=visible}} | |
| {{i18n 'groups.visible'}} | |
| </label> | |
| </div> | |
| <div> | |
| <label for="alias">{{i18n 'groups.alias_levels.title'}}</label> | |
| {{combo-box name="alias" valueAttribute="value" value=alias_level content=aliasLevelOptions}} | |
| </div> | |
| <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> | |
| </form> |
| <form class="form-horizontal"> | |
| <div class="control-group"> | |
| <label class="control-label">{{i18n 'admin.groups.group_members'}}</label> | |
| <div class="controls"> | |
| {{user-selector usernames=usernames id="group-users" placeholderKey="admin.groups.selector_placeholder" tabindex="1" disabled=automatic}} | |
| <div> | |
| {{#if automatic}} | |
| <h3>{{name}}</h3> | |
| {{else}} | |
| <label for="name">{{i18n 'admin.groups.name'}}</label> | |
| {{text-field name="name" value=name placeholderKey="admin.groups.name_placeholder"}} | |
| {{/if}} | |
| </div> | |
| </div> | |
| <div class="control-group"> | |
| <div class="controls"> | |
| {{input type="checkbox" checked=visible}} {{i18n 'groups.visible'}} | |
| {{#if id}} | |
| <div> | |
| <label>{{i18n 'admin.groups.group_members'}} ({{user_count}})</label> | |
| <div> | |
| <a {{bind-attr class=":previous showingFirst:disabled"}} {{action "previous"}}>{{fa-icon "fast-backward"}}</a> | |
| {{currentPage}}/{{totalPages}} | |
| <a {{bind-attr class=":next showingLast:disabled"}} {{action "next"}}>{{fa-icon "fast-forward"}}</a> | |
| </div> | |
| <div class="ac-wrap clearfix"> | |
| {{each member in members itemView="group-member"}} | |
| </div> | |
| </div> | |
| {{#unless automatic}} | |
| <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> | |
| </div> | |
| {{/unless}} | |
| {{/if}} | |
| <div> | |
| <label> | |
| {{input type="checkbox" checked=visible}} | |
| {{i18n 'groups.visible'}} | |
| </label> | |
| </div> | |
| </div> | |
| <div class="control-group"> | |
| <label class="control-label">{{i18n 'groups.alias_levels.title'}}</label> | |
| <div class="controls"> | |
| {{combo-box valueAttribute="value" value=alias_level content=aliasLevelOptions}} | |
| <div> | |
| <label for="alias">{{i18n 'groups.alias_levels.title'}}</label> | |
| {{combo-box name="alias" valueAttribute="value" value=alias_level content=aliasLevelOptions}} | |
| </div> | |
| </div> | |
| <div class='controls'> | |
| <button {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn'>{{i18n 'admin.customize.save'}}</button> | |
| {{#unless automatic}} | |
| <button {{action "destroy"}} class='btn btn-danger'><i class="fa fa-trash-o"></i>{{i18n 'admin.customize.delete'}}</button> | |
| {{/unless}} | |
| </div> | |
| <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> | |
| </form> | |
| <div class="form-horizontal"> | |
| <div> | |
| {{`#if` automatic}} | |
| <h3>{{name}}</h3> | |
| {{else}} | |
| <label for="name">{{i18n 'admin.groups.name'}}</label> | |
| {{text-field name="name" value=name placeholderKey="admin.groups.name_placeholder"}} | |
| {{/if}} | |
| </div> | |
| {{`#if` id}} | |
| <div> | |
| <label>{{i18n 'admin.groups.group_members'}} ({{user_count}})</label> | |
| <div> | |
| <a {{bind-attr class=":previous showingFirst:disabled"}} {{action "previous"}}>{{fa-icon "fast-backward"}}</a> | |
| {{currentPage}}/{{totalPages}} | |
| <a {{bind-attr class=":next showingLast:disabled"}} {{action "next"}}>{{fa-icon "fast-forward"}}</a> | |
| </div> | |
| <div class="ac-wrap clearfix"> | |
| {{each member in members itemView="group-member"}} | |
| </div> | |
| </div> | |
| {{`#unless` automatic}} | |
| <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> | |
| </div> | |
| {{/unless}} | |
| {{/if}} | |
| <div> | |
| <label> | |
| {{input type="checkbox" checked=visible}} | |
| {{i18n 'groups.visible'}} | |
| </label> | |
| </div> | |
| <div> | |
| <label for="alias">{{i18n 'groups.alias_levels.title'}}</label> | |
| {{combo-box name="alias" valueAttribute="value" value=alias_level content=aliasLevelOptions}} | |
| </div> | |
| <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> |
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/templates/group.hbs` around lines 1 - 53, The
wrapper <form> causes native submit on Enter; either replace the top-level
<form> with a <div> to avoid browser reloads, or add a submit handler that
prevents default (e.g. wire the template to an Ember action on submit that calls
event.preventDefault() and delegates to save/addMembers as needed). Locate the
form tag in this template and update it; if adding the handler, ensure it
integrates with existing actions like "save", "addMembers" and "destroy" so
Enter doesn't trigger a page reload.
| findMembers: function() { | ||
| if (Em.isEmpty(this.get('name'))) { return Ember.RSVP.resolve([]); } | ||
| if (Em.isEmpty(this.get('name'))) { return ; } | ||
|
|
||
| return Discourse.ajax('/groups/' + this.get('name') + '/members').then(function(result) { | ||
| return result.map(function(u) { return Discourse.User.create(u) }); | ||
| var self = this, offset = Math.min(this.get("user_count"), Math.max(this.get("offset"), 0)); | ||
|
|
||
| return Discourse.ajax('/groups/' + this.get('name') + '/members.json', { | ||
| data: { | ||
| limit: this.get("limit"), | ||
| offset: offset | ||
| } | ||
| }).then(function(result) { | ||
| self.setProperties({ | ||
| user_count: result.meta.total, | ||
| limit: result.meta.limit, | ||
| offset: result.meta.offset, | ||
| members: result.members.map(function(member) { return Discourse.User.create(member); }) | ||
| }); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -C5 'findMembers' --type=jsRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 3495
🏁 Script executed:
cat -n app/assets/javascripts/discourse/models/group.js | sed -n '20,38p'Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 840
🏁 Script executed:
# Let's also check if there are any tests or documentation about expected offset behavior
rg -n "offset" app/assets/javascripts/discourse/models/group.js -B2 -A2Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 791
🏁 Script executed:
rg -n "return ;" app/assets/javascripts/discourse/models/group.jsRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 141
🏁 Script executed:
# Also check Ember/Discourse conventions for early returns in similar methods
rg -n "if.*isEmpty.*return" app/assets/javascripts/discourse/models/ --type=js -A1 | head -30Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1320
Return a resolved promise instead of undefined to match Discourse patterns.
Line 21 uses a bare return statement when the group name is empty, but other Discourse model methods like those in post_stream.js return Ember.RSVP.resolve() in similar guard clauses. This inconsistency could cause issues if future callers chain .then() on the result. Align with the established pattern:
if (Em.isEmpty(this.get('name'))) { return Ember.RSVP.resolve(); }The offset clamping on line 23 also allows offset to equal user_count, which would request data starting exactly at the total count and return zero results. Consider clamping to Math.max(0, this.get("user_count") - this.get("limit")) or letting the server handle edge cases.
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/models/group.js` around lines 20 - 38,
Change the early-return in findMembers to return a resolved promise (use
Ember.RSVP.resolve()) rather than undefined so callers can safely chain .then();
also tighten the offset calculation in findMembers so it cannot equal user_count
(e.g., clamp offset to at most Math.max(0, this.get("user_count") -
this.get("limit")) instead of Math.min(this.get("user_count"), ...)) to avoid
requesting from exactly the total count.
| save: function(){ | ||
| return Discourse.ajax("/admin/groups/" + this.get('id'), { type: "PUT", data: this.asJSON() }); | ||
| }, |
There was a problem hiding this comment.
save() doesn't guard against a missing id, unlike destroy().
If save() is called on a group that hasn't been created yet (no id), the request goes to /admin/groups/undefined. The destroy() method at line 82 already guards for this — consider adding a similar check here, or at minimum documenting that create() must be called first.
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/models/group.js` around lines 77 - 79, The
save() method issues a PUT to "/admin/groups/" + this.get('id') without checking
for a missing id, which can result in requests to "/admin/groups/undefined"; add
the same guard used in destroy() (check this.get('id') exists) and return a
rejected promise or throw/early-return when id is falsy, or alternatively
document that create() must be called first; update the save() implementation to
validate this.get('id') before calling Discourse.ajax so it behaves consistently
with destroy().
| destroy: function(){ | ||
| if (!this.get('id')) { return }; | ||
| return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"}); | ||
| }, |
There was a problem hiding this comment.
destroy() returns undefined when id is falsy — same chaining risk as findMembers.
If a caller does group.destroy().then(...) on a group without an id, this will throw a TypeError. Return a resolved promise for a consistent thenable API. Also, the semicolon after } on line 82 is a stray empty statement.
Proposed fix
destroy: function(){
- if (!this.get('id')) { return };
+ if (!this.get('id')) { return Ember.RSVP.resolve(); }
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 function currently returns undefined when this.get('id') is falsy,
causing callers like group.destroy().then(...) to throw; update destroy (the
method named destroy in the group model) to return a resolved promise when no id
is present (e.g., return a resolved
Promise/Discourse.Promise/Ember.RSVP.resolve()) so it always returns a thenable,
and remove the stray semicolon after the closing brace to eliminate the empty
statement; this mirrors the safe behavior used by findMembers.
| def add_members | ||
| group = Group.find(params.require(:group_id).to_i) | ||
| usernames = params.require(:usernames) | ||
|
|
||
| return can_not_modify_automatic if group.automatic | ||
|
|
||
| usernames.split(",").each do |username| | ||
| if user = User.find_by_username(username) | ||
| group.add(user) | ||
| end | ||
| end | ||
|
|
||
| if group.save | ||
| render_serialized(group, BasicGroupSerializer) | ||
| render json: success_json | ||
| else | ||
| render_json_error group | ||
| render_json_error(group) | ||
| end | ||
| end |
There was a problem hiding this comment.
Usernames are not stripped — spaces in comma-separated input silently skip valid users.
usernames.split(",") on line 71 preserves surrounding whitespace. If the client sends "alice, bob", the lookup for " bob" will return nil and bob is silently skipped. Also, there's no feedback when a username doesn't match any user, which could silently ignore typos.
Proposed fix: strip and collect failures
usernames.split(",").each do |username|
- if user = User.find_by_username(username)
+ if user = User.find_by_username(username.strip)
group.add(user)
end
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.
| def add_members | |
| group = Group.find(params.require(:group_id).to_i) | |
| usernames = params.require(:usernames) | |
| return can_not_modify_automatic if group.automatic | |
| usernames.split(",").each do |username| | |
| if user = User.find_by_username(username) | |
| group.add(user) | |
| end | |
| end | |
| if group.save | |
| render_serialized(group, BasicGroupSerializer) | |
| render json: success_json | |
| else | |
| render_json_error group | |
| render_json_error(group) | |
| end | |
| end | |
| def add_members | |
| group = Group.find(params.require(:group_id).to_i) | |
| usernames = params.require(:usernames) | |
| return can_not_modify_automatic if group.automatic | |
| usernames.split(",").each do |username| | |
| if user = User.find_by_username(username.strip) | |
| group.add(user) | |
| end | |
| end | |
| if group.save | |
| render json: success_json | |
| else | |
| render_json_error(group) | |
| end | |
| end |
🧰 Tools
🪛 RuboCop (1.84.0)
[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)
🤖 Prompt for AI Agents
In `@app/controllers/admin/groups_controller.rb` around lines 65 - 82, In
add_members: trim whitespace from each entry returned by usernames.split(",")
(e.g., call strip on each username) before looking up users with
User.find_by_username so " bob" matches "bob"; collect usernames that fail to
resolve and, after attempting to add all valid users via group.add, return a
helpful response listing missing usernames (or include them in the error path)
instead of silently skipping them, while preserving existing group.save and
render json success/error behavior.
| limit = (params[:limit] || 50).to_i | ||
| offset = params[:offset].to_i |
There was a problem hiding this comment.
Unbounded limit parameter allows arbitrarily large result sets.
A client can pass limit=999999 to fetch all group members in a single request, causing excessive memory usage and slow queries. Cap it to a reasonable maximum.
🛡️ Proposed fix
- limit = (params[:limit] || 50).to_i
- offset = params[:offset].to_i
+ limit = [(params[:limit] || 50).to_i, 1000].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, 1000].min | |
| offset = [params[:offset].to_i, 0].max |
🤖 Prompt for AI Agents
In `@app/controllers/groups_controller.rb` around lines 22 - 23, The limit param
is currently unbounded; add a maximum cap and sanitize it before use by defining
a MAX_LIMIT constant (e.g., MAX_LIMIT = 100) in GroupsController and replacing
the existing limit assignment to parse the param, enforce a minimum (>=1) and
clamp it to MAX_LIMIT (use Integer parsing or to_i then .clamp(1, MAX_LIMIT)).
Update the variable referenced as limit (and optionally ensure offset is
non-negative) so requests cannot request arbitrarily large result sets.
| 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.
🧩 Analysis chain
🏁 Script executed:
# Search for the remove_member route definition
rg -n "remove_member" config/routes.rb -A 2 -B 2Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 290
🏁 Script executed:
# Also check the test file at the specific lines mentioned
sed -n '110,132p' spec/controllers/admin/groups_controller_spec.rbRepository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 600
HTTP method mismatch: remove_member test uses :put but the route is DELETE.
Line 115 sends a PUT request for remove_member, but the route is defined as DELETE "members" => "groups#remove_member" (config/routes.rb:49). The companion test on line 125 correctly uses :delete. While functional tests dispatch by action name, using the wrong HTTP verb means this test doesn't validate the actual route and could mask routing issues.
Proposed fix
- xhr :put, :remove_member, group_id: 1, user_id: 42
+ xhr :delete, :remove_member, group_id: 1, user_id: 42📝 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
test "cannot remove members from automatic groups" is sending the wrong HTTP
verb; change the request in that example to use the DELETE verb instead of PUT
so it exercises the actual route — update the test that calls remove_member (the
xhr :put, :remove_member, group_id: 1, user_id: 42 line) to use xhr :delete with
the same parameters so the spec validates the DELETE "members" =>
"groups#remove_member" route.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
New Features
Style
Tests