Merge branch 'validate_service_project_id_nil_if_template' into 'master'

Validate absence of project_id if service template

See merge request gitlab-org/gitlab!26563

app/models/service.rb

@@ -33,7 +33,7 @@ class Service < ApplicationRecord
   has_one :service_hook
 
   validates :project_id, presence: true, unless: -> { template? || instance? }
-  validates :project_id, absence: true, if: -> { instance? }
+  validates :project_id, absence: true, if: -> { template? || instance? }
   validates :type, presence: true
   validates :template, uniqueness: { scope: :type }, if: -> { template? }
   validates :instance, uniqueness: { scope: :type }, if: -> { instance? }

changelogs/unreleased/validate_service_project_id_nil_if_template.yml

+---
+title: Validate absence of project_id if service is a template
+merge_request: 26563
+author:
+type: other

db/migrate/20200305151736_delete_template_project_services.rb

+# frozen_string_literal: true
+
+class DeleteTemplateProjectServices < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    # In 12.9 an ActiveRecord validation for services not being a template and
+    # attached to a project at the same time is introduced. This migration cleans up invalid data.
+    execute <<~SQL
+      DELETE
+      FROM services
+      WHERE TEMPLATE = TRUE AND project_id IS NOT NULL
+    SQL
+  end
+
+  def down
+    # This migration cannot be reversed.
+  end
+end

spec/controllers/admin/services_controller_spec.rb

@@ -30,9 +30,9 @@ describe Admin::ServicesController do
 
   describe "#update" do
     let(:project) { create(:project) }
-    let!(:service) do
+    let!(:service_template) do
       RedmineService.create(
-        project: project,
+        project: nil,
         active: false,
         template: true,
         properties: {
@@ -44,9 +44,9 @@ describe Admin::ServicesController do
     end
 
     it 'calls the propagation worker when service is active' do
-      expect(PropagateServiceTemplateWorker).to receive(:perform_async).with(service.id)
+      expect(PropagateServiceTemplateWorker).to receive(:perform_async).with(service_template.id)
 
-      put :update, params: { id: service.id, service: { active: true } }
+      put :update, params: { id: service_template.id, service: { active: true } }
 
       expect(response).to have_gitlab_http_status(:found)
     end
@@ -54,7 +54,7 @@ describe Admin::ServicesController do
     it 'does not call the propagation worker when service is not active' do
       expect(PropagateServiceTemplateWorker).not_to receive(:perform_async)
 
-      put :update, params: { id: service.id, service: { properties: {} } }
+      put :update, params: { id: service_template.id, service: { properties: {} } }
 
       expect(response).to have_gitlab_http_status(:found)
     end

spec/factories/services.rb

@@ -4,11 +4,6 @@ FactoryBot.define do
   factory :service do
     project
     type { 'Service' }
-
-    trait :instance do
-      project { nil }
-      instance { true }
-    end
   end
 
   factory :custom_issue_tracker_service, class: 'CustomIssueTrackerService' do
@@ -69,6 +64,7 @@ FactoryBot.define do
   factory :jira_service do
     project
     active { true }
+    type { 'JiraService' }
 
     transient do
       create_data { true }
@@ -159,4 +155,14 @@ FactoryBot.define do
       IssueTrackerService.set_callback(:validation, :before, :handle_properties)
     end
   end
+
+  trait :template do
+    project { nil }
+    template { true }
+  end
+
+  trait :instance do
+    project { nil }
+    instance { true }
+  end
 end

spec/lib/gitlab/usage_data_spec.rb

@@ -27,7 +27,7 @@ describe Gitlab::UsageData do
           create(:service, project: projects[1], type: 'SlackService', active: true)
           create(:service, project: projects[2], type: 'SlackService', active: true)
           create(:service, project: projects[2], type: 'MattermostService', active: false)
-          create(:service, project: projects[2], type: 'MattermostService', active: true, template: true)
+          create(:service, :template, type: 'MattermostService', active: true)
           create(:service, project: projects[2], type: 'CustomIssueTrackerService', active: true)
           create(:project_error_tracking_setting, project: projects[0])
           create(:project_error_tracking_setting, project: projects[1], enabled: false)

spec/migrations/delete_template_project_services_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200305151736_delete_template_project_services.rb')
+
+describe DeleteTemplateProjectServices, :migration do
+  let(:services) { table(:services) }
+  let(:project) { table(:projects).create!(namespace_id: 1) }
+
+  before do
+    services.create!(template: true, project_id: project.id)
+    services.create!(template: true)
+    services.create!(template: false, project_id: project.id)
+  end
+
+  it 'deletes services when template and attached to a project' do
+    expect { migrate! }.to change { services.where(template: true, project_id: project.id).count }.from(1).to(0)
+      .and not_change { services.where(template: true, project_id: nil).count }
+      .and not_change { services.where(template: false).where.not(project_id: nil).count }
+  end
+end

spec/models/service_spec.rb

@@ -28,17 +28,22 @@ describe Service do
       expect(build(:service, instance: true)).to be_invalid
     end
 
+    it 'validates absence of project_id if template', :aggregate_failures do
+      expect(build(:service, template: true)).to validate_absence_of(:project_id)
+      expect(build(:service, template: false)).not_to validate_absence_of(:project_id)
+    end
+
     it 'validates service is template or instance' do
       expect(build(:service, project_id: nil, template: true, instance: true)).to be_invalid
     end
 
     context 'with an existing service template' do
       before do
-        create(:service, type: 'Service', template: true)
+        create(:service, :template)
       end
 
       it 'validates only one service template per type' do
-        expect(build(:service, type: 'Service', template: true)).to be_invalid
+        expect(build(:service, :template)).to be_invalid
       end
     end
 
@@ -192,7 +197,7 @@ describe Service do
 
         context 'when data are stored in separated fields' do
           let(:template) do
-            create(:jira_service, data_params.merge(properties: {}, title: title, description: description, template: true))
+            create(:jira_service, :template, data_params.merge(properties: {}, title: title, description: description))
           end
 
           it_behaves_like 'service creation from a template'