Skip to content

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

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

Fix totalPages off-by-one when user_count is a multiple of limit.
With Math.floor(...)+1, 20 users at limit 10 yields 3 pages (should be 2). This produces an extra empty page and incorrect showingLast.

🐛 Proposed fix
-    return Math.floor(this.get("user_count") / this.get("limit")) + 1;
+    return Math.ceil(this.get("user_count") / this.get("limit"));
📝 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 totalPages computed property (totalPages) incorrectly adds 1 after
Math.floor, causing an extra page when user_count is an exact multiple of limit;
change the calculation to compute ceil(user_count / limit) or use
Math.floor((user_count + limit - 1) / limit) so 20 users with limit 10 yields 2
pages; update the totalPages function (property("limit","user_count")) to return
0 when user_count is 0 and otherwise the correct ceiling division so downstream
logic (e.g., showingLast) no longer sees an extra empty page.


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

Clamp offset to the last valid page, not to user_count.
When user_count is exactly divisible by limit, this allows offset == user_count, yielding an empty page.

🐛 Proposed fix
-      var group = this.get("model"),
-          offset = Math.min(group.get("offset") + group.get("limit"), group.get("user_count"));
+      var group = this.get("model"),
+          maxOffset = Math.max(group.get("user_count") - group.get("limit"), 0),
+          offset = Math.min(group.get("offset") + group.get("limit"), maxOffset);
📝 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"),
maxOffset = Math.max(group.get("user_count") - group.get("limit"), 0),
offset = Math.min(group.get("offset") + group.get("limit"), maxOffset);
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 method allows offset to equal user_count when user_count % limit
=== 0; change the offset clamp logic in the next function so it sets offset to
the last valid page start instead of user_count: compute a maxOffset based on
group.get("user_count") and group.get("limit") (e.g. last page start =
(Math.ceil(user_count / limit) - 1) * limit or
floor((user_count-1)/limit)*limit), then use Math.min(group.get("offset") +
group.get("limit"), maxOffset) before group.set("offset") and call
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
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>
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}}

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

🏁 Script executed:

# Check if the file exists and view its content
find . -name "group_member.hbs" -type f

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 145


🏁 Script executed:

# Search for similar .remove or remove action patterns in templates
find . -name "*.hbs" -type f -path "*/templates/*" | head -20

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 1205


🏁 Script executed:

# Check for CSS styling for .remove class
rg "\.remove\s*\{" --type css --type scss

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 116


🏁 Script executed:

# Search for button or <a> patterns in other handlebars templates to understand project conventions
rg "class=['\"]remove['\"]" --type hbs

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 306


🏁 Script executed:

# Read the actual file content
cat -n ./app/assets/javascripts/admin/templates/group_member.hbs

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 257


🏁 Script executed:

# Search for .remove styling in CSS files
find . -name "*.css" -type f | xargs rg "\.remove" 2>/dev/null | head -20

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 70


🏁 Script executed:

# Search for how buttons are implemented in other admin templates
rg "<button" app/assets/javascripts/admin/templates/ -A 2 | head -40

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 3806


🏁 Script executed:

# Search for how other interactive actions are implemented in templates
rg "action.*remove" app/assets/javascripts/admin/templates/ -B 2 -A 2

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 306


🏁 Script executed:

# Check how fa-icon is used in other templates to understand the helper
rg "fa-icon" app/assets/javascripts/admin/templates/ | head -10

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 2141


🏁 Script executed:

# Look for similar delete/remove patterns with <a> in other templates
rg "<a[^>]*remove" app/assets/javascripts/admin/templates/ -A 1

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 306


🏁 Script executed:

# Check if there are accessibility attributes (aria-label, role) on other <a> tags without href
rg "<a[^>]*(aria-label|role)[^>]*>" app/assets/javascripts/admin/templates/ | head -10

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 70


Use a <button> instead of <a> for the remove action.

An <a> without href is not keyboard-focusable. The codebase consistently uses <button> for interactive actions—change this to match the project convention and improve accessibility.

♿ Proposed fix
-{{avatar member imageSize="small"}} {{member.username}} {{`#unless` automatic}}<a class='remove' {{action "removeMember" member}}>{{fa-icon "times"}}</a>{{/unless}}
+{{avatar member imageSize="small"}} {{member.username}}
+{{`#unless` automatic}}
+  <button type="button"
+          class="remove"
+          {{action "removeMember" member}}
+          aria-label="{{i18n 'admin_js.admin.groups.delete'}}"
+          title="{{i18n 'admin_js.admin.groups.delete'}}">
+    {{fa-icon "times"}}
+  </button>
+{{/unless}}
📝 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
{{avatar member imageSize="small"}} {{member.username}} {{#unless automatic}}<a class='remove' {{action "removeMember" member}}>{{fa-icon "times"}}</a>{{/unless}}
{{avatar member imageSize="small"}} {{member.username}}
{{`#unless` automatic}}
<button type="button"
class="remove"
{{action "removeMember" member}}
aria-label="{{i18n 'admin_js.admin.groups.delete'}}"
title="{{i18n 'admin_js.admin.groups.delete'}}">
{{fa-icon "times"}}
</button>
{{/unless}}
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/templates/group_member.hbs` at line 1, Replace
the anchor used for the removal action with a button to match project convention
and restore keyboard accessibility: in the template where {{avatar member
imageSize="small"}} {{member.username}} and the conditional {{`#unless`
automatic}} is rendered, change the <a class='remove' {{action "removeMember"
member}}> element to a <button type="button" class="remove" {{action
"removeMember" member}}> (keeping the {{fa-icon "times"}} inside) so the
removeMember action is invoked from an accessible, focusable control.

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

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 +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 | 🟡 Minor

Syntax error: extraneous semicolon after closing brace.

Line 82 has a semicolon immediately after the closing brace of the if statement, which is a syntax error or no-op empty statement.

🐛 Proposed fix
   destroy: function(){
-    if (!this.get('id')) { return };
+    if (!this.get('id')) { return; }
     return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"});
   },
📝 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
destroy: function(){
if (!this.get('id')) { return };
return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"});
},
destroy: function(){
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 81 - 84, The
destroy method contains an extraneous semicolon after the if-block closing brace
which creates a stray empty statement; in the destroy function (method name:
destroy) remove the semicolon immediately following the if (!this.get('id')) {
return } so the if statement is properly terminated and the subsequent return
Discourse.ajax(...) executes as intended.


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