FIX: proper handling of group memberships#3
Conversation
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| 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.
Member pagination adds phantom final page
Medium Severity
totalPages uses Math.floor(user_count / limit) + 1, which overcounts when user_count is an exact multiple of limit. That makes showingLast false on the real last page and allows next to request offset == user_count, producing an empty page in admin-group.js.es6.
Additional Locations (1)
|
|
||
| total = group.users.count | ||
| members = group.users.order(:username_lower).limit(limit).offset(offset) | ||
|
|
There was a problem hiding this comment.
Group members page silently truncates results
Medium Severity
GroupsController#members now always applies pagination with a default limit of 50, but the public group-members route still loads members only once via model.findMembers() and exposes no paging controls. Large groups now show only the first 50 members in group/members.
Additional Locations (1)
|
|
||
| render_serialized(members.to_a, GroupUserSerializer) | ||
| limit = (params[:limit] || 50).to_i | ||
| offset = params[:offset].to_i |
There was a problem hiding this comment.
Unchecked pagination params can trigger server errors
Medium Severity
GroupsController#members now trusts params[:limit] and params[:offset] without bounds checks. Negative values flow into .limit/.offset, which can produce invalid SQL for PostgreSQL and return 500s. This became user-reachable for all groups after pagination was applied unconditionally.
2 participants
Test 8
Summary by CodeRabbit
Release Notes
New Features
Style
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#8
Note
Medium Risk
Touches both server and client group membership flows (new endpoints + changed JSON shape for member lists), so regressions could affect group administration and member visibility, but changes are localized and covered by controller specs.
Overview
Adds paginated group member loading by changing
GroupsController#membersto return{ members, meta }(withlimit/offset/total) and updatingDiscourse.Group#findMembersplus the group members routes/templates to consumemembersfrom the model.Revamps admin group membership management: the admin group screen now lists members with paging controls, supports adding users via a username input, and removing individual members with confirmation. Backend support is moved to explicit
PUT/DELETE /admin/groups/:id/membersendpoints (add_members/remove_member), with group create/update simplified to no longer accept usernames inline; specs and i18n strings are updated accordingly, along with minor admin styling tweaks.Written by Cursor Bugbot for commit 060cda7. Configure here.