Add primary email to emails even when skip_reconfirmation! called

Previously, in cases when skip_reconfirmation! was called before
updating the primary email, the email (which is to be considered
confirmed) would not be added to the emails table, because
`User#confirm` was not called.

Now instead of overriding `User#confirm` we use an `after_save` callback
that checks if the primary email changed and if it is confirmed, and if
so adds it to the emails. This works also in case skip_reconfirmation is
called, and is also cleaner than overriding a Devise method.

app/models/user.rb

@@ -277,7 +277,19 @@ class User < ApplicationRecord
   after_update :username_changed_hook, if: :saved_change_to_username?
   after_destroy :post_destroy_hook
   after_destroy :remove_key_cache
-  after_create :add_primary_email_to_emails!, if: :confirmed?
+  after_save if: -> { saved_change_to_email? && confirmed? } do
+    email_to_confirm = self.emails.find_by(email: self.email)
+
+    if email_to_confirm.present?
+      if skip_confirmation_period_expiry_check
+        email_to_confirm.force_confirm
+      else
+        email_to_confirm.confirm
+      end
+    else
+      add_primary_email_to_emails!
+    end
+  end
   after_commit(on: :update) do
     if previous_changes.key?('email')
       # Add the old primary email to Emails if not added already - this should be removed
@@ -2013,29 +2025,6 @@ class User < ApplicationRecord
     ci_job_token_scope.present?
   end
 
-  # override from Devise::Models::Confirmable
-  #
-  # Add the primary email to user.emails (or confirm it if it was already
-  # present) when the primary email is confirmed.
-  def confirm(args = {})
-    saved = super(args)
-    return false unless saved
-
-    email_to_confirm = self.emails.find_by(email: self.email)
-
-    if email_to_confirm.present?
-      if skip_confirmation_period_expiry_check
-        email_to_confirm.force_confirm(args)
-      else
-        email_to_confirm.confirm(args)
-      end
-    else
-      add_primary_email_to_emails!
-    end
-
-    saved
-  end
-
   def user_project
     strong_memoize(:user_project) do
       personal_projects.find_by(path: username, visibility_level: Gitlab::VisibilityLevel::PUBLIC)

ee/spec/lib/ee/gitlab/usage_data_non_sql_metrics_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe Gitlab::UsageDataNonSqlMetrics do
         described_class.data
       end
 
-      expect(recorder.count).to eq(65)
+      expect(recorder.count).to eq(67)
     end
   end
 end

spec/models/user_spec.rb

@@ -1172,8 +1172,8 @@ RSpec.describe User do
         @user.update!(email: 'new_primary@example.com')
         @user.reload
 
-        expect(@user.emails.count).to eq 2
-        expect(@user.emails.pluck(:email)).to match_array([@secondary.email, 'primary@example.com'])
+        expect(@user.emails.count).to eq 3
+        expect(@user.emails.pluck(:email)).to match_array([@secondary.email, 'primary@example.com', 'new_primary@example.com'])
       end
 
       context 'when the first email was unconfirmed and the second email gets confirmed' do
@@ -1594,6 +1594,66 @@ RSpec.describe User do
     end
   end
 
+  describe 'saving primary email to the emails table' do
+    context 'when calling skip_reconfirmation! while updating the primary email' do
+      let(:user) { create(:user, email: 'primary@example.com') }
+
+      it 'adds the new email to emails' do
+        user.skip_reconfirmation!
+        user.update!(email: 'new_primary@example.com')
+
+        expect(user.email).to eq('new_primary@example.com')
+        expect(user.unconfirmed_email).to be_nil
+        expect(user).to be_confirmed
+        expect(user.emails.pluck(:email)).to include('new_primary@example.com')
+        expect(user.emails.find_by(email: 'new_primary@example.com')).to be_confirmed
+      end
+    end
+
+    context 'when the email is changed but not confirmed' do
+      let(:user) { create(:user, email: 'primary@example.com') }
+
+      it 'does not add the new email to emails yet' do
+        user.update!(email: 'new_primary@example.com')
+
+        expect(user.unconfirmed_email).to eq('new_primary@example.com')
+        expect(user.email).to eq('primary@example.com')
+        expect(user).to be_confirmed
+        expect(user.emails.pluck(:email)).not_to include('new_primary@example.com')
+      end
+    end
+
+    context 'when the user is created as not confirmed' do
+      let(:user) { create(:user, :unconfirmed, email: 'primary@example.com') }
+
+      it 'does not add the email to emails yet' do
+        expect(user).not_to be_confirmed
+        expect(user.emails.pluck(:email)).not_to include('primary@example.com')
+      end
+    end
+
+    context 'when the user is created as confirmed' do
+      let(:user) { create(:user, email: 'primary@example.com', confirmed_at: DateTime.now.utc) }
+
+      it 'adds the email to emails' do
+        expect(user).to be_confirmed
+        expect(user.emails.pluck(:email)).to include('primary@example.com')
+      end
+    end
+
+    context 'when skip_confirmation! is called' do
+      let(:user) { build(:user, :unconfirmed, email: 'primary@example.com') }
+
+      it 'adds the email to emails' do
+        user.skip_confirmation!
+        user.save!
+
+        expect(user).to be_confirmed
+        expect(user.emails.pluck(:email)).to include('primary@example.com')
+      end
+    end
+  end
+
   describe '#force_confirm' do
     let(:expired_confirmation_sent_at) { Date.today - described_class.confirm_within - 7.days }
     let(:extant_confirmation_sent_at) { Date.today }