Commit 14ed916c authored by Phil Hughes's avatar Phil Hughes

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
parents 09f6f3ea f09303b0
...@@ -244,6 +244,7 @@ export function getDiffPositionByLineCode(diffFiles) { ...@@ -244,6 +244,7 @@ export function getDiffPositionByLineCode(diffFiles) {
oldLine, oldLine,
newLine, newLine,
lineCode, lineCode,
positionType: 'text',
}; };
} }
}); });
...@@ -259,8 +260,8 @@ export function isDiscussionApplicableToLine({ discussion, diffPosition, latestD ...@@ -259,8 +260,8 @@ export function isDiscussionApplicableToLine({ discussion, diffPosition, latestD
const { lineCode, ...diffPositionCopy } = diffPosition; const { lineCode, ...diffPositionCopy } = diffPosition;
if (discussion.original_position && discussion.position) { if (discussion.original_position && discussion.position) {
const originalRefs = convertObjectPropsToCamelCase(discussion.original_position.formatter); const originalRefs = convertObjectPropsToCamelCase(discussion.original_position);
const refs = convertObjectPropsToCamelCase(discussion.position.formatter); const refs = convertObjectPropsToCamelCase(discussion.position);
return _.isEqual(refs, diffPositionCopy) || _.isEqual(originalRefs, diffPositionCopy); return _.isEqual(refs, diffPositionCopy) || _.isEqual(originalRefs, diffPositionCopy);
} }
......
...@@ -126,8 +126,8 @@ export const unresolvedDiscussionsIdsByDiff = (state, getters) => ...@@ -126,8 +126,8 @@ export const unresolvedDiscussionsIdsByDiff = (state, getters) =>
const filenameComparison = a.diff_file.file_path.localeCompare(b.diff_file.file_path); const filenameComparison = a.diff_file.file_path.localeCompare(b.diff_file.file_path);
// Get the line numbers, to compare within the same file // Get the line numbers, to compare within the same file
const aLines = [a.position.formatter.new_line, a.position.formatter.old_line]; const aLines = [a.position.new_line, a.position.old_line];
const bLines = [b.position.formatter.new_line, b.position.formatter.old_line]; const bLines = [b.position.new_line, b.position.old_line];
return filenameComparison < 0 || return filenameComparison < 0 ||
(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 ...@@ -69,6 +69,10 @@ module Gitlab
JSON.generate(formatter.to_h, opts) JSON.generate(formatter.to_h, opts)
end end
def as_json(opts = nil)
to_h.as_json(opts)
end
def type def type
formatter.line_age formatter.line_age
end end
......
...@@ -2,15 +2,13 @@ export default { ...@@ -2,15 +2,13 @@ export default {
id: '6b232e05bea388c6b043ccc243ba505faac04ea8', id: '6b232e05bea388c6b043ccc243ba505faac04ea8',
reply_id: '6b232e05bea388c6b043ccc243ba505faac04ea8', reply_id: '6b232e05bea388c6b043ccc243ba505faac04ea8',
position: { position: {
formatter: { old_line: null,
old_line: null, new_line: 2,
new_line: 2, old_path: 'CHANGELOG',
old_path: 'CHANGELOG', new_path: 'CHANGELOG',
new_path: 'CHANGELOG', base_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a',
base_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a', start_sha: 'd9eaefe5a676b820c57ff18cf5b68316025f7962',
start_sha: 'd9eaefe5a676b820c57ff18cf5b68316025f7962', head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13',
head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13',
},
}, },
line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2', line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2',
expanded: true, expanded: true,
......
...@@ -148,12 +148,8 @@ describe('DiffsStoreActions', () => { ...@@ -148,12 +148,8 @@ describe('DiffsStoreActions', () => {
}, },
fileHash: 'ABC', fileHash: 'ABC',
resolvable: true, resolvable: true,
position: { position: diffPosition,
formatter: diffPosition, original_position: diffPosition,
},
original_position: {
formatter: diffPosition,
},
}; };
const discussions = reduceDiscussionsToLineCodes([singleDiscussion]); const discussions = reduceDiscussionsToLineCodes([singleDiscussion]);
...@@ -178,6 +174,7 @@ describe('DiffsStoreActions', () => { ...@@ -178,6 +174,7 @@ describe('DiffsStoreActions', () => {
oldLine: 5, oldLine: 5,
oldPath: 'file2', oldPath: 'file2',
lineCode: 'ABC_1_1', lineCode: 'ABC_1_1',
positionType: 'text',
}, },
}, },
}, },
......
...@@ -194,24 +194,16 @@ describe('DiffsStoreMutations', () => { ...@@ -194,24 +194,16 @@ describe('DiffsStoreMutations', () => {
line_code: 'ABC_1', line_code: 'ABC_1',
diff_discussion: true, diff_discussion: true,
resolvable: true, resolvable: true,
original_position: { original_position: diffPosition,
formatter: diffPosition, position: diffPosition,
},
position: {
formatter: diffPosition,
},
}, },
{ {
id: 2, id: 2,
line_code: 'ABC_1', line_code: 'ABC_1',
diff_discussion: true, diff_discussion: true,
resolvable: true, resolvable: true,
original_position: { original_position: diffPosition,
formatter: diffPosition, position: diffPosition,
},
position: {
formatter: diffPosition,
},
}, },
]; ];
......
...@@ -333,20 +333,12 @@ describe('DiffsStoreUtils', () => { ...@@ -333,20 +333,12 @@ describe('DiffsStoreUtils', () => {
const discussions = { const discussions = {
upToDateDiscussion1: { upToDateDiscussion1: {
original_position: { original_position: diffPosition,
formatter: diffPosition, position: wrongDiffPosition,
},
position: {
formatter: wrongDiffPosition,
},
}, },
outDatedDiscussion1: { outDatedDiscussion1: {
original_position: { original_position: wrongDiffPosition,
formatter: wrongDiffPosition, position: wrongDiffPosition,
},
position: {
formatter: wrongDiffPosition,
},
}, },
}; };
......
...@@ -1177,10 +1177,8 @@ export const discussion1 = { ...@@ -1177,10 +1177,8 @@ export const discussion1 = {
file_path: 'about.md', file_path: 'about.md',
}, },
position: { position: {
formatter: { new_line: 50,
new_line: 50, old_line: null,
old_line: null,
},
}, },
notes: [ notes: [
{ {
...@@ -1197,10 +1195,8 @@ export const resolvedDiscussion1 = { ...@@ -1197,10 +1195,8 @@ export const resolvedDiscussion1 = {
file_path: 'about.md', file_path: 'about.md',
}, },
position: { position: {
formatter: { new_line: 50,
new_line: 50, old_line: null,
old_line: null,
},
}, },
notes: [ notes: [
{ {
...@@ -1217,10 +1213,8 @@ export const discussion2 = { ...@@ -1217,10 +1213,8 @@ export const discussion2 = {
file_path: 'README.md', file_path: 'README.md',
}, },
position: { position: {
formatter: { new_line: null,
new_line: null, old_line: 20,
old_line: 20,
},
}, },
notes: [ notes: [
{ {
...@@ -1237,10 +1231,8 @@ export const discussion3 = { ...@@ -1237,10 +1231,8 @@ export const discussion3 = {
file_path: 'README.md', file_path: 'README.md',
}, },
position: { position: {
formatter: { new_line: 21,
new_line: 21, old_line: null,
old_line: null,
},
}, },
notes: [ notes: [
{ {
......
...@@ -5,6 +5,34 @@ describe Gitlab::Diff::Position do ...@@ -5,6 +5,34 @@ describe Gitlab::Diff::Position do
let(:project) { create(:project, :repository) } 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 describe "position for an added text file" do
let(:commit) { project.commit("2ea1f3dec713d940208fb5ce4a38765ecb5d3f73") } let(:commit) { project.commit("2ea1f3dec713d940208fb5ce4a38765ecb5d3f73") }
...@@ -529,53 +557,49 @@ describe Gitlab::Diff::Position do ...@@ -529,53 +557,49 @@ describe Gitlab::Diff::Position do
end end
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 describe "#to_json" do
shared_examples "diff position json" do shared_examples "diff position json" do
let(:diff_position) { described_class.new(args) }
it "returns the position as JSON" do 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 end
it "works when nested under another hash" do 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
end end
context "for text positon" do context "for text positon" do
let(:hash) do let(:args) { args_for_text }
{
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" it_behaves_like "diff position json"
end end
context "for image positon" do context "for image positon" do
let(:hash) do let(:args) { args_for_img }
{
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" it_behaves_like "diff position json"
end 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