Commit 169e409c authored by Justin Boyson's avatar Justin Boyson Committed by Paul Slaughter

Fix Jump to next unresolved thread

Use relative discussion id when clicking next button from within
a discussion.
parent 8076bb38
...@@ -73,7 +73,7 @@ export default { ...@@ -73,7 +73,7 @@ export default {
v-if="discussion.resolvable && shouldShowJumpToNextDiscussion" v-if="discussion.resolvable && shouldShowJumpToNextDiscussion"
class="btn-group discussion-actions ml-sm-2" class="btn-group discussion-actions ml-sm-2"
> >
<jump-to-next-discussion-button /> <jump-to-next-discussion-button :from-discussion-id="discussion.id" />
</div> </div>
</div> </div>
</template> </template>
...@@ -39,7 +39,11 @@ export default { ...@@ -39,7 +39,11 @@ export default {
</script> </script>
<template> <template>
<div v-if="resolvableDiscussionsCount > 0" class="line-resolve-all-container full-width-mobile"> <div
v-if="resolvableDiscussionsCount > 0"
ref="discussionCounter"
class="line-resolve-all-container full-width-mobile"
>
<div class="full-width-mobile d-flex d-sm-block"> <div class="full-width-mobile d-flex d-sm-block">
<div :class="{ 'has-next-btn': hasNextButton }" class="line-resolve-all"> <div :class="{ 'has-next-btn': hasNextButton }" class="line-resolve-all">
<span <span
......
...@@ -12,6 +12,12 @@ export default { ...@@ -12,6 +12,12 @@ export default {
GlTooltip: GlTooltipDirective, GlTooltip: GlTooltipDirective,
}, },
mixins: [discussionNavigation], mixins: [discussionNavigation],
props: {
fromDiscussionId: {
type: String,
required: true,
},
},
}; };
</script> </script>
...@@ -22,7 +28,7 @@ export default { ...@@ -22,7 +28,7 @@ export default {
v-gl-tooltip v-gl-tooltip
class="btn btn-default discussion-next-btn" class="btn btn-default discussion-next-btn"
:title="s__('MergeRequests|Jump to next unresolved thread')" :title="s__('MergeRequests|Jump to next unresolved thread')"
@click="jumpToNextDiscussion" @click="jumpToNextRelativeDiscussion(fromDiscussionId)"
> >
<icon name="comment-next" /> <icon name="comment-next" />
</button> </button>
......
...@@ -2,90 +2,114 @@ import { mapGetters, mapActions, mapState } from 'vuex'; ...@@ -2,90 +2,114 @@ import { mapGetters, mapActions, mapState } from 'vuex';
import { scrollToElement } from '~/lib/utils/common_utils'; import { scrollToElement } from '~/lib/utils/common_utils';
import eventHub from '../../notes/event_hub'; import eventHub from '../../notes/event_hub';
export default { /**
computed: { * @param {string} selector
...mapGetters([ * @returns {boolean}
'nextUnresolvedDiscussionId', */
'previousUnresolvedDiscussionId', function scrollTo(selector) {
'getDiscussion',
]),
...mapState({
currentDiscussionId: state => state.notes.currentDiscussionId,
}),
},
methods: {
...mapActions(['expandDiscussion', 'setCurrentDiscussionId']),
diffsJump(id) {
const selector = `ul.notes[data-discussion-id="${id}"]`;
eventHub.$once('scrollToDiscussion', () => {
const el = document.querySelector(selector); const el = document.querySelector(selector);
if (el) { if (el) {
scrollToElement(el); scrollToElement(el);
return true; return true;
} }
return false; return false;
}); }
this.expandDiscussion({ discussionId: id }); /**
}, * @param {object} self Component instance with mixin applied
discussionJump(id) { * @param {string} id Discussion id we are jumping to
*/
function diffsJump({ expandDiscussion }, id) {
const selector = `ul.notes[data-discussion-id="${id}"]`;
eventHub.$once('scrollToDiscussion', () => scrollTo(selector));
expandDiscussion({ discussionId: id });
}
/**
* @param {object} self Component instance with mixin applied
* @param {string} id Discussion id we are jumping to
* @returns {boolean}
*/
function discussionJump({ expandDiscussion }, id) {
const selector = `div.discussion[data-discussion-id="${id}"]`; const selector = `div.discussion[data-discussion-id="${id}"]`;
expandDiscussion({ discussionId: id });
const el = document.querySelector(selector); return scrollTo(selector);
}
this.expandDiscussion({ discussionId: id });
/**
if (el) { * @param {object} self Component instance with mixin applied
scrollToElement(el); * @param {string} id Discussion id we are jumping to
*/
return true; function switchToDiscussionsTabAndJumpTo(self, id) {
}
return false;
},
switchToDiscussionsTabAndJumpTo(id) {
window.mrTabs.eventHub.$once('MergeRequestTabChange', () => { window.mrTabs.eventHub.$once('MergeRequestTabChange', () => {
setTimeout(() => this.discussionJump(id), 0); setTimeout(() => discussionJump(self, id), 0);
}); });
window.mrTabs.tabShown('show'); window.mrTabs.tabShown('show');
}, }
jumpToDiscussion(discussion) { /**
* @param {object} self Component instance with mixin applied
* @param {object} discussion Discussion we are jumping to
*/
function jumpToDiscussion(self, discussion) {
const { id, diff_discussion: isDiffDiscussion } = discussion; const { id, diff_discussion: isDiffDiscussion } = discussion;
if (id) { if (id) {
const activeTab = window.mrTabs.currentAction; const activeTab = window.mrTabs.currentAction;
if (activeTab === 'diffs' && isDiffDiscussion) { if (activeTab === 'diffs' && isDiffDiscussion) {
this.diffsJump(id); diffsJump(self, id);
} else if (activeTab === 'show') { } else if (activeTab === 'show') {
this.discussionJump(id); discussionJump(self, id);
} else { } else {
this.switchToDiscussionsTabAndJumpTo(id); switchToDiscussionsTabAndJumpTo(self, id);
} }
} }
}
/**
* @param {object} self Component instance with mixin applied
* @param {function} fn Which function used to get the target discussion's id
* @param {string} [discussionId=this.currentDiscussionId] Current discussion id, will be null if discussions have not been traversed yet
*/
function handleDiscussionJump(self, fn, discussionId = self.currentDiscussionId) {
const isDiffView = window.mrTabs.currentAction === 'diffs';
const targetId = fn(discussionId, isDiffView);
const discussion = self.getDiscussion(targetId);
jumpToDiscussion(self, discussion);
self.setCurrentDiscussionId(targetId);
}
export default {
computed: {
...mapGetters([
'nextUnresolvedDiscussionId',
'previousUnresolvedDiscussionId',
'getDiscussion',
]),
...mapState({
currentDiscussionId: state => state.notes.currentDiscussionId,
}),
}, },
methods: {
...mapActions(['expandDiscussion', 'setCurrentDiscussionId']),
jumpToNextDiscussion() { jumpToNextDiscussion() {
this.handleDiscussionJump(this.nextUnresolvedDiscussionId); handleDiscussionJump(this, this.nextUnresolvedDiscussionId);
}, },
jumpToPreviousDiscussion() { jumpToPreviousDiscussion() {
this.handleDiscussionJump(this.previousUnresolvedDiscussionId); handleDiscussionJump(this, this.previousUnresolvedDiscussionId);
}, },
handleDiscussionJump(fn) { /**
const isDiffView = window.mrTabs.currentAction === 'diffs'; * Go to the next discussion from the given discussionId
const targetId = fn(this.currentDiscussionId, isDiffView); * @param {String} discussionId The id we are jumping from
const discussion = this.getDiscussion(targetId); */
this.jumpToDiscussion(discussion); jumpToNextRelativeDiscussion(discussionId) {
this.setCurrentDiscussionId(targetId); handleDiscussionJump(this, this.nextUnresolvedDiscussionId, discussionId);
}, },
}, },
}; };
---
title: Fix Jump to next unresolved thread
merge_request: 24728
author:
type: fixed
import Vuex from 'vuex';
import { shallowMount, createLocalVue } from '@vue/test-utils';
import notesModule from '~/notes/stores/modules';
import DiscussionCounter from '~/notes/components/discussion_counter.vue';
import { noteableDataMock, discussionMock, notesDataMock, userDataMock } from '../mock_data';
import * as types from '~/notes/stores/mutation_types';
describe('DiscussionCounter component', () => {
let store;
let wrapper;
const localVue = createLocalVue();
localVue.use(Vuex);
beforeEach(() => {
window.mrTabs = {};
const { state, getters, mutations, actions } = notesModule();
store = new Vuex.Store({
state: {
...state,
userData: userDataMock,
},
getters,
mutations,
actions,
});
store.dispatch('setNoteableData', {
...noteableDataMock,
create_issue_to_resolve_discussions_path: '/test',
});
store.dispatch('setNotesData', notesDataMock);
});
afterEach(() => {
wrapper.vm.$destroy();
wrapper = null;
});
describe('has no discussions', () => {
it('does not render', () => {
wrapper = shallowMount(DiscussionCounter, { store, localVue });
expect(wrapper.find({ ref: 'discussionCounter' }).exists()).toBe(false);
});
});
describe('has no resolvable discussions', () => {
it('does not render', () => {
store.commit(types.SET_INITIAL_DISCUSSIONS, [{ ...discussionMock, resolvable: false }]);
store.dispatch('updateResolvableDiscussionsCounts');
wrapper = shallowMount(DiscussionCounter, { store, localVue });
expect(wrapper.find({ ref: 'discussionCounter' }).exists()).toBe(false);
});
});
describe('has resolvable discussions', () => {
const updateStore = (note = {}) => {
discussionMock.notes[0] = { ...discussionMock.notes[0], ...note };
store.commit(types.SET_INITIAL_DISCUSSIONS, [discussionMock]);
store.dispatch('updateResolvableDiscussionsCounts');
};
afterEach(() => {
delete discussionMock.notes[0].resolvable;
delete discussionMock.notes[0].resolved;
});
it('renders', () => {
updateStore();
wrapper = shallowMount(DiscussionCounter, { store, localVue });
expect(wrapper.find({ ref: 'discussionCounter' }).exists()).toBe(true);
});
it.each`
title | resolved | hasNextBtn | isActive | icon | groupLength
${'hasNextButton'} | ${false} | ${true} | ${false} | ${'check-circle'} | ${2}
${'allResolved'} | ${true} | ${false} | ${true} | ${'check-circle-filled'} | ${0}
`('renders correctly if $title', ({ resolved, hasNextBtn, isActive, icon, groupLength }) => {
updateStore({ resolvable: true, resolved });
wrapper = shallowMount(DiscussionCounter, { store, localVue });
expect(wrapper.find(`.has-next-btn`).exists()).toBe(hasNextBtn);
expect(wrapper.find(`.is-active`).exists()).toBe(isActive);
expect(wrapper.find({ name: icon }).exists()).toBe(true);
expect(wrapper.findAll('[role="group"').length).toBe(groupLength);
});
});
});
...@@ -3,9 +3,12 @@ import JumpToNextDiscussionButton from '~/notes/components/discussion_jump_to_ne ...@@ -3,9 +3,12 @@ import JumpToNextDiscussionButton from '~/notes/components/discussion_jump_to_ne
describe('JumpToNextDiscussionButton', () => { describe('JumpToNextDiscussionButton', () => {
let wrapper; let wrapper;
const fromDiscussionId = 'abc123';
beforeEach(() => { beforeEach(() => {
wrapper = shallowMount(JumpToNextDiscussionButton); wrapper = shallowMount(JumpToNextDiscussionButton, {
propsData: { fromDiscussionId },
});
}); });
afterEach(() => { afterEach(() => {
...@@ -15,4 +18,11 @@ describe('JumpToNextDiscussionButton', () => { ...@@ -15,4 +18,11 @@ describe('JumpToNextDiscussionButton', () => {
it('matches the snapshot', () => { it('matches the snapshot', () => {
expect(wrapper.vm.$el).toMatchSnapshot(); expect(wrapper.vm.$el).toMatchSnapshot();
}); });
it('calls jumpToNextRelativeDiscussion when clicked', () => {
const jumpToNextRelativeDiscussion = jest.fn();
wrapper.setMethods({ jumpToNextRelativeDiscussion });
wrapper.find({ ref: 'button' }).trigger('click');
expect(jumpToNextRelativeDiscussion).toHaveBeenCalledWith(fromDiscussionId);
});
}); });
/* global Mousetrap */ /* global Mousetrap */
import 'mousetrap'; import 'mousetrap';
import { shallowMount, createLocalVue } from '@vue/test-utils'; import { shallowMount, createLocalVue } from '@vue/test-utils';
import Vuex from 'vuex';
import DiscussionKeyboardNavigator from '~/notes/components/discussion_keyboard_navigator.vue'; import DiscussionKeyboardNavigator from '~/notes/components/discussion_keyboard_navigator.vue';
import notesModule from '~/notes/stores/modules';
const localVue = createLocalVue();
localVue.use(Vuex);
const NEXT_ID = 'abc123';
const PREV_ID = 'def456';
const NEXT_DIFF_ID = 'abc123_diff';
const PREV_DIFF_ID = 'def456_diff';
describe('notes/components/discussion_keyboard_navigator', () => { describe('notes/components/discussion_keyboard_navigator', () => {
let storeOptions; const localVue = createLocalVue();
let wrapper;
let store;
const createComponent = (options = {}) => { let wrapper;
store = new Vuex.Store(storeOptions); let jumpToNextDiscussion;
let jumpToPreviousDiscussion;
const createComponent = () => {
wrapper = shallowMount(DiscussionKeyboardNavigator, { wrapper = shallowMount(DiscussionKeyboardNavigator, {
localVue, mixins: [
store, localVue.extend({
...options, methods: {
jumpToNextDiscussion,
jumpToPreviousDiscussion,
},
}),
],
}); });
wrapper.vm.jumpToDiscussion = jest.fn();
}; };
beforeEach(() => { beforeEach(() => {
const notes = notesModule(); jumpToNextDiscussion = jest.fn();
jumpToPreviousDiscussion = jest.fn();
notes.getters.nextUnresolvedDiscussionId = () => (currId, isDiff) =>
isDiff ? NEXT_DIFF_ID : NEXT_ID;
notes.getters.previousUnresolvedDiscussionId = () => (currId, isDiff) =>
isDiff ? PREV_DIFF_ID : PREV_ID;
notes.getters.getDiscussion = () => id => ({ id });
storeOptions = {
modules: {
notes,
},
};
}); });
afterEach(() => { afterEach(() => {
wrapper.destroy(); wrapper.destroy();
storeOptions = null; wrapper = null;
store = null;
}); });
describe.each` describe('on mount', () => {
currentAction | expectedNextId | expectedPrevId
${'diffs'} | ${NEXT_DIFF_ID} | ${PREV_DIFF_ID}
${'show'} | ${NEXT_ID} | ${PREV_ID}
`('when isDiffView is $isDiffView', ({ currentAction, expectedNextId, expectedPrevId }) => {
beforeEach(() => { beforeEach(() => {
window.mrTabs = { currentAction };
createComponent(); createComponent();
}); });
afterEach(() => delete window.mrTabs);
it('calls jumpToNextDiscussion when pressing `n`', () => { it('calls jumpToNextDiscussion when pressing `n`', () => {
Mousetrap.trigger('n'); Mousetrap.trigger('n');
expect(wrapper.vm.jumpToDiscussion).toHaveBeenCalledWith( expect(jumpToNextDiscussion).toHaveBeenCalled();
expect.objectContaining({ id: expectedNextId }),
);
expect(wrapper.vm.currentDiscussionId).toEqual(expectedNextId);
}); });
it('calls jumpToPreviousDiscussion when pressing `p`', () => { it('calls jumpToPreviousDiscussion when pressing `p`', () => {
Mousetrap.trigger('p'); Mousetrap.trigger('p');
expect(wrapper.vm.jumpToDiscussion).toHaveBeenCalledWith( expect(jumpToPreviousDiscussion).toHaveBeenCalled();
expect.objectContaining({ id: expectedPrevId }),
);
expect(wrapper.vm.currentDiscussionId).toEqual(expectedPrevId);
}); });
}); });
...@@ -99,13 +68,13 @@ describe('notes/components/discussion_keyboard_navigator', () => { ...@@ -99,13 +68,13 @@ describe('notes/components/discussion_keyboard_navigator', () => {
it('does not call jumpToNextDiscussion when pressing `n`', () => { it('does not call jumpToNextDiscussion when pressing `n`', () => {
Mousetrap.trigger('n'); Mousetrap.trigger('n');
expect(wrapper.vm.jumpToDiscussion).not.toHaveBeenCalled(); expect(jumpToNextDiscussion).not.toHaveBeenCalled();
}); });
it('does not call jumpToNextDiscussion when pressing `p`', () => { it('does not call jumpToNextDiscussion when pressing `p`', () => {
Mousetrap.trigger('p'); Mousetrap.trigger('p');
expect(wrapper.vm.jumpToDiscussion).not.toHaveBeenCalled(); expect(jumpToPreviousDiscussion).not.toHaveBeenCalled();
}); });
}); });
}); });
import Vuex from 'vuex';
import { shallowMount, createLocalVue } from '@vue/test-utils';
import * as utils from '~/lib/utils/common_utils';
import discussionNavigation from '~/notes/mixins/discussion_navigation';
import eventHub from '~/notes/event_hub';
import notesModule from '~/notes/stores/modules';
import { setHTMLFixture } from 'helpers/fixtures';
const discussion = (id, index) => ({
id,
resolvable: index % 2 === 0,
active: true,
notes: [{}],
diff_discussion: true,
});
const createDiscussions = () => [...'abcde'].map(discussion);
const createComponent = () => ({
mixins: [discussionNavigation],
render() {
return this.$slots.default;
},
});
describe('Discussion navigation mixin', () => {
const localVue = createLocalVue();
localVue.use(Vuex);
let wrapper;
let store;
let expandDiscussion;
beforeEach(() => {
setHTMLFixture(
[...'abcde']
.map(
id =>
`<ul class="notes" data-discussion-id="${id}"></ul>
<div class="discussion" data-discussion-id="${id}"></div>`,
)
.join(''),
);
jest.spyOn(utils, 'scrollToElement');
expandDiscussion = jest.fn();
const { actions, ...notesRest } = notesModule();
store = new Vuex.Store({
modules: {
notes: {
...notesRest,
actions: { ...actions, expandDiscussion },
},
},
});
store.state.notes.discussions = createDiscussions();
wrapper = shallowMount(createComponent(), { store, localVue });
});
afterEach(() => {
wrapper.vm.$destroy();
jest.clearAllMocks();
});
const findDiscussion = (selector, id) =>
document.querySelector(`${selector}[data-discussion-id="${id}"]`);
describe('cycle through discussions', () => {
beforeEach(() => {
// eslint-disable-next-line new-cap
window.mrTabs = { eventHub: new localVue(), tabShown: jest.fn() };
});
describe.each`
fn | args | currentId | expected
${'jumpToNextDiscussion'} | ${[]} | ${null} | ${'a'}
${'jumpToNextDiscussion'} | ${[]} | ${'a'} | ${'c'}
${'jumpToNextDiscussion'} | ${[]} | ${'e'} | ${'a'}
${'jumpToPreviousDiscussion'} | ${[]} | ${null} | ${'e'}
${'jumpToPreviousDiscussion'} | ${[]} | ${'e'} | ${'c'}
${'jumpToPreviousDiscussion'} | ${[]} | ${'c'} | ${'a'}
${'jumpToNextRelativeDiscussion'} | ${[null]} | ${null} | ${'a'}
${'jumpToNextRelativeDiscussion'} | ${['a']} | ${null} | ${'c'}
${'jumpToNextRelativeDiscussion'} | ${['e']} | ${'c'} | ${'a'}
`('$fn (args = $args, currentId = $currentId)', ({ fn, args, currentId, expected }) => {
beforeEach(() => {
store.state.notes.currentDiscussionId = currentId;
});
describe('on `show` active tab', () => {
beforeEach(() => {
window.mrTabs.currentAction = 'show';
wrapper.vm[fn](...args);
});
it('sets current discussion', () => {
expect(store.state.notes.currentDiscussionId).toEqual(expected);
});
it('expands discussion', () => {
expect(expandDiscussion).toHaveBeenCalled();
});
it('scrolls to element', () => {
expect(utils.scrollToElement).toHaveBeenCalledWith(
findDiscussion('div.discussion', expected),
);
});
});
describe('on `diffs` active tab', () => {
beforeEach(() => {
window.mrTabs.currentAction = 'diffs';
wrapper.vm[fn](...args);
});
it('sets current discussion', () => {
expect(store.state.notes.currentDiscussionId).toEqual(expected);
});
it('expands discussion', () => {
expect(expandDiscussion).toHaveBeenCalled();
});
it('scrolls when scrollToDiscussion is emitted', () => {
expect(utils.scrollToElement).not.toHaveBeenCalled();
eventHub.$emit('scrollToDiscussion');
expect(utils.scrollToElement).toHaveBeenCalledWith(findDiscussion('ul.notes', expected));
});
});
describe('on `other` active tab', () => {
beforeEach(() => {
window.mrTabs.currentAction = 'other';
wrapper.vm[fn](...args);
});
it('sets current discussion', () => {
expect(store.state.notes.currentDiscussionId).toEqual(expected);
});
it('does not expand discussion yet', () => {
expect(expandDiscussion).not.toHaveBeenCalled();
});
it('shows mrTabs', () => {
expect(window.mrTabs.tabShown).toHaveBeenCalledWith('show');
});
describe('when tab is changed', () => {
beforeEach(() => {
window.mrTabs.eventHub.$emit('MergeRequestTabChange');
jest.runAllTimers();
});
it('expands discussion', () => {
expect(expandDiscussion).toHaveBeenCalledWith(
expect.anything(),
{
discussionId: expected,
},
undefined,
);
});
it('scrolls to discussion', () => {
expect(utils.scrollToElement).toHaveBeenCalledWith(
findDiscussion('div.discussion', expected),
);
});
});
});
});
});
});
import Vue from 'vue';
import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
import createStore from '~/notes/stores';
import DiscussionCounter from '~/notes/components/discussion_counter.vue';
import { noteableDataMock, discussionMock, notesDataMock } from '../mock_data';
describe('DiscussionCounter component', () => {
let store;
let vm;
const notes = { currentDiscussionId: null };
beforeEach(() => {
window.mrTabs = {};
const Component = Vue.extend(DiscussionCounter);
store = createStore();
store.dispatch('setNoteableData', noteableDataMock);
store.dispatch('setNotesData', notesDataMock);
vm = createComponentWithStore(Component, store);
});
afterEach(() => {
vm.$destroy();
});
describe('methods', () => {
describe('jumpToNextDiscussion', () => {
it('expands unresolved discussion', () => {
window.mrTabs.currentAction = 'show';
spyOn(vm, 'expandDiscussion').and.stub();
const discussions = [
{
...discussionMock,
id: discussionMock.id,
notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: true }],
resolved: true,
},
{
...discussionMock,
id: discussionMock.id + 1,
notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: false }],
resolved: false,
},
];
const firstDiscussionId = discussionMock.id + 1;
store.replaceState({
...store.state,
discussions,
notes,
});
vm.jumpToNextDiscussion();
expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: firstDiscussionId });
});
it('jumps to next 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,
notes,
});
vm.jumpToNextDiscussion();
expect(vm.switchToDiscussionsTabAndJumpTo).toHaveBeenCalledWith(unresolvedId);
});
});
});
});
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment