FIX: proper handling of group memberships#4
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors group membership handling to improve the API design and user interface for managing group members. The changes separate member addition and removal into distinct endpoints and update the frontend to properly handle paginated member lists.
Key Changes:
- Replaced bulk update operations with dedicated
add_membersandremove_memberendpoints - Implemented pagination for group member lists with proper offset/limit handling
- Refactored frontend controllers and routes to support the new member management workflow
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| app/controllers/admin/groups_controller.rb | Added dedicated add_members and remove_member actions, removed patch-based update logic |
| app/controllers/groups_controller.rb | Updated members action to return paginated results with metadata |
| app/assets/javascripts/discourse/models/group.js | Added addMembers and removeMember methods with pagination support |
| app/assets/javascripts/admin/controllers/admin-group.js.es6 | Added pagination controls and actions for member management |
| spec/controllers/admin/groups_controller_spec.rb | Reorganized tests to match new controller structure |
| config/routes.rb | Added routes for new member management endpoints |
| app/assets/javascripts/admin/routes/admin_group_route.js | Simplified route by removing afterModel hook |
| app/assets/javascripts/discourse/routes/group-members.js.es6 | Simplified route by removing afterModel hook |
| config/locales/client.en.yml | Added new translation keys for member management UI |
| app/assets/stylesheets/common/admin/admin_base.scss | Added styles for groups interface including pagination controls |
| app/assets/javascripts/admin/templates/group.hbs | Updated template to show paginated member list with add/remove controls |
| app/assets/javascripts/discourse/templates/group/members.hbs | Updated to use members property instead of model |
| app/assets/javascripts/admin/views/group-member.js.es6 | New view for individual group member items |
| app/assets/javascripts/admin/templates/group_member.hbs | New template for rendering individual group members |
| app/assets/javascripts/discourse/templates/user-selector-autocomplete.raw.hbs | Minor formatting improvements |
| app/assets/javascripts/discourse/templates/components/admin-group-selector.hbs | Removed empty lines |
|
|
||
| render_serialized(members.to_a, GroupUserSerializer) | ||
| limit = (params[:limit] || 50).to_i | ||
| offset = params[:offset].to_i |
There was a problem hiding this comment.
The offset parameter should have a default value like the limit parameter. Without a default, if params[:offset] is nil, to_i returns 0, but this is implicit. For consistency with the limit parameter handling, use (params[:offset] || 0).to_i.
| offset = params[:offset].to_i | |
| offset = (params[:offset] || 0).to_i |
|
|
||
| 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.
The return statement is missing a value. This should explicitly return a rejected promise or resolved empty promise for consistency. Consider return Ember.RSVP.resolve(); or handle the empty case in the caller.
| if (Em.isEmpty(this.get('name'))) { return ; } | |
| if (Em.isEmpty(this.get('name'))) { return Ember.RSVP.resolve(); } |
| }).then(function() { | ||
| // reload member list | ||
| self.findMembers(); | ||
| }) |
There was a problem hiding this comment.
Missing semicolon at the end of the statement. JavaScript allows this through automatic semicolon insertion, but for consistency with the rest of the codebase (see line 48 and 84), a semicolon should be added.
| }) | |
| }); |
| }, | ||
|
|
||
| destroy: function(){ | ||
| if (!this.get('id')) { return }; |
There was a problem hiding this comment.
The return statement has an empty expression followed by a semicolon. This should either be return; without the semicolon inside the block, or the semicolon should be removed for consistency with JavaScript conventions.
| if (!this.get('id')) { return }; | |
| if (!this.get('id')) { return; } |
| if (this.get("user_count") == 0) { return 0; } | ||
| return Math.floor(this.get("offset") / this.get("limit")) + 1; | ||
| }.property("limit", "offset", "user_count"), | ||
|
|
||
| totalPages: function() { | ||
| if (this.get("user_count") == 0) { return 0; } |
There was a problem hiding this comment.
Use strict equality operator === instead of == for type-safe comparisons. This is a JavaScript best practice to avoid type coercion issues.
| if (this.get("user_count") == 0) { return 0; } | |
| return Math.floor(this.get("offset") / this.get("limit")) + 1; | |
| }.property("limit", "offset", "user_count"), | |
| totalPages: function() { | |
| if (this.get("user_count") == 0) { return 0; } | |
| if (this.get("user_count") === 0) { return 0; } | |
| return Math.floor(this.get("offset") / this.get("limit")) + 1; | |
| }.property("limit", "offset", "user_count"), | |
| totalPages: function() { | |
| if (this.get("user_count") === 0) { return 0; } |
| }.property("limit", "offset", "user_count"), | ||
|
|
||
| totalPages: function() { | ||
| if (this.get("user_count") == 0) { return 0; } |
There was a problem hiding this comment.
Use strict equality operator === instead of == for type-safe comparisons. This is a JavaScript best practice to avoid type coercion issues.
| if (this.get("user_count") == 0) { return 0; } | |
| if (this.get("user_count") === 0) { return 0; } |
Test 8
Replicated from ai-code-review-evaluation/discourse-copilot#8