FIX: proper handling of group memberships#3
Conversation
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
This PR refactors group member management in the admin interface by implementing pagination and separating member operations from group CRUD operations. The changes introduce dedicated API endpoints for adding and removing members, replacing the previous patch-based approach. The frontend now supports paginated member lists with navigation controls, improved form accessibility with proper labels, and a new group-member component for rendering individual members. The refactoring consolidates data fetching logic, moves username handling from save operations to dedicated member management methods, and applies consistent pagination (50 members per page) across all groups. Backend changes include simplified controller methods, RESTful routing, and enhanced API responses with metadata.
Changes
| File(s) | Summary |
|---|---|
app/assets/javascripts/admin/controllers/admin-group.js.es6 |
Implemented pagination for group members with currentPage, totalPages, showingFirst, and showingLast computed properties, plus next/previous navigation actions. Added addMembers and removeMember actions. Removed members and usernames properties, updated save action to use simplified methods without username handling. |
app/assets/javascripts/admin/routes/admin_group_route.jsapp/assets/javascripts/discourse/routes/group-members.js.es6 |
Refactored route lifecycle by removing afterModel hook and moving member fetching to setupController. Eliminated intermediate _members property storage and changed from synchronous to asynchronous loading pattern. Updated quote consistency. |
app/assets/javascripts/admin/templates/group.hbs |
Restructured template with semantic <form> element, added pagination controls with previous/next navigation and member count display. Enhanced accessibility with explicit <label> elements and for attributes. Conditionally show member addition section for non-automatic groups. Updated button styling and replaced Font Awesome tags with {{fa-icon}} helpers. |
app/assets/javascripts/admin/templates/group_member.hbsapp/assets/javascripts/admin/views/group-member.js.es6 |
Created new group member component with template displaying avatar, username, and conditional remove button. View extends Discourse.View with 'item' CSS class. |
app/assets/javascripts/discourse/models/group.js |
Added limit, offset, and user_count properties for pagination. Modified findMembers() to accept pagination parameters and return metadata. Added addMembers() and removeMember() methods with automatic member list reload. Simplified asJSON() to exclude usernames, split create() and save() methods to remove username handling. |
app/assets/javascripts/discourse/templates/group/members.hbs |
Updated iteration variable from 'model' to 'members' for semantic clarity. |
app/assets/javascripts/discourse/templates/components/admin-group-selector.hbsapp/assets/javascripts/discourse/templates/user-selector-autocomplete.raw.hbs |
Applied code formatting improvements: removed extraneous blank lines and improved indentation consistency. No functional changes. |
app/assets/stylesheets/common/admin/admin_base.scss |
Extracted common form styling to shared selector (.groups, .badges) to reduce duplication. Added .groups-specific styles for autocomplete wrapper, pagination controls, and add button. |
app/controllers/admin/groups_controller.rb |
Consolidated update logic into single method, removing dual-method approach. Extracted member management into dedicated add_members and remove_member endpoints. Simplified create method to accept flat parameters. Standardized SQL ILIKE to uppercase. |
app/controllers/groups_controller.rb |
Refactored members action to apply consistent pagination (default limit 50) to all groups. Changed response from serialized array to structured JSON with metadata (total count, limit, offset). Fixed indentation and updated order syntax. Removed TODO comment. |
config/locales/client.en.yml |
Updated user selector placeholder from 'add users' to 'enter username'. Added translation keys: 'delete_member_confirm', 'name', 'add', and 'add_members'. |
config/routes.rb |
Removed GET 'users' endpoint. Added DELETE 'members' (remove_member) and PUT 'members' (add_members) endpoints for RESTful member management. |
spec/controllers/admin/groups_controller_spec.rb |
Reorganized tests into logical context blocks (.index, .create, .update, .destroy, .refresh_automatic_groups, .add_members, .remove_member). Added tests for name stripping, automatic group protection, and dedicated member management endpoints. Removed incremental update tests. |
Sequence Diagram
This diagram shows the interactions between components:
sequenceDiagram
participant User
participant Controller as AdminGroupController
participant Model as Group Model
participant Bootbox as Bootbox Dialog
Note over Controller,Model: Pagination Flow
User->>Controller: next()
Controller->>Controller: Check showingLast
alt not showing last page
Controller->>Model: get("offset"), get("limit"), get("user_count")
Controller->>Model: set("offset", new offset)
Controller->>Model: findMembers()
Model-->>Controller: Updated members
end
User->>Controller: previous()
Controller->>Controller: Check showingFirst
alt not showing first page
Controller->>Model: get("offset"), get("limit")
Controller->>Model: set("offset", new offset)
Controller->>Model: findMembers()
Model-->>Controller: Updated members
end
Note over Controller,Model: Member Management Flow
User->>Controller: addMembers()
Controller->>Controller: Check usernames not empty
alt usernames provided
Controller->>Model: addMembers(usernames)
Model-->>Controller: Members added
end
User->>Controller: removeMember(member)
Controller->>Bootbox: confirm(message)
Bootbox-->>User: Show confirmation dialog
User->>Bootbox: Confirm
alt user confirms
Bootbox-->>Controller: confirm = true
Controller->>Model: removeMember(member)
Model-->>Controller: Member removed
end
Note over Controller,Model: Save Flow (Simplified)
User->>Controller: save()
Controller->>Controller: set("disableSave", true)
Controller->>Model: get("id")
alt group exists (has id)
Controller->>Model: save()
Model-->>Controller: Save complete
else new group
Controller->>Model: create()
Model-->>Controller: Create complete
Controller->>Controller: Add to adminGroups controller
end
Controller->>Controller: set("disableSave", false)
🔗 Cross-Repository Impact Analysis
Enable automatic detection of breaking changes across your dependent repositories. → Set up now
Learn more about Cross-Repository Analysis
What It Does
- Automatically identifies repositories that depend on this code
- Analyzes potential breaking changes across your entire codebase
- Provides risk assessment before merging to prevent cross-repo issues
How to Enable
- Visit Settings → Code Management
- Configure repository dependencies
- Future PRs will automatically include cross-repo impact analysis!
Benefits
- 🛡️ Prevent breaking changes across repositories
- 🔍 Catch integration issues before they reach production
- 📊 Better visibility into your multi-repo architecture
Install the extension
Note for Windsurf
Please change the default marketplace provider to the following in the windsurf settings:Marketplace Extension Gallery Service URL: https://marketplace.visualstudio.com/_apis/public/gallery
Marketplace Gallery Item URL: https://marketplace.visualstudio.com/items
Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts below
Emoji Descriptions:
⚠️ Potential Issue - May require further investigation.- 🔒 Security Vulnerability - Fix to ensure system safety.
- 💻 Code Improvement - Suggestions to enhance code quality.
- 🔨 Refactor Suggestion - Recommendations for restructuring code.
- ℹ️ Others - General comments and information.
Interact with the Bot:
- Send a message or request using the format:
@entelligenceai + *your message*
Example: @entelligenceai Can you suggest improvements for this code?
- Help the Bot learn by providing feedback on its responses.
@entelligenceai + *feedback*
Example: @entelligenceai Do not comment on `save_auth` function !
Also you can trigger various commands with the bot by doing
@entelligenceai command
The current supported commands are
config- shows the current configretrigger_review- retriggers the review
More commands to be added soon.
|
|
||
| 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.
Correctness: Math.floor(...)+1 overcounts when user_count is an exact multiple of limit (e.g., 20/10 -> 3). This makes showingLast false on the real last page and can allow navigation to an empty page. Use Math.ceil instead.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
File: app/assets/javascripts/admin/controllers/admin-group.js.es6. At the totalPages computed property (line with Math.floor(user_count/limit)+1), replace the formula with Math.ceil(user_count/limit) to avoid overcounting when user_count is an exact multiple of limit. Keep the existing zero-check.
| if (group.get('id')) { | ||
| promise = group.saveWithUsernames(this.get('usernames')); | ||
| promise = group.save(); | ||
| } else { | ||
| promise = group.createWithUsernames(this.get('usernames')).then(function() { | ||
| promise = group.create().then(function() { |
There was a problem hiding this comment.
Correctness: The change to group.save() and group.create() (lines 88, 90) omits the usernames argument previously provided by saveWithUsernames and createWithUsernames. This prevents members from being associated with the group during persistence. Restore the passing of this.get('usernames') to ensure members are saved.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
File: app/assets/javascripts/admin/controllers/admin-group.js.es6 (lines 74-77). The diff removed save/create variants that include usernames. Restore passing usernames (use saveWithUsernames/createWithUsernames or call addMembers after save/create) so selected members are persisted when saving/creating a group.
| controller.set("model", model); | ||
| model.findMembers(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Correctness: The removal of controller.set('members', ...) in setupController results in the members property remaining undefined on the controller. The new model.findMembers() call discards the promise result. This breaks templates and logic relying on the members collection.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
File: app/assets/javascripts/admin/routes/admin_group_route.js (around the setupController change). The diff removed afterModel and now calls model.findMembers() without using the result, so controller.members is never set. Update setupController to return model.findMembers().then(...) and set controller.members, or reintroduce afterModel to preserve the previous behavior.
| asJSON: function() { | ||
| return { | ||
| name: this.get('name'), | ||
| alias_level: this.get('alias_level'), | ||
| visible: !!this.get('visible') | ||
| }; |
There was a problem hiding this comment.
Correctness: The asJSON method (line 64) removes the group root key, breaking the API contract for create and save. The /admin/groups endpoints expect attributes to be nested under a group key, as seen in the previous implementation. Restore the { group: { ... } } wrapper.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
File: app/assets/javascripts/discourse/models/group.js (lines 62-67). The diff removed the `group` wrapper from `asJSON`, but `create`/`save` send this payload directly to `/admin/groups`, which previously expected `{ group: {...} }`. Restore the wrapper in `asJSON` or confirm/update the API contract. Apply the suggested diff to re-wrap the payload.
| group = Group.find(params[:id].to_i) | ||
|
|
||
| group.alias_level = payload[:alias_level].to_i if payload[:alias_level].present? | ||
| group.visible = payload[:visible] == "true" | ||
|
|
||
| if group.automatic | ||
| # group rename & membership changes are ignored/prohibited for automatic groups | ||
| else | ||
| group.usernames = payload[:usernames] if payload[:usernames] | ||
| group.name = payload[:name] if payload[:name] | ||
| end | ||
| group.alias_level = params[:alias_level].to_i if params[:alias_level].present? | ||
| group.visible = params[:visible] == "true" |
There was a problem hiding this comment.
Correctness: The update method breaks the API contract by moving attributes from params[:group] to the top-level params. Existing clients sending nested payloads will fail to update name and alias_level. Additionally, group.visible = params[:visible] == "true" will incorrectly set the group to hidden if the parameter is nested or missing (since nil == "true" is false). Revert to using params[:group] and ensure visible is only updated if the key is present.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
File: app/controllers/admin/groups_controller.rb (update method). The update action now reads alias_level/visible/name directly from params, but the previous contract used params[:group]. This breaks existing clients. Modify update to accept both shapes by using payload = params[:group] || params and read fields from payload; keep automatic-group name guard.
| limit = (params[:limit] || 50).to_i | ||
| offset = params[:offset].to_i | ||
|
|
||
| total = group.users.count | ||
| members = group.users.order(:username_lower).limit(limit).offset(offset) |
There was a problem hiding this comment.
Correctness: The members method breaks the API contract by changing the response from a JSON array to an object and reducing the default limit to 50 (previously 'all' for manual groups and 200 for automatic). This will cause parsing failures and data truncation for existing clients. Restore the previous response format and conditional pagination logic.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `app/controllers/groups_controller.rb` around lines 22-26, pagination is now always applied with a default limit of 50. Restore backward compatibility by applying `limit/offset` only when explicitly requested (or only for `group.automatic?`), so non‑automatic groups still return the full list. Update `members` and meta fields accordingly.
| controller.set("model", model); | ||
| model.findMembers(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Correctness: The removal of controller.set('members', ...) in setupController results in the members property remaining undefined on the controller. The new model.findMembers() call discards the promise result. This breaks templates and logic relying on the members collection.
Affected Locations:
- app/assets/javascripts/admin/routes/admin_group_route.js:15-15
- app/assets/javascripts/discourse/routes/group-members.js.es6:12-12
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
File: app/assets/javascripts/admin/routes/admin_group_route.js (around the setupController change). The diff removed afterModel and now calls model.findMembers() without using the result, so controller.members is never set. Update setupController to return model.findMembers().then(...) and set controller.members, or reintroduce afterModel to preserve the previous behavior.
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
EntelligenceAI PR Summary
This PR refactors group member management by implementing pagination and separating member operations into dedicated API endpoints.