Skip to content
Open
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Off-by-one error in totalPages calculation.

When user_count is an exact multiple of limit, this formula adds an extra page. For example, with user_count=10 and limit=5, the result is Math.floor(10/5) + 1 = 3, but there are only 2 pages.

Proposed fix
   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"),
🤖 Prompt for AI Agents
In @app/assets/javascripts/admin/controllers/admin-group.js.es6 around lines 11
- 14, The totalPages computed property has an off-by-one bug: replace the
current floor+1 logic in totalPages with a proper ceiling-based calculation (use
Math.ceil(this.get("user_count") / this.get("limit"))) and keep the zero-check;
also guard against a zero or falsy limit by returning 0 when this.get("limit")
is not positive. This change should be applied inside the totalPages property
implementation to correctly handle exact multiples of limit.


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();
},

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);
}
});
},
Comment on lines +51 to +59

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Missing error handling for removeMember operation.

The model.removeMember(member) call likely returns a promise, but errors are not caught or displayed to the user. If the server request fails, the user won't know.

Suggested improvement
     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);
+          self.get("model").removeMember(member).catch(function(e) {
+            var message = $.parseJSON(e.responseText).errors;
+            bootbox.alert(message);
+          });
         }
       });
     },
📝 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
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);
}
});
},
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).catch(function(e) {
var message = $.parseJSON(e.responseText).errors;
bootbox.alert(message);
});
}
});
},
🤖 Prompt for AI Agents
In @app/assets/javascripts/admin/controllers/admin-group.js.es6 around lines 51
- 59, The removeMember method currently calls
this.get("model").removeMember(member) without handling its promise; update
removeMember to handle the returned promise from model.removeMember(member) by
attaching success and failure handlers (e.g., .then(...) and .catch(...) or
using async/await) so that on success you proceed as before and on failure you
surface an error to the user (use bootbox.alert or I18n.t-based message) and
optionally log the error; reference the removeMember function and the
model.removeMember call when making the change.


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>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add type="button" to prevent unintended form submission.

The "add members" button is inside a <form> element. Without an explicit type="button", it defaults to type="submit", which will submit the form instead of just triggering the addMembers action.

Proposed fix
-        <button {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button>
+        <button type="button" {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button>
📝 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
<button {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button>
<button type="button" {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button>
🤖 Prompt for AI Agents
In @app/assets/javascripts/admin/templates/group.hbs at line 29, The button that
triggers the addMembers action is inside a form and lacks an explicit type, so
it defaults to submitting the form; update the button element that invokes the
"addMembers" action (the <button {{action "addMembers"}} ...> in the template)
to include type="button" to prevent form submission and ensure it only fires the
addMembers action.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add type="button" to the delete button.

Same issue as the add button—without type="button", clicking delete will submit the form rather than triggering the destroy action.

Proposed fix
-      <button {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>
+      <button type="button" {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>
📝 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
<button {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>
<button type="button" {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>
🤖 Prompt for AI Agents
In @app/assets/javascripts/admin/templates/group.hbs at line 49, The delete
button currently lacks a type attribute so clicking it submits the surrounding
form instead of invoking the "destroy" action; update the button element that
uses {{action "destroy"}} and class 'btn btn-danger' to include type="button" so
it does not act as a submit control and continues to trigger the destroy action
as intended.

{{/unless}}
</div>

</form>
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); })
});
});
},

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Promise chain doesn't wait for member reload.

The findMembers() call result is not returned, so the outer promise resolves before the member list refresh completes. Callers waiting on removeMember() may see stale data.

♻️ Suggested fix
   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();
+      return self.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
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();
});
},
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
return self.findMembers();
});
},
🤖 Prompt for AI Agents
In @app/assets/javascripts/discourse/models/group.js around lines 40 - 49, The
removeMember function currently calls this.findMembers() inside the .then
handler but does not return its promise, so callers of removeMember can resolve
before the member reload completes; update the .then handler in removeMember
(the Discourse.ajax(...).then(...) block) to return self.findMembers() (i.e.,
replace the fire-and-forget call with a returned promise) so the outer promise
chain waits for the member list refresh to finish.


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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Same promise chain issue and missing semicolon.

Similar to removeMember, the findMembers() result should be returned to properly chain the promise. Also, there's a missing semicolon on line 59.

♻️ Suggested fix
   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();
-    })
+      return self.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
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();
})
},
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
return self.findMembers();
});
},
🤖 Prompt for AI Agents
In @app/assets/javascripts/discourse/models/group.js around lines 51 - 60, The
addMembers function doesn't return the promise from self.findMembers(), breaking
proper chaining, and it's missing a trailing semicolon; update addMembers (in
the group.js model) so the .then callback returns self.findMembers() and add the
missing semicolon after the .then block to fix promise chaining and syntax.


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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Missing id guard in save and syntax issue in destroy.

  1. save() doesn't guard against missing id, which could result in a request to /admin/groups/undefined.
  2. Line 82 has an errant semicolon after return: { return }; — the semicolon after the closing brace serves no purpose.
♻️ Suggested fix
   save: function(){
+    if (!this.get('id')) { return; }
     return Discourse.ajax("/admin/groups/" + this.get('id'), { type: "PUT", data: this.asJSON() });
   },

   destroy: function(){
-    if (!this.get('id')) { return };
+    if (!this.get('id')) { return; }
     return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"});
   },
🤖 Prompt for AI Agents
In @app/assets/javascripts/discourse/models/group.js around lines 77 - 84, The
save method lacks an id guard and the destroy method contains an extraneous
semicolon; update save (function save) to return early if this.get('id') is
falsy (mirroring destroy's guard) to avoid calling /admin/groups/undefined, and
remove the stray semicolon after the return in destroy (change "if
(!this.get('id')) { return };" to "if (!this.get('id')) { return }") so the
syntax is clean.


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