From f6e581df6bcbdf0e120e39a90737ae5328f70528 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Mon, 14 Oct 2019 13:17:29 +0800 Subject: [PATCH 1/4] Fix showing diff when it has legacy diff notes Do not try to unfold `LegacyDiffNote`s as they don't support `position` even though they're in the same `notes` table as `DiffNote`s. This is done by using `Gitlab::Diff::PositionCollection` which will be responsible for filtering the positions. --- .../projects/merge_requests/diffs_controller.rb | 5 ++--- lib/gitlab/diff/position_collection.rb | 17 ++++++++++++++--- .../merge_requests/diffs_controller_spec.rb | 4 ++-- .../lib/gitlab/diff/position_collection_spec.rb | 13 +++++++++++-- 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index 1913d7cd580f27..75765711ffafdc 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/app/controllers/projects/merge_requests/diffs_controller.rb @@ -50,10 +50,9 @@ def preloadable_mr_relations def render_diffs @environment = @merge_request.environments_for(current_user).last + note_positions = Gitlab::Diff::PositionCollection.new(renderable_notes.map(&:position)) - note_positions = renderable_notes.map(&:position).compact - @diffs.unfold_diff_files(note_positions) - + @diffs.unfold_diff_files(note_positions.unfoldable) @diffs.write_cache request = { diff --git a/lib/gitlab/diff/position_collection.rb b/lib/gitlab/diff/position_collection.rb index 59c60f77aaaeb0..8a16cc7f7e0e86 100644 --- a/lib/gitlab/diff/position_collection.rb +++ b/lib/gitlab/diff/position_collection.rb @@ -6,13 +6,16 @@ class PositionCollection include Enumerable # collection - An array of Gitlab::Diff::Position - def initialize(collection, diff_head_sha) + def initialize(collection, diff_head_sha = nil) @collection = collection @diff_head_sha = diff_head_sha end def each(&block) - @collection.each(&block) + @collection + .map { |item| item if item.is_a?(Position) } + .compact + .each(&block) end def concat(positions) @@ -23,9 +26,17 @@ def concat(positions) # positions (https://gitlab.com/gitlab-org/gitlab/issues/33271). def unfoldable select do |position| - position.unfoldable? && position.head_sha == @diff_head_sha + position.unfoldable? && valid_head_sha?(position) end end + + private + + def valid_head_sha?(position) + return true unless @diff_head_sha + + position.head_sha == @diff_head_sha + end end end end diff --git a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb b/spec/controllers/projects/merge_requests/diffs_controller_spec.rb index e677e836145fc2..5c02e8d6461b4a 100644 --- a/spec/controllers/projects/merge_requests/diffs_controller_spec.rb +++ b/spec/controllers/projects/merge_requests/diffs_controller_spec.rb @@ -82,9 +82,9 @@ def go(extra_params = {}) end end - context 'when note has no position' do + context 'when note is a legacy diff note' do before do - create(:legacy_diff_note_on_merge_request, project: project, noteable: merge_request, position: nil) + create(:legacy_diff_note_on_merge_request, project: project, noteable: merge_request) end it 'serializes merge request diff collection' do diff --git a/spec/lib/gitlab/diff/position_collection_spec.rb b/spec/lib/gitlab/diff/position_collection_spec.rb index de0e631ab0318f..f2a8312587cbc3 100644 --- a/spec/lib/gitlab/diff/position_collection_spec.rb +++ b/spec/lib/gitlab/diff/position_collection_spec.rb @@ -35,14 +35,15 @@ def build_image_position(attrs = {}) let(:text_position) { build_text_position } let(:folded_text_position) { build_text_position(old_line: 1, new_line: 1) } let(:image_position) { build_image_position } + let(:invalid_position) { 'a position' } let(:head_sha) { merge_request.diff_head_sha } let(:collection) do - described_class.new([text_position, folded_text_position, image_position], head_sha) + described_class.new([text_position, folded_text_position, image_position, invalid_position], head_sha) end describe '#to_a' do - it 'returns all positions' do + it 'returns all positions that are Gitlab::Diff::Position' do expect(collection.to_a).to eq([text_position, folded_text_position, image_position]) end end @@ -59,6 +60,14 @@ def build_image_position(attrs = {}) expect(collection.unfoldable).to be_empty end end + + context 'when given head_sha is nil' do + let(:head_sha) { nil } + + it 'returns unfoldable diff positions unfiltered by head_sha' do + expect(collection.unfoldable).to eq([folded_text_position]) + end + end end describe '#concat' do -- GitLab From 86f085c95d6d40de2727b07c198803db88dfd87d Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Mon, 14 Oct 2019 13:20:51 +0800 Subject: [PATCH 2/4] Add changelog entry --- changelogs/unreleased/27715-fix-unrenderable-notes.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/27715-fix-unrenderable-notes.yml diff --git a/changelogs/unreleased/27715-fix-unrenderable-notes.yml b/changelogs/unreleased/27715-fix-unrenderable-notes.yml new file mode 100644 index 00000000000000..329f9cbb30c48e --- /dev/null +++ b/changelogs/unreleased/27715-fix-unrenderable-notes.yml @@ -0,0 +1,5 @@ +--- +title: Fix showing diff when it has legacy diff notes +merge_request: 18510 +author: +type: fixed -- GitLab From 8961036ccceeef53c3d51c21fb652bb29295d5a0 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Wed, 16 Oct 2019 01:52:01 +0000 Subject: [PATCH 3/4] Apply suggestion to lib/gitlab/diff/position_collection.rb --- lib/gitlab/diff/position_collection.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/gitlab/diff/position_collection.rb b/lib/gitlab/diff/position_collection.rb index 8a16cc7f7e0e86..3fcf28af35369d 100644 --- a/lib/gitlab/diff/position_collection.rb +++ b/lib/gitlab/diff/position_collection.rb @@ -13,8 +13,7 @@ def initialize(collection, diff_head_sha = nil) def each(&block) @collection - .map { |item| item if item.is_a?(Position) } - .compact + .select { |item| item.is_a?(Position) } .each(&block) end -- GitLab From 902b7a4a5e1e52615040ae534bffd8d99aed0f43 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Wed, 16 Oct 2019 18:31:19 +0800 Subject: [PATCH 4/4] Add some private methods --- .../projects/merge_requests/diffs_controller.rb | 5 ++++- lib/gitlab/diff/position_collection.rb | 8 +++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index 75765711ffafdc..4a37dfe5c19ffc 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/app/controllers/projects/merge_requests/diffs_controller.rb @@ -50,7 +50,6 @@ def preloadable_mr_relations def render_diffs @environment = @merge_request.environments_for(current_user).last - note_positions = Gitlab::Diff::PositionCollection.new(renderable_notes.map(&:position)) @diffs.unfold_diff_files(note_positions.unfoldable) @diffs.write_cache @@ -139,6 +138,10 @@ def define_diff_comment_vars @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes), @merge_request) end + def note_positions + @note_positions ||= Gitlab::Diff::PositionCollection.new(renderable_notes.map(&:position)) + end + def renderable_notes define_diff_comment_vars unless @notes diff --git a/lib/gitlab/diff/position_collection.rb b/lib/gitlab/diff/position_collection.rb index 3fcf28af35369d..2112d347678e28 100644 --- a/lib/gitlab/diff/position_collection.rb +++ b/lib/gitlab/diff/position_collection.rb @@ -12,9 +12,7 @@ def initialize(collection, diff_head_sha = nil) end def each(&block) - @collection - .select { |item| item.is_a?(Position) } - .each(&block) + filtered_positions.each(&block) end def concat(positions) @@ -31,6 +29,10 @@ def unfoldable private + def filtered_positions + @collection.select { |item| item.is_a?(Position) } + end + def valid_head_sha?(position) return true unless @diff_head_sha -- GitLab