DEV: simplify username suggester (#14531)

DEV: simplify username suggester (#14531)

This PR doesn’t change any behavior, but just removes code that wasn’t in use. This is a pretty dangerous place to change, since it gets called during user’s registration. At the same time the refactoring is very straightforward, it’s clear that this code wasn’t doing any work (it still needs to be double-checked during review though). Also, the test coverage of UserNameSuggester is good.

diff --git a/app/models/discourse_single_sign_on.rb b/app/models/discourse_single_sign_on.rb
index e268d98..04812f1 100644
--- a/app/models/discourse_single_sign_on.rb
+++ b/app/models/discourse_single_sign_on.rb
@@ -323,8 +323,8 @@ class DiscourseSingleSignOn < SingleSignOn
     if SiteSetting.auth_overrides_username? && username.present?
       if user.username.downcase == username.downcase
         user.username = username # there may be a change of case
-      elsif user.username != username
-        user.username = UserNameSuggester.suggest(username || name || email, user.username)
+      elsif user.username != UserNameSuggester.fix_username(username)
+        user.username = UserNameSuggester.suggest(username)
       end
     end
 
diff --git a/lib/auth/result.rb b/lib/auth/result.rb
index 08a1220..5f68606 100644
--- a/lib/auth/result.rb
+++ b/lib/auth/result.rb
@@ -77,8 +77,8 @@ class Auth::Result
 
   def apply_user_attributes!
     change_made = false
-    if SiteSetting.auth_overrides_username? && username.present? && username != user.username
-      user.username = UserNameSuggester.suggest(username_suggester_attributes, user.username)
+    if SiteSetting.auth_overrides_username? && username.present? && UserNameSuggester.fix_username(username) != user.username
+      user.username = UserNameSuggester.suggest(username)
       change_made = true
     end
 
diff --git a/lib/user_name_suggester.rb b/lib/user_name_suggester.rb
index 3a294c0..dcb7877 100644
--- a/lib/user_name_suggester.rb
+++ b/lib/user_name_suggester.rb
@@ -4,9 +4,9 @@ module UserNameSuggester
   GENERIC_NAMES = ['i', 'me', 'info', 'support', 'admin', 'webmaster', 'hello', 'mail', 'office', 'contact', 'team']
   LAST_RESORT_USERNAME = "user"
 
-  def self.suggest(name_or_email, allowed_username = nil)
+  def self.suggest(name_or_email)
     name = parse_name_from_email(name_or_email)
-    find_available_username_based_on(name, allowed_username)
+    find_available_username_based_on(name)
   end
 
   def self.parse_name_from_email(name_or_email)
@@ -20,22 +20,13 @@ module UserNameSuggester
     name
   end
 
-  def self.find_available_username_based_on(name, allowed_username = nil)
+  def self.find_available_username_based_on(name)
     name = fix_username(name)
     offset = nil
     i = 1
 
     attempt = name
-    normalized_attempt = User.normalize_username(attempt)
-
-    original_allowed_username = allowed_username
-    allowed_username = User.normalize_username(allowed_username) if allowed_username
-
-    until (
-      normalized_attempt == allowed_username ||
-      User.username_available?(attempt) ||
-      i > 100
-    )
+    until User.username_available?(attempt) || i > 100
 
       if offset.nil?
         normalized = User.normalize_username(name)
@@ -51,8 +42,7 @@ module UserNameSuggester
 
           params = {
             count: count + 10,
-            name: normalized,
-            allowed_normalized: allowed_username || ''
+            name: normalized
           }
 
           # increasing the search space a bit to allow for some extra noise
@@ -60,11 +50,7 @@ module UserNameSuggester
             WITH numbers AS (SELECT generate_series(1, :count) AS n)
 
             SELECT n FROM numbers
-            LEFT JOIN users ON (
-              username_lower = :name || n::varchar
-            ) AND (
-              username_lower <> :allowed_normalized
-            )
+            LEFT JOIN users ON (username_lower = :name || n::varchar)
             WHERE users.id IS NULL
             ORDER by n ASC
             LIMIT 1
@@ -82,22 +68,15 @@ module UserNameSuggester
 
       max_length = User.username_length.end - suffix.length
       attempt = "#{truncate(name, max_length)}#{suffix}"
-      normalized_attempt = User.normalize_username(attempt)
       i += 1
     end
 
-    until normalized_attempt == allowed_username || User.username_available?(attempt) || i > 200
+    until User.username_available?(attempt) || i > 200
       attempt = SecureRandom.hex[1..SiteSetting.max_username_length]
-      normalized_attempt = User.normalize_username(attempt)
       i += 1
     end
 
-    if allowed_username == normalized_attempt
-      original_allowed_username
-    else
-      attempt
-    end
-
+    attempt
   end
 
   def self.fix_username(name)
diff --git a/spec/components/user_name_suggester_spec.rb b/spec/components/user_name_suggester_spec.rb
index 3adc48a..21a44b2 100644
--- a/spec/components/user_name_suggester_spec.rb
+++ b/spec/components/user_name_suggester_spec.rb
@@ -160,14 +160,6 @@ describe UserNameSuggester do
         expect(UserNameSuggester.suggest('য়া')).to eq('য়া11')
       end
 
-      it "does not skip ove allowed names" do
-        Fabricate(:user, username: 'sam')
-        Fabricate(:user, username: 'saM1')
-        Fabricate(:user, username: 'sam2')
-
-        expect(UserNameSuggester.suggest('SaM', 'Sam1')).to eq('Sam1')
-      end
-
       it "normalizes usernames" do
         actual = 'Löwe'    # NFD, "Lo\u0308we"
         expected = 'Löwe'  # NFC, "L\u00F6we"
diff --git a/spec/models/discourse_single_sign_on_spec.rb b/spec/models/discourse_single_sign_on_spec.rb
index b9fdba6..ebc2d08 100644
--- a/spec/models/discourse_single_sign_on_spec.rb
+++ b/spec/models/discourse_single_sign_on_spec.rb
@@ -265,27 +265,6 @@ describe DiscourseSingleSignOn do
     expect(add_group4.usernames).to eq(user.username)
   end
 
-  it 'can override username properly when only the case changes' do
-    SiteSetting.auth_overrides_username = true
-
-    sso = new_discourse_sso
-    sso.username = "testuser"
-    sso.name = "test user"
-    sso.email = "test@test.com"
-    sso.external_id = "100"
-    sso.bio = "This **is** the bio"
-    sso.suppress_welcome_message = true
-
-    # create the original user
-    user = sso.lookup_or_create_user(ip_address)
-    expect(user.username).to eq "testuser"
-
-    # change the username case
-    sso.username = "TestUser"
-    user = sso.lookup_or_create_user(ip_address)
-    expect(user.username).to eq "TestUser"
-  end
-
   it 'behaves properly when auth_overrides_username is set but username is missing or blank' do
     SiteSetting.auth_overrides_username = true
 
@@ -347,6 +326,46 @@ describe DiscourseSingleSignOn do
     expect(admin.name).to eq "Louis C.K."
   end
 
+  it 'can override username properly when only the case changes' do
+    SiteSetting.auth_overrides_username = true
+
+    sso = new_discourse_sso
+    sso.username = "testuser"
+    sso.name = "test user"
+    sso.email = "test@test.com"
+    sso.external_id = "100"
+    sso.bio = "This **is** the bio"
+    sso.suppress_welcome_message = true
+
+    # create the original user
+    user = sso.lookup_or_create_user(ip_address)
+    expect(user.username).to eq "testuser"
+
+    # change the username case
+    sso.username = "TestUser"
+    user = sso.lookup_or_create_user(ip_address)
+    expect(user.username).to eq "TestUser"
+  end
+
+  it 'do not override username when a new username after fixing is the same' do
+    SiteSetting.auth_overrides_username = true
+
+    sso = new_discourse_sso
+    sso.username = "testuser"
+    sso.name = "test user"
+    sso.email = "test@test.com"
+    sso.external_id = "100"
+
+    # create the original user
+    user = sso.lookup_or_create_user(ip_address)
+    expect(user.username).to eq "testuser"
+
+    # change the username case
+    sso.username = "testuserგამარჯობა"
+    user = sso.lookup_or_create_user(ip_address)
+    expect(user.username).to eq "testuser"
+  end
+
   it "doesn't use email as a source for username suggestions by default" do
     sso = new_discourse_sso
     sso.external_id = "100"

GitHub sha: 19d95c64af9d38c7e34173f96745ae040ef0190e

This commit appears in #14531 which was approved by eviltrout and gschlager. It was merged by AndrewPrigorshnev.