Merge branch 'fix-gh-pr-import' into 'master'

Fix importing PR's from GitHub when the source repo was removed

Closes #13847 
Closes gitlab-com/support-forum#584

See merge request !3172

3 files changed, 40 insertions, 45 deletions

diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb
index e2a85f29825..172c5441e36 100644
--- a/lib/gitlab/github_import/importer.rb
+++ b/lib/gitlab/github_import/importer.rb
@@ -45,10 +45,13 @@ module Gitlab
                                                     direction: :asc).each do |raw_data|
           pull_request = PullRequestFormatter.new(project, raw_data)
 
-          if !pull_request.cross_project? && pull_request.valid?
-            merge_request = MergeRequest.create!(pull_request.attributes)
-            import_comments(pull_request.number, merge_request)
-            import_comments_on_diff(pull_request.number, merge_request)
+          if pull_request.valid?
+            merge_request = MergeRequest.new(pull_request.attributes)
+
+            if merge_request.save
+              import_comments(pull_request.number, merge_request)
+              import_comments_on_diff(pull_request.number, merge_request)
+            end
           end
         end
 
diff --git a/lib/gitlab/github_import/pull_request_formatter.rb b/lib/gitlab/github_import/pull_request_formatter.rb
index f96fed0f5cf..4e507b090e8 100644
--- a/lib/gitlab/github_import/pull_request_formatter.rb
+++ b/lib/gitlab/github_import/pull_request_formatter.rb
@@ -17,16 +17,12 @@ module Gitlab
         }
       end
 
-      def cross_project?
-        source_repo.id != target_repo.id
-      end
-
       def number
         raw_data.number
       end
 
       def valid?
-        source_branch.present? && target_branch.present?
+        !cross_project? && source_branch.present? && target_branch.present?
       end
 
       private
@@ -53,6 +49,10 @@ module Gitlab
         raw_data.body || ""
       end
 
+      def cross_project?
+        source_repo.present? && target_repo.present? && source_repo.id != target_repo.id
+      end
+
       def description
         formatter.author_line(author) + body
       end
diff --git a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb
index 6cebcb5009a..e49dcb42342 100644
--- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb
+++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb
@@ -127,34 +127,6 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
     end
   end
 
-  describe '#cross_project?' do
-    context 'when source, and target repositories are the same' do
-      let(:raw_data) { OpenStruct.new(base_data) }
-
-      it 'returns false' do
-        expect(pull_request.cross_project?).to eq false
-      end
-    end
-
-    context 'when source repo is a fork' do
-      let(:source_repo) { OpenStruct.new(id: 2, fork: true) }
-      let(:raw_data) { OpenStruct.new(base_data) }
-
-      it 'returns true' do
-        expect(pull_request.cross_project?).to eq true
-      end
-    end
-
-    context 'when target repo is a fork' do
-      let(:target_repo) { OpenStruct.new(id: 2, fork: true) }
-      let(:raw_data) { OpenStruct.new(base_data) }
-
-      it 'returns true' do
-        expect(pull_request.cross_project?).to eq true
-      end
-    end
-  end
-
   describe '#number' do
     let(:raw_data) { OpenStruct.new(base_data.merge(number: 1347)) }
 
@@ -166,24 +138,44 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
   describe '#valid?' do
     let(:invalid_branch) { OpenStruct.new(ref: 'invalid-branch') }
 
-    context 'when source and target branches exists' do
-      let(:raw_data) { OpenStruct.new(base_data.merge(head: source_branch, base: target_branch)) }
+    context 'when source, and target repositories are the same' do
+      context 'and source and target branches exists' do
+        let(:raw_data) { OpenStruct.new(base_data.merge(head: source_branch, base: target_branch)) }
 
-      it 'returns true' do
-        expect(pull_request.valid?).to eq true
+        it 'returns true' do
+          expect(pull_request.valid?).to eq true
+        end
+      end
+
+      context 'and source branch doesn not exists' do
+        let(:raw_data) { OpenStruct.new(base_data.merge(head: invalid_branch, base: target_branch)) }
+
+        it 'returns false' do
+          expect(pull_request.valid?).to eq false
+        end
+      end
+
+      context 'and target branch doesn not exists' do
+        let(:raw_data) { OpenStruct.new(base_data.merge(head: source_branch, base: invalid_branch)) }
+
+        it 'returns false' do
+          expect(pull_request.valid?).to eq false
+        end
       end
     end
 
-    context 'when source branch doesn not exists' do
-      let(:raw_data) { OpenStruct.new(base_data.merge(head: invalid_branch, base: target_branch)) }
+    context 'when source repo is a fork' do
+      let(:source_repo) { OpenStruct.new(id: 2, fork: true) }
+      let(:raw_data) { OpenStruct.new(base_data) }
 
       it 'returns false' do
         expect(pull_request.valid?).to eq false
       end
     end
 
-    context 'when target branch doesn not exists' do
-      let(:raw_data) { OpenStruct.new(base_data.merge(head: source_branch, base: invalid_branch)) }
+    context 'when target repo is a fork' do
+      let(:target_repo) { OpenStruct.new(id: 2, fork: true) }
+      let(:raw_data) { OpenStruct.new(base_data) }
 
       it 'returns false' do
         expect(pull_request.valid?).to eq false