Merge branch '48167-fix-outdated-discussions-new-datastructure' into 'master'

Resolve "Merge requests show outdated discussions on changes tab"

Closes #48167

See merge request gitlab-org/gitlab-ce!21543

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,
+      });
     }
   });
 };

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 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) ||

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);
+}

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? }

changelogs/unreleased/48167-fix-outdated-discussions-new-datastructure.yml

+---
+title: Fix outdated discussions being shown on Merge Request Changes tab
+merge_request: 21543
+author:
+type: fixed

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',
+                },
+              },
             },
           },
         ],

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);

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);
+    });
+  });
 });