FIX: proper handling of group memberships#3
Conversation
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. |
codeant-ai
Bot
added
the
size:L
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| 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.
Suggestion: The total page count calculation is off by one when the user count is an exact multiple of the page size, causing an extra non-existent page to be shown and enabling navigation to an empty members page. [off-by-one]
Severity Level: Major ⚠️
- ⚠️ Admin group members UI shows an extra empty page.
- ⚠️ Extra AJAX request for non-existent member page is made.| if (this.get("user_count") == 0) { return 0; } | |
| return Math.floor(this.get("user_count") / this.get("limit")) + 1; | |
| var userCount = this.get("user_count"), | |
| limit = this.get("limit"); | |
| if (userCount === 0 || !limit) { return 0; } | |
| return Math.ceil(userCount / limit); |
Steps of Reproduction ✅
1. Open the admin group page for an existing group via the Ember route
`Discourse.AdminGroupRoute` defined in
`app/assets/javascripts/admin/routes/admin_group_route.js:1-16`, which loads the group
model and calls `model.findMembers()` at line 13.
2. Ensure the group has a member count that is an exact multiple of the page size, for
example 100 members with default `limit: 50` as defined in the group model
`app/assets/javascripts/discourse/models/group.js:10`, and note that `findMembers()`
(lines 20-37) sets `user_count` and `limit` from `result.meta.total` and
`result.meta.limit`.
3. On initial load of the admin group members UI, the controller
`app/assets/javascripts/admin/controllers/admin-group.js.es6:6-14` computes `currentPage`
as `Math.floor(offset / limit) + 1` (with `offset` 0) and `totalPages` as
`Math.floor(user_count / limit) + 1`; with `user_count = 100` and `limit = 50` this yields
`totalPages = 3`, which is displayed in
`app/assets/javascripts/admin/templates/group.hbs:17` as `{{currentPage}}/{{totalPages}}`.
4. Click the "next" pagination action twice, which triggers the `next` handler in
`admin-group.js.es6:28-38` to set `offset` first to 50 and then to 100 (using
`Math.min(group.get("offset") + group.get("limit"), group.get("user_count"))` at line 33)
and call `group.findMembers()` each time; when `offset` reaches 100, the server returns an
empty `members` array (because there are no users beyond 100), but `currentPage` becomes 3
and `totalPages` remains 3, producing a third, empty page that the UI treats as the last
page even though only two full pages of members exist.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/assets/javascripts/admin/controllers/admin-group.js.es6
**Line:** 12:13
**Comment:**
*Off By One: The total page count calculation is off by one when the user count is an exact multiple of the page size, causing an extra non-existent page to be shown and enabling navigation to an empty members page.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| def create | ||
| group = Group.new | ||
| group.name = (params[:name] || '').strip | ||
| group.visible = params[:visible] == "true" |
There was a problem hiding this comment.
Suggestion: When creating a group, the alias level parameter sent by the client is ignored, so newly created groups always use the default alias level even if the admin selected a different value; this is inconsistent with the update action, which does honor alias_level, and will surprise users who set it during creation. The fix is to set group.alias_level in create using the same pattern as in update. [logic error]
Severity Level: Major ⚠️
- ⚠️ New group alias level ignores admin-selected value.
- ⚠️ Group alias behavior inconsistent between create and update.| group.visible = params[:visible] == "true" | |
| group.alias_level = params[:alias_level].to_i if params[:alias_level].present? |
Steps of Reproduction ✅
1. From the admin UI, create a group using the Ember controller at
`app/assets/javascripts/admin/controllers/admin-group.js.es6:67-81`, which calls the
model's `create()` method when no `id` is present.
2. The `Discourse.Group` model at `app/assets/javascripts/discourse/models/group.js:62-75`
builds the payload in `asJSON` (including `alias_level` at lines 62-67) and POSTs it to
`/admin/groups` in `create()` at lines 70-75.
3. The request is routed via `config/routes.rb:34-51` (admin namespace, `resources
:groups`) to `Admin::GroupsController#create` at
`app/controllers/admin/groups_controller.rb:22-32`.
4. In `create`, only `name` and `visible` are assigned (lines 23-25); `alias_level` from
params is never set, so the record uses the default `0` defined in
`db/migrate/20140102104229_add_alias_level_to_groups.rb:3`. When groups are later listed
via `index` at `groups_controller.rb:3-16` and serialized with `BasicGroupSerializer`
(includes `:alias_level` per `app/serializers/basic_group_serializer.rb:2`), the new
group's `alias_level` always appears as the default, regardless of what the admin selected
during creation.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/controllers/admin/groups_controller.rb
**Line:** 25:25
**Comment:**
*Logic Error: When creating a group, the alias level parameter sent by the client is ignored, so newly created groups always use the default alias level even if the admin selected a different value; this is inconsistent with the update action, which does honor `alias_level`, and will surprise users who set it during creation. The fix is to set `group.alias_level` in `create` using the same pattern as in `update`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| 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: The "cannot remove members from automatic groups" spec calls the remove_member action with a PUT request, even though the route is defined as DELETE, so the test does not exercise the real HTTP verb and could pass even if the DELETE route were misconfigured or broken. [logic error]
Severity Level: Major ⚠️
- ⚠️ remove_member automatic-group guard untested in controller specs.
- ⚠️ DELETE members route regressions may go unnoticed.| xhr :put, :remove_member, group_id: 1, user_id: 42 | |
| xhr :delete, :remove_member, group_id: 1, user_id: 42 |
Steps of Reproduction ✅
1. Note the admin group member routes in `config/routes.rb:45-51`, where `delete "members"
=> "groups#remove_member"` defines the DELETE route and `put "members" =>
"groups#add_members"` defines a separate PUT route.
2. Open the spec `spec/controllers/admin/groups_controller_spec.rb:112-120`, where the
`.remove_member` context contains the example `"cannot remove members from automatic
groups"` calling `xhr :put, :remove_member, group_id: 1, user_id: 42`.
3. Observe that a PUT to the `members` path is routed (per `routes.rb:49-50`) to
`Admin::GroupsController#add_members`
(`app/controllers/admin/groups_controller.rb:65-82`), which for automatic groups
immediately calls `can_not_modify_automatic` and returns HTTP 422.
4. When running this spec, the example at lines `114-117` receives a 422 response from
`add_members`, so it passes while never exercising the DELETE route or
`Admin::GroupsController#remove_member`
(`app/controllers/admin/groups_controller.rb:84-97`), meaning any misconfiguration or bug
in the DELETE path would go undetected.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** spec/controllers/admin/groups_controller_spec.rb
**Line:** 115:115
**Comment:**
*Logic Error: The "cannot remove members from automatic groups" spec calls the `remove_member` action with a PUT request, even though the route is defined as DELETE, so the test does not exercise the real HTTP verb and could pass even if the DELETE route were misconfigured or broken.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished running the review. |
2 participants
User description
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
CodeAnt-AI Description
Improve group management: paginated member lists, add/remove members, and safer automatic-group handling
What Changed
Impact
✅ Clearer group member lists with totals and pages✅ Fewer admin UI round-trips when loading large member lists✅ Clearer errors when attempting to modify automatic groups💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.