Github importer - import pull request merged by

The merged by field is not available on the list of pull requests, which
is used to import the merge request. Therefore, for each merged pull
request we need to do another request to Github to fetch this field.

Once fetched, if the user can be mapped to a GitLab user, it's added as
to the `MergeRequest::Metrics#merged_by`. Otherwise, a note is added to
the merge request with the text: 'Merged by "login"', where "login" is
the Github login of the user who merged the pull request in Github.

app/workers/all_queues.yml

@@ -707,6 +707,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: github_importer:github_import_import_pull_request_merged_by
+  :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:
@@ -763,6 +771,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: github_importer:github_import_stage_import_pull_requests_merged_by
+  :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

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

app/workers/gitlab/github_import/import_pull_request_merged_by_worker.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module GithubImport
+    class ImportPullRequestMergedByWorker # rubocop:disable Scalability/IdempotentWorker
+      include ObjectImporter
+
+      def representation_class
+        Gitlab::GithubImport::Representation::PullRequest
+      end
+
+      def importer_class
+        Importer::PullRequestMergedByImporter
+      end
+
+      def counter_name
+        :github_importer_imported_pull_requests_merged_by
+      end
+
+      def counter_description
+        'The number of imported GitHub pull requests merged by'
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module GithubImport
+    module Stage
+      class ImportPullRequestsMergedByWorker # 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 = Importer::PullRequestsMergedByImporter
+            .new(project, client)
+            .execute
+
+          project.import_state.refresh_jid_expiration
+
+          AdvanceStageWorker.perform_async(
+            project.id,
+            { waiter.key => waiter.jobs_remaining },
+            :issues_and_diff_notes
+          )
+        end
+      end
+    end
+  end
+end

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

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

changelogs/unreleased/kassio-add-github-merged-by.yml

+---
+title: Github Importer - import the pull request `merged by` field
+merge_request: 48561
+author:
+type: changed

lib/gitlab/github_import/client.rb

@@ -76,6 +76,10 @@ module Gitlab
         with_rate_limit { octokit.repo(name) }
       end
 
+      def pull_request(repo_name, iid)
+        with_rate_limit { octokit.pull_request(repo_name, iid) }
+      end
+
       def labels(*args)
         each_object(:labels, *args)
       end

lib/gitlab/github_import/importer/pull_request_merged_by_importer.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module GithubImport
+    module Importer
+      class PullRequestMergedByImporter
+        def initialize(pull_request, project, client)
+          @project = project
+          @pull_request = pull_request
+          @client = client
+        end
+
+        def execute
+          merge_request = project.merge_requests.find_by_iid(pull_request.iid)
+          user_finder = GithubImport::UserFinder.new(project, client)
+          gitlab_user_id = user_finder.user_id_for(pull_request.merged_by)
+
+          if gitlab_user_id
+            timestamp = Time.new.utc
+            MergeRequest::Metrics.upsert({
+              target_project_id: project.id,
+              merge_request_id: merge_request.id,
+              merged_by_id: gitlab_user_id,
+              created_at: timestamp,
+              updated_at: timestamp
+            }, unique_by: :merge_request_id)
+          else
+            merge_request.notes.create!(
+              note: "*Merged by: #{pull_request.merged_by.login}*",
+              author_id: project.creator_id,
+              project: project,
+              created_at: pull_request.created_at
+            )
+          end
+        end
+
+        private
+
+        attr_reader :project, :pull_request, :client
+      end
+    end
+  end
+end

lib/gitlab/github_import/importer/pull_requests_merged_by_importer.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module GithubImport
+    module Importer
+      class PullRequestsMergedByImporter
+        include ParallelScheduling
+
+        def importer_class
+          PullRequestMergedByImporter
+        end
+
+        def representation_class
+          Gitlab::GithubImport::Representation::PullRequest
+        end
+
+        def sidekiq_worker_class
+          ImportPullRequestMergedByWorker
+        end
+
+        def collection_method
+          :pull_requests_merged_by
+        end
+
+        def id_for_already_imported_cache(pr)
+          pr.number
+        end
+
+        def each_object_to_import
+          project.merge_requests.with_state(:merged).find_each do |merge_request|
+            pull_request = client.pull_request(project.import_source, merge_request.iid)
+            yield(pull_request)
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/github_import/representation/pull_request.rb

@@ -13,18 +13,16 @@ module Gitlab
                          :source_branch_sha, :target_branch, :target_branch_sha,
                          :milestone_number, :author, :assignee, :created_at,
                          :updated_at, :merged_at, :source_repository_id,
-                         :target_repository_id, :source_repository_owner
+                         :target_repository_id, :source_repository_owner, :merged_by
 
         # Builds a PR from a GitHub API response.
         #
         # issue - An instance of `Sawyer::Resource` containing the PR details.
         def self.from_api_response(pr)
-          assignee =
-            if pr.assignee
-              Representation::User.from_api_response(pr.assignee)
-            end
-
+          assignee = Representation::User.from_api_response(pr.assignee) if pr.assignee
           user = Representation::User.from_api_response(pr.user) if pr.user
+          merged_by = Representation::User.from_api_response(pr.merged_by) if pr.merged_by
+
           hash = {
             iid: pr.number,
             title: pr.title,
@@ -42,7 +40,8 @@ module Gitlab
             assignee: assignee,
             created_at: pr.created_at,
             updated_at: pr.updated_at,
-            merged_at: pr.merged_at
+            merged_at: pr.merged_at,
+            merged_by: merged_by
           }
 
           new(hash)
@@ -57,8 +56,8 @@ module Gitlab
 
           # Assignees are optional so we only convert it from a Hash if one was
           # set.
-          hash[:assignee] &&= Representation::User
-            .from_json_hash(hash[:assignee])
+          hash[:assignee] &&= Representation::User.from_json_hash(hash[:assignee])
+          hash[:merged_by] &&= Representation::User.from_json_hash(hash[:merged_by])
 
           new(hash)
         end

spec/lib/gitlab/github_import/client_spec.rb

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

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

@@ -43,7 +43,7 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestImporter, :clean_gitla
 
   describe '#execute' do
     it 'imports the pull request' do
-      mr = double(:merge_request, id: 10)
+      mr = double(:merge_request, id: 10, merged?: false)
 
       expect(importer)
         .to receive(:create_merge_request)

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::GithubImport::Importer::PullRequestMergedByImporter, :clean_gitlab_redis_cache do
+  let_it_be(:merge_request) { create(:merged_merge_request) }
+  let(:project) { merge_request.project }
+  let(:created_at) { Time.new(2017, 1, 1, 12, 00).utc }
+  let(:client_double) { double(user: double(id: 999, login: 'merger', email: 'merger@email.com')) }
+
+  let(:pull_request) do
+    instance_double(
+      Gitlab::GithubImport::Representation::PullRequest,
+      iid: merge_request.iid,
+      created_at: created_at,
+      merged_by: double(id: 999, login: 'merger')
+    )
+  end
+
+  subject { described_class.new(pull_request, project, client_double) }
+
+  it 'assigns the merged by user when mapped' do
+    merge_user = create(:user, email: 'merger@email.com')
+
+    subject.execute
+
+    expect(merge_request.metrics.reload.merged_by).to eq(merge_user)
+  end
+
+  it 'adds a note referencing the merger user when the user cannot be mapped' do
+    expect { subject.execute }.to change(Note, :count).by(1)
+
+    last_note = merge_request.notes.last
+
+    expect(last_note.note).to eq("*Merged by: merger*")
+    expect(last_note.created_at).to eq(created_at)
+    expect(last_note.author).to eq(project.creator)
+  end
+end

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

@@ -29,6 +29,7 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestsImporter do
       milestone: double(:milestone, number: 4),
       user: double(:user, id: 4, login: 'alice'),
       assignee: double(:user, id: 4, login: 'alice'),
+      merged_by: double(:user, id: 4, login: 'alice'),
       created_at: 1.second.ago,
       updated_at: 1.second.ago,
       merged_at: 1.second.ago

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::GithubImport::Importer::PullRequestsMergedByImporter do
+  let(:client) { double }
+  let(:project) { create(:project, import_source: 'http://somegithub.com') }
+
+  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::PullRequest) }
+  end
+
+  describe '#importer_class' do
+    it { expect(subject.importer_class).to eq(Gitlab::GithubImport::Importer::PullRequestMergedByImporter) }
+  end
+
+  describe '#collection_method' do
+    it { expect(subject.collection_method).to eq(:pull_requests_merged_by) }
+  end
+
+  describe '#id_for_already_imported_cache' do
+    it { expect(subject.id_for_already_imported_cache(double(number: 1))).to eq(1) }
+  end
+
+  describe '#each_object_to_import' do
+    it 'fetchs the merged pull requests data' do
+      pull_request = double
+      create(
+        :merged_merge_request,
+        iid: 999,
+        source_project: project,
+        target_project: project
+      )
+
+      allow(client)
+        .to receive(:pull_request)
+        .with('http://somegithub.com', 999)
+        .and_return(pull_request)
+
+      expect { |b| subject.each_object_to_import(&b) }.to yield_with_args(pull_request)
+    end
+  end
+end

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

@@ -115,6 +115,7 @@ RSpec.describe Gitlab::GithubImport::Representation::PullRequest do
         milestone: double(:milestone, number: 4),
         user: double(:user, id: 4, login: 'alice'),
         assignee: double(:user, id: 4, login: 'alice'),
+        merged_by: double(:user, id: 4, login: 'alice'),
         created_at: created_at,
         updated_at: updated_at,
         merged_at: merged_at

spec/workers/gitlab/github_import/import_pull_request_merged_by_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::GithubImport::ImportPullRequestMergedByWorker do
+  it { is_expected.to include_module(Gitlab::GithubImport::ObjectImporter) }
+
+  describe '#representation_class' do
+    it { expect(subject.representation_class).to eq(Gitlab::GithubImport::Representation::PullRequest) }
+  end
+
+  describe '#importer_class' do
+    it { expect(subject.importer_class).to eq(Gitlab::GithubImport::Importer::PullRequestMergedByImporter) }
+  end
+
+  describe '#counter_name' do
+    it { expect(subject.counter_name).to eq(:github_importer_imported_pull_requests_merged_by) }
+  end
+
+  describe '#counter_description' do
+    it { expect(subject.counter_description).to eq('The number of imported GitHub pull requests merged by') }
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::GithubImport::Stage::ImportPullRequestsMergedByWorker do
+  let(:project) { create(:project) }
+  let(:import_state) { create(:import_state, project: project) }
+  let(:worker) { described_class.new }
+
+  describe '#import' do
+    it 'imports all the pull requests' do
+      importer = double(:importer)
+      client = double(:client)
+      waiter = Gitlab::JobWaiter.new(2, '123')
+
+      expect(Gitlab::GithubImport::Importer::PullRequestsMergedByImporter)
+        .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

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

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