Skip to content

FIX: proper handling of group memberships #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
akshayutture-augment wants to merge 1 commit into
base: group-dm-user-addition-pre
Choose a base branch
from
Open

FIX: proper handling of group memberships #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 58 additions & 23 deletions app/assets/javascripts/admin/controllers/admin-group.js.es6
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,69 @@
export default Em.ObjectController.extend({
needs: ['adminGroups'],
members: null,
disableSave: false,
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"),

totalPages: function() {
if (this.get("user_count") == 0) { return 0; }
return Math.floor(this.get("user_count") / this.get("limit")) + 1;
}.property("limit", "user_count"),

showingFirst: Em.computed.lte("currentPage", 1),
showingLast: Discourse.computed.propertyEqual("currentPage", "totalPages"),
Comment on lines +6 to +17
Copy link

@coderabbitai coderabbitai bot Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Fix off‑by‑one error in totalPages pagination logic

totalPages (Lines 11-15) computes:

if (this.get("user_count") == 0) { return 0; }
return Math.floor(this.get("user_count") / this.get("limit")) + 1;

This overcounts when user_count is an exact multiple of limit (e.g., 20 users, page size 10 → returns 3 pages). Combined with the next action (Lines 29-38), this allows navigation to an extra, empty page and an unnecessary findMembers() call.

Use Math.ceil instead:

-  totalPages: function() {
-    if (this.get("user_count") == 0) { return 0; }
-    return Math.floor(this.get("user_count") / this.get("limit")) + 1;
-  }.property("limit", "user_count"),
+  totalPages: function() {
+    if (this.get("user_count") == 0) { return 0; }
+    return Math.ceil(this.get("user_count") / this.get("limit"));
+  }.property("limit", "user_count"),

This keeps showingFirst/showingLast and next/previous in sync with the actual number of pages.

Please verify pagination in the UI with user counts that are and are not multiples of the page size (e.g., 10, 11, 20 members).

Also applies to: 29-50

🏁 Script executed:

#!/bin/bash
# Find and examine the admin-group controller file
find . -name "*admin-group*" -type f 2>/dev/null | head -20

Length of output: 275

🏁 Script executed:

#!/bin/bash
# Extract the relevant sections from the admin-group controller
cat ./app/assets/javascripts/admin/controllers/admin-group.js.es6 2>/dev/null | head -60

Length of output: 1928

🏁 Script executed:

#!/bin/bash
# Search for pagination-related tests or validation
find . -name "*test*" -o -name "*spec*" | xargs grep -l "totalPages\|currentPage" 2>/dev/null | head -10

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Check if there are any guards or clamping logic in the next/previous actions
rg -n "next|previous" ./app/assets/javascripts/admin/controllers/admin-group.js.es6 -A 5 -B 1 2>/dev/null

Length of output: 451

🏁 Script executed:

#!/bin/bash
# Examine the group model's findMembers method to understand side effects
rg -n "findMembers" ./app/assets/javascripts/models/ -A 3 2>/dev/null | head -30

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Check if showingLast property is used to prevent navigation beyond last page
rg -n "showingLast" ./app/assets/javascripts/admin/ -B 2 -A 2 2>/dev/null | head -30

Length of output: 1528

Fix off‑by‑one error in totalPages pagination logic

totalPages (lines 11-14) computes:

if (this.get("user_count") == 0) { return 0; }
return Math.floor(this.get("user_count") / this.get("limit")) + 1;

This overcounts when user_count is an exact multiple of limit (e.g., 20 users, page size 10 → returns 3 instead of 2). The showingLast guard at line 30 fails to prevent navigation to the extra page because both currentPage and totalPages are overcounted, so the comparison never blocks at the actual boundary. This allows navigation to empty pages and unnecessary findMembers() calls.

Use Math.ceil instead:

-  totalPages: function() {
-    if (this.get("user_count") == 0) { return 0; }
-    return Math.floor(this.get("user_count") / this.get("limit")) + 1;
-  }.property("limit", "user_count"),
+  totalPages: function() {
+    if (this.get("user_count") == 0) { return 0; }
+    return Math.ceil(this.get("user_count") / this.get("limit"));
+  }.property("limit", "user_count"),

This keeps pagination boundaries consistent with showingFirst, showingLast, and the next/previous actions.

🤖 Prompt for AI Agents 
In app/assets/javascripts/admin/controllers/admin-group.js.es6 around lines 6 to
17, totalPages currently uses Math.floor(user_count / limit) + 1 which
overcounts when user_count is an exact multiple of limit; replace that
computation with Math.ceil(user_count / limit) (keeping the zero-check) so
totalPages correctly reflects the number of pages and aligns with
showingFirst/showingLast and next/previous navigation.


aliasLevelOptions: function() {
return [
{ name: I18n.t("groups.alias_levels.nobody"), value: 0},
{ name: I18n.t("groups.alias_levels.mods_and_admins"), value: 2},
{ name: I18n.t("groups.alias_levels.members_mods_and_admins"), value: 3},
{ name: I18n.t("groups.alias_levels.everyone"), value: 99}
{ name: I18n.t("groups.alias_levels.nobody"), value: 0 },
{ name: I18n.t("groups.alias_levels.mods_and_admins"), value: 2 },
{ name: I18n.t("groups.alias_levels.members_mods_and_admins"), value: 3 },
{ name: I18n.t("groups.alias_levels.everyone"), value: 99 }
];
}.property(),

usernames: function(key, value) {
var members = this.get('members');
if (arguments.length > 1) {
this.set('_usernames', value);
} else {
var usernames;
if(members) {
usernames = members.map(function(user) {
return user.get('username');
}).join(',');
}
this.set('_usernames', usernames);
}
return this.get('_usernames');
}.property('[email protected]'),

actions: {
next: function() {
if (this.get("showingLast")) { return; }

var group = this.get("model"),
offset = Math.min(group.get("offset") + group.get("limit"), group.get("user_count"));

group.set("offset", offset);

return group.findMembers();
},

previous: function() {
if (this.get("showingFirst")) { return; }

var group = this.get("model"),
offset = Math.max(group.get("offset") - group.get("limit"), 0);

group.set("offset", offset);

return group.findMembers();
},

removeMember: function(member) {
var self = this,
message = I18n.t("admin.groups.delete_member_confirm", { username: member.get("username"), group: this.get("name") });
return bootbox.confirm(message, I18n.t("no_value"), I18n.t("yes_value"), function(confirm) {
if (confirm) {
self.get("model").removeMember(member);
}
});
},

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

save: function() {
var self = this,
group = this.get('model');
Expand All @@ -37,9 +72,9 @@ export default Em.ObjectController.extend({

var promise;
if (group.get('id')) {
promise = group.saveWithUsernames(this.get('usernames'));
promise = group.save();
} else {
promise = group.createWithUsernames(this.get('usernames')).then(function() {
promise = group.create().then(function() {
var groupsController = self.get('controllers.adminGroups');
groupsController.addObject(group);
});
Expand Down
12 changes: 3 additions & 9 deletions app/assets/javascripts/admin/routes/admin_group_route.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,10 @@ Discourse.AdminGroupRoute = Discourse.Route.extend({
return group;
},

afterModel: function(model) {
var self = this;
return model.findMembers().then(function(members) {
self.set('_members', members);
});
},

setupController: function(controller, model) {
controller.set('model', model);
controller.set('members', this.get('_members'));
controller.set("model", model);
model.findMembers();
}

});

74 changes: 49 additions & 25 deletions app/assets/javascripts/admin/templates/group.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,53 @@
{{#if automatic}}
<h3>{{name}}</h3>
{{else}}
{{text-field value=name placeholderKey="admin.groups.name_placeholder"}}
{{/if}}
<form class="form-horizontal">

<div class="control-group">
<label class="control-label">{{i18n 'admin.groups.group_members'}}</label>
<div class="controls">
{{user-selector usernames=usernames id="group-users" placeholderKey="admin.groups.selector_placeholder" tabindex="1" disabled=automatic}}
<div>
{{#if automatic}}
<h3>{{name}}</h3>
{{else}}
<label for="name">{{i18n 'admin.groups.name'}}</label>
{{text-field name="name" value=name placeholderKey="admin.groups.name_placeholder"}}
{{/if}}
</div>
</div>
<div class="control-group">
<div class="controls">
{{input type="checkbox" checked=visible}} {{i18n 'groups.visible'}}

{{#if id}}
<div>
<label>{{i18n 'admin.groups.group_members'}} ({{user_count}})</label>
<div>
<a {{bind-attr class=":previous showingFirst:disabled"}} {{action "previous"}}>{{fa-icon "fast-backward"}}</a>
{{currentPage}}/{{totalPages}}
<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"}}
</div>
</div>

{{#unless automatic}}
<div>
<label for="user-selector">{{i18n 'admin.groups.add_members'}}</label>
{{user-selector usernames=usernames placeholderKey="admin.groups.selector_placeholder" id="user-selector"}}
<button {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button>
</div>
{{/unless}}
{{/if}}

<div>
<label>
{{input type="checkbox" checked=visible}}
{{i18n 'groups.visible'}}
</label>
</div>
</div>
<div class="control-group">
<label class="control-label">{{i18n 'groups.alias_levels.title'}}</label>
<div class="controls">
{{combo-box valueAttribute="value" value=alias_level content=aliasLevelOptions}}

<div>
<label for="alias">{{i18n 'groups.alias_levels.title'}}</label>
{{combo-box name="alias" valueAttribute="value" value=alias_level content=aliasLevelOptions}}
</div>
</div>
<div class='controls'>
<button {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn'>{{i18n 'admin.customize.save'}}</button>
{{#unless automatic}}
<button {{action "destroy"}} class='btn btn-danger'><i class="fa fa-trash-o"></i>{{i18n 'admin.customize.delete'}}</button>
{{/unless}}
</div>

<div class='buttons'>
<button {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn btn-primary'>{{i18n 'admin.customize.save'}}</button>
{{#unless automatic}}
<button {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>
{{/unless}}
</div>

</form>
Comment on lines +1 to +53
Copy link

@coderabbitai coderabbitai bot Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Handle form submit explicitly to avoid native browser submission

Wrapping everything in <form class="form-horizontal"> but only wiring actions on buttons means pressing Enter in a field may trigger a native form submit (page reload) instead of the intended Ember save action. Consider adding an explicit submit handler on the form, e.g. {{action "save" on="submit"}}, and preventing default there so keyboard submit paths behave the same as clicking Save.

🤖 Prompt for AI Agents 
In app/assets/javascripts/admin/templates/group.hbs around lines 1-53 the form
currently relies on button actions only, so pressing Enter can trigger a native
browser submit; add an explicit submit handler to the form (e.g. attach {{action
"save" on="submit"}} to the <form>) and update the corresponding "save" action
in the component/controller to accept the submit event and call
event.preventDefault() (or return false) before performing the existing save
logic so keyboard/Enter submits invoke the Ember save flow instead of a page
reload.

1 change: 1 addition & 0 deletions app/assets/javascripts/admin/templates/group_member.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{{avatar member imageSize="small"}} {{member.username}} {{#unless automatic}}<a class='remove' {{action "removeMember" member}}>{{fa-icon "times"}}</a>{{/unless}}
4 changes: 4 additions & 0 deletions app/assets/javascripts/admin/views/group-member.js.es6
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export default Discourse.View.extend({
classNames: ["item"],
templateName: "admin/templates/group_member"
});
81 changes: 54 additions & 27 deletions app/assets/javascripts/discourse/models/group.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,53 +7,80 @@
@module Discourse
**/
Discourse.Group = Discourse.Model.extend({
limit: 50,
offset: 0,
user_count: 0,

userCountDisplay: function(){
var c = this.get('user_count');
// don't display zero its ugly
if(c > 0) {
return c;
}
if (c > 0) { return c; }
}.property('user_count'),

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) {
return result.map(function(u) { return Discourse.User.create(u) });
var self = this, offset = Math.min(this.get("user_count"), Math.max(this.get("offset"), 0));

return Discourse.ajax('/groups/' + this.get('name') + '/members.json', {
data: {
limit: this.get("limit"),
offset: offset
}
}).then(function(result) {
self.setProperties({
user_count: result.meta.total,
limit: result.meta.limit,
offset: result.meta.offset,
members: result.members.map(function(member) { return Discourse.User.create(member); })
});
});
},

destroy: function(){
if(!this.get('id')) return;
return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"});
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();
});
},

asJSON: function() {
return { group: {
name: this.get('name'),
alias_level: this.get('alias_level'),
visible: !!this.get('visible'),
usernames: this.get('usernames') } };
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();
})
},

createWithUsernames: function(usernames){
var self = this,
json = this.asJSON();
json.group.usernames = usernames;
asJSON: function() {
return {
name: this.get('name'),
alias_level: this.get('alias_level'),
visible: !!this.get('visible')
};
},

return Discourse.ajax("/admin/groups", {type: "POST", data: json}).then(function(resp) {
create: function(){
var self = this;
return Discourse.ajax("/admin/groups", { type: "POST", data: this.asJSON() }).then(function(resp) {
self.set('id', resp.basic_group.id);
});
},

saveWithUsernames: function(usernames){
var json = this.asJSON();
json.group.usernames = usernames;
return Discourse.ajax("/admin/groups/" + this.get('id'), {
type: "PUT",
data: json
});
save: function(){
return Discourse.ajax("/admin/groups/" + this.get('id'), { type: "PUT", data: this.asJSON() });
},

destroy: function(){
if (!this.get('id')) { return };
return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"});
},

findPosts: function(opts) {
Expand Down
13 changes: 3 additions & 10 deletions app/assets/javascripts/discourse/routes/group-members.js.es6
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,10 @@ export default Discourse.Route.extend(ShowFooter, {
return this.modelFor('group');
},

afterModel: function(model) {
var self = this;
return model.findMembers().then(function(result) {
self.set('_members', result);
});
},

setupController: function(controller) {
controller.set('model', this.get('_members'));
setupController: function(controller, model) {
this.controllerFor('group').set('showing', 'members');
controller.set("model", model);
model.findMembers();
}

});

2 changes: 0 additions & 2 deletions app/assets/javascripts/discourse/templates/components/admin-group-selector.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1 @@

<input type="text">

2 changes: 1 addition & 1 deletion app/assets/javascripts/discourse/templates/group/members.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<tr>
<th colspan="3" class="seen">{{i18n 'last_seen'}}</th>
</tr>
{{#each m in model}}
{{#each m in members}}
<tr>
<td class='avatar'>
{{avatar m imageSize="large"}}
Expand Down
16 changes: 9 additions & 7 deletions app/assets/javascripts/discourse/templates/user-selector-autocomplete.raw.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,21 @@
<ul>
{{#each options.users}}
<li>
<a href='#'>{{avatar this imageSize="tiny"}}
<span class='username'>{{this.username}}</span>
<span class='name'>{{this.name}}</span></a>
<a href='#'>{{avatar this imageSize="tiny"}}
<span class='username'>{{this.username}}</span>
<span class='name'>{{this.name}}</span>
</a>
</li>
{{/each}}
{{#if options.groups}}
{{#if options.users}}<hr>{{/if}}
{{#each options.groups}}
<li>
<a href=''><i class='icon-group'></i>
<span class='username'>{{this.name}}</span>
<span class='name'>{{max-usernames usernames max="3"}}</span>
</a>
<a href=''>
<i class='icon-group'></i>
<span class='username'>{{this.name}}</span>
<span class='name'>{{max-usernames usernames max="3"}}</span>
</a>
</li>
{{/each}}
{{/if}}
Expand Down
Loading