Merge branch 'sy-reduce-policy-setting-complexity' into 'master'

Clean up IssuableEscalationStatuses::PrepareUpdateService

See merge request gitlab-org/gitlab!78507

app/services/incident_management/issuable_escalation_statuses/prepare_update_service.rb

@@ -2,18 +2,16 @@
 
 module IncidentManagement
   module IssuableEscalationStatuses
-    class PrepareUpdateService
+    class PrepareUpdateService < ::BaseProjectService
       include Gitlab::Utils::StrongMemoize
 
       SUPPORTED_PARAMS = %i[status status_change_reason].freeze
 
-      InvalidParamError = Class.new(StandardError)
-
       def initialize(issuable, current_user, params)
         @issuable = issuable
-        @current_user = current_user
-        @params = params.dup || {}
-        @project = issuable.project
+        @param_errors = []
+
+        super(project: issuable.project, current_user: current_user, params: Hash(params))
       end
 
       def execute
@@ -23,14 +21,14 @@ module IncidentManagement
         filter_attributes
         filter_redundant_params
 
+        return invalid_param_error if param_errors.any?
+
         ServiceResponse.success(payload: { escalation_status: params })
-      rescue InvalidParamError
-        invalid_param_error
       end
 
       private
 
-      attr_reader :issuable, :current_user, :params, :project
+      attr_reader :issuable, :param_errors
 
       def available?
         issuable.supports_escalation? &&
@@ -65,7 +63,7 @@ module IncidentManagement
         return unless status
 
         status_event = escalation_status.status_event_for(status)
-        raise InvalidParamError unless status_event
+        add_param_error(:status) && return unless status_event
 
         params[:status_event] = status_event
       end
@@ -84,12 +82,16 @@ module IncidentManagement
         end
       end
 
+      def add_param_error(param)
+        param_errors << param
+      end
+
       def availability_error
         ServiceResponse.error(message: 'Escalation status updates are not available for this issue, user, or project.')
       end
 
       def invalid_param_error
-        ServiceResponse.error(message: 'Invalid value was provided for a parameter.')
+        ServiceResponse.error(message: "Invalid value was provided for parameters: #{param_errors.join(', ')}")
       end
     end
   end

ee/app/services/ee/incident_management/issuable_escalation_statuses/prepare_update_service.rb

@@ -21,23 +21,35 @@ module EE
         end
 
         def filter_policy
+          return unless params.include?(:policy)
+
           policy = params.delete(:policy)
+          return unless policies_permitted?
+
+          policy ? set_policy(policy) : unset_policy
+        end
 
-          return unless ::Gitlab::IncidentManagement.escalation_policies_available?(project)
-          return if issuable.alert_management_alert # Cannot change the policy for an alert
+        def policies_permitted?
+          ::Gitlab::IncidentManagement.escalation_policies_available?(project) &&
+            issuable.alert_management_alert.nil? # Cannot change the policy for an alert
+        end
 
-          if policy
-            return if policy.id == escalation_status.policy_id
-            if policy.project_id != issuable.project_id
-              raise ::IncidentManagement::IssuableEscalationStatuses::PrepareUpdateService::InvalidParamError
-            end
+        def set_policy(policy)
+          return if policy.id == escalation_status.policy_id
 
-            # Override any provided status if setting new policy
-            params[:status_event] = :trigger
+          unless policy.project_id == issuable.project_id
+            add_param_error(:policy)
+            return
           end
 
           params[:policy] = policy
-          params[:escalations_started_at] = policy ? Time.current : nil
+          params[:escalations_started_at] = Time.current
+          params[:status_event] = :trigger # Override any provided status if setting new policy
+        end
+
+        def unset_policy
+          params[:policy] = nil
+          params[:escalations_started_at] = nil
         end
 
         override :current_params

ee/spec/services/ee/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb

@@ -67,7 +67,7 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::PrepareUpdateServ
 
     it 'returns an error response' do
       expect(result).to be_error
-      expect(result.message).to eq('Invalid value was provided for a parameter.')
+      expect(result.message).to eq('Invalid value was provided for parameters: policy')
     end
   end
 
@@ -80,6 +80,12 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::PrepareUpdateServ
       it_behaves_like 'successful response without policy params'
     end
 
+    context 'when policy is excluded' do
+      let(:params) { { status: status } }
+
+      it_behaves_like 'successful response without policy params'
+    end
+
     context 'when policy is nil' do
       let(:params) { { status: status, policy: nil } }
 

spec/services/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb

@@ -37,7 +37,7 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::PrepareUpdateServ
   end
 
   shared_examples 'invalid params error response' do
-    include_examples 'error response', 'Invalid value was provided for a parameter.'
+    include_examples 'error response', 'Invalid value was provided for parameters: status'
   end
 
   it_behaves_like 'successful response', { status_event: :acknowledge }