Merge branch 'el-require-email-verification-first' into 'master'

Reverse the order of identity verification steps

See merge request !137513



Merged-by: Bojan Marjanovic <bmarjanovic@gitlab.com>
Approved-by: Aboobacker MK <akarakath@gitlab.com>
Approved-by: Hinam Mehra <hmehra@gitlab.com>
Approved-by: Bojan Marjanovic <bmarjanovic@gitlab.com>
Co-authored-by: Eugie Limpin <elimpin@gitlab.com>

ee/app/models/concerns/identity_verifiable.rb

@@ -63,21 +63,21 @@ def required_identity_verification_methods
     return methods if exempt_from_identity_verification?
 
     if exempt_from_phone_number_verification?
-      methods.prepend VERIFICATION_METHODS[:CREDIT_CARD] if credit_card_verification_enabled?
+      methods.append VERIFICATION_METHODS[:CREDIT_CARD] if credit_card_verification_enabled?
 
       return methods
     end
 
     case arkose_risk_band
     when Arkose::VerifyResponse::RISK_BAND_HIGH.downcase
-      methods.prepend VERIFICATION_METHODS[:PHONE_NUMBER] if phone_number_verification_enabled?
-      methods.prepend VERIFICATION_METHODS[:CREDIT_CARD] if credit_card_verification_enabled?
+      methods.append VERIFICATION_METHODS[:PHONE_NUMBER] if phone_number_verification_enabled?
+      methods.append VERIFICATION_METHODS[:CREDIT_CARD] if credit_card_verification_enabled?
     when Arkose::VerifyResponse::RISK_BAND_MEDIUM.downcase
-      methods.prepend VERIFICATION_METHODS[:PHONE_NUMBER] if phone_number_verification_enabled?
+      methods.append VERIFICATION_METHODS[:PHONE_NUMBER] if phone_number_verification_enabled?
     when Arkose::VerifyResponse::RISK_BAND_LOW.downcase
       if phone_number_verification_enabled?
         experiment(:phone_verification_for_low_risk_users, user: self) do |e|
-          e.candidate { methods.prepend VERIFICATION_METHODS[:PHONE_NUMBER] }
+          e.candidate { methods.append VERIFICATION_METHODS[:PHONE_NUMBER] }
         end
       end
     end

ee/spec/features/registrations/identity_verification_spec.rb

@@ -100,10 +100,10 @@
     it 'verifies the user' do
       expect_to_see_identity_verification_page
 
-      verify_phone_number
-
       verify_email unless skip_email_validation
 
+      verify_phone_number
+
       expect_verification_completed
 
       expect_to_see_dashboard_page
@@ -113,17 +113,16 @@
       it 'verifies the user' do
         expect_to_see_identity_verification_page
 
+        verify_email unless skip_email_validation
+
         request_phone_exemption
 
         verify_credit_card
 
-        verify_email unless skip_email_validation
-
-        # if skip_email_validation = true, email is already verified
-        # plus, verify_credit_card creates a credit_card verification record &
-        # refreshes the page. This causes an automatic redirect to the welcome page,
-        # hence skipping the verification successful badge.
-        expect_verification_completed unless skip_email_validation
+        # verify_credit_card creates a credit_card verification record &
+        # refreshes the page. This causes an automatic redirect to the welcome
+        # page, skipping the verification successful badge, and preventing us
+        # from calling expect_verification_completed
 
         expect_to_see_dashboard_page
       end
@@ -136,13 +135,11 @@
     it 'verifies the user' do
       expect_to_see_identity_verification_page
 
-      verify_credit_card
+      verify_email unless skip_email_validation
 
       verify_phone_number
 
-      verify_email unless skip_email_validation
-
-      expect_verification_completed
+      verify_credit_card
 
       expect_to_see_dashboard_page
     end
@@ -153,15 +150,14 @@
 
         expect_to_see_identity_verification_page
 
-        verify_credit_card
-
         verify_email unless skip_email_validation
 
-        # if skip_email_validation = true, email is already verified
-        # plus, verify_credit_card creates a credit_card verification record &
-        # refreshes the page. This causes an automatic redirect to the welcome page,
-        # hence skipping the verification successful badge.
-        expect_verification_completed unless skip_email_validation
+        verify_credit_card
+
+        # verify_credit_card creates a credit_card verification record &
+        # refreshes the page. This causes an automatic redirect to the welcome
+        # page, skipping the verification successful badge, and preventing us
+        # from calling expect_verification_completed
 
         expect_to_see_dashboard_page
       end

ee/spec/models/concerns/identity_verifiable_spec.rb

@@ -144,15 +144,15 @@ def add_identity_verification_exemption
     let(:user) { create(:user) }
 
     where(:risk_band, :credit_card, :phone_number, :phone_exempt, :identity_verification_exempt, :result) do
-      'High'   | true  | true  | false | false | %w[credit_card phone email]
-      'High'   | true  | true  | true  | false | %w[credit_card email]
+      'High'   | true  | true  | false | false | %w[email phone credit_card]
+      'High'   | true  | true  | true  | false | %w[email credit_card]
       'High'   | true  | true  | false | true  | %w[email]
-      'High'   | false | true  | false | false | %w[phone email]
-      'High'   | true  | false | false | false | %w[credit_card email]
+      'High'   | false | true  | false | false | %w[email phone]
+      'High'   | true  | false | false | false | %w[email credit_card]
       'High'   | false | false | false | false | %w[email]
-      'Medium' | true  | true  | false | false | %w[phone email]
-      'Medium' | false | true  | false | false | %w[phone email]
-      'Medium' | true  | true  | true  | false | %w[credit_card email]
+      'Medium' | true  | true  | false | false | %w[email phone]
+      'Medium' | false | true  | false | false | %w[email phone]
+      'Medium' | true  | true  | true  | false | %w[email credit_card]
       'Medium' | true  | true  | false | true  | %w[email]
       'Medium' | true  | false | false | false | %w[email]
       'Medium' | false | false | false | false | %w[email]
@@ -183,8 +183,8 @@ def add_identity_verification_exemption
       let_it_be(:another_user) { create(:user) }
 
       where(:risk_band, :credit_card, :phone_number, :result) do
-        'High'   | true  | false | %w[credit_card email]
-        'Medium' | false | true  | %w[phone email]
+        'High'   | true  | false | %w[email credit_card]
+        'Medium' | false | true  | %w[email phone]
       end
 
       with_them do
@@ -238,7 +238,7 @@ def add_identity_verification_exemption
           stub_experiments(phone_verification_for_low_risk_users: :candidate)
         end
 
-        it { is_expected.to eq(%w[phone email]) }
+        it { is_expected.to eq(%w[email phone]) }
 
         it 'tracks candidate group assignment for the user' do
           expect(experiment_instance).to track(:assignment).on_next_instance.with_context(user: user).for(:candidate)

ee/spec/requests/users/identity_verification_controller_spec.rb

@@ -793,7 +793,7 @@
         do_request
 
         expect(json_response).to eq({
-          'verification_methods' => %w[credit_card email],
+          'verification_methods' => %w[email credit_card],
           'verification_state' => { "credit_card" => false, "email" => false }
         })
       end