From 85b133f8486d0281d5d9afe5181ea35c1fc5eb04 Mon Sep 17 00:00:00 2001 From: Kassio Borges Date: Fri, 7 May 2021 16:24:54 +0100 Subject: [PATCH] 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 --- ...github-importer-null-review-author-bug.yml | 5 ++++ .../importer/pull_request_review_importer.rb | 10 +++---- lib/gitlab/github_import/markdown_text.rb | 6 ++--- lib/gitlab/github_import/user_finder.rb | 2 +- .../pull_request_review_importer_spec.rb | 26 ++++++++++++++----- .../gitlab/github_import/user_finder_spec.rb | 4 +++ 6 files changed, 38 insertions(+), 15 deletions(-) create mode 100644 changelogs/unreleased/kassio-github-importer-null-review-author-bug.yml 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 00000000000000..1046d5f3b18377 --- /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 9f495913897921..f476ee13392fbb 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 b25c4f7becf692..fbd54c4d4da090 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 34d1231b9a5ef6..8d584415202677 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 5002e0384f374f..fa8b5e6ccf065b 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 0dd2bd4df459ca..20e67a784e1c4b 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 -- GitLab