From 04c0d12d1a6cfaa54d2e5f510922b9d27c5c0a77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Lu=C3=ADs?= Date: Sat, 8 Sep 2018 06:37:41 +0000 Subject: [PATCH] Resolve "Merge requests show outdated discussions on changes tab" --- app/assets/javascripts/diffs/store/actions.js | 11 +++- .../javascripts/diffs/store/mutations.js | 19 +++++-- app/assets/javascripts/diffs/store/utils.js | 11 +++- app/serializers/discussion_entity.rb | 1 + ...outdated-discussions-new-datastructure.yml | 5 ++ spec/javascripts/diffs/store/actions_spec.js | 38 ++++++++++++- .../javascripts/diffs/store/mutations_spec.js | 36 ++++++++++++- spec/javascripts/diffs/store/utils_spec.js | 53 +++++++++++++++++++ 8 files changed, 166 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/48167-fix-outdated-discussions-new-datastructure.yml diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 184a90c6033..027df2ec841 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -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, + }); } }); }; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 676c4dab1dd..6dc5bf16c65 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -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) || diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 6b1659a412c..4139a999574 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -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); +} diff --git a/app/serializers/discussion_entity.rb b/app/serializers/discussion_entity.rb index ed09db0f3f4..ebe76c9fcda 100644 --- a/app/serializers/discussion_entity.rb +++ b/app/serializers/discussion_entity.rb @@ -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? } diff --git a/changelogs/unreleased/48167-fix-outdated-discussions-new-datastructure.yml b/changelogs/unreleased/48167-fix-outdated-discussions-new-datastructure.yml new file mode 100644 index 00000000000..a8fcce2eeb8 --- /dev/null +++ b/changelogs/unreleased/48167-fix-outdated-discussions-new-datastructure.yml @@ -0,0 +1,5 @@ +--- +title: Fix outdated discussions being shown on Merge Request Changes tab +merge_request: 21543 +author: +type: fixed diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index c162fc965ba..cfb8f862598 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -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', + }, + }, }, }, ], diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js index 9a89bc57404..7eeca6712cc 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/spec/javascripts/diffs/store/mutations_spec.js @@ -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); diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index bd9d63769a1..1c580580582 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -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); + }); + }); });