From 35745166b5a4d1f48b206192b21d8f8b44be67ef Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 26 Mar 2018 14:30:37 +0800 Subject: [PATCH] UX: New group membership management workflow. https://meta.discourse.org/t/adding-owners-members-ux-is-inconsistent-and-misleading/58084 --- .../group-navigation-dropdown.js.es6 | 25 ++++++++++++ .../discourse/controllers/group-index.js.es6 | 14 ++++--- .../controllers/group-membership.js.es6 | 30 ++++++++++++++ .../discourse/controllers/group.js.es6 | 5 +++ .../javascripts/discourse/models/group.js.es6 | 13 +++--- .../discourse/routes/group-index.js.es6 | 7 ++++ .../javascripts/discourse/routes/group.js.es6 | 8 ++++ .../discourse/templates/group-edit.hbs | 4 -- .../javascripts/discourse/templates/group.hbs | 6 +++ .../templates/modal/group-membership.hbs | 13 ++++++ app/assets/stylesheets/common/base/group.scss | 6 +++ app/controllers/groups_controller.rb | 40 ++++++++++--------- app/models/user.rb | 18 +++++++-- config/locales/client.en.yml | 3 ++ config/locales/server.en.yml | 4 +- spec/models/user_spec.rb | 38 ++++++++++-------- spec/requests/groups_controller_spec.rb | 34 ++++++++++++++-- .../acceptance/group-index-test.js.es6 | 10 +++++ 18 files changed, 218 insertions(+), 60 deletions(-) create mode 100644 app/assets/javascripts/discourse/components/group-navigation-dropdown.js.es6 create mode 100644 app/assets/javascripts/discourse/controllers/group-membership.js.es6 create mode 100644 app/assets/javascripts/discourse/templates/modal/group-membership.hbs diff --git a/app/assets/javascripts/discourse/components/group-navigation-dropdown.js.es6 b/app/assets/javascripts/discourse/components/group-navigation-dropdown.js.es6 new file mode 100644 index 0000000000..263fc426e4 --- /dev/null +++ b/app/assets/javascripts/discourse/components/group-navigation-dropdown.js.es6 @@ -0,0 +1,25 @@ +import DropdownSelectBox from "select-kit/components/dropdown-select-box"; + +export default DropdownSelectBox.extend({ + classNames: ["group-navigation-dropdown", "pull-right"], + nameProperty: "label", + headerIcon: ["bars"], + showFullTitle: false, + + computeContent() { + const content = []; + + content.push({ + id: "manageMembership", + icon: "user-plus", + label: I18n.t("groups.add_members.title"), + description: I18n.t("groups.add_members.description"), + }); + + return content; + }, + + mutateValue(value) { + this.get(value)(this.get('model')); + } +}); diff --git a/app/assets/javascripts/discourse/controllers/group-index.js.es6 b/app/assets/javascripts/discourse/controllers/group-index.js.es6 index 83c6b9f3e8..fffc78e0ac 100644 --- a/app/assets/javascripts/discourse/controllers/group-index.js.es6 +++ b/app/assets/javascripts/discourse/controllers/group-index.js.es6 @@ -26,14 +26,16 @@ export default Ember.Controller.extend({ const model = this.get('model'); if (model) { - model.findMembers({ - order: this.get('order'), - desc: this.get('desc'), - filter: this.get('filter'), - }).finally(() => this.set('loading', false)); + model.findMembers(this.get('memberParams')) + .finally(() => this.set('loading', false)); } }, + @computed('order', 'desc', 'filter') + memberParams(order, desc, filter) { + return { order, desc, filter }; + }, + @computed('model.members') hasMembers(members) { return members && members.length > 0; @@ -59,7 +61,7 @@ export default Ember.Controller.extend({ }, removeMember(user) { - this.get('model').removeMember(user); + this.get('model').removeMember(user, this.get('memberParams')); }, makeOwner(username) { diff --git a/app/assets/javascripts/discourse/controllers/group-membership.js.es6 b/app/assets/javascripts/discourse/controllers/group-membership.js.es6 new file mode 100644 index 0000000000..76dafb878a --- /dev/null +++ b/app/assets/javascripts/discourse/controllers/group-membership.js.es6 @@ -0,0 +1,30 @@ +import ModalFunctionality from 'discourse/mixins/modal-functionality'; +import computed from 'ember-addons/ember-computed-decorators'; +import { extractError } from 'discourse/lib/ajax-error'; + +export default Ember.Controller.extend(ModalFunctionality, { + loading: false, + + @computed('model.usernames') + disableAddButton(usernames) { + return !usernames || !(usernames.length > 0); + }, + + actions: { + addMembers() { + if (Em.isEmpty(this.get("model.usernames"))) { return; } + + 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')); + }, + }, +}); diff --git a/app/assets/javascripts/discourse/controllers/group.js.es6 b/app/assets/javascripts/discourse/controllers/group.js.es6 index c0df0d6513..46faf5ae8b 100644 --- a/app/assets/javascripts/discourse/controllers/group.js.es6 +++ b/app/assets/javascripts/discourse/controllers/group.js.es6 @@ -84,6 +84,11 @@ export default Ember.Controller.extend({ return this.currentUser && messageable; }, + @computed('model') + canManageGroup(model) { + return this.currentUser && this.currentUser.canManageGroup(model); + }, + actions: { messageGroup() { this.send('createNewMessageViaParams', this.get('model.name')); diff --git a/app/assets/javascripts/discourse/models/group.js.es6 b/app/assets/javascripts/discourse/models/group.js.es6 index 34b5e75231..14844e58ec 100644 --- a/app/assets/javascripts/discourse/models/group.js.es6 +++ b/app/assets/javascripts/discourse/models/group.js.es6 @@ -65,24 +65,21 @@ const Group = RestModel.extend({ }); }, - removeMember(member) { - var self = this; + removeMember(member, params) { return ajax('/groups/' + this.get('id') + '/members.json', { type: "DELETE", data: { user_id: member.get("id") } - }).then(function() { - // reload member list - self.findMembers(); + }).then(() => { + this.findMembers(params); }); }, addMembers(usernames) { - var self = this; return ajax('/groups/' + this.get('id') + '/members.json', { type: "PUT", data: { usernames: usernames } - }).then(function() { - self.findMembers(); + }).then(response => { + this.findMembers({ filter: response.usernames.join(',') }); }); }, diff --git a/app/assets/javascripts/discourse/routes/group-index.js.es6 b/app/assets/javascripts/discourse/routes/group-index.js.es6 index 4d37501f30..815308f20a 100644 --- a/app/assets/javascripts/discourse/routes/group-index.js.es6 +++ b/app/assets/javascripts/discourse/routes/group-index.js.es6 @@ -17,5 +17,12 @@ export default Discourse.Route.extend({ }); controller.refreshMembers(); + }, + + actions: { + didTransition() { + this.controllerFor("group-index").set("filterInput", this._params.filter); + return true; + } } }); diff --git a/app/assets/javascripts/discourse/routes/group.js.es6 b/app/assets/javascripts/discourse/routes/group.js.es6 index 3df20f794b..6bb9610aed 100644 --- a/app/assets/javascripts/discourse/routes/group.js.es6 +++ b/app/assets/javascripts/discourse/routes/group.js.es6 @@ -1,4 +1,5 @@ import Group from 'discourse/models/group'; +import showModal from 'discourse/lib/show-modal'; export default Discourse.Route.extend({ @@ -16,5 +17,12 @@ export default Discourse.Route.extend({ setupController(controller, model) { controller.setProperties({ model, counts: this.get('counts') }); + }, + + actions: { + showGroupMembershipModal(model) { + showModal('group-membership', { model }); + this.controllerFor('modal').set('modalClass', 'group-membership-modal'); + }, } }); diff --git a/app/assets/javascripts/discourse/templates/group-edit.hbs b/app/assets/javascripts/discourse/templates/group-edit.hbs index b149a4ad7f..7e56a0ead2 100644 --- a/app/assets/javascripts/discourse/templates/group-edit.hbs +++ b/app/assets/javascripts/discourse/templates/group-edit.hbs @@ -10,10 +10,6 @@ {{d-editor value=model.bio_raw class="group-edit-bio"}} -
- {{group-members-input model=model}} -
-
{{group-flair-inputs model=model}}
diff --git a/app/assets/javascripts/discourse/templates/group.hbs b/app/assets/javascripts/discourse/templates/group.hbs index dfb900b91e..6fc009b830 100644 --- a/app/assets/javascripts/discourse/templates/group.hbs +++ b/app/assets/javascripts/discourse/templates/group.hbs @@ -42,6 +42,12 @@
{{group-navigation group=model currentPath=application.currentPath tabs=tabs}} + {{#if canManageGroup}} + {{group-navigation-dropdown + model=model + manageMembership=(route-action "showGroupMembershipModal")}} + {{/if}} + {{#if displayGroupMessageButton}} {{d-button action="messageGroup" diff --git a/app/assets/javascripts/discourse/templates/modal/group-membership.hbs b/app/assets/javascripts/discourse/templates/modal/group-membership.hbs new file mode 100644 index 0000000000..7c5b07bfc6 --- /dev/null +++ b/app/assets/javascripts/discourse/templates/modal/group-membership.hbs @@ -0,0 +1,13 @@ +{{#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"}} +{{/d-modal-body}} + +
+ {{d-button action="addMembers" + class="add btn-primary" + icon="plus" + disabled=disableAddButton + label="groups.add"}} +
diff --git a/app/assets/stylesheets/common/base/group.scss b/app/assets/stylesheets/common/base/group.scss index 059680df3e..6d42830f74 100644 --- a/app/assets/stylesheets/common/base/group.scss +++ b/app/assets/stylesheets/common/base/group.scss @@ -208,3 +208,9 @@ table.group-members { } } } + +.group-membership { + .ac-wrap { + width: 100% !important; + } +} diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 70d219afc0..b41f170edf 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -188,6 +188,16 @@ class GroupsController < ApplicationController users = group.users.human_users total = users.count + if (filter = params[:filter]).present? + filter = filter.split(',') if filter.include?(',') + + if current_user&.admin + users = users.filter_by_username_or_email(filter) + else + users = users.filter_by_username(filter) + end + end + members = users .order('NOT group_users.owner') .order(order) @@ -200,16 +210,6 @@ class GroupsController < ApplicationController .order(username_lower: dir) .where('group_users.owner') - if (filter = params[:filter]).present? - if current_user&.admin - owners = owners.filter_by_username_or_email(filter) - members = members.filter_by_username_or_email(filter) - else - owners = owners.filter_by_username(filter) - members = members.filter_by_username(filter) - end - end - render json: { members: serialize_data(members, GroupUserSerializer), owners: serialize_data(owners, GroupUserSerializer), @@ -250,19 +250,21 @@ class GroupsController < ApplicationController end end - users.each do |user| - if !group.users.include?(user) + if (usernames = group.users.where(id: users.pluck(:id)).pluck(:username)).present? + render_json_error(I18n.t( + "groups.errors.member_already_exist", + username: usernames.join(", "), + count: usernames.size + )) + else + users.each do |user| group.add(user) GroupActionLogger.new(current_user, group).log_add_user_to_group(user) - else - return render_json_error I18n.t('groups.errors.member_already_exist', username: user.username) end - end - if group.save - render json: success_json - else - render_json_error(group) + render json: success_json.merge!( + usernames: users.map(&:username) + ) end end diff --git a/app/models/user.rb b/app/models/user.rb index c89e5dd69c..a821e0c572 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -160,7 +160,11 @@ class User < ActiveRecord::Base scope :activated, -> { where(active: true) } scope :filter_by_username, ->(filter) do - where('username_lower ILIKE ?', "%#{filter}%") + if filter.is_a?(Array) + where('username_lower ~* ?', "(#{filter.join('|')})") + else + where('username_lower ILIKE ?', "%#{filter}%") + end end scope :filter_by_username_or_email, ->(filter) do @@ -171,11 +175,19 @@ class User < ActiveRecord::Base end end - joins(:primary_email) - .where( + users = joins(:primary_email) + + if filter.is_a?(Array) + users.where( + 'username_lower ~* :filter OR lower(user_emails.email) SIMILAR TO :filter', + filter: "(#{filter.join('|')})" + ) + else + users.where( 'username_lower ILIKE :filter OR lower(user_emails.email) ILIKE :filter', filter: "%#{filter}%" ) + end end module NewTopicDuration diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index af7318e25e..e58c141552 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -464,6 +464,9 @@ en: one: "Group" other: "Groups" activity: "Activity" + add_members: + title: "Add Members" + description: "Manage the membership of this group" members: title: "Members" filter_placeholder_admin: "username or email" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 1e8ae34d90..761d2591c7 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -296,7 +296,9 @@ en: bulk_add: "%{users_added} users have been added to the group." errors: can_not_modify_automatic: "You cannot modify an automatic group" - member_already_exist: "'%{username}' is already a member of this group." + member_already_exist: + one: "'%{username}' is already a member of this group." + other: "The following users are already members of this group: %{username}" invalid_domain: "'%{domain}' is not a valid domain." invalid_incoming_email: "'%{email}' is not a valid email address." email_already_used_in_group: "'%{email}' is already used by the group '%{group_name}'." diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 17268fa7a1..2f0f117312 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1651,16 +1651,29 @@ describe User do end end + def filter_by(method) + username = 'someuniqueusername' + user.update!(username: username) + + username2 = 'awesomeusername' + user2 = Fabricate(:user, username: username2) + + expect(User.public_send(method, username)) + .to eq([user]) + + expect(User.public_send(method, 'UNiQuE')) + .to eq([user]) + + expect(User.public_send(method, [username, username2])) + .to contain_exactly(user, user2) + + expect(User.public_send(method, ['UNiQuE', 'sOME'])) + .to contain_exactly(user, user2) + end + describe '#filter_by_username' do it 'should be able to filter by username' do - username = 'someuniqueusername' - user.update!(username: username) - - expect(User.filter_by_username(username)) - .to eq([user]) - - expect(User.filter_by_username('UNiQuE')) - .to eq([user]) + filter_by(:filter_by_username) end end @@ -1677,14 +1690,7 @@ describe User do end it 'should be able to filter by username' do - username = 'someuniqueusername' - user.update!(username: username) - - expect(User.filter_by_username_or_email(username)) - .to eq([user]) - - expect(User.filter_by_username_or_email('UNiQuE')) - .to eq([user]) + filter_by(:filter_by_username_or_email) end end end diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 494d149989..f859a8058d 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -427,12 +427,16 @@ describe GroupsController do email = 'uniquetest@discourse.org' user1.update!(email: email) - [email.upcase, 'QUEtes'].each do |filter| + { + email.upcase => [user1.id], + 'QUEtes' => [user1.id], + "#{user1.email},#{user2.email}" => [user1.id, user2.id] + }.each do |filter, ids| get "/groups/#{group.name}/members.json", params: { filter: filter } expect(response.status).to eq(200) members = JSON.parse(response.body)["members"] - expect(members.map { |m| m["id"] }).to contain_exactly(user1.id) + expect(members.map { |m| m["id"] }).to contain_exactly(*ids) end end @@ -519,7 +523,7 @@ describe GroupsController do sign_in(admin) end - context 'add_members' do + context '#add_members' do it "can make incremental adds" do user2 = Fabricate(:user) @@ -573,12 +577,36 @@ describe GroupsController do expect(response).to be_success end + + it 'fails when multiple member already exists' do + user3 = Fabricate(:user) + [user2, user3].each { |user| group.add(user) } + + expect do + put "/groups/#{group.id}/members.json", + params: { user_emails: [user1.email, user2.email, user3.email].join(",") } + end.to change { group.users.count }.by(0) + + expect(response.status).to eq(422) + + expect(JSON.parse(response.body)["errors"]).to include(I18n.t( + "groups.errors.member_already_exist", + username: "#{user2.username}, #{user3.username}", + count: 2 + )) + end end it "returns 422 if member already exists" do put "/groups/#{group.id}/members.json", params: { usernames: user.username } expect(response.status).to eq(422) + + expect(JSON.parse(response.body)["errors"]).to include(I18n.t( + "groups.errors.member_already_exist", + username: user.username, + count: 1 + )) end it "returns 404 if member is not found" do diff --git a/test/javascripts/acceptance/group-index-test.js.es6 b/test/javascripts/acceptance/group-index-test.js.es6 index caa0a3b8d9..701fb8af59 100644 --- a/test/javascripts/acceptance/group-index-test.js.es6 +++ b/test/javascripts/acceptance/group-index-test.js.es6 @@ -9,6 +9,11 @@ QUnit.test("Viewing Members as anon user", assert => { assert.ok(count('.avatar-flair .fa-adjust') === 1, "it displays the group's avatar flair"); assert.ok(count('.group-members tr') > 0, "it lists group members"); + assert.ok( + count('.group-navigation-dropdown') === 0, + 'it should not display the group navigation dropdown menu' + ); + assert.ok( count('.group-member-dropdown') === 0, 'it does not allow anon user to manage group members' @@ -29,6 +34,11 @@ QUnit.test("Viewing Members as an admin user", assert => { visit("/groups/discourse"); andThen(() => { + assert.ok( + count('.group-navigation-dropdown') === 1, + 'it should display the group navigation dropdown menu' + ); + assert.ok( count('.group-member-dropdown') > 0, 'it allows admin user to manage group members'