kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Resolve "Merge requests show outdated discussions on changes tab"

Browse source
This commit is contained in:
André Luís 2018-09-08 06:37:41 +00:00 committed by Tim Zallmann
parent 5949f55235
commit 04c0d12d1a
8 changed files with 166 additions and 8 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

11
app/assets/javascripts/diffs/store/actions.js
View file

@ -3,6 +3,7 @@ import axios from '~/lib/utils/axios_utils';
import Cookies from 'js-cookie';
import { handleLocationHash, historyPushState } from '~/lib/utils/common_utils';
import { mergeUrlParams } from '~/lib/utils/url_utility';
import { getDiffPositionByLineCode } from './utils';
import * as types from './mutation_types';
import {
PARALLEL_DIFF_VIEW_TYPE,
@ -31,11 +32,17 @@ 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 = ({ commit }, allLineDiscussions) => {
export const assignDiscussionsToDiff = ({ state, commit }, allLineDiscussions) => {
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 });
commit(types.SET_LINE_DISCUSSIONS_FOR_FILE, {
fileHash,
discussions,
diffPositionByLineCode,
});
}
});
};

19
app/assets/javascripts/diffs/store/mutations.js
View file

@ -6,6 +6,7 @@ import {
removeMatchLine,
addContextLines,
prepareDiffData,
isDiscussionApplicableToLine,
} from './utils';
import * as types from './mutation_types';
@ -84,10 +85,22 @@ export default {
}));
},
[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, discussions }) {
[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, discussions, diffPositionByLineCode }) {
const selectedFile = state.diffFiles.find(f => f.fileHash === fileHash);
if (selectedFile) {
const firstDiscussion = discussions[0];
const firstDiscussion = discussions[0];
const isDiffDiscussion = firstDiscussion.diff_discussion;
const hasLineCode = firstDiscussion.line_code;
const isResolvable = firstDiscussion.resolvable;
const diffPosition = diffPositionByLineCode[firstDiscussion.line_code];
if (
selectedFile &&
isDiffDiscussion &&
hasLineCode &&
isResolvable &&
diffPosition &&
isDiscussionApplicableToLine(firstDiscussion, diffPosition)
) {
const targetLine = selectedFile.parallelDiffLines.find(
line =>
(line.left && line.left.lineCode === firstDiscussion.line_code) ||

11
app/assets/javascripts/diffs/store/utils.js
View file

@ -217,7 +217,7 @@ export function prepareDiffData(diffData) {
}
}
export function getDiffRefsByLineCode(diffFiles) {
export function getDiffPositionByLineCode(diffFiles) {
return diffFiles.reduce((acc, diffFile) => {
const { baseSha, headSha, startSha } = diffFile.diffRefs;
const { newPath, oldPath } = diffFile;
@ -237,3 +237,12 @@ export function getDiffRefsByLineCode(diffFiles) {
return acc;
}, {});
}
// This method will check whether the discussion is still applicable
// to the diff line in question regarding different versions of the MR
export function isDiscussionApplicableToLine(discussion, diffPosition) {
const originalRefs = convertObjectPropsToCamelCase(discussion.original_position.formatter);
const refs = convertObjectPropsToCamelCase(discussion.position.formatter);
return _.isEqual(refs, diffPosition) || _.isEqual(originalRefs, diffPosition);
}

1
app/serializers/discussion_entity.rb
View file

@ -6,6 +6,7 @@ class DiscussionEntity < Grape::Entity
expose :id, :reply_id
expose :position, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? }
expose :original_position, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? }
expose :line_code, if: -> (d, _) { d.diff_discussion? }
expose :expanded?, as: :expanded
expose :active?, as: :active, if: -> (d, _) { d.diff_discussion? }

5
changelogs/unreleased/48167-fix-outdated-discussions-new-datastructure.yml Normal file
View file

@ -0,0 +1,5 @@
---
title: Fix outdated discussions being shown on Merge Request Changes tab
merge_request: 21543
author:
type: fixed

38
spec/javascripts/diffs/store/actions_spec.js
View file

@ -95,20 +95,45 @@ describe('DiffsStoreActions', () => {
{
lineCode: 'ABC_1_1',
discussions: [],
oldLine: 5,
newLine: null,
},
],
diffRefs: {
baseSha: 'abc',
headSha: 'def',
startSha: 'ghi',
},
newPath: 'file1',
oldPath: 'file2',
},
],
};
const diffPosition = {
baseSha: 'abc',
headSha: 'def',
startSha: 'ghi',
newLine: null,
newPath: 'file1',
oldLine: 5,
oldPath: 'file2',
};
const singleDiscussion = {
line_code: 'ABC_1_1',
diff_discussion: {},
diff_file: {
file_hash: 'ABC',
},
resolvable: true,
fileHash: 'ABC',
resolvable: true,
position: {
formatter: diffPosition,
},
original_position: {
formatter: diffPosition,
},
};
const discussions = reduceDiscussionsToLineCodes([singleDiscussion]);
@ -123,6 +148,17 @@ describe('DiffsStoreActions', () => {
payload: {
fileHash: 'ABC',
discussions: [singleDiscussion],
diffPositionByLineCode: {
ABC_1_1: {
baseSha: 'abc',
headSha: 'def',
startSha: 'ghi',
newLine: null,
newPath: 'file1',
oldLine: 5,
oldPath: 'file2',
},
},
},
},
],

36
spec/javascripts/diffs/store/mutations_spec.js
View file

@ -151,6 +151,16 @@ describe('DiffsStoreMutations', () => {
describe('SET_LINE_DISCUSSIONS_FOR_FILE', () => {
it('should add discussions to the given line', () => {
const diffPosition = {
baseSha: 'ed13df29948c41ba367caa757ab3ec4892509910',
headSha: 'b921914f9a834ac47e6fd9420f78db0f83559130',
newLine: null,
newPath: '500-lines-4.txt',
oldLine: 5,
oldPath: '500-lines-4.txt',
startSha: 'ed13df29948c41ba367caa757ab3ec4892509910',
};
const state = {
diffFiles: [
{
@ -180,14 +190,38 @@ describe('DiffsStoreMutations', () => {
{
id: 1,
line_code: 'ABC_1',
diff_discussion: true,
resolvable: true,
original_position: {
formatter: diffPosition,
},
position: {
formatter: diffPosition,
},
},
{
id: 2,
line_code: 'ABC_1',
diff_discussion: true,
resolvable: true,
original_position: {
formatter: diffPosition,
},
position: {
formatter: diffPosition,
},
},
];
mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash: 'ABC', discussions });
const diffPositionByLineCode = {
ABC_1: diffPosition,
};
mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, {
fileHash: 'ABC',
discussions,
diffPositionByLineCode,
});
expect(state.diffFiles[0].parallelDiffLines[0].left.discussions.length).toEqual(2);
expect(state.diffFiles[0].parallelDiffLines[0].left.discussions[1].id).toEqual(2);

53
spec/javascripts/diffs/store/utils_spec.js
View file

@ -239,4 +239,57 @@ describe('DiffsStoreUtils', () => {
expect(preparedDiff.diffFiles[0].collapsed).toBeFalsy();
});
});
describe('isDiscussionApplicableToLine', () => {
const diffPosition = {
baseSha: 'ed13df29948c41ba367caa757ab3ec4892509910',
headSha: 'b921914f9a834ac47e6fd9420f78db0f83559130',
newLine: null,
newPath: '500-lines-4.txt',
oldLine: 5,
oldPath: '500-lines-4.txt',
startSha: 'ed13df29948c41ba367caa757ab3ec4892509910',
};
const wrongDiffPosition = {
baseSha: 'wrong',
headSha: 'wrong',
newLine: null,
newPath: '500-lines-4.txt',
oldLine: 5,
oldPath: '500-lines-4.txt',
startSha: 'wrong',
};
const discussions = {
upToDateDiscussion1: {
original_position: {
formatter: diffPosition,
},
position: {
formatter: wrongDiffPosition,
},
},
outDatedDiscussion1: {
original_position: {
formatter: wrongDiffPosition,
},
position: {
formatter: wrongDiffPosition,
},
},
};
it('returns true when the discussion is up to date', () => {
expect(
utils.isDiscussionApplicableToLine(discussions.upToDateDiscussion1, diffPosition),
).toBe(true);
});
it('returns false when the discussion is not up to date', () => {
expect(
utils.isDiscussionApplicableToLine(discussions.outDatedDiscussion1, diffPosition),
).toBe(false);
});
});
});