Merge branch 'diff-suggestions-popover' into 'master'

Added diff suggestion popover

Closes #56523

See merge request gitlab-org/gitlab-ce!29473

app/assets/javascripts/diffs/components/app.vue

@@ -69,6 +69,16 @@ export default {
       required: false,
       default: false,
     },
+    dismissEndpoint: {
+      type: String,
+      required: false,
+      default: '',
+    },
+    showSuggestPopover: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     const treeWidth =
@@ -141,7 +151,12 @@ export default {
     showTreeList: 'adjustView',
   },
   mounted() {
-    this.setBaseConfig({ endpoint: this.endpoint, projectPath: this.projectPath });
+    this.setBaseConfig({
+      endpoint: this.endpoint,
+      projectPath: this.projectPath,
+      dismissEndpoint: this.dismissEndpoint,
+      showSuggestPopover: this.showSuggestPopover,
+    });
 
     if (this.shouldShow) {
       this.fetchData();

app/assets/javascripts/diffs/components/diff_line_note_form.vue

@@ -42,6 +42,7 @@ export default {
       noteableData: state => state.notes.noteableData,
       diffViewType: state => state.diffs.diffViewType,
     }),
+    ...mapState('diffs', ['showSuggestPopover']),
     ...mapGetters('diffs', ['getDiffFileByHash']),
     ...mapGetters([
       'isLoggedIn',
@@ -80,7 +81,12 @@ export default {
     }
   },
   methods: {
-    ...mapActions('diffs', ['cancelCommentForm', 'assignDiscussionsToDiff', 'saveDiffDiscussion']),
+    ...mapActions('diffs', [
+      'cancelCommentForm',
+      'assignDiscussionsToDiff',
+      'saveDiffDiscussion',
+      'setSuggestPopoverDismissed',
+    ]),
     handleCancelCommentForm(shouldConfirm, isDirty) {
       if (shouldConfirm && isDirty) {
         const msg = s__('Notes|Are you sure you want to cancel creating this comment?');
@@ -125,11 +131,13 @@ export default {
       :line="line"
       :help-page-path="helpPagePath"
       :diff-file="diffFile"
+      :show-suggest-popover="showSuggestPopover"
       save-button-title="Comment"
       class="diff-comment-form"
       @handleFormUpdateAddToReview="addToReview"
       @cancelForm="handleCancelCommentForm"
       @handleFormUpdate="handleSaveNote"
+      @handleSuggestDismissed="setSuggestPopoverDismissed"
     />
   </div>
 </template>

app/assets/javascripts/diffs/index.js

@@ -72,6 +72,8 @@ export default function initDiffsApp(store) {
         currentUser: JSON.parse(dataset.currentUserData) || {},
         changesEmptyStateIllustration: dataset.changesEmptyStateIllustration,
         isFluidLayout: parseBoolean(dataset.isFluidLayout),
+        dismissEndpoint: dataset.dismissEndpoint,
+        showSuggestPopover: parseBoolean(dataset.showSuggestPopover),
       };
     },
     computed: {
@@ -99,6 +101,8 @@ export default function initDiffsApp(store) {
           shouldShow: this.activeTab === 'diffs',
           changesEmptyStateIllustration: this.changesEmptyStateIllustration,
           isFluidLayout: this.isFluidLayout,
+          dismissEndpoint: this.dismissEndpoint,
+          showSuggestPopover: this.showSuggestPopover,
         },
       });
     },

app/assets/javascripts/diffs/store/actions.js

@@ -36,8 +36,8 @@ import {
 import { diffViewerModes } from '~/ide/constants';
 
 export const setBaseConfig = ({ commit }, options) => {
-  const { endpoint, projectPath } = options;
-  commit(types.SET_BASE_CONFIG, { endpoint, projectPath });
+  const { endpoint, projectPath, dismissEndpoint, showSuggestPopover } = options;
+  commit(types.SET_BASE_CONFIG, { endpoint, projectPath, dismissEndpoint, showSuggestPopover });
 };
 
 export const fetchDiffFiles = ({ state, commit }) => {
@@ -455,5 +455,17 @@ export const toggleFullDiff = ({ dispatch, getters, state }, filePath) => {
 export const setFileCollapsed = ({ commit }, { filePath, collapsed }) =>
   commit(types.SET_FILE_COLLAPSED, { filePath, collapsed });
 
+export const setSuggestPopoverDismissed = ({ commit, state }) =>
+  axios
+    .post(state.dismissEndpoint, {
+      feature_name: 'suggest_popover_dismissed',
+    })
+    .then(() => {
+      commit(types.SET_SHOW_SUGGEST_POPOVER);
+    })
+    .catch(() => {
+      createFlash(s__('MergeRequest|Error dismissing suggestion popover. Please try again.'));
+    });
+
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};

app/assets/javascripts/diffs/store/modules/diff_state.js

@@ -28,4 +28,6 @@ export default () => ({
   renderTreeList: true,
   showWhitespace: true,
   fileFinderVisible: false,
+  dismissEndpoint: '',
+  showSuggestPopover: true,
 });

app/assets/javascripts/diffs/store/mutation_types.js

@@ -33,3 +33,5 @@ export const SET_HIDDEN_VIEW_DIFF_FILE_LINES = 'SET_HIDDEN_VIEW_DIFF_FILE_LINES'
 export const SET_CURRENT_VIEW_DIFF_FILE_LINES = 'SET_CURRENT_VIEW_DIFF_FILE_LINES';
 export const ADD_CURRENT_VIEW_DIFF_FILE_LINES = 'ADD_CURRENT_VIEW_DIFF_FILE_LINES';
 export const TOGGLE_DIFF_FILE_RENDERING_MORE = 'TOGGLE_DIFF_FILE_RENDERING_MORE';
+
+export const SET_SHOW_SUGGEST_POPOVER = 'SET_SHOW_SUGGEST_POPOVER';

app/assets/javascripts/diffs/store/mutations.js

@@ -11,8 +11,8 @@ import * as types from './mutation_types';
 
 export default {
   [types.SET_BASE_CONFIG](state, options) {
-    const { endpoint, projectPath } = options;
-    Object.assign(state, { endpoint, projectPath });
+    const { endpoint, projectPath, dismissEndpoint, showSuggestPopover } = options;
+    Object.assign(state, { endpoint, projectPath, dismissEndpoint, showSuggestPopover });
   },
 
   [types.SET_LOADING](state, isLoading) {
@@ -302,4 +302,7 @@ export default {
 
     file.renderingLines = !file.renderingLines;
   },
+  [types.SET_SHOW_SUGGEST_POPOVER](state) {
+    state.showSuggestPopover = false;
+  },
 };

app/assets/javascripts/notes/components/note_form.vue

@@ -77,6 +77,11 @@ export default {
       required: false,
       default: '',
     },
+    showSuggestPopover: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     let updatedNoteBody = this.noteBody;
@@ -247,6 +252,8 @@ export default {
         :can-suggest="canSuggest"
         :add-spacing-classes="false"
         :help-page-path="helpPagePath"
+        :show-suggest-popover="showSuggestPopover"
+        @handleSuggestDismissed="() => $emit('handleSuggestDismissed')"
       >
         <textarea
           id="note_note"
@@ -303,7 +310,7 @@ export default {
               {{ __('Add comment now') }}
             </button>
             <button
-              class="btn btn-cancel note-edit-cancel js-close-discussion-note-form"
+              class="btn note-edit-cancel js-close-discussion-note-form"
               type="button"
               @click="cancelHandler()"
             >

app/assets/javascripts/vue_shared/components/markdown/field.vue

@@ -67,6 +67,11 @@ export default {
       required: false,
       default: '',
     },
+    showSuggestPopover: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     return {
@@ -194,8 +199,10 @@ export default {
       :preview-markdown="previewMarkdown"
       :line-content="lineContent"
       :can-suggest="canSuggest"
+      :show-suggest-popover="showSuggestPopover"
       @preview-markdown="showPreviewTab"
       @write-markdown="showWriteTab"
+      @handleSuggestDismissed="() => $emit('handleSuggestDismissed')"
     />
     <div v-show="!previewMarkdown" class="md-write-holder">
       <div class="zen-backdrop">

app/assets/javascripts/vue_shared/components/markdown/header.vue

 <script>
 import $ from 'jquery';
-import { GlTooltipDirective } from '@gitlab/ui';
+import { GlPopover, GlButton, GlTooltipDirective } from '@gitlab/ui';
 import ToolbarButton from './toolbar_button.vue';
 import Icon from '../icon.vue';
 
@@ -8,6 +8,8 @@ export default {
   components: {
     ToolbarButton,
     Icon,
+    GlPopover,
+    GlButton,
   },
   directives: {
     GlTooltip: GlTooltipDirective,
@@ -27,6 +29,11 @@ export default {
       required: false,
       default: true,
     },
+    showSuggestPopover: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   computed: {
     mdTable() {
@@ -70,6 +77,9 @@ export default {
 
       this.$emit('write-markdown');
     },
+    handleSuggestDismissed() {
+      this.$emit('handleSuggestDismissed');
+    },
   },
 };
 </script>
@@ -93,66 +103,92 @@ export default {
         </button>
       </li>
       <li :class="{ active: !previewMarkdown }" class="md-header-toolbar">
-        <toolbar-button tag="**" :button-title="__('Add bold text')" icon="bold" />
-        <toolbar-button tag="*" :button-title="__('Add italic text')" icon="italic" />
-        <toolbar-button
-          :prepend="true"
-          tag="> "
-          :button-title="__('Insert a quote')"
-          icon="quote"
-        />
-        <toolbar-button tag="`" tag-block="```" :button-title="__('Insert code')" icon="code" />
-        <toolbar-button
-          tag="[{text}](url)"
-          tag-select="url"
-          :button-title="__('Add a link')"
-          icon="link"
-        />
-        <toolbar-button
-          :prepend="true"
-          tag="* "
-          :button-title="__('Add a bullet list')"
-          icon="list-bulleted"
-        />
-        <toolbar-button
-          :prepend="true"
-          tag="1. "
-          :button-title="__('Add a numbered list')"
-          icon="list-numbered"
-        />
-        <toolbar-button
-          :prepend="true"
-          tag="* [ ] "
-          :button-title="__('Add a task list')"
-          icon="task-done"
-        />
-        <toolbar-button
-          :tag="mdTable"
-          :prepend="true"
-          :button-title="__('Add a table')"
-          icon="table"
-        />
-        <toolbar-button
-          v-if="canSuggest"
-          :tag="mdSuggestion"
-          :prepend="true"
-          :button-title="__('Insert suggestion')"
-          :cursor-offset="4"
-          :tag-content="lineContent"
-          icon="doc-code"
-          class="qa-suggestion-btn"
-        />
-        <button
-          v-gl-tooltip
-          :aria-label="__('Go full screen')"
-          class="toolbar-btn toolbar-fullscreen-btn js-zen-enter"
-          data-container="body"
-          tabindex="-1"
-          :title="__('Go full screen')"
-          type="button"
-        >
-          <icon name="screen-full" />
-        </button>
+        <div class="d-inline-block">
+          <toolbar-button tag="**" :button-title="__('Add bold text')" icon="bold" />
+          <toolbar-button tag="*" :button-title="__('Add italic text')" icon="italic" />
+          <toolbar-button
+            :prepend="true"
+            tag="> "
+            :button-title="__('Insert a quote')"
+            icon="quote"
+          />
+        </div>
+        <div class="d-inline-block ml-md-2 ml-0">
+          <template v-if="canSuggest">
+            <toolbar-button
+              ref="suggestButton"
+              :tag="mdSuggestion"
+              :prepend="true"
+              :button-title="__('Insert suggestion')"
+              :cursor-offset="4"
+              :tag-content="lineContent"
+              icon="doc-code"
+              class="qa-suggestion-btn"
+              @click="handleSuggestDismissed"
+            />
+            <gl-popover
+              v-if="showSuggestPopover"
+              :target="() => $refs.suggestButton"
+              :css-classes="['diff-suggest-popover']"
+              placement="bottom"
+              :show="showSuggestPopover"
+            >
+              <strong>{{ __('New! Suggest changes directly') }}</strong>
+              <p class="mb-2">
+                {{ __('Suggest code changes which are immediately applied. Try it out!') }}
+              </p>
+              <gl-button variant="primary" size="sm" @click="handleSuggestDismissed">
+                {{ __('Got it') }}
+              </gl-button>
+            </gl-popover>
+          </template>
+          <toolbar-button tag="`" tag-block="```" :button-title="__('Insert code')" icon="code" />
+          <toolbar-button
+            tag="[{text}](url)"
+            tag-select="url"
+            :button-title="__('Add a link')"
+            icon="link"
+          />
+        </div>
+        <div class="d-inline-block ml-md-2 ml-0">
+          <toolbar-button
+            :prepend="true"
+            tag="* "
+            :button-title="__('Add a bullet list')"
+            icon="list-bulleted"
+          />
+          <toolbar-button
+            :prepend="true"
+            tag="1. "
+            :button-title="__('Add a numbered list')"
+            icon="list-numbered"
+          />
+          <toolbar-button
+            :prepend="true"
+            tag="* [ ] "
+            :button-title="__('Add a task list')"
+            icon="task-done"
+          />
+          <toolbar-button
+            :tag="mdTable"
+            :prepend="true"
+            :button-title="__('Add a table')"
+            icon="table"
+          />
+        </div>
+        <div class="d-inline-block ml-md-2 ml-0">
+          <button
+            v-gl-tooltip
+            :aria-label="__('Go full screen')"
+            class="toolbar-btn toolbar-fullscreen-btn js-zen-enter"
+            data-container="body"
+            tabindex="-1"
+            :title="__('Go full screen')"
+            type="button"
+          >
+            <icon name="screen-full" />
+          </button>
+        </div>
       </li>
     </ul>
   </div>

app/assets/javascripts/vue_shared/components/markdown/toolbar_button.vue

@@ -66,6 +66,7 @@ export default {
     class="toolbar-btn js-md"
     tabindex="-1"
     data-container="body"
+    @click="() => $emit('click')"
   >
     <icon :name="icon" />
   </button>

app/assets/stylesheets/framework/markdown_area.scss

@@ -154,11 +154,9 @@
 }
 
 .toolbar-fullscreen-btn {
-  margin-left: $gl-padding;
   margin-right: -5px;
 
   @include media-breakpoint-down(xs) {
-    margin-left: 0;
     margin-right: 0;
   }
 }

app/assets/stylesheets/pages/diff.scss

@@ -14,7 +14,7 @@
       position: -webkit-sticky;
       position: sticky;
       top: $mr-file-header-top;
-      z-index: 102;
+      z-index: 220;
 
       &::before {
         content: '';
@@ -1122,3 +1122,15 @@ table.code {
     outline: 0;
   }
 }
+
+.diff-suggest-popover {
+  &.popover {
+    width: 250px;
+    min-width: 250px;
+    z-index: 210;
+  }
+
+  .popover-header {
+    display: none;
+  }
+}

app/helpers/user_callouts_helper.rb

@@ -3,6 +3,7 @@
 module UserCalloutsHelper
   GKE_CLUSTER_INTEGRATION = 'gke_cluster_integration'.freeze
   GCP_SIGNUP_OFFER = 'gcp_signup_offer'.freeze
+  SUGGEST_POPOVER_DISMISSED = 'suggest_popover_dismissed'.freeze
 
   def show_gke_cluster_integration_callout?(project)
     can?(current_user, :create_cluster, project) &&
@@ -20,6 +21,10 @@ module UserCalloutsHelper
   def render_dashboard_gold_trial(user)
   end
 
+  def show_suggest_popover?
+    !user_dismissed?(SUGGEST_POPOVER_DISMISSED)
+  end
+
   private
 
   def user_dismissed?(feature_name)

app/models/user_callout_enums.rb

@@ -10,7 +10,8 @@ module UserCalloutEnums
     {
       gke_cluster_integration: 1,
       gcp_signup_offer: 2,
-      cluster_security_warning: 3
+      cluster_security_warning: 3,
+      suggest_popover_dismissed: 4
     }
   end
 end

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

@@ -80,7 +80,9 @@
         current_user_data: UserSerializer.new(project: @project).represent(current_user, {}, MergeRequestUserEntity).to_json,
         project_path: project_path(@merge_request.project),
         changes_empty_state_illustration: image_path('illustrations/merge_request_changes_empty.svg'),
-        is_fluid_layout: fluid_layout.to_s } }
+        is_fluid_layout: fluid_layout.to_s,
+        dismiss_endpoint: user_callouts_path,
+        show_suggest_popover: show_suggest_popover?.to_s } }
 
     .mr-loading-status
       = spinner

changelogs/unreleased/diff-suggestions-popover.yml

+---
+title: Added diff suggestion feature discovery popover
+merge_request:
+author:
+type: added

locale/gitlab.pot

@@ -4761,6 +4761,9 @@ msgstr ""
 msgid "Google authentication is not %{link_to_documentation}. Ask your GitLab administrator if you want to use this service."
 msgstr ""
 
+msgid "Got it"
+msgstr ""
+
 msgid "Got it!"
 msgstr ""
 
@@ -6153,6 +6156,9 @@ msgstr ""
 msgid "MergeRequest| %{paragraphStart}changed the description %{descriptionChangedTimes} times %{timeDifferenceMinutes}%{paragraphEnd}"
 msgstr ""
 
+msgid "MergeRequest|Error dismissing suggestion popover. Please try again."
+msgstr ""
+
 msgid "MergeRequest|Error loading full diff. Please try again."
 msgstr ""
 
@@ -6509,6 +6515,9 @@ msgstr ""
 msgid "New users set to external"
 msgstr ""
 
+msgid "New! Suggest changes directly"
+msgstr ""
+
 msgid "New..."
 msgstr ""
 
@@ -9655,6 +9664,9 @@ msgstr ""
 msgid "Successfully unlocked"
 msgstr ""
 
+msgid "Suggest code changes which are immediately applied. Try it out!"
+msgstr ""
+
 msgid "Suggested change"
 msgstr ""
 

spec/features/issues/markdown_toolbar_spec.rb

@@ -32,7 +32,7 @@ describe 'Issue markdown toolbar', :js do
     find('.js-main-target-form #note-body')
     page.evaluate_script('document.querySelectorAll(".js-main-target-form #note-body")[0].setSelectionRange(4, 50)')
 
-    find('.toolbar-btn:nth-child(2)').click
+    all('.toolbar-btn')[1].click
 
     expect(find('#note-body')[:value]).to eq("test\n*underline*\n")
   end

spec/features/merge_request/user_posts_notes_spec.rb

@@ -141,7 +141,7 @@ describe 'Merge request > User posts notes', :js do
 
         page.within('.current-note-edit-form') do
           expect(find('#note_note').value).to eq('This is the new content')
-          find('.js-md:first-child').click
+          first('.js-md').click
           expect(find('#note_note').value).to eq('This is the new content****')
         end
       end

spec/features/merge_request/user_suggests_changes_on_diff_spec.rb

@@ -28,6 +28,18 @@ describe 'User comments on a diff', :js do
   end
 
   context 'single suggestion note' do
+    it 'hides suggestion popover' do
+      click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
+
+      expect(page).to have_selector('.diff-suggest-popover')
+
+      page.within('.diff-suggest-popover') do
+        click_button 'Got it'
+      end
+
+      expect(page).not_to have_selector('.diff-suggest-popover')
+    end
+
     it 'suggestion is presented' do
       click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
 

spec/javascripts/diffs/components/app_spec.js

@@ -37,6 +37,8 @@ describe('diffs/components/app', () => {
         projectPath: 'namespace/project',
         currentUser: {},
         changesEmptyStateIllustration: '',
+        dismissEndpoint: '',
+        showSuggestPopover: true,
         ...props,
       },
       store,

spec/javascripts/diffs/store/actions_spec.js

@@ -37,6 +37,7 @@ import actions, {
   toggleFullDiff,
   setFileCollapsed,
   setExpandedDiffLines,
+  setSuggestPopoverDismissed,
 } from '~/diffs/store/actions';
 import eventHub from '~/notes/event_hub';
 import * as types from '~/diffs/store/mutation_types';
@@ -68,12 +69,19 @@ describe('DiffsStoreActions', () => {
     it('should set given endpoint and project path', done => {
       const endpoint = '/diffs/set/endpoint';
       const projectPath = '/root/project';
+      const dismissEndpoint = '/-/user_callouts';
+      const showSuggestPopover = false;
 
       testAction(
         setBaseConfig,
-        { endpoint, projectPath },
-        { endpoint: '', projectPath: '' },
-        [{ type: types.SET_BASE_CONFIG, payload: { endpoint, projectPath } }],
+        { endpoint, projectPath, dismissEndpoint, showSuggestPopover },
+        { endpoint: '', projectPath: '', dismissEndpoint: '', showSuggestPopover: true },
+        [
+          {
+            type: types.SET_BASE_CONFIG,
+            payload: { endpoint, projectPath, dismissEndpoint, showSuggestPopover },
+          },
+        ],
         [],
         done,
       );
@@ -1080,4 +1088,30 @@ describe('DiffsStoreActions', () => {
       );
     });
   });
+
+  describe('setSuggestPopoverDismissed', () => {
+    it('commits SET_SHOW_SUGGEST_POPOVER', done => {
+      const state = { dismissEndpoint: `${gl.TEST_HOST}/-/user_callouts` };
+      const mock = new MockAdapter(axios);
+      mock.onPost(state.dismissEndpoint).reply(200, {});
+
+      spyOn(axios, 'post').and.callThrough();
+
+      testAction(
+        setSuggestPopoverDismissed,
+        null,
+        state,
+        [{ type: types.SET_SHOW_SUGGEST_POPOVER }],
+        [],
+        () => {
+          expect(axios.post).toHaveBeenCalledWith(state.dismissEndpoint, {
+            feature_name: 'suggest_popover_dismissed',
+          });
+
+          mock.restore();
+          done();
+        },
+      );
+    });
+  });
 });

spec/javascripts/diffs/store/mutations_spec.js

@@ -850,4 +850,14 @@ describe('DiffsStoreMutations', () => {
       expect(file.renderingLines).toBe(false);
     });
   });
+
+  describe('SET_SHOW_SUGGEST_POPOVER', () => {
+    it('sets showSuggestPopover to false', () => {
+      const state = { showSuggestPopover: true };
+
+      mutations[types.SET_SHOW_SUGGEST_POPOVER](state);
+
+      expect(state.showSuggestPopover).toBe(false);
+    });
+  });
 });

spec/javascripts/vue_shared/components/markdown/header_spec.js

@@ -22,13 +22,13 @@ describe('Markdown field header component', () => {
       'Add bold text',
       'Add italic text',
       'Insert a quote',
+      'Insert suggestion',
       'Insert code',
       'Add a link',
       'Add a bullet list',
       'Add a numbered list',
       'Add a task list',
       'Add a table',
-      'Insert suggestion',
       'Go full screen',
     ];
     const elements = vm.$el.querySelectorAll('.toolbar-btn');