From 95d90966d1e0e066fb02f08cb76f7d0ef262b429 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 4 Oct 2018 07:37:24 +0000 Subject: 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 --- app/assets/javascripts/diffs/store/utils.js | 21 +++++- app/assets/javascripts/notes/stores/getters.js | 4 +- .../unreleased/51958-fix-mr-discussion-loading.yml | 5 ++ lib/gitlab/diff/position.rb | 4 + .../diffs/mock_data/diff_discussions.js | 16 ++-- spec/javascripts/diffs/store/actions_spec.js | 10 +-- spec/javascripts/diffs/store/mutations_spec.js | 16 +--- spec/javascripts/diffs/store/utils_spec.js | 16 +--- spec/javascripts/notes/mock_data.js | 24 ++---- spec/lib/gitlab/diff/position_spec.rb | 88 ++++++++++++++-------- 10 files changed, 113 insertions(+), 91 deletions(-) create mode 100644 changelogs/unreleased/51958-fix-mr-discussion-loading.yml diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index b7e52a8f37f..a027da971b5 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -230,7 +230,17 @@ 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, + lineCode, + positionType: 'text', + }; } }); } @@ -245,5 +255,12 @@ export function isDiscussionApplicableToLine(discussion, diffPosition) { const originalRefs = convertObjectPropsToCamelCase(discussion.original_position.formatter); const refs = convertObjectPropsToCamelCase(discussion.position.formatter); - return _.isEqual(refs, diffPosition) || _.isEqual(originalRefs, diffPosition); + 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; } diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index d4babf1fab2..75832884711 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.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 && diff --git a/changelogs/unreleased/51958-fix-mr-discussion-loading.yml b/changelogs/unreleased/51958-fix-mr-discussion-loading.yml new file mode 100644 index 00000000000..f80ee51291d --- /dev/null +++ b/changelogs/unreleased/51958-fix-mr-discussion-loading.yml @@ -0,0 +1,5 @@ +--- +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 978962ab2eb..300f37a95c1 100644 --- a/lib/gitlab/diff/position.rb +++ b/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 diff --git a/spec/javascripts/diffs/mock_data/diff_discussions.js b/spec/javascripts/diffs/mock_data/diff_discussions.js index b29a22da7c2..0ad214ea4a4 100644 --- a/spec/javascripts/diffs/mock_data/diff_discussions.js +++ b/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, diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index 76df3908b97..7722bfd838a 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/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,8 @@ 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 7eeca6712cc..8d2e8569aaf 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/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, }, ]; diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index 4b5cf450c68..3c4a265fd98 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/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, }, }; diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js index 1f030e5af28..9a0e7f34a9c 100644 --- a/spec/javascripts/notes/mock_data.js +++ b/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: [ { diff --git a/spec/lib/gitlab/diff/position_spec.rb b/spec/lib/gitlab/diff/position_spec.rb index 677eb373d22..2d94356f386 100644 --- a/spec/lib/gitlab/diff/position_spec.rb +++ b/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 -- cgit v1.2.1