From 856a511b4804a0b78294a29bbba86ac111d960f8 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Fri, 5 May 2017 18:57:52 +0200
Subject: [PATCH] refactor code based on feedback

---
 .../projects/propagate_service_template.rb     | 12 ++++++------
 .../propagate_service_template_worker.rb       |  2 --
 .../propagate_service_template_spec.rb         | 18 +++++++++++-------
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/app/services/projects/propagate_service_template.rb b/app/services/projects/propagate_service_template.rb
index 2999e1af385..a8ef2108492 100644
--- a/app/services/projects/propagate_service_template.rb
+++ b/app/services/projects/propagate_service_template.rb
@@ -11,7 +11,7 @@ module Projects
     end
 
     def propagate
-      return unless @template&.active?
+      return unless @template.active?
 
       Rails.logger.info("Propagating services for template #{@template.id}")
 
@@ -32,11 +32,11 @@ module Projects
 
     def bulk_create_from_template(batch)
       service_list = batch.map do |project_id|
-        service_hash.merge('project_id' => project_id).values
+        service_hash.values << project_id
       end
 
       Project.transaction do
-        bulk_insert_services(service_hash.keys + ['project_id'], service_list)
+        bulk_insert_services(service_hash.keys << 'project_id', service_list)
         run_callbacks(batch)
       end
     end
@@ -75,9 +75,9 @@ module Projects
 
           template_hash.each_with_object({}) do |(key, value), service_hash|
             value = value.is_a?(Hash) ? value.to_json : value
-            key = Gitlab::Database.postgresql? ? "\"#{key}\"" : "`#{key}`"
 
-            service_hash[key] = ActiveRecord::Base.sanitize(value)
+            service_hash[ActiveRecord::Base.connection.quote_column_name(key)] =
+              ActiveRecord::Base.sanitize(value)
           end
         end
     end
@@ -93,7 +93,7 @@ module Projects
     end
 
     def active_external_issue_tracker?
-      @template.category == :issue_tracker && !@template.default
+      @template.issue_tracker? && !@template.default
     end
 
     def active_external_wiki?
diff --git a/app/workers/propagate_service_template_worker.rb b/app/workers/propagate_service_template_worker.rb
index f1fc7ccb955..5ce0e0405d0 100644
--- a/app/workers/propagate_service_template_worker.rb
+++ b/app/workers/propagate_service_template_worker.rb
@@ -3,8 +3,6 @@ class PropagateServiceTemplateWorker
   include Sidekiq::Worker
   include DedicatedSidekiqQueue
 
-  sidekiq_options retry: 3
-
   LEASE_TIMEOUT = 4.hours.to_i
 
   def perform(template_id)
diff --git a/spec/services/projects/propagate_service_template_spec.rb b/spec/services/projects/propagate_service_template_spec.rb
index 331fb3c5ac5..90eff3bbc1e 100644
--- a/spec/services/projects/propagate_service_template_spec.rb
+++ b/spec/services/projects/propagate_service_template_spec.rb
@@ -18,8 +18,11 @@ describe Projects::PropagateServiceTemplate, services: true do
     let!(:project) { create(:empty_project) }
 
     it 'creates services for projects' do
-      expect { described_class.propagate(service_template) }.
-        to change { Service.count }.by(1)
+      expect(project.pushover_service).to be_nil
+
+      described_class.propagate(service_template)
+
+      expect(project.reload.pushover_service).to be_present
     end
 
     it 'creates services for a project that has another service' do
@@ -35,8 +38,11 @@ describe Projects::PropagateServiceTemplate, services: true do
         }
       )
 
-      expect { described_class.propagate(service_template) }.
-        to change { Service.count }.by(1)
+      expect(project.pushover_service).to be_nil
+
+      described_class.propagate(service_template)
+
+      expect(project.reload.pushover_service).to be_present
     end
 
     it 'does not create the service if it exists already' do
@@ -61,9 +67,7 @@ describe Projects::PropagateServiceTemplate, services: true do
     it 'creates the service containing the template attributes' do
       described_class.propagate(service_template)
 
-      service = Service.find_by!(type: service_template.type, template: false)
-
-      expect(service.properties).to eq(service_template.properties)
+      expect(project.pushover_service.properties).to eq(service_template.properties)
     end
 
     describe 'bulk update' do