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

This reverts commit 95d90966d1e0e066fb02f08cb76f7d0ef262b429.

10 files changed, 91 insertions, 113 deletions

diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js
index a027da971b5..b7e52a8f37f 100644
--- a/app/assets/javascripts/diffs/store/utils.js
+++ b/app/assets/javascripts/diffs/store/utils.js
@@ -230,17 +230,7 @@ export function getDiffPositionByLineCode(diffFiles) {
         const { lineCode, oldLine, newLine } = line;
 
         if (lineCode) {
-          acc[lineCode] = {
-            baseSha,
-            headSha,
-            startSha,
-            newPath,
-            oldPath,
-            oldLine,
-            newLine,
-            lineCode,
-            positionType: 'text',
-          };
+          acc[lineCode] = { baseSha, headSha, startSha, newPath, oldPath, oldLine, newLine };
         }
       });
     }
@@ -255,12 +245,5 @@ export function isDiscussionApplicableToLine(discussion, diffPosition) {
   const originalRefs = convertObjectPropsToCamelCase(discussion.original_position.formatter);
   const refs = convertObjectPropsToCamelCase(discussion.position.formatter);
 
-  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;
+  return _.isEqual(refs, diffPosition) || _.isEqual(originalRefs, diffPosition);
 }
diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js
index 75832884711..d4babf1fab2 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.new_line, a.position.old_line];
-      const bLines = [b.position.new_line, b.position.old_line];
+      const aLines = [a.position.formatter.new_line, a.position.formatter.old_line];
+      const bLines = [b.position.formatter.new_line, b.position.formatter.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
deleted file mode 100644
index f80ee51291d..00000000000
--- a/changelogs/unreleased/51958-fix-mr-discussion-loading.yml
+++ /dev/null
@@ -1,5 +0,0 @@
----
-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 300f37a95c1..978962ab2eb 100644
--- a/lib/gitlab/diff/position.rb
+++ b/lib/gitlab/diff/position.rb
@@ -69,10 +69,6 @@ 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 0ad214ea4a4..b29a22da7c2 100644
--- a/spec/javascripts/diffs/mock_data/diff_discussions.js
+++ b/spec/javascripts/diffs/mock_data/diff_discussions.js
@@ -2,13 +2,15 @@ export default {
   id: '6b232e05bea388c6b043ccc243ba505faac04ea8',
   reply_id: '6b232e05bea388c6b043ccc243ba505faac04ea8',
   position: {
-    old_line: null,
-    new_line: 2,
-    old_path: 'CHANGELOG',
-    new_path: 'CHANGELOG',
-    base_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a',
-    start_sha: 'd9eaefe5a676b820c57ff18cf5b68316025f7962',
-    head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13',
+    formatter: {
+      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 7722bfd838a..76df3908b97 100644
--- a/spec/javascripts/diffs/store/actions_spec.js
+++ b/spec/javascripts/diffs/store/actions_spec.js
@@ -144,8 +144,12 @@ describe('DiffsStoreActions', () => {
         },
         fileHash: 'ABC',
         resolvable: true,
-        position: diffPosition,
-        original_position: diffPosition,
+        position: {
+          formatter: diffPosition,
+        },
+        original_position: {
+          formatter: diffPosition,
+        },
       };
 
       const discussions = reduceDiscussionsToLineCodes([singleDiscussion]);
@@ -169,8 +173,6 @@ 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 8d2e8569aaf..7eeca6712cc 100644
--- a/spec/javascripts/diffs/store/mutations_spec.js
+++ b/spec/javascripts/diffs/store/mutations_spec.js
@@ -192,16 +192,24 @@ describe('DiffsStoreMutations', () => {
           line_code: 'ABC_1',
           diff_discussion: true,
           resolvable: true,
-          original_position: diffPosition,
-          position: diffPosition,
+          original_position: {
+            formatter: diffPosition,
+          },
+          position: {
+            formatter: diffPosition,
+          },
         },
         {
           id: 2,
           line_code: 'ABC_1',
           diff_discussion: true,
           resolvable: true,
-          original_position: diffPosition,
-          position: diffPosition,
+          original_position: {
+            formatter: diffPosition,
+          },
+          position: {
+            formatter: diffPosition,
+          },
         },
       ];
 
diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js
index 3c4a265fd98..4b5cf450c68 100644
--- a/spec/javascripts/diffs/store/utils_spec.js
+++ b/spec/javascripts/diffs/store/utils_spec.js
@@ -272,12 +272,20 @@ describe('DiffsStoreUtils', () => {
 
     const discussions = {
       upToDateDiscussion1: {
-        original_position: diffPosition,
-        position: wrongDiffPosition,
+        original_position: {
+          formatter: diffPosition,
+        },
+        position: {
+          formatter: wrongDiffPosition,
+        },
       },
       outDatedDiscussion1: {
-        original_position: wrongDiffPosition,
-        position: wrongDiffPosition,
+        original_position: {
+          formatter: wrongDiffPosition,
+        },
+        position: {
+          formatter: wrongDiffPosition,
+        },
       },
     };
 
diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js
index 9a0e7f34a9c..1f030e5af28 100644
--- a/spec/javascripts/notes/mock_data.js
+++ b/spec/javascripts/notes/mock_data.js
@@ -1177,8 +1177,10 @@ export const discussion1 = {
     file_path: 'about.md',
   },
   position: {
-    new_line: 50,
-    old_line: null,
+    formatter: {
+      new_line: 50,
+      old_line: null,
+    },
   },
   notes: [
     {
@@ -1195,8 +1197,10 @@ export const resolvedDiscussion1 = {
     file_path: 'about.md',
   },
   position: {
-    new_line: 50,
-    old_line: null,
+    formatter: {
+      new_line: 50,
+      old_line: null,
+    },
   },
   notes: [
     {
@@ -1213,8 +1217,10 @@ export const discussion2 = {
     file_path: 'README.md',
   },
   position: {
-    new_line: null,
-    old_line: 20,
+    formatter: {
+      new_line: null,
+      old_line: 20,
+    },
   },
   notes: [
     {
@@ -1231,8 +1237,10 @@ export const discussion3 = {
     file_path: 'README.md',
   },
   position: {
-    new_line: 21,
-    old_line: null,
+    formatter: {
+      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 2d94356f386..677eb373d22 100644
--- a/spec/lib/gitlab/diff/position_spec.rb
+++ b/spec/lib/gitlab/diff/position_spec.rb
@@ -5,34 +5,6 @@ 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") }
 
@@ -557,49 +529,53 @@ 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(args.stringify_keys)
+        expect(JSON.parse(diff_position.to_json)).to eq(hash.stringify_keys)
       end
 
       it "works when nested under another hash" do
-        expect(JSON.parse(JSON.generate(pos: diff_position))).to eq('pos' => args.stringify_keys)
+        expect(JSON.parse(JSON.generate(pos: diff_position))).to eq('pos' => hash.stringify_keys)
       end
     end
 
     context "for text positon" do
-      let(:args) { args_for_text }
+      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) }
 
       it_behaves_like "diff position json"
     end
 
     context "for image positon" do
-      let(:args) { args_for_img }
+      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) }
 
       it_behaves_like "diff position json"
     end