Merge branch 'xanf-fix-unresolved-discussion-jump' into 'master'

Next unresolved discussion button takes user to the right place

See merge request gitlab-org/gitlab!20620

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

@@ -19,6 +19,7 @@ export default {
       'resolvableDiscussionsCount',
       'firstUnresolvedDiscussionId',
       'unresolvedDiscussionsCount',
+      'getDiscussion',
     ]),
     isLoggedIn() {
       return this.getUserData.id;
@@ -40,9 +41,10 @@ export default {
     ...mapActions(['expandDiscussion']),
     jumpToFirstUnresolvedDiscussion() {
       const diffTab = window.mrTabs.currentAction === 'diffs';
-      const discussionId = this.firstUnresolvedDiscussionId(diffTab);
-
-      this.jumpToDiscussion(discussionId);
+      const discussionId =
+        this.firstUnresolvedDiscussionId(diffTab) || this.firstUnresolvedDiscussionId();
+      const firstDiscussion = this.getDiscussion(discussionId);
+      this.jumpToDiscussion(firstDiscussion);
     },
   },
 };

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

@@ -19,7 +19,11 @@ export default {
     };
   },
   computed: {
-    ...mapGetters(['nextUnresolvedDiscussionId', 'previousUnresolvedDiscussionId']),
+    ...mapGetters([
+      'nextUnresolvedDiscussionId',
+      'previousUnresolvedDiscussionId',
+      'getDiscussion',
+    ]),
   },
   mounted() {
     Mousetrap.bind('n', () => this.jumpToNextDiscussion());
@@ -33,14 +37,14 @@ export default {
     ...mapActions(['expandDiscussion']),
     jumpToNextDiscussion() {
       const nextId = this.nextUnresolvedDiscussionId(this.currentDiscussionId, this.isDiffView);
-
-      this.jumpToDiscussion(nextId);
+      const nextDiscussion = this.getDiscussion(nextId);
+      this.jumpToDiscussion(nextDiscussion);
       this.currentDiscussionId = nextId;
     },
     jumpToPreviousDiscussion() {
       const prevId = this.previousUnresolvedDiscussionId(this.currentDiscussionId, this.isDiffView);
-
-      this.jumpToDiscussion(prevId);
+      const prevDiscussion = this.getDiscussion(prevId);
+      this.jumpToDiscussion(prevDiscussion);
       this.currentDiscussionId = prevId;
     },
   },

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

@@ -84,6 +84,7 @@ export default {
       'hasUnresolvedDiscussions',
       'showJumpToNextDiscussion',
       'getUserData',
+      'getDiscussion',
     ]),
     currentUser() {
       return this.getUserData;
@@ -221,8 +222,9 @@ export default {
         this.discussion.id,
         this.discussionsByDiffOrder,
       );
+      const nextDiscussion = this.getDiscussion(nextId);
 
-      this.jumpToDiscussion(nextId);
+      this.jumpToDiscussion(nextDiscussion);
     },
     deleteNoteHandler(note) {
       this.$emit('noteDeleted', this.discussion, note);

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

@@ -35,20 +35,26 @@ export default {
 
       return false;
     },
-    jumpToDiscussion(id) {
+
+    switchToDiscussionsTabAndJumpTo(id) {
+      window.mrTabs.eventHub.$once('MergeRequestTabChange', () => {
+        setTimeout(() => this.discussionJump(id), 0);
+      });
+
+      window.mrTabs.tabShown('show');
+    },
+
+    jumpToDiscussion(discussion) {
+      const { id, diff_discussion: isDiffDiscussion } = discussion;
       if (id) {
         const activeTab = window.mrTabs.currentAction;
 
-        if (activeTab === 'diffs') {
+        if (activeTab === 'diffs' && isDiffDiscussion) {
           this.diffsJump(id);
-        } else if (activeTab === 'commits' || activeTab === 'pipelines') {
-          window.mrTabs.eventHub.$once('MergeRequestTabChange', () => {
-            setTimeout(() => this.discussionJump(id), 0);
-          });
-
-          window.mrTabs.tabShown('show');
-        } else {
+        } else if (activeTab === 'show') {
           this.discussionJump(id);
+        } else {
+          this.switchToDiscussionsTabAndJumpTo(id);
         }
       }
     },

changelogs/unreleased/xanf-fix-unresolved-discussion-jump.yml

+---
+title: Ensure next unresolved discussion button takes user to the right place
+merge_request: 20620
+author:
+type: fixed

spec/frontend/notes/components/discussion_keyboard_navigator_spec.js

@@ -37,6 +37,7 @@ describe('notes/components/discussion_keyboard_navigator', () => {
       isDiff ? NEXT_DIFF_ID : NEXT_ID;
     notes.getters.previousUnresolvedDiscussionId = () => (currId, isDiff) =>
       isDiff ? PREV_DIFF_ID : PREV_ID;
+    notes.getters.getDiscussion = () => id => ({ id });
 
     storeOptions = {
       modules: {
@@ -63,14 +64,18 @@ describe('notes/components/discussion_keyboard_navigator', () => {
     it('calls jumpToNextDiscussion when pressing `n`', () => {
       Mousetrap.trigger('n');
 
-      expect(wrapper.vm.jumpToDiscussion).toHaveBeenCalledWith(expectedNextId);
+      expect(wrapper.vm.jumpToDiscussion).toHaveBeenCalledWith(
+        expect.objectContaining({ id: expectedNextId }),
+      );
       expect(wrapper.vm.currentDiscussionId).toEqual(expectedNextId);
     });
 
     it('calls jumpToPreviousDiscussion when pressing `p`', () => {
       Mousetrap.trigger('p');
 
-      expect(wrapper.vm.jumpToDiscussion).toHaveBeenCalledWith(expectedPrevId);
+      expect(wrapper.vm.jumpToDiscussion).toHaveBeenCalledWith(
+        expect.objectContaining({ id: expectedPrevId }),
+      );
       expect(wrapper.vm.currentDiscussionId).toEqual(expectedPrevId);
     });
   });

spec/javascripts/notes/components/discussion_counter_spec.js

@@ -27,6 +27,8 @@ describe('DiscussionCounter component', () => {
   describe('methods', () => {
     describe('jumpToFirstUnresolvedDiscussion', () => {
       it('expands unresolved discussion', () => {
+        window.mrTabs.currentAction = 'show';
+
         spyOn(vm, 'expandDiscussion').and.stub();
         const discussions = [
           {
@@ -47,14 +49,39 @@ describe('DiscussionCounter component', () => {
           ...store.state,
           discussions,
         });
-        setFixtures(`
-          <div class="discussion" data-discussion-id="${firstDiscussionId}"></div>
-        `);
-
         vm.jumpToFirstUnresolvedDiscussion();
 
         expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: firstDiscussionId });
       });
+
+      it('jumps to first unresolved discussion from diff tab if all diff discussions are resolved', () => {
+        window.mrTabs.currentAction = 'diff';
+        spyOn(vm, 'switchToDiscussionsTabAndJumpTo').and.stub();
+
+        const unresolvedId = discussionMock.id + 1;
+        const discussions = [
+          {
+            ...discussionMock,
+            id: discussionMock.id,
+            diff_discussion: true,
+            notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: true }],
+            resolved: true,
+          },
+          {
+            ...discussionMock,
+            id: unresolvedId,
+            notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: false }],
+            resolved: false,
+          },
+        ];
+        store.replaceState({
+          ...store.state,
+          discussions,
+        });
+        vm.jumpToFirstUnresolvedDiscussion();
+
+        expect(vm.switchToDiscussionsTabAndJumpTo).toHaveBeenCalledWith(unresolvedId);
+      });
     });
   });
 });