GithubImport: Fix Review importer when the author doesn't exist anymore

If the author of a review is deleted, in the Github system, the API
sends `null` in the `user` field. This commit fixes the
PullRequestReview Importer to now raise any exceptions in this scenario.

Changelog: fixed
MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61257

changelogs/unreleased/kassio-github-importer-null-review-author-bug.yml

+---
+title: 'GithubImport: Fix Review importer when the author does not exist anymore'
+merge_request: 61257
+author:
+type: fixed

lib/gitlab/github_import/importer/pull_request_review_importer.rb

@@ -36,12 +36,12 @@ module Gitlab
         def add_complementary_review_note!(author_id)
           return if review.note.empty? && !review.approval?
 
-          note = "*Created by %{login}*\n\n%{note}" % {
-            note: review_note_content,
-            login: review.author.login
-          }
+          note_body = MarkdownText.format(
+            review_note_content,
+            review.author
+          )
 
-          add_note!(author_id, note)
+          add_note!(author_id, note_body)
         end
 
         def review_note_content

lib/gitlab/github_import/markdown_text.rb

@@ -19,10 +19,10 @@ module Gitlab
       end
 
       def to_s
-        if exists
-          text
-        else
+        if author&.login.present? && !exists
           "*Created by: #{author.login}*\n\n#{text}"
+        else
+          text
         end
       end
     end

lib/gitlab/github_import/user_finder.rb

@@ -63,7 +63,7 @@ module Gitlab
       #
       # user - An instance of `Gitlab::GithubImport::Representation::User`.
       def user_id_for(user)
-        find(user.id, user.login)
+        find(user.id, user.login) if user.present?
       end
 
       # Returns the GitLab ID for the given GitHub ID or username.

spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb

@@ -130,7 +130,7 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean
             .to change(Note, :count).by(1)
 
           last_note = merge_request.notes.last
-          expect(last_note.note).to eq("*Created by author*\n\n**Review:** Approved")
+          expect(last_note.note).to eq("*Created by: author*\n\n**Review:** Approved")
           expect(last_note.author).to eq(project.creator)
           expect(last_note.created_at).to eq(submitted_at)
         end
@@ -153,6 +153,20 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean
       end
     end
 
+    context 'when original author was deleted in github' do
+      let(:review) { create_review(type: 'APPROVED', note: '', author: nil) }
+
+      it 'creates a note for the review without the author information' do
+        expect { subject.execute }
+          .to change(Note, :count).by(1)
+
+        last_note = merge_request.notes.last
+        expect(last_note.note).to eq('**Review:** Approved')
+        expect(last_note.author).to eq(project.creator)
+        expect(last_note.created_at).to eq(submitted_at)
+      end
+    end
+
     context 'when the review has a note text' do
       context 'when the review is "APPROVED"' do
         let(:review) { create_review(type: 'APPROVED') }
@@ -163,7 +177,7 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean
 
           last_note = merge_request.notes.last
 
-          expect(last_note.note).to eq("*Created by author*\n\n**Review:** Approved\n\nnote")
+          expect(last_note.note).to eq("*Created by: author*\n\n**Review:** Approved\n\nnote")
           expect(last_note.author).to eq(project.creator)
           expect(last_note.created_at).to eq(submitted_at)
         end
@@ -178,7 +192,7 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean
 
           last_note = merge_request.notes.last
 
-          expect(last_note.note).to eq("*Created by author*\n\n**Review:** Commented\n\nnote")
+          expect(last_note.note).to eq("*Created by: author*\n\n**Review:** Commented\n\nnote")
           expect(last_note.author).to eq(project.creator)
           expect(last_note.created_at).to eq(submitted_at)
         end
@@ -193,7 +207,7 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean
 
           last_note = merge_request.notes.last
 
-          expect(last_note.note).to eq("*Created by author*\n\n**Review:** Changes requested\n\nnote")
+          expect(last_note.note).to eq("*Created by: author*\n\n**Review:** Changes requested\n\nnote")
           expect(last_note.author).to eq(project.creator)
           expect(last_note.created_at).to eq(submitted_at)
         end
@@ -201,13 +215,13 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean
     end
   end
 
-  def create_review(type:, note: 'note')
+  def create_review(type:, note: 'note', author: { id: 999, login: 'author' })
     Gitlab::GithubImport::Representation::PullRequestReview.from_json_hash(
       merge_request_id: merge_request.id,
       review_type: type,
       note: note,
       submitted_at: submitted_at.to_s,
-      author: { id: 999, login: 'author' }
+      author: author
     )
   end
 end

spec/lib/gitlab/github_import/user_finder_spec.rb

@@ -61,6 +61,10 @@ RSpec.describe Gitlab::GithubImport::UserFinder, :clean_gitlab_redis_cache do
       expect(finder).to receive(:find).with(user.id, user.login).and_return(42)
       expect(finder.user_id_for(user)).to eq(42)
     end
+
+    it 'does not fail with empty input' do
+      expect(finder.user_id_for(nil)).to eq(nil)
+    end
   end
 
   describe '#find' do