diff --git a/app/assets/javascripts/discourse/components/group-members-input.js.es6 b/app/assets/javascripts/discourse/components/group-members-input.js.es6
index b2d892095b9..691eb051bbc 100644
--- a/app/assets/javascripts/discourse/components/group-members-input.js.es6
+++ b/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) {
diff --git a/app/assets/javascripts/discourse/models/group.js.es6 b/app/assets/javascripts/discourse/models/group.js.es6
index c156613ef56..51ad388a1f3 100644
--- a/app/assets/javascripts/discourse/models/group.js.es6
+++ b/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() {
diff --git a/app/assets/javascripts/discourse/templates/components/group-members-input.hbs b/app/assets/javascripts/discourse/templates/components/group-members-input.hbs
index 24db7b5c877..9127b7096de 100644
--- a/app/assets/javascripts/discourse/templates/components/group-members-input.hbs
+++ b/app/assets/javascripts/discourse/templates/components/group-members-input.hbs
@@ -17,10 +17,12 @@
{{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}}
{{/unless}}
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index 2a1f9a36e02..3da78834fd4 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/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
diff --git a/app/models/group.rb b/app/models/group.rb
index e99d3482977..517247beb6d 100644
--- a/app/models/group.rb
+++ b/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
diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb
index 5e6b51581ff..833613133e1 100644
--- a/spec/controllers/admin/groups_controller_spec.rb
+++ b/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)
diff --git a/spec/integration/admin/groups_spec.rb b/spec/integration/admin/groups_spec.rb
new file mode 100644
index 00000000000..4f41f6d219a
--- /dev/null
+++ b/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
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 3a53b68bd07..dece2cf4e38 100644
--- a/spec/models/group_spec.rb
+++ b/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