Show available services onlyshow_available_services_only

Since services use STI, deleting a subclass can cause errors when a row
for a deleted class is queried. Since we want to delete services but not
loose data, we instead filter only for available services.

5 files changed, 37 insertions, 14 deletions

diff --git a/app/controllers/admin/services_controller.rb b/app/controllers/admin/services_controller.rb
index c455930c044..649cbbd1f18 100644
--- a/app/controllers/admin/services_controller.rb
+++ b/app/controllers/admin/services_controller.rb
@@ -30,20 +30,16 @@ class Admin::ServicesController < Admin::ApplicationController
 
   private
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def services_templates
     Service.available_services_names.map do |service_name|
       service_template = "#{service_name}_service".camelize.constantize
-      service_template.where(template: true).first_or_create
+      service_template.templates.first_or_create
     end
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def service
-    @service ||= Service.where(id: params[:id], template: true).first
+    @service ||= Service.templates.find_by_id(params[:id])
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   def whitelist_query_limiting
     Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42430')
diff --git a/app/models/project.rb b/app/models/project.rb
index 20895923d3b..fb713b96ed2 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1101,7 +1101,7 @@ class Project < ApplicationRecord
   def find_or_initialize_service(name)
     return if disabled_services.include?(name)
 
-    service = find_service(services, name)
+    service = find_service(available_services, name)
     return service if service
 
     # We should check if template for the service exists
@@ -2271,6 +2271,14 @@ class Project < ApplicationRecord
   end
 
   def services_templates
-    @services_templates ||= Service.where(template: true)
+    strong_memoize(:services_templates) do
+      Service.available.templates
+    end
+  end
+
+  def available_services
+    strong_memoize(:available_services) do
+      services.available
+    end
   end
 end
diff --git a/app/models/service.rb b/app/models/service.rb
index 9896aa12e90..93b8211651a 100644
--- a/app/models/service.rb
+++ b/app/models/service.rb
@@ -53,6 +53,10 @@ class Service < ApplicationRecord
   scope :deployment_hooks, -> { where(deployment_events: true, active: true) }
   scope :external_issue_trackers, -> { issue_trackers.active.without_defaults }
   scope :deployment, -> { where(category: 'deployment') }
+  scope :templates, -> { where(template: true) }
+  scope :available, -> (names = available_services_names) {
+    where(type: names.map { |name| "#{name}_service".camelize })
+  }
 
   default_value_for :category, 'common'
 
@@ -68,10 +72,6 @@ class Service < ApplicationRecord
     true
   end
 
-  def template?
-    template
-  end
-
   def category
     read_attribute(:category).to_sym
   end
@@ -299,7 +299,7 @@ class Service < ApplicationRecord
   end
 
   def self.find_by_template
-    find_by(template: true)
+    templates.first
   end
 
   private
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 453f9761602..875ad59d556 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -4543,7 +4543,7 @@ describe Project do
   end
 
   describe "#find_or_initialize_services" do
-    subject { build(:project) }
+    subject { create(:project) }
 
     it 'returns only enabled services' do
       allow(Service).to receive(:available_services_names).and_return(%w(prometheus pushover))
@@ -4554,6 +4554,16 @@ describe Project do
       expect(services.count).to eq 1
       expect(services).to include(PushoverService)
     end
+
+    it 'returns only available services' do
+      allow(Service).to receive(:available_services_names).and_return(%w(pushover))
+      create(Service, type: 'UnavailableService', project: subject)
+
+      services = subject.find_or_initialize_services
+
+      expect(services.count).to eq 1
+      expect(services).to include(PushoverService)
+    end
   end
 
   describe "#find_or_initialize_service" do
diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb
index 64db32781fe..85f51a9fb62 100644
--- a/spec/models/service_spec.rb
+++ b/spec/models/service_spec.rb
@@ -26,6 +26,15 @@ describe Service do
         expect(described_class.confidential_note_hooks.count).to eq 0
       end
     end
+
+    describe '.available' do
+      let!(:available_service) { create(:kubernetes_service) }
+      let!(:unavailable_service) { create(:service, type: 'UnavailableService') }
+
+      it 'includes only available services' do
+        expect(described_class.available).to contain_exactly(available_service)
+      end
+    end
   end
 
   describe "Test Button" do