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")); |
4 participants
Test 8