Prefer service object over after_save hook

Closes #26921

app/controllers/admin/runners_controller.rb

@@ -13,7 +13,7 @@ class Admin::RunnersController < Admin::ApplicationController
   end
 
   def update
-    if @runner.update_attributes(runner_params)
+    if Ci::UpdateRunnerService.new(@runner).update(runner_params)
       respond_to do |format|
         format.js
         format.html { redirect_to admin_runner_path(@runner) }
@@ -31,7 +31,7 @@ class Admin::RunnersController < Admin::ApplicationController
   end
 
   def resume
-    if @runner.update_attributes(active: true)
+    if Ci::UpdateRunnerService.new(@runner).update(active: true)
       redirect_to admin_runners_path, notice: 'Runner was successfully updated.'
     else
       redirect_to admin_runners_path, alert: 'Runner was not updated.'
@@ -39,7 +39,7 @@ class Admin::RunnersController < Admin::ApplicationController
   end
 
   def pause
-    if @runner.update_attributes(active: false)
+    if Ci::UpdateRunnerService.new(@runner).update(active: false)
       redirect_to admin_runners_path, notice: 'Runner was successfully updated.'
     else
       redirect_to admin_runners_path, alert: 'Runner was not updated.'

app/controllers/projects/runners_controller.rb

@@ -16,7 +16,7 @@ class Projects::RunnersController < Projects::ApplicationController
   end
 
   def update
-    if @runner.update_attributes(runner_params)
+    if Ci::UpdateRunnerService.new(@runner).update(runner_params)
       redirect_to runner_path(@runner), notice: 'Runner was successfully updated.'
     else
       render 'edit'
@@ -32,7 +32,7 @@ class Projects::RunnersController < Projects::ApplicationController
   end
 
   def resume
-    if @runner.update_attributes(active: true)
+    if Ci::UpdateRunnerService.new(@runner).update(active: true)
       redirect_to runner_path(@runner), notice: 'Runner was successfully updated.'
     else
       redirect_to runner_path(@runner), alert: 'Runner was not updated.'
@@ -40,7 +40,7 @@ class Projects::RunnersController < Projects::ApplicationController
   end
 
   def pause
-    if @runner.update_attributes(active: false)
+    if Ci::UpdateRunnerService.new(@runner).update(active: false)
       redirect_to runner_path(@runner), notice: 'Runner was successfully updated.'
     else
       redirect_to runner_path(@runner), alert: 'Runner was not updated.'

app/models/ci/runner.rb

@@ -22,8 +22,6 @@ module Ci
     scope :online, ->() { where('contacted_at > ?', LAST_CONTACT_TIME) }
     scope :ordered, ->() { order(id: :desc) }
 
-    after_save :tick_runner_queue, if: :form_editable_changed?
-
     scope :owned_or_shared, ->(project_id) do
       joins('LEFT JOIN ci_runner_projects ON ci_runner_projects.runner_id = ci_runners.id')
         .where("ci_runner_projects.gl_project_id = :project_id OR ci_runners.is_shared = true", project_id: project_id)
@@ -149,12 +147,6 @@ module Ci
       "runner:build_queue:#{self.token}"
     end
 
-    def form_editable_changed?
-      FORM_EDITABLE.any? do |editable|
-        public_send("#{editable}_changed?")
-      end
-    end
-
     def tag_constraints
       unless has_tags? || run_untagged?
         errors.add(:tags_list,

app/services/ci/update_runner_service.rb

@@ -0,0 +1,15 @@
+module Ci
+  class UpdateRunnerService
+    attr_reader :runner
+
+    def initialize(runner)
+      @runner = runner
+    end
+
+    def update(params)
+      runner.update(params).tap do
+        runner.tick_runner_queue
+      end
+    end
+  end
+end

lib/ci/api/runners.rb

@@ -28,23 +28,27 @@ module Ci
         post "register" do
           required_attributes! [:token]
 
-          attributes = attributes_for_keys(
+          project = nil
-            [:description, :tag_list, :run_untagged, :locked]
-          )
-
           runner =
             if runner_registration_token_valid?
               # Create shared runner. Requires admin access
-              Ci::Runner.create(attributes.merge(is_shared: true))
+              Ci::Runner.new(is_shared: true)
             elsif project = Project.find_by(runners_token: params[:token])
-              # Create a specific runner for project.
+              Ci::Runner.new
-              project.runners.create(attributes)
             end
 
           return forbidden! unless runner
 
+          attributes = attributes_for_keys(
+            [:description, :tag_list, :run_untagged, :locked]
+          ).merge(get_runner_version_from_params || {})
+
+          Ci::UpdateRunnerService.new(runner).update(attributes)
+
+          # Assign the specific runner for the project
+          project.runners << runner if project
+
           if runner.id
-            runner.update(get_runner_version_from_params)
             present runner, with: Entities::Runner
           else
             not_found!

spec/models/ci/runner_spec.rb

@@ -291,7 +291,7 @@ describe Ci::Runner, models: true do
       let!(:last_update) { runner.ensure_runner_queue_value }
 
       before do
-        runner.update(description: 'new runner')
+        Ci::UpdateRunnerService.new(runner).update(description: 'new runner')
       end
 
       it 'sets a new last_update value' do

spec/services/ci/update_runner_service_spec.rb

@@ -0,0 +1,18 @@
+require 'spec_helper'
+
+describe Ci::UpdateRunnerService, services: true do
+  let(:runner) { create(:ci_runner) }
+
+  describe '#update' do
+    before do
+      allow(runner).to receive(:tick_runner_queue)
+
+      described_class.new(runner).update(description: 'new runner')
+    end
+
+    it 'updates the runner and ticking the queue' do
+      expect(runner.description).to eq('new runner')
+      expect(runner).to have_received(:tick_runner_queue)
+    end
+  end
+end