Merge branch 'ph/343329/requestAttentionOnboarding' into 'master'

Add onboarding steps for attention requests

See merge request gitlab-org/gitlab!82097

app/assets/javascripts/attention_requests/components/navigation_popover.vue

+<script>
+import { GlPopover, GlSprintf, GlButton, GlLink, GlIcon } from '@gitlab/ui';
+import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils';
+import { helpPagePath } from '~/helpers/help_page_helper';
+import UserCalloutDismisser from '~/vue_shared/components/user_callout_dismisser.vue';
+
+export default {
+  components: {
+    GlPopover,
+    GlSprintf,
+    GlButton,
+    GlLink,
+    GlIcon,
+    UserCalloutDismisser,
+  },
+  inject: {
+    message: {
+      default: '',
+    },
+    observerElSelector: {
+      default: '',
+    },
+    observerElToggledClass: {
+      default: '',
+    },
+    featureName: {
+      default: '',
+    },
+    popoverTarget: {
+      default: '',
+    },
+    showAttentionIcon: {
+      default: false,
+    },
+    delay: {
+      default: 0,
+    },
+    popoverCssClass: {
+      default: '',
+    },
+  },
+  data() {
+    return {
+      showPopover: false,
+      popoverPlacement: this.popoverPosition(),
+    };
+  },
+  mounted() {
+    this.observeEl = document.querySelector(this.observerElSelector);
+    this.observer = new MutationObserver(this.callback);
+    this.observer.observe(this.observeEl, {
+      attributes: true,
+    });
+    this.callback();
+
+    window.addEventListener('resize', () => {
+      this.popoverPlacement = this.popoverPosition();
+    });
+  },
+  beforeDestroy() {
+    this.observer.disconnect();
+  },
+  methods: {
+    callback() {
+      if (this.showPopover) {
+        this.$root.$emit('bv::hide::popover');
+      }
+
+      setTimeout(() => this.toggleShowPopover(), this.delay);
+    },
+    toggleShowPopover() {
+      this.showPopover = this.observeEl.classList.contains(this.observerElToggledClass);
+    },
+    getPopoverTarget() {
+      return document.querySelector(this.popoverTarget);
+    },
+    popoverPosition() {
+      if (bp.isDesktop()) {
+        return 'left';
+      }
+
+      return 'bottom';
+    },
+  },
+  docsPage: helpPagePath('development/code_review.html'),
+};
+</script>
+
+<template>
+  <user-callout-dismisser :feature-name="featureName">
+    <template #default="{ shouldShowCallout, dismiss }">
+      <gl-popover
+        v-if="shouldShowCallout"
+        :show-close-button="false"
+        :target="() => getPopoverTarget()"
+        :show="showPopover"
+        :delay="0"
+        triggers="manual"
+        :placement="popoverPlacement"
+        boundary="window"
+        no-fade
+        :css-classes="[popoverCssClass]"
+      >
+        <p v-for="(m, index) in message" :key="index" class="gl-mb-5">
+          <gl-sprintf :message="m">
+            <template #strong="{ content }">
+              <strong><gl-icon v-if="showAttentionIcon" name="attention" /> {{ content }}</strong>
+            </template>
+          </gl-sprintf>
+        </p>
+        <div class="gl-display-flex gl-align-items-center">
+          <gl-button size="small" variant="confirm" class="gl-mr-5" @click.prevent.stop="dismiss">
+            {{ __('Got it!') }}
+          </gl-button>
+          <gl-link :href="$options.docsPage" target="_blank">{{ __('Learn more') }}</gl-link>
+        </div>
+      </gl-popover>
+    </template>
+  </user-callout-dismisser>
+</template>

app/assets/javascripts/attention_requests/index.js

+import Vue from 'vue';
+import VueApollo from 'vue-apollo';
+import { __ } from '~/locale';
+import createDefaultClient from '~/lib/graphql';
+import NavigationPopover from './components/navigation_popover.vue';
+
+Vue.use(VueApollo);
+
+const apolloProvider = new VueApollo({
+  defaultClient: createDefaultClient(),
+});
+
+export const initTopNavPopover = () => {
+  const el = document.getElementById('js-need-attention-nav-onboarding');
+
+  if (!el) return;
+
+  // eslint-disable-next-line no-new
+  new Vue({
+    el,
+    apolloProvider,
+    provide: {
+      observerElSelector: '.user-counter.dropdown',
+      observerElToggledClass: 'show',
+      message: [
+        __(
+          '%{strongStart}Need your attention%{strongEnd} are the merge requests that need your help to move forward, as an assignee or reviewer.',
+        ),
+      ],
+      featureName: 'attention_requests_top_nav',
+      popoverTarget: '#js-need-attention-nav',
+    },
+    render(h) {
+      return h(NavigationPopover);
+    },
+  });
+};
+
+export const initSideNavPopover = () => {
+  const el = document.getElementById('js-need-attention-sidebar-onboarding');
+
+  if (!el) return;
+
+  // eslint-disable-next-line no-new
+  new Vue({
+    el,
+    apolloProvider,
+    provide: {
+      observerElSelector: '.js-right-sidebar',
+      observerElToggledClass: 'right-sidebar-expanded',
+      message: [
+        __(
+          'To ask someone to look at a merge request, select %{strongStart}Request attention%{strongEnd}. Select again to remove the request.',
+        ),
+        __(
+          'Some actions remove attention requests, like a reviewer approving or anyone merging the merge request.',
+        ),
+      ],
+      featureName: 'attention_requests_side_nav',
+      popoverTarget: '.js-attention-request-toggle',
+      showAttentionIcon: true,
+      delay: 500,
+      popoverCssClass: 'attention-request-sidebar-popover',
+    },
+    render(h) {
+      return h(NavigationPopover);
+    },
+  });
+};
+
+export default () => {
+  initTopNavPopover();
+};

app/assets/javascripts/main.js

@@ -161,6 +161,12 @@ function deferredInitialisation() {
 
   // Adding a helper class to activate animations only after all is rendered
   setTimeout(() => $body.addClass('page-initialised'), 1000);
+
+  if (window.gon?.features?.mrAttentionRequests) {
+    import('~/attention_requests')
+      .then((module) => module.default())
+      .catch(() => {});
+  }
 }
 
 const $body = $('body');

app/assets/javascripts/sidebar/components/attention_requested_toggle.vue

@@ -70,7 +70,10 @@ export default {
 </script>
 
 <template>
-  <span v-gl-tooltip.left.viewport="tooltipTitle" class="gl-display-inline-block">
+  <span
+    v-gl-tooltip.left.viewport="tooltipTitle"
+    class="gl-display-inline-block js-attention-request-toggle"
+  >
     <gl-button
       :loading="loading"
       :variant="user.attention_requested ? 'warning' : 'default'"

app/assets/javascripts/sidebar/sidebar_bundle.js

@@ -3,7 +3,17 @@ import Mediator from './sidebar_mediator';
 
 export default (store) => {
   const mediator = new Mediator(getSidebarOptions());
-  mediator.fetch();
+  mediator
+    .fetch()
+    .then(() => {
+      if (window.gon?.features?.mrAttentionRequests) {
+        return import('~/attention_requests');
+      }
+
+      return null;
+    })
+    .then((module) => module?.initSideNavPopover())
+    .catch(() => {});
 
   mountSidebar(mediator, store);
 };

app/assets/stylesheets/page_bundles/merge_requests.scss

@@ -757,3 +757,7 @@ $tabs-holder-z-index: 250;
     background: linear-gradient(to bottom, rgba(#333, 0), rgba(#333, 1));
   }
 }
+
+.attention-request-sidebar-popover {
+  z-index: 999;
+}

app/controllers/dashboard_controller.rb

@@ -20,10 +20,6 @@ class DashboardController < Dashboard::ApplicationController
 
   urgency :low, [:merge_requests]
 
-  before_action only: [:merge_requests] do
-    push_frontend_feature_flag(:mr_attention_requests, default_enabled: :yaml)
-  end
-
   def activity
     respond_to do |format|
       format.html

app/controllers/projects/merge_requests_controller.rb

@@ -30,9 +30,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   before_action :set_issuables_index, only: [:index]
   before_action :authenticate_user!, only: [:assign_related_issues]
   before_action :check_user_can_push_to_source_branch!, only: [:rebase]
-  before_action only: [:index, :show] do
-    push_frontend_feature_flag(:mr_attention_requests, project, default_enabled: :yaml)
-  end
 
   before_action only: [:show] do
     push_frontend_feature_flag(:file_identifier_hash)

app/models/users/callout.rb

@@ -46,7 +46,9 @@ module Users
       storage_enforcement_banner_first_enforcement_threshold: 43,
       storage_enforcement_banner_second_enforcement_threshold: 44,
       storage_enforcement_banner_third_enforcement_threshold: 45,
-      storage_enforcement_banner_fourth_enforcement_threshold: 46
+      storage_enforcement_banner_fourth_enforcement_threshold: 46,
+      attention_requests_top_nav: 47,
+      attention_requests_side_nav: 48
     }
 
     validates :feature_name,

app/views/layouts/header/_default.html.haml

@@ -79,7 +79,8 @@
                   %li.dropdown-header
                     = _('Merge requests')
                   - if Feature.enabled?(:mr_attention_requests, default_enabled: :yaml)
-                    %li
+                    %li#js-need-attention-nav
+                      #js-need-attention-nav-onboarding
                       = link_to attention_requested_mrs_dashboard_path, class: 'gl-display-flex! gl-align-items-center js-prefetch-document' do
                         = _('Need your attention')
                         = gl_badge_tag user_merge_requests_counts[:attention_requested_count], { size: :sm, variant: user_merge_requests_counts[:attention_requested_count] == 0 ? :neutral : :warning }, { class: 'merge-request-badge gl-ml-auto js-attention-count' }

app/views/projects/merge_requests/show.html.haml

@@ -92,5 +92,8 @@
 
 #js-review-bar
 
+- if Feature.enabled?(:mr_attention_requests, default_enabled: :yaml)
+  #js-need-attention-sidebar-onboarding
+
 = render 'projects/invite_members_modal', project: @project
 = render 'shared/web_ide_path'

doc/api/graphql/reference/index.md

@@ -18994,6 +18994,8 @@ Name of the feature that the callout is for.
 | Value | Description |
 | ----- | ----------- |
 | <a id="usercalloutfeaturenameenumactive_user_count_threshold"></a>`ACTIVE_USER_COUNT_THRESHOLD` | Callout feature name for active_user_count_threshold. |
+| <a id="usercalloutfeaturenameenumattention_requests_side_nav"></a>`ATTENTION_REQUESTS_SIDE_NAV` | Callout feature name for attention_requests_side_nav. |
+| <a id="usercalloutfeaturenameenumattention_requests_top_nav"></a>`ATTENTION_REQUESTS_TOP_NAV` | Callout feature name for attention_requests_top_nav. |
 | <a id="usercalloutfeaturenameenumbuy_pipeline_minutes_notification_dot"></a>`BUY_PIPELINE_MINUTES_NOTIFICATION_DOT` | Callout feature name for buy_pipeline_minutes_notification_dot. |
 | <a id="usercalloutfeaturenameenumcanary_deployment"></a>`CANARY_DEPLOYMENT` | Callout feature name for canary_deployment. |
 | <a id="usercalloutfeaturenameenumci_deprecation_warning_for_types_keyword"></a>`CI_DEPRECATION_WARNING_FOR_TYPES_KEYWORD` | Callout feature name for ci_deprecation_warning_for_types_keyword. |

lib/gitlab/gon_helper.rb

@@ -59,6 +59,7 @@ module Gitlab
       push_frontend_feature_flag(:sandboxed_mermaid, default_enabled: :yaml)
       push_frontend_feature_flag(:source_editor_toolbar, default_enabled: :yaml)
       push_frontend_feature_flag(:gl_avatar_for_all_user_avatars, default_enabled: :yaml)
+      push_frontend_feature_flag(:mr_attention_requests, default_enabled: :yaml)
     end
 
     # Exposes the state of a feature flag to the frontend code.

locale/gitlab.pot

@@ -984,6 +984,9 @@ msgstr ""
 msgid "%{strongOpen}Warning:%{strongClose} SAML group links can cause GitLab to automatically remove members from groups."
 msgstr ""
 
+msgid "%{strongStart}Need your attention%{strongEnd} are the merge requests that need your help to move forward, as an assignee or reviewer."
+msgstr ""
+
 msgid "%{strongStart}Tip:%{strongEnd} You can also check out merge requests locally. %{linkStart}Learn more.%{linkEnd}"
 msgstr ""
 
@@ -34428,6 +34431,9 @@ msgstr ""
 msgid "Solution"
 msgstr ""
 
+msgid "Some actions remove attention requests, like a reviewer approving or anyone merging the merge request."
+msgstr ""
+
 msgid "Some changes are not shown"
 msgstr ""
 
@@ -38468,6 +38474,9 @@ msgstr ""
 msgid "To add the entry manually, provide the following details to the application on your phone."
 msgstr ""
 
+msgid "To ask someone to look at a merge request, select %{strongStart}Request attention%{strongEnd}. Select again to remove the request."
+msgstr ""
+
 msgid "To confirm, type %{phrase_code}"
 msgstr ""
 

spec/features/users/login_spec.rb

@@ -797,6 +797,7 @@ RSpec.describe 'Login', :clean_gitlab_redis_sessions do
 
     context 'when 2FA is required for the user' do
       before do
+        stub_feature_flags(mr_attention_requests: false)
         group = create(:group, require_two_factor_authentication: true)
         group.add_developer(user)
       end

spec/frontend/attention_requests/components/navigation_popover_spec.js

+import { shallowMount } from '@vue/test-utils';
+import { GlPopover, GlButton, GlSprintf, GlIcon } from '@gitlab/ui';
+import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils';
+import NavigationPopover from '~/attention_requests/components/navigation_popover.vue';
+import { makeMockUserCalloutDismisser } from 'helpers/mock_user_callout_dismisser';
+
+let wrapper;
+let dismiss;
+
+function createComponent(provideData = {}, shouldShowCallout = true) {
+  wrapper = shallowMount(NavigationPopover, {
+    provide: {
+      message: ['Test'],
+      observerElSelector: '.js-test',
+      observerElToggledClass: 'show',
+      featureName: 'attention_requests',
+      popoverTarget: '.js-test-popover',
+      ...provideData,
+    },
+    stubs: {
+      UserCalloutDismisser: makeMockUserCalloutDismisser({
+        dismiss,
+        shouldShowCallout,
+      }),
+      GlSprintf,
+    },
+  });
+}
+
+describe('Attention requests navigation popover', () => {
+  beforeEach(() => {
+    setFixtures('<div><div class="js-test-popover"></div><div class="js-test"></div></div>');
+    dismiss = jest.fn();
+  });
+
+  afterEach(() => {
+    wrapper.destroy();
+    wrapper = null;
+  });
+
+  it('hides popover if callout is disabled', () => {
+    createComponent({}, false);
+
+    expect(wrapper.findComponent(GlPopover).exists()).toBe(false);
+  });
+
+  it('shows popover if callout is enabled', () => {
+    createComponent();
+
+    expect(wrapper.findComponent(GlPopover).exists()).toBe(true);
+  });
+
+  it.each`
+    isDesktop | device       | expectedPlacement
+    ${true}   | ${'desktop'} | ${'left'}
+    ${false}  | ${'mobile'}  | ${'bottom'}
+  `(
+    'sets popover position to $expectedPlacement on $device',
+    ({ isDesktop, expectedPlacement }) => {
+      jest.spyOn(bp, 'isDesktop').mockReturnValue(isDesktop);
+
+      createComponent();
+
+      expect(wrapper.findComponent(GlPopover).props('placement')).toBe(expectedPlacement);
+    },
+  );
+
+  it('calls dismiss when clicking action button', () => {
+    createComponent();
+
+    wrapper
+      .findComponent(GlButton)
+      .vm.$emit('click', { preventDefault() {}, stopPropagation() {} });
+
+    expect(dismiss).toHaveBeenCalled();
+  });
+
+  it('shows icon in text', () => {
+    createComponent({ showAttentionIcon: true, message: ['%{strongStart}Test%{strongEnd}'] });
+
+    const icon = wrapper.findComponent(GlIcon);
+
+    expect(icon.exists()).toBe(true);
+    expect(icon.props('name')).toBe('attention');
+  });
+});