Merge branch...

Merge branch '297366-the-banner-prompting-users-to-check-their-account-recovery-settings-causes-confusion-2' into 'master'

Update copy in recovery settings alert

See merge request gitlab-org/gitlab!68001

app/controllers/profiles/two_factor_auths_controller.rb

@@ -57,6 +57,8 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
         @codes = user.generate_otp_backup_codes!
       end
 
+      helpers.dismiss_account_recovery_regular_check
+
       render 'create'
     else
       @error = _('Invalid pin code')
@@ -105,6 +107,8 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
   def codes
     Users::UpdateService.new(current_user, user: current_user).execute! do |user|
       @codes = user.generate_otp_backup_codes!
+
+      helpers.dismiss_account_recovery_regular_check
     end
   end
 

app/helpers/user_callouts_helper.rb

@@ -53,6 +53,9 @@ module UserCalloutsHelper
     !user_dismissed?(REGISTRATION_ENABLED_CALLOUT)
   end
 
+  def dismiss_account_recovery_regular_check
+  end
+
   private
 
   def user_dismissed?(feature_name, ignore_dismissal_earlier_than = nil)

app/views/shared/_check_recovery_settings.html.haml

 = render 'shared/global_alert',
   variant: :warning,
   alert_class: 'js-recovery-settings-callout',
-  alert_data: { feature_id: 'account_recovery_regular_check', dismiss_endpoint: user_callouts_path, defer_links: 'true' } do
+  alert_data: { feature_id: 'account_recovery_regular_check', dismiss_endpoint: user_callouts_path, defer_links: 'true' },
+  close_button_data: { testid: 'close-account-recovery-regular-check-callout' } do
   .gl-alert-body
-    = s_('Profiles|We recommend you ensure two-factor authentication is enabled and the settings are up to date.')
-    = link_to _('Learn more.'), help_page_path('user/profile/account/two_factor_authentication'), target: '_blank', rel: 'noopener noreferrer'
+    = s_('Profiles|Ensure you have two-factor authentication recovery codes stored in a safe place.')
+    = link_to _('Learn more.'), help_page_path('user/profile/account/two_factor_authentication', anchor: 'recovery-codes'), target: '_blank', rel: 'noopener noreferrer'
   .gl-alert-actions
     = link_to profile_two_factor_auth_path, class: 'deferred-link btn gl-alert-action btn-confirm btn-md gl-button' do
       = s_('Profiles|Manage two-factor authentication')

ee/app/helpers/ee/user_callouts_helper.rb

@@ -58,7 +58,7 @@ module EE
     def render_account_recovery_regular_check
       return unless current_user &&
           ::Gitlab.com? &&
-          3.months.ago > current_user.created_at &&
+          current_user.two_factor_otp_enabled? &&
           !user_dismissed?(ACCOUNT_RECOVERY_REGULAR_CHECK, 3.months.ago)
 
       render 'shared/check_recovery_settings'
@@ -95,6 +95,13 @@ module EE
       (namespace.group? && namespace.has_owner?(current_user.id)) || !namespace.group?
     end
 
+    override :dismiss_account_recovery_regular_check
+    def dismiss_account_recovery_regular_check
+      ::Users::DismissUserCalloutService.new(
+        container: nil, current_user: current_user, params: { feature_name: ACCOUNT_RECOVERY_REGULAR_CHECK }
+      ).execute
+    end
+
     private
 
     def eoa_bronze_plan_end_date

ee/spec/features/account_recovery_regular_check_spec.rb

@@ -4,54 +4,110 @@ require 'spec_helper'
 
 RSpec.describe 'Account recovery regular check callout' do
   context 'when signed in' do
-    let(:user) { create(:user, created_at: 4.months.ago ) }
-    let(:message) { "We recommend you ensure two-factor authentication is enabled and the settings are up to date." }
+    let(:user_two_factor_disabled) { create(:user ) }
+    let(:user_two_factor_enabled) { create(:user, :two_factor) }
+    let(:message) { 'Ensure you have two-factor authentication recovery codes stored in a safe place.' }
     let(:action_button) { 'Manage two-factor authentication' }
 
     before do
       allow(Gitlab).to receive(:com?) { true }
-      sign_in(user)
     end
 
-    it 'shows callout if not dismissed' do
-      visit root_dashboard_path
+    context 'when user has two-factor authentication disabled' do
+      before do
+        sign_in(user_two_factor_disabled)
+      end
 
-      expect(page).to have_content(message)
-      expect(page).to have_link(action_button, href: profile_two_factor_auth_path)
-    end
+      it 'does not show the callout' do
+        visit root_dashboard_path
+
+        expect(page).not_to have_content(message)
+      end
+
+      context 'when user sets up two-factor authentication' do
+        it 'does not show the callout', :js do
+          visit profile_two_factor_auth_path
 
-    it 'hides callout when user clicks action button', :js do
-      visit root_dashboard_path
+          fill_in 'pin_code', with: user_two_factor_disabled.reload.current_otp
 
-      expect(page).to have_content(message)
+          click_button 'Register with two-factor app'
 
-      click_link action_button
-      wait_for_requests
+          expect(page).not_to have_content(message)
 
-      expect(page).not_to have_content(message)
+          click_button 'Copy codes'
+          click_link 'Proceed'
+
+          expect(page).not_to have_content(message)
+        end
+      end
     end
 
-    it 'hides callout when user clicks close', :js do
-      visit root_dashboard_path
+    context 'when user has two-factor authentication enabled' do
+      before do
+        sign_in(user_two_factor_enabled)
+      end
 
-      expect(page).to have_content(message)
+      it 'shows callout if not dismissed' do
+        visit root_dashboard_path
 
-      find('.js-recovery-settings-callout .js-close').click
-      wait_for_requests
+        expect(page).to have_content(message)
+        expect(page).to have_link(action_button, href: profile_two_factor_auth_path)
+      end
 
-      expect(page).not_to have_content(message)
-    end
+      it 'hides callout when user clicks action button', :js do
+        visit root_dashboard_path
+
+        expect(page).to have_content(message)
+
+        click_link action_button
+        wait_for_requests
+
+        expect(page).not_to have_content(message)
+      end
+
+      it 'hides callout when user clicks close', :js do
+        visit root_dashboard_path
+
+        expect(page).to have_content(message)
+
+        close_callout
 
-    it 'shows callout on next session if user did not dismissed it' do
-      visit root_dashboard_path
+        expect(page).not_to have_content(message)
+      end
 
-      expect(page).to have_content(message)
+      it 'shows callout on next session if user did not dismissed it' do
+        visit root_dashboard_path
 
-      gitlab_sign_out
-      gitlab_sign_in(user)
-      visit root_dashboard_path
+        expect(page).to have_content(message)
 
-      expect(page).to have_content(message)
+        start_new_session(user_two_factor_enabled)
+        visit root_dashboard_path
+
+        expect(page).to have_content(message)
+      end
+
+      it 'hides callout on next session if user dismissed it', :js do
+        visit root_dashboard_path
+
+        expect(page).to have_content(message)
+
+        close_callout
+
+        start_new_session(user_two_factor_enabled)
+        visit root_dashboard_path
+
+        expect(page).not_to have_content(message)
+      end
     end
   end
+
+  def close_callout
+    find('[data-testid="close-account-recovery-regular-check-callout"]').click
+    wait_for_requests
+  end
+
+  def start_new_session(user)
+    gitlab_sign_out
+    gitlab_sign_in(user, two_factor_auth: true)
+  end
 end

ee/spec/helpers/ee/user_callouts_helper_spec.rb

@@ -181,19 +181,19 @@ RSpec.describe EE::UserCalloutsHelper do
   end
 
   describe '#render_account_recovery_regular_check' do
-    let(:new_user) { create(:user) }
-    let(:old_user) { create(:user, created_at: 4.months.ago )}
+    let(:user_two_factor_disabled) { create(:user) }
+    let(:user_two_factor_enabled) { create(:user, :two_factor) }
     let(:anonymous) { nil }
 
     where(:kind_of_user, :is_gitlab_com?, :dismissed_callout?, :should_render?) do
-      :anonymous | false | false | false
-      :anonymous | true  | false | false
-      :new_user  | false | false | false
-      :new_user  | true  | false | false
-      :old_user  | false | false | false
-      :old_user  | true  | false | true
-      :old_user  | false | true  | false
-      :old_user  | true  | true  | false
+      :anonymous                | false | false | false
+      :anonymous                | true  | false | false
+      :user_two_factor_disabled | false | false | false
+      :user_two_factor_disabled | true  | false | false
+      :user_two_factor_disabled | true  | true  | false
+      :user_two_factor_enabled  | false | false | false
+      :user_two_factor_enabled  | true  | false | true
+      :user_two_factor_enabled  | true  | true  | false
     end
 
     with_them do
@@ -391,4 +391,25 @@ RSpec.describe EE::UserCalloutsHelper do
       expect(helper.send(:eoa_bronze_plan_end_date).is_a?(Date)).to eq(true)
     end
   end
+
+  describe '#dismiss_account_recovery_regular_check' do
+    let_it_be(:user) { create(:user) }
+
+    before do
+      allow(helper).to receive(:current_user).and_return(user)
+    end
+
+    it 'dismisses `ACCOUNT_RECOVERY_REGULAR_CHECK` callout' do
+      expect(::Users::DismissUserCalloutService)
+        .to receive(:new)
+        .with(
+          container: nil,
+          current_user: user,
+          params: { feature_name: UserCalloutsHelper::ACCOUNT_RECOVERY_REGULAR_CHECK }
+        )
+        .and_call_original
+
+      helper.dismiss_account_recovery_regular_check
+    end
+  end
 end

locale/gitlab.pot

@@ -25557,6 +25557,9 @@ msgstr ""
 msgid "Profiles|Edit Profile"
 msgstr ""
 
+msgid "Profiles|Ensure you have two-factor authentication recovery codes stored in a safe place."
+msgstr ""
+
 msgid "Profiles|Enter how your name is pronounced to help people address you correctly"
 msgstr ""
 
@@ -25749,9 +25752,6 @@ msgstr ""
 msgid "Profiles|Using emojis in names seems fun, but please try to set a status message instead"
 msgstr ""
 
-msgid "Profiles|We recommend you ensure two-factor authentication is enabled and the settings are up to date."
-msgstr ""
-
 msgid "Profiles|What's your status?"
 msgstr ""
 

spec/controllers/profiles/two_factor_auths_controller_spec.rb

@@ -70,6 +70,12 @@ RSpec.describe Profiles::TwoFactorAuthsController do
         go
       end
 
+      it 'dismisses the `ACCOUNT_RECOVERY_REGULAR_CHECK` callout' do
+        expect(controller.helpers).to receive(:dismiss_account_recovery_regular_check)
+
+        go
+      end
+
       it 'renders create' do
         go
         expect(response).to render_template(:create)
@@ -117,6 +123,12 @@ RSpec.describe Profiles::TwoFactorAuthsController do
       user.reload
       expect(user.otp_backup_codes).not_to be_empty
     end
+
+    it 'dismisses the `ACCOUNT_RECOVERY_REGULAR_CHECK` callout' do
+      expect(controller.helpers).to receive(:dismiss_account_recovery_regular_check)
+
+      post :codes
+    end
   end
 
   describe 'DELETE destroy' do

spec/support/helpers/login_helpers.rb

@@ -88,9 +88,10 @@ module LoginHelpers
 
   # Private: Login as the specified user
   #
-  # user     - User instance to login with
+  # user - User instance to login with
   # remember - Whether or not to check "Remember me" (default: false)
-  def gitlab_sign_in_with(user, remember: false)
+  # two_factor_auth - If two-factor authentication is enabled (default: false)
+  def gitlab_sign_in_with(user, remember: false, two_factor_auth: false)
     visit new_user_session_path
 
     fill_in "user_login", with: user.email
@@ -98,6 +99,11 @@ module LoginHelpers
     check 'user_remember_me' if remember
 
     click_button "Sign in"
+
+    if two_factor_auth
+      fill_in "user_otp_attempt", with: user.reload.current_otp
+      click_button "Verify code"
+    end
   end
 
   def login_via(provider, user, uid, remember_me: false, additional_info: {})