Merge branch 'github-avoid-conflicts-with-admin-labels' into 'master'

Avoid conflict with Admin labels when importing GitHub labels

If the GitHub project have duplicated labels from the Admin labels, the importer will use the Admin label.

Fixes #21319

See merge request !6158

5 files changed, 41 insertions, 11 deletions

diff --git a/CHANGELOG b/CHANGELOG
index f528ca074fa..9d1c8ea45b2 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -102,6 +102,7 @@ v 8.12.0 (unreleased)
   - Use the default branch for displaying the project icon instead of master !5792 (Hannes Rosenögger)
   - Adds response mime type to transaction metric action when it's not HTML
   - Fix hover leading space bug in pipeline graph !5980
+  - Avoid conflict with admin labels when importing GitHub labels
   - User can edit closed MR with deleted fork (Katarzyna Kobierska Ula Budziszewska) !5496
   - Fix repository page ui issues
   - Fixed invisible scroll controls on build page on iPhone
diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb
index 4fdc2f46be0..0388c58f811 100644
--- a/lib/gitlab/github_import/importer.rb
+++ b/lib/gitlab/github_import/importer.rb
@@ -133,8 +133,7 @@ module Gitlab
 
         if issue.labels.count > 0
           label_ids = issue.labels
-            .map { |raw| LabelFormatter.new(project, raw).attributes }
-            .map { |attrs| Label.find_by(attrs).try(:id) }
+            .map { |attrs| project.labels.find_by(title: attrs.name).try(:id) }
             .compact
 
           issuable.update_attribute(:label_ids, label_ids)
diff --git a/lib/gitlab/github_import/label_formatter.rb b/lib/gitlab/github_import/label_formatter.rb
index 9f18244e7d7..2cad7fca88e 100644
--- a/lib/gitlab/github_import/label_formatter.rb
+++ b/lib/gitlab/github_import/label_formatter.rb
@@ -13,6 +13,12 @@ module Gitlab
         Label
       end
 
+      def create!
+        project.labels.find_or_create_by!(title: title) do |label|
+          label.color = color
+        end
+      end
+
       private
 
       def color
diff --git a/spec/lib/gitlab/github_import/importer_spec.rb b/spec/lib/gitlab/github_import/importer_spec.rb
index 3fb8de81545..7df288f619f 100644
--- a/spec/lib/gitlab/github_import/importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer_spec.rb
@@ -13,7 +13,7 @@ describe Gitlab::GithubImport::Importer, lib: true do
       let(:target_sha) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit).id }
       let(:target_branch) { double(ref: 'master', repo: repository, sha: target_sha) }
 
-      let(:label) do
+      let(:label1) do
         double(
           name: 'Bug',
           color: 'ff0000',
@@ -21,6 +21,14 @@ describe Gitlab::GithubImport::Importer, lib: true do
         )
       end
 
+      let(:label2) do
+        double(
+          name: nil,
+          color: 'ff0000',
+          url: 'https://api.github.com/repos/octocat/Hello-World/labels/bug'
+        )
+      end
+
       let(:milestone) do
         double(
           number: 1347,
@@ -93,7 +101,7 @@ describe Gitlab::GithubImport::Importer, lib: true do
       before do
         allow(project).to receive(:import_data).and_return(double.as_null_object)
         allow_any_instance_of(Octokit::Client).to receive(:rate_limit!).and_raise(Octokit::NotFound)
-        allow_any_instance_of(Octokit::Client).to receive(:labels).and_return([label, label])
+        allow_any_instance_of(Octokit::Client).to receive(:labels).and_return([label1, label2])
         allow_any_instance_of(Octokit::Client).to receive(:milestones).and_return([milestone, milestone])
         allow_any_instance_of(Octokit::Client).to receive(:issues).and_return([issue1, issue2])
         allow_any_instance_of(Octokit::Client).to receive(:pull_requests).and_return([pull_request, pull_request])
@@ -113,7 +121,7 @@ describe Gitlab::GithubImport::Importer, lib: true do
         error = {
           message: 'The remote data could not be fully imported.',
           errors: [
-            { type: :label, url: "https://api.github.com/repos/octocat/Hello-World/labels/bug", errors: "Validation failed: Title has already been taken" },
+            { 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/1347", errors: "Invalid Repository. Use user/repo format." },
             { 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)" },
diff --git a/spec/lib/gitlab/github_import/label_formatter_spec.rb b/spec/lib/gitlab/github_import/label_formatter_spec.rb
index 87593e32db0..8098754d735 100644
--- a/spec/lib/gitlab/github_import/label_formatter_spec.rb
+++ b/spec/lib/gitlab/github_import/label_formatter_spec.rb
@@ -1,18 +1,34 @@
 require 'spec_helper'
 
 describe Gitlab::GithubImport::LabelFormatter, lib: true do
-  describe '#attributes' do
-    it 'returns formatted attributes' do
-      project = create(:project)
-      raw = double(name: 'improvements', color: 'e6e6e6')
+  let(:project) { create(:project) }
+  let(:raw) { double(name: 'improvements', color: 'e6e6e6') }
 
-      formatter = described_class.new(project, raw)
+  subject { described_class.new(project, raw) }
 
-      expect(formatter.attributes).to eq({
+  describe '#attributes' do
+    it 'returns formatted attributes' do
+      expect(subject.attributes).to eq({
         project: project,
         title: 'improvements',
         color: '#e6e6e6'
       })
     end
   end
+
+  describe '#create!' do
+    context 'when label does not exist' do
+      it 'creates a new label' do
+        expect { subject.create! }.to change(Label, :count).by(1)
+      end
+    end
+
+    context 'when label exists' do
+      it 'does not create a new label' do
+        project.labels.create(name: raw.name)
+
+        expect { subject.create! }.not_to change(Label, :count)
+      end
+    end
+  end
 end