summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPhil Hughes <me@iamphill.com>2018-10-19 14:09:16 +0100
committerPhil Hughes <me@iamphill.com>2018-10-24 13:30:30 +0100
commit80a689fab83861aa412a14983411710c3f04fb15 (patch)
treeeb98af8f02ac4c003adbb28e56bfb4e6dafebe1f
parent0baa0c4b14073e1fced374ecf8a2d76d01efd729 (diff)
downloadgitlab-ce-mr-diff-data.tar.gz
Impove diff discussion datamr-diff-data
Pre-request to https://gitlab.com/gitlab-org/gitlab-ce/issues/48956
-rw-r--r--app/assets/javascripts/diffs/components/app.vue16
-rw-r--r--app/assets/javascripts/diffs/components/diff_file.vue4
-rw-r--r--app/assets/javascripts/diffs/store/actions.js42
-rw-r--r--app/assets/javascripts/diffs/store/mutations.js73
-rw-r--r--app/assets/javascripts/notes/stores/getters.js4
-rw-r--r--app/assets/javascripts/notes/stores/utils.js12
-rw-r--r--spec/javascripts/diffs/store/actions_spec.js8
-rw-r--r--spec/javascripts/diffs/store/mutations_spec.js4
8 files changed, 80 insertions, 83 deletions
diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue
index edca45f22f9..9ddca8548e0 100644
--- a/app/assets/javascripts/diffs/components/app.vue
+++ b/app/assets/javascripts/diffs/components/app.vue
@@ -41,6 +41,11 @@ export default {
required: true,
},
},
+ data() {
+ return {
+ assignedDiscussions: false,
+ };
+ },
computed: {
...mapState({
isLoading: state => state.diffs.isLoading,
@@ -60,7 +65,7 @@ export default {
}),
...mapState('diffs', ['showTreeList']),
...mapGetters('diffs', ['isParallelView']),
- ...mapGetters(['isNotesFetched', 'discussionsStructuredByLineCode']),
+ ...mapGetters(['isNotesFetched', 'getNoteableData']),
targetBranch() {
return {
branchName: this.targetBranchName,
@@ -147,11 +152,12 @@ export default {
}
},
setDiscussions() {
- if (this.isNotesFetched) {
+ if (this.isNotesFetched && !this.assignedDiscussions) {
requestIdleCallback(
- () => {
- this.assignDiscussionsToDiff(this.discussionsStructuredByLineCode);
- },
+ () =>
+ this.assignDiscussionsToDiff().then(() => {
+ this.assignedDiscussions = true;
+ }),
{ timeout: 1000 },
);
}
diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue
index f72c7a84e5c..958e57c5652 100644
--- a/app/assets/javascripts/diffs/components/diff_file.vue
+++ b/app/assets/javascripts/diffs/components/diff_file.vue
@@ -29,7 +29,7 @@ export default {
},
computed: {
...mapState('diffs', ['currentDiffFileId']),
- ...mapGetters(['isNotesFetched', 'discussionsStructuredByLineCode']),
+ ...mapGetters(['isNotesFetched']),
isCollapsed() {
return this.file.collapsed || false;
},
@@ -79,7 +79,7 @@ export default {
.then(() => {
requestIdleCallback(
() => {
- this.assignDiscussionsToDiff(this.discussionsStructuredByLineCode);
+ this.assignDiscussionsToDiff();
},
{ timeout: 1000 },
);
diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js
index 1e0b27b538d..e79351454ab 100644
--- a/app/assets/javascripts/diffs/store/actions.js
+++ b/app/assets/javascripts/diffs/store/actions.js
@@ -5,7 +5,6 @@ import createFlash from '~/flash';
import { s__ } from '~/locale';
import { handleLocationHash, historyPushState } from '~/lib/utils/common_utils';
import { mergeUrlParams, getLocationHash } from '~/lib/utils/url_utility';
-import { reduceDiscussionsToLineCodes } from '../../notes/stores/utils';
import { getDiffPositionByLineCode, getNoteFormData } from './utils';
import * as types from './mutation_types';
import {
@@ -36,18 +35,33 @@ export const fetchDiffFiles = ({ state, commit }) => {
// This is adding line discussions to the actual lines in the diff tree
// 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);
-
- Object.values(allLineDiscussions).forEach(discussions => {
- if (discussions.length > 0) {
- const { fileHash } = discussions[0];
- commit(types.SET_LINE_DISCUSSIONS_FOR_FILE, {
- fileHash,
- discussions,
- diffPositionByLineCode,
- });
- }
+ const discussionsByDiffFile = discussionsToAssign
+ .filter(discussion => discussion.diff_discussion)
+ .reduce((acc, discussion) => {
+ const discussions = (
+ acc[discussion.diff_file.file_hash] || { discussions: [] }
+ ).discussions.concat(discussion);
+
+ return {
+ ...acc,
+ [discussion.diff_file.file_hash]: {
+ diffFile: state.diffFiles.find(file => file.fileHash === discussion.diff_file.file_hash),
+ discussions,
+ },
+ };
+ }, {});
+
+ 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 })
.then(result => dispatch('updateDiscussion', result.discussion, { root: true }))
- .then(discussion =>
- dispatch('assignDiscussionsToDiff', reduceDiscussionsToLineCodes([discussion])),
- )
+ .then(discussion => dispatch('assignDiscussionsToDiff', [discussion]))
.catch(() => createFlash(s__('MergeRequests|Saving the comment failed')));
};
diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js
index 0b4485ecdb5..6a57a075579 100644
--- a/app/assets/javascripts/diffs/store/mutations.js
+++ b/app/assets/javascripts/diffs/store/mutations.js
@@ -90,53 +90,50 @@ export default {
}));
},
- [types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, discussions, diffPositionByLineCode }) {
- const selectedFile = state.diffFiles.find(f => f.fileHash === fileHash);
- const firstDiscussion = discussions[0];
- const isDiffDiscussion = firstDiscussion.diff_discussion;
- const hasLineCode = firstDiscussion.line_code;
- const diffPosition = diffPositionByLineCode[firstDiscussion.line_code];
-
- if (
- selectedFile &&
- isDiffDiscussion &&
- hasLineCode &&
- diffPosition &&
- isDiscussionApplicableToLine({
- discussion: firstDiscussion,
- diffPosition,
- latestDiff: state.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,
+ [types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { diffFile, discussions, diffPositionByLineCode }) {
+ const { latestDiff } = state;
+
+ discussions.forEach(discussion => {
+ const discussionLineCode = discussion.line_code;
+ const lineCheck = ({ lineCode }) =>
+ lineCode === discussionLineCode &&
+ isDiscussionApplicableToLine({
+ discussion,
+ diffPosition: diffPositionByLineCode[lineCode],
+ latestDiff,
+ });
+
+ if (diffFile.highlightedDiffLines) {
+ const line = diffFile.highlightedDiffLines.find(lineCheck);
+
+ if (line) {
+ Object.assign(line, {
+ discussions: line.discussions.concat(discussion),
});
}
}
- if (selectedFile.highlightedDiffLines) {
- const targetInlineLine = selectedFile.highlightedDiffLines.find(
- line => line.lineCode === firstDiscussion.line_code,
+ if (diffFile.parallelDiffLines) {
+ const { left, right } = diffFile.parallelDiffLines.find(
+ parallelLine =>
+ (parallelLine.left && lineCheck(parallelLine.left)) ||
+ (parallelLine.right && lineCheck(parallelLine.right)),
);
+ const line = left && left.lineCode === discussionLineCode ? left : right;
- if (targetInlineLine) {
- Object.assign(targetInlineLine, {
- discussions,
+ if (line) {
+ Object.assign(line, {
+ 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 }) {
diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js
index 21c334a9d33..e4f36154fcd 100644
--- a/app/assets/javascripts/notes/stores/getters.js
+++ b/app/assets/javascripts/notes/stores/getters.js
@@ -1,6 +1,5 @@
import _ from 'underscore';
import * as constants from '../constants';
-import { reduceDiscussionsToLineCodes } from './utils';
import { collapseSystemNotes } from './collapse_utils';
export const discussions = state => collapseSystemNotes(state.discussions);
@@ -31,9 +30,6 @@ export const notesById = state =>
return acc;
}, {});
-export const discussionsStructuredByLineCode = state =>
- reduceDiscussionsToLineCodes(state.discussions);
-
export const noteableType = state => {
const { ISSUE_NOTEABLE_TYPE, MERGE_REQUEST_NOTEABLE_TYPE, EPIC_NOTEABLE_TYPE } = constants;
diff --git a/app/assets/javascripts/notes/stores/utils.js b/app/assets/javascripts/notes/stores/utils.js
index 0e41ff03d67..dd57539e4d8 100644
--- a/app/assets/javascripts/notes/stores/utils.js
+++ b/app/assets/javascripts/notes/stores/utils.js
@@ -25,18 +25,6 @@ export const getQuickActionText = note => {
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 stripQuickActions = note => note.replace(REGEX_QUICK_ACTIONS, '').trim();
diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js
index 85c1926fcb1..12db3cc6a25 100644
--- a/spec/javascripts/diffs/store/actions_spec.js
+++ b/spec/javascripts/diffs/store/actions_spec.js
@@ -27,7 +27,6 @@ import actions, {
toggleShowTreeList,
} from '~/diffs/store/actions';
import * as types from '~/diffs/store/mutation_types';
-import { reduceDiscussionsToLineCodes } from '~/notes/stores/utils';
import axios from '~/lib/utils/axios_utils';
import testAction from '../../helpers/vuex_action_helper';
@@ -152,7 +151,7 @@ describe('DiffsStoreActions', () => {
original_position: diffPosition,
};
- const discussions = reduceDiscussionsToLineCodes([singleDiscussion]);
+ const discussions = [singleDiscussion];
testAction(
assignDiscussionsToDiff,
@@ -162,7 +161,7 @@ describe('DiffsStoreActions', () => {
{
type: types.SET_LINE_DISCUSSIONS_FOR_FILE,
payload: {
- fileHash: 'ABC',
+ diffFile: state.diffFiles[0],
discussions: [singleDiscussion],
diffPositionByLineCode: {
ABC_1_1: {
@@ -581,7 +580,6 @@ describe('DiffsStoreActions', () => {
describe('saveDiffDiscussion', () => {
beforeEach(() => {
spyOnDependency(actions, 'getNoteFormData').and.returnValue('testData');
- spyOnDependency(actions, 'reduceDiscussionsToLineCodes').and.returnValue('discussions');
});
it('dispatches actions', done => {
@@ -602,7 +600,7 @@ describe('DiffsStoreActions', () => {
.then(() => {
expect(dispatch.calls.argsFor(0)).toEqual(['saveNote', 'testData', { 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)
.catch(done.fail);
diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js
index b7e28391419..3a9c8f3a825 100644
--- a/spec/javascripts/diffs/store/mutations_spec.js
+++ b/spec/javascripts/diffs/store/mutations_spec.js
@@ -222,7 +222,7 @@ describe('DiffsStoreMutations', () => {
};
mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, {
- fileHash: 'ABC',
+ diffFile: state.diffFiles[0],
discussions,
diffPositionByLineCode,
});
@@ -292,7 +292,7 @@ describe('DiffsStoreMutations', () => {
};
mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, {
- fileHash: 'ABC',
+ diffFile: state.diffFiles[0],
discussions,
diffPositionByLineCode,
});