From 1ba1348c8d9d4b632346e4098f97b3f86cd88457 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 22 Jul 2020 15:19:20 -0600 Subject: [PATCH 1/3] Jump to first unresolved thread for the MR widget - In the discussion navigation mixin: add the actual behavior along with all of the other navigation behaviors - In the discussion keyboard navigator component, adds the event hub listener that triggers the behavior - In the UI component: - Rewords - Adds buttons - Triggers event hub event - Adds CSS class to wrap at small sizes - Updates translations --- .../discussion_keyboard_navigator.vue | 6 +++ .../notes/mixins/discussion_navigation.js | 8 ++++ .../states/unresolved_discussions.vue | 37 ++++++++++++++----- locale/gitlab.pot | 15 +++++--- 4 files changed, 51 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/notes/components/discussion_keyboard_navigator.vue b/app/assets/javascripts/notes/components/discussion_keyboard_navigator.vue index 2dc222d08f91dc..facc53e27a6689 100644 --- a/app/assets/javascripts/notes/components/discussion_keyboard_navigator.vue +++ b/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; diff --git a/app/assets/javascripts/notes/mixins/discussion_navigation.js b/app/assets/javascripts/notes/mixins/discussion_navigation.js index 889883a23d0721..d65679d8f94bf9 100644 --- a/app/assets/javascripts/notes/mixins/discussion_navigation.js +++ b/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 diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/unresolved_discussions.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/unresolved_discussions.vue index d4a5fdb4b97068..78e69b9ff9b6a3 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/unresolved_discussions.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/unresolved_discussions.vue @@ -1,10 +1,13 @@ diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 7d79e514ff8483..ca515b98bc75e4 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -28418,6 +28418,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 "" @@ -28442,9 +28445,6 @@ msgstr "" msgid "mrWidget|Closes" msgstr "" -msgid "mrWidget|Create an issue to resolve them later" -msgstr "" - msgid "mrWidget|Delete source branch" msgstr "" @@ -28478,6 +28478,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 "" @@ -28541,6 +28544,9 @@ msgstr "" msgid "mrWidget|Request to merge" msgstr "" +msgid "mrWidget|Resolve all threads in new issue" +msgstr "" + msgid "mrWidget|Resolve conflicts" msgstr "" @@ -28592,9 +28598,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 "" -- GitLab From 6a4dd162e1d6bb63955bdf2f3a9ba39322e75ff5 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 22 Jul 2020 15:29:48 -0600 Subject: [PATCH 2/3] Test new interactive jump to first unresolved thread button - Updates the RSpec tests with the new UI text - Tests the Discussion Navigation for the new behavior - Tests the unresolved discussions component to ensure it's triggering the correct hub event --- ...e_for_discussions_in_merge_request_spec.rb | 23 ++++--- ...epending_on_unresolved_discussions_spec.rb | 2 +- .../discussion_keyboard_navigator_spec.js | 32 +++++++++- .../mixins/discussion_navigation_spec.js | 29 +++++++++ .../mr_widget_unresolved_discussions_spec.js | 64 +++++++++++++------ 5 files changed, 119 insertions(+), 31 deletions(-) diff --git a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb index 6fc648954b464e..12682905559971 100644 --- a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb +++ b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb @@ -8,10 +8,14 @@ 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 @@ def resolve_all_discussions_link_selector 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 @@ def resolve_all_discussions_link_selector 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 @@ def resolve_all_discussions_link_selector 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 @@ def resolve_all_discussions_link_selector 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' diff --git a/spec/features/merge_request/user_sees_merge_button_depending_on_unresolved_discussions_spec.rb b/spec/features/merge_request/user_sees_merge_button_depending_on_unresolved_discussions_spec.rb index cae04dd1693a79..ac38b2b854c0b1 100644 --- a/spec/features/merge_request/user_sees_merge_button_depending_on_unresolved_discussions_spec.rb +++ b/spec/features/merge_request/user_sees_merge_button_depending_on_unresolved_discussions_spec.rb @@ -21,7 +21,7 @@ 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 diff --git a/spec/frontend/notes/components/discussion_keyboard_navigator_spec.js b/spec/frontend/notes/components/discussion_keyboard_navigator_spec.js index e932133b869912..fd0383f3b9d24a 100644 --- a/spec/frontend/notes/components/discussion_keyboard_navigator_spec.js +++ b/spec/frontend/notes/components/discussion_keyboard_navigator_spec.js @@ -1,7 +1,9 @@ /* 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'); diff --git a/spec/frontend/notes/mixins/discussion_navigation_spec.js b/spec/frontend/notes/mixins/discussion_navigation_spec.js index ecff95b6fe062d..32c6cfb110edfd 100644 --- a/spec/frontend/notes/mixins/discussion_navigation_spec.js +++ b/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() }; diff --git a/spec/frontend/vue_mr_widget/components/states/mr_widget_unresolved_discussions_spec.js b/spec/frontend/vue_mr_widget/components/states/mr_widget_unresolved_discussions_spec.js index 33e52f4fd36193..d80dc60ee8d7bd 100644 --- a/spec/frontend/vue_mr_widget/components/states/mr_widget_unresolved_discussions_spec.js +++ b/spec/frontend/vue_mr_widget/components/states/mr_widget_unresolved_discussions_spec.js @@ -1,46 +1,68 @@ -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); }); }); }); -- GitLab From f5c96d800970aa733f0dc3ad06249cfc2d5403ba Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 21 Jul 2020 01:10:45 -0600 Subject: [PATCH 3/3] Add a changelog for the new jump to first thread button --- .../unreleased/resolve-threads-jump-to-thread-button.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelogs/unreleased/resolve-threads-jump-to-thread-button.yml diff --git a/changelogs/unreleased/resolve-threads-jump-to-thread-button.yml b/changelogs/unreleased/resolve-threads-jump-to-thread-button.yml new file mode 100644 index 00000000000000..cad7f5a79eb48f --- /dev/null +++ b/changelogs/unreleased/resolve-threads-jump-to-thread-button.yml @@ -0,0 +1,6 @@ +--- +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 -- GitLab