Merge branch 'sy-abbreviate-escalation-rule-limit' into 'master'

Shorten error messages for escalation rule count and fix edge case

See merge request gitlab-org/gitlab!65623

ee/app/models/incident_management/escalation_rule.rb

@@ -4,8 +4,6 @@ module IncidentManagement
   class EscalationRule < ApplicationRecord
     self.table_name = 'incident_management_escalation_rules'
 
-    MAX_RULE_PER_POLICY_COUNT = 10
-
     belongs_to :policy, class_name: 'EscalationPolicy', inverse_of: 'rules', foreign_key: 'policy_id'
     belongs_to :oncall_schedule, class_name: 'OncallSchedule', inverse_of: 'rotations', foreign_key: 'oncall_schedule_id'
 
@@ -18,16 +16,5 @@ module IncidentManagement
               numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than_or_equal_to: 24.hours }
 
     validates :policy_id, uniqueness: { scope: [:oncall_schedule_id, :status, :elapsed_time_seconds], message: _('must have a unique schedule, status, and elapsed time') }
-
-    validate :rules_count_not_exceeded, on: :create, if: :policy
-
-    private
-
-    def rules_count_not_exceeded
-      # We need to add to the count if we aren't creating the rules at the same time as the policy.
-      rules_count = policy.new_record? ? policy.rules.size : policy.rules.size + 1
-
-      errors.add(:base, "cannot have more than #{MAX_RULE_PER_POLICY_COUNT} rules") if rules_count > MAX_RULE_PER_POLICY_COUNT
-    end
   end
 end

ee/app/services/incident_management/escalation_policies/base_service.rb

@@ -3,10 +3,16 @@
 module IncidentManagement
   module EscalationPolicies
     class BaseService
+      MAX_RULE_COUNT = 10
+
       def allowed?
         user&.can?(:admin_incident_management_escalation_policy, project)
       end
 
+      def too_many_rules?
+        params[:rules_attributes] && params[:rules_attributes].size > MAX_RULE_COUNT
+      end
+
       def invalid_schedules?
         params[:rules_attributes]&.any? { |attrs| attrs[:oncall_schedule]&.project != project }
       end
@@ -27,6 +33,10 @@ module IncidentManagement
         error(_('Escalation policies must have at least one rule'))
       end
 
+      def error_too_many_rules
+        error(_('Escalation policies may not have more than %{rule_count} rules') % { rule_count: MAX_RULE_COUNT })
+      end
+
       def error_bad_schedules
         error(_('All escalations rules must have a schedule in the same project as the policy'))
       end

ee/app/services/incident_management/escalation_policies/create_service.rb

@@ -21,6 +21,7 @@ module IncidentManagement
       def execute
         return error_no_permissions unless allowed?
         return error_no_rules if params[:rules_attributes].blank?
+        return error_too_many_rules if too_many_rules?
         return error_bad_schedules if invalid_schedules?
 
         escalation_policy = project.incident_management_escalation_policies.create(params)

ee/app/services/incident_management/escalation_policies/update_service.rb

@@ -26,6 +26,7 @@ module IncidentManagement
       def execute
         return error_no_permissions unless allowed?
         return error_no_rules if empty_rules?
+        return error_too_many_rules if too_many_rules?
         return error_bad_schedules if invalid_schedules?
 
         reconcile_rules!

ee/spec/models/incident_management/escalation_rule_spec.rb

@@ -20,13 +20,5 @@ RSpec.describe IncidentManagement::EscalationRule do
     it { is_expected.to validate_presence_of(:elapsed_time_seconds) }
     it { is_expected.to validate_numericality_of(:elapsed_time_seconds).is_greater_than_or_equal_to(0).is_less_than_or_equal_to(24.hours) }
     it { is_expected.to validate_uniqueness_of(:policy_id).scoped_to([:oncall_schedule_id, :status, :elapsed_time_seconds] ).with_message('must have a unique schedule, status, and elapsed time') }
-
-    it 'validates the number of rules' do
-      policy = create(:incident_management_escalation_policy, rule_count: 10)
-      rule = build(:incident_management_escalation_rule, policy: policy)
-
-      expect(rule).not_to be_valid
-      expect(rule.errors).to contain_exactly("cannot have more than #{described_class::MAX_RULE_PER_POLICY_COUNT} rules")
-    end
   end
 end

ee/spec/services/incident_management/escalation_policies/create_service_spec.rb

@@ -71,6 +71,20 @@ RSpec.describe IncidentManagement::EscalationPolicies::CreateService do
         it_behaves_like 'error response', 'Escalation policies must have at least one rule'
       end
 
+      context 'too many rules are given' do
+        let(:rule_params) do
+          (0..10).map do |idx|
+            {
+              oncall_schedule: oncall_schedule,
+              elapsed_time_seconds: idx,
+              status: :acknowledged
+            }
+          end
+        end
+
+        it_behaves_like 'error response', 'Escalation policies may not have more than 10 rules'
+      end
+
       context 'oncall schedule is blank' do
         before do
           rule_params[0][:oncall_schedule] = nil

ee/spec/services/incident_management/escalation_policies/update_service_spec.rb

@@ -148,6 +148,21 @@ RSpec.describe IncidentManagement::EscalationPolicies::UpdateService do
       it_behaves_like 'error response', 'Escalation policies must have at least one rule'
     end
 
+    context 'when too many rules are given' do
+      let(:rule_params) { [*existing_rules_params, *new_rule_params] }
+      let(:new_rule_params) do
+        (0..9).map do |idx|
+          {
+            oncall_schedule: oncall_schedule,
+            elapsed_time_seconds: idx,
+            status: :acknowledged
+          }
+        end
+      end
+
+      it_behaves_like 'error response', 'Escalation policies may not have more than 10 rules'
+    end
+
     context 'when the on-call schedule is not present on the rule' do
       let(:rule_params) { [new_rule_params.except(:oncall_schedule)] }
 

locale/gitlab.pot

@@ -12990,6 +12990,9 @@ msgstr ""
 msgid "Escalation policies"
 msgstr ""
 
+msgid "Escalation policies may not have more than %{rule_count} rules"
+msgstr ""
+
 msgid "Escalation policies must have at least one rule"
 msgstr ""