Commit 7e304896 authored by Mark Chao's avatar Mark Chao

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".
parent c29d471f
Pipeline #32805440 passed with stages
in 42 minutes and 54 seconds
......@@ -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);
}
......@@ -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 &&
......
---
title: Fix loading issue on some merge request discussion
merge_request: 21982
author:
type: fixed
......@@ -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
......
......@@ -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,
......
......@@ -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',
},
},
},
......
......@@ -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,
},
];
......
......@@ -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,
},
};
......
......@@ -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: [
{
......
......@@ -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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment