Merge branch 'xanf-recovery-settings-callout' into 'master'

Prompt users to double check their account recovery settings

See merge request gitlab-org/gitlab!23994

app/assets/javascripts/main.js

@@ -36,6 +36,7 @@ import initSearchAutocomplete from './search_autocomplete';
 import GlFieldErrors from './gl_field_errors';
 import initUserPopovers from './user_popovers';
 import initBroadcastNotifications from './broadcast_notification';
+import PersistentUserCallout from './persistent_user_callout';
 import { initUserTracking } from './tracking';
 import { __ } from './locale';
 
@@ -108,6 +109,9 @@ function deferredInitialisation() {
   initUserTracking();
   initBroadcastNotifications();
 
+  const recoverySettingsCallout = document.querySelector('.js-recovery-settings-callout');
+  PersistentUserCallout.factory(recoverySettingsCallout);
+
   if (document.querySelector('.search')) initSearchAutocomplete();
 
   addSelectOnFocusBehaviour('.js-select-on-focus');

app/controllers/user_callouts_controller.rb

@@ -2,7 +2,10 @@
 
 class UserCalloutsController < ApplicationController
   def create
-    if ensure_callout.persisted?
+    callout = ensure_callout
+
+    if callout.persisted?
+      callout.update(dismissed_at: Time.now)
       respond_to do |format|
         format.json { head :ok }
       end

app/helpers/user_callouts_helper.rb

@@ -22,6 +22,9 @@ module UserCalloutsHelper
   def render_dashboard_gold_trial(user)
   end
 
+  def render_account_recovery_regular_check
+  end
+
   def show_suggest_popover?
     !user_dismissed?(SUGGEST_POPOVER_DISMISSED)
   end
@@ -32,8 +35,10 @@ module UserCalloutsHelper
 
   private
 
-  def user_dismissed?(feature_name)
-    current_user&.callouts&.find_by(feature_name: UserCallout.feature_names[feature_name])
+  def user_dismissed?(feature_name, ignore_dismissal_earlier_than = nil)
+    return false unless current_user
+
+    current_user.dismissed_callout?(feature_name: feature_name, ignore_dismissal_earlier_than: ignore_dismissal_earlier_than)
   end
 end
 

app/models/user.rb

@@ -1644,6 +1644,13 @@ class User < ApplicationRecord
   end
   # End of signup_flow experiment methods
 
+  def dismissed_callout?(feature_name:, ignore_dismissal_earlier_than: nil)
+    callouts = self.callouts.with_feature_name(feature_name)
+    callouts = callouts.with_dismissed_after(ignore_dismissal_earlier_than) if ignore_dismissal_earlier_than
+
+    callouts.any?
+  end
+
   # @deprecated
   alias_method :owned_or_masters_groups, :owned_or_maintainers_groups
 

app/models/user_callout.rb

@@ -12,4 +12,7 @@ class UserCallout < ApplicationRecord
     presence: true,
     uniqueness: { scope: :user_id },
     inclusion: { in: UserCallout.feature_names.keys }
+
+  scope :with_feature_name, -> (feature_name) { where(feature_name: UserCallout.feature_names[feature_name]) }
+  scope :with_dismissed_after, -> (dismissed_after) { where('dismissed_at > ?', dismissed_after) }
 end

app/views/layouts/_page.html.haml

@@ -11,6 +11,7 @@
       = render "layouts/nav/classification_level_banner"
       = yield :flash_message
       = render "shared/ping_consent"
+      = render_account_recovery_regular_check
       - unless @hide_breadcrumbs
         = render "layouts/nav/breadcrumbs"
       .d-flex

app/views/shared/_check_recovery_settings.html.haml

+.gl-alert.gl-alert-warning.js-recovery-settings-callout{ role: 'alert', data: { feature_id: "account_recovery_regular_check", dismiss_endpoint: user_callouts_path, defer_links: "true" } }
+  %button.js-close.gl-alert-dismiss.gl-cursor-pointer{ type: 'button', 'aria-label' => _('Dismiss') }
+    = sprite_icon('close', size: 16, css_class: 'gl-icon')
+  .gl-alert-body
+    - account_link_start = '<a class="deferred-link" href="%{url}">'.html_safe % { url: profile_account_path }
+    = _("Please ensure your account's %{account_link_start}recovery settings%{account_link_end} are up to date.").html_safe % { account_link_start: account_link_start, account_link_end: '</a>'.html_safe }

db/migrate/20200205143231_add_dissmised_at_to_user_callouts.rb

+# frozen_string_literal: true
+
+class AddDissmisedAtToUserCallouts < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    add_column :user_callouts, :dismissed_at, :datetime_with_timezone
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_02_04_131054) do
+ActiveRecord::Schema.define(version: 2020_02_05_143231) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -4101,6 +4101,7 @@ ActiveRecord::Schema.define(version: 2020_02_04_131054) do
   create_table "user_callouts", id: :serial, force: :cascade do |t|
     t.integer "feature_name", null: false
     t.integer "user_id", null: false
+    t.datetime_with_timezone "dismissed_at"
     t.index ["user_id", "feature_name"], name: "index_user_callouts_on_user_id_and_feature_name", unique: true
     t.index ["user_id"], name: "index_user_callouts_on_user_id"
   end

ee/app/helpers/ee/user_callouts_helper.rb

@@ -10,6 +10,7 @@ module EE
     GOLD_TRIAL = 'gold_trial'
     GOLD_TRIAL_BILLINGS = 'gold_trial_billings'
     THREAT_MONITORING_INFO = 'threat_monitoring_info'
+    ACCOUNT_RECOVERY_REGULAR_CHECK = 'account_recovery_regular_check'
 
     def show_canary_deployment_callout?(project)
       !user_dismissed?(CANARY_DEPLOYMENT) &&
@@ -57,6 +58,14 @@ module EE
       render 'shared/gold_trial_callout_content'
     end
 
+    def render_account_recovery_regular_check
+      return unless current_user &&
+          3.months.ago > current_user.created_at &&
+          !user_dismissed?(ACCOUNT_RECOVERY_REGULAR_CHECK, 3.months.ago)
+
+      render 'shared/check_recovery_settings'
+    end
+
     def render_billings_gold_trial(user, namespace)
       return if namespace.gold_plan?
       return unless namespace.never_had_trial?

ee/app/models/ee/user_callout_enums.rb

@@ -15,7 +15,8 @@ module EE
           geo_migrate_hashed_storage: 6,
           canary_deployment: 7,
           gold_trial_billings: 8,
-          threat_monitoring_info: 11
+          threat_monitoring_info: 11,
+          account_recovery_regular_check: 12
         )
       end
     end

ee/changelogs/unreleased/xanf-prompt-user-recovery.yml

+---
+title: Prompt users to check their account settings
+merge_request: 23994
+author:
+type: added

ee/spec/features/account_recovery_regular_check_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Account recovery regular check callout' do
+  context 'when signed in' do
+    let(:user) { create(:user, created_at: 4.months.ago ) }
+    let(:message) { "Please ensure your account's recovery settings are up to date." }
+
+    before do
+      gitlab_sign_in(user)
+    end
+
+    it 'shows callout if not dismissed' do
+      visit root_dashboard_path
+
+      expect(page).to have_content(message)
+    end
+
+    it 'hides callout when user opens profile', :js do
+      visit root_dashboard_path
+
+      expect(page).to have_content(message)
+
+      click_link 'recovery settings'
+      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)
+
+      find('.js-recovery-settings-callout .js-close').click
+      wait_for_requests
+
+      expect(page).not_to have_content(message)
+    end
+
+    it 'shows callout on next session if user did not dismissed it' do
+      visit root_dashboard_path
+
+      expect(page).to have_content(message)
+
+      gitlab_sign_out
+      gitlab_sign_in(user)
+      visit root_dashboard_path
+
+      expect(page).to have_content(message)
+    end
+  end
+end

ee/spec/helpers/ee/user_callouts_helper_spec.rb

@@ -61,7 +61,7 @@ describe EE::UserCalloutsHelper do
     context 'when hashed storage is disabled' do
       before do
         stub_application_setting(hashed_storage_enabled: false)
-        expect(helper).to receive(:current_user).and_return(user)
+        allow(helper).to receive(:current_user).and_return(user)
       end
 
       context 'when the enable warning has not been dismissed' do
@@ -102,7 +102,7 @@ describe EE::UserCalloutsHelper do
     context 'when hashed storage is enabled' do
       before do
         stub_application_setting(hashed_storage_enabled: true)
-        expect(helper).to receive(:current_user).and_return(user)
+        allow(helper).to receive(:current_user).and_return(user)
       end
 
       context 'when the enable warning has not been dismissed' do
@@ -269,13 +269,47 @@ describe EE::UserCalloutsHelper do
     end
   end
 
+  describe '#render_account_recovery_regular_check' do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:new_user) { create(:user) }
+    let(:old_user) { create(:user, created_at: 4.months.ago )}
+    let(:anonymous) { nil }
+
+    where(:kind_of_user, :dismissed_callout?, :should_render?) do
+      :anonymous | false | false
+      :new_user  | false | false
+      :old_user  | false | true
+      :old_user  | true  | false
+    end
+
+    with_them do
+      before do
+        user = send(kind_of_user)
+
+        allow(helper).to receive(:current_user).and_return(user)
+        allow(user).to receive(:dismissed_callout?).and_return(dismissed_callout?) if user
+      end
+
+      it do
+        if should_render?
+          expect(helper).to receive(:render).with('shared/check_recovery_settings')
+        else
+          expect(helper).not_to receive(:render)
+        end
+
+        helper.render_account_recovery_regular_check
+      end
+    end
+  end
+
   describe '.show_threat_monitoring_info?' do
     subject { helper.show_threat_monitoring_info? }
 
     let(:user) { create(:user) }
 
     before do
-      expect(helper).to receive(:current_user).and_return(user)
+      allow(helper).to receive(:current_user).and_return(user)
     end
 
     context 'when the threat monitoring info has not been dismissed' do

locale/gitlab.pot

@@ -13861,6 +13861,9 @@ msgstr ""
 msgid "Please enable and migrate to hashed storage to avoid security issues and ensure data integrity. %{migrate_link}"
 msgstr ""
 
+msgid "Please ensure your account's %{account_link_start}recovery settings%{account_link_end} are up to date."
+msgstr ""
+
 msgid "Please enter a non-negative number"
 msgstr ""
 

spec/controllers/user_callouts_controller_spec.rb

@@ -13,7 +13,7 @@ describe UserCalloutsController do
     subject { post :create, params: { feature_name: feature_name }, format: :json }
 
     context 'with valid feature name' do
-      let(:feature_name) { UserCallout.feature_names.first.first }
+      let(:feature_name) { UserCallout.feature_names.keys.first }
 
       context 'when callout entry does not exist' do
         it 'creates a callout entry with dismissed state' do
@@ -28,7 +28,7 @@ describe UserCalloutsController do
       end
 
       context 'when callout entry already exists' do
-        let!(:callout) { create(:user_callout, feature_name: UserCallout.feature_names.first.first, user: user) }
+        let!(:callout) { create(:user_callout, feature_name: UserCallout.feature_names.keys.first, user: user) }
 
         it 'returns success' do
           subject

spec/models/user_callout_spec.rb

@@ -17,4 +17,37 @@ describe UserCallout do
     it { is_expected.to validate_presence_of(:feature_name) }
     it { is_expected.to validate_uniqueness_of(:feature_name).scoped_to(:user_id).ignoring_case_sensitivity }
   end
+
+  describe 'scopes' do
+    describe '.with_feature_name' do
+      let(:second_feature_name) { described_class.feature_names.keys.second }
+      let(:last_feature_name) { described_class.feature_names.keys.last }
+
+      it 'returns callout for requested feature name only' do
+        callout1 = create(:user_callout, feature_name: second_feature_name )
+        create(:user_callout, feature_name: last_feature_name )
+
+        callouts = described_class.with_feature_name(second_feature_name)
+
+        expect(callouts).to match_array([callout1])
+      end
+    end
+
+    describe '.with_dismissed_after' do
+      let(:some_feature_name) { described_class.feature_names.keys.second }
+      let(:callout_dismissed_month_ago) { create(:user_callout, feature_name: some_feature_name, dismissed_at: 1.month.ago )}
+
+      it 'does not return callouts dismissed before specified date' do
+        callouts = described_class.with_dismissed_after(15.days.ago)
+
+        expect(callouts).to match_array([])
+      end
+
+      it 'returns callouts dismissed after specified date' do
+        callouts = described_class.with_dismissed_after(2.months.ago)
+
+        expect(callouts).to match_array([callout_dismissed_month_ago])
+      end
+    end
+  end
 end

spec/models/user_spec.rb

@@ -4157,6 +4157,40 @@ describe User, :do_not_mock_admin_mode do
     end
   end
 
+  describe '#dismissed_callout?' do
+    subject(:user) { create(:user) }
+
+    let(:feature_name) { UserCallout.feature_names.keys.first }
+
+    context 'when no callout dismissal record exists' do
+      it 'returns false when no ignore_dismissal_earlier_than provided' do
+        expect(user.dismissed_callout?(feature_name: feature_name)).to eq false
+      end
+
+      it 'returns false when ignore_dismissal_earlier_than provided' do
+        expect(user.dismissed_callout?(feature_name: feature_name, ignore_dismissal_earlier_than: 3.months.ago)).to eq false
+      end
+    end
+
+    context 'when dismissed callout exists' do
+      before do
+        create(:user_callout, user: user, feature_name: feature_name, dismissed_at: 4.months.ago)
+      end
+
+      it 'returns true when no ignore_dismissal_earlier_than provided' do
+        expect(user.dismissed_callout?(feature_name: feature_name)).to eq true
+      end
+
+      it 'returns true when ignore_dismissal_earlier_than is earlier than dismissed_at' do
+        expect(user.dismissed_callout?(feature_name: feature_name, ignore_dismissal_earlier_than: 6.months.ago)).to eq true
+      end
+
+      it 'returns false when ignore_dismissal_earlier_than is later than dismissed_at' do
+        expect(user.dismissed_callout?(feature_name: feature_name, ignore_dismissal_earlier_than: 3.months.ago)).to eq false
+      end
+    end
+  end
+
   describe 'bots & humans' do
     it 'returns corresponding users' do
       human = create(:user)