Merge branch '346304-refactor-retry-services' into 'master'

Add Detailed Error Message for Credit Card-less Users

See merge request gitlab-org/gitlab!80657

app/assets/javascripts/pipelines/components/header_component.vue

@@ -174,6 +174,8 @@ export default {
         });
 
         if (errors.length > 0) {
+          this.isRetrying = false;
+
           this.reportFailure(POST_FAILURE);
         } else {
           await this.$apollo.queries.pipeline.refetch();
@@ -182,6 +184,8 @@ export default {
           }
         }
       } catch {
+        this.isRetrying = false;
+
         this.reportFailure(POST_FAILURE);
       }
     },

app/controllers/projects/pipelines_controller.rb

@@ -161,14 +161,20 @@ class Projects::PipelinesController < Projects::ApplicationController
   end
 
   def retry
-    ::Ci::RetryPipelineWorker.perform_async(pipeline.id, current_user.id) # rubocop:disable CodeReuse/Worker
+    # Check for access before execution to allow for async execution while still returning access results
+    access_response = ::Ci::RetryPipelineService.new(@project, current_user).check_access(pipeline)
+
+    if access_response.error?
+      response = { json: { errors: [access_response.message] }, status: access_response.http_status }
+    else
+      response = { json: {}, status: :no_content }
+      ::Ci::RetryPipelineWorker.perform_async(pipeline.id, current_user.id) # rubocop:disable CodeReuse/Worker
+    end
 
     respond_to do |format|
-      format.html do
-        redirect_back_or_default default: project_pipelines_path(project)
+      format.json do
+        render response
       end
-
-      format.json { head :no_content }
     end
   end
 

app/graphql/mutations/ci/pipeline/retry.rb

@@ -17,10 +17,11 @@ module Mutations
           pipeline = authorized_find!(id: id)
           project = pipeline.project
 
-          ::Ci::RetryPipelineService.new(project, current_user).execute(pipeline)
+          service_response = ::Ci::RetryPipelineService.new(project, current_user).execute(pipeline)
+
           {
             pipeline: pipeline,
-            errors: errors_on_object(pipeline)
+            errors: errors_on_object(pipeline) + service_response.errors
           }
         end
       end

app/services/ci/retry_build_service.rb

@@ -65,7 +65,7 @@ module Ci
 
     def check_access!(build)
       unless can?(current_user, :update_build, build)
-        raise Gitlab::Access::AccessDeniedError
+        raise Gitlab::Access::AccessDeniedError, '403 Forbidden'
       end
     end
 

app/services/ci/retry_pipeline_service.rb

@@ -5,9 +5,8 @@ module Ci
     include Gitlab::OptimisticLocking
 
     def execute(pipeline)
-      unless can?(current_user, :update_pipeline, pipeline)
-        raise Gitlab::Access::AccessDeniedError
-      end
+      access_response = check_access(pipeline)
+      return access_response if access_response.error?
 
       pipeline.ensure_scheduling_type!
 
@@ -30,6 +29,18 @@ module Ci
       Ci::ProcessPipelineService
         .new(pipeline)
         .execute
+
+      ServiceResponse.success
+    rescue Gitlab::Access::AccessDeniedError => e
+      ServiceResponse.error(message: e.message, http_status: :forbidden)
+    end
+
+    def check_access(pipeline)
+      if can?(current_user, :update_pipeline, pipeline)
+        ServiceResponse.success
+      else
+        ServiceResponse.error(message: '403 Forbidden', http_status: :forbidden)
+      end
     end
 
     private

ee/app/services/ee/ci/retry_pipeline_service.rb

@@ -6,6 +6,15 @@ module EE
       extend ::Gitlab::Utils::Override
       include ::Gitlab::Utils::StrongMemoize
 
+      override :check_access
+      def check_access(pipeline)
+        if current_user && !current_user.has_required_credit_card_to_run_pipelines?(project)
+          ServiceResponse.error(message: 'Credit card required to be on file in order to retry a pipeline', http_status: :forbidden)
+        else
+          super
+        end
+      end
+
       private
 
       override :builds_relation

ee/spec/services/ci/retry_pipeline_service_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 RSpec.describe Ci::RetryPipelineService do
   let_it_be(:runner) { create(:ci_runner, :instance, :online) }
   let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project) }
 
   let(:pipeline) { create(:ci_pipeline, project: project) }
   let(:service) { described_class.new(project, user) }
@@ -38,6 +39,21 @@ RSpec.describe Ci::RetryPipelineService do
     end
   end
 
+  context 'when user is not allowed to retry pipeline because of missing credit card' do
+    it 'returns an error' do
+      allow(user)
+        .to receive(:has_required_credit_card_to_run_pipelines?)
+        .with(project)
+        .and_return(false)
+
+      response = service.execute(pipeline)
+
+      expect(response.http_status).to eq(:forbidden)
+      expect(response.errors).to include('Credit card required to be on file in order to retry a pipeline')
+      expect(pipeline.reload).not_to be_running
+    end
+  end
+
   def build(name)
     pipeline.reload.statuses.latest.find_by(name: name)
   end

lib/api/ci/pipelines.rb

@@ -223,9 +223,13 @@ module API
         post ':id/pipelines/:pipeline_id/retry', feature_category: :continuous_integration do
           authorize! :update_pipeline, pipeline
 
-          pipeline.retry_failed(current_user)
+          response = pipeline.retry_failed(current_user)
 
-          present pipeline, with: Entities::Ci::Pipeline
+          if response.success?
+            present pipeline, with: Entities::Ci::Pipeline
+          else
+            render_api_error!(response.errors.join(', '), response.http_status)
+          end
         end
 
         desc 'Cancel all builds in the pipeline' do

spec/controllers/projects/pipelines_controller_spec.rb

@@ -932,6 +932,33 @@ RSpec.describe Projects::PipelinesController do
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
+
+    context 'when access denied' do
+      it 'returns an error' do
+        sign_in(create(:user))
+
+        post_retry
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'when service returns an error' do
+      before do
+        service_response = ServiceResponse.error(message: 'some error', http_status: 404)
+        allow_next_instance_of(::Ci::RetryPipelineService) do |service|
+          allow(service).to receive(:check_access).and_return(service_response)
+        end
+      end
+
+      it 'does not retry' do
+        post_retry
+
+        expect(response).to have_gitlab_http_status(:not_found)
+        expect(response.body).to include('some error')
+        expect(::Ci::RetryPipelineWorker).not_to have_received(:perform_async).with(pipeline.id, user.id)
+      end
+    end
   end
 
   describe 'POST cancel.json' do

spec/frontend/pipelines/header_component_spec.js

 import { GlModal, GlLoadingIcon } from '@gitlab/ui';
 import { shallowMount } from '@vue/test-utils';
+import { nextTick } from 'vue';
+import waitForPromises from 'helpers/wait_for_promises';
 import HeaderComponent from '~/pipelines/components/header_component.vue';
 import cancelPipelineMutation from '~/pipelines/graphql/mutations/cancel_pipeline.mutation.graphql';
 import deletePipelineMutation from '~/pipelines/graphql/mutations/delete_pipeline.mutation.graphql';
@@ -17,6 +19,7 @@ import {
 describe('Pipeline details header', () => {
   let wrapper;
   let glModalDirective;
+  let mutate = jest.fn();
 
   const findDeleteModal = () => wrapper.find(GlModal);
   const findRetryButton = () => wrapper.find('[data-testid="retryPipeline"]');
@@ -44,7 +47,7 @@ describe('Pipeline details header', () => {
           startPolling: jest.fn(),
         },
       },
-      mutate: jest.fn(),
+      mutate,
     };
 
     return shallowMount(HeaderComponent, {
@@ -120,6 +123,26 @@ describe('Pipeline details header', () => {
       });
     });
 
+    describe('Retry action failed', () => {
+      beforeEach(() => {
+        mutate = jest.fn().mockRejectedValue('error');
+
+        wrapper = createComponent(mockCancelledPipelineHeader);
+      });
+
+      it('retry button loading state should reset on error', async () => {
+        findRetryButton().vm.$emit('click');
+
+        await nextTick();
+
+        expect(findRetryButton().props('loading')).toBe(true);
+
+        await waitForPromises();
+
+        expect(findRetryButton().props('loading')).toBe(false);
+      });
+    });
+
     describe('Cancel action', () => {
       beforeEach(() => {
         wrapper = createComponent(mockRunningPipelineHeader);

spec/requests/api/ci/pipelines_spec.rb

@@ -1075,6 +1075,23 @@ RSpec.describe API::Ci::Pipelines do
         expect(json_response['id']).to be nil
       end
     end
+
+    context 'handles errors' do
+      before do
+        service_response = ServiceResponse.error(http_status: 403, message: 'hello world')
+        allow_next_instance_of(::Ci::RetryPipelineService) do |service|
+          allow(service).to receive(:check_access).and_return(service_response)
+        end
+      end
+
+      it 'returns error' do
+        post api("/projects/#{project.id}/pipelines/#{pipeline.id}/retry", user)
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+        expect(json_response['message']).to eq 'hello world'
+        expect(json_response['id']).to be nil
+      end
+    end
   end
 
   describe 'POST /projects/:id/pipelines/:pipeline_id/cancel' do

spec/services/ci/retry_pipeline_service_spec.rb

@@ -137,7 +137,7 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do
       end
     end
 
-    context 'when the last stage was skipepd' do
+    context 'when the last stage was skipped' do
       before do
         create_build('build 1', :success, 0)
         create_build('test 2', :failed, 1)
@@ -336,12 +336,32 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do
         expect(pipeline.reload).to be_running
       end
     end
+
+    context 'when user is not allowed to retry build' do
+      before do
+        build = create(:ci_build, pipeline: pipeline, status: :failed)
+        allow_next_instance_of(Ci::RetryBuildService) do |service|
+          allow(service).to receive(:can?).with(user, :update_build, build).and_return(false)
+        end
+      end
+
+      it 'returns an error' do
+        response = service.execute(pipeline)
+
+        expect(response.http_status).to eq(:forbidden)
+        expect(response.errors).to include('403 Forbidden')
+        expect(pipeline.reload).not_to be_running
+      end
+    end
   end
 
   context 'when user is not allowed to retry pipeline' do
-    it 'raises an error' do
-      expect { service.execute(pipeline) }
-        .to raise_error Gitlab::Access::AccessDeniedError
+    it 'returns an error' do
+      response = service.execute(pipeline)
+
+      expect(response.http_status).to eq(:forbidden)
+      expect(response.errors).to include('403 Forbidden')
+      expect(pipeline.reload).not_to be_running
     end
   end
 
@@ -359,9 +379,12 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do
         create_build('verify', :canceled, 1)
       end
 
-      it 'raises an error' do
-        expect { service.execute(pipeline) }
-          .to raise_error Gitlab::Access::AccessDeniedError
+      it 'returns an error' do
+        response = service.execute(pipeline)
+
+        expect(response.http_status).to eq(:forbidden)
+        expect(response.errors).to include('403 Forbidden')
+        expect(pipeline.reload).not_to be_running
       end
     end
 
@@ -372,9 +395,12 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do
         create_build('verify', :canceled, 2)
       end
 
-      it 'raises an error' do
-        expect { service.execute(pipeline) }
-          .to raise_error Gitlab::Access::AccessDeniedError
+      it 'returns an error' do
+        response = service.execute(pipeline)
+
+        expect(response.http_status).to eq(:forbidden)
+        expect(response.errors).to include('403 Forbidden')
+        expect(pipeline.reload).not_to be_running
       end
     end
   end