Merge branch 'issue_48474' into 'master'

Fix discussion entity for legacy diff notes

Closes #48474

See merge request gitlab-org/gitlab-ce!20214

app/serializers/diff_file_entity.rb

@@ -25,6 +25,8 @@ class DiffFileEntity < Grape::Entity
   expose :can_modify_blob do |diff_file|
     merge_request = options[:merge_request]
 
+    next unless diff_file.blob
+
     if merge_request&.source_project && current_user
       can_modify_blob?(diff_file.blob, merge_request.source_project, merge_request.source_branch)
     else
@@ -108,6 +110,7 @@ class DiffFileEntity < Grape::Entity
     project = merge_request.target_project
 
     next unless project
+    next unless diff_file.content_sha
 
     project_blob_path(project, tree_join(diff_file.content_sha, diff_file.new_path))
   end
@@ -125,6 +128,8 @@ class DiffFileEntity < Grape::Entity
   end
 
   expose :context_lines_path, if: -> (diff_file, _) { diff_file.text? } do |diff_file|
+    next unless diff_file.content_sha
+
     project_blob_diff_path(diff_file.repository.project, tree_join(diff_file.content_sha, diff_file.file_path))
   end
 

app/serializers/discussion_entity.rb

@@ -3,7 +3,7 @@ class DiscussionEntity < Grape::Entity
   include NotesHelper
 
   expose :id, :reply_id
-  expose :position, if: -> (d, _) { d.diff_discussion? }
+  expose :position, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? }
   expose :line_code, if: -> (d, _) { d.diff_discussion? }
   expose :expanded?, as: :expanded
   expose :active?, as: :active, if: -> (d, _) { d.diff_discussion? }

lib/gitlab/diff/file.rb

@@ -247,6 +247,7 @@ def diff_lines_for_serializer
         lines = highlighted_diff_lines
 
         return if lines.empty?
+        return if blob.nil?
 
         last_line = lines.last
 

spec/serializers/diff_file_entity_spec.rb

@@ -25,6 +25,20 @@
         :context_lines_path
       )
     end
+
+    # Converted diff files from GitHub import does not contain blob file
+    # and content sha.
+    context 'when diff file does not have a blob and content sha' do
+      it 'exposes some attributes as nil' do
+        allow(diff_file).to receive(:content_sha).and_return(nil)
+        allow(diff_file).to receive(:blob).and_return(nil)
+
+        expect(subject[:context_lines_path]).to be_nil
+        expect(subject[:view_path]).to be_nil
+        expect(subject[:highlighted_diff_lines]).to be_nil
+        expect(subject[:can_modify_blob]).to be_nil
+      end
+    end
   end
 
   context 'when there is no merge request' do

spec/serializers/discussion_entity_spec.rb

@@ -36,6 +36,25 @@
     )
   end
 
+  context 'when is LegacyDiffDiscussion' do
+    let(:project) { create(:project) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+    let(:discussion) { create(:legacy_diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion }
+
+    it 'exposes correct attributes' do
+      expect(subject.keys.sort).to include(
+        :diff_discussion,
+        :expanded,
+        :id,
+        :individual_note,
+        :notes,
+        :discussion_path,
+        :for_commit,
+        :commit_id
+      )
+    end
+  end
+
   context 'when diff file is present' do
     let(:note) { create(:diff_note_on_merge_request) }