FIX: proper handling of group memberships#8
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors group membership handling to improve the user interface and API design for managing group members. The changes transition from a single update endpoint to dedicated endpoints for adding and removing members, along with significant UI improvements for group administration.
- Refactored group membership API to use dedicated
add_membersandremove_memberendpoints instead of patch-based updates - Enhanced group member display with pagination and individual member management controls
- Improved admin UI with better form layouts and member management interface
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 | Refactored controller to use dedicated member management endpoints and simplified group operations |
| config/routes.rb | Added new routes for member addition and removal operations |
| spec/controllers/admin/groups_controller_spec.rb | Updated tests to reflect new API structure and member management functionality |
| app/controllers/groups_controller.rb | Enhanced members endpoint with pagination metadata |
| config/locales/client.en.yml | Added new localization strings for member management UI |
| app/assets/javascripts/discourse/models/group.js | Refactored group model to support new member management methods and pagination |
| Multiple template/view files | Updated UI templates to support new member management interface with pagination |
| app/assets/stylesheets/common/admin/admin_base.scss | Added styling for new group management interface |
| if group.save | ||
| render_serialized(group, BasicGroupSerializer) | ||
| render json: success_json | ||
| else | ||
| render_json_error group | ||
| render_json_error(group) | ||
| end |
There was a problem hiding this comment.
Calling save after add is unnecessary since the add method already persists the changes to the database. This could result in duplicate save operations.
| if group.save | ||
| render json: success_json | ||
| else | ||
| render_json_error(group) | ||
| end |
There was a problem hiding this comment.
Calling save after users.delete is unnecessary since the delete operation already persists the changes to the database. This could result in duplicate save operations.
| if group.save | |
| render json: success_json | |
| else | |
| render_json_error(group) | |
| end | |
| render json: success_json |
| }, | ||
|
|
||
| destroy: function(){ | ||
| if (!this.get('id')) { return }; |
There was a problem hiding this comment.
Missing return value for early exit. The function should return a resolved promise or undefined explicitly to maintain consistent return types.
| if (!this.get('id')) { return }; | |
| if (!this.get('id')) { return Promise.resolve(); } |
| setupController: function(controller, model) { | ||
| this.controllerFor('group').set('showing', 'members'); | ||
| controller.set("model", model); | ||
| model.findMembers(); |
There was a problem hiding this comment.
The findMembers() method returns a promise but the return value is not handled. This could lead to timing issues if the controller expects the members to be loaded.
| setupController: function(controller, model) { | |
| this.controllerFor('group').set('showing', 'members'); | |
| controller.set("model", model); | |
| model.findMembers(); | |
| async setupController(controller, model) { | |
| this.controllerFor('group').set('showing', 'members'); | |
| controller.set("model", model); | |
| try { | |
| await model.findMembers(); | |
| } catch (error) { | |
| console.error("Failed to load members:", error); | |
| } |
|
|
||
| context ".create" do | ||
|
|
||
| it "strip spaces on the group name" do |
There was a problem hiding this comment.
Grammar error: should be "strips spaces from the group name" to match the action being tested.
| it "strip spaces on the group name" do | |
| it "strips spaces from the group name" do |
| Group.expects(:refresh_automatic_groups!).returns(true) | ||
| context ".update" do | ||
|
|
||
| it "ignore name change on automatic group" do |
There was a problem hiding this comment.
Grammar error: should be "ignores name change on automatic group" to match the action being tested.
| it "ignore name change on automatic group" do | |
| it "ignores name change on automatic group" do |
|
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. |
Test 8