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;
Comment on lines +7 to +13
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use strict equality operators

Replace loose equality operators with strict equality to avoid type coercion issues.

   currentPage: 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; }
+    if (this.get("user_count") === 0) { return 0; }
     return Math.floor(this.get("user_count") / this.get("limit")) + 1;
   }.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
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;
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"),
🤖 Prompt for AI Agents
In app/assets/javascripts/admin/controllers/admin-group.js.es6 around lines 7 to
13, replace all instances of loose equality operators (==) with strict equality
operators (===) to prevent unintended type coercion and ensure accurate
comparisons.

}.property("limit", "user_count"),

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

addMembers: function() {
// TODO: should clear the input
if (Em.isEmpty(this.get("usernames"))) { return; }
this.get("model").addMembers(this.get("usernames"));
},
Comment on lines +61 to +65
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Address TODO: Clear usernames input after adding members

The TODO comment indicates missing functionality. The usernames input should be cleared after successfully adding members.

   addMembers: function() {
-    // TODO: should clear the input
     if (Em.isEmpty(this.get("usernames"))) { return; }
-    this.get("model").addMembers(this.get("usernames"));
+    var self = this;
+    this.get("model").addMembers(this.get("usernames")).then(function() {
+      self.set("usernames", null);
+    });
   },

Would you like me to create an issue to track this enhancement?

📝 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() {
// TODO: should clear the input
if (Em.isEmpty(this.get("usernames"))) { return; }
this.get("model").addMembers(this.get("usernames"));
},
addMembers: function() {
if (Em.isEmpty(this.get("usernames"))) { return; }
var self = this;
this.get("model").addMembers(this.get("usernames")).then(function() {
self.set("usernames", null);
});
},
🤖 Prompt for AI Agents
In app/assets/javascripts/admin/controllers/admin-group.js.es6 around lines 61
to 65, the addMembers function currently does not clear the usernames input
after adding members, as noted by the TODO comment. To fix this, after calling
addMembers on the model with the usernames, reset the usernames property to an
empty string or appropriate empty value to clear the input field.


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"}}
Comment on lines +16 to +21
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update deprecated Ember.js syntax

The template uses deprecated Ember helpers that should be updated to modern syntax:

  • Replace bind-attr with direct attribute binding
  • Use modern each block syntax
-        <a {{bind-attr class=":previous showingFirst:disabled"}} {{action "previous"}}>{{fa-icon "fast-backward"}}</a>
+        <a class="previous {{if 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>
+        <a class="next {{if showingLast 'disabled'}}" {{action "next"}}>{{fa-icon "fast-forward"}}</a>
       </div>
       <div class="ac-wrap clearfix">
-        {{each member in members itemView="group-member"}}
+        {{#each members as |member|}}
+          {{group-member member=member}}
+        {{/each}}
📝 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
<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"}}
<a class="previous {{if showingFirst 'disabled'}}" {{action "previous"}}>{{fa-icon "fast-backward"}}</a>
{{currentPage}}/{{totalPages}}
<a class="next {{if showingLast 'disabled'}}" {{action "next"}}>{{fa-icon "fast-fast-forward"}}</a>
</div>
<div class="ac-wrap clearfix">
{{#each members as |member|}}
{{group-member member=member}}
{{/each}}
🤖 Prompt for AI Agents
In app/assets/javascripts/admin/templates/group.hbs around lines 16 to 21, the
template uses deprecated Ember.js syntax such as `bind-attr` and the old `each`
helper. Replace `bind-attr` with direct class attribute bindings using curly
braces and class concatenation or class helper. Update the `each` loop to use
the modern block form `{{#each members as |member|}}` instead of `{{each member
in members}}`.

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

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update deprecated bind-attr helper

Replace the deprecated bind-attr helper with modern attribute binding syntax.

-    <button {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn btn-primary'>{{i18n 'admin.customize.save'}}</button>
+    <button {{action "save"}} disabled={{disableSave}} class='btn btn-primary'>{{i18n 'admin.customize.save'}}</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 "save"}} {{bind-attr disabled="disableSave"}} class='btn btn-primary'>{{i18n 'admin.customize.save'}}</button>
<button {{action "save"}} disabled={{disableSave}} class='btn btn-primary'>{{i18n 'admin.customize.save'}}</button>
🤖 Prompt for AI Agents
In app/assets/javascripts/admin/templates/group.hbs at line 47, the code uses
the deprecated bind-attr helper to bind the disabled attribute. Replace
bind-attr with the modern Ember attribute binding syntax by directly using
disabled={{disableSave}} inside the button tag to properly bind the disabled
attribute.

{{#unless automatic}}
<button {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>
{{/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();
});
},

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 +40 to 60
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling to member management methods

Both removeMember and addMembers methods lack error handling. Also, the promise should be returned to allow proper chaining in the controller.

   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();
+    }).catch(function(error) {
+      bootbox.alert(I18n.t("generic_error"));
+      throw error;
     });
   },

   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();
+    }).catch(function(error) {
+      bootbox.alert(I18n.t("generic_error"));
+      throw error;
+    });
   },
📝 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();
});
},
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();
})
},
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();
}).catch(function(error) {
bootbox.alert(I18n.t("generic_error"));
throw error;
});
},
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();
}).catch(function(error) {
bootbox.alert(I18n.t("generic_error"));
throw error;
});
},
🤖 Prompt for AI Agents
In app/assets/javascripts/discourse/models/group.js between lines 40 and 60, the
removeMember and addMembers methods lack error handling and do not properly
return promises for chaining. To fix this, add .catch handlers to both methods
to handle potential AJAX errors gracefully, and ensure the promise returned by
Discourse.ajax is returned from the methods so callers can chain further actions
or handle errors as needed.


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