Merge branch 'fix/gh-import-bugs' into 'master'

Fix couple of GitHub importing bugs

Fix a bug in GH comment importing and label applying for imported MRs.

See merge request !7139

4 files changed, 26 insertions, 13 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9151a27e100..56d9d4e2809 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -27,6 +27,8 @@ Please view this file on the master branch, on stable branches it's out of date.
   - Fix Sign in page 'Forgot your password?' link overlaps on medium-large screens
   - Show full status link on MR & commit pipelines
   - Fix documents and comments on Build API `scope`
+  - Fix applying labels for GitHub-imported MRs
+  - Fix importing MR comments from GitHub
   - Refactor email, use setter method instead AR callbacks for email attribute (Semyon Pupkov)
   - Shortened merge request modal to let clipboard button not overlap
 
diff --git a/lib/gitlab/github_import/client.rb b/lib/gitlab/github_import/client.rb
index 7f424b74efb..348005d5659 100644
--- a/lib/gitlab/github_import/client.rb
+++ b/lib/gitlab/github_import/client.rb
@@ -105,18 +105,20 @@ module Gitlab
         data = api.send(method, *args)
         return data unless data.is_a?(Array)
 
+        last_response = api.last_response
+
         if block_given?
           yield data
-          each_response_page(&block)
+          # api.last_response could change while we're yielding (e.g. fetching labels for each PR)
+          # so we cache our own last request
+          each_response_page(last_response, &block)
         else
-          each_response_page { |page| data.concat(page) }
+          each_response_page(last_response) { |page| data.concat(page) }
           data
         end
       end
 
-      def each_response_page
-        last_response = api.last_response
-
+      def each_response_page(last_response)
         while last_response.rels[:next]
           sleep rate_limit_sleep_time if rate_limit_exceed?
           last_response = last_response.rels[:next].get
diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb
index 4b70f33a851..27946dff608 100644
--- a/lib/gitlab/github_import/importer.rb
+++ b/lib/gitlab/github_import/importer.rb
@@ -132,8 +132,15 @@ module Gitlab
       end
 
       def apply_labels(issuable, raw_issuable)
-        if raw_issuable.labels.count > 0
-          label_ids = raw_issuable.labels
+        # GH returns labels for issues but not for pull requests!
+        labels = if issuable.is_a?(MergeRequest)
+                   client.labels_for_issue(repo, raw_issuable.number)
+                 else
+                   raw_issuable.labels
+                 end
+
+        if labels.count > 0
+          label_ids = labels
             .map { |attrs| @labels[attrs.name] }
             .compact
 
@@ -143,21 +150,22 @@ module Gitlab
 
       def import_comments
         client.issues_comments(repo, per_page: 100) do |comments|
-          create_comments(comments, :issue)
+          create_comments(comments)
         end
 
         client.pull_requests_comments(repo, per_page: 100) do |comments|
-          create_comments(comments, :pull_request)
+          create_comments(comments)
         end
       end
 
-      def create_comments(comments, issuable_type)
+      def create_comments(comments)
         ActiveRecord::Base.no_touching do
           comments.each do |raw|
             begin
-              comment        = CommentFormatter.new(project, raw)
-              issuable_class = issuable_type == :issue ? Issue : MergeRequest
-              iid            = raw.send("#{issuable_type}_url").split('/').last # GH doesn't return parent ID directly
+              comment         = CommentFormatter.new(project, raw)
+              # GH does not return info about comment's parent, so we guess it by checking its URL!
+              *_, parent, iid = URI(raw.html_url).path.split('/')
+              issuable_class = parent == 'issues' ? Issue : MergeRequest
               issuable       = issuable_class.find_by_iid(iid)
               next unless issuable
 
diff --git a/spec/lib/gitlab/github_import/importer_spec.rb b/spec/lib/gitlab/github_import/importer_spec.rb
index 1af553f8f03..84f280ceaae 100644
--- a/spec/lib/gitlab/github_import/importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer_spec.rb
@@ -154,6 +154,7 @@ describe Gitlab::GithubImport::Importer, lib: true do
             { type: :label, url: "https://api.github.com/repos/octocat/Hello-World/labels/bug", errors: "Validation failed: Title can't be blank, Title is invalid" },
             { type: :milestone, url: "https://api.github.com/repos/octocat/Hello-World/milestones/1", errors: "Validation failed: Title has already been taken" },
             { type: :issue, url: "https://api.github.com/repos/octocat/Hello-World/issues/1348", errors: "Validation failed: Title can't be blank, Title is too short (minimum is 0 characters)" },
+            { type: :pull_request, url: "https://api.github.com/repos/octocat/Hello-World/pulls/1347", errors: "Invalid Repository. Use user/repo format." },
             { type: :pull_request, url: "https://api.github.com/repos/octocat/Hello-World/pulls/1347", errors: "Validation failed: Validate branches Cannot Create: This merge request already exists: [\"New feature\"]" },
             { type: :wiki, errors: "Gitlab::Shell::Error" },
             { type: :release, url: 'https://api.github.com/repos/octocat/Hello-World/releases/2', errors: "Validation failed: Description can't be blank" }