Merge branch '27715-fix-unrenderable-notes' into 'master'

Fix showing diff when it has legacy diff notes

See merge request !18510

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -51,9 +51,7 @@ def preloadable_mr_relations
   def render_diffs
     @environment = @merge_request.environments_for(current_user).last
 
-    note_positions = renderable_notes.map(&:position).compact
-    @diffs.unfold_diff_files(note_positions)
-
+    @diffs.unfold_diff_files(note_positions.unfoldable)
     @diffs.write_cache
 
     request = {
@@ -140,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
 

changelogs/unreleased/27715-fix-unrenderable-notes.yml

+---
+title: Fix showing diff when it has legacy diff notes
+merge_request: 18510
+author:
+type: fixed

lib/gitlab/diff/position_collection.rb

@@ -6,13 +6,13 @@ 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)
+        filtered_positions.each(&block)
       end
 
       def concat(positions)
@@ -23,9 +23,21 @@ 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 filtered_positions
+        @collection.select { |item| item.is_a?(Position) }
+      end
+
+      def valid_head_sha?(position)
+        return true unless @diff_head_sha
+
+        position.head_sha == @diff_head_sha
+      end
     end
   end
 end

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

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