Skip to content

Automated Test: group-dm-user-addition-post #312

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
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"),
Comment on lines +11 to +14

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Bug: totalPages is off-by-one when user_count is evenly divisible by limit.

Math.floor(user_count / limit) + 1 yields one extra page whenever user_count is a multiple of limit (e.g., 50 users with limit 50 → reports 2 pages instead of 1). This causes showingLast to be false when actually on the last page, letting the user paginate past the real end into an empty result.

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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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"),
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/controllers/admin-group.js.es6` around lines 11
- 14, The computed property totalPages incorrectly uses Math.floor(...)+1
causing an off-by-one when user_count is divisible by limit; replace the current
formula in totalPages with Math.ceil(this.get("user_count") / this.get("limit"))
while preserving the zero check (if user_count == 0 return 0) so showingLast and
pagination logic reflect the correct number of pages.


showingFirst: Em.computed.lte("currentPage", 1),
showingLast: Discourse.computed.propertyEqual("currentPage", "totalPages"),

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('members.@each.username'),

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();
},
Comment on lines +29 to +38

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

next action can set offset to user_count, producing an empty page.

Line 33 clamps to user_count, but if offset + limit >= user_count, the fetch will return zero results. The upper bound should be the start of the last page.

Suggested fix
     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"));
+          offset = Math.min(group.get("offset") + group.get("limit"),
+                            Math.max(group.get("user_count") - group.get("limit"), 0));

       group.set("offset", offset);

       return group.findMembers();
     },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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();
},
next: function() {
if (this.get("showingLast")) { return; }
var group = this.get("model"),
offset = Math.min(group.get("offset") + group.get("limit"),
Math.max(group.get("user_count") - group.get("limit"), 0));
group.set("offset", offset);
return group.findMembers();
},
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/controllers/admin-group.js.es6` around lines 29
- 38, The next action currently sets offset to Math.min(offset + limit,
user_count) which can equal user_count and produce an empty page; change the
clamp to use the start of the last page by computing lastStart = Math.max(0,
group.get("user_count") - group.get("limit")) and set offset =
Math.min(group.get("offset") + group.get("limit"), lastStart) before calling
group.findMembers(); update the logic inside the next function (refer to next,
group.get("offset"), group.get("limit"), group.get("user_count"), and
findMembers) accordingly.


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

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Native form submission on Enter key will cause a page reload.

Wrapping everything in <form> without a submit handler means pressing Enter while focused on the name text field or user-selector will trigger a browser-native form submission, reloading the page. Either add an Ember submit action that prevents default, or switch to a <div>.

Option A: handle submit on the form
-<form class="form-horizontal">
+<form class="form-horizontal" {{action "save" on="submit"}}>
Option B: use a div instead
-<form class="form-horizontal">
+<div class="form-horizontal">
 ...
-</form>
+</div>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<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>
<form class="form-horizontal" {{action "save" on="submit"}}>
<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>
{{`#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>
<label for="alias">{{i18n 'groups.alias_levels.title'}}</label>
{{combo-box name="alias" valueAttribute="value" value=alias_level content=aliasLevelOptions}}
</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>
Suggested change
<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>
<div class="form-horizontal">
<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>
{{`#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>
<label for="alias">{{i18n 'groups.alias_levels.title'}}</label>
{{combo-box name="alias" valueAttribute="value" value=alias_level content=aliasLevelOptions}}
</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>
</div>
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/templates/group.hbs` around lines 1 - 53, The
wrapper <form> causes native submit on Enter; either replace the top-level
<form> with a <div> to avoid browser reloads, or add a submit handler that
prevents default (e.g. wire the template to an Ember action on submit that calls
event.preventDefault() and delegates to save/addMembers as needed). Locate the
form tag in this template and update it; if adding the handler, ensure it
integrates with existing actions like "save", "addMembers" and "destroy" so
Enter doesn't trigger a page reload.

1 change: 1 addition & 0 deletions app/assets/javascripts/admin/templates/group_member.hbs
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
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
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); })
});
});
},
Comment on lines 20 to 38

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

rg -n -C5 'findMembers' --type=js

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 3495


🏁 Script executed:

cat -n app/assets/javascripts/discourse/models/group.js | sed -n '20,38p'

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 840


🏁 Script executed:

# Let's also check if there are any tests or documentation about expected offset behavior
rg -n "offset" app/assets/javascripts/discourse/models/group.js -B2 -A2

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 791


🏁 Script executed:

rg -n "return ;" app/assets/javascripts/discourse/models/group.js

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 141


🏁 Script executed:

# Also check Ember/Discourse conventions for early returns in similar methods
rg -n "if.*isEmpty.*return" app/assets/javascripts/discourse/models/ --type=js -A1 | head -30

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 1320


Return a resolved promise instead of undefined to match Discourse patterns.

Line 21 uses a bare return statement when the group name is empty, but other Discourse model methods like those in post_stream.js return Ember.RSVP.resolve() in similar guard clauses. This inconsistency could cause issues if future callers chain .then() on the result. Align with the established pattern:

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

The offset clamping on line 23 also allows offset to equal user_count, which would request data starting exactly at the total count and return zero results. Consider clamping to Math.max(0, this.get("user_count") - this.get("limit")) or letting the server handle edge cases.

🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/models/group.js` around lines 20 - 38,
Change the early-return in findMembers to return a resolved promise (use
Ember.RSVP.resolve()) rather than undefined so callers can safely chain .then();
also tighten the offset calculation in findMembers so it cannot equal user_count
(e.g., clamp offset to at most Math.max(0, this.get("user_count") -
this.get("limit")) instead of Math.min(this.get("user_count"), ...)) to avoid
requesting from exactly the total count.


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() });
},
Comment on lines +77 to +79

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

save() doesn't guard against a missing id, unlike destroy().

If save() is called on a group that hasn't been created yet (no id), the request goes to /admin/groups/undefined. The destroy() method at line 82 already guards for this — consider adding a similar check here, or at minimum documenting that create() must be called first.

🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/models/group.js` around lines 77 - 79, The
save() method issues a PUT to "/admin/groups/" + this.get('id') without checking
for a missing id, which can result in requests to "/admin/groups/undefined"; add
the same guard used in destroy() (check this.get('id') exists) and return a
rejected promise or throw/early-return when id is falsy, or alternatively
document that create() must be called first; update the save() implementation to
validate this.get('id') before calling Discourse.ajax so it behaves consistently
with destroy().


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

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

destroy() returns undefined when id is falsy — same chaining risk as findMembers.

If a caller does group.destroy().then(...) on a group without an id, this will throw a TypeError. Return a resolved promise for a consistent thenable API. Also, the semicolon after } on line 82 is a stray empty statement.

Proposed fix
   destroy: function(){
-    if (!this.get('id')) { return };
+    if (!this.get('id')) { return Ember.RSVP.resolve(); }
     return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"});
   },
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/models/group.js` around lines 81 - 84, The
destroy function currently returns undefined when this.get('id') is falsy,
causing callers like group.destroy().then(...) to throw; update destroy (the
method named destroy in the group model) to return a resolved promise when no id
is present (e.g., return a resolved
Promise/Discourse.Promise/Ember.RSVP.resolve()) so it always returns a thenable,
and remove the stray semicolon after the closing brace to eliminate the empty
statement; this mirrors the safe behavior used by findMembers.


findPosts: function(opts) {
Expand Down
13 changes: 3 additions & 10 deletions app/assets/javascripts/discourse/routes/group-members.js.es6
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();
}

});

Original file line number Diff line number Diff line change
@@ -1,3 +1 @@

<input type="text">

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