FIX: proper handling of group memberships by ShashankFC · Pull Request #3 · AI-Code-Review-Evals/copilot-discourse

ShashankFC · 2026-01-22T09:19:00Z

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

Copilot

Pull request overview

This PR refactors group membership handling to use dedicated API endpoints for adding and removing members, replacing the previous approach of updating group members through the update endpoint. The changes implement proper pagination for member lists and separate member management from group property updates.

Changes:

Refactored group membership API to use dedicated add_members and remove_member endpoints instead of handling membership through the update endpoint
Implemented pagination controls (previous/next) for group member lists with configurable limit
Updated UI to show member management interface with add/remove functionality

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
app/controllers/admin/groups_controller.rb	Added separate `add_members` and `remove_member` endpoints; simplified `update` method to only handle group properties
app/controllers/groups_controller.rb	Enhanced `members` endpoint to return paginated results with metadata
config/routes.rb	Added new routes for member management endpoints
spec/controllers/admin/groups_controller_spec.rb	Reorganized and expanded tests for new member management endpoints
app/assets/javascripts/discourse/models/group.js	Added `addMembers` and `removeMember` methods; refactored to remove username-based save operations
app/assets/javascripts/admin/controllers/admin-group.js.es6	Added pagination logic and member management actions
app/assets/javascripts/admin/templates/group.hbs	Redesigned form to show paginated member list with add/remove controls
config/locales/client.en.yml	Added localization strings for member management UI
app/assets/stylesheets/common/admin/admin_base.scss	Added styles for groups interface including pagination controls
app/assets/javascripts/discourse/routes/group-members.js.es6	Simplified route to trigger member loading directly on model
app/assets/javascripts/admin/routes/admin_group_route.js	Simplified route to trigger member loading directly on model
app/assets/javascripts/admin/views/group-member.js.es6	New view for rendering individual group members
app/assets/javascripts/admin/templates/group_member.hbs	New template for displaying member with remove button
app/assets/javascripts/discourse/templates/group/members.hbs	Updated to use new `members` property
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

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-30T10:25:39Z


  findMembers: function() {
-    if (Em.isEmpty(this.get('name'))) { return Ember.RSVP.resolve([]); }
+    if (Em.isEmpty(this.get('name'))) { return ; }


The return statement returns undefined instead of a Promise. This breaks the calling code's expectation of a Promise return value. Change to return Ember.RSVP.resolve(); to maintain consistent return type.

Suggested change

if (Em.isEmpty(this.get('name'))) { return ; }

if (Em.isEmpty(this.get('name'))) { return Ember.RSVP.resolve(); }

Copilot · 2026-01-30T10:25:39Z

+  },
+
+  destroy: function(){
+    if (!this.get('id')) { return };


The return statement returns undefined instead of a Promise. This breaks consistency with other methods that return Promises. Change to return Ember.RSVP.resolve(); to maintain consistent return type.

Suggested change

if (!this.get('id')) { return };

if (!this.get('id')) { return Ember.RSVP.resolve(); }

Copilot · 2026-01-30T10:25:39Z

+    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; }


Use strict equality (===) instead of loose equality (==) for comparisons to avoid type coercion issues.

Suggested change

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; }

Copilot · 2026-01-30T10:25:40Z

+    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; }


Use strict equality (===) instead of loose equality (==) for comparisons to avoid type coercion issues.

Suggested change

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; }

Copilot · 2026-01-30T10:25:40Z

+
+  context ".create" do
+
+    it "strip spaces on the group name" do


The test name should use the third-person singular form 'strips' instead of 'strip' to match RSpec conventions.

Suggested change

it "strip spaces on the group name" do

it "strips spaces on the group name" do

Copilot · 2026-01-30T10:25:40Z

-    Group.expects(:refresh_automatic_groups!).returns(true)
+  context ".update" do
+
+    it "ignore name change on automatic group" do


The test name should use the third-person singular form 'ignores' instead of 'ignore' to match RSpec conventions.

Suggested change

it "ignore name change on automatic group" do

it "ignores name change on automatic group" do

Copilot · 2026-01-30T10:25:40Z

+      // TODO: should clear the input
+      if (Em.isEmpty(this.get("usernames"))) { return; }
+      this.get("model").addMembers(this.get("usernames"));


This TODO comment indicates incomplete functionality. The usernames input should be cleared after successfully adding members to provide better user feedback.

Suggested change

// TODO: should clear the input

if (Em.isEmpty(this.get("usernames"))) { return; }

this.get("model").addMembers(this.get("usernames"));

if (Em.isEmpty(this.get("usernames"))) { return; }

var usernames = this.get("usernames");

this.set("usernames", null);

this.get("model").addMembers(usernames);

FIX: proper handling of group memberships

060cda7

ShashankFC requested a review from Copilot January 30, 2026 10:24

Copilot AI reviewed Jan 30, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: proper handling of group memberships#3

FIX: proper handling of group memberships#3
ShashankFC wants to merge 1 commit into
group-dm-user-addition-prefrom
group-dm-user-addition-post

ShashankFC commented Jan 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 30, 2026

Uh oh!

Copilot AI Jan 30, 2026

Uh oh!

Copilot AI Jan 30, 2026

Uh oh!

Copilot AI Jan 30, 2026

Uh oh!

Copilot AI Jan 30, 2026

Uh oh!

Copilot AI Jan 30, 2026

Uh oh!

Copilot AI Jan 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if (Em.isEmpty(this.get('name'))) { return ; }
	if (Em.isEmpty(this.get('name'))) { return Ember.RSVP.resolve(); }

	if (!this.get('id')) { return };
	if (!this.get('id')) { return Ember.RSVP.resolve(); }

	it "strip spaces on the group name" do
	it "strips spaces on the group name" do

	it "ignore name change on automatic group" do
	it "ignores name change on automatic group" do

Conversation

ShashankFC commented Jan 22, 2026

Summary by CodeRabbit

Release Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants