refactor a few things based on feedback

app/controllers/admin/services_controller.rb

@@ -15,7 +15,9 @@ class Admin::ServicesController < Admin::ApplicationController
   end
   def update
-    if service.update_and_propagate(service_params[:service])
+    if service.update_attributes(service_params[:service])
+      PropagateProjectServiceWorker.perform_async(service.id) if  service.active?
+
       redirect_to admin_application_settings_services_path,
         notice: 'Application settings saved successfully'
     else

app/models/service.rb

@@ -254,16 +254,6 @@ class Service < ActiveRecord::Base
     service
   end
-  def update_and_propagate(service_params)
-    return false unless update_attributes(service_params)
-
-    if service_params[:active]
-      PropagateProjectServiceWorker.perform_async(id)
-    end
-
-    true
-  end
-
   private
   def cache_project_has_external_issue_tracker

app/services/projects/propagate_service.rb

@@ -2,15 +2,15 @@ module Projects
   class PropagateService
     BATCH_SIZE = 100
-    def self.propagate!(*args)
-      new(*args).propagate!
+    def self.propagate(*args)
+      new(*args).propagate
     end
     def initialize(template)
       @template = template
     end
-    def propagate!
+    def propagate
       return unless @template&.active
       Rails.logger.info("Propagating services for template #{@template.id}")
@@ -28,7 +28,7 @@ module Projects
         batch.each { |project_id| create_from_template(project_id) }
-        break if batch.count < BATCH_SIZE
+        break if batch.size < BATCH_SIZE
         offset += BATCH_SIZE
       end

app/workers/propagate_project_service_worker.rb

@@ -10,7 +10,7 @@ class PropagateProjectServiceWorker
   def perform(template_id)
     return unless try_obtain_lease_for(template_id)
-    Projects::PropagateService.propagate!(Service.find_by(id: template_id))
+    Projects::PropagateService.propagate(Service.find_by(id: template_id))
   end
   private

spec/controllers/admin/services_controller_spec.rb

@@ -23,4 +23,36 @@ describe Admin::ServicesController do
       end
     end
   end
+
+  describe "#update" do
+    let(:project) { create(:empty_project) }
+    let!(:service) do
+      RedmineService.create(
+        project: project,
+        active: false,
+        template: true,
+        properties: {
+          project_url: 'http://abc',
+          issues_url: 'http://abc',
+          new_issue_url: 'http://abc'
+        }
+      )
+    end
+
+    it 'updates the service params successfully and calls the propagation worker' do
+      expect(PropagateProjectServiceWorker).to receive(:perform_async).with(service.id)
+
+      put :update, id: service.id, service: { active: true }
+
+      expect(response).to have_http_status(302)
+    end
+
+    it 'updates the service params successfully' do
+      expect(PropagateProjectServiceWorker).not_to receive(:perform_async)
+
+      put :update, id: service.id, service: { properties: {} }
+
+      expect(response).to have_http_status(302)
+    end
+  end
 end

spec/models/service_spec.rb

@@ -254,31 +254,4 @@ describe Service, models: true do
       end
     end
   end
-
-  describe "#update_and_propagate" do
-    let(:project) { create(:empty_project) }
-    let!(:service) do
-      RedmineService.create(
-        project: project,
-        active: false,
-        properties: {
-          project_url: 'http://abc',
-          issues_url: 'http://abc',
-          new_issue_url: 'http://abc'
-        }
-      )
-    end
-
-    it 'updates the service params successfully and calls the propagation worker' do
-      expect(PropagateProjectServiceWorker).to receive(:perform_async).with(service.id)
-
-      expect(service.update_and_propagate(active: true)).to be true
-    end
-
-    it 'updates the service params successfully' do
-      expect(PropagateProjectServiceWorker).not_to receive(:perform_async)
-
-      expect(service.update_and_propagate(properties: {})).to be true
-    end
-  end
 end

spec/services/projects/propagate_service_spec.rb

@@ -18,7 +18,7 @@ describe Projects::PropagateService, services: true do
     let!(:project) { create(:empty_project) }
     it 'creates services for projects' do
-      expect { described_class.propagate!(service_template) }.
+      expect { described_class.propagate(service_template) }.
         to change { Service.count }.by(1)
     end
@@ -36,7 +36,7 @@ describe Projects::PropagateService, services: true do
       Service.build_from_template(project.id, other_service).save!
-      expect { described_class.propagate!(service_template) }.
+      expect { described_class.propagate(service_template) }.
         to change { Service.count }.by(1)
     end
@@ -55,12 +55,12 @@ describe Projects::PropagateService, services: true do
       Service.build_from_template(project.id, service_template).save!
       Service.build_from_template(project.id, other_service).save!
-      expect { described_class.propagate!(service_template) }.
+      expect { described_class.propagate(service_template) }.
         not_to change { Service.count }
     end
     it 'creates the service containing the template attributes' do
-      described_class.propagate!(service_template)
+      described_class.propagate(service_template)
       service = Service.find_by(type: service_template.type, template: false)

spec/workers/propagate_project_service_worker_spec.rb

@@ -21,7 +21,7 @@ describe PropagateProjectServiceWorker do
   describe '#perform' do
     it 'calls the propagate service with the template' do
-      expect(Projects::PropagateService).to receive(:propagate!).with(service_template)
+      expect(Projects::PropagateService).to receive(:propagate).with(service_template)
       subject.perform(service_template.id)
     end