DEV: make sure we handle staged users correctly in DiscourseConnect (#15320)

Some time ago, we made this fix to external authentication –  https://github.com/discourse/discourse/pull/13706. We didn't address Discourse Connect (https://meta.discourse.org/t/discourseconnect-official-single-sign-on-for-discourse-sso/13045) at that moment, so I wanted to fix it for Discourse Connect as well.

Turned out though that Discourse Connect doesn't contain this problem and already handles staged users correctly. This PR adds tests that confirm it. Also, I've extracted two functions in Discourse Connect implementation along the way and decided to merge this refactoring too (the refactoring is supported with tests).

app/models/discourse_single_sign_on.rb

@@ -239,20 +239,10 @@ class DiscourseSingleSignOn < SingleSignOn
     DistributedMutex.synchronize("discourse_single_sign_on_#{email}") do
       user = User.find_by_email(email) if !require_activation
       if !user
-        try_name = name.presence
-        try_username = username.presence
-
-        name_suggester_input = try_username
-        username_suggester_input = try_username || try_name
-        if SiteSetting.use_email_for_username_and_name_suggestions
-          name_suggester_input = name_suggester_input || email
-          username_suggester_input = username_suggester_input || email
-        end
-
         user_params = {
           primary_email: UserEmail.new(email: email, primary: true),
-          name: try_name || User.suggest_name(name_suggester_input),
-          username: UserNameSuggester.suggest(username_suggester_input),
+          name: resolve_name,
+          username: resolve_username,
           ip_address: ip_address
         }
 
@@ -381,4 +371,22 @@ class DiscourseSingleSignOn < SingleSignOn
     sso_record.external_profile_background_url = profile_background_url
     sso_record.external_card_background_url = card_background_url
   end
+
+  def resolve_username
+    username_suggester_input = username.presence || name.presence
+    if SiteSetting.use_email_for_username_and_name_suggestions
+      username_suggester_input = username_suggester_input || email
+    end
+
+    UserNameSuggester.suggest(username_suggester_input)
+  end
+
+  def resolve_name
+    name_suggester_input = username.presence
+    if SiteSetting.use_email_for_username_and_name_suggestions
+      name_suggester_input = name_suggester_input || email
+    end
+
+    name.presence || User.suggest_name(name_suggester_input)
+  end
 end

spec/models/discourse_single_sign_on_spec.rb

@@ -390,6 +390,54 @@ describe DiscourseSingleSignOn do
     expect(user.username).to eq "bill3"
   end
 
+  it "uses name if it's present in payload" do
+    sso = new_discourse_sso
+    sso.external_id = "100"
+
+    name = "John"
+    sso.name = name
+    sso.email = "mail@mail.com"
+    user = sso.lookup_or_create_user(ip_address)
+
+    expect(user.name).to eq name
+  end
+
+  it "uses username for name suggestions if name isn't present in payload" do
+    sso = new_discourse_sso
+    sso.external_id = "100"
+
+    sso.name = ""
+    sso.username = "user_john"
+    sso.email = "mail@mail.com"
+    user = sso.lookup_or_create_user(ip_address)
+
+    expect(user.name).to eq "User John"
+  end
+
+  it "uses username for username suggestions if it's present in payload" do
+    sso = new_discourse_sso
+    sso.external_id = "100"
+
+    username = "user_john"
+    sso.username = username
+    sso.email = "mail@mail.com"
+    user = sso.lookup_or_create_user(ip_address)
+
+    expect(user.username).to eq username
+  end
+
+  it "uses name for username suggestions if username isn't present in payload" do
+    sso = new_discourse_sso
+    sso.external_id = "100"
+
+    sso.username = ""
+    sso.name = "John Smith"
+    sso.email = "mail@mail.com"
+    user = sso.lookup_or_create_user(ip_address)
+
+    expect(user.username).to eq "John_Smith"
+  end
+
   it "doesn't use email as a source for username suggestions by default" do
     sso = new_discourse_sso
     sso.external_id = "100"
@@ -1027,4 +1075,38 @@ describe DiscourseSingleSignOn do
     end
   end
 
+  context "when user is staged" do
+    it "uses username of the staged user if username is not present in payload" do
+      staged_username = "staged_user"
+      email = "staged@user.com"
+      Fabricate(:user, staged: true, username: staged_username, email: email)
+
+      sso = new_discourse_sso
+      sso.email = email
+      sso.external_id = "B"
+
+      user = sso.lookup_or_create_user(ip_address)
+      user.reload
+
+      expect(user.username).to eq(staged_username)
+    end
+
+    it "uses username of the staged user if username in payload is the same" do
+      # it's important to check this, because we had regressions
+      # where usernames were changed to the same username with "1" added at the end
+      staged_username = "staged_user"
+      email = "staged@user.com"
+      Fabricate(:user, staged: true, username: staged_username, email: email)
+
+      sso = new_discourse_sso
+      sso.username = staged_username
+      sso.email = email
+      sso.external_id = "B"
+
+      user = sso.lookup_or_create_user(ip_address)
+      user.reload
+
+      expect(user.username).to eq(staged_username)
+    end
+  end
 end

spec/requests/omniauth_callbacks_controller_spec.rb

@@ -916,6 +916,9 @@ RSpec.describe Users::OmniauthCallbacksController do
       end
 
       it "should use username of the staged user if username in payload is the same" do
+        # it's important to check this, because we had regressions
+        # when usernames were changed to the same username with "1" added at the end
+
         mock_auth(staged_user.email, staged_user.username)
 
         get "/auth/google_oauth2/callback.json"