summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/models/service.rb2
-rw-r--r--changelogs/unreleased/validate_service_project_id_nil_if_template.yml5
-rw-r--r--db/migrate/20200305151736_delete_template_project_services.rb19
-rw-r--r--spec/controllers/admin/services_controller_spec.rb10
-rw-r--r--spec/factories/services.rb16
-rw-r--r--spec/lib/gitlab/usage_data_spec.rb2
-rw-r--r--spec/migrations/delete_template_project_services_spec.rb21
-rw-r--r--spec/models/service_spec.rb11
8 files changed, 71 insertions, 15 deletions
diff --git a/app/models/service.rb b/app/models/service.rb
index 8f1772e67f9..d866eeb3531 100644
--- a/app/models/service.rb
+++ b/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? }
diff --git a/changelogs/unreleased/validate_service_project_id_nil_if_template.yml b/changelogs/unreleased/validate_service_project_id_nil_if_template.yml
new file mode 100644
index 00000000000..a04683705ef
--- /dev/null
+++ b/changelogs/unreleased/validate_service_project_id_nil_if_template.yml
@@ -0,0 +1,5 @@
+---
+title: Validate absence of project_id if service is a template
+merge_request: 26563
+author:
+type: other
diff --git a/db/migrate/20200305151736_delete_template_project_services.rb b/db/migrate/20200305151736_delete_template_project_services.rb
new file mode 100644
index 00000000000..2ab8d46a94e
--- /dev/null
+++ b/db/migrate/20200305151736_delete_template_project_services.rb
@@ -0,0 +1,19 @@
+# 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
diff --git a/spec/controllers/admin/services_controller_spec.rb b/spec/controllers/admin/services_controller_spec.rb
index 44233776865..35801643181 100644
--- a/spec/controllers/admin/services_controller_spec.rb
+++ b/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
diff --git a/spec/factories/services.rb b/spec/factories/services.rb
index ebab370ccf6..2d6d8d71917 100644
--- a/spec/factories/services.rb
+++ b/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
diff --git a/spec/lib/gitlab/usage_data_spec.rb b/spec/lib/gitlab/usage_data_spec.rb
index fed33c89726..113cb4ba6bf 100644
--- a/spec/lib/gitlab/usage_data_spec.rb
+++ b/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)
diff --git a/spec/migrations/delete_template_project_services_spec.rb b/spec/migrations/delete_template_project_services_spec.rb
new file mode 100644
index 00000000000..3c6709ec310
--- /dev/null
+++ b/spec/migrations/delete_template_project_services_spec.rb
@@ -0,0 +1,21 @@
+# 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
diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb
index 825e5f5902f..d0673b21329 100644
--- a/spec/models/service_spec.rb
+++ b/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'
View Lorry Controller Status