Merge branch '51122-fix-navigating-discussions' into 'master'

Resolve "Navigating unresolved discussions on Merge Request page"

Closes #51122

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

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

@@ -4,6 +4,7 @@ import _ from 'underscore';
 import { __, sprintf } from '~/locale';
 import createFlash from '~/flash';
 import { GlLoadingIcon } from '@gitlab/ui';
+import eventHub from '../../notes/event_hub';
 import DiffFileHeader from './diff_file_header.vue';
 import DiffContent from './diff_content.vue';
 
@@ -75,6 +76,9 @@ export default {
       }
     },
   },
+  created() {
+    eventHub.$on(`loadCollapsedDiff/${this.file.file_hash}`, this.handleLoadCollapsedDiff);
+  },
   methods: {
     ...mapActions('diffs', ['loadCollapsedDiff', 'assignDiscussionsToDiff']),
     handleToggle() {

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

@@ -3,8 +3,9 @@ import axios from '~/lib/utils/axios_utils';
 import Cookies from 'js-cookie';
 import createFlash from '~/flash';
 import { s__ } from '~/locale';
-import { handleLocationHash, historyPushState } from '~/lib/utils/common_utils';
+import { handleLocationHash, historyPushState, scrollToElement } from '~/lib/utils/common_utils';
 import { mergeUrlParams, getLocationHash } from '~/lib/utils/url_utility';
+import eventHub from '../../notes/event_hub';
 import { getDiffPositionByLineCode, getNoteFormData } from './utils';
 import * as types from './mutation_types';
 import {
@@ -53,6 +54,10 @@ export const assignDiscussionsToDiff = (
         diffPositionByLineCode,
       });
     });
+
+  Vue.nextTick(() => {
+    eventHub.$emit('scrollToDiscussion');
+  });
 };
 
 export const removeDiscussionsFromDiff = ({ commit }, removeDiscussion) => {
@@ -60,6 +65,27 @@ export const removeDiscussionsFromDiff = ({ commit }, removeDiscussion) => {
   commit(types.REMOVE_LINE_DISCUSSIONS_FOR_FILE, { fileHash: file_hash, lineCode: line_code, id });
 };
 
+export const renderFileForDiscussionId = ({ commit, rootState, state }, discussionId) => {
+  const discussion = rootState.notes.discussions.find(d => d.id === discussionId);
+
+  if (discussion) {
+    const file = state.diffFiles.find(f => f.file_hash === discussion.diff_file.file_hash);
+
+    if (file) {
+      if (!file.renderIt) {
+        commit(types.RENDER_FILE, file);
+      }
+
+      if (file.collapsed) {
+        eventHub.$emit(`loadCollapsedDiff/${file.file_hash}`);
+        scrollToElement(document.getElementById(file.file_hash));
+      } else {
+        eventHub.$emit('scrollToDiscussion');
+      }
+    }
+  }
+};
+
 export const startRenderDiffsQueue = ({ state, commit }) => {
   const checkItem = () =>
     new Promise(resolve => {

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

@@ -170,7 +170,7 @@ export default {
         }
 
         if (!file.parallel_diff_lines || !file.highlighted_diff_lines) {
-          file.discussions = file.discussions.concat(discussion);
+          file.discussions = (file.discussions || []).concat(discussion);
         }
 
         return file;

app/assets/javascripts/lib/utils/common_utils.js

@@ -192,8 +192,12 @@ export const contentTop = () => {
   const mrTabsHeight = $('.merge-request-tabs').height() || 0;
   const headerHeight = $('.navbar-gitlab').height() || 0;
   const diffFilesChanged = $('.js-diff-files-changed').height() || 0;
+  const diffFileLargeEnoughScreen =
+    'matchMedia' in window ? window.matchMedia('min-width: 768') : true;
+  const diffFileTitleBar =
+    (diffFileLargeEnoughScreen && $('.diff-file .file-title-flex-parent:visible').height()) || 0;
 
-  return perfBar + mrTabsHeight + headerHeight + diffFilesChanged;
+  return perfBar + mrTabsHeight + headerHeight + diffFilesChanged + diffFileTitleBar;
 };
 
 export const scrollToElement = element => {

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

@@ -81,6 +81,7 @@ export default {
       'nextUnresolvedDiscussionId',
       'unresolvedDiscussionsCount',
       'hasUnresolvedDiscussions',
+      'showJumpToNextDiscussion',
     ]),
     author() {
       return this.initialDiscussion.author;
@@ -121,6 +122,12 @@ export default {
     resolvedText() {
       return this.discussion.resolved_by_push ? __('Automatically resolved') : __('Resolved');
     },
+    shouldShowJumpToNextDiscussion() {
+      return this.showJumpToNextDiscussion(
+        this.discussion.id,
+        this.discussionsByDiffOrder ? 'diff' : 'discussion',
+      );
+    },
     shouldRenderDiffs() {
       return this.discussion.diff_discussion && this.renderDiffFile;
     },
@@ -418,7 +425,7 @@ Please check your network connection and try again.`;
                           <icon name="issue-new" />
                         </a>
                       </div>
-                      <div v-if="hasUnresolvedDiscussions" class="btn-group" role="group">
+                      <div v-if="shouldShowJumpToNextDiscussion" class="btn-group" role="group">
                         <button
                           v-gl-tooltip
                           class="btn btn-default discussion-next-btn"

app/assets/javascripts/notes/mixins/discussion_navigation.js

 import { scrollToElement } from '~/lib/utils/common_utils';
+import eventHub from '../../notes/event_hub';
 
 export default {
   methods: {
-    jumpToDiscussion(id) {
-      if (id) {
-        const activeTab = window.mrTabs.currentAction;
-        const selector =
-          activeTab === 'diffs'
-            ? `ul.notes[data-discussion-id="${id}"]`
-            : `div.discussion[data-discussion-id="${id}"]`;
+    diffsJump(id) {
+      const selector = `ul.notes[data-discussion-id="${id}"]`;
+
+      eventHub.$once('scrollToDiscussion', () => {
         const el = document.querySelector(selector);
 
-        if (activeTab === 'commits' || activeTab === 'pipelines') {
-          window.mrTabs.activateTab('show');
+        if (el) {
+          scrollToElement(el);
+
+          return true;
         }
 
-        if (el) {
+        return false;
+      });
+
       this.expandDiscussion({ discussionId: id });
+    },
+    discussionJump(id) {
+      const selector = `div.discussion[data-discussion-id="${id}"]`;
+
+      const el = document.querySelector(selector);
 
+      this.expandDiscussion({ discussionId: id });
+
+      if (el) {
         scrollToElement(el);
+
         return true;
       }
-      }
 
       return false;
     },
+    jumpToDiscussion(id) {
+      if (id) {
+        const activeTab = window.mrTabs.currentAction;
+
+        if (activeTab === 'diffs') {
+          this.diffsJump(id);
+        } else if (activeTab === 'commits' || activeTab === 'pipelines') {
+          window.mrTabs.eventHub.$once('MergeRequestTabChange', () => {
+            setTimeout(() => this.discussionJump(id), 0);
+          });
+
+          window.mrTabs.tabShown('show');
+        } else {
+          this.discussionJump(id);
+        }
+      }
+    },
   },
 };

app/assets/javascripts/notes/stores/actions.js

@@ -17,7 +17,13 @@ import { __ } from '~/locale';
 
 let eTagPoll;
 
-export const expandDiscussion = ({ commit }, data) => commit(types.EXPAND_DISCUSSION, data);
+export const expandDiscussion = ({ commit, dispatch }, data) => {
+  if (data.discussionId) {
+    dispatch('diffs/renderFileForDiscussionId', data.discussionId, { root: true });
+  }
+
+  commit(types.EXPAND_DISCUSSION, data);
+};
 
 export const collapseDiscussion = ({ commit }, data) => commit(types.COLLAPSE_DISCUSSION, data);
 

app/assets/javascripts/notes/stores/getters.js

@@ -57,6 +57,17 @@ export const unresolvedDiscussionsCount = state => state.unresolvedDiscussionsCo
 export const resolvableDiscussionsCount = state => state.resolvableDiscussionsCount;
 export const hasUnresolvedDiscussions = state => state.hasUnresolvedDiscussions;
 
+export const showJumpToNextDiscussion = (state, getters) => (discussionId, mode = 'discussion') => {
+  const orderedDiffs =
+    mode !== 'discussion'
+      ? getters.unresolvedDiscussionsIdsByDiff
+      : getters.unresolvedDiscussionsIdsByDate;
+
+  const indexOf = orderedDiffs.indexOf(discussionId);
+
+  return indexOf !== -1 && indexOf < orderedDiffs.length - 1;
+};
+
 export const isDiscussionResolved = (state, getters) => discussionId =>
   getters.resolvedDiscussionsById[discussionId] !== undefined;
 
@@ -104,7 +115,7 @@ export const unresolvedDiscussionsIdsByDate = (state, getters) =>
 // line numbers.
 export const unresolvedDiscussionsIdsByDiff = (state, getters) =>
   getters.allResolvableDiscussions
-    .filter(d => !d.resolved)
+    .filter(d => !d.resolved && d.active)
     .sort((a, b) => {
       if (!a.diff_file || !b.diff_file) {
         return 0;

app/assets/javascripts/notes/stores/mutations.js

@@ -22,6 +22,7 @@ export default {
       if (isDiscussion && isInMRPage()) {
         noteData.resolvable = note.resolvable;
         noteData.resolved = false;
+        noteData.active = true;
         noteData.resolve_path = note.resolve_path;
         noteData.resolve_with_issue_path = note.resolve_with_issue_path;
         noteData.diff_discussion = false;

changelogs/unreleased/51122-fix-navigating-discussions.yml

+---
+title: Fix navigating by unresolved discussions on Merge Request page
+merge_request: 22789
+author:
+type: fixed

spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb

@@ -361,8 +361,14 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
         end
       end
 
-      it 'shows jump to next discussion button' do
-        expect(page.all('.discussion-reply-holder', count: 2)).to all(have_selector('.discussion-next-btn'))
+      it 'shows jump to next discussion button except on last discussion' do
+        wait_for_requests
+
+        all_discussion_replies = page.all('.discussion-reply-holder')
+
+        expect(all_discussion_replies.count).to eq(2)
+        expect(all_discussion_replies.first.all('.discussion-next-btn').count).to eq(1)
+        expect(all_discussion_replies.last.all('.discussion-next-btn').count).to eq(0)
       end
 
       it 'displays next discussion even if hidden' do
@@ -380,7 +386,13 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
           page.find('.discussion-next-btn').click
         end
 
-        expect(find('.discussion-with-resolve-btn')).to have_selector('.btn', text: 'Resolve discussion')
+        page.all('.note-discussion').first do
+          expect(page.find('.discussion-with-resolve-btn')).to have_selector('.btn', text: 'Resolve discussion')
+        end
+
+        page.all('.note-discussion').last do
+          expect(page.find('.discussion-with-resolve-btn')).not.to have_selector('.btn', text: 'Resolve discussion')
+        end
       end
     end
 

spec/javascripts/diffs/store/actions_spec.js

@@ -26,7 +26,9 @@ import actions, {
   toggleTreeOpen,
   scrollToFile,
   toggleShowTreeList,
+  renderFileForDiscussionId,
 } from '~/diffs/store/actions';
+import eventHub from '~/notes/event_hub';
 import * as types from '~/diffs/store/mutation_types';
 import axios from '~/lib/utils/axios_utils';
 import mockDiffFile from 'spec/diffs/mock_data/diff_file';
@@ -735,4 +737,63 @@ describe('DiffsStoreActions', () => {
       expect(localStorage.setItem).toHaveBeenCalledWith('mr_tree_show', true);
     });
   });
+
+  describe('renderFileForDiscussionId', () => {
+    const rootState = {
+      notes: {
+        discussions: [
+          {
+            id: '123',
+            diff_file: {
+              file_hash: 'HASH',
+            },
+          },
+          {
+            id: '456',
+            diff_file: {
+              file_hash: 'HASH',
+            },
+          },
+        ],
+      },
+    };
+    let commit;
+    let $emit;
+    let scrollToElement;
+    const state = ({ collapsed, renderIt }) => ({
+      diffFiles: [
+        {
+          file_hash: 'HASH',
+          collapsed,
+          renderIt,
+        },
+      ],
+    });
+
+    beforeEach(() => {
+      commit = jasmine.createSpy('commit');
+      scrollToElement = spyOnDependency(actions, 'scrollToElement').and.stub();
+      $emit = spyOn(eventHub, '$emit');
+    });
+
+    it('renders and expands file for the given discussion id', () => {
+      const localState = state({ collapsed: true, renderIt: false });
+
+      renderFileForDiscussionId({ rootState, state: localState, commit }, '123');
+
+      expect(commit).toHaveBeenCalledWith('RENDER_FILE', localState.diffFiles[0]);
+      expect($emit).toHaveBeenCalledTimes(1);
+      expect(scrollToElement).toHaveBeenCalledTimes(1);
+    });
+
+    it('jumps to discussion on already rendered and expanded file', () => {
+      const localState = state({ collapsed: false, renderIt: true });
+
+      renderFileForDiscussionId({ rootState, state: localState, commit }, '123');
+
+      expect(commit).not.toHaveBeenCalled();
+      expect($emit).toHaveBeenCalledTimes(1);
+      expect(scrollToElement).not.toHaveBeenCalled();
+    });
+  });
 });

spec/javascripts/notes/components/noteable_discussion_spec.js

@@ -83,6 +83,7 @@ describe('noteable_discussion component', () => {
       it('expands next unresolved discussion', done => {
         const discussion2 = getJSONFixture(discussionWithTwoUnresolvedNotes)[0];
         discussion2.resolved = false;
+        discussion2.active = true;
         discussion2.id = 'next'; // prepare this for being identified as next one (to be jumped to)
         vm.$store.dispatch('setInitialNotes', [discussionMock, discussion2]);
         window.mrTabs.currentAction = 'show';

spec/javascripts/notes/mock_data.js

@@ -305,6 +305,7 @@ export const discussionMock = {
   ],
   individual_note: false,
   resolvable: true,
+  active: true,
 };
 
 export const loggedOutnoteableData = {
@@ -1173,6 +1174,7 @@ export const discussion1 = {
   id: 'abc1',
   resolvable: true,
   resolved: false,
+  active: true,
   diff_file: {
     file_path: 'about.md',
   },
@@ -1209,6 +1211,7 @@ export const discussion2 = {
   id: 'abc2',
   resolvable: true,
   resolved: false,
+  active: true,
   diff_file: {
     file_path: 'README.md',
   },
@@ -1226,6 +1229,7 @@ export const discussion2 = {
 export const discussion3 = {
   id: 'abc3',
   resolvable: true,
+  active: true,
   resolved: false,
   diff_file: {
     file_path: 'README.md',

spec/javascripts/notes/stores/actions_spec.js

@@ -124,7 +124,7 @@ describe('Actions Notes Store', () => {
         { discussionId: discussionMock.id },
         { notes: [discussionMock] },
         [{ type: 'EXPAND_DISCUSSION', payload: { discussionId: discussionMock.id } }],
-        [],
+        [{ type: 'diffs/renderFileForDiscussionId', payload: discussionMock.id }],
         done,
       );
     });