FIX: proper handling of group memberships - #8
Conversation
|
This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days. |
mfeuerstein
left a comment
mfeuerstein
left a comment
There was a problem hiding this comment.
PR Review — approved
Reviewed 16 files. 0 high-severity issues found. Verdict: approved.
app/assets/javascripts/discourse/templates/components/admin-group-selector.hbs (low)
- Reviewed app/assets/javascripts/discourse/templates/components/admin-group-selector.hbs — looks good
app/assets/javascripts/admin/controllers/admin-group.js.es6 (medium)
- Reviewed app/assets/javascripts/admin/controllers/admin-group.js.es6 — looks good
app/assets/javascripts/admin/templates/group.hbs (low)
- Reviewed app/assets/javascripts/admin/templates/group.hbs — looks good
app/assets/javascripts/discourse/models/group.js (low)
- Reviewed app/assets/javascripts/discourse/models/group.js — looks good
app/assets/javascripts/admin/templates/group_member.hbs (low)
- Reviewed app/assets/javascripts/admin/templates/group_member.hbs — looks good
app/assets/javascripts/admin/views/group-member.js.es6 (low)
- Reviewed app/assets/javascripts/admin/views/group-member.js.es6 — looks good
app/assets/javascripts/admin/routes/admin_group_route.js (low)
- Reviewed app/assets/javascripts/admin/routes/admin_group_route.js — looks good
app/assets/stylesheets/common/admin/admin_base.scss (low)
- Reviewed app/assets/stylesheets/common/admin/admin_base.scss — looks good
app/assets/javascripts/discourse/templates/user-selector-autocomplete.raw.hbs (low)
- Reviewed app/assets/javascripts/discourse/templates/user-selector-autocomplete.raw.hbs — looks good
config/locales/client.en.yml (low)
- Reviewed config/locales/client.en.yml — looks good
app/assets/javascripts/discourse/routes/group-members.js.es6 (low)
- Reviewed app/assets/javascripts/discourse/routes/group-members.js.es6 — looks good
app/assets/javascripts/discourse/templates/group/members.hbs (low)
- Reviewed app/assets/javascripts/discourse/templates/group/members.hbs — looks good
app/controllers/groups_controller.rb (medium)
- Reviewed app/controllers/groups_controller.rb — looks good
config/routes.rb (low)
- Reviewed config/routes.rb — looks good
spec/controllers/admin/groups_controller_spec.rb (low)
- Reviewed spec/controllers/admin/groups_controller_spec.rb — looks good
app/controllers/admin/groups_controller.rb (low)
- Reviewed app/controllers/admin/groups_controller.rb — looks good
zach-source
left a comment
zach-source
left a comment
There was a problem hiding this comment.
Found 3 blocking issues.
- high
app/controllers/admin/groups_controller.rb:63— add_members reads params[:group_id] but the route only provides params[:id] - high
app/controllers/admin/groups_controller.rb:82— remove_member reads params[:group_id] but the route only provides params[:id] - medium
app/assets/javascripts/admin/controllers/admin-group.js.es6:13— totalPages off-by-one when user_count is an exact multiple of limit
| def refresh_automatic_groups | ||
| Group.refresh_automatic_groups! | ||
| render json: success_json | ||
| end |
There was a problem hiding this comment.
[high] add_members reads params[:group_id] but the route only provides params[:id]
routes.rb defines put "members" => "groups#add_members" as a bare route inside resources :groups do ... end (same position as the removed get "users" line), which Rails scopes as a member route: /admin/groups/:id/members. That matches the JS caller (Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {type: "PUT", ...})), so the route param is :id, never :group_id. params.require(:group_id) will therefore raise ActionController::ParameterMissing on every real request. The controller spec only passes because it calls xhr :put, :add_members, group_id: 1, ... directly, sidestepping the actual route/param wiring.
Suggestion: Use params[:id] (the member-route id) to look up the group, matching how the JS client calls the endpoint and how the sibling update/destroy actions already read params[:id].
| end | |
| group = Group.find(params.require(:id).to_i) |
| render_json_error group | ||
| render_json_error(group) | ||
| end | ||
| end |
There was a problem hiding this comment.
[high] remove_member reads params[:group_id] but the route only provides params[:id]
Same mismatch as add_members: delete "members" => "groups#remove_member" is a member-scoped route (/admin/groups/:id/members), matching the JS call Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {type: "DELETE", data: { user_id: ... }}). params.require(:group_id) will never find a value and raises ActionController::ParameterMissing on every real removal request.
Suggestion: Read the group id from params[:id] instead of params[:group_id].
| end | |
| group = Group.find(params.require(:id).to_i) |
|
|
||
| totalPages: function() { | ||
| if (this.get("user_count") == 0) { return 0; } | ||
| return Math.floor(this.get("user_count") / this.get("limit")) + 1; |
There was a problem hiding this comment.
[medium] totalPages off-by-one when user_count is an exact multiple of limit
totalPages is computed as Math.floor(user_count / limit) + 1. When user_count is an exact multiple of limit (e.g. 100 users, limit 50), this yields 3 instead of the correct 2 pages. Because showingLast is currentPage === totalPages, the pager never reports the real last page as "last": clicking next from the true last page (currentPage=2) is still allowed, offset becomes 100 (>= user_count), and findMembers fetches/renders an empty page that the UI now reports as the actual last page.
Suggestion: Use Math.ceil instead of floor+1, which is correct for both exact and non-exact multiples.
| return Math.floor(this.get("user_count") / this.get("limit")) + 1; | |
| return Math.ceil(this.get("user_count") / this.get("limit")); |
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Test 8
Problem
This PR refactors group membership management from a single update action (handling add/remove via params[:changes]) into dedicated add_members / remove_member endpoints, and adds a paginated public members API plus an Ember pagination UI (prev/next, page counter) for the admin group screen.
Solution Reviewed
Backend: new Admin::GroupsController#add_members / #remove_member actions backed by new put/delete "members" member routes; GroupsController#members rewritten to return { members, meta: { total, limit, offset } } with limit/offset pagination. Frontend: Group model gains findMembers/addMembers/removeMember AJAX methods; admin-group controller gains currentPage/totalPages/next/previous; templates render a paginated member list with add/remove controls. Specs rewritten for the new action shapes.
Summary
The endpoint split and the new paginated members contract are sound, and the routing/param handling is correct (verified against Rails 4.1.8's mapper — bare routes inside resources use the nested scope and yield params[:group_id], matching the controllers). One real blocking bug in the new pagination math, plus several non-blocking gaps around missing test coverage for the new public contract and dropped edge-case tests.
Files Reviewed
app/controllers/admin/groups_controller.rb— deeply reviewedapp/controllers/groups_controller.rb— deeply reviewedapp/assets/javascripts/discourse/models/group.js— deeply reviewedapp/assets/javascripts/admin/controllers/admin-group.js.es6— deeply reviewedapp/assets/javascripts/admin/templates/group.hbs— lightly reviewedconfig/routes.rb— deeply reviewed (routing semantics verified against Rails 4.1.8 source)spec/controllers/admin/groups_controller_spec.rb— deeply reviewedspec/controllers/groups_controller_spec.rb— deeply reviewed (unchanged by PR; noted in body)- SCSS / locale / template formatting changes — lightly reviewed
Verification
- Rails 4.1.8
action_dispatch/routing/mapper.rbsource read — confirmed bare in-block routes underresourcesresolve vianestedscope →:group_id(not:id); the repo's ownGroupsController#show→:idvsmembers/posts/counts→:group_idsplit independently confirms this. Theadd_members/remove_memberparams.require(:group_id)usage is correct. - Rails 4.1.8
collection_association.rbsource read — confirmedCollectionAssociation#deletecoercesFixnum/Stringargs viafindbeforedelete_or_destroy/raise_on_type_mismatch!, sogroup.users.delete(user_id)with an integer is valid. - No Ruby/JS runtime available in this environment; existing specs not executed.
Issues Found
Not anchorable to the diff (file unchanged by this PR)
spec/controllers/groups_controller_spec.rb:65-85— Themembersdescribe block has no valid coverage for the new{ members, meta }contract this PR introduces. The activeitat line 65 ("callsposts_forand responds with JSON") actually callsxhr :get, :posts(line 67), never:members— a copy-paste of thepostsspec. The only test that hits:memberswith body assertions ispending "ensures that membership can be paginated"(line 72), which is still pending and parses the response as a bare array (members.map{ |m| m['username'] }), a shape the PR no longer returns. Net: the new public pagination contract has zero passing coverage. Recommend fixing line 67 to:membersand asserting themembers/metakeys, and un-pending + rewriting the pagination test against the new shape.
Verdict
Recommend changes before merge — the pagination totalPages off-by-one is a real correctness bug in the PR's headline feature and is a one-line fix; the test-coverage gaps should be closed so the new public contract is actually guarded.
|
|
||
| totalPages: function() { | ||
| if (this.get("user_count") == 0) { return 0; } | ||
| return Math.floor(this.get("user_count") / this.get("limit")) + 1; |
There was a problem hiding this comment.
🔴 Blocking — totalPages off-by-one. Math.floor(user_count / limit) + 1 overcounts by exactly one page whenever user_count is an exact multiple of limit. Example: 100 users, limit 50 → floor(100/50)+1 = 3, but the correct page count is 2.
Downstream impact: showingLast = propertyEqual("currentPage", "totalPages") (line 17) is false on the real last page (currentPage 2 vs totalPages 3), so the next guard if (this.get("showingLast")) { return; } (line 30) does not stop navigation — the user can click Next into a phantom page 3 whose fetch returns zero members, and the header reads 2/3 then 3/3.
Fix: Math.ceil(user_count / limit) (with the existing user_count == 0 guard keeping 0 members → 0 pages), e.g.:
totalPages: function() {
if (this.get("user_count") == 0) { return 0; }
return Math.ceil(this.get("user_count") / this.get("limit"));
}.property("limit", "user_count"),|
|
||
| findMembers: function() { | ||
| if (Em.isEmpty(this.get('name'))) { return Ember.RSVP.resolve([]); } | ||
| if (Em.isEmpty(this.get('name'))) { return ; } |
There was a problem hiding this comment.
🟡 findMembers now returns bare undefined when the group name is empty (return ;), where the previous implementation returned Ember.RSVP.resolve([]). This breaks the method's implicit always-returns-a-promise contract: any caller doing findMembers().then(...) (the pattern the old afterModel used) will throw TypeError: Cannot read property 'then' of undefined when the name is empty. No current caller chains directly on the result today (routes call it fire-and-forget), so this is latent — but the contract regression is introduced by this PR. Restore return Ember.RSVP.resolve(); (or []) for the empty-name branch.
| end | ||
|
|
||
| render_serialized(members.to_a, GroupUserSerializer) | ||
| limit = (params[:limit] || 50).to_i |
There was a problem hiding this comment.
🟡 The public (non-admin) members endpoint takes limit/offset straight from params with no clamping: limit = (params[:limit] || 50).to_i, offset = params[:offset].to_i. Negative values pass through unchanged and PostgreSQL rejects them (OFFSET must not be negative / LIMIT must not be negative) → unhandled 500 on ?offset=-1 or ?limit=-1. There is also no upper cap on limit, so ?limit=999999 forces loading/serializing the entire visible membership into memory (the old code capped automatic groups at 200). This endpoint is reachable by any client for visible groups. Clamp both, e.g. limit = [[params[:limit].to_i, 1].max, 200].min and offset = [params[:offset].to_i, 0].max.
| return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', { | ||
| type: "DELETE", | ||
| data: { user_id: member.get("id") } | ||
| }).then(function() { |
There was a problem hiding this comment.
🟡 removeMember (and identically addMembers at line 56 and findMembers at line 30) register only a fulfillment handler on the AJAX promise. Discourse.ajax rejects on any non-2xx, so a failed DELETE/PUT (network error, 422 for an automatic group, 500) is an unhandled rejection; the bootbox confirm callback in admin-group.js.es6 does not chain a .catch, so the user gets no feedback and the member list is never reloaded — a failed remove leaves the member appearing to still be present. This matches the era's fire-and-forget AJAX convention, so non-blocking, but consider adding a rejection handler consistent with the save action's bootbox.alert error pattern.
| }, | ||
|
|
||
| addMembers: function() { | ||
| // TODO: should clear the input |
There was a problem hiding this comment.
🟡 The addMembers action is fire-and-forget: it discards the returned promise, never disables the Add button, and the // TODO: should clear the input right here confirms usernames is not cleared after add. A double-click or double-Enter fires two concurrent PUTs to /admin/groups/:id/members.json; because group_users has a unique index on [group_id, user_id], re-adding an existing member makes the second group.save fail with a unique-constraint violation — a swallowed 500 with no user feedback. Chain on the returned promise, set an in-flight flag to disable the button, and clear usernames in a .then/.finally.
| group.reload | ||
| group.users.count.should == 2 | ||
| group.name.should == 'fred' | ||
| end |
There was a problem hiding this comment.
🟡 Edge-case behavior from the deleted incremental tests was not migrated. The old suite had "succeeds silently when adding non-existent users" and "succeeds silently when removing non-members"; the new controller still implements those silent-skip paths (add_members skips unknown usernames via if user = User.find_by_username(username); remove_member's group.users.delete(user_id) is a no-op for a non-member id), but neither is exercised by the new .add_members / .remove_member specs. A regression that raised on missing users or non-members would pass CI undetected. Add specs mirroring the deleted ones: add a non-existent username and assert success + count unchanged; remove a non-member user_id and assert success + count unchanged.
| context ".remove_member" do | ||
|
|
||
| it "cannot remove members from automatic groups" do | ||
| xhr :put, :remove_member, group_id: 1, user_id: 42 |
There was a problem hiding this comment.
💡 Verb/route mismatch: this "cannot remove members from automatic groups" test calls xhr :put, :remove_member while the route registers only delete "members" (config/routes.rb) and the sibling success test correctly uses xhr :delete, :remove_member (line 125). Controller specs bypass routing, so the :put still invokes the action and asserts 422, but it would not catch a regression that gated remove_member behind request.delete? or a route/verb change. Use xhr :delete here to match the real route verb.
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Test 8
Problem
This PR refactors group membership management from a single overloaded update action (add/remove via params[:changes]) into dedicated add_members / remove_member endpoints, and introduces a paginated public members API plus an Ember pagination UI (prev/next, page counter) for the admin group screen.
Solution Reviewed
Backend: new Admin::GroupsController#add_members / #remove_member actions backed by new put/delete "members" member routes; GroupsController#members rewritten to return { members, meta: { total, limit, offset } } with limit/offset pagination. Frontend: Group model gains findMembers/addMembers/removeMember AJAX methods; admin-group controller gains currentPage/totalPages/next/previous; templates render a paginated member list with add/remove controls. Specs rewritten for the new action shapes.
Summary
The endpoint split and the new paginated members contract are sound, and the routing/param handling is correct (verified statically: bare routes inside resources :groups resolve to params[:group_id], matching both the new admin actions and the existing public GroupsController#members→find_group(:group_id) convention). One real blocking bug in the pagination math, plus several non-blocking gaps around unbounded pagination params and missing test coverage for the new public contract and dropped edge-case tests.
Files Reviewed
app/controllers/admin/groups_controller.rb— deeply reviewedapp/controllers/groups_controller.rb— deeply reviewedapp/assets/javascripts/discourse/models/group.js— deeply reviewedapp/assets/javascripts/admin/controllers/admin-group.js.es6— deeply reviewedapp/assets/javascripts/admin/templates/group.hbs— lightly reviewedconfig/routes.rb— deeply reviewed (routing semantics verified against in-repo convention)spec/controllers/admin/groups_controller_spec.rb— deeply reviewedspec/controllers/groups_controller_spec.rb— deeply reviewed (unchanged by PR; noted below)- SCSS / locale / template formatting changes — lightly reviewed
Verification
- Route param resolution verified statically against the in-repo convention:
config/routes.rb:271-272has the identical pattern (resources :groups do get 'members' end) and the existing publicGroupsController#membersreadsparams[:group_id]viafind_group(:group_id). The new adminadd_members/remove_memberreadingparams[:group_id]is correct; the standard REST actions (show/update/destroy) correctly useparams[:id]. No Ruby/JS runtime available in this environment; existing specs not executed.
Issues Found
See inline comments. Headline: 🔴 totalPages off-by-one breaks the pagination feature this PR introduces.
Additional finding (file unchanged by this PR)
🟡 spec/controllers/groups_controller_spec.rb:65 — new public pagination contract has zero passing coverage
The members describe block has no valid coverage for the new { members, meta } contract this PR introduces. The active it at this line ("calls posts_for and responds with JSON") actually calls xhr :get, :posts (line 67), never :members — a copy-paste of the posts spec. The only test that hits :members with body assertions is pending "ensures that membership can be paginated" (line 72), which is still pending and parses the response as a bare array (members.map{ |m| m['username'] }), a shape the PR no longer returns. Net: the new public pagination contract has zero passing coverage. Fix line 67 to :members and assert the members/meta keys, and un-pending + update the pagination spec to read JSON.parse(response.body)['members'].
Verdict
Recommend changes before merge — the pagination totalPages off-by-one is a real correctness bug in the PR's headline feature and is a one-line fix; the test-coverage gaps should be closed so the new public contract is actually guarded.
|
|
||
| totalPages: function() { | ||
| if (this.get("user_count") == 0) { return 0; } | ||
| return Math.floor(this.get("user_count") / this.get("limit")) + 1; |
There was a problem hiding this comment.
🔴 totalPages off-by-one. Math.floor(user_count / limit) + 1 overcounts by exactly one page whenever user_count is an exact multiple of limit. Example: 100 users, limit 50 → floor(100/50)+1 = 3, but the correct page count is 2.
Downstream impact: showingLast = propertyEqual("currentPage", "totalPages") (line 17) is false on the real last page (currentPage 2 vs totalPages 3), so the next guard if (this.get("showingLast")) { return; } (line 30) does not stop navigation — the user can click Next into a phantom page 3 whose fetch returns zero members, and the header reads 2/3 then 3/3.
Fix with Math.ceil, keeping the existing user_count == 0 guard for the 0-page case:
return Math.ceil(this.get("user_count") / this.get("limit"));| end | ||
|
|
||
| render_serialized(members.to_a, GroupUserSerializer) | ||
| limit = (params[:limit] || 50).to_i |
There was a problem hiding this comment.
🟡 The public members endpoint takes limit/offset straight from params with no clamping: limit = (params[:limit] || 50).to_i, offset = params[:offset].to_i. Negative values pass through unchanged and PostgreSQL rejects them (OFFSET must not be negative / LIMIT must not be negative) → unhandled 500 on ?offset=-1 or ?limit=-1. There is also no upper cap on limit, so ?limit=999999 forces loading/serializing the entire visible membership into memory (the old code capped automatic groups at 200). This endpoint is reachable by any client for visible groups. Clamp both, e.g.:
limit = [[params[:limit].to_i, 1].max, 200].min
offset = [params[:offset].to_i, 0].max| }, | ||
|
|
||
| addMembers: function() { | ||
| // TODO: should clear the input |
There was a problem hiding this comment.
🟡 The addMembers action is fire-and-forget: it discards the returned promise, never disables the Add button, and the // TODO: should clear the input right here confirms usernames is not cleared after add. A double-click or double-Enter fires two concurrent PUTs to /admin/groups/:id/members.json; because group_users has a unique index on [group_id, user_id], re-adding an existing member makes the second group.save fail with a unique-constraint violation — a swallowed 500 with no user feedback. The same missing .catch affects removeMember/addMembers/findMembers in the model: a failed DELETE/PUT is an unhandled rejection and the member list is never reloaded, so a failed remove leaves the member appearing present. Chain on the returned promise, set an in-flight flag to disable the button, clear usernames in a .then/.finally, and add a rejection handler consistent with the save action's bootbox.alert pattern.
| response.status.should == 422 | ||
| end | ||
|
|
||
| it "is able to add several members to a group" do |
There was a problem hiding this comment.
🟡 Edge-case behavior from the deleted incremental tests was not migrated. The old suite had "succeeds silently when adding non-existent users" and "succeeds silently when removing non-members"; the new controller still implements those silent-skip paths (add_members skips unknown usernames via if user = User.find_by_username(username); remove_member's group.users.delete(user_id) is a no-op for a non-member id), but neither is exercised by the new .add_members / .remove_member specs. A regression that raised on missing users or non-members would pass CI undetected. Add specs mirroring the deleted ones: add a non-existent username and assert success + count unchanged; remove a non-member user_id and assert success + count unchanged.
| context ".remove_member" do | ||
|
|
||
| it "cannot remove members from automatic groups" do | ||
| xhr :put, :remove_member, group_id: 1, user_id: 42 |
There was a problem hiding this comment.
💡 Verb/route mismatch: this "cannot remove members from automatic groups" test calls xhr :put, :remove_member while the route registers only delete "members" (config/routes.rb) and the sibling success test correctly uses xhr :delete, :remove_member (line 125). Controller specs bypass routing, so the :put still invokes the action and asserts 422, but it would not catch a regression that gated remove_member behind request.delete? or a route/verb change. Use xhr :delete here to match the real route verb.
ron-x5labs
left a comment
ron-x5labs
left a comment
There was a problem hiding this comment.
Code Review: Test 8
Problem
Refactor group membership management from a single overloaded update action (add/remove via params[:changes]) into dedicated add_members / remove_member admin endpoints, plus a paginated public members API ({ members, meta: { total, limit, offset } }) and an Ember prev/next pager for the admin group screen.
Solution Reviewed
Backend: Admin::GroupsController#add_members / #remove_member backed by new put/delete "members" member routes; GroupsController#members rewritten to paginate by limit/offset. Frontend: Discourse.Group gains findMembers/addMembers/removeMember; admin-group controller gains currentPage/totalPages/next/previous/addMembers/removeMember.
Summary
The new endpoints are wired incorrectly: both add_members and remove_member read params[:group_id], but the member routes supply params[:id], so every real add/remove request raises ActionController::ParameterMissing -> 500. The specs stay green only because they pass group_id: directly, bypassing routing. Beyond that, the pager has an off-by-one, the public members endpoint is unclamped, and re-adding an existing member raises RecordNotUnique. Recommend changes before merge.
Files Reviewed
app/controllers/admin/groups_controller.rb-- deeply reviewedapp/controllers/groups_controller.rb-- deeply reviewedconfig/routes.rb-- deeply reviewed (route scoping)app/assets/javascripts/discourse/models/group.js-- deeply reviewedapp/assets/javascripts/admin/controllers/admin-group.js.es6-- deeply reviewedapp/assets/javascripts/admin/routes/admin_group_route.js-- lightly reviewedapp/assets/javascripts/discourse/routes/group-members.js.es6-- lightly reviewedapp/assets/javascripts/admin/templates/group.hbs-- lightly reviewedapp/assets/javascripts/admin/templates/group_member.hbs-- lightly reviewedapp/assets/javascripts/admin/views/group-member.js.es6-- lightly reviewed (templateName convention matches siblings)app/assets/javascripts/discourse/templates/group/members.hbs-- lightly reviewedapp/assets/javascripts/discourse/templates/components/admin-group-selector.hbs-- trivial (whitespace)app/assets/javascripts/discourse/templates/user-selector-autocomplete.raw.hbs-- trivial (reindent)app/assets/stylesheets/common/admin/admin_base.scss-- lightly reviewed (style refactor)config/locales/client.en.yml-- lightly reviewed (i18n keys added)spec/controllers/admin/groups_controller_spec.rb-- deeply reviewed
Verification
- Static analysis (Rails routing semantics + caller URL + sibling-action convention + Ruby
Groupmodel +group_usersmigration) -- confirms the:group_id/:idmismatch, theRecordNotUniquepath, and the pager off-by-one. - Full test suite not run: this is a legacy Discourse-era app (Ember 1.x, RSpec
shouldsyntax) and the worktree has no DB/bundleboot configured; the controller specs are the ones giving false confidence here, so a green run would not add signal.
Issues Found
See inline comments. Three blocking (both new endpoints broken on real requests; pager off-by-one), four non-blocking (unclamped public pagination, non-idempotent add, fire-and-forget add/remove UX, findMembers contract regression), two suggestions (test verb mismatch, unmigrated edge-case specs).
Verdict
Recommend changes before merge -- the add/remove-member feature is non-functional on real requests due to the route/param mismatch; the green specs mask it.
| end | ||
|
|
||
| def add_members | ||
| group = Group.find(params.require(:group_id).to_i) |
There was a problem hiding this comment.
Blocking: the put "members" route is a bare route inside resources :groups, so Rails scopes it as a member route -- PUT /admin/groups/:id/members(.:format) -- meaning the path param is :id, not :group_id. The JS caller confirms this: Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {type: "PUT", ...}) (group.js:53). So params.require(:group_id) here raises ActionController::ParameterMissing on every real add request -> 500. The spec at line 103 passes group_id: directly, bypassing routing, which is why CI stays green. Sibling actions update/destroy already use params[:id] -- match them, and update the specs to pass id: instead of group_id:.
group = Group.find(params.require(:id).to_i)| else | ||
| group.destroy | ||
| def remove_member | ||
| group = Group.find(params.require(:group_id).to_i) |
There was a problem hiding this comment.
Blocking: same params[:group_id] vs :id mismatch as add_members. The delete "members" route is member-scoped (DELETE /admin/groups/:id/members), matching the JS call Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {type: "DELETE", ...}) (group.js:42), so the param is :id. params.require(:group_id) raises ActionController::ParameterMissing on every real removal -> 500; the spec at line 125 masks it by passing group_id: directly. Read the id from params[:id] to match the route and the update/destroy actions, and switch the spec to id:.
group = Group.find(params.require(:id).to_i)|
|
||
| totalPages: function() { | ||
| if (this.get("user_count") == 0) { return 0; } | ||
| return Math.floor(this.get("user_count") / this.get("limit")) + 1; |
There was a problem hiding this comment.
Blocking: Math.floor(user_count / limit) + 1 overcounts by one page whenever user_count is an exact multiple of limit (100 members, limit 50 -> 3 pages, but the correct count is 2). Since showingLast = currentPage == totalPages (line 17), the real last page is never reported as "last", so the next guard at line 30 does not fire and the user can page into a phantom empty page that the header then renders as 2/3 -> 3/3. Math.ceil is correct for both exact and inexact multiples (the user_count == 0 guard above still yields 0 pages):
return Math.ceil(this.get("user_count") / this.get("limit"));| end | ||
|
|
||
| render_serialized(members.to_a, GroupUserSerializer) | ||
| limit = (params[:limit] || 50).to_i |
There was a problem hiding this comment.
Non-blocking (security/robustness): limit/offset are taken straight from params with no clamping on this public (non-admin) endpoint reachable by any client for visible groups. ?offset=-1 or ?limit=-1 is passed through to Postgres, which rejects it (OFFSET/LIMIT must not be negative) -> unhandled 500; and there is no upper cap, so ?limit=999999 forces loading + serializing the entire visible membership into memory (the old code capped automatic groups at 200). Clamp both:
limit = [[params[:limit].to_i, 1].max, 200].min
offset = [params[:offset].to_i, 0].max|
|
||
| usernames.split(",").each do |username| | ||
| if user = User.find_by_username(username) | ||
| group.add(user) |
There was a problem hiding this comment.
Non-blocking (logic/edge-case): group.add(user) is self.users.push(user) (app/models/group.rb:272) -- it blindly appends a join record even when the user is already a member. group_users has a unique index on [group_id, user_id] (db/migrate/20130416004933_group_users.rb:9), so re-adding an existing member makes group.save raise ActiveRecord::RecordNotUnique -- a DB error, not a validation failure, so the if group.save branch never runs and it surfaces as a 500. If any username in the batch is already a member, the entire add fails. The replaced usernames= setter diffed (additions = expected - current, group.rb:248) and never touched existing members, so this is a behavior regression. Guard before adding:
group.add(user) unless group.group_users.exists?(user_id: user.id)| addMembers: function() { | ||
| // TODO: should clear the input | ||
| if (Em.isEmpty(this.get("usernames"))) { return; } | ||
| this.get("model").addMembers(this.get("usernames")); |
There was a problem hiding this comment.
Non-blocking: addMembers discards the returned promise, never disables the Add button, and leaves usernames uncleared (the // TODO on line 62 confirms the last point). A double-click or double-Enter fires two concurrent PUTs to /admin/groups/:id/members.json; combined with the unique index on group_users, the second request becomes a 500. The model's addMembers/removeMember/findMembers (group.js:51/40/20) register only a .then, so any failed PUT/DELETE is an unhandled rejection with no user feedback and no member-list reload -- a failed remove leaves the member looking present. Chain on the promise, set an in-flight flag to disable the button, and clear usernames in a .then/.finally.
|
|
||
| findMembers: function() { | ||
| if (Em.isEmpty(this.get('name'))) { return Ember.RSVP.resolve([]); } | ||
| if (Em.isEmpty(this.get('name'))) { return ; } |
There was a problem hiding this comment.
Non-blocking: for an empty group name this now returns bare undefined (return ;) where the previous implementation returned Ember.RSVP.resolve([]). That breaks the method's always-returns-a-promise contract -- any caller doing findMembers().then(...) (the pattern the old afterModel used) throws TypeError: Cannot read property 'then' of undefined. No current caller chains on it today (routes are fire-and-forget), so this is latent, but the contract regression is introduced by this PR. Restore a resolved promise:
if (Em.isEmpty(this.get('name'))) { return Ember.RSVP.resolve(); }| context ".remove_member" do | ||
|
|
||
| it "cannot remove members from automatic groups" do | ||
| xhr :put, :remove_member, group_id: 1, user_id: 42 |
There was a problem hiding this comment.
Suggestion: verb/route mismatch. This "cannot remove members from automatic groups" test calls xhr :put, :remove_member, but the route registers only delete "members" (config/routes.rb) and the sibling success test at line 125 correctly uses xhr :delete. Controller specs bypass routing, so the :put still invokes the action and asserts 422, but it would miss a regression that gated remove_member behind request.delete? or a route/verb change. Use :delete here to match the real route verb:
xhr :delete, :remove_member, group_id: 1, user_id: 42| response.status.should == 422 | ||
| end | ||
|
|
||
| it "is able to add several members to a group" do |
There was a problem hiding this comment.
Suggestion (test coverage gap): the deleted incremental specs ("succeeds silently when adding non-existent users" and "succeeds silently when removing non-members") were not migrated. The new controller still implements those silent-skip paths -- add_members skips unknown usernames via if user = User.find_by_username(username), and remove_member's group.users.delete(user_id) is a no-op for a non-member id -- but neither is exercised by the new .add_members / .remove_member specs. A regression that raised on missing users or non-members would pass CI undetected. Add specs mirroring the deleted ones: add a non-existent username and assert success + count unchanged; remove a non-member user_id and assert success + count unchanged.
5 participants
Test 8