From c29d471f2386a3d798d24d7ece8939cc46fd3ee2 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 12 Oct 2018 16:32:30 +0200 Subject: Revert "Merge branch '51958-fix-mr-discussion-loading' into 'master'" This reverts commit 95d90966d1e0e066fb02f08cb76f7d0ef262b429. --- 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, 91 insertions(+), 113 deletions(-) delete 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 a027da971b5..b7e52a8f37f 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -230,17 +230,7 @@ export function getDiffPositionByLineCode(diffFiles) { const { lineCode, oldLine, newLine } = line; if (lineCode) { - acc[lineCode] = { - baseSha, - headSha, - startSha, - newPath, - oldPath, - oldLine, - newLine, - lineCode, - positionType: 'text', - }; + acc[lineCode] = { baseSha, headSha, startSha, newPath, oldPath, oldLine, newLine }; } }); } @@ -255,12 +245,5 @@ export function isDiscussionApplicableToLine(discussion, diffPosition) { const originalRefs = convertObjectPropsToCamelCase(discussion.original_position.formatter); const refs = convertObjectPropsToCamelCase(discussion.position.formatter); - 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; + return _.isEqual(refs, diffPosition) || _.isEqual(originalRefs, diffPosition); } diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 75832884711..d4babf1fab2 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.new_line, a.position.old_line]; - const bLines = [b.position.new_line, b.position.old_line]; + const aLines = [a.position.formatter.new_line, a.position.formatter.old_line]; + const bLines = [b.position.formatter.new_line, b.position.formatter.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 deleted file mode 100644 index f80ee51291d..00000000000 --- a/changelogs/unreleased/51958-fix-mr-discussion-loading.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -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 300f37a95c1..978962ab2eb 100644 --- a/lib/gitlab/diff/position.rb +++ b/lib/gitlab/diff/position.rb @@ -69,10 +69,6 @@ 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 0ad214ea4a4..b29a22da7c2 100644 --- a/spec/javascripts/diffs/mock_data/diff_discussions.js +++ b/spec/javascripts/diffs/mock_data/diff_discussions.js @@ -2,13 +2,15 @@ export default { id: '6b232e05bea388c6b043ccc243ba505faac04ea8', reply_id: '6b232e05bea388c6b043ccc243ba505faac04ea8', position: { - old_line: null, - new_line: 2, - old_path: 'CHANGELOG', - new_path: 'CHANGELOG', - base_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a', - start_sha: 'd9eaefe5a676b820c57ff18cf5b68316025f7962', - head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13', + formatter: { + 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 7722bfd838a..76df3908b97 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -144,8 +144,12 @@ describe('DiffsStoreActions', () => { }, fileHash: 'ABC', resolvable: true, - position: diffPosition, - original_position: diffPosition, + position: { + formatter: diffPosition, + }, + original_position: { + formatter: diffPosition, + }, }; const discussions = reduceDiscussionsToLineCodes([singleDiscussion]); @@ -169,8 +173,6 @@ 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 8d2e8569aaf..7eeca6712cc 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/spec/javascripts/diffs/store/mutations_spec.js @@ -192,16 +192,24 @@ describe('DiffsStoreMutations', () => { line_code: 'ABC_1', diff_discussion: true, resolvable: true, - original_position: diffPosition, - position: diffPosition, + original_position: { + formatter: diffPosition, + }, + position: { + formatter: diffPosition, + }, }, { id: 2, line_code: 'ABC_1', diff_discussion: true, resolvable: true, - original_position: diffPosition, - position: diffPosition, + original_position: { + formatter: diffPosition, + }, + position: { + formatter: diffPosition, + }, }, ]; diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index 3c4a265fd98..4b5cf450c68 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -272,12 +272,20 @@ describe('DiffsStoreUtils', () => { const discussions = { upToDateDiscussion1: { - original_position: diffPosition, - position: wrongDiffPosition, + original_position: { + formatter: diffPosition, + }, + position: { + formatter: wrongDiffPosition, + }, }, outDatedDiscussion1: { - original_position: wrongDiffPosition, - position: wrongDiffPosition, + original_position: { + formatter: wrongDiffPosition, + }, + position: { + formatter: wrongDiffPosition, + }, }, }; diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js index 9a0e7f34a9c..1f030e5af28 100644 --- a/spec/javascripts/notes/mock_data.js +++ b/spec/javascripts/notes/mock_data.js @@ -1177,8 +1177,10 @@ export const discussion1 = { file_path: 'about.md', }, position: { - new_line: 50, - old_line: null, + formatter: { + new_line: 50, + old_line: null, + }, }, notes: [ { @@ -1195,8 +1197,10 @@ export const resolvedDiscussion1 = { file_path: 'about.md', }, position: { - new_line: 50, - old_line: null, + formatter: { + new_line: 50, + old_line: null, + }, }, notes: [ { @@ -1213,8 +1217,10 @@ export const discussion2 = { file_path: 'README.md', }, position: { - new_line: null, - old_line: 20, + formatter: { + new_line: null, + old_line: 20, + }, }, notes: [ { @@ -1231,8 +1237,10 @@ export const discussion3 = { file_path: 'README.md', }, position: { - new_line: 21, - old_line: null, + formatter: { + 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 2d94356f386..677eb373d22 100644 --- a/spec/lib/gitlab/diff/position_spec.rb +++ b/spec/lib/gitlab/diff/position_spec.rb @@ -5,34 +5,6 @@ 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") } @@ -557,49 +529,53 @@ 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(args.stringify_keys) + expect(JSON.parse(diff_position.to_json)).to eq(hash.stringify_keys) end it "works when nested under another hash" do - expect(JSON.parse(JSON.generate(pos: diff_position))).to eq('pos' => args.stringify_keys) + expect(JSON.parse(JSON.generate(pos: diff_position))).to eq('pos' => hash.stringify_keys) end end context "for text positon" do - let(:args) { args_for_text } + 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) } it_behaves_like "diff position json" end context "for image positon" do - let(:args) { args_for_img } + 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) } it_behaves_like "diff position json" end -- cgit v1.2.1