UX: New group membership management workflow.

https://meta.discourse.org/t/adding-owners-members-ux-is-inconsistent-and-misleading/58084

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'));
+  }
+});

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

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

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'));

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

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

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

app/assets/javascripts/discourse/templates/group-edit.hbs

@@ -10,10 +10,6 @@
       {{d-editor value=model.bio_raw class="group-edit-bio"}}
     </div>
 
-    <div class="control-group">
-      {{group-members-input model=model}}
-    </div>
-
     <div class="control-group">
       {{group-flair-inputs model=model}}
     </div>

app/assets/javascripts/discourse/templates/group.hbs

@@ -42,6 +42,12 @@
     <div class="container">
       {{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"

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}}
+
+<div class="modal-footer">
+  {{d-button action="addMembers"
+      class="add btn-primary"
+      icon="plus"
+      disabled=disableAddButton
+      label="groups.add"}}
+</div>

app/assets/stylesheets/common/base/group.scss

@@ -208,3 +208,9 @@ table.group-members {
     }
   }
 }
+
+.group-membership {
+  .ac-wrap {
+    width: 100% !important;
+  }
+}

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
 

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

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"

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}'."

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

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

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'