Merge branch...

Merge branch '215174-in-a-mr-please-resolve-these-threads-should-be-a-hyperlink-pointing-to-the-threads-to-be' into 'master'

Reword unresolved discussions widget and make 'resolve threads' interactive

See merge request gitlab-org/gitlab!36164

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

@@ -2,9 +2,13 @@
 /* global Mousetrap */
 import 'mousetrap';
 import discussionNavigation from '~/notes/mixins/discussion_navigation';
+import eventHub from '~/notes/event_hub';
 
 export default {
   mixins: [discussionNavigation],
+  created() {
+    eventHub.$on('jumpToFirstUnresolvedDiscussion', this.jumpToFirstUnresolvedDiscussion);
+  },
   mounted() {
     Mousetrap.bind('n', this.jumpToNextDiscussion);
     Mousetrap.bind('p', this.jumpToPreviousDiscussion);
@@ -12,6 +16,8 @@ export default {
   beforeDestroy() {
     Mousetrap.unbind('n');
     Mousetrap.unbind('p');
+
+    eventHub.$off('jumpToFirstUnresolvedDiscussion', this.jumpToFirstUnresolvedDiscussion);
   },
   render() {
     return this.$slots.default;

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

@@ -113,6 +113,14 @@ export default {
       handleDiscussionJump(this, this.previousUnresolvedDiscussionId);
     },
 
+    jumpToFirstUnresolvedDiscussion() {
+      this.setCurrentDiscussionId(null)
+        .then(() => {
+          this.jumpToNextDiscussion();
+        })
+        .catch(() => {});
+    },
+
     /**
      * Go to the next discussion from the given discussionId
      * @param {String} discussionId The id we are jumping from

app/assets/javascripts/vue_merge_request_widget/components/states/unresolved_discussions.vue

 <script>
+import { GlButton } from '@gitlab/ui';
 import statusIcon from '../mr_widget_status_icon.vue';
+import notesEventHub from '~/notes/event_hub';
 
 export default {
   name: 'UnresolvedDiscussions',
   components: {
     statusIcon,
+    GlButton,
   },
   props: {
     mr: {
@@ -12,23 +15,39 @@ export default {
       required: true,
     },
   },
+  methods: {
+    jumpToFirstUnresolvedDiscussion() {
+      notesEventHub.$emit('jumpToFirstUnresolvedDiscussion');
+    },
+  },
 };
 </script>
 
 <template>
-  <div class="mr-widget-body media">
+  <div class="mr-widget-body media gl-flex-wrap">
     <status-icon :show-disabled-button="true" status="warning" />
-    <div class="media-body space-children">
-      <span class="bold">
-        {{ s__('mrWidget|There are unresolved threads. Please resolve these threads') }}
-      </span>
-      <a
+    <div class="media-body">
+      <span class="gl-ml-3 gl-font-weight-bold gl-display-block gl-w-100">{{
+        s__('mrWidget|Before this can be merged, one or more threads must be resolved.')
+      }}</span>
+      <gl-button
+        data-testid="jump-to-first"
+        class="gl-ml-3"
+        size="small"
+        icon="comment-next"
+        @click="jumpToFirstUnresolvedDiscussion"
+      >
+        {{ s__('mrWidget|Jump to first unresolved thread') }}
+      </gl-button>
+      <gl-button
         v-if="mr.createIssueToResolveDiscussionsPath"
         :href="mr.createIssueToResolveDiscussionsPath"
-        class="btn btn-default btn-sm js-create-issue"
+        class="js-create-issue gl-ml-3"
+        size="small"
+        icon="issue-new"
       >
-        {{ s__('mrWidget|Create an issue to resolve them later') }}
-      </a>
+        {{ s__('mrWidget|Resolve all threads in new issue') }}
+      </gl-button>
     </div>
   </div>
 </template>

changelogs/unreleased/resolve-threads-jump-to-thread-button.yml

+---
+title: Prompt to resolve unresolved threads on an MR is a button that jumps to the
+  first such thread
+merge_request: 36164
+author:
+type: added

locale/gitlab.pot

@@ -28451,6 +28451,9 @@ msgstr ""
 msgid "mrWidget|Are you adding technical debt or code vulnerabilities?"
 msgstr ""
 
+msgid "mrWidget|Before this can be merged, one or more threads must be resolved."
+msgstr ""
+
 msgid "mrWidget|Cancel automatic merge"
 msgstr ""
 
@@ -28475,9 +28478,6 @@ msgstr ""
 msgid "mrWidget|Closes"
 msgstr ""
 
-msgid "mrWidget|Create an issue to resolve them later"
-msgstr ""
-
 msgid "mrWidget|Delete source branch"
 msgstr ""
 
@@ -28511,6 +28511,9 @@ msgstr ""
 msgid "mrWidget|In the merge train at position %{mergeTrainPosition}"
 msgstr ""
 
+msgid "mrWidget|Jump to first unresolved thread"
+msgstr ""
+
 msgid "mrWidget|Loading deployment statistics"
 msgstr ""
 
@@ -28574,6 +28577,9 @@ msgstr ""
 msgid "mrWidget|Request to merge"
 msgstr ""
 
+msgid "mrWidget|Resolve all threads in new issue"
+msgstr ""
+
 msgid "mrWidget|Resolve conflicts"
 msgstr ""
 
@@ -28625,9 +28631,6 @@ msgstr ""
 msgid "mrWidget|There are merge conflicts"
 msgstr ""
 
-msgid "mrWidget|There are unresolved threads. Please resolve these threads"
-msgstr ""
-
 msgid "mrWidget|This feature merges changes from the target branch to the source branch. You cannot use this feature since the source branch is protected."
 msgstr ""
 

spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb

@@ -8,10 +8,14 @@ RSpec.describe 'Resolving all open threads in a merge request from an issue', :j
   let(:merge_request) { create(:merge_request, source_project: project) }
   let!(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion }
 
-  def resolve_all_discussions_link_selector
-    text = "Resolve all threads in new issue"
+  def resolve_all_discussions_link_selector(title: "")
     url = new_project_issue_path(project, merge_request_to_resolve_discussions_of: merge_request.iid)
-    %Q{a[title="#{text}"][href="#{url}"]}
+
+    if title.empty?
+      %Q{a[href="#{url}"]}
+    else
+      %Q{a[title="#{title}"][href="#{url}"]}
+    end
   end
 
   describe 'as a user with access to the project' do
@@ -23,7 +27,7 @@ RSpec.describe 'Resolving all open threads in a merge request from an issue', :j
 
     it 'shows a button to resolve all threads by creating a new issue' do
       within('.line-resolve-all-container') do
-        expect(page).to have_selector resolve_all_discussions_link_selector
+        expect(page).to have_selector resolve_all_discussions_link_selector( title: "Resolve all threads in new issue" )
       end
     end
 
@@ -34,6 +38,7 @@ RSpec.describe 'Resolving all open threads in a merge request from an issue', :j
 
       it 'hides the link for creating a new issue' do
         expect(page).not_to have_selector resolve_all_discussions_link_selector
+        expect(page).not_to have_content "Resolve all threads in new issue"
       end
     end
 
@@ -57,7 +62,7 @@ RSpec.describe 'Resolving all open threads in a merge request from an issue', :j
         end
 
         it 'does not show a link to create a new issue' do
-          expect(page).not_to have_link 'Create an issue to resolve them later'
+          expect(page).not_to have_link 'Resolve all threads in new issue'
         end
       end
 
@@ -67,18 +72,20 @@ RSpec.describe 'Resolving all open threads in a merge request from an issue', :j
         end
 
         it 'shows a warning that the merge request contains unresolved threads' do
-          expect(page).to have_content 'There are unresolved threads.'
+          expect(page).to have_content 'Before this can be merged,'
         end
 
         it 'has a link to resolve all threads by creating an issue' do
           page.within '.mr-widget-body' do
-            expect(page).to have_link 'Create an issue to resolve them later', href: new_project_issue_path(project, merge_request_to_resolve_discussions_of: merge_request.iid)
+            expect(page).to have_link 'Resolve all threads in new issue', href: new_project_issue_path(project, merge_request_to_resolve_discussions_of: merge_request.iid)
           end
         end
 
         context 'creating an issue for threads' do
           before do
-            page.click_link 'Create an issue to resolve them later', href: new_project_issue_path(project, merge_request_to_resolve_discussions_of: merge_request.iid)
+            page.within '.mr-widget-body' do
+              page.click_link 'Resolve all threads in new issue', href: new_project_issue_path(project, merge_request_to_resolve_discussions_of: merge_request.iid)
+            end
           end
 
           it_behaves_like 'creating an issue for a thread'

spec/features/merge_request/user_sees_merge_button_depending_on_unresolved_discussions_spec.rb

@@ -21,7 +21,7 @@ RSpec.describe 'Merge request > User sees merge button depending on unresolved t
     context 'with unresolved threads' do
       it 'does not allow to merge' do
         expect(page).not_to have_button 'Merge'
-        expect(page).to have_content('There are unresolved threads.')
+        expect(page).to have_content('Before this can be merged,')
       end
     end
 

spec/frontend/notes/components/discussion_keyboard_navigator_spec.js

 /* global Mousetrap */
 import 'mousetrap';
+import Vue from 'vue';
 import { shallowMount, createLocalVue } from '@vue/test-utils';
 import DiscussionKeyboardNavigator from '~/notes/components/discussion_keyboard_navigator.vue';
+import eventHub from '~/notes/event_hub';
 
 describe('notes/components/discussion_keyboard_navigator', () => {
   const localVue = createLocalVue();
@@ -29,10 +31,29 @@ describe('notes/components/discussion_keyboard_navigator', () => {
   });
 
   afterEach(() => {
-    wrapper.destroy();
+    if (wrapper) {
+      wrapper.destroy();
+    }
     wrapper = null;
   });
 
+  describe('on create', () => {
+    let onSpy;
+    let vm;
+
+    beforeEach(() => {
+      onSpy = jest.spyOn(eventHub, '$on');
+      vm = new (Vue.extend(DiscussionKeyboardNavigator))();
+    });
+
+    it('listens for jumpToFirstUnresolvedDiscussion events', () => {
+      expect(onSpy).toHaveBeenCalledWith(
+        'jumpToFirstUnresolvedDiscussion',
+        vm.jumpToFirstUnresolvedDiscussion,
+      );
+    });
+  });
+
   describe('on mount', () => {
     beforeEach(() => {
       createComponent();
@@ -52,11 +73,16 @@ describe('notes/components/discussion_keyboard_navigator', () => {
   });
 
   describe('on destroy', () => {
+    let jumpFn;
+
     beforeEach(() => {
       jest.spyOn(Mousetrap, 'unbind');
+      jest.spyOn(eventHub, '$off');
 
       createComponent();
 
+      jumpFn = wrapper.vm.jumpToFirstUnresolvedDiscussion;
+
       wrapper.destroy();
     });
 
@@ -65,6 +91,10 @@ describe('notes/components/discussion_keyboard_navigator', () => {
       expect(Mousetrap.unbind).toHaveBeenCalledWith('p');
     });
 
+    it('unbinds event hub listeners', () => {
+      expect(eventHub.$off).toHaveBeenCalledWith('jumpToFirstUnresolvedDiscussion', jumpFn);
+    });
+
     it('does not call jumpToNextDiscussion when pressing `n`', () => {
       Mousetrap.trigger('n');
 

spec/frontend/notes/mixins/discussion_navigation_spec.js

@@ -66,6 +66,35 @@ describe('Discussion navigation mixin', () => {
   const findDiscussion = (selector, id) =>
     document.querySelector(`${selector}[data-discussion-id="${id}"]`);
 
+  describe('jumpToFirstUnresolvedDiscussion method', () => {
+    let vm;
+
+    beforeEach(() => {
+      createComponent();
+
+      ({ vm } = wrapper);
+
+      jest.spyOn(store, 'dispatch');
+      jest.spyOn(vm, 'jumpToNextDiscussion');
+    });
+
+    it('triggers the setCurrentDiscussionId action with null as the value', () => {
+      vm.jumpToFirstUnresolvedDiscussion();
+
+      expect(store.dispatch).toHaveBeenCalledWith('setCurrentDiscussionId', null);
+    });
+
+    it('triggers the jumpToNextDiscussion action when the previous store action succeeds', () => {
+      store.dispatch.mockResolvedValue();
+
+      vm.jumpToFirstUnresolvedDiscussion();
+
+      return vm.$nextTick().then(() => {
+        expect(vm.jumpToNextDiscussion).toHaveBeenCalled();
+      });
+    });
+  });
+
   describe('cycle through discussions', () => {
     beforeEach(() => {
       window.mrTabs = { eventHub: createEventHub(), tabShown: jest.fn() };

spec/frontend/vue_mr_widget/components/states/mr_widget_unresolved_discussions_spec.js

-import Vue from 'vue';
-import mountComponent from 'helpers/vue_mount_component_helper';
+import { mount } from '@vue/test-utils';
 import UnresolvedDiscussions from '~/vue_merge_request_widget/components/states/unresolved_discussions.vue';
+import notesEventHub from '~/notes/event_hub';
 import { TEST_HOST } from 'helpers/test_constants';
 
+function createComponent({ path = '' } = {}) {
+  return mount(UnresolvedDiscussions, {
+    propsData: {
+      mr: {
+        createIssueToResolveDiscussionsPath: path,
+      },
+    },
+  });
+}
+
 describe('UnresolvedDiscussions', () => {
-  const Component = Vue.extend(UnresolvedDiscussions);
-  let vm;
+  let wrapper;
+
+  beforeEach(() => {
+    wrapper = createComponent();
+  });
 
   afterEach(() => {
-    vm.$destroy();
+    wrapper.destroy();
+  });
+
+  it('triggers the correct notes event when the jump to first unresolved discussion button is clicked', () => {
+    jest.spyOn(notesEventHub, '$emit');
+
+    wrapper.find('[data-testid="jump-to-first"]').trigger('click');
+
+    expect(notesEventHub.$emit).toHaveBeenCalledWith('jumpToFirstUnresolvedDiscussion');
   });
 
   describe('with threads path', () => {
     beforeEach(() => {
-      vm = mountComponent(Component, {
-        mr: {
-          createIssueToResolveDiscussionsPath: TEST_HOST,
-        },
-      });
+      wrapper = createComponent({ path: TEST_HOST });
+    });
+
+    afterEach(() => {
+      wrapper.destroy();
     });
 
     it('should have correct elements', () => {
-      expect(vm.$el.innerText).toContain(
-        'There are unresolved threads. Please resolve these threads',
+      expect(wrapper.element.innerText).toContain(
+        `Before this can be merged, one or more threads must be resolved.`,
       );
 
-      expect(vm.$el.innerText).toContain('Create an issue to resolve them later');
-      expect(vm.$el.querySelector('.js-create-issue').getAttribute('href')).toEqual(TEST_HOST);
+      expect(wrapper.element.innerText).toContain('Jump to first unresolved thread');
+      expect(wrapper.element.innerText).toContain('Resolve all threads in new issue');
+      expect(wrapper.element.querySelector('.js-create-issue').getAttribute('href')).toEqual(
+        TEST_HOST,
+      );
     });
   });
 
   describe('without threads path', () => {
-    beforeEach(() => {
-      vm = mountComponent(Component, { mr: {} });
-    });
-
     it('should not show create issue link if user cannot create issue', () => {
-      expect(vm.$el.innerText).toContain(
-        'There are unresolved threads. Please resolve these threads',
+      expect(wrapper.element.innerText).toContain(
+        `Before this can be merged, one or more threads must be resolved.`,
       );
 
-      expect(vm.$el.querySelector('.js-create-issue')).toEqual(null);
+      expect(wrapper.element.innerText).toContain('Jump to first unresolved thread');
+      expect(wrapper.element.innerText).not.toContain('Resolve all threads in new issue');
+      expect(wrapper.element.querySelector('.js-create-issue')).toEqual(null);
     });
   });
 });