Merge branch '51958-fix-mr-discussion-loading-11-3-stable-patch-5' into '11-3-stable-patch-5'

[11-3] Backport of "Fix MR discussion not loaded issue"

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

app/assets/javascripts/diffs/store/utils.js

@@ -230,7 +230,7 @@ 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, positionType: 'text' };
         }
       });
     }
@@ -242,8 +242,8 @@ export function getDiffPositionByLineCode(diffFiles) {
 // 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);
+  const originalRefs = convertObjectPropsToCamelCase(discussion.original_position);
+  const refs = convertObjectPropsToCamelCase(discussion.position);
   return _.isEqual(refs, diffPosition) || _.isEqual(originalRefs, diffPosition);
 }

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

changelogs/unreleased/51958-fix-mr-discussion-loading.yml

+---
+title: Fix loading issue on some merge request discussion
+merge_request: 21982
+author:
+type: fixed

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

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,

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,7 @@ describe('DiffsStoreActions', () => {
                   newPath: 'file1',
                   oldLine: 5,
                   oldPath: 'file2',
+                  positionType: 'text',
                 },
               },
             },

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,
         },
       ];

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

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: [
     {

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