Merge branch 'jdb/cycle-unresolved-threads' into 'master'

Cycle unresolved threads

See merge request gitlab-org/gitlab!23123

app/assets/javascripts/diff_notes/components/jump_to_discussion.js

@@ -24,9 +24,9 @@ const JumpToDiscussion = Vue.extend({
   computed: {
     buttonText() {
       if (this.discussionId) {
-        return __('Jump to next unresolved discussion');
+        return __('Jump to next unresolved thread');
       } else {
-        return __('Jump to first unresolved discussion');
+        return __('Jump to first unresolved thread');
       }
     },
     allResolved() {

app/assets/javascripts/mr_notes/init_notes.js

@@ -69,13 +69,11 @@ export default () => {
       },
     },
     render(createElement) {
-      const isDiffView = this.activeTab === 'diffs';
-
       // NOTE: Even though `discussionKeyboardNavigator` is added to the `notes-app`,
       // it adds a global key listener so it works on the diffs tab as well.
       // If we create a single Vue app for all of the MR tabs, we should move this
       // up the tree, to the root.
-      return createElement(discussionKeyboardNavigator, { props: { isDiffView } }, [
+      return createElement(discussionKeyboardNavigator, [
         createElement('notes-app', {
           props: {
             noteableData: this.noteableData,

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

@@ -73,7 +73,7 @@ export default {
       v-if="discussion.resolvable && shouldShowJumpToNextDiscussion"
       class="btn-group discussion-actions ml-sm-2"
     >
-      <jump-to-next-discussion-button @onClick="$emit('jumpToNextDiscussion')" />
+      <jump-to-next-discussion-button />
     </div>
   </div>
 </template>

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

 <script>
-import { mapActions, mapGetters } from 'vuex';
+import { mapGetters } from 'vuex';
 import { GlTooltipDirective } from '@gitlab/ui';
 import Icon from '~/vue_shared/components/icon.vue';
 import discussionNavigation from '../mixins/discussion_navigation';
@@ -17,9 +17,7 @@ export default {
       'getUserData',
       'getNoteableData',
       'resolvableDiscussionsCount',
-      'firstUnresolvedDiscussionId',
       'unresolvedDiscussionsCount',
-      'getDiscussion',
     ]),
     isLoggedIn() {
       return this.getUserData.id;
@@ -37,16 +35,6 @@ export default {
       return this.resolvableDiscussionsCount - this.unresolvedDiscussionsCount;
     },
   },
-  methods: {
-    ...mapActions(['expandDiscussion']),
-    jumpToFirstUnresolvedDiscussion() {
-      const diffTab = window.mrTabs.currentAction === 'diffs';
-      const discussionId =
-        this.firstUnresolvedDiscussionId(diffTab) || this.firstUnresolvedDiscussionId();
-      const firstDiscussion = this.getDiscussion(discussionId);
-      this.jumpToDiscussion(firstDiscussion);
-    },
-  },
 };
 </script>
 
@@ -83,9 +71,9 @@ export default {
       <div v-if="isLoggedIn && !allResolved" class="btn-group btn-group-sm" role="group">
         <button
           v-gl-tooltip
-          title="Jump to first unresolved thread"
+          title="Jump to next unresolved thread"
           class="btn btn-default discussion-next-btn"
-          @click="jumpToFirstUnresolvedDiscussion"
+          @click="jumpToNextDiscussion"
         >
           <icon name="comment-next" />
         </button>

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

 <script>
 import { GlTooltipDirective } from '@gitlab/ui';
 import icon from '~/vue_shared/components/icon.vue';
+import discussionNavigation from '../mixins/discussion_navigation';
 
 export default {
   name: 'JumpToNextDiscussionButton',
@@ -10,6 +11,7 @@ export default {
   directives: {
     GlTooltip: GlTooltipDirective,
   },
+  mixins: [discussionNavigation],
 };
 </script>
 
@@ -19,8 +21,8 @@ export default {
       ref="button"
       v-gl-tooltip
       class="btn btn-default discussion-next-btn"
-      :title="s__('MergeRequests|Jump to next unresolved discussion')"
-      @click="$emit('onClick')"
+      :title="s__('MergeRequests|Jump to next unresolved thread')"
+      @click="jumpToNextDiscussion"
     >
       <icon name="comment-next" />
     </button>

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

 <script>
 /* global Mousetrap */
 import 'mousetrap';
-import { mapGetters, mapActions } from 'vuex';
 import discussionNavigation from '~/notes/mixins/discussion_navigation';
 
 export default {
   mixins: [discussionNavigation],
-  props: {
-    isDiffView: {
-      type: Boolean,
-      required: false,
-      default: false,
-    },
-  },
-  data() {
-    return {
-      currentDiscussionId: null,
-    };
-  },
-  computed: {
-    ...mapGetters([
-      'nextUnresolvedDiscussionId',
-      'previousUnresolvedDiscussionId',
-      'getDiscussion',
-    ]),
-  },
   mounted() {
-    Mousetrap.bind('n', () => this.jumpToNextDiscussion());
-    Mousetrap.bind('p', () => this.jumpToPreviousDiscussion());
+    Mousetrap.bind('n', this.jumpToNextDiscussion);
+    Mousetrap.bind('p', this.jumpToPreviousDiscussion);
   },
   beforeDestroy() {
     Mousetrap.unbind('n');
     Mousetrap.unbind('p');
   },
-  methods: {
-    ...mapActions(['expandDiscussion']),
-    jumpToNextDiscussion() {
-      const nextId = this.nextUnresolvedDiscussionId(this.currentDiscussionId, this.isDiffView);
-      const nextDiscussion = this.getDiscussion(nextId);
-      this.jumpToDiscussion(nextDiscussion);
-      this.currentDiscussionId = nextId;
-    },
-    jumpToPreviousDiscussion() {
-      const prevId = this.previousUnresolvedDiscussionId(this.currentDiscussionId, this.isDiffView);
-      const prevDiscussion = this.getDiscussion(prevId);
-      this.jumpToDiscussion(prevDiscussion);
-      this.currentDiscussionId = prevId;
-    },
-  },
   render() {
     return this.$slots.default;
   },

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

@@ -14,7 +14,6 @@ import noteForm from './note_form.vue';
 import diffWithNote from './diff_with_note.vue';
 import noteable from '../mixins/noteable';
 import resolvable from '../mixins/resolvable';
-import discussionNavigation from '../mixins/discussion_navigation';
 import eventHub from '../event_hub';
 import DiscussionNotes from './discussion_notes.vue';
 import DiscussionActions from './discussion_actions.vue';
@@ -35,7 +34,7 @@ export default {
   directives: {
     GlTooltip: GlTooltipDirective,
   },
-  mixins: [noteable, resolvable, discussionNavigation, diffLineNoteFormMixin],
+  mixins: [noteable, resolvable, diffLineNoteFormMixin],
   props: {
     discussion: {
       type: Object,
@@ -79,12 +78,8 @@ export default {
       'convertedDisscussionIds',
       'getNoteableData',
       'userCanReply',
-      'nextUnresolvedDiscussionId',
-      'unresolvedDiscussionsCount',
-      'hasUnresolvedDiscussions',
       'showJumpToNextDiscussion',
       'getUserData',
-      'getDiscussion',
     ]),
     currentUser() {
       return this.getUserData;
@@ -152,7 +147,6 @@ export default {
       'saveNote',
       'removePlaceholderNotes',
       'toggleResolveNote',
-      'expandDiscussion',
       'removeConvertedDiscussion',
     ]),
     showReplyForm() {
@@ -219,15 +213,6 @@ export default {
           callback(err);
         });
     },
-    jumpToNextDiscussion() {
-      const nextId = this.nextUnresolvedDiscussionId(
-        this.discussion.id,
-        this.discussionsByDiffOrder,
-      );
-      const nextDiscussion = this.getDiscussion(nextId);
-
-      this.jumpToDiscussion(nextDiscussion);
-    },
     deleteNoteHandler(note) {
       this.$emit('noteDeleted', this.discussion, note);
     },
@@ -294,7 +279,6 @@ export default {
                     :should-show-jump-to-next-discussion="shouldShowJumpToNextDiscussion"
                     @showReplyForm="showReplyForm"
                     @resolve="resolveHandler"
-                    @jumpToNextDiscussion="jumpToNextDiscussion"
                   />
                   <div v-if="isReplying" class="avatar-note-form-holder">
                     <user-avatar-link

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

+import { mapGetters, mapActions, mapState } from 'vuex';
 import { scrollToElement } from '~/lib/utils/common_utils';
 import eventHub from '../../notes/event_hub';
 
 export default {
+  computed: {
+    ...mapGetters([
+      'nextUnresolvedDiscussionId',
+      'previousUnresolvedDiscussionId',
+      'getDiscussion',
+    ]),
+    ...mapState({
+      currentDiscussionId: state => state.notes.currentDiscussionId,
+    }),
+  },
   methods: {
+    ...mapActions(['expandDiscussion', 'setCurrentDiscussionId']),
+
     diffsJump(id) {
       const selector = `ul.notes[data-discussion-id="${id}"]`;
 
@@ -58,5 +71,21 @@ export default {
         }
       }
     },
+
+    jumpToNextDiscussion() {
+      this.handleDiscussionJump(this.nextUnresolvedDiscussionId);
+    },
+
+    jumpToPreviousDiscussion() {
+      this.handleDiscussionJump(this.previousUnresolvedDiscussionId);
+    },
+
+    handleDiscussionJump(fn) {
+      const isDiffView = window.mrTabs.currentAction === 'diffs';
+      const targetId = fn(this.currentDiscussionId, isDiffView);
+      const discussion = this.getDiscussion(targetId);
+      this.jumpToDiscussion(discussion);
+      this.setCurrentDiscussionId(targetId);
+    },
   },
 };

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

@@ -506,5 +506,8 @@ export const fetchDescriptionVersion = (_, { endpoint, startingVersion }) => {
     });
 };
 
+export const setCurrentDiscussionId = ({ commit }, discussionId) =>
+  commit(types.SET_CURRENT_DISCUSSION_ID, discussionId);
+
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};

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

@@ -59,7 +59,6 @@ export const getDiscussionLastNote = state => discussion =>
 
 export const unresolvedDiscussionsCount = state => state.unresolvedDiscussionsCount;
 export const resolvableDiscussionsCount = state => state.resolvableDiscussionsCount;
-export const hasUnresolvedDiscussions = state => state.hasUnresolvedDiscussions;
 
 export const showJumpToNextDiscussion = (state, getters) => (mode = 'discussion') => {
   const orderedDiffs =

app/assets/javascripts/notes/stores/modules/index.js

@@ -8,6 +8,7 @@ export default () => ({
     convertedDisscussionIds: [],
     targetNoteHash: null,
     lastFetchedAt: null,
+    currentDiscussionId: null,
 
     // View layer
     isToggleStateButtonLoading: false,
@@ -26,7 +27,6 @@ export default () => ({
     commentsDisabled: false,
     resolvableDiscussionsCount: 0,
     unresolvedDiscussionsCount: 0,
-    hasUnresolvedDiscussions: false,
   },
   actions,
   getters,

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

@@ -25,6 +25,7 @@ export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION';
 export const EXPAND_DISCUSSION = 'EXPAND_DISCUSSION';
 export const TOGGLE_DISCUSSION = 'TOGGLE_DISCUSSION';
 export const UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS = 'UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS';
+export const SET_CURRENT_DISCUSSION_ID = 'SET_CURRENT_DISCUSSION_ID';
 
 // Issue
 export const CLOSE_ISSUE = 'CLOSE_ISSUE';

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

@@ -267,7 +267,6 @@ export default {
         discussion.resolvable &&
         discussion.notes.some(note => note.resolvable && !note.resolved),
     ).length;
-    state.hasUnresolvedDiscussions = state.unresolvedDiscussionsCount > 1;
   },
 
   [types.CONVERT_TO_DISCUSSION](state, discussionId) {
@@ -281,4 +280,8 @@ export default {
     convertedDisscussionIds.splice(convertedDisscussionIds.indexOf(discussionId), 1);
     Object.assign(state, { convertedDisscussionIds });
   },
+
+  [types.SET_CURRENT_DISCUSSION_ID](state, discussionId) {
+    state.currentDiscussionId = discussionId;
+  },
 };

changelogs/unreleased/jdb-cycle-unresolved-threads.yml

+---
+title: Cycle unresolved threads
+merge_request: 23123
+author:
+type: changed

doc/user/discussions/index.md

@@ -89,7 +89,7 @@ When a merge request has a large number of comments it can be difficult to track
 what remains unresolved. You can jump between unresolved threads with the
 Jump button next to the Reply field on a thread.
 
-You can also jump to the first unresolved thread from the button next to the
+You can also jump to the next unresolved thread from the button next to the
 resolved threads tracker.
 
 You can also use keyboard shortcuts to navigate among threads:

locale/gitlab.pot

@@ -10770,10 +10770,10 @@ msgstr ""
 msgid "July"
 msgstr ""
 
-msgid "Jump to first unresolved discussion"
+msgid "Jump to first unresolved thread"
 msgstr ""
 
-msgid "Jump to next unresolved discussion"
+msgid "Jump to next unresolved thread"
 msgstr ""
 
 msgid "Jun"
@@ -11787,7 +11787,7 @@ msgstr ""
 msgid "MergeRequests|Failed to squash. Should be done manually."
 msgstr ""
 
-msgid "MergeRequests|Jump to next unresolved discussion"
+msgid "MergeRequests|Jump to next unresolved thread"
 msgstr ""
 
 msgid "MergeRequests|Reply..."

spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb

@@ -368,16 +368,6 @@ describe 'Merge request > User resolves diff notes and threads', :js do
         end
       end
 
-      it 'shows jump to next discussion button on all discussions' 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(1)
-      end
-
       it 'displays next thread even if hidden' do
         page.all('.note-discussion', count: 2).each do |discussion|
           page.within discussion do

spec/frontend/notes/components/__snapshots__/discussion_jump_to_next_button_spec.js.snap

@@ -7,7 +7,7 @@ exports[`JumpToNextDiscussionButton matches the snapshot 1`] = `
 >
   <button
     class="btn btn-default discussion-next-btn"
-    title="Jump to next unresolved discussion"
+    title="Jump to next unresolved thread"
   >
     <icon-stub
       name="comment-next"

spec/frontend/notes/components/discussion_actions_spec.js

@@ -120,14 +120,5 @@ describe('DiscussionActions', () => {
         .trigger('click');
       expect(wrapper.vm.$emit).toHaveBeenCalledWith('resolve');
     });
-
-    it('emits jumpToNextDiscussion event when clicking on jump to next discussion button', () => {
-      jest.spyOn(wrapper.vm, '$emit');
-      wrapper
-        .find(JumpToNextDiscussionButton)
-        .find('button')
-        .trigger('click');
-      expect(wrapper.vm.$emit).toHaveBeenCalledWith('jumpToNextDiscussion');
-    });
   });
 });

spec/frontend/notes/components/discussion_jump_to_next_button_spec.js

@@ -15,15 +15,4 @@ describe('JumpToNextDiscussionButton', () => {
   it('matches the snapshot', () => {
     expect(wrapper.vm.$el).toMatchSnapshot();
   });
-
-  it('emits onClick event on button click', () => {
-    const button = wrapper.find({ ref: 'button' });
-
-    button.trigger('click');
-
-    return wrapper.vm.$nextTick().then(() => {
-      expect(wrapper.emitted().onClick).toBeTruthy();
-      expect(wrapper.emitted().onClick.length).toBe(1);
-    });
-  });
 });

spec/frontend/notes/components/discussion_keyboard_navigator_spec.js

@@ -53,13 +53,15 @@ describe('notes/components/discussion_keyboard_navigator', () => {
   });
 
   describe.each`
-    isDiffView | expectedNextId  | expectedPrevId
-    ${true}    | ${NEXT_DIFF_ID} | ${PREV_DIFF_ID}
-    ${false}   | ${NEXT_ID}      | ${PREV_ID}
-  `('when isDiffView is $isDiffView', ({ isDiffView, expectedNextId, expectedPrevId }) => {
+    currentAction | expectedNextId  | expectedPrevId
+    ${'diffs'}    | ${NEXT_DIFF_ID} | ${PREV_DIFF_ID}
+    ${'show'}     | ${NEXT_ID}      | ${PREV_ID}
+  `('when isDiffView is $isDiffView', ({ currentAction, expectedNextId, expectedPrevId }) => {
     beforeEach(() => {
-      createComponent({ propsData: { isDiffView } });
+      window.mrTabs = { currentAction };
+      createComponent();
     });
+    afterEach(() => delete window.mrTabs);
 
     it('calls jumpToNextDiscussion when pressing `n`', () => {
       Mousetrap.trigger('n');

spec/frontend/notes/stores/mutation_spec.js

@@ -501,7 +501,6 @@ describe('Notes Store mutations', () => {
         expect.objectContaining({
           resolvableDiscussionsCount: 1,
           unresolvedDiscussionsCount: 1,
-          hasUnresolvedDiscussions: false,
         }),
       );
     });
@@ -538,7 +537,6 @@ describe('Notes Store mutations', () => {
         expect.objectContaining({
           resolvableDiscussionsCount: 4,
           unresolvedDiscussionsCount: 2,
-          hasUnresolvedDiscussions: true,
         }),
       );
     });

spec/javascripts/notes/components/discussion_counter_spec.js

@@ -7,6 +7,7 @@ import { noteableDataMock, discussionMock, notesDataMock } from '../mock_data';
 describe('DiscussionCounter component', () => {
   let store;
   let vm;
+  const notes = { currentDiscussionId: null };
 
   beforeEach(() => {
     window.mrTabs = {};
@@ -25,7 +26,7 @@ describe('DiscussionCounter component', () => {
   });
 
   describe('methods', () => {
-    describe('jumpToFirstUnresolvedDiscussion', () => {
+    describe('jumpToNextDiscussion', () => {
       it('expands unresolved discussion', () => {
         window.mrTabs.currentAction = 'show';
 
@@ -48,13 +49,14 @@ describe('DiscussionCounter component', () => {
         store.replaceState({
           ...store.state,
           discussions,
+          notes,
         });
-        vm.jumpToFirstUnresolvedDiscussion();
+        vm.jumpToNextDiscussion();
 
         expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: firstDiscussionId });
       });
 
-      it('jumps to first unresolved discussion from diff tab if all diff discussions are resolved', () => {
+      it('jumps to next unresolved discussion from diff tab if all diff discussions are resolved', () => {
         window.mrTabs.currentAction = 'diff';
         spyOn(vm, 'switchToDiscussionsTabAndJumpTo').and.stub();
 
@@ -77,8 +79,9 @@ describe('DiscussionCounter component', () => {
         store.replaceState({
           ...store.state,
           discussions,
+          notes,
         });
-        vm.jumpToFirstUnresolvedDiscussion();
+        vm.jumpToNextDiscussion();
 
         expect(vm.switchToDiscussionsTabAndJumpTo).toHaveBeenCalledWith(unresolvedId);
       });

spec/javascripts/notes/components/noteable_discussion_spec.js

@@ -101,37 +101,6 @@ describe('noteable_discussion component', () => {
     });
   });
 
-  describe('methods', () => {
-    describe('jumpToNextDiscussion', () => {
-      it('expands next unresolved thread', 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)
-        store.dispatch('setInitialNotes', [discussionMock, discussion2]);
-        window.mrTabs.currentAction = 'show';
-
-        wrapper.vm
-          .$nextTick()
-          .then(() => {
-            spyOn(wrapper.vm, 'expandDiscussion').and.stub();
-
-            const nextDiscussionId = discussion2.id;
-
-            setFixtures(`<div class="discussion" data-discussion-id="${nextDiscussionId}"></div>`);
-
-            wrapper.vm.jumpToNextDiscussion();
-
-            expect(wrapper.vm.expandDiscussion).toHaveBeenCalledWith({
-              discussionId: nextDiscussionId,
-            });
-          })
-          .then(done)
-          .catch(done.fail);
-      });
-    });
-  });
-
   describe('for resolved thread', () => {
     beforeEach(() => {
       const discussion = getJSONFixture(discussionWithTwoUnresolvedNotes)[0];