Merge branch 'pedropombeiro/349542/5-extract-unassign-runner-service' into 'master'

Extract service to unassign runner from project

See merge request gitlab-org/gitlab!81532

app/controllers/admin/runner_projects_controller.rb

@@ -18,7 +18,8 @@ class Admin::RunnerProjectsController < Admin::ApplicationController
   def destroy
     rp = Ci::RunnerProject.find(params[:id])
     runner = rp.runner
-    rp.destroy
+
+    ::Ci::Runners::UnassignRunnerService.new(rp, current_user).execute
 
     redirect_to edit_admin_runner_url(runner), status: :found, notice: s_('Runners|Runner unassigned from project.')
   end

app/controllers/projects/runner_projects_controller.rb

@@ -26,7 +26,8 @@ class Projects::RunnerProjectsController < Projects::ApplicationController
 
   def destroy
     runner_project = project.runner_projects.find(params[:id])
-    runner_project.destroy
+
+    ::Ci::Runners::UnassignRunnerService.new(runner_project, current_user).execute
 
     redirect_to project_runners_path(project), status: :found, notice: s_('Runners|Runner unassigned from project.')
   end

app/services/ci/runners/unassign_runner_service.rb

+# frozen_string_literal: true
+
+module Ci
+  module Runners
+    class UnassignRunnerService
+      # @param [Ci::RunnerProject] runner_project the runner/project association to destroy
+      # @param [User] user the user performing the operation
+      def initialize(runner_project, user)
+        @runner = runner_project.runner
+        @runner_project = runner_project
+        @user = user
+      end
+
+      def execute
+        return false unless @user.present? && @user.can?(:assign_runner, @runner)
+
+        @runner_project.destroy
+      end
+    end
+  end
+end

spec/controllers/admin/runner_projects_controller_spec.rb

@@ -13,7 +13,7 @@ RSpec.describe Admin::RunnerProjectsController do
   describe '#create' do
     let(:project_id) { project.path }
 
-    subject do
+    subject(:send_create) do
       post :create, params: {
         namespace_id: group.path,
         project_id: project_id,
@@ -25,7 +25,7 @@ RSpec.describe Admin::RunnerProjectsController do
       let(:project_runner) { create(:ci_runner, :project, projects: [project]) }
 
       it 'redirects to the admin runner edit page' do
-        subject
+        send_create
 
         expect(response).to have_gitlab_http_status(:redirect)
         expect(response).to redirect_to edit_admin_runner_url(project_runner)
@@ -37,7 +37,7 @@ RSpec.describe Admin::RunnerProjectsController do
       let(:source_project) { create(:project) }
 
       it 'redirects to the admin runner edit page' do
-        subject
+        send_create
 
         expect(response).to have_gitlab_http_status(:redirect)
         expect(response).to redirect_to edit_admin_runner_url(project_runner)
@@ -50,7 +50,42 @@ RSpec.describe Admin::RunnerProjectsController do
       let(:project_id) { 0 }
 
       it 'shows 404 for unknown project' do
-        subject
+        send_create
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+  end
+
+  describe '#destroy' do
+    let_it_be(:project_runner) { create(:ci_runner, :project, projects: [project]) }
+
+    let(:project_id) { project.path }
+
+    subject(:send_destroy) do
+      delete :destroy, params: {
+        namespace_id: group.path,
+        project_id: project_id,
+        id: runner_project_id
+      }
+    end
+
+    context 'unassigning runner from project' do
+      let(:runner_project_id) { project_runner.runner_projects.last.id }
+
+      it 'redirects to the admin runner edit page' do
+        send_destroy
+
+        expect(response).to have_gitlab_http_status(:redirect)
+        expect(response).to redirect_to edit_admin_runner_url(project_runner)
+      end
+    end
+
+    context 'for unknown project runner relationship' do
+      let(:runner_project_id) { 0 }
+
+      it 'shows 404 for unknown project runner relationship' do
+        send_destroy
 
         expect(response).to have_gitlab_http_status(:not_found)
       end

spec/services/ci/runners/unassign_runner_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Ci::Runners::UnassignRunnerService, '#execute' do
+  subject(:service) { described_class.new(runner_project, user).execute }
+
+  let_it_be(:runner) { create(:ci_runner, :project, projects: [project]) }
+  let_it_be(:project) { create(:project) }
+
+  let(:runner_project) { runner.runner_projects.last }
+
+  context 'without user' do
+    let(:user) { nil }
+
+    it 'does not destroy runner_project', :aggregate_failures do
+      expect(runner_project).not_to receive(:destroy)
+      expect { service }.not_to change { runner.runner_projects.count }.from(1)
+
+      is_expected.to eq(false)
+    end
+  end
+
+  context 'with unauthorized user' do
+    let(:user) { build(:user) }
+
+    it 'does not call destroy on runner_project' do
+      expect(runner).not_to receive(:destroy)
+
+      service
+    end
+  end
+
+  context 'with admin user', :enable_admin_mode do
+    let(:user) { create_default(:user, :admin) }
+
+    it 'destroys runner_project' do
+      expect(runner_project).to receive(:destroy).once
+
+      service
+    end
+  end
+end