From dcd1d422d1251f104abdecb72d151ea7c4357dfb Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 26 Mar 2018 17:33:03 +0800 Subject: [PATCH] UX: Allow admins to set users as owners while adding users. https://meta.discourse.org/t/adding-owners-members-ux-is-inconsistent-and-misleading/58084/9 --- .../controllers/group-membership.js.es6 | 33 ++++++++++++------- .../javascripts/discourse/models/group.js.es6 | 22 ++++++++++--- .../templates/modal/group-membership.hbs | 18 ++++++++-- .../stylesheets/common/base/discourse.scss | 4 +++ app/assets/stylesheets/common/base/group.scss | 15 +++++++++ app/controllers/admin/groups_controller.rb | 2 +- config/locales/client.en.yml | 2 ++ spec/requests/admin/groups_controller_spec.rb | 6 +++- .../acceptance/group-index-test.js.es6 | 19 +++++++++++ 9 files changed, 99 insertions(+), 22 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/group-membership.js.es6 b/app/assets/javascripts/discourse/controllers/group-membership.js.es6 index 76dafb878a3..8c22d2765e2 100644 --- a/app/assets/javascripts/discourse/controllers/group-membership.js.es6 +++ b/app/assets/javascripts/discourse/controllers/group-membership.js.es6 @@ -4,6 +4,7 @@ import { extractError } from 'discourse/lib/ajax-error'; export default Ember.Controller.extend(ModalFunctionality, { loading: false, + setAsOwner: false, @computed('model.usernames') disableAddButton(usernames) { @@ -12,19 +13,27 @@ export default Ember.Controller.extend(ModalFunctionality, { actions: { addMembers() { - if (Em.isEmpty(this.get("model.usernames"))) { return; } + const model = this.get('model'); + const usernames = model.get('usernames'); + if (Em.isEmpty(usernames)) { return; } + let promise; - this.get("model").addMembers(this.get("model.usernames")) - .then(() => { - this.transitionToRoute( - "group.members", - this.get('model.name'), - { queryParams: { filter: this.get('model.usernames') } } - ); - this.set("model.usernames", null); - this.send('closeModal'); - }) - .catch(error => this.flash(extractError(error), 'error')); + if (this.get('setAsOwner')) { + promise = model.addOwners(usernames, true); + } else { + promise = model.addMembers(usernames, true); + } + + promise.then(() => { + this.transitionToRoute( + "group.members", + this.get('model.name'), + { queryParams: { filter: usernames } } + ); + model.set("usernames", null); + this.send('closeModal'); + }) + .catch(error => this.flash(extractError(error), 'error')); }, }, }); diff --git a/app/assets/javascripts/discourse/models/group.js.es6 b/app/assets/javascripts/discourse/models/group.js.es6 index 14844e58ec1..4ad96c34fb6 100644 --- a/app/assets/javascripts/discourse/models/group.js.es6 +++ b/app/assets/javascripts/discourse/models/group.js.es6 @@ -74,24 +74,36 @@ const Group = RestModel.extend({ }); }, - addMembers(usernames) { + addMembers(usernames, filter) { return ajax('/groups/' + this.get('id') + '/members.json', { type: "PUT", data: { usernames: usernames } }).then(response => { - this.findMembers({ filter: response.usernames.join(',') }); + if (filter) { + this._filterMembers(response); + } else { + this.findMembers(); + } }); }, - addOwners(usernames) { + addOwners(usernames, filter) { return ajax(`/admin/groups/${this.get('id')}/owners.json`, { type: "PUT", data: { group: { usernames: usernames } } - }).then(() => { - this.findMembers(); + }).then(response => { + if (filter) { + this._filterMembers(response); + } else { + this.findMembers(); + } }); }, + _filterMembers(response) { + return this.findMembers({ filter: response.usernames.join(",") }); + }, + @computed("display_name", "name") displayName(groupDisplayName, name) { return groupDisplayName || name; diff --git a/app/assets/javascripts/discourse/templates/modal/group-membership.hbs b/app/assets/javascripts/discourse/templates/modal/group-membership.hbs index 7c5b07bfc66..92c35ab9f0a 100644 --- a/app/assets/javascripts/discourse/templates/modal/group-membership.hbs +++ b/app/assets/javascripts/discourse/templates/modal/group-membership.hbs @@ -1,7 +1,19 @@ {{#d-modal-body class='group-membership' title="groups.add_members.title"}} - {{user-selector usernames=model.usernames - placeholderKey="groups.selector_placeholder" - id="group-membership-user-selector"}} +
+ + + {{user-selector + usernames=model.usernames + placeholderKey="groups.selector_placeholder" + id="group-membership-user-selector"}} +
+ + {{#if this.currentUser.admin}} +
+ + {{input type="checkbox" class="inline" checked=setAsOwner}} +
+ {{/if}} {{/d-modal-body}}
diff --git a/app/assets/stylesheets/common/base/discourse.scss b/app/assets/stylesheets/common/base/discourse.scss index 21060272c52..1c8e0ca17dc 100644 --- a/app/assets/stylesheets/common/base/discourse.scss +++ b/app/assets/stylesheets/common/base/discourse.scss @@ -533,3 +533,7 @@ select { } } } + +.inline { + display: inline; +} diff --git a/app/assets/stylesheets/common/base/group.scss b/app/assets/stylesheets/common/base/group.scss index 6d42830f748..082adb401e3 100644 --- a/app/assets/stylesheets/common/base/group.scss +++ b/app/assets/stylesheets/common/base/group.scss @@ -213,4 +213,19 @@ table.group-members { .ac-wrap { width: 100% !important; } + + label { + font-weight: bold; + } + + .group-membership-make-owner { + label { + display: inline; + vertical-align: middle; + } + + input[type='checkbox'] { + vertical-align: middle; + } + } } diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 8a6a7a72394..5de992cee60 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -166,7 +166,7 @@ class Admin::GroupsController < Admin::AdminController group.restore_user_count! - render json: success_json + render json: success_json.merge!(usernames: users.pluck(:username)) end def remove_owner diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index e58c1415523..523163cd467 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -467,6 +467,8 @@ en: add_members: title: "Add Members" description: "Manage the membership of this group" + usernames: "Usernames" + as_owner: "Set user(s) as owner(s) of this group" members: title: "Members" filter_placeholder_admin: "username or email" diff --git a/spec/requests/admin/groups_controller_spec.rb b/spec/requests/admin/groups_controller_spec.rb index 7d6045b6085..e7f50fbf118 100644 --- a/spec/requests/admin/groups_controller_spec.rb +++ b/spec/requests/admin/groups_controller_spec.rb @@ -40,7 +40,11 @@ RSpec.describe Admin::GroupsController do } } - expect(response).to be_success + expect(response.status).to eq(200) + + response_body = JSON.parse(response.body) + + expect(response_body["usernames"]).to contain_exactly(user.username, admin.username) expect(group.group_users.where(owner: true).map(&:user)) .to contain_exactly(user, admin) diff --git a/test/javascripts/acceptance/group-index-test.js.es6 b/test/javascripts/acceptance/group-index-test.js.es6 index 701fb8af593..cfd52691af1 100644 --- a/test/javascripts/acceptance/group-index-test.js.es6 +++ b/test/javascripts/acceptance/group-index-test.js.es6 @@ -50,4 +50,23 @@ QUnit.test("Viewing Members as an admin user", assert => { 'it should display the right filter placehodler' ); }); + + selectKit('.group-navigation-dropdown').expand().selectRowByValue('manageMembership'); + + andThen(() => { + assert.ok( + count('.group-membership') === 1, + 'it should display the right modal' + ); + + assert.ok( + count('#group-membership-user-selector') === 1, + 'it should display the user selector' + ); + + assert.ok( + count(".group-membership-make-owner input[type='checkbox']") === 1, + 'it should display the input to set users as owners' + ); + }); });