diff --git a/app/models/group.rb b/app/models/group.rb index d8fc3dd1916..c0b5ff68074 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -43,7 +43,7 @@ class Group < ActiveRecord::Base end validate :name_format_validator - validates :name, presence: true, uniqueness: { case_sensitive: false } + validates :name, presence: true validate :automatic_membership_email_domains_format_validator validate :incoming_email_validator validate :can_allow_membership_requests, if: :allow_membership_requests @@ -597,7 +597,19 @@ class Group < ActiveRecord::Base def name_format_validator self.name.strip! - UsernameValidator.perform_validation(self, 'name') + self.name.downcase! + + UsernameValidator.perform_validation(self, 'name') || begin + if will_save_change_to_name? + existing = Group.exec_sql( + User::USERNAME_EXISTS_SQL, username: self.name + ).values.present? + + if existing + errors.add(:name, I18n.t("activerecord.errors.messages.taken")) + end + end + end end def automatic_membership_email_domains_format_validator diff --git a/app/models/user.rb b/app/models/user.rb index a821e0c5720..45bea901600 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1121,12 +1121,28 @@ class User < ActiveRecord::Base self.username_lower = username.downcase end + USERNAME_EXISTS_SQL = <<~SQL + (SELECT 1 FROM users + WHERE users.username_lower = :username) + + UNION ALL + + (SELECT 1 FROM groups + WHERE lower(groups.name) = :username) + SQL + def username_validator username_format_validator || begin - lower = username.downcase - existing = User.find_by(username_lower: lower) - if will_save_change_to_username? && existing && existing.id != self.id - errors.add(:username, I18n.t(:'user.username.unique')) + if will_save_change_to_username? + lower = username.downcase + + existing = User.exec_sql( + USERNAME_EXISTS_SQL, username: lower + ).values.present? + + if existing + errors.add(:username, I18n.t(:'user.username.unique')) + end end end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 255b1d245e2..53b018de6f6 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -5,6 +5,32 @@ describe Group do let(:user) { Fabricate(:user) } let(:group) { Fabricate(:group) } + context 'validations' do + describe '#username' do + context 'when a user with a similar name exists' do + it 'should not be valid' do + new_group = Fabricate.build(:group, name: admin.username.upcase) + + expect(new_group).to_not be_valid + + expect(new_group.errors.full_messages.first) + .to include(I18n.t("activerecord.errors.messages.taken")) + end + end + + context 'when a group with a similar name exists' do + it 'should not be valid' do + new_group = Fabricate.build(:group, name: group.name.upcase) + + expect(new_group).to_not be_valid + + expect(new_group.errors.full_messages.first) + .to include(I18n.t("activerecord.errors.messages.taken")) + end + end + end + end + describe "#posts_for" do it "returns the post in the group" do p = Fabricate(:post) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 2f0f117312f..fe2299cb7bf 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -5,10 +5,36 @@ describe User do let(:user) { Fabricate(:user) } context 'validations' do - it { is_expected.to validate_presence_of :username } - it { is_expected.to validate_presence_of :primary_email } + describe '#username' do + it { is_expected.to validate_presence_of :username } + + describe 'when username already exists' do + it 'should not be valid' do + new_user = Fabricate.build(:user, username: user.username.upcase) + + expect(new_user).to_not be_valid + + expect(new_user.errors.full_messages.first) + .to include(I18n.t(:'user.username.unique')) + end + end + + describe 'when group with a same name already exists' do + it 'should not be valid' do + group = Fabricate(:group) + new_user = Fabricate.build(:user, username: group.name.upcase) + + expect(new_user).to_not be_valid + + expect(new_user.errors.full_messages.first) + .to include(I18n.t(:'user.username.unique')) + end + end + end describe 'emails' do + it { is_expected.to validate_presence_of :primary_email } + let(:user) { Fabricate.build(:user) } describe 'when record has a valid email' do