Merge branch 'sy-webhooks-for-incident-escalations' into 'master'

Add escalation-related attributes to issue webhook

See merge request gitlab-org/gitlab!80437

app/models/concerns/issuable.rb

@@ -464,37 +464,54 @@ module Issuable
     false
   end
 
-  def to_hook_data(user, old_associations: {})
-    changes = previous_changes
+  def hook_association_changes(old_associations)
+    changes = {}
 
-    if old_associations
-      old_labels = old_associations.fetch(:labels, labels)
-      old_assignees = old_associations.fetch(:assignees, assignees)
-      old_severity = old_associations.fetch(:severity, severity)
+    old_labels = old_associations.fetch(:labels, labels)
+    old_assignees = old_associations.fetch(:assignees, assignees)
+    old_severity = old_associations.fetch(:severity, severity)
 
-      if old_labels != labels
-        changes[:labels] = [old_labels.map(&:hook_attrs), labels.map(&:hook_attrs)]
-      end
+    if old_labels != labels
+      changes[:labels] = [old_labels.map(&:hook_attrs), labels.map(&:hook_attrs)]
+    end
 
-      if old_assignees != assignees
-        changes[:assignees] = [old_assignees.map(&:hook_attrs), assignees.map(&:hook_attrs)]
-      end
+    if old_assignees != assignees
+      changes[:assignees] = [old_assignees.map(&:hook_attrs), assignees.map(&:hook_attrs)]
+    end
+
+    if supports_severity? && old_severity != severity
+      changes[:severity] = [old_severity, severity]
+    end
+
+    if supports_escalation? && escalation_status
+      current_escalation_status = escalation_status.status_name
+      old_escalation_status = old_associations.fetch(:escalation_status, current_escalation_status)
 
-      if supports_severity? && old_severity != severity
-        changes[:severity] = [old_severity, severity]
+      if old_escalation_status != current_escalation_status
+        changes[:escalation_status] = [old_escalation_status, current_escalation_status]
       end
+    end
 
-      if self.respond_to?(:total_time_spent)
-        old_total_time_spent = old_associations.fetch(:total_time_spent, total_time_spent)
-        old_time_change = old_associations.fetch(:time_change, time_change)
+    if self.respond_to?(:total_time_spent)
+      old_total_time_spent = old_associations.fetch(:total_time_spent, total_time_spent)
+      old_time_change = old_associations.fetch(:time_change, time_change)
 
-        if old_total_time_spent != total_time_spent
-          changes[:total_time_spent] = [old_total_time_spent, total_time_spent]
-          changes[:time_change] = [old_time_change, time_change]
-        end
+      if old_total_time_spent != total_time_spent
+        changes[:total_time_spent] = [old_total_time_spent, total_time_spent]
+        changes[:time_change] = [old_time_change, time_change]
       end
     end
 
+    changes
+  end
+
+  def to_hook_data(user, old_associations: {})
+    changes = previous_changes
+
+    if old_associations.present?
+      changes.merge!(hook_association_changes(old_associations))
+    end
+
     Gitlab::HookData::IssuableBuilder.new(self).build(user: user, changes: changes)
   end
 

app/services/issuable_base_service.rb

@@ -160,7 +160,7 @@ class IssuableBaseService < ::BaseProjectService
       params.delete(:escalation_status)
     ).execute
 
-    return unless result.success? && result.payload.present?
+    return unless result.success? && result[:escalation_status].present?
 
     @escalation_status_change_reason = result[:escalation_status].delete(:status_change_reason)
 
@@ -486,7 +486,10 @@ class IssuableBaseService < ::BaseProjectService
     associations[:description] = issuable.description
     associations[:reviewers] = issuable.reviewers.to_a if issuable.allows_reviewers?
     associations[:severity] = issuable.severity if issuable.supports_severity?
-    associations[:escalation_status] = issuable.escalation_status&.slice(:status, :policy_id) if issuable.supports_escalation?
+
+    if issuable.supports_escalation? && issuable.escalation_status
+      associations[:escalation_status] = issuable.escalation_status.status_name
+    end
 
     associations
   end

app/services/issues/update_service.rb

@@ -51,7 +51,6 @@ module Issues
       old_mentioned_users = old_associations.fetch(:mentioned_users, [])
       old_assignees = old_associations.fetch(:assignees, [])
       old_severity = old_associations[:severity]
-      old_escalation_status = old_associations[:escalation_status]
 
       if has_changes?(issue, old_labels: old_labels, old_assignees: old_assignees)
         todo_service.resolve_todos_for_target(issue, current_user)
@@ -68,7 +67,7 @@ module Issues
       handle_milestone_change(issue)
       handle_added_mentions(issue, old_mentioned_users)
       handle_severity_change(issue, old_severity)
-      handle_escalation_status_change(issue, old_escalation_status)
+      handle_escalation_status_change(issue)
       handle_issue_type_change(issue)
     end
 
@@ -196,9 +195,8 @@ module Issues
       ::IncidentManagement::AddSeveritySystemNoteWorker.perform_async(issue.id, current_user.id)
     end
 
-    def handle_escalation_status_change(issue, old_escalation_status)
-      return unless old_escalation_status.present?
-      return if issue.escalation_status&.slice(:status, :policy_id) == old_escalation_status
+    def handle_escalation_status_change(issue)
+      return unless issue.supports_escalation? && issue.escalation_status
 
       ::IncidentManagement::IssuableEscalationStatuses::AfterUpdateService.new(
         issue,

ee/app/models/concerns/ee/issuable.rb

@@ -28,9 +28,9 @@ module EE
     end
 
     def escalation_policies_available?
-      return false unless ::Gitlab::IncidentManagement.escalation_policies_available?(project)
+      return false unless supports_escalation?
 
-      supports_escalation?
+      ::Gitlab::IncidentManagement.escalation_policies_available?(project)
     end
 
     def metric_images_available?
@@ -50,5 +50,21 @@ module EE
     def supports_iterations?
       false
     end
+
+    override :hook_association_changes
+    def hook_association_changes(old_associations)
+      changes = super
+
+      if supports_escalation? && escalation_status
+        current_escalation_policy = escalation_status.policy
+        old_escalation_policy = old_associations.fetch(:escalation_policy, current_escalation_policy)
+
+        if old_escalation_policy != current_escalation_policy
+          changes[:escalation_policy] = [old_escalation_policy&.hook_attrs, current_escalation_policy&.hook_attrs]
+        end
+      end
+
+      changes
+    end
   end
 end

ee/app/models/incident_management/escalation_policy.rb

@@ -20,5 +20,12 @@ module IncidentManagement
     accepts_nested_attributes_for :rules
 
     delegate :name, to: :project, prefix: true
+
+    def hook_attrs
+      {
+        id: id,
+        name: name
+      }
+    end
   end
 end

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

@@ -56,7 +56,8 @@ module EE
         def current_params
           strong_memoize(:current_params) do
             super.merge(
-              policy: escalation_status.policy
+              policy: escalation_status.policy,
+              escalations_started_at: escalation_status.escalations_started_at
             )
           end
         end

ee/app/services/ee/issuable_base_service.rb

@@ -7,6 +7,17 @@ module EE
 
     private
 
+    override :associations_before_update
+    def associations_before_update(issuable)
+      associations = super
+
+      if issuable.escalation_policies_available? && issuable.escalation_status
+        associations[:escalation_policy] = issuable.escalation_status.policy
+      end
+
+      associations
+    end
+
     attr_reader :label_ids_ordered_by_selection
 
     override :filter_params

ee/lib/ee/gitlab/hook_data/issue_builder.rb

@@ -4,14 +4,35 @@ module EE
     module HookData
       module IssueBuilder
         extend ActiveSupport::Concern
+        extend ::Gitlab::Utils::Override
+
+        EE_SAFE_HOOK_RELATIONS = %i[
+          escalation_policy
+        ].freeze
 
         EE_SAFE_HOOK_ATTRIBUTES = %i[
           weight
         ].freeze
 
+        override :build
+        def build
+          attrs = super
+
+          if issue.escalation_policies_available? && issue.escalation_status
+            attrs[:escalation_policy] = issue.escalation_status.policy&.hook_attrs
+          end
+
+          attrs
+        end
+
         class_methods do
           extend ::Gitlab::Utils::Override
 
+          override :safe_hook_relations
+          def safe_hook_relations
+            super + EE_SAFE_HOOK_RELATIONS
+          end
+
           override :safe_hook_attributes
           def safe_hook_attributes
             super + EE_SAFE_HOOK_ATTRIBUTES

ee/spec/lib/ee/gitlab/hook_data/issue_builder_spec.rb

@@ -50,5 +50,17 @@ RSpec.describe Gitlab::HookData::IssueBuilder do
         expect(data[:description]).to eq("test![Issue_Image](#{Settings.gitlab.url}/#{expected_path}")
       end
     end
+
+    context 'for incident with escalation policies feature enabled' do
+      let_it_be(:issue) { create(:incident, :with_escalation_status) }
+
+      before do
+        stub_licensed_features(oncall_schedules: true, escalation_policies: true)
+      end
+
+      it 'includes additional attr' do
+        expect(data).to include(:escalation_policy)
+      end
+    end
   end
 end

ee/spec/models/concerns/ee/issuable_spec.rb

@@ -135,4 +135,55 @@ RSpec.describe EE::Issuable do
       it { is_expected.to eq(available) }
     end
   end
+
+  describe '#to_hook_data' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:escalation_policy) { create(:incident_management_escalation_policy, project: project) }
+
+    let(:builder) { double }
+
+    context 'escalation status is updated' do
+      let(:issue) { create(:incident, :with_escalation_status) }
+      let(:policy_changes) { { policy: escalation_policy, escalations_started_at: Time.current } }
+      let(:status_changes) { {} }
+      let(:old_associations) { { escalation_status: :triggered, escalation_policy: nil } }
+      let(:expected_policy_hash) { { 'id' => escalation_policy.id, 'name' => escalation_policy.name } }
+
+      before do
+        stub_licensed_features(oncall_schedules: true, escalation_policies: true)
+
+        issue.escalation_status.update!(**policy_changes, **status_changes)
+
+        expect(Gitlab::HookData::IssuableBuilder).to receive(:new).with(issue).and_return(builder)
+      end
+
+      it 'delegates to Gitlab::HookData::IssuableBuilder#build' do
+        expect(builder).to receive(:build).with(
+          user: user,
+          changes: hash_including(
+            'escalation_policy' => [nil, expected_policy_hash]
+          )
+        )
+
+        issue.to_hook_data(user, old_associations: old_associations)
+      end
+
+      context 'with policy and status changes' do
+        let(:status_changes) { { status: IncidentManagement::IssuableEscalationStatus::STATUSES[:acknowledged] } }
+
+        it 'includes both status and policy fields simultaneously' do
+          expect(builder).to receive(:build).with(
+            user: user,
+            changes: hash_including(
+              'escalation_status' => %i(triggered acknowledged),
+              'escalation_policy' => [nil, expected_policy_hash]
+            )
+          )
+
+          issue.to_hook_data(user, old_associations: old_associations)
+        end
+      end
+    end
+  end
 end

ee/spec/models/incident_management/escalation_policy_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe IncidentManagement::EscalationPolicy do
-  subject { build(:incident_management_escalation_policy) }
+  subject(:escalation_policy) { build(:incident_management_escalation_policy) }
 
   it { is_expected.to be_valid }
 
@@ -54,4 +54,10 @@ RSpec.describe IncidentManagement::EscalationPolicy do
       end
     end
   end
+
+  describe '#hook_attrs' do
+    subject { escalation_policy.hook_attrs }
+
+    it { is_expected.to eq({ id: escalation_policy.id, name: escalation_policy.name }) }
+  end
 end

ee/spec/models/incident_management/issuable_escalation_status_spec.rb

@@ -4,9 +4,14 @@ require 'spec_helper'
 
 RSpec.describe IncidentManagement::IssuableEscalationStatus do
   let_it_be(:escalation_status, reload: true) { create(:incident_management_issuable_escalation_status, :paging, :acknowledged) }
+  let_it_be(:escalation_policy) { escalation_status.policy }
 
   subject { escalation_status }
 
+  before do
+    stub_licensed_features(oncall_schedules: true, escalation_policies: true)
+  end
+
   describe 'validations' do
     context 'when policy and escalation start time are both provided' do
       it { is_expected.to be_valid }

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

@@ -44,6 +44,14 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::PrepareUpdateServ
     end
   end
 
+  context 'when policy is unchanged' do
+    let(:params) { { policy: nil } }
+
+    it_behaves_like 'successful response' do
+      let(:payload) { { escalation_status: {} } }
+    end
+  end
+
   context 'when escalation policies feature is unavailable' do
     before do
       stub_licensed_features(oncall_schedules: false, escalation_policies: false)

ee/spec/services/ee/issues/update_service_spec.rb

@@ -475,6 +475,8 @@ RSpec.describe Issues::UpdateService do
         it 'triggers side-effects' do
           expect(escalation_update_class).to receive(:new).with(issue, user, status_change_reason: nil).and_return(service_double)
           expect(service_double).to receive(:execute)
+          expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks)
+          expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :issue_hooks)
 
           update_issue(opts)
         end
@@ -486,7 +488,8 @@ RSpec.describe Issues::UpdateService do
         end
 
         it 'does not trigger side-effects' do
-          expect(escalation_update_class).not_to receive(:new)
+          expect(project).not_to receive(:execute_hooks)
+          expect(project).not_to receive(:execute_integrations)
 
           update_issue(opts)
         end

lib/gitlab/hook_data/issuable_builder.rb

@@ -26,7 +26,7 @@ module Gitlab
       end
 
       def safe_keys
-        issuable_builder.safe_hook_attributes + issuable_builder::SAFE_HOOK_RELATIONS
+        issuable_builder.safe_hook_attributes + issuable_builder.safe_hook_relations
       end
 
       private

lib/gitlab/hook_data/issue_builder.rb

@@ -3,13 +3,16 @@
 module Gitlab
   module HookData
     class IssueBuilder < BaseBuilder
-      SAFE_HOOK_RELATIONS = %i[
-        assignees
-        labels
-        total_time_spent
-        time_change
-        severity
-      ].freeze
+      def self.safe_hook_relations
+        %i[
+          assignees
+          labels
+          total_time_spent
+          time_change
+          severity
+          escalation_status
+        ].freeze
+      end
 
       def self.safe_hook_attributes
         %i[
@@ -56,6 +59,10 @@ module Gitlab
             severity: issue.severity
         }
 
+        if issue.supports_escalation? && issue.escalation_status
+          attrs[:escalation_status] = issue.escalation_status.status_name
+        end
+
         issue.attributes.with_indifferent_access.slice(*self.class.safe_hook_attributes)
           .merge!(attrs)
       end

lib/gitlab/hook_data/merge_request_builder.rb

@@ -34,12 +34,14 @@ module Gitlab
         ].freeze
       end
 
-      SAFE_HOOK_RELATIONS = %i[
-        assignees
-        labels
-        total_time_spent
-        time_change
-      ].freeze
+      def self.safe_hook_relations
+        %i[
+          assignees
+          labels
+          total_time_spent
+          time_change
+        ].freeze
+      end
 
       alias_method :merge_request, :object
 

spec/lib/gitlab/hook_data/issue_builder_spec.rb

@@ -63,5 +63,13 @@ RSpec.describe Gitlab::HookData::IssueBuilder do
           .to eq("test![Issue_Image](#{Settings.gitlab.url}/#{expected_path})")
       end
     end
+
+    context 'for incident' do
+      let_it_be(:issue) { create(:incident, :with_escalation_status) }
+
+      it 'includes additional attr' do
+        expect(data).to include(:escalation_status)
+      end
+    end
   end
 end

spec/models/concerns/issuable_spec.rb

@@ -572,6 +572,27 @@ RSpec.describe Issuable do
         issue.to_hook_data(user, old_associations: { severity: 'unknown' })
       end
     end
+
+    context 'escalation status is updated' do
+      let(:issue) { create(:incident, :with_escalation_status) }
+      let(:acknowledged) { IncidentManagement::IssuableEscalationStatus::STATUSES[:acknowledged] }
+
+      before do
+        issue.escalation_status.update!(status: acknowledged)
+
+        expect(Gitlab::HookData::IssuableBuilder).to receive(:new).with(issue).and_return(builder)
+      end
+
+      it 'delegates to Gitlab::HookData::IssuableBuilder#build' do
+        expect(builder).to receive(:build).with(
+          user: user,
+          changes: hash_including(
+            'escalation_status' => %i(triggered acknowledged)
+          ))
+
+        issue.to_hook_data(user, old_associations: { escalation_status: :triggered })
+      end
+    end
   end
 
   describe '#labels_array' do

spec/models/incident_management/issuable_escalation_status_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe IncidentManagement::IssuableEscalationStatus do
-  let_it_be(:issue) { create(:issue) }
+  let_it_be(:issue) { create(:incident) }
 
   subject(:escalation_status) { build(:incident_management_issuable_escalation_status, issue: issue) }
 

spec/services/issues/update_service_spec.rb

@@ -1157,6 +1157,13 @@ RSpec.describe Issues::UpdateService, :mailer do
 
           expect(issue.escalation_status.status_name).to eq(expected_status)
         end
+
+        it 'triggers webhooks' do
+          expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks)
+          expect(project).to receive(:execute_integrations).with(an_instance_of(Hash), :issue_hooks)
+
+          update_issue(opts)
+        end
       end
 
       shared_examples 'does not change the status record' do
@@ -1169,7 +1176,8 @@ RSpec.describe Issues::UpdateService, :mailer do
         end
 
         it 'does not trigger side-effects' do
-          expect(escalation_update_class).not_to receive(:new)
+          expect(project).not_to receive(:execute_hooks)
+          expect(project).not_to receive(:execute_integrations)
 
           update_issue(opts)
         end