Skip to content

G
gitlab-ce
Commit f074afe3 authored by André Luís's avatar André Luís
Browse files
Options

Revert "Merge branch '48817-fix-mr-changes-discussion-navigation' into 'master'" 

This reverts commit ced005f3, reversing
changes made to 9b01b293.
parent 2313ed7c
Hide whitespace changes
Inline Side-by-side
Showing with 68 additions and 407 deletions
app/assets/javascripts/diffs/components/diff_discussions.vue
View file @ f074afe3
......@@ -30,7 +30,6 @@ export default {
:render-header="false"
:render-diff-file="false"
:always-expanded="true"
:discussions-by-diff-order="true"
/>
</ul>
</div>
......
app/assets/javascripts/notes/components/discussion_counter.vue
View file @ f074afe3
......@@ -5,20 +5,19 @@ import resolvedSvg from 'icons/_icon_status_success_solid.svg';
import mrIssueSvg from 'icons/_icon_mr_issue.svg';
import nextDiscussionSvg from 'icons/_next_discussion.svg';
import { pluralize } from '../../lib/utils/text_utility';
import discussionNavigation from '../mixins/discussion_navigation';
import { scrollToElement } from '../../lib/utils/common_utils';
import tooltip from '../../vue_shared/directives/tooltip';
export default {
directives: {
tooltip,
},
mixins: [discussionNavigation],
computed: {
...mapGetters([
'getUserData',
'getNoteableData',
'discussionCount',
'firstUnresolvedDiscussionId',
'unresolvedDiscussions',
'resolvedDiscussionCount',
]),
isLoggedIn() {
......@@ -36,6 +35,11 @@ export default {
resolveAllDiscussionsIssuePath() {
return this.getNoteableData.create_issue_to_resolve_discussions_path;
},
firstUnresolvedDiscussionId() {
const item = this.unresolvedDiscussions[0] || {};
return item.id;
},
},
created() {
this.resolveSvg = resolveSvg;
......@@ -46,10 +50,22 @@ export default {
methods: {
...mapActions(['expandDiscussion']),
jumpToFirstUnresolvedDiscussion() {
const diffTab = window.mrTabs.currentAction === 'diffs';
const discussionId = this.firstUnresolvedDiscussionId(diffTab);
const discussionId = this.firstUnresolvedDiscussionId;
if (!discussionId) {
return;
}
const el = document.querySelector(`[data-discussion-id="${discussionId}"]`);
const activeTab = window.mrTabs.currentAction;
if (activeTab === 'commits' || activeTab === 'pipelines') {
window.mrTabs.activateTab('show');
}
this.jumpToDiscussion(discussionId);
if (el) {
this.expandDiscussion({ discussionId });
scrollToElement(el);
}
},
},
};
......
app/assets/javascripts/notes/components/noteable_discussion.vue
View file @ f074afe3
<script>
import _ from 'underscore';
import { mapActions, mapGetters } from 'vuex';
import resolveDiscussionsSvg from 'icons/_icon_mr_issue.svg';
import nextDiscussionsSvg from 'icons/_next_discussion.svg';
import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
import { convertObjectPropsToCamelCase, scrollToElement } from '~/lib/utils/common_utils';
import { truncateSha } from '~/lib/utils/text_utility';
import systemNote from '~/vue_shared/components/notes/system_note.vue';
import { s__ } from '~/locale';
......@@ -20,7 +21,6 @@ import placeholderSystemNote from '../../vue_shared/components/notes/placeholder
import autosave from '../mixins/autosave';
import noteable from '../mixins/noteable';
import resolvable from '../mixins/resolvable';
import discussionNavigation from '../mixins/discussion_navigation';
import tooltip from '../../vue_shared/directives/tooltip';
export default {
......@@ -40,7 +40,7 @@ export default {
directives: {
tooltip,
},
mixins: [autosave, noteable, resolvable, discussionNavigation],
mixins: [autosave, noteable, resolvable],
props: {
discussion: {
type: Object,
......@@ -61,11 +61,6 @@ export default {
required: false,
default: false,
},
discussionsByDiffOrder: {
type: Boolean,
required: false,
default: false,
},
},
data() {
return {
......@@ -80,12 +75,7 @@ export default {
'discussionCount',
'resolvedDiscussionCount',
'allDiscussions',
'unresolvedDiscussionsIdsByDiff',
'unresolvedDiscussionsIdsByDate',
'unresolvedDiscussions',
'unresolvedDiscussionsIdsOrdered',
'nextUnresolvedDiscussionId',
'isLastUnresolvedDiscussion',
]),
transformedDiscussion() {
return {
......@@ -136,10 +126,6 @@ export default {
hasMultipleUnresolvedDiscussions() {
return this.unresolvedDiscussions.length > 1;
},
showJumpToNextDiscussion() {
return this.hasMultipleUnresolvedDiscussions &&
!this.isLastUnresolvedDiscussion(this.discussion.id, this.discussionsByDiffOrder);
},
shouldRenderDiffs() {
const { diffDiscussion, diffFile } = this.transformedDiscussion;
......@@ -256,10 +242,21 @@ Please check your network connection and try again.`;
});
},
jumpToNextDiscussion() {
const nextId =
this.nextUnresolvedDiscussionId(this.discussion.id, this.discussionsByDiffOrder);
const discussionIds = this.allDiscussions.map(d => d.id);
const unresolvedIds = this.unresolvedDiscussions.map(d => d.id);
const currentIndex = discussionIds.indexOf(this.discussion.id);
const remainingAfterCurrent = discussionIds.slice(currentIndex + 1);
const nextIndex = _.findIndex(remainingAfterCurrent, id => unresolvedIds.indexOf(id) > -1);
if (nextIndex > -1) {
const nextId = remainingAfterCurrent[nextIndex];
const el = document.querySelector(`[data-discussion-id="${nextId}"]`);
this.jumpToDiscussion(nextId);
if (el) {
this.expandDiscussion({ discussionId: nextId });
scrollToElement(el);
}
}
},
},
};
......@@ -401,7 +398,7 @@ Please check your network connection and try again.`;
</a>
</div>
<div
v-if="showJumpToNextDiscussion"
v-if="hasMultipleUnresolvedDiscussions"
class="btn-group"
role="group">
<button
......
app/assets/javascripts/notes/mixins/discussion_navigation.js deleted 100644 → 0
View file @ 2313ed7c
import { scrollToElement } from '~/lib/utils/common_utils';
export default {
methods: {
jumpToDiscussion(id) {
if (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') {
window.mrTabs.activateTab('show');
}
if (el) {
this.expandDiscussion({ discussionId: id });
scrollToElement(el);
return true;
}
}
return false;
},
},
};
app/assets/javascripts/notes/stores/getters.js
View file @ f074afe3
......@@ -70,9 +70,6 @@ export const allDiscussions = (state, getters) => {
return Object.values(resolved).concat(unresolved);
};
export const allResolvableDiscussions = (state, getters) =>
getters.allDiscussions.filter(d => !d.individual_note && d.resolvable);
export const resolvedDiscussionsById = state => {
const map = {};
......@@ -89,51 +86,6 @@ export const resolvedDiscussionsById = state => {
return map;
};
// Gets Discussions IDs ordered by the date of their initial note
export const unresolvedDiscussionsIdsByDate = (state, getters) =>
getters.allResolvableDiscussions
.filter(d => !d.resolved)
.sort((a, b) => {
const aDate = new Date(a.notes[0].created_at);
const bDate = new Date(b.notes[0].created_at);
if (aDate < bDate) {
return -1;
}
return aDate === bDate ? 0 : 1;
})
.map(d => d.id);
// Gets Discussions IDs ordered by their position in the diff
//
// Sorts the array of resolvable yet unresolved discussions by
// comparing file names first. If file names are the same, compares
// line numbers.
export const unresolvedDiscussionsIdsByDiff = (state, getters) =>
getters.allResolvableDiscussions
.filter(d => !d.resolved)
.sort((a, b) => {
if (!a.diff_file || !b.diff_file) {
return 0;
}
// Get file names comparison result
const filenameComparison = a.diff_file.file_path.localeCompare(b.diff_file.file_path);
// Get the line numbers, to compare within the same file
const aLines = [a.position.formatter.new_line, a.position.formatter.old_line];
const bLines = [b.position.formatter.new_line, b.position.formatter.old_line];
return filenameComparison < 0 ||
(filenameComparison === 0 &&
// .max() because one of them might be zero (if removed/added)
Math.max(aLines[0], aLines[1]) < Math.max(bLines[0], bLines[1]))
? -1
: 1;
})
.map(d => d.id);
export const resolvedDiscussionCount = (state, getters) => {
const resolvedMap = getters.resolvedDiscussionsById;
......@@ -150,42 +102,5 @@ export const discussionTabCounter = state => {
return all.length;
};
// Returns the list of discussion IDs ordered according to given parameter
// @param {Boolean} diffOrder - is ordered by diff?
export const unresolvedDiscussionsIdsOrdered = (state, getters) => diffOrder => {
if (diffOrder) {
return getters.unresolvedDiscussionsIdsByDiff;
}
return getters.unresolvedDiscussionsIdsByDate;
};
// Checks if a given discussion is the last in the current order (diff or date)
// @param {Boolean} discussionId - id of the discussion
// @param {Boolean} diffOrder - is ordered by diff?
export const isLastUnresolvedDiscussion = (state, getters) => (discussionId, diffOrder) => {
const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder);
const lastDiscussionId = idsOrdered[idsOrdered.length - 1];
return lastDiscussionId === discussionId;
};
// Gets the ID of the discussion following the one provided, respecting order (diff or date)
// @param {Boolean} discussionId - id of the current discussion
// @param {Boolean} diffOrder - is ordered by diff?
export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => {
const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder);
const currentIndex = idsOrdered.indexOf(discussionId);
return idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0];
};
// @param {Boolean} diffOrder - is ordered by diff?
export const firstUnresolvedDiscussionId = (state, getters) => diffOrder => {
if (diffOrder) {
return getters.unresolvedDiscussionsIdsByDiff[0];
}
return getters.unresolvedDiscussionsIdsByDate[0];
};
// prevent babel-plugin-rewire from generating an invalid default during karma tests
export default () => {};
spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb
View file @ f074afe3
......@@ -342,9 +342,8 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
end
end
it 'shows jump to next discussion button, apart from the last one' do
expect(page).to have_selector('.discussion-reply-holder', count: 2)
expect(page).to have_selector('.discussion-reply-holder .discussion-next-btn', count: 1)
it 'shows jump to next discussion button' do
expect(page.all('.discussion-reply-holder', count: 2)).to all(have_selector('.discussion-next-btn'))
end
it 'displays next discussion even if hidden' do
......
spec/javascripts/notes/components/discussion_counter_spec.js
View file @ f074afe3
......@@ -46,7 +46,7 @@ describe('DiscussionCounter component', () => {
discussions,
});
setFixtures(`
<div class="discussion" data-discussion-id="${firstDiscussionId}"></div>
<div data-discussion-id="${firstDiscussionId}"></div>
`);
vm.jumpToFirstUnresolvedDiscussion();
......
spec/javascripts/notes/components/noteable_discussion_spec.js
View file @ f074afe3
......@@ -14,7 +14,6 @@ describe('noteable_discussion component', () => {
preloadFixtures(discussionWithTwoUnresolvedNotes);
beforeEach(() => {
window.mrTabs = {};
store = createStore();
store.dispatch('setNoteableData', noteableDataMock);
store.dispatch('setNotesData', notesDataMock);
......@@ -107,29 +106,33 @@ describe('noteable_discussion component', () => {
describe('methods', () => {
describe('jumpToNextDiscussion', () => {
it('expands next unresolved discussion', done => {
const discussion2 = getJSONFixture(discussionWithTwoUnresolvedNotes)[0];
discussion2.resolved = false;
discussion2.id = 'next'; // prepare this for being identified as next one (to be jumped to)
vm.$store.dispatch('setInitialNotes', [discussionMock, discussion2]);
window.mrTabs.currentAction = 'show';
Vue.nextTick()
.then(() => {
spyOn(vm, 'expandDiscussion').and.stub();
const nextDiscussionId = discussion2.id;
setFixtures(`
<div class="discussion" data-discussion-id="${nextDiscussionId}"></div>
`);
it('expands next unresolved discussion', () => {
spyOn(vm, 'expandDiscussion').and.stub();
const discussions = [
discussionMock,
{
...discussionMock,
id: discussionMock.id + 1,
notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: true }],
},
{
...discussionMock,
id: discussionMock.id + 2,
notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: false }],
},
];
const nextDiscussionId = discussionMock.id + 2;
store.replaceState({
...store.state,
discussions,
});
setFixtures(`
<div data-discussion-id="${nextDiscussionId}"></div>
`);
vm.jumpToNextDiscussion();
vm.jumpToNextDiscussion();
expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: nextDiscussionId });
})
.then(done)
.catch(done.fail);
expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: nextDiscussionId });
});
});
});
......
spec/javascripts/notes/mock_data.js
View file @ f074afe3
......@@ -1168,87 +1168,3 @@ export const collapsedSystemNotes = [
diff_discussion: false,
},
];
export const discussion1 = {
id: 'abc1',
resolvable: true,
resolved: false,
diff_file: {
file_path: 'about.md',
},
position: {
formatter: {
new_line: 50,
old_line: null,
},
},
notes: [
{
created_at: '2018-07-04T16:25:41.749Z',
},
],
};
export const resolvedDiscussion1 = {
id: 'abc1',
resolvable: true,
resolved: true,
diff_file: {
file_path: 'about.md',
},
position: {
formatter: {
new_line: 50,
old_line: null,
},
},
notes: [
{
created_at: '2018-07-04T16:25:41.749Z',
},
],
};
export const discussion2 = {
id: 'abc2',
resolvable: true,
resolved: false,
diff_file: {
file_path: 'README.md',
},
position: {
formatter: {
new_line: null,
old_line: 20,
},
},
notes: [
{
created_at: '2018-07-04T12:05:41.749Z',
},
],
};
export const discussion3 = {
id: 'abc3',
resolvable: true,
resolved: false,
diff_file: {
file_path: 'README.md',
},
position: {
formatter: {
new_line: 21,
old_line: null,
},
},
notes: [
{
created_at: '2018-07-05T17:25:41.749Z',
},
],
};
export const unresolvableDiscussion = {
resolvable: false,
};
spec/javascripts/notes/stores/getters_spec.js
View file @ f074afe3
......@@ -5,11 +5,6 @@ import {
noteableDataMock,
individualNote,
collapseNotesMock,
discussion1,
discussion2,
discussion3,
resolvedDiscussion1,
unresolvableDiscussion,
} from '../mock_data';
const discussionWithTwoUnresolvedNotes = 'merge_requests/resolved_diff_discussion.json';
......@@ -114,154 +109,4 @@ describe('Getters Notes Store', () => {
expect(getters.isNotesFetched(state)).toBeFalsy();
});
});
describe('allResolvableDiscussions', () => {
it('should return only resolvable discussions in same order', () => {
const localGetters = {
allDiscussions: [
discussion3,
unresolvableDiscussion,
discussion1,
unresolvableDiscussion,
discussion2,
],
};
expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([
discussion3,
discussion1,
discussion2,
]);
});
it('should return empty array if there are no resolvable discussions', () => {
const localGetters = {
allDiscussions: [unresolvableDiscussion, unresolvableDiscussion],
};
expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([]);
});
});
describe('unresolvedDiscussionsIdsByDiff', () => {
it('should return all discussions IDs in diff order', () => {
const localGetters = {
allResolvableDiscussions: [discussion3, discussion1, discussion2],
};
expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([
'abc1',
'abc2',
'abc3',
]);
});
it('should return empty array if all discussions have been resolved', () => {
const localGetters = {
allResolvableDiscussions: [resolvedDiscussion1],
};
expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([]);
});
});
describe('unresolvedDiscussionsIdsByDate', () => {
it('should return all discussions in date ascending order', () => {
const localGetters = {
allResolvableDiscussions: [discussion3, discussion1, discussion2],
};
expect(getters.unresolvedDiscussionsIdsByDate(state, localGetters)).toEqual([
'abc2',
'abc1',
'abc3',
]);
});
it('should return empty array if all discussions have been resolved', () => {
const localGetters = {
allResolvableDiscussions: [resolvedDiscussion1],
};
expect(getters.unresolvedDiscussionsIdsByDate(state, localGetters)).toEqual([]);
});
});
describe('unresolvedDiscussionsIdsOrdered', () => {
const localGetters = {
unresolvedDiscussionsIdsByDate: ['123', '456'],
unresolvedDiscussionsIdsByDiff: ['abc', 'def'],
};
it('should return IDs ordered by diff when diffOrder param is true', () => {
expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(true)).toEqual([
'abc',
'def',
]);
});
it('should return IDs ordered by date when diffOrder param is not true', () => {
expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(false)).toEqual([
'123',
'456',
]);
expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(undefined)).toEqual([
'123',
'456',
]);
});
});
describe('isLastUnresolvedDiscussion', () => {
const localGetters = {
unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'],
};
it('should return true if the discussion id provided is the last', () => {
expect(getters.isLastUnresolvedDiscussion(state, localGetters)('789')).toBe(true);
});
it('should return false if the discussion id provided is not the last', () => {
expect(getters.isLastUnresolvedDiscussion(state, localGetters)('123')).toBe(false);
expect(getters.isLastUnresolvedDiscussion(state, localGetters)('456')).toBe(false);
});
});
describe('nextUnresolvedDiscussionId', () => {
const localGetters = {
unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'],
};
it('should return the ID of the discussion after the ID provided', () => {
expect(getters.nextUnresolvedDiscussionId(state, localGetters)('123')).toBe('456');
expect(getters.nextUnresolvedDiscussionId(state, localGetters)('456')).toBe('789');
expect(getters.nextUnresolvedDiscussionId(state, localGetters)('789')).toBe(undefined);
});
});
describe('firstUnresolvedDiscussionId', () => {
const localGetters = {
unresolvedDiscussionsIdsByDate: ['123', '456'],
unresolvedDiscussionsIdsByDiff: ['abc', 'def'],
};
it('should return the first discussion id by diff when diffOrder param is true', () => {
expect(getters.firstUnresolvedDiscussionId(state, localGetters)(true)).toBe('abc');
});
it('should return the first discussion id by date when diffOrder param is not true', () => {
expect(getters.firstUnresolvedDiscussionId(state, localGetters)(false)).toBe('123');
expect(getters.firstUnresolvedDiscussionId(state, localGetters)(undefined)).toBe('123');
});
it('should be falsy if all discussions are resolved', () => {
const localGettersFalsy = {
unresolvedDiscussionsIdsByDiff: [],
unresolvedDiscussionsIdsByDate: [],
};
expect(getters.firstUnresolvedDiscussionId(state, localGettersFalsy)(true)).toBeFalsy();
expect(getters.firstUnresolvedDiscussionId(state, localGettersFalsy)(false)).toBeFalsy();
});
});
});
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7