Fix MR discussion not loaded issue

Display `formatter` as the sole content of `position` object.
This means `diff_file` data is not referenced, which is the
caseu of "IOError: not opened for reading".

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

@@ -244,6 +244,7 @@ export function getDiffPositionByLineCode(diffFiles) {
             oldLine,
             newLine,
             lineCode,
+            positionType: 'text',
           };
         }
       });
@@ -259,8 +260,8 @@ export function isDiscussionApplicableToLine({ discussion, diffPosition, latestD
   const { lineCode, ...diffPositionCopy } = diffPosition;
 
   if (discussion.original_position && discussion.position) {
-    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, diffPositionCopy) || _.isEqual(originalRefs, diffPositionCopy);
   }

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,7 +2,6 @@ export default {
   id: '6b232e05bea388c6b043ccc243ba505faac04ea8',
   reply_id: '6b232e05bea388c6b043ccc243ba505faac04ea8',
   position: {
-    formatter: {
     old_line: null,
     new_line: 2,
     old_path: 'CHANGELOG',
@@ -11,7 +10,6 @@ export default {
     start_sha: 'd9eaefe5a676b820c57ff18cf5b68316025f7962',
     head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13',
   },
-  },
   line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2',
   expanded: true,
   notes: [

spec/javascripts/diffs/store/actions_spec.js

@@ -145,12 +145,8 @@ describe('DiffsStoreActions', () => {
         },
         fileHash: 'ABC',
         resolvable: true,
-        position: {
-          formatter: diffPosition,
-        },
-        original_position: {
-          formatter: diffPosition,
-        },
+        position: diffPosition,
+        original_position: diffPosition,
       };
 
       const discussions = reduceDiscussionsToLineCodes([singleDiscussion]);
@@ -175,6 +171,7 @@ describe('DiffsStoreActions', () => {
                   oldLine: 5,
                   oldPath: 'file2',
                   lineCode: 'ABC_1_1',
+                  positionType: 'text',
                 },
               },
             },

spec/javascripts/diffs/store/mutations_spec.js

@@ -193,24 +193,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

@@ -333,20 +333,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,11 +1177,9 @@ export const discussion1 = {
     file_path: 'about.md',
   },
   position: {
-    formatter: {
     new_line: 50,
     old_line: null,
   },
-  },
   notes: [
     {
       created_at: '2018-07-04T16:25:41.749Z',
@@ -1197,11 +1195,9 @@ export const resolvedDiscussion1 = {
     file_path: 'about.md',
   },
   position: {
-    formatter: {
     new_line: 50,
     old_line: null,
   },
-  },
   notes: [
     {
       created_at: '2018-07-04T16:25:41.749Z',
@@ -1217,11 +1213,9 @@ export const discussion2 = {
     file_path: 'README.md',
   },
   position: {
-    formatter: {
     new_line: null,
     old_line: 20,
   },
-  },
   notes: [
     {
       created_at: '2018-07-04T12:05:41.749Z',
@@ -1237,11 +1231,9 @@ export const discussion3 = {
     file_path: 'README.md',
   },
   position: {
-    formatter: {
     new_line: 21,
     old_line: null,
   },
-  },
   notes: [
     {
       created_at: '2018-07-05T17:25:41.749Z',

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