Commit 719f46e2 authored by Filipa Lacerda's avatar Filipa Lacerda

Merge branch 'mr-diff-data' into 'master'

Impove diff discussion data

See merge request gitlab-org/gitlab-ce!22491
parents 9e6098a6 81944ec9
...@@ -41,6 +41,11 @@ export default { ...@@ -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,
...@@ -58,9 +63,9 @@ export default { ...@@ -58,9 +63,9 @@ export default {
plainDiffPath: state => state.diffs.plainDiffPath, plainDiffPath: state => state.diffs.plainDiffPath,
emailPatchPath: state => state.diffs.emailPatchPath, emailPatchPath: state => state.diffs.emailPatchPath,
}), }),
...mapState('diffs', ['showTreeList']), ...mapState('diffs', ['showTreeList', 'isLoading']),
...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 { ...@@ -147,11 +152,12 @@ export default {
} }
}, },
setDiscussions() { setDiscussions() {
if (this.isNotesFetched) { if (this.isNotesFetched && !this.assignedDiscussions && !this.isLoading) {
requestIdleCallback( requestIdleCallback(
() => { () =>
this.assignDiscussionsToDiff(this.discussionsStructuredByLineCode); this.assignDiscussionsToDiff().then(() => {
}, this.assignedDiscussions = true;
}),
{ timeout: 1000 }, { timeout: 1000 },
); );
} }
......
...@@ -29,7 +29,7 @@ export default { ...@@ -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 { ...@@ -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'; ...@@ -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,17 @@ export const fetchDiffFiles = ({ state, commit }) => { ...@@ -36,18 +35,17 @@ 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 },
discussions = rootState.notes.discussions,
) => {
const diffPositionByLineCode = getDiffPositionByLineCode(state.diffFiles); const diffPositionByLineCode = getDiffPositionByLineCode(state.diffFiles);
Object.values(allLineDiscussions).forEach(discussions => { discussions.filter(discussion => discussion.diff_discussion).forEach(discussion => {
if (discussions.length > 0) {
const { fileHash } = discussions[0];
commit(types.SET_LINE_DISCUSSIONS_FOR_FILE, { commit(types.SET_LINE_DISCUSSIONS_FOR_FILE, {
fileHash, discussion,
discussions,
diffPositionByLineCode, diffPositionByLineCode,
}); });
}
}); });
}; };
...@@ -190,9 +188,7 @@ export const saveDiffDiscussion = ({ dispatch }, { note, formData }) => { ...@@ -190,9 +188,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,67 @@ export default { ...@@ -90,53 +90,67 @@ export default {
})); }));
}, },
[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, discussions, diffPositionByLineCode }) { [types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { discussion, diffPositionByLineCode }) {
const selectedFile = state.diffFiles.find(f => f.fileHash === fileHash); const { latestDiff } = state;
const firstDiscussion = discussions[0];
const isDiffDiscussion = firstDiscussion.diff_discussion; const discussionLineCode = discussion.line_code;
const hasLineCode = firstDiscussion.line_code; const fileHash = discussion.diff_file.file_hash;
const diffPosition = diffPositionByLineCode[firstDiscussion.line_code]; const lineCheck = ({ lineCode }) =>
lineCode === discussionLineCode &&
if (
selectedFile &&
isDiffDiscussion &&
hasLineCode &&
diffPosition &&
isDiscussionApplicableToLine({ isDiscussionApplicableToLine({
discussion: firstDiscussion, discussion,
diffPosition, diffPosition: diffPositionByLineCode[lineCode],
latestDiff: state.latestDiff, latestDiff,
})
) {
const targetLine = selectedFile.parallelDiffLines.find(
line =>
(line.left && line.left.lineCode === firstDiscussion.line_code) ||
(line.right && line.right.lineCode === firstDiscussion.line_code),
);
if (targetLine) {
if (targetLine.left && targetLine.left.lineCode === firstDiscussion.line_code) {
Object.assign(targetLine.left, {
discussions,
});
} else {
Object.assign(targetLine.right, {
discussions,
}); });
state.diffFiles = state.diffFiles.map(diffFile => {
if (diffFile.fileHash === fileHash) {
const file = { ...diffFile };
if (file.highlightedDiffLines) {
file.highlightedDiffLines = file.highlightedDiffLines.map(line => {
if (lineCheck(line)) {
return {
...line,
discussions: line.discussions.concat(discussion),
};
} }
return line;
});
} }
if (selectedFile.highlightedDiffLines) { if (file.parallelDiffLines) {
const targetInlineLine = selectedFile.highlightedDiffLines.find( file.parallelDiffLines = file.parallelDiffLines.map(line => {
line => line.lineCode === firstDiscussion.line_code, const left = line.left && lineCheck(line.left);
); const right = line.right && lineCheck(line.right);
if (targetInlineLine) { if (left || right) {
Object.assign(targetInlineLine, { return {
discussions, left: {
...line.left,
discussions: left ? line.left.discussions.concat(discussion) : [],
},
right: {
...line.right,
discussions: right ? line.right.discussions.concat(discussion) : [],
},
};
}
return line;
}); });
} }
if (!file.parallelDiffLines || !file.highlightedDiffLines) {
file.discussions = file.discussions.concat(discussion);
} }
return file;
} }
return diffFile;
});
}, },
[types.REMOVE_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, lineCode }) { [types.REMOVE_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, lineCode }) {
......
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 => ...@@ -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 => { ...@@ -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, { ...@@ -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', () => { ...@@ -152,7 +151,7 @@ describe('DiffsStoreActions', () => {
original_position: diffPosition, original_position: diffPosition,
}; };
const discussions = reduceDiscussionsToLineCodes([singleDiscussion]); const discussions = [singleDiscussion];
testAction( testAction(
assignDiscussionsToDiff, assignDiscussionsToDiff,
...@@ -162,8 +161,7 @@ describe('DiffsStoreActions', () => { ...@@ -162,8 +161,7 @@ describe('DiffsStoreActions', () => {
{ {
type: types.SET_LINE_DISCUSSIONS_FOR_FILE, type: types.SET_LINE_DISCUSSIONS_FOR_FILE,
payload: { payload: {
fileHash: 'ABC', discussion: singleDiscussion,
discussions: [singleDiscussion],
diffPositionByLineCode: { diffPositionByLineCode: {
ABC_1_1: { ABC_1_1: {
baseSha: 'abc', baseSha: 'abc',
...@@ -581,7 +579,6 @@ describe('DiffsStoreActions', () => { ...@@ -581,7 +579,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 +599,7 @@ describe('DiffsStoreActions', () => { ...@@ -602,7 +599,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);
......
...@@ -198,40 +198,32 @@ describe('DiffsStoreMutations', () => { ...@@ -198,40 +198,32 @@ describe('DiffsStoreMutations', () => {
}, },
], ],
}; };
const discussions = [ const discussion = {
{
id: 1, id: 1,
line_code: 'ABC_1', line_code: 'ABC_1',
diff_discussion: true, diff_discussion: true,
resolvable: true, resolvable: true,
original_position: diffPosition, original_position: diffPosition,
position: diffPosition, position: diffPosition,
diff_file: {
file_hash: state.diffFiles[0].fileHash,
}, },
{ };
id: 2,
line_code: 'ABC_1',
diff_discussion: true,
resolvable: true,
original_position: diffPosition,
position: diffPosition,
},
];
const diffPositionByLineCode = { const diffPositionByLineCode = {
ABC_1: diffPosition, ABC_1: diffPosition,
}; };
mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, {
fileHash: 'ABC', discussion,
discussions,
diffPositionByLineCode, diffPositionByLineCode,
}); });
expect(state.diffFiles[0].parallelDiffLines[0].left.discussions.length).toEqual(2); expect(state.diffFiles[0].parallelDiffLines[0].left.discussions.length).toEqual(1);
expect(state.diffFiles[0].parallelDiffLines[0].left.discussions[1].id).toEqual(2); expect(state.diffFiles[0].parallelDiffLines[0].left.discussions[0].id).toEqual(1);
expect(state.diffFiles[0].highlightedDiffLines[0].discussions.length).toEqual(2); expect(state.diffFiles[0].highlightedDiffLines[0].discussions.length).toEqual(1);
expect(state.diffFiles[0].highlightedDiffLines[0].discussions[1].id).toEqual(2); expect(state.diffFiles[0].highlightedDiffLines[0].discussions[0].id).toEqual(1);
}); });
it('should add legacy discussions to the given line', () => { it('should add legacy discussions to the given line', () => {
...@@ -272,36 +264,30 @@ describe('DiffsStoreMutations', () => { ...@@ -272,36 +264,30 @@ describe('DiffsStoreMutations', () => {
}, },
], ],
}; };
const discussions = [ const discussion = {
{
id: 1, id: 1,
line_code: 'ABC_1', line_code: 'ABC_1',
diff_discussion: true, diff_discussion: true,
active: true, active: true,
diff_file: {
file_hash: state.diffFiles[0].fileHash,
}, },
{ };
id: 2,
line_code: 'ABC_1',
diff_discussion: true,
active: true,
},
];
const diffPositionByLineCode = { const diffPositionByLineCode = {
ABC_1: diffPosition, ABC_1: diffPosition,
}; };
mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, {
fileHash: 'ABC', discussion,
discussions,
diffPositionByLineCode, diffPositionByLineCode,
}); });
expect(state.diffFiles[0].parallelDiffLines[0].left.discussions.length).toEqual(2); expect(state.diffFiles[0].parallelDiffLines[0].left.discussions.length).toEqual(1);
expect(state.diffFiles[0].parallelDiffLines[0].left.discussions[1].id).toEqual(2); expect(state.diffFiles[0].parallelDiffLines[0].left.discussions[0].id).toEqual(1);
expect(state.diffFiles[0].highlightedDiffLines[0].discussions.length).toEqual(2); expect(state.diffFiles[0].highlightedDiffLines[0].discussions.length).toEqual(1);
expect(state.diffFiles[0].highlightedDiffLines[0].discussions[1].id).toEqual(2); expect(state.diffFiles[0].highlightedDiffLines[0].discussions[0].id).toEqual(1);
}); });
}); });
......
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