Impove diff discussion data
Pre-request to https://gitlab.com/gitlab-org/gitlab-ce/issues/48956
This commit is contained in:
parent
0baa0c4b14
commit
80a689fab8
|
@ -41,6 +41,11 @@ export default {
|
||||||
required: true,
|
required: true,
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
|
data() {
|
||||||
|
return {
|
||||||
|
assignedDiscussions: false,
|
||||||
|
};
|
||||||
|
},
|
||||||
computed: {
|
computed: {
|
||||||
...mapState({
|
...mapState({
|
||||||
isLoading: state => state.diffs.isLoading,
|
isLoading: state => state.diffs.isLoading,
|
||||||
|
@ -60,7 +65,7 @@ export default {
|
||||||
}),
|
}),
|
||||||
...mapState('diffs', ['showTreeList']),
|
...mapState('diffs', ['showTreeList']),
|
||||||
...mapGetters('diffs', ['isParallelView']),
|
...mapGetters('diffs', ['isParallelView']),
|
||||||
...mapGetters(['isNotesFetched', 'discussionsStructuredByLineCode']),
|
...mapGetters(['isNotesFetched', 'getNoteableData']),
|
||||||
targetBranch() {
|
targetBranch() {
|
||||||
return {
|
return {
|
||||||
branchName: this.targetBranchName,
|
branchName: this.targetBranchName,
|
||||||
|
@ -147,11 +152,12 @@ export default {
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
setDiscussions() {
|
setDiscussions() {
|
||||||
if (this.isNotesFetched) {
|
if (this.isNotesFetched && !this.assignedDiscussions) {
|
||||||
requestIdleCallback(
|
requestIdleCallback(
|
||||||
() => {
|
() =>
|
||||||
this.assignDiscussionsToDiff(this.discussionsStructuredByLineCode);
|
this.assignDiscussionsToDiff().then(() => {
|
||||||
},
|
this.assignedDiscussions = true;
|
||||||
|
}),
|
||||||
{ timeout: 1000 },
|
{ timeout: 1000 },
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
|
@ -29,7 +29,7 @@ export default {
|
||||||
},
|
},
|
||||||
computed: {
|
computed: {
|
||||||
...mapState('diffs', ['currentDiffFileId']),
|
...mapState('diffs', ['currentDiffFileId']),
|
||||||
...mapGetters(['isNotesFetched', 'discussionsStructuredByLineCode']),
|
...mapGetters(['isNotesFetched']),
|
||||||
isCollapsed() {
|
isCollapsed() {
|
||||||
return this.file.collapsed || false;
|
return this.file.collapsed || false;
|
||||||
},
|
},
|
||||||
|
@ -79,7 +79,7 @@ export default {
|
||||||
.then(() => {
|
.then(() => {
|
||||||
requestIdleCallback(
|
requestIdleCallback(
|
||||||
() => {
|
() => {
|
||||||
this.assignDiscussionsToDiff(this.discussionsStructuredByLineCode);
|
this.assignDiscussionsToDiff();
|
||||||
},
|
},
|
||||||
{ timeout: 1000 },
|
{ timeout: 1000 },
|
||||||
);
|
);
|
||||||
|
|
|
@ -5,7 +5,6 @@ import createFlash from '~/flash';
|
||||||
import { s__ } from '~/locale';
|
import { s__ } from '~/locale';
|
||||||
import { handleLocationHash, historyPushState } from '~/lib/utils/common_utils';
|
import { handleLocationHash, historyPushState } from '~/lib/utils/common_utils';
|
||||||
import { mergeUrlParams, getLocationHash } from '~/lib/utils/url_utility';
|
import { mergeUrlParams, getLocationHash } from '~/lib/utils/url_utility';
|
||||||
import { reduceDiscussionsToLineCodes } from '../../notes/stores/utils';
|
|
||||||
import { getDiffPositionByLineCode, getNoteFormData } from './utils';
|
import { getDiffPositionByLineCode, getNoteFormData } from './utils';
|
||||||
import * as types from './mutation_types';
|
import * as types from './mutation_types';
|
||||||
import {
|
import {
|
||||||
|
@ -36,18 +35,33 @@ export const fetchDiffFiles = ({ state, commit }) => {
|
||||||
|
|
||||||
// This is adding line discussions to the actual lines in the diff tree
|
// This is adding line discussions to the actual lines in the diff tree
|
||||||
// once for parallel and once for inline mode
|
// once for parallel and once for inline mode
|
||||||
export const assignDiscussionsToDiff = ({ state, commit }, allLineDiscussions) => {
|
export const assignDiscussionsToDiff = (
|
||||||
|
{ commit, state, rootState },
|
||||||
|
discussionsToAssign = rootState.notes.discussions,
|
||||||
|
) => {
|
||||||
const diffPositionByLineCode = getDiffPositionByLineCode(state.diffFiles);
|
const diffPositionByLineCode = getDiffPositionByLineCode(state.diffFiles);
|
||||||
|
const discussionsByDiffFile = discussionsToAssign
|
||||||
|
.filter(discussion => discussion.diff_discussion)
|
||||||
|
.reduce((acc, discussion) => {
|
||||||
|
const discussions = (
|
||||||
|
acc[discussion.diff_file.file_hash] || { discussions: [] }
|
||||||
|
).discussions.concat(discussion);
|
||||||
|
|
||||||
Object.values(allLineDiscussions).forEach(discussions => {
|
return {
|
||||||
if (discussions.length > 0) {
|
...acc,
|
||||||
const { fileHash } = discussions[0];
|
[discussion.diff_file.file_hash]: {
|
||||||
commit(types.SET_LINE_DISCUSSIONS_FOR_FILE, {
|
diffFile: state.diffFiles.find(file => file.fileHash === discussion.diff_file.file_hash),
|
||||||
fileHash,
|
discussions,
|
||||||
discussions,
|
},
|
||||||
diffPositionByLineCode,
|
};
|
||||||
});
|
}, {});
|
||||||
}
|
|
||||||
|
Object.values(discussionsByDiffFile).forEach(({ discussions, diffFile }) => {
|
||||||
|
commit(types.SET_LINE_DISCUSSIONS_FOR_FILE, {
|
||||||
|
diffFile,
|
||||||
|
discussions,
|
||||||
|
diffPositionByLineCode,
|
||||||
|
});
|
||||||
});
|
});
|
||||||
};
|
};
|
||||||
|
|
||||||
|
@ -190,9 +204,7 @@ export const saveDiffDiscussion = ({ dispatch }, { note, formData }) => {
|
||||||
|
|
||||||
return dispatch('saveNote', postData, { root: true })
|
return dispatch('saveNote', postData, { root: true })
|
||||||
.then(result => dispatch('updateDiscussion', result.discussion, { root: true }))
|
.then(result => dispatch('updateDiscussion', result.discussion, { root: true }))
|
||||||
.then(discussion =>
|
.then(discussion => dispatch('assignDiscussionsToDiff', [discussion]))
|
||||||
dispatch('assignDiscussionsToDiff', reduceDiscussionsToLineCodes([discussion])),
|
|
||||||
)
|
|
||||||
.catch(() => createFlash(s__('MergeRequests|Saving the comment failed')));
|
.catch(() => createFlash(s__('MergeRequests|Saving the comment failed')));
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|
|
@ -90,53 +90,50 @@ export default {
|
||||||
}));
|
}));
|
||||||
},
|
},
|
||||||
|
|
||||||
[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, discussions, diffPositionByLineCode }) {
|
[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { diffFile, discussions, diffPositionByLineCode }) {
|
||||||
const selectedFile = state.diffFiles.find(f => f.fileHash === fileHash);
|
const { latestDiff } = state;
|
||||||
const firstDiscussion = discussions[0];
|
|
||||||
const isDiffDiscussion = firstDiscussion.diff_discussion;
|
|
||||||
const hasLineCode = firstDiscussion.line_code;
|
|
||||||
const diffPosition = diffPositionByLineCode[firstDiscussion.line_code];
|
|
||||||
|
|
||||||
if (
|
discussions.forEach(discussion => {
|
||||||
selectedFile &&
|
const discussionLineCode = discussion.line_code;
|
||||||
isDiffDiscussion &&
|
const lineCheck = ({ lineCode }) =>
|
||||||
hasLineCode &&
|
lineCode === discussionLineCode &&
|
||||||
diffPosition &&
|
isDiscussionApplicableToLine({
|
||||||
isDiscussionApplicableToLine({
|
discussion,
|
||||||
discussion: firstDiscussion,
|
diffPosition: diffPositionByLineCode[lineCode],
|
||||||
diffPosition,
|
latestDiff,
|
||||||
latestDiff: state.latestDiff,
|
});
|
||||||
})
|
|
||||||
) {
|
if (diffFile.highlightedDiffLines) {
|
||||||
const targetLine = selectedFile.parallelDiffLines.find(
|
const line = diffFile.highlightedDiffLines.find(lineCheck);
|
||||||
line =>
|
|
||||||
(line.left && line.left.lineCode === firstDiscussion.line_code) ||
|
if (line) {
|
||||||
(line.right && line.right.lineCode === firstDiscussion.line_code),
|
Object.assign(line, {
|
||||||
);
|
discussions: line.discussions.concat(discussion),
|
||||||
if (targetLine) {
|
|
||||||
if (targetLine.left && targetLine.left.lineCode === firstDiscussion.line_code) {
|
|
||||||
Object.assign(targetLine.left, {
|
|
||||||
discussions,
|
|
||||||
});
|
|
||||||
} else {
|
|
||||||
Object.assign(targetLine.right, {
|
|
||||||
discussions,
|
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if (selectedFile.highlightedDiffLines) {
|
if (diffFile.parallelDiffLines) {
|
||||||
const targetInlineLine = selectedFile.highlightedDiffLines.find(
|
const { left, right } = diffFile.parallelDiffLines.find(
|
||||||
line => line.lineCode === firstDiscussion.line_code,
|
parallelLine =>
|
||||||
|
(parallelLine.left && lineCheck(parallelLine.left)) ||
|
||||||
|
(parallelLine.right && lineCheck(parallelLine.right)),
|
||||||
);
|
);
|
||||||
|
const line = left && left.lineCode === discussionLineCode ? left : right;
|
||||||
|
|
||||||
if (targetInlineLine) {
|
if (line) {
|
||||||
Object.assign(targetInlineLine, {
|
Object.assign(line, {
|
||||||
discussions,
|
discussions: line.discussions.concat(discussion),
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
|
||||||
|
if (!diffFile.parallelDiffLines || !diffFile.highlightedDiffLines) {
|
||||||
|
Object.assign(diffFile, {
|
||||||
|
discussions: diffFile.discussions.concat(discussion),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
});
|
||||||
},
|
},
|
||||||
|
|
||||||
[types.REMOVE_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, lineCode }) {
|
[types.REMOVE_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, lineCode }) {
|
||||||
|
|
|
@ -1,6 +1,5 @@
|
||||||
import _ from 'underscore';
|
import _ from 'underscore';
|
||||||
import * as constants from '../constants';
|
import * as constants from '../constants';
|
||||||
import { reduceDiscussionsToLineCodes } from './utils';
|
|
||||||
import { collapseSystemNotes } from './collapse_utils';
|
import { collapseSystemNotes } from './collapse_utils';
|
||||||
|
|
||||||
export const discussions = state => collapseSystemNotes(state.discussions);
|
export const discussions = state => collapseSystemNotes(state.discussions);
|
||||||
|
@ -31,9 +30,6 @@ export const notesById = state =>
|
||||||
return acc;
|
return acc;
|
||||||
}, {});
|
}, {});
|
||||||
|
|
||||||
export const discussionsStructuredByLineCode = state =>
|
|
||||||
reduceDiscussionsToLineCodes(state.discussions);
|
|
||||||
|
|
||||||
export const noteableType = state => {
|
export const noteableType = state => {
|
||||||
const { ISSUE_NOTEABLE_TYPE, MERGE_REQUEST_NOTEABLE_TYPE, EPIC_NOTEABLE_TYPE } = constants;
|
const { ISSUE_NOTEABLE_TYPE, MERGE_REQUEST_NOTEABLE_TYPE, EPIC_NOTEABLE_TYPE } = constants;
|
||||||
|
|
||||||
|
|
|
@ -25,18 +25,6 @@ export const getQuickActionText = note => {
|
||||||
return text;
|
return text;
|
||||||
};
|
};
|
||||||
|
|
||||||
export const reduceDiscussionsToLineCodes = selectedDiscussions =>
|
|
||||||
selectedDiscussions.reduce((acc, note) => {
|
|
||||||
if (note.diff_discussion && note.line_code) {
|
|
||||||
// For context about line notes: there might be multiple notes with the same line code
|
|
||||||
const items = acc[note.line_code] || [];
|
|
||||||
items.push(note);
|
|
||||||
|
|
||||||
Object.assign(acc, { [note.line_code]: items });
|
|
||||||
}
|
|
||||||
return acc;
|
|
||||||
}, {});
|
|
||||||
|
|
||||||
export const hasQuickActions = note => REGEX_QUICK_ACTIONS.test(note);
|
export const hasQuickActions = note => REGEX_QUICK_ACTIONS.test(note);
|
||||||
|
|
||||||
export const stripQuickActions = note => note.replace(REGEX_QUICK_ACTIONS, '').trim();
|
export const stripQuickActions = note => note.replace(REGEX_QUICK_ACTIONS, '').trim();
|
||||||
|
|
|
@ -27,7 +27,6 @@ import actions, {
|
||||||
toggleShowTreeList,
|
toggleShowTreeList,
|
||||||
} from '~/diffs/store/actions';
|
} from '~/diffs/store/actions';
|
||||||
import * as types from '~/diffs/store/mutation_types';
|
import * as types from '~/diffs/store/mutation_types';
|
||||||
import { reduceDiscussionsToLineCodes } from '~/notes/stores/utils';
|
|
||||||
import axios from '~/lib/utils/axios_utils';
|
import axios from '~/lib/utils/axios_utils';
|
||||||
import testAction from '../../helpers/vuex_action_helper';
|
import testAction from '../../helpers/vuex_action_helper';
|
||||||
|
|
||||||
|
@ -152,7 +151,7 @@ describe('DiffsStoreActions', () => {
|
||||||
original_position: diffPosition,
|
original_position: diffPosition,
|
||||||
};
|
};
|
||||||
|
|
||||||
const discussions = reduceDiscussionsToLineCodes([singleDiscussion]);
|
const discussions = [singleDiscussion];
|
||||||
|
|
||||||
testAction(
|
testAction(
|
||||||
assignDiscussionsToDiff,
|
assignDiscussionsToDiff,
|
||||||
|
@ -162,7 +161,7 @@ describe('DiffsStoreActions', () => {
|
||||||
{
|
{
|
||||||
type: types.SET_LINE_DISCUSSIONS_FOR_FILE,
|
type: types.SET_LINE_DISCUSSIONS_FOR_FILE,
|
||||||
payload: {
|
payload: {
|
||||||
fileHash: 'ABC',
|
diffFile: state.diffFiles[0],
|
||||||
discussions: [singleDiscussion],
|
discussions: [singleDiscussion],
|
||||||
diffPositionByLineCode: {
|
diffPositionByLineCode: {
|
||||||
ABC_1_1: {
|
ABC_1_1: {
|
||||||
|
@ -581,7 +580,6 @@ describe('DiffsStoreActions', () => {
|
||||||
describe('saveDiffDiscussion', () => {
|
describe('saveDiffDiscussion', () => {
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
spyOnDependency(actions, 'getNoteFormData').and.returnValue('testData');
|
spyOnDependency(actions, 'getNoteFormData').and.returnValue('testData');
|
||||||
spyOnDependency(actions, 'reduceDiscussionsToLineCodes').and.returnValue('discussions');
|
|
||||||
});
|
});
|
||||||
|
|
||||||
it('dispatches actions', done => {
|
it('dispatches actions', done => {
|
||||||
|
@ -602,7 +600,7 @@ describe('DiffsStoreActions', () => {
|
||||||
.then(() => {
|
.then(() => {
|
||||||
expect(dispatch.calls.argsFor(0)).toEqual(['saveNote', 'testData', { root: true }]);
|
expect(dispatch.calls.argsFor(0)).toEqual(['saveNote', 'testData', { root: true }]);
|
||||||
expect(dispatch.calls.argsFor(1)).toEqual(['updateDiscussion', 'test', { root: true }]);
|
expect(dispatch.calls.argsFor(1)).toEqual(['updateDiscussion', 'test', { root: true }]);
|
||||||
expect(dispatch.calls.argsFor(2)).toEqual(['assignDiscussionsToDiff', 'discussions']);
|
expect(dispatch.calls.argsFor(2)).toEqual(['assignDiscussionsToDiff', ['discussion']]);
|
||||||
})
|
})
|
||||||
.then(done)
|
.then(done)
|
||||||
.catch(done.fail);
|
.catch(done.fail);
|
||||||
|
|
|
@ -222,7 +222,7 @@ describe('DiffsStoreMutations', () => {
|
||||||
};
|
};
|
||||||
|
|
||||||
mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, {
|
mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, {
|
||||||
fileHash: 'ABC',
|
diffFile: state.diffFiles[0],
|
||||||
discussions,
|
discussions,
|
||||||
diffPositionByLineCode,
|
diffPositionByLineCode,
|
||||||
});
|
});
|
||||||
|
@ -292,7 +292,7 @@ describe('DiffsStoreMutations', () => {
|
||||||
};
|
};
|
||||||
|
|
||||||
mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, {
|
mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, {
|
||||||
fileHash: 'ABC',
|
diffFile: state.diffFiles[0],
|
||||||
discussions,
|
discussions,
|
||||||
diffPositionByLineCode,
|
diffPositionByLineCode,
|
||||||
});
|
});
|
||||||
|
|
Loading…
Reference in New Issue