Merge branch 'sy-escalation-rules-sorted' into 'master'

Order escalation rules by elapsed time and status

See merge request gitlab-org/gitlab!65097

ee/app/graphql/resolvers/incident_management/escalation_policies_resolver.rb

@@ -25,7 +25,7 @@ module Resolvers
 
       def preloads
         {
-          rules: [{ rules: :oncall_schedule }]
+          rules: [:ordered_rules]
         }
       end
     end

ee/app/graphql/types/incident_management/escalation_policy_type.rb

@@ -22,7 +22,8 @@ module Types
 
       field :rules, [Types::IncidentManagement::EscalationRuleType],
             null: true,
-            description: 'Steps of the escalation policy.'
+            description: 'Steps of the escalation policy.',
+            method: :ordered_rules
     end
   end
 end

ee/app/graphql/types/incident_management/escalation_rule_type.rb

@@ -22,6 +22,10 @@ module Types
       field :status, Types::IncidentManagement::EscalationRuleStatusEnum,
             null: true,
             description: 'The status required to prevent the rule from activating.'
+
+      def oncall_schedule
+        Gitlab::Graphql::Loaders::BatchModelLoader.new(::IncidentManagement::OncallSchedule, object.oncall_schedule_id).find
+      end
     end
     # rubocop: enable Graphql/AuthorizeTypes
   end

ee/app/models/incident_management/escalation_policy.rb

@@ -6,6 +6,7 @@ module IncidentManagement
 
     belongs_to :project
     has_many :rules, class_name: 'EscalationRule', inverse_of: :policy, foreign_key: 'policy_id', index_errors: true
+    has_many :ordered_rules, -> { order(:elapsed_time_seconds, :status) }, class_name: 'EscalationRule', inverse_of: :policy, foreign_key: 'policy_id'
 
     validates :project_id, uniqueness: { message: _('can only have one escalation policy') }, on: :create
     validates :name, presence: true, uniqueness: { scope: [:project_id] }, length: { maximum: 72 }

ee/spec/factories/incident_management/escalation_rules.rb

@@ -6,5 +6,9 @@ FactoryBot.define do
     oncall_schedule { association :incident_management_oncall_schedule, project: policy.project }
     status { IncidentManagement::EscalationRule.statuses[:acknowledged] }
     elapsed_time_seconds { 5.minutes }
+
+    trait :resolved do
+      status { IncidentManagement::EscalationRule.statuses[:resolved] }
+    end
   end
 end

ee/spec/models/incident_management/escalation_policy_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe IncidentManagement::EscalationPolicy do
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to have_many(:rules) }
+    it { is_expected.to have_many(:ordered_rules).order(elapsed_time_seconds: :asc, status: :asc) }
   end
 
   describe 'validations' do

ee/spec/requests/api/graphql/mutations/incident_management/escalation_policy/create_spec.rb

@@ -56,6 +56,10 @@ RSpec.describe 'creating escalation policy' do
     expect(first_rule['status']).to eq(create_params.dig(:rules, 0, :status))
   end
 
+  include_examples 'correctly reorders escalation rule inputs' do
+    let(:variables) { params }
+  end
+
   context 'errors' do
     context 'user does not have permission' do
       subject(:resolve) { post_graphql_mutation(mutation, current_user: create(:user)) }

ee/spec/requests/api/graphql/mutations/incident_management/escalation_policy/update_spec.rb

@@ -84,4 +84,8 @@ RSpec.describe 'Updating an escalation policy' do
       ]
     )
   end
+
+  include_examples 'correctly reorders escalation rule inputs' do
+    let(:resolve) { post_graphql_mutation(mutation, current_user: user) }
+  end
 end

ee/spec/requests/api/graphql/project/incident_management/escalation_policy/rules_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'getting Incident Management escalation policies' do
+  include GraphqlHelpers
+
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:policy) { create(:incident_management_escalation_policy, project: project) }
+  let_it_be(:rule) { policy.rules.first }
+  let_it_be(:schedule) { rule.oncall_schedule }
+
+  let(:params) { {} }
+
+  let(:fields) do
+    <<~QUERY
+      nodes {
+        id
+        rules {
+          id
+          elapsedTimeSeconds
+          status
+          oncallSchedule {
+            iid
+            name
+          }
+        }
+      }
+    QUERY
+  end
+
+  let(:query) do
+    graphql_query_for(
+      'project',
+      { 'fullPath' => project.full_path },
+      query_graphql_field('incidentManagementEscalationPolicies', {}, fields)
+    )
+  end
+
+  let(:escalation_policy_response) { graphql_data.dig('project', 'incidentManagementEscalationPolicies', 'nodes').first }
+  let(:escalation_rules_response) { escalation_policy_response['rules'] }
+
+  before do
+    stub_licensed_features(oncall_schedules: true, escalation_policies: true)
+    project.add_reporter(current_user)
+  end
+
+  it 'includes expected data' do
+    post_graphql(query, current_user: current_user)
+
+    expect(escalation_rules_response).to eq([{
+      'id' => global_id(rule),
+      'elapsedTimeSeconds' => rule.elapsed_time_seconds, # 5 min
+      'status' => rule.status.upcase, # 'ACKNOWLEDGED'
+      'oncallSchedule' => {
+        'iid' => schedule.iid.to_s,
+        'name' => schedule.name
+      }
+    }])
+  end
+
+  context 'with multiple rules' do
+    let_it_be(:later_acknowledged_rule) { create(:incident_management_escalation_rule, policy: policy, elapsed_time_seconds: 10.minutes) }
+    let_it_be(:earlier_resolved_rule) { create(:incident_management_escalation_rule, :resolved, policy: policy, elapsed_time_seconds: 1.minute) }
+    let_it_be(:equivalent_resolved_rule) { create(:incident_management_escalation_rule, :resolved, policy: policy) }
+
+    it 'orders rules by time and status' do
+      post_graphql(query, current_user: current_user)
+
+      expect(escalation_rules_response.length).to eq(4)
+      expect(escalation_rules_response.map { |rule| rule['id'] }).to eq([
+        global_id(earlier_resolved_rule),
+        global_id(rule),
+        global_id(equivalent_resolved_rule),
+        global_id(later_acknowledged_rule)
+      ])
+    end
+  end
+
+  it 'avoids N+1 queries' do
+    post_graphql(query, current_user: current_user)
+
+    base_count = ActiveRecord::QueryRecorder.new do
+      post_graphql(query, current_user: current_user)
+    end
+
+    create(:incident_management_escalation_rule, policy: policy, elapsed_time_seconds: 1.hour)
+
+    expect { post_graphql(query, current_user: current_user) }.not_to exceed_query_limit(base_count)
+  end
+
+  private
+
+  def global_id(object)
+    object.to_global_id.to_s
+  end
+end

ee/spec/support/shared_examples/requests/api/graphql/incident_management/escalation_policies_shared_examples.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# Expected variables:
+# schedule - IncidentManagement::OncallSchedule
+# resolve - method which posts a mutation
+# variables - attributes provided to the mutation
+RSpec.shared_examples 'correctly reorders escalation rule inputs' do
+  context 'when rules are provided out of order' do
+    before do
+      variables[:rules] = [
+        {
+          oncallScheduleIid: schedule.iid,
+          elapsedTimeSeconds: 60,
+          status: 'RESOLVED'
+        },
+        {
+          oncallScheduleIid: schedule.iid,
+          elapsedTimeSeconds: 60,
+          status: 'ACKNOWLEDGED'
+        },
+        {
+          oncallScheduleIid: schedule.iid,
+          elapsedTimeSeconds: 0,
+          status: 'ACKNOWLEDGED'
+        }
+      ]
+    end
+
+    it 'successfully creates the policy and reorders the rules' do
+      resolve
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(mutation_response['errors']).to be_empty
+      expect(pluck_from_rules_response('status')).to eq(%w(ACKNOWLEDGED ACKNOWLEDGED RESOLVED))
+      expect(pluck_from_rules_response('elapsedTimeSeconds')).to eq([0, 60, 60])
+    end
+
+    private
+
+    def pluck_from_rules_response(attribute)
+      mutation_response['escalationPolicy']['rules'].map { |rule| rule[attribute] }
+    end
+  end
+end