Merge branch '51958-fix-mr-discussion-loading' into 'master'

Fix MR discussion not loaded issue

Closes #51958

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

10 files changed, 113 insertions, 91 deletions

diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js
index b7e52a8f37f..a027da971b5 100644
--- a/app/assets/javascripts/diffs/store/utils.js
+++ b/app/assets/javascripts/diffs/store/utils.js
@@ -230,7 +230,17 @@ export function getDiffPositionByLineCode(diffFiles) {
         const { lineCode, oldLine, newLine } = line;
 
         if (lineCode) {
-          acc[lineCode] = { baseSha, headSha, startSha, newPath, oldPath, oldLine, newLine };
+          acc[lineCode] = {
+            baseSha,
+            headSha,
+            startSha,
+            newPath,
+            oldPath,
+            oldLine,
+            newLine,
+            lineCode,
+            positionType: 'text',
+          };
         }
       });
     }
@@ -245,5 +255,12 @@ 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);
+  if (discussion.original_position && discussion.position) {
+    const originalRefs = convertObjectPropsToCamelCase(discussion.original_position);
+    const refs = convertObjectPropsToCamelCase(discussion.position);
+
+    return _.isEqual(refs, diffPositionCopy) || _.isEqual(originalRefs, diffPositionCopy);
+  }
+
+  return latestDiff && discussion.active && lineCode === discussion.line_code;
 }
diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js
index d4babf1fab2..75832884711 100644
--- a/app/assets/javascripts/notes/stores/getters.js
+++ b/app/assets/javascripts/notes/stores/getters.js
@@ -126,8 +126,8 @@ export const unresolvedDiscussionsIdsByDiff = (state, getters) =>
       const filenameComparison = a.diff_file.file_path.localeCompare(b.diff_file.file_path);
 
       // Get the line numbers, to compare within the same file
-      const aLines = [a.position.formatter.new_line, a.position.formatter.old_line];
-      const bLines = [b.position.formatter.new_line, b.position.formatter.old_line];
+      const aLines = [a.position.new_line, a.position.old_line];
+      const bLines = [b.position.new_line, b.position.old_line];
 
       return filenameComparison < 0 ||
         (filenameComparison === 0 &&
diff --git a/changelogs/unreleased/51958-fix-mr-discussion-loading.yml b/changelogs/unreleased/51958-fix-mr-discussion-loading.yml
new file mode 100644
index 00000000000..f80ee51291d
--- /dev/null
+++ b/changelogs/unreleased/51958-fix-mr-discussion-loading.yml
@@ -0,0 +1,5 @@
+---
+title: Fix loading issue on some merge request discussion
+merge_request: 21982
+author:
+type: fixed
diff --git a/lib/gitlab/diff/position.rb b/lib/gitlab/diff/position.rb
index 978962ab2eb..300f37a95c1 100644
--- a/lib/gitlab/diff/position.rb
+++ b/lib/gitlab/diff/position.rb
@@ -69,6 +69,10 @@ module Gitlab
         JSON.generate(formatter.to_h, opts)
       end
 
+      def as_json(opts = nil)
+        to_h.as_json(opts)
+      end
+
       def type
         formatter.line_age
       end
diff --git a/spec/javascripts/diffs/mock_data/diff_discussions.js b/spec/javascripts/diffs/mock_data/diff_discussions.js
index b29a22da7c2..0ad214ea4a4 100644
--- a/spec/javascripts/diffs/mock_data/diff_discussions.js
+++ b/spec/javascripts/diffs/mock_data/diff_discussions.js
@@ -2,15 +2,13 @@ export default {
   id: '6b232e05bea388c6b043ccc243ba505faac04ea8',
   reply_id: '6b232e05bea388c6b043ccc243ba505faac04ea8',
   position: {
-    formatter: {
-      old_line: null,
-      new_line: 2,
-      old_path: 'CHANGELOG',
-      new_path: 'CHANGELOG',
-      base_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a',
-      start_sha: 'd9eaefe5a676b820c57ff18cf5b68316025f7962',
-      head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13',
-    },
+    old_line: null,
+    new_line: 2,
+    old_path: 'CHANGELOG',
+    new_path: 'CHANGELOG',
+    base_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a',
+    start_sha: 'd9eaefe5a676b820c57ff18cf5b68316025f7962',
+    head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13',
   },
   line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2',
   expanded: true,
diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js
index 76df3908b97..7722bfd838a 100644
--- a/spec/javascripts/diffs/store/actions_spec.js
+++ b/spec/javascripts/diffs/store/actions_spec.js
@@ -144,12 +144,8 @@ describe('DiffsStoreActions', () => {
         },
         fileHash: 'ABC',
         resolvable: true,
-        position: {
-          formatter: diffPosition,
-        },
-        original_position: {
-          formatter: diffPosition,
-        },
+        position: diffPosition,
+        original_position: diffPosition,
       };
 
       const discussions = reduceDiscussionsToLineCodes([singleDiscussion]);
@@ -173,6 +169,8 @@ describe('DiffsStoreActions', () => {
                   newPath: 'file1',
                   oldLine: 5,
                   oldPath: 'file2',
+                  lineCode: 'ABC_1_1',
+                  positionType: 'text',
                 },
               },
             },
diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js
index 7eeca6712cc..8d2e8569aaf 100644
--- a/spec/javascripts/diffs/store/mutations_spec.js
+++ b/spec/javascripts/diffs/store/mutations_spec.js
@@ -192,24 +192,16 @@ describe('DiffsStoreMutations', () => {
           line_code: 'ABC_1',
           diff_discussion: true,
           resolvable: true,
-          original_position: {
-            formatter: diffPosition,
-          },
-          position: {
-            formatter: diffPosition,
-          },
+          original_position: diffPosition,
+          position: diffPosition,
         },
         {
           id: 2,
           line_code: 'ABC_1',
           diff_discussion: true,
           resolvable: true,
-          original_position: {
-            formatter: diffPosition,
-          },
-          position: {
-            formatter: diffPosition,
-          },
+          original_position: diffPosition,
+          position: diffPosition,
         },
       ];
 
diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js
index 4b5cf450c68..3c4a265fd98 100644
--- a/spec/javascripts/diffs/store/utils_spec.js
+++ b/spec/javascripts/diffs/store/utils_spec.js
@@ -272,20 +272,12 @@ describe('DiffsStoreUtils', () => {
 
     const discussions = {
       upToDateDiscussion1: {
-        original_position: {
-          formatter: diffPosition,
-        },
-        position: {
-          formatter: wrongDiffPosition,
-        },
+        original_position: diffPosition,
+        position: wrongDiffPosition,
       },
       outDatedDiscussion1: {
-        original_position: {
-          formatter: wrongDiffPosition,
-        },
-        position: {
-          formatter: wrongDiffPosition,
-        },
+        original_position: wrongDiffPosition,
+        position: wrongDiffPosition,
       },
     };
 
diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js
index 1f030e5af28..9a0e7f34a9c 100644
--- a/spec/javascripts/notes/mock_data.js
+++ b/spec/javascripts/notes/mock_data.js
@@ -1177,10 +1177,8 @@ export const discussion1 = {
     file_path: 'about.md',
   },
   position: {
-    formatter: {
-      new_line: 50,
-      old_line: null,
-    },
+    new_line: 50,
+    old_line: null,
   },
   notes: [
     {
@@ -1197,10 +1195,8 @@ export const resolvedDiscussion1 = {
     file_path: 'about.md',
   },
   position: {
-    formatter: {
-      new_line: 50,
-      old_line: null,
-    },
+    new_line: 50,
+    old_line: null,
   },
   notes: [
     {
@@ -1217,10 +1213,8 @@ export const discussion2 = {
     file_path: 'README.md',
   },
   position: {
-    formatter: {
-      new_line: null,
-      old_line: 20,
-    },
+    new_line: null,
+    old_line: 20,
   },
   notes: [
     {
@@ -1237,10 +1231,8 @@ export const discussion3 = {
     file_path: 'README.md',
   },
   position: {
-    formatter: {
-      new_line: 21,
-      old_line: null,
-    },
+    new_line: 21,
+    old_line: null,
   },
   notes: [
     {
diff --git a/spec/lib/gitlab/diff/position_spec.rb b/spec/lib/gitlab/diff/position_spec.rb
index 677eb373d22..2d94356f386 100644
--- a/spec/lib/gitlab/diff/position_spec.rb
+++ b/spec/lib/gitlab/diff/position_spec.rb
@@ -5,6 +5,34 @@ describe Gitlab::Diff::Position do
 
   let(:project) { create(:project, :repository) }
 
+  let(:args_for_img) do
+    {
+      old_path: "files/any.img",
+      new_path: "files/any.img",
+      base_sha: nil,
+      head_sha: nil,
+      start_sha: nil,
+      width: 100,
+      height: 100,
+      x: 1,
+      y: 100,
+      position_type: "image"
+    }
+  end
+
+  let(:args_for_text) do
+    {
+      old_path: "files/ruby/popen.rb",
+      new_path: "files/ruby/popen.rb",
+      old_line: nil,
+      new_line: 14,
+      base_sha: nil,
+      head_sha: nil,
+      start_sha: nil,
+      position_type: "text"
+    }
+  end
+
   describe "position for an added text file" do
     let(:commit) { project.commit("2ea1f3dec713d940208fb5ce4a38765ecb5d3f73") }
 
@@ -529,53 +557,49 @@ describe Gitlab::Diff::Position do
     end
   end
 
+  describe "#as_json" do
+    shared_examples "diff position json" do
+      let(:diff_position) { described_class.new(args) }
+
+      it "returns the position as JSON" do
+        expect(diff_position.as_json).to eq(args.stringify_keys)
+      end
+    end
+
+    context "for text positon" do
+      let(:args) { args_for_text }
+
+      it_behaves_like "diff position json"
+    end
+
+    context "for image positon" do
+      let(:args) { args_for_img }
+
+      it_behaves_like "diff position json"
+    end
+  end
+
   describe "#to_json" do
     shared_examples "diff position json" do
+      let(:diff_position) { described_class.new(args) }
+
       it "returns the position as JSON" do
-        expect(JSON.parse(diff_position.to_json)).to eq(hash.stringify_keys)
+        expect(JSON.parse(diff_position.to_json)).to eq(args.stringify_keys)
       end
 
       it "works when nested under another hash" do
-        expect(JSON.parse(JSON.generate(pos: diff_position))).to eq('pos' => hash.stringify_keys)
+        expect(JSON.parse(JSON.generate(pos: diff_position))).to eq('pos' => args.stringify_keys)
       end
     end
 
     context "for text positon" do
-      let(:hash) do
-        {
-          old_path: "files/ruby/popen.rb",
-          new_path: "files/ruby/popen.rb",
-          old_line: nil,
-          new_line: 14,
-          base_sha: nil,
-          head_sha: nil,
-          start_sha: nil,
-          position_type: "text"
-        }
-      end
-
-      let(:diff_position) { described_class.new(hash) }
+      let(:args) { args_for_text }
 
       it_behaves_like "diff position json"
     end
 
     context "for image positon" do
-      let(:hash) do
-        {
-          old_path: "files/any.img",
-          new_path: "files/any.img",
-          base_sha: nil,
-          head_sha: nil,
-          start_sha: nil,
-          width: 100,
-          height: 100,
-          x: 1,
-          y: 100,
-          position_type: "image"
-        }
-      end
-
-      let(:diff_position) { described_class.new(hash) }
+      let(:args) { args_for_img }
 
       it_behaves_like "diff position json"
     end