UX: Better group creation workflow.

* Owners and users can now be added to a group during creation.

https://meta.discourse.org/t/you-cannot-allow-membership-requests-without-any-owners/64760/3

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

@@ -9,19 +9,18 @@
     {{/if}}
   </div>
 
-  {{#if model.id}}
-    {{#unless model.automatic}}
-      <div>
-        <label for='full_name'>{{i18n 'groups.edit.full_name'}}</label>
-        {{input type='text' name='full_name' value=model.full_name class='group-edit-full-name'}}
-      </div>
+  {{#unless model.automatic}}
+    <div>
+      <label for='full_name'>{{i18n 'groups.edit.full_name'}}</label>
+      {{input type='text' name='full_name' value=model.full_name class='group-edit-full-name'}}
+    </div>
 
-      <div>
-        <label for="bio">{{i18n 'groups.bio'}}</label>
-        {{d-editor value=model.bio_raw}}
-      </div>
+    <div>
+      <label for="bio">{{i18n 'groups.bio'}}</label>
+      {{d-editor value=model.bio_raw}}
+    </div>
 
-      {{#if model.hasOwners}}
+    {{#if model.hasOwners}}
       <div>
         <label for='owner-list'>{{i18n 'admin.groups.group_owners'}}</label>
         <div class="ac-wrap clearfix" id='owner-list'>
@@ -30,17 +29,28 @@
           {{/each}}
         </div>
       </div>
-      {{/if}}
-      <div>
-        <label for="owner-selector">{{i18n 'admin.groups.add_owners'}}</label>
-        {{user-selector usernames=model.ownerUsernames placeholderKey="admin.groups.selector_placeholder" id="owner-selector"}}
-        {{d-button action="addOwners" class="add" icon="plus" label="admin.groups.add"}}
-      </div>
-    {{/unless}}
+    {{/if}}
+
     <div>
-      {{group-members-input model=model}}
+      <label for="owner-selector">{{i18n 'admin.groups.add_owners'}}</label>
+
+      {{user-selector usernames=model.ownerUsernames
+          placeholderKey="admin.groups.selector_placeholder"
+          id="owner-selector"}}
+
+      {{#if model.id}}
+        {{d-button
+            action="addOwners"
+            class="add"
+            icon="plus"
+            label="admin.groups.add"}}
+      {{/if}}
     </div>
-  {{/if}}
+  {{/unless}}
+
+  <div>
+    {{group-members-input model=model addButton=model.id}}
+  </div>
 
   <div>
     <label for="visiblity">{{i18n 'groups.visibility_levels.title'}}</label>

app/assets/javascripts/discourse/components/group-members-input.js.es6

@@ -4,6 +4,7 @@ import { propertyEqual } from 'discourse/lib/computed';
 
 export default Ember.Component.extend({
   classNames: ["group-members-input"],
+  addButton: true,
 
   @computed('model.limit', 'model.offset', 'model.user_count')
   currentPage(limit, offset, userCount) {

app/assets/javascripts/discourse/models/group.js.es6

@@ -130,7 +130,7 @@ const Group = RestModel.extend({
   },
 
   asJSON() {
-    return {
+    const attrs = {
       name: this.get('name'),
       alias_level: this.get('alias_level'),
       visibility_level: this.get('visibility_level'),
@@ -149,13 +149,26 @@ const Group = RestModel.extend({
       full_name: this.get('full_name'),
       default_notification_level: this.get('default_notification_level')
     };
+
+    if (!this.get('id')) {
+      attrs['usernames'] = this.get('usernames');
+      attrs['owner_usernames'] = this.get('ownerUsernames');
+    }
+
+    return attrs;
   },
 
   create() {
-    var self = this;
-    return ajax("/admin/groups", { type: "POST", data:  { group: this.asJSON() } }).then(function(resp) {
-      self.set('id', resp.basic_group.id);
-    });
+    return ajax("/admin/groups", { type: "POST", data:  { group: this.asJSON() } })
+      .then(resp => {
+        this.setProperties({
+          id: resp.basic_group.id,
+          usernames: null,
+          ownerUsernames: null
+        });
+
+        this.findMembers();
+      });
   },
 
   save() {

app/assets/javascripts/discourse/templates/components/group-members-input.hbs

@@ -17,10 +17,12 @@
   <div class="group-members-input-selector">
     {{user-selector usernames=model.usernames}}
 
-    {{d-button action="addMembers"
-        class="add"
-        icon="plus"
-        disabled=disableAddButton
-        label="groups.edit.add_members"}}
+    {{#if addButton}}
+      {{d-button action="addMembers"
+          class="add"
+          icon="plus"
+          disabled=disableAddButton
+          label="groups.edit.add_members"}}
+    {{/if}}
   </div>
 {{/unless}}

app/controllers/admin/groups_controller.rb

@@ -32,10 +32,7 @@ class Admin::GroupsController < Admin::AdminController
   end
 
   def create
-    group = Group.new
-
-    group.name = (group_params[:name] || '').strip
-    save_group(group)
+    save_group(Group.new)
   end
 
   def update
@@ -47,6 +44,7 @@ class Admin::GroupsController < Admin::AdminController
   end
 
   def save_group(group)
+    group.name = group_params[:name] if group_params[:name].present? && !group.automatic
     group.alias_level = group_params[:alias_level].to_i if group_params[:alias_level].present?
 
     if group_params[:visibility_level]
@@ -81,8 +79,27 @@ class Admin::GroupsController < Admin::AdminController
       group.allow_membership_requests = group_params[:allow_membership_requests]
     end
 
+    if group_params[:owner_usernames].present?
+      owner_ids = User.where(
+        username: group_params[:owner_usernames].split(",")
+      ).pluck(:id)
+
+      owner_ids.each do |user_id|
+        group.group_users.build(user_id: user_id, owner: true)
+      end
+    end
+
+    if group_params[:usernames].present?
+      user_ids = User.where(username: group_params[:usernames].split(",")).pluck(:id)
+      user_ids -= owner_ids if owner_ids
+
+      user_ids.each do |user_id|
+        group.group_users.build(user_id: user_id)
+      end
+    end
+
     if group.save
-      Group.reset_counters(group.id, :group_users)
+      group.restore_user_count!
 
       yield(group) if block_given?
 
@@ -111,8 +128,7 @@ class Admin::GroupsController < Admin::AdminController
   def add_owners
     group = Group.find(params.require(:id))
     return can_not_modify_automatic if group.automatic
-
-    users = User.where(username: params[:usernames].split(","))
+    users = User.where(username: group_params[:usernames].split(","))
 
     users.each do |user|
       group_action_logger = GroupActionLogger.new(current_user, group)
@@ -125,7 +141,7 @@ class Admin::GroupsController < Admin::AdminController
       group_action_logger.log_make_user_group_owner(user)
     end
 
-    Group.reset_counters(group.id, :group_users)
+    group.restore_user_count!
 
     render json: success_json
   end
@@ -157,7 +173,7 @@ class Admin::GroupsController < Admin::AdminController
       :automatic_membership_retroactive, :title, :primary_group,
       :grant_trust_level, :incoming_email, :flair_url, :flair_bg_color,
       :flair_color, :bio_raw, :public, :allow_membership_requests, :full_name,
-      :default_notification_level
+      :default_notification_level, :usernames, :owner_usernames
     )
   end
 end

app/models/group.rb

@@ -37,10 +37,10 @@ class Group < ActiveRecord::Base
   end
 
   validate :name_format_validator
-  validates_uniqueness_of :name, case_sensitive: false
+  validates :name, presence: true, uniqueness: { case_sensitive: false }
   validate :automatic_membership_email_domains_format_validator
   validate :incoming_email_validator
-  validate :can_allow_membership_requests
+  validate :can_allow_membership_requests, if: :allow_membership_requests
   validates :flair_url, url: true, if: Proc.new { |g| g.flair_url && g.flair_url[0,3] != 'fa-' }
 
   AUTO_GROUPS = {
@@ -426,9 +426,9 @@ class Group < ActiveRecord::Base
 
   def add_owner(user)
     if group_user = self.group_users.find_by(user: user)
-      group_user.update_attributes!(owner: true) if !group_user.owner
+      group_user.update!(owner: true) if !group_user.owner
     else
-      GroupUser.create!(user: user, group: self, owner: true)
+      self.group_users.create!(user: user, owner: true)
     end
   end
 
@@ -437,7 +437,9 @@ class Group < ActiveRecord::Base
   end
 
   def bulk_add(user_ids)
-    if user_ids.present?
+    return unless user_ids.present?
+
+    Group.transaction do
       sql = <<~SQL
       INSERT INTO group_users
         (group_id, user_id, created_at, updated_at)
@@ -470,12 +472,16 @@ class Group < ActiveRecord::Base
       if user_attributes.present?
         User.where(id: user_ids).update_all(user_attributes)
       end
-
-      if self.grant_trust_level.present?
-        Jobs.enqueue(:bulk_grant_trust_level, user_ids: user_ids, trust_level: self.grant_trust_level)
-      end
     end
-    true
+
+    if self.grant_trust_level.present?
+      Jobs.enqueue(:bulk_grant_trust_level,
+        user_ids: user_ids,
+        trust_level: self.grant_trust_level
+      )
+    end
+
+    self
   end
 
   def staff?
@@ -485,6 +491,7 @@ class Group < ActiveRecord::Base
   protected
 
     def name_format_validator
+      self.name.strip!
       UsernameValidator.perform_validation(self, 'name')
     end
 
@@ -565,7 +572,16 @@ class Group < ActiveRecord::Base
   private
 
     def can_allow_membership_requests
-      if self.allow_membership_requests && !self.group_users.where(owner: true).exists?
+      valid = true
+
+      valid =
+        if self.persisted?
+          self.group_users.where(owner: true).exists?
+        else
+          self.group_users.any?(&:owner)
+        end
+
+      if !valid
         self.errors.add(:base, I18n.t('groups.errors.cant_allow_membership_requests'))
       end
     end

spec/controllers/admin/groups_controller_spec.rb

@@ -39,21 +39,6 @@ describe Admin::GroupsController do
     end
   end
 
-  context "#create" do
-
-    it "strip spaces on the group name" do
-      xhr :post, :create, { group: { name: " bob " } }
-
-      expect(response.status).to eq(200)
-
-      groups = Group.where(name: "bob").to_a
-
-      expect(groups.count).to eq(1)
-      expect(groups[0].name).to eq("bob")
-    end
-
-  end
-
   context "#update" do
     it 'should update a group' do
       group.add_owner(user)

spec/integration/admin/groups_spec.rb

@@ -0,0 +1,29 @@
+require 'rails_helper'
+
+RSpec.describe "Managing groups as an admin" do
+  let(:admin) { Fabricate(:admin) }
+  let(:user) { Fabricate(:user) }
+
+  before do
+    sign_in(admin)
+  end
+
+  describe 'creating a new group' do
+    it 'should work' do
+      post "/admin/groups.json", group: {
+        name: 'testing',
+        usernames: [admin.username, user.username].join(","),
+        owner_usernames: [user.username].join(","),
+        allow_membership_requests: true
+      }
+
+      expect(response).to be_success
+
+      group = Group.last
+
+      expect(group.name).to eq('testing')
+      expect(group.users).to contain_exactly(admin, user)
+      expect(group.allow_membership_requests).to eq(true)
+    end
+  end
+end

spec/models/group_spec.rb

@@ -47,6 +47,13 @@ describe Group do
       expect(group.valid?).to eq false
     end
 
+    it 'strips trailing and leading spaces' do
+      group.name = '  dragon  '
+
+      expect(group.save).to eq(true)
+      expect(group.reload.name).to eq('dragon')
+    end
+
     it "is invalid for case-insensitive existing names" do
       build(:group, name: 'this_is_a_name').save
       group.name = 'This_Is_A_Name'
@@ -79,6 +86,22 @@ describe Group do
     end
 
     context 'when a group has no owners' do
+      describe 'group has not been persisted' do
+        it 'should not allow membership requests' do
+          group = Fabricate.build(:group, allow_membership_requests: true)
+
+          expect(group.valid?).to eq(false)
+
+          expect(group.errors.full_messages).to include(I18n.t(
+            "groups.errors.cant_allow_membership_requests"
+          ))
+
+          group.group_users.build(user_id: user.id, owner: true)
+
+          expect(group.valid?).to eq(true)
+        end
+      end
+
       it 'should not allow membership requests' do
         group.allow_membership_requests = true
 
@@ -533,4 +556,12 @@ describe Group do
       expect(Group.search_group('test2')).to eq([])
     end
   end
+
+  describe '#bulk_add' do
+    it 'should be able to add multiple users' do
+      group.bulk_add([user.id, admin.id])
+
+      expect(group.group_users.map(&:user_id)).to contain_exactly(user.id, admin.id)
+    end
+  end
 end