Merge branch 'refactor-clone-dashboard-service' into 'master'

Refactor CloneDashboardService to remove catch/throw

See merge request gitlab-org/gitlab!27617

app/services/metrics/dashboard/clone_dashboard_service.rb

@@ -5,9 +5,18 @@
 module Metrics
   module Dashboard
     class CloneDashboardService < ::BaseService
+      include Stepable
+
       ALLOWED_FILE_TYPE = '.yml'
       USER_DASHBOARDS_DIR = ::Metrics::Dashboard::CustomDashboardService::DASHBOARD_ROOT
 
+      steps :check_push_authorized,
+        :check_branch_name,
+        :check_file_type,
+        :check_dashboard_template,
+        :create_file,
+        :refresh_repository_method_caches
+
       class << self
         def allowed_dashboard_templates
           @allowed_dashboard_templates ||= Set[::Metrics::Dashboard::SystemDashboardService::DASHBOARD_PATH].freeze
@@ -22,21 +31,52 @@ module Metrics
         end
       end
 
-      # rubocop:disable Cop/BanCatchThrow
       def execute
-        catch(:error) do
-          throw(:error, error(_(%q(You are not allowed to push into this branch. Create another branch or open a merge request.)), :forbidden)) unless push_authorized?
+        execute_steps
+      end
 
-          result = ::Files::CreateService.new(project, current_user, dashboard_attrs).execute
-          throw(:error, wrap_error(result)) unless result[:status] == :success
+      private
 
-          repository.refresh_method_caches([:metrics_dashboard])
-          success(result.merge(http_status: :created, dashboard: dashboard_details))
+      def check_push_authorized(result)
+        return error(_('You are not allowed to push into this branch. Create another branch or open a merge request.'), :forbidden) unless push_authorized?
+
+        success(result)
       end
+
+      def check_branch_name(result)
+        return error(_('There was an error creating the dashboard, branch name is invalid.'), :bad_request) unless valid_branch_name?
+        return error(_('There was an error creating the dashboard, branch named: %{branch} already exists.') % { branch: params[:branch] }, :bad_request) unless new_or_default_branch?
+
+        success(result)
       end
-      # rubocop:enable Cop/BanCatchThrow
 
-      private
+      def check_file_type(result)
+        return error(_('The file name should have a .yml extension'), :bad_request) unless target_file_type_valid?
+
+        success(result)
+      end
+
+      def check_dashboard_template(result)
+        return error(_('Not found.'), :not_found) unless self.class.allowed_dashboard_templates.include?(params[:dashboard])
+
+        success(result)
+      end
+
+      def create_file(result)
+        create_file_response = ::Files::CreateService.new(project, current_user, dashboard_attrs).execute
+
+        if create_file_response[:status] == :success
+          success(result.merge(create_file_response))
+        else
+          wrap_error(create_file_response)
+        end
+      end
+
+      def refresh_repository_method_caches(result)
+        repository.refresh_method_caches([:metrics_dashboard])
+
+        success(result.merge(http_status: :created, dashboard: dashboard_details))
+      end
 
       def dashboard_attrs
         {
@@ -62,26 +102,13 @@ module Metrics
         Gitlab::UserAccess.new(current_user, project: project).can_push_to_branch?(branch)
       end
 
-      # rubocop:disable Cop/BanCatchThrow
       def dashboard_template
-        @dashboard_template ||= begin
-          throw(:error, error(_('Not found.'), :not_found)) unless self.class.allowed_dashboard_templates.include?(params[:dashboard])
-
-          params[:dashboard]
-        end
+        @dashboard_template ||= params[:dashboard]
       end
-      # rubocop:enable Cop/BanCatchThrow
 
-      # rubocop:disable Cop/BanCatchThrow
       def branch
-        @branch ||= begin
-          throw(:error, error(_('There was an error creating the dashboard, branch name is invalid.'), :bad_request)) unless valid_branch_name?
-          throw(:error, error(_('There was an error creating the dashboard, branch named: %{branch} already exists.') % { branch: params[:branch] }, :bad_request)) unless new_or_default_branch? # temporary validation for first UI iteration
-
-          params[:branch]
-        end
+        @branch ||= params[:branch]
       end
-      # rubocop:enable Cop/BanCatchThrow
 
       def new_or_default_branch?
         !repository.branch_exists?(params[:branch]) || project.default_branch == params[:branch]
@@ -95,20 +122,22 @@ module Metrics
         @new_dashboard_path ||= File.join(USER_DASHBOARDS_DIR, file_name)
       end
 
-      # rubocop:disable Cop/BanCatchThrow
       def file_name
-        @file_name ||= begin
-          throw(:error, error(_('The file name should have a .yml extension'), :bad_request)) unless target_file_type_valid?
-
-          File.basename(params[:file_name])
+        @file_name ||= File.basename(params[:file_name])
       end
-      end
-      # rubocop:enable Cop/BanCatchThrow
 
       def target_file_type_valid?
         File.extname(params[:file_name]) == ALLOWED_FILE_TYPE
       end
 
+      def wrap_error(result)
+        if result[:message] == 'A file with this name already exists'
+          error(_("A file with '%{file_name}' already exists in %{branch} branch") % { file_name: file_name, branch: branch }, :bad_request)
+        else
+          result
+        end
+      end
+
       def new_dashboard_content
         ::Gitlab::Metrics::Dashboard::Processor
           .new(project, raw_dashboard, sequence, {})
@@ -119,14 +148,6 @@ module Metrics
         @repository ||= project.repository
       end
 
-      def wrap_error(result)
-        if result[:message] == 'A file with this name already exists'
-          error(_("A file with '%{file_name}' already exists in %{branch} branch") % { file_name: file_name, branch: branch }, :bad_request)
-        else
-          result
-        end
-      end
-
       def raw_dashboard
         YAML.safe_load(File.read(Rails.root.join(dashboard_template)))
       end

spec/services/metrics/dashboard/clone_dashboard_service_spec.rb

@@ -29,7 +29,7 @@ describe Metrics::Dashboard::CloneDashboardService, :use_clean_rails_memory_stor
     end
 
     context 'user does not have push right to repository' do
-      it_behaves_like 'misconfigured dashboard service response', :forbidden, %q(You are not allowed to push into this branch. Create another branch or open a merge request.)
+      it_behaves_like 'misconfigured dashboard service response with stepable', :forbidden, 'You are not allowed to push into this branch. Create another branch or open a merge request.'
     end
 
     context 'with rights to push to the repository' do
@@ -40,19 +40,19 @@ describe Metrics::Dashboard::CloneDashboardService, :use_clean_rails_memory_stor
       context 'wrong target file extension' do
         let(:file_name) { 'custom_dashboard.txt' }
 
-        it_behaves_like 'misconfigured dashboard service response', :bad_request, 'The file name should have a .yml extension'
+        it_behaves_like 'misconfigured dashboard service response with stepable', :bad_request, 'The file name should have a .yml extension'
       end
 
       context 'wrong source dashboard file' do
         let(:dashboard) { 'config/prometheus/common_metrics_123.yml' }
 
-        it_behaves_like 'misconfigured dashboard service response', :not_found, 'Not found.'
+        it_behaves_like 'misconfigured dashboard service response with stepable', :not_found, 'Not found.'
       end
 
       context 'path traversal attack attempt' do
         let(:dashboard) { 'config/prometheus/../database.yml' }
 
-        it_behaves_like 'misconfigured dashboard service response', :not_found, 'Not found.'
+        it_behaves_like 'misconfigured dashboard service response with stepable', :not_found, 'Not found.'
       end
 
       context 'path traversal attack attempt on target file' do
@@ -92,7 +92,7 @@ describe Metrics::Dashboard::CloneDashboardService, :use_clean_rails_memory_stor
             project.repository.add_branch(user, branch, 'master')
           end
 
-          it_behaves_like 'misconfigured dashboard service response', :bad_request, "There was an error creating the dashboard, branch named: existing_branch already exists."
+          it_behaves_like 'misconfigured dashboard service response with stepable', :bad_request, 'There was an error creating the dashboard, branch named: existing_branch already exists.'
 
           # temporary not available function for first iteration
           # follow up issue https://gitlab.com/gitlab-org/gitlab/issues/196237 which
@@ -111,7 +111,7 @@ describe Metrics::Dashboard::CloneDashboardService, :use_clean_rails_memory_stor
         context 'blank branch name' do
           let(:branch) { '' }
 
-          it_behaves_like 'misconfigured dashboard service response', :bad_request, 'There was an error creating the dashboard, branch name is invalid.'
+          it_behaves_like 'misconfigured dashboard service response with stepable', :bad_request, 'There was an error creating the dashboard, branch name is invalid.'
         end
 
         context 'dashboard file already exists' do
@@ -129,7 +129,7 @@ describe Metrics::Dashboard::CloneDashboardService, :use_clean_rails_memory_stor
             ).execute
           end
 
-          it_behaves_like 'misconfigured dashboard service response', :bad_request, "A file with 'custom_dashboard.yml' already exists in custom_dashboard branch"
+          it_behaves_like 'misconfigured dashboard service response with stepable', :bad_request, "A file with 'custom_dashboard.yml' already exists in custom_dashboard branch"
         end
 
         it 'extends dashboard template path to absolute url' do

spec/support/shared_examples/services/metrics/dashboard_shared_examples.rb

@@ -107,3 +107,14 @@ RSpec.shared_examples 'valid dashboard update process' do
     service_call
   end
 end
+
+RSpec.shared_examples 'misconfigured dashboard service response with stepable' do |status_code, message = nil|
+  it 'returns an appropriate message and status code', :aggregate_failures do
+    result = service_call
+
+    expect(result.keys).to contain_exactly(:message, :http_status, :status, :last_step)
+    expect(result[:status]).to eq(:error)
+    expect(result[:http_status]).to eq(status_code)
+    expect(result[:message]).to eq(message) if message
+  end
+end