diff options
author | André Luís <me@andr3.net> | 2018-09-08 09:37:41 +0300 |
---|---|---|
committer | Tim Zallmann <tzallmann@gitlab.com> | 2018-09-08 09:37:41 +0300 |
commit | 04c0d12d1a6cfaa54d2e5f510922b9d27c5c0a77 (patch) | |
tree | a633c79637f18b84b16f67dfca296ac0c1939185 /app | |
parent | 5949f55235da76eac6e204916502843a87a33d97 (diff) |
Resolve "Merge requests show outdated discussions on changes tab"
Diffstat (limited to 'app')
-rw-r--r-- | app/assets/javascripts/diffs/store/actions.js | 11 | ||||
-rw-r--r-- | app/assets/javascripts/diffs/store/mutations.js | 19 | ||||
-rw-r--r-- | app/assets/javascripts/diffs/store/utils.js | 11 | ||||
-rw-r--r-- | app/serializers/discussion_entity.rb | 1 |
4 files changed, 36 insertions, 6 deletions
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? } |