FIX: proper handling of group memberships #8
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a comprehensive refactoring of group membership management in the Discourse admin interface. The changes move away from a complex single-endpoint approach to dedicated REST endpoints for member operations, improving both API design and user experience.
Key architectural changes:
- Backend API refactoring: The
Admin::GroupsControllernow has dedicatedadd_membersandremove_membermethods instead of handling everything through a complexupdatemethod that used different HTTP verbs - Route restructuring: New RESTful routes (
DELETE /membersandPUT /members) replace the genericget "users"route - Frontend pagination: Both client and server now support proper pagination for group members with metadata (total count, limit, offset)
- Improved data flow: Routes no longer pre-fetch member data in
afterModelhooks, instead delegating loading to the model'sfindMembers()method - Template enhancements: The admin group template now shows paginated member lists with navigation controls and a dedicated "Add Members" section
Integration with existing codebase:
These changes build upon Discourse's existing Ember.js architecture and Rails conventions. The refactoring maintains backward compatibility while providing cleaner separation of concerns. The new pagination system integrates with existing serializers (GroupUserSerializer) and follows Discourse's established patterns for API responses with metadata. The frontend changes leverage existing components like user-selector and fa-icon while introducing new view classes (group-member.js.es6) that follow Ember's itemView pattern.
Template and styling updates:
CSS changes extract common form styling into shared selectors (.groups, .badges) and add specific styling for the new groups interface. New localization keys provide clearer UI text and member removal confirmation dialogs.
Confidence score: 2/5
• This PR introduces several race conditions and potential timing issues that could cause UI malfunctions
• Race conditions in route loading, missing error handling in async operations, and incomplete template implementations create instability risks
• Files requiring attention: app/assets/javascripts/discourse/routes/group-members.js.es6, app/assets/javascripts/admin/routes/admin_group_route.js, app/assets/javascripts/discourse/models/group.js, app/assets/javascripts/discourse/templates/components/admin-group-selector.hbs
16 files reviewed, 15 comments
| 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.
logic: HTTP method mismatch - test uses PUT but remove_member action expects DELETE
| xhr :put, :remove_member, group_id: 1, user_id: 42 | |
| xhr :delete, :remove_member, group_id: 1, user_id: 42 |
|
|
||
| context ".create" do | ||
|
|
||
| it "strip spaces on the group name" do |
There was a problem hiding this comment.
style: Grammar: should be "strips spaces from the group name"
| 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.
style: Grammar: should be "ignores name change on automatic group"
| setupController: function(controller, model) { | ||
| this.controllerFor('group').set('showing', 'members'); | ||
| controller.set("model", model); | ||
| model.findMembers(); |
There was a problem hiding this comment.
logic: The findMembers() call is now asynchronous and unhandled. The controller may not have member data immediately available, creating a race condition.
| controller.set('model', model); | ||
| controller.set('members', this.get('_members')); | ||
| controller.set("model", model); | ||
| model.findMembers(); |
There was a problem hiding this comment.
logic: This creates a race condition - findMembers() is async but the controller setup continues immediately. Templates expecting member data may render before it's available.
| }.property("limit", "offset", "user_count"), | ||
|
|
||
| totalPages: function() { | ||
| if (this.get("user_count") == 0) { return 0; } |
There was a problem hiding this comment.
style: Using == instead of === for comparison - should use strict equality
| @@ -1,3 +1 @@ | |||
|
|
|||
| <input type="text"> | |||
There was a problem hiding this comment.
logic: This template appears incomplete - the corresponding JavaScript component expects Select2 integration with multiple selection capabilities, but this template only contains a plain HTML input field with no Ember bindings or component integration.
| <a {{bind-attr class=":next showingLast:disabled"}} {{action "next"}}>{{fa-icon "fast-forward"}}</a> | ||
| </div> | ||
| <div class="ac-wrap clearfix"> | ||
| {{each member in members itemView="group-member"}} |
There was a problem hiding this comment.
style: The itemView syntax is deprecated in newer Ember versions - consider using {{#each}} with component syntax instead
|
|
||
| render_serialized(members.to_a, GroupUserSerializer) | ||
| limit = (params[:limit] || 50).to_i | ||
| offset = params[:offset].to_i |
There was a problem hiding this comment.
style: params[:offset].to_i may convert nil to 0, but consider using (params[:offset] || 0).to_i for explicit default handling like limit parameter
| @@ -0,0 +1 @@ | |||
| {{avatar member imageSize="small"}} {{member.username}} {{#unless automatic}}<a class='remove' {{action "removeMember" member}}>{{fa-icon "times"}}</a>{{/unless}} | |||
There was a problem hiding this comment.
logic: The automatic variable should be member.automatic or explicitly passed from the parent context, as the current reference may be ambiguous.
2 participants
Test 8