Merge branch 'unique-service-template-per-type' into 'master'

Validates only one service template per type

See merge request gitlab-org/gitlab!26380

app/models/service.rb

@@ -34,6 +34,7 @@ class Service < ApplicationRecord
 
   validates :project_id, presence: true, unless: -> { template? }
   validates :type, presence: true
+  validates :template, uniqueness: { scope: :type }, if: -> { template? }
 
   scope :visible, -> { where.not(type: 'GitlabIssueTrackerService') }
   scope :issue_trackers, -> { where(category: 'issue_tracker') }

changelogs/unreleased/unique-service-template-per-type.yml

+---
+title: Validates only one service template per type
+merge_request: 26380
+author:
+type: other

db/migrate/20200304160801_delete_template_services_duplicated_by_type.rb

+# frozen_string_literal: true
+
+class DeleteTemplateServicesDuplicatedByType < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    # Delete service templates with duplicated types. Keep the service
+    # template with the lowest `id` because that is the service template used:
+    # https://gitlab.com/gitlab-org/gitlab/-/blob/v12.8.1-ee/app/controllers/admin/services_controller.rb#L37
+    execute <<~SQL
+      DELETE
+      FROM services
+      WHERE TEMPLATE = TRUE
+        AND id NOT IN
+          (SELECT MIN(id)
+           FROM services
+           WHERE TEMPLATE = TRUE
+           GROUP BY TYPE);
+    SQL
+  end
+
+  def down
+    # This migration cannot be reversed.
+  end
+end

db/migrate/20200304160823_add_index_to_service_unique_template_per_type.rb

+# frozen_string_literal: true
+
+class AddIndexToServiceUniqueTemplatePerType < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:services, [:type, :template], unique: true, where: 'template IS TRUE')
+  end
+
+  def down
+    remove_concurrent_index(:services, [:type, :template])
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_03_04_090155) do
+ActiveRecord::Schema.define(version: 2020_03_04_160823) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -3898,6 +3898,7 @@ ActiveRecord::Schema.define(version: 2020_03_04_090155) do
     t.boolean "template", default: false
     t.index ["project_id"], name: "index_services_on_project_id"
     t.index ["template"], name: "index_services_on_template"
+    t.index ["type", "template"], name: "index_services_on_type_and_template", unique: true, where: "(template IS TRUE)"
     t.index ["type"], name: "index_services_on_type"
   end
 

spec/migrations/delete_template_services_duplicated_by_type_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200304160801_delete_template_services_duplicated_by_type.rb')
+
+describe DeleteTemplateServicesDuplicatedByType, :migration do
+  let(:services) { table(:services) }
+
+  before do
+    services.create!(template: true, type: 'JenkinsService')
+    services.create!(template: true, type: 'JenkinsService')
+    services.create!(template: true, type: 'JiraService')
+    services.create!(template: true, type: 'JenkinsService')
+  end
+
+  it 'deletes service templates duplicated by type except the one with the lowest ID' do
+    jenkins_service_id = services.where(type: 'JenkinsService').order(:id).pluck(:id).first
+    jira_service_id = services.where(type: 'JiraService').pluck(:id).first
+
+    migrate!
+
+    expect(services.pluck(:id)).to contain_exactly(jenkins_service_id, jira_service_id)
+  end
+end

spec/models/service_spec.rb

@@ -17,6 +17,16 @@ describe Service do
       expect(build(:service, project_id: nil, template: true)).to be_valid
       expect(build(:service, project_id: nil, template: false)).to be_invalid
     end
+
+    context 'with an existing service template' do
+      before do
+        create(:service, type: 'Service', template: true)
+      end
+
+      it 'validates only one service template per type' do
+        expect(build(:service, type: 'Service', template: true)).to be_invalid
+      end
+    end
   end
 
   describe 'Scopes' do