Commit 85daddbe authored by André Luís's avatar André Luís Committed by Phil Hughes

Resolve "Navigating unresolved discussions on Merge Request page"

parent f3299596
...@@ -4,6 +4,7 @@ import _ from 'underscore'; ...@@ -4,6 +4,7 @@ import _ from 'underscore';
import { __, sprintf } from '~/locale'; import { __, sprintf } from '~/locale';
import createFlash from '~/flash'; import createFlash from '~/flash';
import { GlLoadingIcon } from '@gitlab/ui'; import { GlLoadingIcon } from '@gitlab/ui';
import eventHub from '../../notes/event_hub';
import DiffFileHeader from './diff_file_header.vue'; import DiffFileHeader from './diff_file_header.vue';
import DiffContent from './diff_content.vue'; import DiffContent from './diff_content.vue';
...@@ -75,6 +76,9 @@ export default { ...@@ -75,6 +76,9 @@ export default {
} }
}, },
}, },
created() {
eventHub.$on(`loadCollapsedDiff/${this.file.file_hash}`, this.handleLoadCollapsedDiff);
},
methods: { methods: {
...mapActions('diffs', ['loadCollapsedDiff', 'assignDiscussionsToDiff']), ...mapActions('diffs', ['loadCollapsedDiff', 'assignDiscussionsToDiff']),
handleToggle() { handleToggle() {
......
...@@ -3,8 +3,9 @@ import axios from '~/lib/utils/axios_utils'; ...@@ -3,8 +3,9 @@ import axios from '~/lib/utils/axios_utils';
import Cookies from 'js-cookie'; import Cookies from 'js-cookie';
import createFlash from '~/flash'; import createFlash from '~/flash';
import { s__ } from '~/locale'; import { s__ } from '~/locale';
import { handleLocationHash, historyPushState } from '~/lib/utils/common_utils'; import { handleLocationHash, historyPushState, scrollToElement } from '~/lib/utils/common_utils';
import { mergeUrlParams, getLocationHash } from '~/lib/utils/url_utility'; import { mergeUrlParams, getLocationHash } from '~/lib/utils/url_utility';
import eventHub from '../../notes/event_hub';
import { getDiffPositionByLineCode, getNoteFormData } from './utils'; import { getDiffPositionByLineCode, getNoteFormData } from './utils';
import * as types from './mutation_types'; import * as types from './mutation_types';
import { import {
...@@ -53,6 +54,10 @@ export const assignDiscussionsToDiff = ( ...@@ -53,6 +54,10 @@ export const assignDiscussionsToDiff = (
diffPositionByLineCode, diffPositionByLineCode,
}); });
}); });
Vue.nextTick(() => {
eventHub.$emit('scrollToDiscussion');
});
}; };
export const removeDiscussionsFromDiff = ({ commit }, removeDiscussion) => { export const removeDiscussionsFromDiff = ({ commit }, removeDiscussion) => {
...@@ -60,6 +65,27 @@ export const removeDiscussionsFromDiff = ({ commit }, removeDiscussion) => { ...@@ -60,6 +65,27 @@ export const removeDiscussionsFromDiff = ({ commit }, removeDiscussion) => {
commit(types.REMOVE_LINE_DISCUSSIONS_FOR_FILE, { fileHash: file_hash, lineCode: line_code, id }); commit(types.REMOVE_LINE_DISCUSSIONS_FOR_FILE, { fileHash: file_hash, lineCode: line_code, id });
}; };
export const renderFileForDiscussionId = ({ commit, rootState, state }, discussionId) => {
const discussion = rootState.notes.discussions.find(d => d.id === discussionId);
if (discussion) {
const file = state.diffFiles.find(f => f.file_hash === discussion.diff_file.file_hash);
if (file) {
if (!file.renderIt) {
commit(types.RENDER_FILE, file);
}
if (file.collapsed) {
eventHub.$emit(`loadCollapsedDiff/${file.file_hash}`);
scrollToElement(document.getElementById(file.file_hash));
} else {
eventHub.$emit('scrollToDiscussion');
}
}
}
};
export const startRenderDiffsQueue = ({ state, commit }) => { export const startRenderDiffsQueue = ({ state, commit }) => {
const checkItem = () => const checkItem = () =>
new Promise(resolve => { new Promise(resolve => {
......
...@@ -170,7 +170,7 @@ export default { ...@@ -170,7 +170,7 @@ export default {
} }
if (!file.parallel_diff_lines || !file.highlighted_diff_lines) { if (!file.parallel_diff_lines || !file.highlighted_diff_lines) {
file.discussions = file.discussions.concat(discussion); file.discussions = (file.discussions || []).concat(discussion);
} }
return file; return file;
......
...@@ -192,8 +192,12 @@ export const contentTop = () => { ...@@ -192,8 +192,12 @@ export const contentTop = () => {
const mrTabsHeight = $('.merge-request-tabs').height() || 0; const mrTabsHeight = $('.merge-request-tabs').height() || 0;
const headerHeight = $('.navbar-gitlab').height() || 0; const headerHeight = $('.navbar-gitlab').height() || 0;
const diffFilesChanged = $('.js-diff-files-changed').height() || 0; const diffFilesChanged = $('.js-diff-files-changed').height() || 0;
const diffFileLargeEnoughScreen =
'matchMedia' in window ? window.matchMedia('min-width: 768') : true;
const diffFileTitleBar =
(diffFileLargeEnoughScreen && $('.diff-file .file-title-flex-parent:visible').height()) || 0;
return perfBar + mrTabsHeight + headerHeight + diffFilesChanged; return perfBar + mrTabsHeight + headerHeight + diffFilesChanged + diffFileTitleBar;
}; };
export const scrollToElement = element => { export const scrollToElement = element => {
......
...@@ -81,6 +81,7 @@ export default { ...@@ -81,6 +81,7 @@ export default {
'nextUnresolvedDiscussionId', 'nextUnresolvedDiscussionId',
'unresolvedDiscussionsCount', 'unresolvedDiscussionsCount',
'hasUnresolvedDiscussions', 'hasUnresolvedDiscussions',
'showJumpToNextDiscussion',
]), ]),
author() { author() {
return this.initialDiscussion.author; return this.initialDiscussion.author;
...@@ -121,6 +122,12 @@ export default { ...@@ -121,6 +122,12 @@ export default {
resolvedText() { resolvedText() {
return this.discussion.resolved_by_push ? __('Automatically resolved') : __('Resolved'); return this.discussion.resolved_by_push ? __('Automatically resolved') : __('Resolved');
}, },
shouldShowJumpToNextDiscussion() {
return this.showJumpToNextDiscussion(
this.discussion.id,
this.discussionsByDiffOrder ? 'diff' : 'discussion',
);
},
shouldRenderDiffs() { shouldRenderDiffs() {
return this.discussion.diff_discussion && this.renderDiffFile; return this.discussion.diff_discussion && this.renderDiffFile;
}, },
...@@ -418,7 +425,7 @@ Please check your network connection and try again.`; ...@@ -418,7 +425,7 @@ Please check your network connection and try again.`;
<icon name="issue-new" /> <icon name="issue-new" />
</a> </a>
</div> </div>
<div v-if="hasUnresolvedDiscussions" class="btn-group" role="group"> <div v-if="shouldShowJumpToNextDiscussion" class="btn-group" role="group">
<button <button
v-gl-tooltip v-gl-tooltip
class="btn btn-default discussion-next-btn" class="btn btn-default discussion-next-btn"
......
import { scrollToElement } from '~/lib/utils/common_utils'; import { scrollToElement } from '~/lib/utils/common_utils';
import eventHub from '../../notes/event_hub';
export default { export default {
methods: { methods: {
jumpToDiscussion(id) { diffsJump(id) {
if (id) { const selector = `ul.notes[data-discussion-id="${id}"]`;
const activeTab = window.mrTabs.currentAction;
const selector =
activeTab === 'diffs'
? `ul.notes[data-discussion-id="${id}"]`
: `div.discussion[data-discussion-id="${id}"]`;
const el = document.querySelector(selector);
if (activeTab === 'commits' || activeTab === 'pipelines') { eventHub.$once('scrollToDiscussion', () => {
window.mrTabs.activateTab('show'); const el = document.querySelector(selector);
}
if (el) { if (el) {
this.expandDiscussion({ discussionId: id });
scrollToElement(el); scrollToElement(el);
return true; 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; return false;
}, },
jumpToDiscussion(id) {
if (id) {
const activeTab = window.mrTabs.currentAction;
if (activeTab === 'diffs') {
this.diffsJump(id);
} else if (activeTab === 'commits' || activeTab === 'pipelines') {
window.mrTabs.eventHub.$once('MergeRequestTabChange', () => {
setTimeout(() => this.discussionJump(id), 0);
});
window.mrTabs.tabShown('show');
} else {
this.discussionJump(id);
}
}
},
}, },
}; };
...@@ -17,7 +17,13 @@ import { __ } from '~/locale'; ...@@ -17,7 +17,13 @@ import { __ } from '~/locale';
let eTagPoll; let eTagPoll;
export const expandDiscussion = ({ commit }, data) => commit(types.EXPAND_DISCUSSION, data); export const expandDiscussion = ({ commit, dispatch }, data) => {
if (data.discussionId) {
dispatch('diffs/renderFileForDiscussionId', data.discussionId, { root: true });
}
commit(types.EXPAND_DISCUSSION, data);
};
export const collapseDiscussion = ({ commit }, data) => commit(types.COLLAPSE_DISCUSSION, data); export const collapseDiscussion = ({ commit }, data) => commit(types.COLLAPSE_DISCUSSION, data);
......
...@@ -57,6 +57,17 @@ export const unresolvedDiscussionsCount = state => state.unresolvedDiscussionsCo ...@@ -57,6 +57,17 @@ export const unresolvedDiscussionsCount = state => state.unresolvedDiscussionsCo
export const resolvableDiscussionsCount = state => state.resolvableDiscussionsCount; export const resolvableDiscussionsCount = state => state.resolvableDiscussionsCount;
export const hasUnresolvedDiscussions = state => state.hasUnresolvedDiscussions; export const hasUnresolvedDiscussions = state => state.hasUnresolvedDiscussions;
export const showJumpToNextDiscussion = (state, getters) => (discussionId, mode = 'discussion') => {
const orderedDiffs =
mode !== 'discussion'
? getters.unresolvedDiscussionsIdsByDiff
: getters.unresolvedDiscussionsIdsByDate;
const indexOf = orderedDiffs.indexOf(discussionId);
return indexOf !== -1 && indexOf < orderedDiffs.length - 1;
};
export const isDiscussionResolved = (state, getters) => discussionId => export const isDiscussionResolved = (state, getters) => discussionId =>
getters.resolvedDiscussionsById[discussionId] !== undefined; getters.resolvedDiscussionsById[discussionId] !== undefined;
...@@ -104,7 +115,7 @@ export const unresolvedDiscussionsIdsByDate = (state, getters) => ...@@ -104,7 +115,7 @@ export const unresolvedDiscussionsIdsByDate = (state, getters) =>
// line numbers. // line numbers.
export const unresolvedDiscussionsIdsByDiff = (state, getters) => export const unresolvedDiscussionsIdsByDiff = (state, getters) =>
getters.allResolvableDiscussions getters.allResolvableDiscussions
.filter(d => !d.resolved) .filter(d => !d.resolved && d.active)
.sort((a, b) => { .sort((a, b) => {
if (!a.diff_file || !b.diff_file) { if (!a.diff_file || !b.diff_file) {
return 0; return 0;
......
...@@ -22,6 +22,7 @@ export default { ...@@ -22,6 +22,7 @@ export default {
if (isDiscussion && isInMRPage()) { if (isDiscussion && isInMRPage()) {
noteData.resolvable = note.resolvable; noteData.resolvable = note.resolvable;
noteData.resolved = false; noteData.resolved = false;
noteData.active = true;
noteData.resolve_path = note.resolve_path; noteData.resolve_path = note.resolve_path;
noteData.resolve_with_issue_path = note.resolve_with_issue_path; noteData.resolve_with_issue_path = note.resolve_with_issue_path;
noteData.diff_discussion = false; noteData.diff_discussion = false;
......
---
title: Fix navigating by unresolved discussions on Merge Request page
merge_request: 22789
author:
type: fixed
...@@ -361,8 +361,14 @@ describe 'Merge request > User resolves diff notes and discussions', :js do ...@@ -361,8 +361,14 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
end end
end end
it 'shows jump to next discussion button' do it 'shows jump to next discussion button except on last discussion' do
expect(page.all('.discussion-reply-holder', count: 2)).to all(have_selector('.discussion-next-btn')) 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(0)
end end
it 'displays next discussion even if hidden' do it 'displays next discussion even if hidden' do
...@@ -380,7 +386,13 @@ describe 'Merge request > User resolves diff notes and discussions', :js do ...@@ -380,7 +386,13 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
page.find('.discussion-next-btn').click page.find('.discussion-next-btn').click
end end
expect(find('.discussion-with-resolve-btn')).to have_selector('.btn', text: 'Resolve discussion') page.all('.note-discussion').first do
expect(page.find('.discussion-with-resolve-btn')).to have_selector('.btn', text: 'Resolve discussion')
end
page.all('.note-discussion').last do
expect(page.find('.discussion-with-resolve-btn')).not.to have_selector('.btn', text: 'Resolve discussion')
end
end end
end end
......
...@@ -26,7 +26,9 @@ import actions, { ...@@ -26,7 +26,9 @@ import actions, {
toggleTreeOpen, toggleTreeOpen,
scrollToFile, scrollToFile,
toggleShowTreeList, toggleShowTreeList,
renderFileForDiscussionId,
} from '~/diffs/store/actions'; } from '~/diffs/store/actions';
import eventHub from '~/notes/event_hub';
import * as types from '~/diffs/store/mutation_types'; import * as types from '~/diffs/store/mutation_types';
import axios from '~/lib/utils/axios_utils'; import axios from '~/lib/utils/axios_utils';
import mockDiffFile from 'spec/diffs/mock_data/diff_file'; import mockDiffFile from 'spec/diffs/mock_data/diff_file';
...@@ -735,4 +737,63 @@ describe('DiffsStoreActions', () => { ...@@ -735,4 +737,63 @@ describe('DiffsStoreActions', () => {
expect(localStorage.setItem).toHaveBeenCalledWith('mr_tree_show', true); expect(localStorage.setItem).toHaveBeenCalledWith('mr_tree_show', true);
}); });
}); });
describe('renderFileForDiscussionId', () => {
const rootState = {
notes: {
discussions: [
{
id: '123',
diff_file: {
file_hash: 'HASH',
},
},
{
id: '456',
diff_file: {
file_hash: 'HASH',
},
},
],
},
};
let commit;
let $emit;
let scrollToElement;
const state = ({ collapsed, renderIt }) => ({
diffFiles: [
{
file_hash: 'HASH',
collapsed,
renderIt,
},
],
});
beforeEach(() => {
commit = jasmine.createSpy('commit');
scrollToElement = spyOnDependency(actions, 'scrollToElement').and.stub();
$emit = spyOn(eventHub, '$emit');
});
it('renders and expands file for the given discussion id', () => {
const localState = state({ collapsed: true, renderIt: false });
renderFileForDiscussionId({ rootState, state: localState, commit }, '123');
expect(commit).toHaveBeenCalledWith('RENDER_FILE', localState.diffFiles[0]);
expect($emit).toHaveBeenCalledTimes(1);
expect(scrollToElement).toHaveBeenCalledTimes(1);
});
it('jumps to discussion on already rendered and expanded file', () => {
const localState = state({ collapsed: false, renderIt: true });
renderFileForDiscussionId({ rootState, state: localState, commit }, '123');
expect(commit).not.toHaveBeenCalled();
expect($emit).toHaveBeenCalledTimes(1);
expect(scrollToElement).not.toHaveBeenCalled();
});
});
}); });
...@@ -83,6 +83,7 @@ describe('noteable_discussion component', () => { ...@@ -83,6 +83,7 @@ describe('noteable_discussion component', () => {
it('expands next unresolved discussion', done => { it('expands next unresolved discussion', done => {
const discussion2 = getJSONFixture(discussionWithTwoUnresolvedNotes)[0]; const discussion2 = getJSONFixture(discussionWithTwoUnresolvedNotes)[0];
discussion2.resolved = false; discussion2.resolved = false;
discussion2.active = true;
discussion2.id = 'next'; // prepare this for being identified as next one (to be jumped to) discussion2.id = 'next'; // prepare this for being identified as next one (to be jumped to)
vm.$store.dispatch('setInitialNotes', [discussionMock, discussion2]); vm.$store.dispatch('setInitialNotes', [discussionMock, discussion2]);
window.mrTabs.currentAction = 'show'; window.mrTabs.currentAction = 'show';
......
...@@ -305,6 +305,7 @@ export const discussionMock = { ...@@ -305,6 +305,7 @@ export const discussionMock = {
], ],
individual_note: false, individual_note: false,
resolvable: true, resolvable: true,
active: true,
}; };
export const loggedOutnoteableData = { export const loggedOutnoteableData = {
...@@ -1173,6 +1174,7 @@ export const discussion1 = { ...@@ -1173,6 +1174,7 @@ export const discussion1 = {
id: 'abc1', id: 'abc1',
resolvable: true, resolvable: true,
resolved: false, resolved: false,
active: true,
diff_file: { diff_file: {
file_path: 'about.md', file_path: 'about.md',
}, },
...@@ -1209,6 +1211,7 @@ export const discussion2 = { ...@@ -1209,6 +1211,7 @@ export const discussion2 = {
id: 'abc2', id: 'abc2',
resolvable: true, resolvable: true,
resolved: false, resolved: false,
active: true,
diff_file: { diff_file: {
file_path: 'README.md', file_path: 'README.md',
}, },
...@@ -1226,6 +1229,7 @@ export const discussion2 = { ...@@ -1226,6 +1229,7 @@ export const discussion2 = {
export const discussion3 = { export const discussion3 = {
id: 'abc3', id: 'abc3',
resolvable: true, resolvable: true,
active: true,
resolved: false, resolved: false,
diff_file: { diff_file: {
file_path: 'README.md', file_path: 'README.md',
......
...@@ -124,7 +124,7 @@ describe('Actions Notes Store', () => { ...@@ -124,7 +124,7 @@ describe('Actions Notes Store', () => {
{ discussionId: discussionMock.id }, { discussionId: discussionMock.id },
{ notes: [discussionMock] }, { notes: [discussionMock] },
[{ type: 'EXPAND_DISCUSSION', payload: { discussionId: discussionMock.id } }], [{ type: 'EXPAND_DISCUSSION', payload: { discussionId: discussionMock.id } }],
[], [{ type: 'diffs/renderFileForDiscussionId', payload: discussionMock.id }],
done, done,
); );
}); });
......
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