Merge branch '232534-skip-running-unconfirm-migration-when-email-confirmation-is-off' into 'master'

Handle special cases when mass unconfirming users

See merge request gitlab-org/gitlab!38024

changelogs/unreleased/232534-skip-running-unconfirm-migration-when-email-confirmation-is-off.yml

+---
+title: Skip mass unconfirming users when send_user_confirmation_email setting is off
+merge_request: 38024
+author:
+type: changed

db/post_migrate/20200615111857_unconfirm_wrongfully_verified_emails.rb

@@ -12,6 +12,10 @@ class UnconfirmWrongfullyVerifiedEmails < ActiveRecord::Migration[6.0]
   MIGRATION = 'WrongfullyConfirmedEmailUnconfirmer'
   EMAIL_INDEX_NAME = 'tmp_index_for_email_unconfirmation_migration'
 
+  class ApplicationSetting < ActiveRecord::Base
+    self.table_name = 'application_settings'
+  end
+
   class Email < ActiveRecord::Base
     include EachBatch
   end
@@ -19,6 +23,11 @@ class UnconfirmWrongfullyVerifiedEmails < ActiveRecord::Migration[6.0]
   def up
     add_concurrent_index :emails, :id, where: 'confirmed_at IS NOT NULL', name: EMAIL_INDEX_NAME
 
+    ApplicationSetting.reset_column_information
+
+    setting_record = ApplicationSetting.last
+    return unless setting_record&.send_user_confirmation_email
+
     queue_background_migration_jobs_by_range_at_intervals(Email,
                                                           MIGRATION,
                                                           INTERVAL,

lib/gitlab/background_migration/wrongfully_confirmed_email_unconfirmer.rb

@@ -30,6 +30,7 @@ module Gitlab
             .where('emails.confirmed_at IS NOT NULL')
             .where('emails.confirmed_at = users.confirmed_at')
             .where('emails.email <> users.email')
+            .where('NOT EXISTS (SELECT 1 FROM user_synced_attributes_metadata WHERE user_id=users.id AND email_synced IS true)')
         end
       end
 

spec/lib/gitlab/background_migration/wrongfully_confirmed_email_unconfirmer_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 RSpec.describe Gitlab::BackgroundMigration::WrongfullyConfirmedEmailUnconfirmer, schema: 20200615111857 do
   let(:users) { table(:users) }
   let(:emails) { table(:emails) }
+  let(:user_synced_attributes_metadata) { table(:user_synced_attributes_metadata) }
   let(:confirmed_at_2_days_ago) { 2.days.ago }
   let(:confirmed_at_3_days_ago) { 3.days.ago }
   let(:one_year_ago) { 1.year.ago }
@@ -14,6 +15,8 @@ RSpec.describe Gitlab::BackgroundMigration::WrongfullyConfirmedEmailUnconfirmer,
   let!(:user_does_not_need_migration) { users.create!(name: 'user3', email: 'test3@test.com', state: 'active', projects_limit: 1) }
   let!(:inactive_user) { users.create!(name: 'user4', email: 'test4@test.com', state: 'blocked', projects_limit: 1, confirmed_at: confirmed_at_3_days_ago, confirmation_sent_at: one_year_ago) }
   let!(:alert_bot_user) { users.create!(name: 'user5', email: 'test5@test.com', state: 'active', user_type: 2, projects_limit: 1, confirmed_at: confirmed_at_3_days_ago, confirmation_sent_at: one_year_ago) }
+  let!(:user_has_synced_email) { users.create!(name: 'user6', email: 'test6@test.com', state: 'active', projects_limit: 1, confirmed_at: confirmed_at_2_days_ago, confirmation_sent_at: one_year_ago) }
+  let!(:synced_attributes_metadata_for_user) { user_synced_attributes_metadata.create!(user_id: user_has_synced_email.id, email_synced: true) }
 
   let!(:bad_email_1) { emails.create!(user_id: user_needs_migration_1.id, email: 'other1@test.com', confirmed_at: confirmed_at_2_days_ago, confirmation_sent_at: one_year_ago) }
   let!(:bad_email_2) { emails.create!(user_id: user_needs_migration_2.id, email: 'other2@test.com', confirmed_at: confirmed_at_3_days_ago, confirmation_sent_at: one_year_ago) }
@@ -24,8 +27,10 @@ RSpec.describe Gitlab::BackgroundMigration::WrongfullyConfirmedEmailUnconfirmer,
   let!(:good_email_2) { emails.create!(user_id: user_needs_migration_2.id, email: 'other4@test.com', confirmed_at: nil) }
   let!(:good_email_3) { emails.create!(user_id: user_does_not_need_migration.id, email: 'other5@test.com', confirmed_at: confirmed_at_2_days_ago, confirmation_sent_at: one_year_ago) }
 
+  let!(:second_email_for_user_with_synced_email) { emails.create!(user_id: user_has_synced_email.id, email: 'other6@test.com', confirmed_at: confirmed_at_2_days_ago, confirmation_sent_at: one_year_ago) }
+
   subject do
-    email_ids = [bad_email_1, bad_email_2, good_email_1, good_email_2, good_email_3].map(&:id)
+    email_ids = [bad_email_1, bad_email_2, good_email_1, good_email_2, good_email_3, second_email_for_user_with_synced_email].map(&:id)
 
     described_class.new.perform(email_ids.min, email_ids.max)
   end
@@ -61,10 +66,12 @@ RSpec.describe Gitlab::BackgroundMigration::WrongfullyConfirmedEmailUnconfirmer,
     expect(user_does_not_need_migration.reload.confirmed_at).to be_nil
     expect(inactive_user.reload.confirmed_at).to be_within(1.second).of(confirmed_at_3_days_ago)
     expect(alert_bot_user.reload.confirmed_at).to be_within(1.second).of(confirmed_at_3_days_ago)
+    expect(user_has_synced_email.reload.confirmed_at).to be_within(1.second).of(confirmed_at_2_days_ago)
 
     expect(user_does_not_need_migration.reload.confirmation_sent_at).to be_nil
     expect(inactive_user.reload.confirmation_sent_at).to be_within(1.second).of(one_year_ago)
     expect(alert_bot_user.reload.confirmation_sent_at).to be_within(1.second).of(one_year_ago)
+    expect(user_has_synced_email.confirmation_sent_at).to be_within(1.second).of(one_year_ago)
   end
 
   it 'updates confirmation_sent_at column' do

spec/migrations/unconfirm_wrongfully_verified_emails_spec.rb

@@ -9,13 +9,47 @@ RSpec.describe UnconfirmWrongfullyVerifiedEmails do
     table(:emails).create!(email: 'test2@test.com', user_id: user.id)
   end
 
-  it 'enqueues WrongullyConfirmedEmailUnconfirmer job' do
-    Sidekiq::Testing.fake! do
-      migrate!
+  context 'when email confirmation is enabled' do
+    before do
+      table(:application_settings).create!(send_user_confirmation_email: true)
+    end
+
+    it 'enqueues WrongullyConfirmedEmailUnconfirmer job' do
+      Sidekiq::Testing.fake! do
+        migrate!
+
+        jobs = BackgroundMigrationWorker.jobs
+        expect(jobs.size).to eq(1)
+        expect(jobs.first["args"].first).to eq(Gitlab::BackgroundMigration::WrongfullyConfirmedEmailUnconfirmer.name.demodulize)
+      end
+    end
+  end
+
+  context 'when email confirmation is disabled' do
+    before do
+      table(:application_settings).create!(send_user_confirmation_email: false)
+    end
+
+    it 'does not enqueue WrongullyConfirmedEmailUnconfirmer job' do
+      Sidekiq::Testing.fake! do
+        migrate!
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(0)
+      end
+    end
+  end
+
+  context 'when email application setting record does not exist' do
+    before do
+      table(:application_settings).delete_all
+    end
+
+    it 'does not enqueue WrongullyConfirmedEmailUnconfirmer job' do
+      Sidekiq::Testing.fake! do
+        migrate!
 
-      jobs = BackgroundMigrationWorker.jobs
-      expect(jobs.size).to eq(1)
-      expect(jobs.first["args"].first).to eq(Gitlab::BackgroundMigration::WrongfullyConfirmedEmailUnconfirmer.name.demodulize)
+        expect(BackgroundMigrationWorker.jobs.size).to eq(0)
+      end
     end
   end
 end