Merge branch 'kassio/github-import-review-comments' into 'master'

Github Importer - Import pull request reviews

See merge request gitlab-org/gitlab!48632

app/workers/all_queues.yml

@@ -715,6 +715,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: github_importer:github_import_import_pull_request_review
+  :feature_category: :importers
+  :has_external_dependencies: true
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent:
+  :tags: []
 - :name: github_importer:github_import_refresh_import_jid
   :feature_category: :importers
   :has_external_dependencies:
@@ -779,6 +787,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: github_importer:github_import_stage_import_pull_requests_reviews
+  :feature_category: :importers
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent:
+  :tags: []
 - :name: github_importer:github_import_stage_import_repository
   :feature_category: :importers
   :has_external_dependencies:

app/workers/gitlab/github_import/advance_stage_worker.rb

@@ -17,6 +17,7 @@ module Gitlab
       # The known importer stages and their corresponding Sidekiq workers.
       STAGES = {
         pull_requests_merged_by: Stage::ImportPullRequestsMergedByWorker,
+        pull_request_reviews: Stage::ImportPullRequestsReviewsWorker,
         issues_and_diff_notes: Stage::ImportIssuesAndDiffNotesWorker,
         notes: Stage::ImportNotesWorker,
         lfs_objects: Stage::ImportLfsObjectsWorker,

app/workers/gitlab/github_import/import_pull_request_review_worker.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module GithubImport
+    class ImportPullRequestReviewWorker # rubocop:disable Scalability/IdempotentWorker
+      include ObjectImporter
+
+      def representation_class
+        Gitlab::GithubImport::Representation::PullRequestReview
+      end
+
+      def importer_class
+        Importer::PullRequestReviewImporter
+      end
+
+      def counter_name
+        :github_importer_imported_pull_request_reviews
+      end
+
+      def counter_description
+        'The number of imported GitHub pull request reviews'
+      end
+    end
+  end
+end

app/workers/gitlab/github_import/import_pull_request_worker.rb

@@ -6,7 +6,7 @@ module Gitlab
       include ObjectImporter
 
       def representation_class
-        Representation::PullRequest
+        Gitlab::GithubImport::Representation::PullRequest
       end
 
       def importer_class

app/workers/gitlab/github_import/stage/import_pull_requests_merged_by_worker.rb

@@ -20,7 +20,7 @@ module Gitlab
           AdvanceStageWorker.perform_async(
             project.id,
             { waiter.key => waiter.jobs_remaining },
-            :issues_and_diff_notes
+            :pull_request_reviews
           )
         end
       end

app/workers/gitlab/github_import/stage/import_pull_requests_reviews_worker.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module GithubImport
+    module Stage
+      class ImportPullRequestsReviewsWorker # rubocop:disable Scalability/IdempotentWorker
+        include ApplicationWorker
+        include GithubImport::Queue
+        include StageMethods
+
+        # client - An instance of Gitlab::GithubImport::Client.
+        # project - An instance of Project.
+        def import(client, project)
+          waiter =
+            if Feature.enabled?(:github_import_pull_request_reviews, project, default_enabled: true)
+              waiter = Importer::PullRequestsReviewsImporter
+                .new(project, client)
+                .execute
+
+              project.import_state.refresh_jid_expiration
+
+              waiter
+            else
+              JobWaiter.new
+            end
+
+          AdvanceStageWorker.perform_async(
+            project.id,
+            { waiter.key => waiter.jobs_remaining },
+            :issues_and_diff_notes
+          )
+        end
+      end
+    end
+  end
+end

changelogs/unreleased/kassio-github-import-review-comments.yml

+---
+title: Github Importer - import pull request reviews from Github
+merge_request: 48632
+author:
+type: added

config/feature_flags/development/github_import_pull_request_reviews.yml

+---
+name: github_import_pull_request_reviews
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48632
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/289153
+milestone: '13.7'
+type: development
+group: group::import
+default_enabled: true

lib/gitlab/github_import/client.rb

@@ -69,6 +69,10 @@ module Gitlab
         with_rate_limit { octokit.user(username) }
       end
 
+      def pull_request_reviews(repo_name, iid)
+        with_rate_limit { octokit.pull_request_reviews(repo_name, iid) }
+      end
+
       # Returns the details of a GitHub repository.
       #
       # name - The path (in the form `owner/repository`) of the repository.

lib/gitlab/github_import/importer/pull_request_review_importer.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module GithubImport
+    module Importer
+      class PullRequestReviewImporter
+        def initialize(review, project, client)
+          @review = review
+          @project = project
+          @client = client
+          @merge_request = project.merge_requests.find_by_id(review.merge_request_id)
+        end
+
+        def execute
+          user_finder = GithubImport::UserFinder.new(project, client)
+          gitlab_user_id = user_finder.user_id_for(review.author)
+
+          if gitlab_user_id
+            add_review_note!(gitlab_user_id)
+            add_approval!(gitlab_user_id)
+          else
+            add_complementary_review_note!(project.creator_id)
+          end
+        end
+
+        private
+
+        attr_reader :review, :merge_request, :project, :client
+
+        def add_review_note!(author_id)
+          return if review.note.empty?
+
+          add_note!(author_id, review_note_content)
+        end
+
+        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
+          }
+
+          add_note!(author_id, note)
+        end
+
+        def review_note_content
+          header = "**Review:** #{review.review_type.humanize}"
+
+          if review.note.present?
+            "#{header}\n\n#{review.note}"
+          else
+            header
+          end
+        end
+
+        def add_note!(author_id, note)
+          note = Note.new(note_attributes(author_id, note))
+
+          note.save!
+        end
+
+        def note_attributes(author_id, note, extra = {})
+          {
+            importing: true,
+            noteable_id: merge_request.id,
+            noteable_type: 'MergeRequest',
+            project_id: project.id,
+            author_id: author_id,
+            note: note,
+            system: false,
+            created_at: review.submitted_at,
+            updated_at: review.submitted_at
+          }.merge(extra)
+        end
+
+        def add_approval!(user_id)
+          return unless review.review_type == 'APPROVED'
+
+          add_approval_system_note!(user_id)
+
+          merge_request.approvals.create!(
+            user_id: user_id,
+            created_at: review.submitted_at
+          )
+        end
+
+        def add_approval_system_note!(user_id)
+          attributes = note_attributes(
+            user_id,
+            'approved this merge request',
+            system: true,
+            system_note_metadata: SystemNoteMetadata.new(action: 'approved')
+          )
+
+          Note.create!(attributes)
+        end
+      end
+    end
+  end
+end

lib/gitlab/github_import/importer/pull_requests_importer.rb

@@ -11,7 +11,7 @@ module Gitlab
         end
 
         def representation_class
-          Representation::PullRequest
+          Gitlab::GithubImport::Representation::PullRequest
         end
 
         def sidekiq_worker_class

lib/gitlab/github_import/importer/pull_requests_reviews_importer.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module GithubImport
+    module Importer
+      class PullRequestsReviewsImporter
+        include ParallelScheduling
+
+        def importer_class
+          PullRequestReviewImporter
+        end
+
+        def representation_class
+          Gitlab::GithubImport::Representation::PullRequestReview
+        end
+
+        def sidekiq_worker_class
+          ImportPullRequestReviewWorker
+        end
+
+        def collection_method
+          :pull_request_reviews
+        end
+
+        def id_for_already_imported_cache(review)
+          review.github_id
+        end
+
+        def each_object_to_import
+          project.merge_requests.find_each do |merge_request|
+            reviews = client.pull_request_reviews(project.import_source, merge_request.iid)
+            reviews.each do |review|
+              review.merge_request_id = merge_request.id
+              yield(review)
+            end
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/github_import/representation/pull_request_review.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module GithubImport
+    module Representation
+      class PullRequestReview
+        include ToHash
+        include ExposeAttribute
+
+        attr_reader :attributes
+
+        expose_attribute :author, :note, :review_type, :submitted_at, :github_id, :merge_request_id
+
+        def self.from_api_response(review)
+          user = Representation::User.from_api_response(review.user) if review.user
+
+          new(
+            merge_request_id: review.merge_request_id,
+            author: user,
+            note: review.body,
+            review_type: review.state,
+            submitted_at: review.submitted_at,
+            github_id: review.id
+          )
+        end
+
+        # Builds a new note using a Hash that was built from a JSON payload.
+        def self.from_json_hash(raw_hash)
+          hash = Representation.symbolize_hash(raw_hash)
+
+          hash[:author] &&= Representation::User.from_json_hash(hash[:author])
+          hash[:submitted_at] = Time.parse(hash[:submitted_at]).in_time_zone
+
+          new(hash)
+        end
+
+        # attributes - A Hash containing the raw note details. The keys of this
+        #              Hash must be Symbols.
+        def initialize(attributes)
+          @attributes = attributes
+        end
+
+        def approval?
+          review_type == 'APPROVED'
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/github_import/client_spec.rb

@@ -28,6 +28,17 @@ RSpec.describe Gitlab::GithubImport::Client do
     end
   end
 
+  describe '#pull_request_reviews' do
+    it 'returns the pull request reviews' do
+      client = described_class.new('foo')
+
+      expect(client.octokit).to receive(:pull_request_reviews).with('foo/bar', 999)
+      expect(client).to receive(:with_rate_limit).and_yield
+
+      client.pull_request_reviews('foo/bar', 999)
+    end
+  end
+
   describe '#repository' do
     it 'returns the details of a repository' do
       client = described_class.new('foo')

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::GithubImport::Importer::PullRequestReviewImporter, :clean_gitlab_redis_cache do
+  using RSpec::Parameterized::TableSyntax
+
+  let_it_be(:merge_request) { create(:merge_request) }
+  let(:project) { merge_request.project }
+  let(:client_double) { double(user: double(id: 999, login: 'author', email: 'author@email.com')) }
+  let(:submitted_at) { Time.new(2017, 1, 1, 12, 00).utc }
+
+  subject { described_class.new(review, project, client_double) }
+
+  context 'when the review author can be mapped to a gitlab user' do
+    let_it_be(:author) { create(:user, email: 'author@email.com') }
+
+    context 'when the review has no note text' do
+      context 'when the review is "APPROVED"' do
+        let(:review) { create_review(type: 'APPROVED', note: '') }
+
+        it 'creates a note for the review' do
+          expect { subject.execute }.to change(Note, :count)
+
+          last_note = merge_request.notes.last
+          expect(last_note.note).to eq('approved this merge request')
+          expect(last_note.author).to eq(author)
+          expect(last_note.created_at).to eq(submitted_at)
+          expect(last_note.system_note_metadata.action).to eq('approved')
+
+          expect(merge_request.approved_by_users.reload).to include(author)
+          expect(merge_request.approvals.last.created_at).to eq(submitted_at)
+        end
+      end
+
+      context 'when the review is "COMMENTED"' do
+        let(:review) { create_review(type: 'COMMENTED', note: '') }
+
+        it 'creates a note for the review' do
+          expect { subject.execute }.not_to change(Note, :count)
+        end
+      end
+
+      context 'when the review is "CHANGES_REQUESTED"' do
+        let(:review) { create_review(type: 'CHANGES_REQUESTED', note: '') }
+
+        it 'creates a note for the review' do
+          expect { subject.execute }.not_to change(Note, :count)
+        end
+      end
+    end
+
+    context 'when the review has a note text' do
+      context 'when the review is "APPROVED"' do
+        let(:review) { create_review(type: 'APPROVED') }
+
+        it 'creates a note for the review' do
+          expect { subject.execute }
+            .to change(Note, :count).by(2)
+            .and change(Approval, :count).by(1)
+
+          note = merge_request.notes.where(system: false).last
+          expect(note.note).to eq("**Review:** Approved\n\nnote")
+          expect(note.author).to eq(author)
+          expect(note.created_at).to eq(submitted_at)
+
+          system_note = merge_request.notes.where(system: true).last
+          expect(system_note.note).to eq('approved this merge request')
+          expect(system_note.author).to eq(author)
+          expect(system_note.created_at).to eq(submitted_at)
+          expect(system_note.system_note_metadata.action).to eq('approved')
+
+          expect(merge_request.approved_by_users.reload).to include(author)
+          expect(merge_request.approvals.last.created_at).to eq(submitted_at)
+        end
+      end
+
+      context 'when the review is "COMMENTED"' do
+        let(:review) { create_review(type: 'COMMENTED') }
+
+        it 'creates a note for the review' do
+          expect { subject.execute }
+            .to change(Note, :count).by(1)
+            .and not_change(Approval, :count)
+
+          last_note = merge_request.notes.last
+
+          expect(last_note.note).to eq("**Review:** Commented\n\nnote")
+          expect(last_note.author).to eq(author)
+          expect(last_note.created_at).to eq(submitted_at)
+        end
+      end
+
+      context 'when the review is "CHANGES_REQUESTED"' do
+        let(:review) { create_review(type: 'CHANGES_REQUESTED') }
+
+        it 'creates a note for the review' do
+          expect { subject.execute }
+            .to change(Note, :count).by(1)
+            .and not_change(Approval, :count)
+
+          last_note = merge_request.notes.last
+
+          expect(last_note.note).to eq("**Review:** Changes requested\n\nnote")
+          expect(last_note.author).to eq(author)
+          expect(last_note.created_at).to eq(submitted_at)
+        end
+      end
+    end
+  end
+
+  context 'when the review author cannot be mapped to a gitlab user' do
+    context 'when the review has no note text' do
+      context 'when the review is "APPROVED"' do
+        let(:review) { create_review(type: 'APPROVED', note: '') }
+
+        it 'creates a note for the review with *Approved by by<author>*' do
+          expect { subject.execute }
+            .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.author).to eq(project.creator)
+          expect(last_note.created_at).to eq(submitted_at)
+        end
+      end
+
+      context 'when the review is "COMMENTED"' do
+        let(:review) { create_review(type: 'COMMENTED', note: '') }
+
+        it 'creates a note for the review with *Commented by<author>*' do
+          expect { subject.execute }.not_to change(Note, :count)
+        end
+      end
+
+      context 'when the review is "CHANGES_REQUESTED"' do
+        let(:review) { create_review(type: 'CHANGES_REQUESTED', note: '') }
+
+        it 'creates a note for the review with *Changes requested by <author>*' do
+          expect { subject.execute }.not_to change(Note, :count)
+        end
+      end
+    end
+
+    context 'when the review has a note text' do
+      context 'when the review is "APPROVED"' do
+        let(:review) { create_review(type: 'APPROVED') }
+
+        it 'creates a note for the review with *Approved by by<author>*' do
+          expect { subject.execute }
+            .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\n\nnote")
+          expect(last_note.author).to eq(project.creator)
+          expect(last_note.created_at).to eq(submitted_at)
+        end
+      end
+
+      context 'when the review is "COMMENTED"' do
+        let(:review) { create_review(type: 'COMMENTED') }
+
+        it 'creates a note for the review with *Commented by<author>*' do
+          expect { subject.execute }
+            .to change(Note, :count).by(1)
+
+          last_note = merge_request.notes.last
+
+          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
+      end
+
+      context 'when the review is "CHANGES_REQUESTED"' do
+        let(:review) { create_review(type: 'CHANGES_REQUESTED') }
+
+        it 'creates a note for the review with *Changes requested by <author>*' do
+          expect { subject.execute }
+            .to change(Note, :count).by(1)
+
+          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.author).to eq(project.creator)
+          expect(last_note.created_at).to eq(submitted_at)
+        end
+      end
+    end
+  end
+
+  def create_review(type:, note: 'note')
+    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' }
+    )
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::GithubImport::Importer::PullRequestsReviewsImporter do
+  let(:client) { double }
+  let(:project) { create(:project, import_source: 'github/repo') }
+
+  subject { described_class.new(project, client) }
+
+  it { is_expected.to include_module(Gitlab::GithubImport::ParallelScheduling) }
+
+  describe '#representation_class' do
+    it { expect(subject.representation_class).to eq(Gitlab::GithubImport::Representation::PullRequestReview) }
+  end
+
+  describe '#importer_class' do
+    it { expect(subject.importer_class).to eq(Gitlab::GithubImport::Importer::PullRequestReviewImporter) }
+  end
+
+  describe '#collection_method' do
+    it { expect(subject.collection_method).to eq(:pull_request_reviews) }
+  end
+
+  describe '#id_for_already_imported_cache' do
+    it { expect(subject.id_for_already_imported_cache(double(github_id: 1))).to eq(1) }
+  end
+
+  describe '#each_object_to_import' do
+    it 'fetchs the merged pull requests data' do
+      merge_request = create(:merge_request, source_project: project)
+      review = double
+
+      expect(review)
+        .to receive(:merge_request_id=)
+        .with(merge_request.id)
+
+      allow(client)
+        .to receive(:pull_request_reviews)
+        .with('github/repo', merge_request.iid)
+        .and_return([review])
+
+      expect { |b| subject.each_object_to_import(&b) }.to yield_with_args(review)
+    end
+  end
+end

spec/lib/gitlab/github_import/representation/pull_request_review_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::GithubImport::Representation::PullRequestReview do
+  let(:submitted_at) { Time.new(2017, 1, 1, 12, 00).utc }
+
+  shared_examples 'a PullRequest review' do
+    it 'returns an instance of PullRequest' do
+      expect(review).to be_an_instance_of(described_class)
+      expect(review.author).to be_an_instance_of(Gitlab::GithubImport::Representation::User)
+      expect(review.author.id).to eq(4)
+      expect(review.author.login).to eq('alice')
+      expect(review.note).to eq('note')
+      expect(review.review_type).to eq('APPROVED')
+      expect(review.submitted_at).to eq(submitted_at)
+      expect(review.github_id).to eq(999)
+      expect(review.merge_request_id).to eq(42)
+    end
+  end
+
+  describe '.from_api_response' do
+    let(:response) do
+      double(
+        :response,
+        id: 999,
+        merge_request_id: 42,
+        body: 'note',
+        state: 'APPROVED',
+        user: double(:user, id: 4, login: 'alice'),
+        submitted_at: submitted_at
+      )
+    end
+
+    it_behaves_like 'a PullRequest review' do
+      let(:review) { described_class.from_api_response(response) }
+    end
+
+    it 'does not set the user if the response did not include a user' do
+      allow(response)
+        .to receive(:user)
+        .and_return(nil)
+
+      review = described_class.from_api_response(response)
+
+      expect(review.author).to be_nil
+    end
+  end
+
+  describe '.from_json_hash' do
+    let(:hash) do
+      {
+        'github_id' => 999,
+        'merge_request_id' => 42,
+        'note' => 'note',
+        'review_type' => 'APPROVED',
+        'author' => { 'id' => 4, 'login' => 'alice' },
+        'submitted_at' => submitted_at.to_s
+      }
+    end
+
+    it_behaves_like 'a PullRequest review' do
+      let(:review) { described_class.from_json_hash(hash) }
+    end
+
+    it 'does not set the user if the response did not include a user' do
+      review = described_class.from_json_hash(hash.except('author'))
+
+      expect(review.author).to be_nil
+    end
+  end
+end

spec/workers/gitlab/github_import/import_pull_request_review_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::GithubImport::ImportPullRequestReviewWorker do
+  it { is_expected.to include_module(Gitlab::GithubImport::ObjectImporter) }
+
+  describe '#representation_class' do
+    it { expect(subject.representation_class).to eq(Gitlab::GithubImport::Representation::PullRequestReview) }
+  end
+
+  describe '#importer_class' do
+    it { expect(subject.importer_class).to eq(Gitlab::GithubImport::Importer::PullRequestReviewImporter) }
+  end
+
+  describe '#counter_name' do
+    it { expect(subject.counter_name).to eq(:github_importer_imported_pull_request_reviews) }
+  end
+
+  describe '#counter_description' do
+    it { expect(subject.counter_description).to eq('The number of imported GitHub pull request reviews') }
+  end
+end

spec/workers/gitlab/github_import/stage/import_pull_requests_merged_by_worker_spec.rb

@@ -27,7 +27,7 @@ RSpec.describe Gitlab::GithubImport::Stage::ImportPullRequestsMergedByWorker do
 
       expect(Gitlab::GithubImport::AdvanceStageWorker)
         .to receive(:perform_async)
-        .with(project.id, { '123' => 2 }, :issues_and_diff_notes)
+        .with(project.id, { '123' => 2 }, :pull_request_reviews)
 
       worker.import(client, project)
     end

spec/workers/gitlab/github_import/stage/import_pull_requests_reviews_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::GithubImport::Stage::ImportPullRequestsReviewsWorker do
+  let(:project) { create(:project) }
+  let(:import_state) { create(:import_state, project: project) }
+  let(:worker) { described_class.new }
+  let(:client) { double(:client) }
+
+  describe '#import' do
+    it 'does not import with the feature disabled' do
+      stub_feature_flags(github_import_pull_request_reviews: false)
+
+      expect(Gitlab::JobWaiter)
+        .to receive(:new)
+        .and_return(double(key: '123', jobs_remaining: 0))
+
+      expect(Gitlab::GithubImport::AdvanceStageWorker)
+        .to receive(:perform_async)
+        .with(project.id, { '123' => 0 }, :issues_and_diff_notes)
+
+      worker.import(client, project)
+    end
+
+    it 'imports all the pull request reviews' do
+      stub_feature_flags(github_import_pull_request_reviews: true)
+
+      importer = double(:importer)
+
+      waiter = Gitlab::JobWaiter.new(2, '123')
+
+      expect(Gitlab::GithubImport::Importer::PullRequestsReviewsImporter)
+        .to receive(:new)
+        .with(project, client)
+        .and_return(importer)
+
+      expect(importer)
+        .to receive(:execute)
+        .and_return(waiter)
+
+      expect(import_state)
+        .to receive(:refresh_jid_expiration)
+
+      expect(Gitlab::GithubImport::AdvanceStageWorker)
+        .to receive(:perform_async)
+        .with(project.id, { '123' => 2 }, :issues_and_diff_notes)
+
+      worker.import(client, project)
+    end
+  end
+end