Merge branch 'fixed-diff-notes-not-showing-ce' into 'master'

Fixed notes not being applied to diff lines

Closes #55245 and #55248

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

2 files changed, 102 insertions, 28 deletions

diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js
index 61314db1dbd..2ea884d1293 100644
--- a/app/assets/javascripts/diffs/store/mutations.js
+++ b/app/assets/javascripts/diffs/store/mutations.js
@@ -123,22 +123,23 @@ export default {
         diffPosition: diffPositionByLineCode[line.line_code],
         latestDiff,
       });
+    const mapDiscussions = (line, extraCheck = () => true) => ({
+      ...line,
+      discussions: extraCheck()
+        ? line.discussions
+            .filter(() => !line.discussions.some(({ id }) => discussion.id === id))
+            .concat(lineCheck(line) ? discussion : line.discussions)
+        : [],
+    });
 
     state.diffFiles = state.diffFiles.map(diffFile => {
       if (diffFile.file_hash === fileHash) {
         const file = { ...diffFile };
 
         if (file.highlighted_diff_lines) {
-          file.highlighted_diff_lines = file.highlighted_diff_lines.map(line => {
-            if (!line.discussions.some(({ id }) => discussion.id === id) && lineCheck(line)) {
-              return {
-                ...line,
-                discussions: line.discussions.concat(discussion),
-              };
-            }
-
-            return line;
-          });
+          file.highlighted_diff_lines = file.highlighted_diff_lines.map(line =>
+            mapDiscussions(line),
+          );
         }
 
         if (file.parallel_diff_lines) {
@@ -148,20 +149,8 @@ export default {
 
             if (left || right) {
               return {
-                left: {
-                  ...line.left,
-                  discussions:
-                    left && !line.left.discussions.some(({ id }) => id === discussion.id)
-                      ? line.left.discussions.concat(discussion)
-                      : (line.left && line.left.discussions) || [],
-                },
-                right: {
-                  ...line.right,
-                  discussions:
-                    right && !left && !line.right.discussions.some(({ id }) => id === discussion.id)
-                      ? line.right.discussions.concat(discussion)
-                      : (line.right && line.right.discussions) || [],
-                },
+                left: line.left ? mapDiscussions(line.left) : null,
+                right: line.right ? mapDiscussions(line.right, () => !left) : null,
               };
             }
 
@@ -180,7 +169,7 @@ export default {
     });
   },
 
-  [types.REMOVE_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, lineCode, id }) {
+  [types.REMOVE_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, lineCode }) {
     const selectedFile = state.diffFiles.find(f => f.file_hash === fileHash);
     if (selectedFile) {
       if (selectedFile.parallel_diff_lines) {
@@ -193,7 +182,7 @@ export default {
           const side = targetLine.left && targetLine.left.line_code === lineCode ? 'left' : 'right';
 
           Object.assign(targetLine[side], {
-            discussions: [],
+            discussions: targetLine[side].discussions.filter(discussion => discussion.notes.length),
           });
         }
       }
@@ -205,14 +194,14 @@ export default {
 
         if (targetInlineLine) {
           Object.assign(targetInlineLine, {
-            discussions: [],
+            discussions: targetInlineLine.discussions.filter(discussion => discussion.notes.length),
           });
         }
       }
 
       if (selectedFile.discussions && selectedFile.discussions.length) {
         selectedFile.discussions = selectedFile.discussions.filter(
-          discussion => discussion.id !== id,
+          discussion => discussion.notes.length,
         );
       }
     }
diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js
index 23e8761bc55..f3449bec6ec 100644
--- a/spec/javascripts/diffs/store/mutations_spec.js
+++ b/spec/javascripts/diffs/store/mutations_spec.js
@@ -277,6 +277,87 @@ describe('DiffsStoreMutations', () => {
       expect(state.diffFiles[0].highlighted_diff_lines[0].discussions[0].id).toEqual(1);
     });
 
+    it('updates existing discussion', () => {
+      const diffPosition = {
+        base_sha: 'ed13df29948c41ba367caa757ab3ec4892509910',
+        head_sha: 'b921914f9a834ac47e6fd9420f78db0f83559130',
+        new_line: null,
+        new_path: '500-lines-4.txt',
+        old_line: 5,
+        old_path: '500-lines-4.txt',
+        start_sha: 'ed13df29948c41ba367caa757ab3ec4892509910',
+      };
+
+      const state = {
+        latestDiff: true,
+        diffFiles: [
+          {
+            file_hash: 'ABC',
+            parallel_diff_lines: [
+              {
+                left: {
+                  line_code: 'ABC_1',
+                  discussions: [],
+                },
+                right: {
+                  line_code: 'ABC_1',
+                  discussions: [],
+                },
+              },
+            ],
+            highlighted_diff_lines: [
+              {
+                line_code: 'ABC_1',
+                discussions: [],
+              },
+            ],
+          },
+        ],
+      };
+      const discussion = {
+        id: 1,
+        line_code: 'ABC_1',
+        diff_discussion: true,
+        resolvable: true,
+        original_position: diffPosition,
+        position: diffPosition,
+        diff_file: {
+          file_hash: state.diffFiles[0].file_hash,
+        },
+      };
+
+      const diffPositionByLineCode = {
+        ABC_1: diffPosition,
+      };
+
+      mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, {
+        discussion,
+        diffPositionByLineCode,
+      });
+
+      expect(state.diffFiles[0].parallel_diff_lines[0].left.discussions.length).toEqual(1);
+      expect(state.diffFiles[0].parallel_diff_lines[0].left.discussions[0].id).toEqual(1);
+      expect(state.diffFiles[0].parallel_diff_lines[0].right.discussions).toEqual([]);
+
+      expect(state.diffFiles[0].highlighted_diff_lines[0].discussions.length).toEqual(1);
+      expect(state.diffFiles[0].highlighted_diff_lines[0].discussions[0].id).toEqual(1);
+
+      mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, {
+        discussion: {
+          ...discussion,
+          resolved: true,
+          notes: ['test'],
+        },
+        diffPositionByLineCode,
+      });
+
+      expect(state.diffFiles[0].parallel_diff_lines[0].left.discussions[0].notes.length).toBe(1);
+      expect(state.diffFiles[0].highlighted_diff_lines[0].discussions[0].notes.length).toBe(1);
+
+      expect(state.diffFiles[0].parallel_diff_lines[0].left.discussions[0].resolved).toBe(true);
+      expect(state.diffFiles[0].highlighted_diff_lines[0].discussions[0].resolved).toBe(true);
+    });
+
     it('should add legacy discussions to the given line', () => {
       const diffPosition = {
         base_sha: 'ed13df29948c41ba367caa757ab3ec4892509910',
@@ -356,10 +437,12 @@ describe('DiffsStoreMutations', () => {
                     {
                       id: 1,
                       line_code: 'ABC_1',
+                      notes: [],
                     },
                     {
                       id: 2,
                       line_code: 'ABC_1',
+                      notes: [],
                     },
                   ],
                 },
@@ -376,10 +459,12 @@ describe('DiffsStoreMutations', () => {
                   {
                     id: 1,
                     line_code: 'ABC_1',
+                    notes: [],
                   },
                   {
                     id: 2,
                     line_code: 'ABC_1',
+                    notes: [],
                   },
                 ],
               },