Commit 9132ad69 authored by Paul Slaughter's avatar Paul Slaughter

Merge branch 'jdb/fix-jump-to-next-unresolved-thread' into 'master'

Fix Jump to next unresolved thread

See merge request gitlab-org/gitlab!24728
parents 9db42d24 169e409c
......@@ -73,7 +73,7 @@ export default {
v-if="discussion.resolvable && shouldShowJumpToNextDiscussion"
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>
</template>
......@@ -39,7 +39,11 @@ export default {
</script>
<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="{ 'has-next-btn': hasNextButton }" class="line-resolve-all">
<span
......
......@@ -12,6 +12,12 @@ export default {
GlTooltip: GlTooltipDirective,
},
mixins: [discussionNavigation],
props: {
fromDiscussionId: {
type: String,
required: true,
},
},
};
</script>
......@@ -22,7 +28,7 @@ export default {
v-gl-tooltip
class="btn btn-default discussion-next-btn"
:title="s__('MergeRequests|Jump to next unresolved thread')"
@click="jumpToNextDiscussion"
@click="jumpToNextRelativeDiscussion(fromDiscussionId)"
>
<icon name="comment-next" />
</button>
......
......@@ -2,6 +2,86 @@ import { mapGetters, mapActions, mapState } from 'vuex';
import { scrollToElement } from '~/lib/utils/common_utils';
import eventHub from '../../notes/event_hub';
/**
* @param {string} selector
* @returns {boolean}
*/
function scrollTo(selector) {
const el = document.querySelector(selector);
if (el) {
scrollToElement(el);
return true;
}
return false;
}
/**
* @param {object} self Component instance with mixin applied
* @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}"]`;
expandDiscussion({ discussionId: id });
return scrollTo(selector);
}
/**
* @param {object} self Component instance with mixin applied
* @param {string} id Discussion id we are jumping to
*/
function switchToDiscussionsTabAndJumpTo(self, id) {
window.mrTabs.eventHub.$once('MergeRequestTabChange', () => {
setTimeout(() => discussionJump(self, id), 0);
});
window.mrTabs.tabShown('show');
}
/**
* @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;
if (id) {
const activeTab = window.mrTabs.currentAction;
if (activeTab === 'diffs' && isDiffDiscussion) {
diffsJump(self, id);
} else if (activeTab === 'show') {
discussionJump(self, id);
} else {
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([
......@@ -16,76 +96,20 @@ export default {
methods: {
...mapActions(['expandDiscussion', 'setCurrentDiscussionId']),
diffsJump(id) {
const selector = `ul.notes[data-discussion-id="${id}"]`;
eventHub.$once('scrollToDiscussion', () => {
const el = document.querySelector(selector);
if (el) {
scrollToElement(el);
return true;
}
return false;
});
this.expandDiscussion({ discussionId: id });
},
discussionJump(id) {
const selector = `div.discussion[data-discussion-id="${id}"]`;
const el = document.querySelector(selector);
this.expandDiscussion({ discussionId: id });
if (el) {
scrollToElement(el);
return true;
}
return false;
},
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' && isDiffDiscussion) {
this.diffsJump(id);
} else if (activeTab === 'show') {
this.discussionJump(id);
} else {
this.switchToDiscussionsTabAndJumpTo(id);
}
}
},
jumpToNextDiscussion() {
this.handleDiscussionJump(this.nextUnresolvedDiscussionId);
handleDiscussionJump(this, this.nextUnresolvedDiscussionId);
},
jumpToPreviousDiscussion() {
this.handleDiscussionJump(this.previousUnresolvedDiscussionId);
handleDiscussionJump(this, 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);
/**
* Go to the next discussion from the given discussionId
* @param {String} discussionId The id we are jumping from
*/
jumpToNextRelativeDiscussion(discussionId) {
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
describe('JumpToNextDiscussionButton', () => {
let wrapper;
const fromDiscussionId = 'abc123';
beforeEach(() => {
wrapper = shallowMount(JumpToNextDiscussionButton);
wrapper = shallowMount(JumpToNextDiscussionButton, {
propsData: { fromDiscussionId },
});
});
afterEach(() => {
......@@ -15,4 +18,11 @@ describe('JumpToNextDiscussionButton', () => {
it('matches the snapshot', () => {
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 */
import 'mousetrap';
import { shallowMount, createLocalVue } from '@vue/test-utils';
import Vuex from 'vuex';
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', () => {
let storeOptions;
let wrapper;
let store;
const localVue = createLocalVue();
const createComponent = (options = {}) => {
store = new Vuex.Store(storeOptions);
let wrapper;
let jumpToNextDiscussion;
let jumpToPreviousDiscussion;
const createComponent = () => {
wrapper = shallowMount(DiscussionKeyboardNavigator, {
localVue,
store,
...options,
mixins: [
localVue.extend({
methods: {
jumpToNextDiscussion,
jumpToPreviousDiscussion,
},
}),
],
});
wrapper.vm.jumpToDiscussion = jest.fn();
};
beforeEach(() => {
const notes = notesModule();
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,
},
};
jumpToNextDiscussion = jest.fn();
jumpToPreviousDiscussion = jest.fn();
});
afterEach(() => {
wrapper.destroy();
storeOptions = null;
store = null;
wrapper = null;
});
describe.each`
currentAction | expectedNextId | expectedPrevId
${'diffs'} | ${NEXT_DIFF_ID} | ${PREV_DIFF_ID}
${'show'} | ${NEXT_ID} | ${PREV_ID}
`('when isDiffView is $isDiffView', ({ currentAction, expectedNextId, expectedPrevId }) => {
describe('on mount', () => {
beforeEach(() => {
window.mrTabs = { currentAction };
createComponent();
});
afterEach(() => delete window.mrTabs);
it('calls jumpToNextDiscussion when pressing `n`', () => {
Mousetrap.trigger('n');
expect(wrapper.vm.jumpToDiscussion).toHaveBeenCalledWith(
expect.objectContaining({ id: expectedNextId }),
);
expect(wrapper.vm.currentDiscussionId).toEqual(expectedNextId);
expect(jumpToNextDiscussion).toHaveBeenCalled();
});
it('calls jumpToPreviousDiscussion when pressing `p`', () => {
Mousetrap.trigger('p');
expect(wrapper.vm.jumpToDiscussion).toHaveBeenCalledWith(
expect.objectContaining({ id: expectedPrevId }),
);
expect(wrapper.vm.currentDiscussionId).toEqual(expectedPrevId);
expect(jumpToPreviousDiscussion).toHaveBeenCalled();
});
});
......@@ -99,13 +68,13 @@ describe('notes/components/discussion_keyboard_navigator', () => {
it('does not call jumpToNextDiscussion when pressing `n`', () => {
Mousetrap.trigger('n');
expect(wrapper.vm.jumpToDiscussion).not.toHaveBeenCalled();
expect(jumpToNextDiscussion).not.toHaveBeenCalled();
});
it('does not call jumpToNextDiscussion when pressing `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