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
Signed-off-by: Rémy Coutable <remy@rymai.me>

CHANGELOG

 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.11.7
+  - Avoid conflict with admin labels when importing GitHub labels. !6158
   - Restores `fieldName` to allow only string values in `gl_dropdown.js`. !6234
   - Allow the Rails cookie to be used for API authentication.
 

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)

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

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)" },

spec/lib/gitlab/github_import/label_formatter_spec.rb

 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