Merge branch 'gh-pull-requests' into 'master'

CHANGELOG

@@ -49,6 +49,7 @@ v 8.11.0 (unreleased)
   - Document that webhook secret token is sent in X-Gitlab-Token HTTP header !5664 (lycoperdon)
   - Gitlab::Highlight is now instrumented
   - All created issues, API or WebUI, can be submitted to Akismet for spam check !5333
+  - Allow users to import cross-repository pull requests from GitHub
   - The overhead of instrumented method calls has been reduced
   - Remove `search_id` of labels dropdown filter to fix 'Missleading URI for labels in Merge Requests and Issues view'. !5368 (Scott Le)
   - Load project invited groups and members eagerly in `ProjectTeam#fetch_members`

app/views/import/github/status.html.haml

@@ -4,10 +4,6 @@
   %i.fa.fa-github
   Import projects from GitHub
 
-%p
-  %i.fa.fa-warning
-  To import GitHub pull requests, any pull request source branches that had been deleted are temporarily restored on GitHub. To prevent any connected CI services from being overloaded with dozens of irrelevant branches being created and deleted again, GitHub webhooks are temporarily disabled during the import process, but only if you have admin access to the GitHub repository.
-
 %p.light
   Select projects you want to import.
 %hr

doc/workflow/importing/import_projects_from_github.md

@@ -18,9 +18,6 @@ At its current state, GitHub importer can import:
 
 With GitLab 8.7+, references to pull requests and issues are preserved.
 
-It is not yet possible to import your cross-repository pull requests (those from
-forks). We are working on improving this in the near future.
-
 The importer page is visible when you [create a new project][new-project].
 Click on the **GitHub** link and, if you are logged in via the GitHub
 integration, you will be redirected to GitHub for permission to access your

lib/gitlab/github_import/branch_formatter.rb

@@ -7,10 +7,6 @@ module Gitlab
         branch_exists? && commit_exists?
       end
 
-      def name
-        @name ||= exists? ? ref : "#{ref}-#{short_id}"
-      end
-
       def valid?
         repo.present?
       end

lib/gitlab/github_import/hook_formatter.rb

-module Gitlab
-  module GithubImport
-    class HookFormatter
-      EVENTS = %w[* create delete pull_request push].freeze
-
-      attr_reader :raw
-
-      delegate :id, :name, :active, to: :raw
-
-      def initialize(raw)
-        @raw = raw
-      end
-
-      def config
-        raw.config.attrs
-      end
-
-      def valid?
-        (EVENTS & raw.events).any? && active
-      end
-    end
-  end
-end

lib/gitlab/github_import/importer.rb

@@ -12,7 +12,6 @@ module Gitlab
 
         if credentials
           @client = Client.new(credentials[:user])
-          @formatter = Gitlab::ImportFormatter.new
         else
           raise Projects::ImportService::Error, "Unable to find project import data credentials for project ID: #{@project.id}"
         end
@@ -66,73 +65,45 @@ module Gitlab
       end
 
       def import_pull_requests
-        disable_webhooks
-
         pull_requests = client.pull_requests(repo, state: :all, sort: :created, direction: :asc, per_page: 100)
         pull_requests = pull_requests.map { |raw| PullRequestFormatter.new(project, raw) }.select(&:valid?)
 
-        source_branches_removed = pull_requests.reject(&:source_branch_exists?).map { |pr| [pr.source_branch_name, pr.source_branch_sha] }
-        target_branches_removed = pull_requests.reject(&:target_branch_exists?).map { |pr| [pr.target_branch_name, pr.target_branch_sha] }
-        branches_removed = source_branches_removed | target_branches_removed
-
-        restore_branches(branches_removed)
-
         pull_requests.each do |pull_request|
-          merge_request = pull_request.create!
-          apply_labels(merge_request)
-          import_comments(merge_request)
-          import_comments_on_diff(merge_request)
+          begin
+            restore_source_branch(pull_request) unless pull_request.source_branch_exists?
+            restore_target_branch(pull_request) unless pull_request.target_branch_exists?
+
+            merge_request = pull_request.create!
+            apply_labels(merge_request)
+            import_comments(merge_request)
+            import_comments_on_diff(merge_request)
+          rescue ActiveRecord::RecordInvalid => e
+            raise Projects::ImportService::Error, e.message
+          ensure
+            clean_up_restored_branches(pull_request)
+          end
         end
 
         true
-      rescue ActiveRecord::RecordInvalid => e
-        raise Projects::ImportService::Error, e.message
-      ensure
-        clean_up_restored_branches(branches_removed)
-        clean_up_disabled_webhooks
       end
 
-      def disable_webhooks
-        update_webhooks(hooks, active: false)
+      def restore_source_branch(pull_request)
+        project.repository.fetch_ref(repo_url, "pull/#{pull_request.number}/head", pull_request.source_branch_name)
       end
 
-      def clean_up_disabled_webhooks
-        update_webhooks(hooks, active: true)
+      def restore_target_branch(pull_request)
+        project.repository.create_branch(pull_request.target_branch_name, pull_request.target_branch_sha)
       end
 
-      def update_webhooks(hooks, options)
-        hooks.each do |hook|
-          client.edit_hook(repo, hook.id, hook.name, hook.config, options)
-        end
+      def remove_branch(name)
+        project.repository.delete_branch(name)
+      rescue Rugged::ReferenceError
+        nil
       end
 
-      def hooks
-        @hooks ||=
-          begin
-            client.hooks(repo).map { |raw| HookFormatter.new(raw) }.select(&:valid?)
-
-          # The GitHub Repository Webhooks API returns 404 for users
-          # without admin access to the repository when listing hooks.
-          # In this case we just want to return gracefully instead of
-          # spitting out an error and stop the import process.
-          rescue Octokit::NotFound
-            []
-          end
-      end
-
-      def restore_branches(branches)
-        branches.each do |name, sha|
-          client.create_ref(repo, "refs/heads/#{name}", sha)
-        end
-
-        project.repository.fetch_ref(repo_url, '+refs/heads/*', 'refs/heads/*')
-      end
-
-      def clean_up_restored_branches(branches)
-        branches.each do |name, _|
-          client.delete_ref(repo, "heads/#{name}")
-          project.repository.delete_branch(name) rescue Rugged::ReferenceError
-        end
+      def clean_up_restored_branches(pull_request)
+        remove_branch(pull_request.source_branch_name) unless pull_request.source_branch_exists?
+        remove_branch(pull_request.target_branch_name) unless pull_request.target_branch_exists?
 
         project.repository.after_remove_branch
       end

lib/gitlab/github_import/pull_request_formatter.rb

 module Gitlab
   module GithubImport
     class PullRequestFormatter < BaseFormatter
-      delegate :exists?, :name, :project, :repo, :sha, to: :source_branch, prefix: true
-      delegate :exists?, :name, :project, :repo, :sha, to: :target_branch, prefix: true
+      delegate :exists?, :project, :ref, :repo, :sha, to: :source_branch, prefix: true
+      delegate :exists?, :project, :ref, :repo, :sha, to: :target_branch, prefix: true
 
       def attributes
         {
@@ -33,17 +33,29 @@ module Gitlab
       end
 
       def valid?
-        source_branch.valid? && target_branch.valid? && !cross_project?
+        source_branch.valid? && target_branch.valid?
       end
 
       def source_branch
         @source_branch ||= BranchFormatter.new(project, raw_data.head)
       end
 
+      def source_branch_name
+        @source_branch_name ||= begin
+          source_branch_exists? ? source_branch_ref : "pull/#{number}/#{source_branch_ref}"
+        end
+      end
+
       def target_branch
         @target_branch ||= BranchFormatter.new(project, raw_data.base)
       end
 
+      def target_branch_name
+        @target_branch_name ||= begin
+          target_branch_exists? ? target_branch_ref : "pull/#{number}/#{target_branch_ref}"
+        end
+      end
+
       private
 
       def assigned?
@@ -68,10 +80,6 @@ module Gitlab
         raw_data.body || ""
       end
 
-      def cross_project?
-        source_branch_repo.id != target_branch_repo.id
-      end
-
       def description
         formatter.author_line(author) + body
       end

spec/lib/gitlab/github_import/branch_formatter_spec.rb

@@ -32,20 +32,6 @@ describe Gitlab::GithubImport::BranchFormatter, lib: true do
     end
   end
 
-  describe '#name' do
-    it 'returns raw ref when branch exists' do
-      branch = described_class.new(project, double(raw))
-
-      expect(branch.name).to eq 'feature'
-    end
-
-    it 'returns formatted ref when branch does not exist' do
-      branch = described_class.new(project, double(raw.merge(ref: 'removed-branch', sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b')))
-
-      expect(branch.name).to eq 'removed-branch-2e5d3239'
-    end
-  end
-
   describe '#repo' do
     it 'returns raw repo' do
       branch = described_class.new(project, double(raw))

spec/lib/gitlab/github_import/hook_formatter_spec.rb

-require 'spec_helper'
-
-describe Gitlab::GithubImport::HookFormatter, lib: true do
-  describe '#id' do
-    it 'returns raw id' do
-      raw = double(id: 100000)
-      formatter = described_class.new(raw)
-      expect(formatter.id).to eq 100000
-    end
-  end
-
-  describe '#name' do
-    it 'returns raw id' do
-      raw = double(name: 'web')
-      formatter = described_class.new(raw)
-      expect(formatter.name).to eq 'web'
-    end
-  end
-
-  describe '#config' do
-    it 'returns raw config.attrs' do
-      raw = double(config: double(attrs: { url: 'http://something.com/webhook' }))
-      formatter = described_class.new(raw)
-      expect(formatter.config).to eq({ url: 'http://something.com/webhook' })
-    end
-  end
-
-  describe '#valid?' do
-    it 'returns true when events contains the wildcard event' do
-      raw = double(events: ['*', 'commit_comment'], active: true)
-      formatter = described_class.new(raw)
-      expect(formatter.valid?).to eq true
-    end
-
-    it 'returns true when events contains the create event' do
-      raw = double(events: ['create', 'commit_comment'], active: true)
-      formatter = described_class.new(raw)
-      expect(formatter.valid?).to eq true
-    end
-
-    it 'returns true when events contains delete event' do
-      raw = double(events: ['delete', 'commit_comment'], active: true)
-      formatter = described_class.new(raw)
-      expect(formatter.valid?).to eq true
-    end
-
-    it 'returns true when events contains pull_request event' do
-      raw = double(events: ['pull_request', 'commit_comment'], active: true)
-      formatter = described_class.new(raw)
-      expect(formatter.valid?).to eq true
-    end
-
-    it 'returns false when events does not contains branch related events' do
-      raw = double(events: ['member', 'commit_comment'], active: true)
-      formatter = described_class.new(raw)
-      expect(formatter.valid?).to eq false
-    end
-
-    it 'returns false when hook is not active' do
-      raw = double(events: ['pull_request', 'commit_comment'], active: false)
-      formatter = described_class.new(raw)
-      expect(formatter.valid?).to eq false
-    end
-  end
-end

spec/lib/gitlab/github_import/pull_request_formatter_spec.rb

@@ -9,6 +9,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
   let(:source_branch) { double(ref: 'feature', repo: source_repo, sha: source_sha) }
   let(:target_repo) { repository }
   let(:target_branch) { double(ref: 'master', repo: target_repo, sha: target_sha) }
+  let(:removed_branch) { double(ref: 'removed-branch', repo: source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b') }
   let(:octocat) { double(id: 123456, login: 'octocat') }
   let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') }
   let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') }
@@ -165,6 +166,42 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
     end
   end
 
+  describe '#source_branch_name' do
+    context 'when source branch exists' do
+      let(:raw_data) { double(base_data) }
+
+      it 'returns branch ref' do
+        expect(pull_request.source_branch_name).to eq 'feature'
+      end
+    end
+
+    context 'when source branch does not exist' do
+      let(:raw_data) { double(base_data.merge(head: removed_branch)) }
+
+      it 'prefixes branch name with pull request number' do
+        expect(pull_request.source_branch_name).to eq 'pull/1347/removed-branch'
+      end
+    end
+  end
+
+  describe '#target_branch_name' do
+    context 'when source branch exists' do
+      let(:raw_data) { double(base_data) }
+
+      it 'returns branch ref' do
+        expect(pull_request.target_branch_name).to eq 'master'
+      end
+    end
+
+    context 'when target branch does not exist' do
+      let(:raw_data) { double(base_data.merge(base: removed_branch)) }
+
+      it 'prefixes branch name with pull request number' do
+        expect(pull_request.target_branch_name).to eq 'pull/1347/removed-branch'
+      end
+    end
+  end
+
   describe '#valid?' do
     context 'when source, and target repos are not a fork' do
       let(:raw_data) { double(base_data) }
@@ -178,8 +215,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
       let(:source_repo) { double(id: 2) }
       let(:raw_data) { double(base_data) }
 
-      it 'returns false' do
-        expect(pull_request.valid?).to eq false
+      it 'returns true' do
+        expect(pull_request.valid?).to eq true
       end
     end
 
@@ -187,8 +224,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
       let(:target_repo) { double(id: 2) }
       let(:raw_data) { double(base_data) }
 
-      it 'returns false' do
-        expect(pull_request.valid?).to eq false
+      it 'returns true' do
+        expect(pull_request.valid?).to eq true
       end
     end
   end