diff --git a/changelogs/unreleased/kassio-github-importer-null-review-author-bug.yml b/changelogs/unreleased/kassio-github-importer-null-review-author-bug.yml new file mode 100644 index 0000000000000000000000000000000000000000..1046d5f3b18377ca4b106caeae71ca4fa99a45f1 --- /dev/null +++ b/changelogs/unreleased/kassio-github-importer-null-review-author-bug.yml @@ -0,0 +1,5 @@ +--- +title: 'GithubImport: Fix Review importer when the author does not exist anymore' +merge_request: 61257 +author: +type: fixed diff --git a/lib/gitlab/github_import/importer/pull_request_review_importer.rb b/lib/gitlab/github_import/importer/pull_request_review_importer.rb index 9f495913897921ff79cb1723b3254641005991e7..f476ee13392fbb61c44133b3ecae943a9908e691 100644 --- a/lib/gitlab/github_import/importer/pull_request_review_importer.rb +++ b/lib/gitlab/github_import/importer/pull_request_review_importer.rb @@ -36,12 +36,12 @@ def add_review_note!(author_id) 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 diff --git a/lib/gitlab/github_import/markdown_text.rb b/lib/gitlab/github_import/markdown_text.rb index b25c4f7becf69280ecd266e483eab8056aa14b46..fbd54c4d4da090f2e1d8be7ceb6f5c1def9c1263 100644 --- a/lib/gitlab/github_import/markdown_text.rb +++ b/lib/gitlab/github_import/markdown_text.rb @@ -19,10 +19,10 @@ def initialize(text, author, exists = false) 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 diff --git a/lib/gitlab/github_import/user_finder.rb b/lib/gitlab/github_import/user_finder.rb index 34d1231b9a5ef6fa373b635b94be87d738f241ec..8d58441520267775900cd18508595763e5102d17 100644 --- a/lib/gitlab/github_import/user_finder.rb +++ b/lib/gitlab/github_import/user_finder.rb @@ -63,7 +63,7 @@ def assignee_id_for(issuable) # # 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. diff --git a/spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb index 5002e0384f374f67850174f4b4bff2bed57fa42e..fa8b5e6ccf065b8641d8580b9df56b534df71847 100644 --- a/spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer/pull_request_review_importer_spec.rb @@ -130,7 +130,7 @@ .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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 diff --git a/spec/lib/gitlab/github_import/user_finder_spec.rb b/spec/lib/gitlab/github_import/user_finder_spec.rb index 0dd2bd4df459ca054c4aada1bae29aaf28cb0d08..20e67a784e1c4bf434669adab72fc64572079031 100644 --- a/spec/lib/gitlab/github_import/user_finder_spec.rb +++ b/spec/lib/gitlab/github_import/user_finder_spec.rb @@ -61,6 +61,10 @@ 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