FIX: proper handling of group memberships #3

akshayutture-augment · 2025-11-14T23:41:35Z

Test 8

Summary by CodeRabbit

New Features
- Added pagination for group member lists with navigation controls
- Introduced member removal functionality with confirmation dialogs
- Added member addition interface to groups
Bug Fixes
- Improved member management workflow for groups
UI/UX Improvements
- Redesigned group management form layout
- Enhanced member display with avatars and usernames
- Updated form styling and controls for better usability

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Replicated from ai-code-review-evaluation/discourse-coderabbit#8

akshayutture-augment · 2025-11-14T23:44:38Z

@coderabbitai review

akshayutture-augment · 2025-11-22T00:02:42Z

@coderabbitai review

coderabbitai · 2025-11-22T00:02:47Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2025-11-22T00:03:03Z

Walkthrough

Group management system refactored to use pagination for member listing and dedicated REST endpoints. Member operations (add/remove) separated from group save flow. Admin group form template restructured with improved layout. Backend controllers updated with explicit create/update/destroy handlers.

Changes

Cohort / File(s)	Summary
Admin Group Controller & Route `app/assets/javascripts/admin/controllers/admin-group.js.es6`, `app/assets/javascripts/admin/routes/admin_group_route.js`	Controller updated with pagination helpers (currentPage, totalPages, showingFirst, showingLast), new actions for navigation (next/previous) and member operations (removeMember, addMembers). Removed members property; added usernames property. Updated save/create to use group.save()/group.create() instead of saveWithUsernames. Removed afterModel hook from route; setupController now invokes findMembers directly.
Admin Group Templates & Views `app/assets/javascripts/admin/templates/group.hbs`, `app/assets/javascripts/admin/templates/group_member.hbs`, `app/assets/javascripts/admin/views/group-member.js.es6`	New group_member template and view introduced for rendering individual members with remove control. Main group form restructured: wrapped in form element, member section added with pagination controls when id present, add-members section conditional on non-automatic groups. Button layout reorganized with save/destroy in dedicated container.
Public Group Routes & Templates `app/assets/javascripts/discourse/routes/group-members.js.es6`, `app/assets/javascripts/discourse/templates/group/members.hbs`	Route's afterModel removed; setupController expanded to accept model and call findMembers(). Template now iterates over members instead of model.
Group Model `app/assets/javascripts/discourse/models/group.js`	Added pagination properties (limit, offset) and user_count. Refactored findMembers to use paginated /groups/{name}/members.json endpoint. New methods: create(), save(), destroy(), removeMember(member), addMembers(usernames). Removed createWithUsernames/saveWithUsernames. Updated asJSON to return flat object.
Backend Admin Controller `app/controllers/admin/groups_controller.rb`	Consolidated REST endpoints with explicit create, update, destroy methods. Added refresh_automatic_groups, add_members, and remove_member actions. Update now conditionally renames groups (skipped for automatic). All modify operations guarded against automatic groups with can_not_modify_automatic response.
Backend Public Controller `app/controllers/groups_controller.rb`	Members action refactored to use fixed pagination (limit 50, offset from params), compute total members, order by username_lower, return JSON with members and meta object containing total/limit/offset.
Routing & Localization `config/routes.rb`, `config/locales/client.en.yml`	Removed GET /admin/groups/users. Added DELETE /admin/groups/members (remove_member) and PUT /admin/groups/members (add_members). Updated selector placeholder from "add users" to "enter username"; added keys for delete_member_confirm, name, add, add_members.
Styling `app/assets/stylesheets/common/admin/admin_base.scss`	Extracted form-horizontal styling rules from section.details into shared block for .groups and .badges. Added new "Groups area" styling for .ac-wrap items, navigation controls (.next/.previous with disabled states), and .btn.add alignment.
Supporting Templates `app/assets/javascripts/discourse/templates/components/admin-group-selector.hbs`, `app/assets/javascripts/discourse/templates/user-selector-autocomplete.raw.hbs`	Removed text input element from admin-group-selector. Minor whitespace/indentation adjustments in user-selector-autocomplete.
Tests `spec/controllers/admin/groups_controller_spec.rb`	Reorganized tests into action-specific contexts (index, create, update, destroy, refresh_automatic_groups, add_members, remove_member). Added tests for name trimming on create, automatic group protection, and member add/remove functionality. Removed legacy patch/put incremental test scenarios.

Sequence Diagram

sequenceDiagram
    participant User as Admin User
    participant UI as Admin Group UI
    participant Ctrl as Admin Controller
    participant Model as Group Model
    participant API as Backend API

    rect rgb(200, 230, 255)
    note over User,API: Group Member Pagination
    User->>UI: View group members (page 1)
    UI->>Ctrl: findMembers()
    Ctrl->>Model: findMembers()
    Model->>API: GET /groups/{name}/members.json?limit=50&offset=0
    API-->>Model: members[], user_count, limit, offset
    Model->>Model: Update pagination state
    end

    rect rgb(200, 230, 255)
    note over User,API: Navigate to Next Page
    User->>UI: Click next
    UI->>Ctrl: next() action
    Ctrl->>Model: findMembers() with updated offset
    Model->>API: GET /groups/{name}/members.json?offset=50
    API-->>Model: next batch of members
    end

    rect rgb(240, 200, 220)
    note over User,API: Remove Member
    User->>UI: Click remove on member
    UI->>Ctrl: removeMember confirmation
    Ctrl->>Model: removeMember(member)
    Model->>API: DELETE /admin/groups/{id}/members.json
    API-->>Model: success
    Model->>Model: findMembers() to refresh
    end

    rect rgb(240, 200, 220)
    note over User,API: Add Members
    User->>UI: Submit usernames in form
    UI->>Ctrl: addMembers() action
    Ctrl->>Model: addMembers(usernames)
    Model->>API: PUT /admin/groups/{id}/members.json
    API-->>Model: success
    Model->>Model: findMembers() to refresh
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Group model refactoring (group.js): Replace of save/create/destroy patterns with new REST-based methods; pagination logic in findMembers requires careful verification of API contract and member serialization.
Admin controller flow (admin-group.js.es6): New pagination helpers, member navigation, and action handlers need validation of integration with model and template binding.
Template restructuring (group.hbs): Form layout reorganization and conditional sections for automatic groups; member list iteration must align with paginated data flow.
Backend controller changes (admin/groups_controller.rb): New endpoints and automatic-group guards across multiple actions; error handling consistency across create/update/destroy/add_members/remove_member.
Route coordination: Verify afterModel removal and setupController changes across both admin and public routes propagate state correctly.

Poem

🐰 Pagination hops from page to page,
Members added, removed with sage,
Forms now flat, no nested fuss,
REST endpoints carry us!
Automatic guards stand firm and true, 🛡️
Groups now manage cleaner, anew!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'FIX: proper handling of group memberships' accurately summarizes the main changes in this PR, which comprehensively refactor group membership management across controllers, routes, models, and templates.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch group-dm-user-addition-post

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

app/assets/javascripts/discourse/templates/group/members.hbs (1)
1-21: Fix stray </div> and align the loop/indentation

Two issues in this template section:

There’s a </div> (Line 19) with no corresponding opening <div>, nested inside a <table>. This produces invalid markup and may cause browsers to restructure the DOM unexpectedly.

The {{#if}} still guards on model while the loop iterates members. That’s harmless if model is always truthy when members is populated, but a more direct guard on members (or members.length) would be clearer.

Suggested fix:
-{{#if model}}
+{{#if members}}
   <table class='group-members'>
@@
-  {{#each m in members}}
-  <tr>
+  {{#each m in members}}
+    <tr>
@@
-    </tr>
-    </div>
-  {{/each}}
+    </tr>
+  {{/each}}
 </table>
 {{/if}}
The key part is removing the stray </div>; the {{#if members}} change is optional but improves readability.

🧹 Nitpick comments (13)

app/controllers/groups_controller.rb (1)
22-35: Consider clamping limit/offset to safe bounds

The pagination logic looks good and the meta payload is very useful. Two small robustness tweaks would help:

Guard against pathological limit values from params (e.g. very large numbers) to avoid heavy queries.

Ensure offset is never negative before calling offset(offset) to avoid adapter/DB complaints on bad input.

Example tweak:
-    limit = (params[:limit] || 50).to_i
-    offset = params[:offset].to_i
+    limit = (params[:limit] || 50).to_i
+    limit = 50 if limit <= 0
+    limit = [limit, 200].min
+
+    offset = params[:offset].to_i
+    offset = 0 if offset < 0
app/assets/stylesheets/common/admin/admin_base.scss (2)

164-182: Shared .form-horizontal styles for .groups/.badges look reasonable

Centralizing label weight, spacing, and control widths for .groups/.badges forms will keep those sections consistent and replaces more ad‑hoc rules elsewhere. Given .badges .form-horizontal has an additional block later for badge‑specific tweaks, this layered approach is fine.

If you start seeing subtle conflicts, consider extracting a reusable mixin for this layout instead of repeating .form-horizontal rules in multiple places.

462-480: Groups area styles appear consistent, but rely heavily on !important

The dedicated .groups rules for .ac-wrap, pagination links, and .btn.add align with the new UI and should help override third‑party/component defaults.

If possible, consider reducing the use of !important (e.g. by tightening selector specificity) so these overrides remain easier to reason about in the long term.
app/assets/javascripts/discourse/models/group.js (3)
20-37: Paginated findMembers implementation looks sound

The guard on an empty name, clamping offset to [0, user_count], and updating user_count/limit/offset/members from the server response all look correct and match the new /groups/:name/members.json API.

One small improvement would be to make it explicit that members is always an array of Discourse.User instances, e.g. via a brief comment or docstring, since callers may rely on that shape.

40-49: Consider returning the findMembers() promise from the then callbacks

removeMember and addMembers both refresh the member list in a .then, but they don’t return the findMembers() promise from inside the callback. That means callers only know about the completion of the add/remove request, not the subsequent reload.

To allow clean chaining (group.addMembers(...).then(...) after the list is fully refreshed), you can return the findMembers() promise:
-  removeMember: function(member) {
+  removeMember: function(member) {
     var self = this;
     return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
       type: "DELETE",
       data: { user_id: member.get("id") }
-    }).then(function() {
-      // reload member list
-      self.findMembers();
-    });
+    }).then(function() {
+      // reload member list
+      return self.findMembers();
+    });
   },

   addMembers: function(usernames) {
     var self = this;
     return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
       type: "PUT",
       data: { usernames: usernames }
-    }).then(function() {
-      // reload member list
-      self.findMembers();
-    })
+    }).then(function() {
+      // reload member list
+      return self.findMembers();
+    });
   },
Also applies to: 51-60

62-84: Instance asJSON/create/save/destroy APIs fit the new REST endpoints

The flattened asJSON payload (name, alias_level, visible) and the RESTful instance methods for create/save/destroy align with the described backend changes.

Two minor nits to consider:

create/destroy as instance method names can be slightly confusing alongside the class‑level Discourse.Group.create(...); a more explicit naming like createRemote/destroyRemote would be clearer.

In destroy, use an explicit return; to avoid style warnings:
-  destroy: function(){
-    if (!this.get('id')) { return };
+  destroy: function(){
+    if (!this.get('id')) { return; }
     return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"});
   },
These are stylistic/clarity tweaks; behavior is otherwise fine.
app/assets/javascripts/admin/templates/group_member.hbs (1)

1-1: Confirm automatic binding source and consider accessible label on remove link

The {{#unless automatic}} guard assumes automatic is available from the outer controller; if it isn’t, the remove link may show for automatic groups. Also, the icon-only remove anchor lacks any text/aria label, which is mildly problematic for screen readers; consider adding an accessible label or title attribute.

app/assets/javascripts/admin/routes/admin_group_route.js (1)

11-14: Member loading in setupController is fine; set("model", model) is redundant

Calling model.findMembers() here is consistent with the rest of the flow. The explicit controller.set("model", model) merely repeats Ember’s default setupController behavior; you could drop it for brevity if there’s no special reason to keep it.
spec/controllers/admin/groups_controller_spec.rb (2)
51-52: Avoid relying on hard‑coded automatic group id (1) in specs

Several tests assume that Group.find(1) / group_id: 1 refers to an automatic group (Lines 51-52, 93-96, 115-116). That couples the specs to fixture/seed details and may become brittle if seeds change.

Consider explicitly finding or fabricating an automatic group and using its id instead, for example:
- xhr :put, :update, id: 1, name: "WAT", visible: "true"
+ automatic_group = Group.find_by(automatic: true)
+ xhr :put, :update, id: automatic_group.id, name: "WAT", visible: "true"
and similarly for the add_members / remove_member 422 tests.

Also applies to: 93-96, 115-116

13-15: Clean up extra blank lines flagged by RuboCop

RuboCop is reporting Layout/EmptyLinesAroundBlockBody for a number of contexts and examples (e.g., extra blank lines immediately after context/it declarations and before end). Trimming those will keep the spec file in line with the configured style and avoid noise in future diffs.

Also applies to: 31-32, 35-36, 46-47, 50-51, 59-60, 77-78, 81-82, 88-89, 92-93, 109-110, 113-114, 131-132
app/controllers/admin/groups_controller.rb (2)
6-9: Clarify assignment in if conditions to satisfy RuboCop and readability

Lines 6-9 and 71-75 intentionally use assignment within an if (if search = params[:search] and if user = User.find_by_username(username)), which is idiomatic Ruby but flagged by Lint/AssignmentInCondition.

To make intent explicit and silence the linter, you could wrap the assignment in parentheses or split it:
- if search = params[:search]
-   search = search.to_s
+ if (search = params[:search])
+   search = search.to_s

...

- usernames.split(",").each do |username|
-   if user = User.find_by_username(username)
+ usernames.split(",").each do |username|
+   if (user = User.find_by_username(username))
      group.add(user)
    end
  end
This keeps behavior unchanged while improving clarity.

Also applies to: 71-75

84-97: remove_member behavior looks correct but group.save may be unnecessary

remove_member (Lines 84-97) correctly:

guards against automatic groups,

removes the user via group.users.delete(user_id), and

returns success_json / render_json_error.

Given Rails’ association semantics, delete on the association usually persists immediately, so the subsequent group.save may be redundant. It’s harmless, though, and can be left as‑is unless you want to simplify.
app/assets/javascripts/admin/controllers/admin-group.js.es6 (1)
51-65: Consider clearing the usernames input and handling async result in addMembers

addMembers (Lines 61-65) sends the usernames string to the model but leaves the input populated (// TODO: should clear the input) and doesn’t react to success/failure of model.addMembers(...).

You could improve UX by clearing the field on success and optionally surfacing errors, e.g.:
-    addMembers: function() {
-      // TODO: should clear the input
-      if (Em.isEmpty(this.get("usernames"))) { return; }
-      this.get("model").addMembers(this.get("usernames"));
-    },
+    addMembers: function() {
+      var usernames = this.get("usernames");
+      if (Em.isEmpty(usernames)) { return; }
+
+      var self = this;
+      this.get("model").addMembers(usernames).then(function() {
+        self.set("usernames", "");
+      });
+    },
Exact promise handling can be adjusted to match the Group model API.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4975fc2 and 060cda7.

📒 Files selected for processing (16)

app/assets/javascripts/admin/controllers/admin-group.js.es6 (2 hunks)
app/assets/javascripts/admin/routes/admin_group_route.js (1 hunks)
app/assets/javascripts/admin/templates/group.hbs (1 hunks)
app/assets/javascripts/admin/templates/group_member.hbs (1 hunks)
app/assets/javascripts/admin/views/group-member.js.es6 (1 hunks)
app/assets/javascripts/discourse/models/group.js (1 hunks)
app/assets/javascripts/discourse/routes/group-members.js.es6 (1 hunks)
app/assets/javascripts/discourse/templates/components/admin-group-selector.hbs (0 hunks)
app/assets/javascripts/discourse/templates/group/members.hbs (1 hunks)
app/assets/javascripts/discourse/templates/user-selector-autocomplete.raw.hbs (1 hunks)
app/assets/stylesheets/common/admin/admin_base.scss (2 hunks)
app/controllers/admin/groups_controller.rb (2 hunks)
app/controllers/groups_controller.rb (1 hunks)
config/locales/client.en.yml (1 hunks)
config/routes.rb (1 hunks)
spec/controllers/admin/groups_controller_spec.rb (2 hunks)

💤 Files with no reviewable changes (1)

app/assets/javascripts/discourse/templates/components/admin-group-selector.hbs

🧰 Additional context used

🪛 RuboCop (1.81.7)

spec/controllers/admin/groups_controller_spec.rb

[convention] 14-15: Extra empty line detected at block body beginning.