FIX: proper handling of group memberships

[ShashankFC]

Test 8

Summary by CodeRabbit

Release Notes

• New Features

  • Added pagination controls for group member lists with previous/next navigation.
  • Introduced functionality to add and remove members from groups.

• Style

  • Redesigned group management form with improved layout and labeling.
  • Enhanced member list display with better visual organization.

• Tests

  • Expanded test coverage for group creation, member management, and automatic group handling.

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

---

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

[ShashankFC]

@cubic-dev-ai review this pull request

[cubic-dev-ai]

@cubic-dev-ai review this pull request

@ShashankFC I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

8 issues found across 16 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="app/controllers/groups_controller.rb">

<violation number="1" location="app/controllers/groups_controller.rb:22">
P2: The `limit` parameter is not capped, allowing clients to request arbitrarily large result sets (e.g., `?limit=999999`). Additionally, negative values would cause a PostgreSQL error. Clamp the value to a safe range.</violation>
</file>

<file name="app/controllers/admin/groups_controller.rb">

<violation number="1" location="app/controllers/admin/groups_controller.rb:90">
P1: Bug: `group.users.delete(user_id)` bypasses the `Group#remove` method which properly destroys `group_users` join records with callbacks. This skips the `primary_group_id` cleanup logic in `destroy_deletions`, potentially leaving users with stale `primary_group_id` references. Use `group.remove(user)` for consistency with `add_members` which uses `group.add(user)`.</violation>
</file>

<file name="config/locales/client.en.yml">

<violation number="1" location="config/locales/client.en.yml:1630">
P1: Wrong interpolation syntax for client-side locale. This file uses `{{variable}}` (Ember/JS i18n style), but `%{variable}` (Ruby i18n style) is used here. The variables won't be substituted and users will see raw placeholder text like `%{username}`.</violation>
</file>

<file name="app/assets/javascripts/discourse/models/group.js">

<violation number="1" location="app/assets/javascripts/discourse/models/group.js:21">
P2: `findMembers` returns `undefined` instead of a resolved promise when `name` is empty. The previous code returned `Ember.RSVP.resolve([])` to maintain a consistent promise-based API. Any caller chaining `.then()` on the result when `name` is empty will get a TypeError.</violation>

<violation number="2" location="app/assets/javascripts/discourse/models/group.js:47">
P2: `removeMember` and `addMembers` both call `self.findMembers()` inside `.then()` without `return`ing the promise. This means the outer promise resolves before the member list reload completes, so callers cannot reliably know when the operation (including the reload) is finished.</violation>
</file>

<file name="spec/controllers/admin/groups_controller_spec.rb">

<violation number="1" location="spec/controllers/admin/groups_controller_spec.rb:115">
P1: Wrong HTTP method: this test uses `:put` but the route for `remove_member` is mapped to `DELETE` (see `config/routes.rb`). This means the request won't reach the `remove_member` action and the test isn't actually verifying the intended behavior. Should be `xhr :delete, :remove_member, ...` to match the other test in this context.</violation>
</file>

<file name="app/assets/javascripts/admin/controllers/admin-group.js.es6">

<violation number="1" location="app/assets/javascripts/admin/controllers/admin-group.js.es6:7">
P1: Off-by-one bug in `totalPages`: when `user_count` is evenly divisible by `limit`, this returns one page too many (e.g., 10 users with limit 10 → 2 pages instead of 1). This allows navigating to an empty final page. Use `Math.ceil` instead of `Math.floor(...) + 1`.</violation>
</file>

<file name="app/assets/stylesheets/common/admin/admin_base.scss">

<violation number="1" location="app/assets/stylesheets/common/admin/admin_base.scss:472">
P2: Hardcoded hex color `#333` should use a SCSS theme variable (e.g., `$primary`) for consistency with the rest of the file and to support theming/dark mode. The same applies to `#aaa` on the next `&.disabled` rule — consider using something like `scale-color($primary, $lightness: 50%)` or an appropriate theme variable.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P1: Bug: group.users.delete(user_id) bypasses the Group#remove method which properly destroys group_users join records with callbacks. This skips the primary_group_id cleanup logic in destroy_deletions, potentially leaving users with stale primary_group_id references. Use group.remove(user) for consistency with add_members which uses group.add(user).

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At app/controllers/admin/groups_controller.rb, line 90:

<comment>Bug: `group.users.delete(user_id)` bypasses the `Group#remove` method which properly destroys `group_users` join records with callbacks. This skips the `primary_group_id` cleanup logic in `destroy_deletions`, potentially leaving users with stale `primary_group_id` references. Use `group.remove(user)` for consistency with `add_members` which uses `group.add(user)`.</comment>

<file context>
@@ -59,41 +46,59 @@ def update_put(group)
+
+    return can_not_modify_automatic if group.automatic
+
+    group.users.delete(user_id)
+
+    if group.save
</file context>

[cubic-dev-ai]

P1: Wrong interpolation syntax for client-side locale. This file uses {{variable}} (Ember/JS i18n style), but %{variable} (Ruby i18n style) is used here. The variables won't be substituted and users will see raw placeholder text like %{username}.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At config/locales/client.en.yml, line 1630:

<comment>Wrong interpolation syntax for client-side locale. This file uses `{{variable}}` (Ember/JS i18n style), but `%{variable}` (Ruby i18n style) is used here. The variables won't be substituted and users will see raw placeholder text like `%{username}`.</comment>

<file context>
@@ -1620,13 +1620,17 @@ en:
         delete: "Delete"
         delete_confirm: "Delete this group?"
         delete_failed: "Unable to delete group. If this is an automatic group, it cannot be destroyed."
+        delete_member_confirm: "Remove '%{username}' from the '%{group}' group?"
+        name: "Name"
+        add: "Add"
</file context>

Suggested change

	delete_member_confirm: "Remove '%{username}' from the '%{group}' group?"
	delete_member_confirm: "Remove '{{username}}' from the '{{group}}' group?"

[cubic-dev-ai]

P1: Wrong HTTP method: this test uses :put but the route for remove_member is mapped to DELETE (see config/routes.rb). This means the request won't reach the remove_member action and the test isn't actually verifying the intended behavior. Should be xhr :delete, :remove_member, ... to match the other test in this context.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At spec/controllers/admin/groups_controller_spec.rb, line 115:

<comment>Wrong HTTP method: this test uses `:put` but the route for `remove_member` is mapped to `DELETE` (see `config/routes.rb`). This means the request won't reach the `remove_member` action and the test isn't actually verifying the intended behavior. Should be `xhr :delete, :remove_member, ...` to match the other test in this context.</comment>

<file context>
@@ -48,97 +74,61 @@
+  context ".remove_member" do
+
+    it "cannot remove members from automatic groups" do
+      xhr :put, :remove_member, group_id: 1, user_id: 42
+      response.status.should == 422
+    end
</file context>

Suggested change

	xhr :put, :remove_member, group_id: 1, user_id: 42
	xhr :delete, :remove_member, group_id: 1, user_id: 42

[cubic-dev-ai]

P1: Off-by-one bug in totalPages: when user_count is evenly divisible by limit, this returns one page too many (e.g., 10 users with limit 10 → 2 pages instead of 1). This allows navigating to an empty final page. Use Math.ceil instead of Math.floor(...) + 1.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At app/assets/javascripts/admin/controllers/admin-group.js.es6, line 7:

<comment>Off-by-one bug in `totalPages`: when `user_count` is evenly divisible by `limit`, this returns one page too many (e.g., 10 users with limit 10 → 2 pages instead of 1). This allows navigating to an empty final page. Use `Math.ceil` instead of `Math.floor(...) + 1`.</comment>

<file context>
@@ -1,34 +1,69 @@
+  usernames: null,
+
+  currentPage: function() {
+    if (this.get("user_count") == 0) { return 0; }
+    return Math.floor(this.get("offset") / this.get("limit")) + 1;
+  }.property("limit", "offset", "user_count"),
</file context>

[cubic-dev-ai]

P2: The limit parameter is not capped, allowing clients to request arbitrarily large result sets (e.g., ?limit=999999). Additionally, negative values would cause a PostgreSQL error. Clamp the value to a safe range.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At app/controllers/groups_controller.rb, line 22:

<comment>The `limit` parameter is not capped, allowing clients to request arbitrarily large result sets (e.g., `?limit=999999`). Additionally, negative values would cause a PostgreSQL error. Clamp the value to a safe range.</comment>

<file context>
@@ -19,26 +19,29 @@ def posts
-    end
-
-    render_serialized(members.to_a, GroupUserSerializer)
+    limit = (params[:limit] || 50).to_i
+    offset = params[:offset].to_i
+
</file context>

Suggested change

	limit = (params[:limit] || 50).to_i
	limit = (params[:limit] || 50).to_i.clamp(1, 200)

[cubic-dev-ai]

P2: removeMember and addMembers both call self.findMembers() inside .then() without returning the promise. This means the outer promise resolves before the member list reload completes, so callers cannot reliably know when the operation (including the reload) is finished.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At app/assets/javascripts/discourse/models/group.js, line 47:

<comment>`removeMember` and `addMembers` both call `self.findMembers()` inside `.then()` without `return`ing the promise. This means the outer promise resolves before the member list reload completes, so callers cannot reliably know when the operation (including the reload) is finished.</comment>

<file context>
@@ -7,53 +7,80 @@
+      data: { user_id: member.get("id") }
+    }).then(function() {
+      // reload member list
+      self.findMembers();
+    });
   },
</file context>

Suggested change

	self.findMembers();
	return self.findMembers();

[cubic-dev-ai]

P2: findMembers returns undefined instead of a resolved promise when name is empty. The previous code returned Ember.RSVP.resolve([]) to maintain a consistent promise-based API. Any caller chaining .then() on the result when name is empty will get a TypeError.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At app/assets/javascripts/discourse/models/group.js, line 21:

<comment>`findMembers` returns `undefined` instead of a resolved promise when `name` is empty. The previous code returned `Ember.RSVP.resolve([])` to maintain a consistent promise-based API. Any caller chaining `.then()` on the result when `name` is empty will get a TypeError.</comment>

<file context>
@@ -7,53 +7,80 @@
 
   findMembers: function() {
-    if (Em.isEmpty(this.get('name'))) { return Ember.RSVP.resolve([]); }
+    if (Em.isEmpty(this.get('name'))) { return ; }
 
-    return Discourse.ajax('/groups/' + this.get('name') + '/members').then(function(result) {
</file context>

[cubic-dev-ai]

P2: Hardcoded hex color #333 should use a SCSS theme variable (e.g., $primary) for consistency with the rest of the file and to support theming/dark mode. The same applies to #aaa on the next &.disabled rule — consider using something like scale-color($primary, $lightness: 50%) or an appropriate theme variable.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At app/assets/stylesheets/common/admin/admin_base.scss, line 472:

<comment>Hardcoded hex color `#333` should use a SCSS theme variable (e.g., `$primary`) for consistency with the rest of the file and to support theming/dark mode. The same applies to `#aaa` on the next `&.disabled` rule — consider using something like `scale-color($primary, $lightness: 50%)` or an appropriate theme variable.</comment>

<file context>
@@ -454,6 +459,26 @@ section.details {
+    }
+  }
+  .next, .previous {
+    color: #333 !important;
+    &.disabled {
+      color: #aaa !important;
</file context>