FIX: Don't try to rename group when username is taken

FIX: Don’t try to rename group when username is taken

FIX: Always rename groups with the default locale instead of using the user’s locale

diff --git a/app/models/group.rb b/app/models/group.rb
index 9ed0298..0f29590 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -279,10 +279,10 @@ class Group < ActiveRecord::Base
     end
 
     # don't allow shoddy localization to break this
-    localized_name = I18n.t("groups.default_names.#{name}").downcase
+    localized_name = I18n.t("groups.default_names.#{name}", locale: SiteSetting.default_locale).downcase
     validator = UsernameValidator.new(localized_name)
 
-    if !Group.where("LOWER(name) = ?", localized_name).exists? && validator.valid_format?
+    if validator.valid_format? && !User.username_exists?(localized_name)
       group.name = localized_name
     end
 
@@ -626,15 +626,8 @@ class Group < ActiveRecord::Base
     UsernameValidator.perform_validation(self, 'name') || begin
       name_lower = self.name.downcase
 
-      if self.will_save_change_to_name? && self.name_was&.downcase != name_lower
-
-        existing = DB.exec(
-          User::USERNAME_EXISTS_SQL, username: name_lower
-        ) > 0
-
-        if existing
-          errors.add(:name, I18n.t("activerecord.errors.messages.taken"))
-        end
+      if self.will_save_change_to_name? && self.name_was&.downcase != name_lower && User.username_exists?(name_lower)
+        errors.add(:name, I18n.t("activerecord.errors.messages.taken"))
       end
     end
   end
diff --git a/app/models/user.rb b/app/models/user.rb
index 0de21d1..6c9e3be 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -220,7 +220,7 @@ class User < ActiveRecord::Base
   def self.username_available?(username, email = nil, allow_reserved_username: false)
     lower = username.downcase
     return false if !allow_reserved_username && reserved_username?(lower)
-    return true  if DB.exec(User::USERNAME_EXISTS_SQL, username: lower) == 0
+    return true  if !username_exists?(lower)
 
     # staged users can use the same username since they will take over the account
     email.present? && User.joins(:user_emails).exists?(staged: true, username_lower: lower, user_emails: { primary: true, email: email })
@@ -1271,6 +1271,10 @@ class User < ActiveRecord::Base
   WHERE lower(groups.name) = :username)
   SQL
 
+  def self.username_exists?(username_lower)
+    DB.exec(User::USERNAME_EXISTS_SQL, username: username_lower) > 0
+  end
+
   def username_validator
     username_format_validator || begin
       lower = username.downcase
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 37ef673..8419e89 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -236,7 +236,6 @@ describe Group do
 
     it "does not reset the localized name" do
       begin
-        default_locale = SiteSetting.default_locale
         I18n.locale = SiteSetting.default_locale = 'fi'
 
         group = Group.find(Group::AUTO_GROUPS[:everyone])
@@ -251,41 +250,43 @@ describe Group do
         Group.refresh_automatic_group!(:everyone)
 
         expect(group.reload.name).to eq(I18n.t("groups.default_names.everyone"))
-      ensure
-        I18n.locale = SiteSetting.default_locale = default_locale
       end
     end
 
     it "uses the localized name if name has not been taken" do
       begin
-        default_locale = SiteSetting.default_locale
         I18n.locale = SiteSetting.default_locale = 'de'
 
         group = Group.refresh_automatic_group!(:staff)
 
         expect(group.name).to_not eq('staff')
         expect(group.name).to eq(I18n.t('groups.default_names.staff'))
-      ensure
-        I18n.locale = SiteSetting.default_locale = default_locale
       end
     end
 
     it "does not use the localized name if name has already been taken" do
       begin
-        default_locale = SiteSetting.default_locale
         I18n.locale = SiteSetting.default_locale = 'de'
 
-        _another_group = Fabricate(:group,
-          name: I18n.t('groups.default_names.staff').upcase
-        )
-
+        Fabricate(:group, name: I18n.t('groups.default_names.staff').upcase)
         group = Group.refresh_automatic_group!(:staff)
-
         expect(group.name).to eq('staff')
-      ensure
-        I18n.locale = SiteSetting.default_locale = default_locale
+
+        Fabricate(:user_single_email, username: I18n.t('groups.default_names.moderators').upcase)
+        group = Group.refresh_automatic_group!(:moderators)
+        expect(group.name).to eq('moderators')
       end
     end
+
+    it "always uses the default locale" do
+      SiteSetting.default_locale = "de"
+      I18n.locale = "en"
+
+      group = Group.refresh_automatic_group!(:staff)
+
+      expect(group.name).to_not eq('staff')
+      expect(group.name).to eq(I18n.t('groups.default_names.staff', locale: "de"))
+    end
   end
 
   it "Correctly handles removal of primary group" do

GitHub sha: 5d75bd48

1 Like

This fixes the bug that prevented granting admin rights when there is an existing user with the same name as the localized group name.

I’ll take another look at this when I work on updating seeded content when the locale changes, because it’s kinda crazy that we try to rename groups every 12 hours (EnsureDbConsistency job) and during actions like confirming an admin.

/var/www/discourse/app/models/group.rb:297:in `refresh_automatic_group!'
/var/www/discourse/app/models/group.rb:375:in `block in refresh_automatic_groups!'
/var/www/discourse/app/models/group.rb:375:in `each'
/var/www/discourse/app/models/group.rb:375:in `refresh_automatic_groups!'
/var/www/discourse/app/models/concerns/roleable.rb:38:in `block in save_and_refresh_staff_groups!'
/var/www/discourse/app/models/concerns/roleable.rb:36:in `save_and_refresh_staff_groups!'
/var/www/discourse/app/models/concerns/roleable.rb:44:in `set_permission'
/var/www/discourse/app/models/concerns/roleable.rb:28:in `grant_admin!'
/var/www/discourse/lib/admin_confirmation.rb:36:in `email_confirmed!'
/var/www/discourse/app/controllers/users_controller.rb:1024:in `confirm_admin'
1 Like