Merge branch 'sy-add-system-notes-for-incident-status-changes' into 'master'

Add system note when escalation status changes

See merge request gitlab-org/gitlab!77071

app/services/alert_management/alerts/update_service.rb

@@ -12,6 +12,7 @@ module AlertManagement
         @alert = alert
         @param_errors = []
         @status = params.delete(:status)
+        @status_change_reason = params.delete(:status_change_reason)
 
         super(project: alert.project, current_user: current_user, params: params)
       end
@@ -36,7 +37,7 @@ module AlertManagement
 
       private
 
-      attr_reader :alert, :param_errors, :status
+      attr_reader :alert, :param_errors, :status, :status_change_reason
 
       def allowed?
         current_user&.can?(:update_alert_management_alert, alert)
@@ -133,7 +134,7 @@ module AlertManagement
       end
 
       def add_status_change_system_note
-        SystemNoteService.change_alert_status(alert, current_user)
+        SystemNoteService.change_alert_status(alert, current_user, status_change_reason)
       end
 
       def resolve_todos
@@ -144,7 +145,12 @@ module AlertManagement
         ::Issues::UpdateService.new(
           project: project,
           current_user: current_user,
-          params: { escalation_status: { status: status } }
+          params: {
+            escalation_status: {
+              status: status,
+              status_change_reason: " by changing the status of #{alert.to_reference(project)}"
+            }
+          }
         ).execute(alert.issue)
       end
 

app/services/incident_management/issuable_escalation_statuses/after_update_service.rb

@@ -3,12 +3,12 @@
 module IncidentManagement
   module IssuableEscalationStatuses
     class AfterUpdateService < ::BaseProjectService
-      def initialize(issuable, current_user)
+      def initialize(issuable, current_user, **params)
         @issuable = issuable
         @escalation_status = issuable.escalation_status
         @alert = issuable.alert_management_alert
 
-        super(project: issuable.project, current_user: current_user)
+        super(project: issuable.project, current_user: current_user, params: params)
       end
 
       def execute
@@ -22,19 +22,27 @@ module IncidentManagement
       attr_reader :issuable, :escalation_status, :alert
 
       def after_update
-        sync_to_alert
+        sync_status_to_alert
+        add_status_system_note
       end
 
-      def sync_to_alert
+      def sync_status_to_alert
         return unless alert
         return if alert.status == escalation_status.status
 
         ::AlertManagement::Alerts::UpdateService.new(
           alert,
           current_user,
-          status: escalation_status.status_name
+          status: escalation_status.status_name,
+          status_change_reason: " by changing the incident status of #{issuable.to_reference(project)}"
         ).execute
       end
+
+      def add_status_system_note
+        return unless escalation_status.status_previously_changed?
+
+        SystemNoteService.change_incident_status(issuable, current_user, params[:status_change_reason])
+      end
     end
   end
 end

app/services/incident_management/issuable_escalation_statuses/prepare_update_service.rb

@@ -5,7 +5,7 @@ module IncidentManagement
     class PrepareUpdateService
       include Gitlab::Utils::StrongMemoize
 
-      SUPPORTED_PARAMS = %i[status].freeze
+      SUPPORTED_PARAMS = %i[status status_change_reason].freeze
 
       InvalidParamError = Class.new(StandardError)
 

app/services/issuable_base_service.rb

@@ -162,6 +162,8 @@ class IssuableBaseService < ::BaseProjectService
 
     return unless result.success? && result.payload.present?
 
+    @escalation_status_change_reason = result[:escalation_status].delete(:status_change_reason)
+
     params[:incident_management_issuable_escalation_status_attributes] = result[:escalation_status]
   end
 

app/services/issues/update_service.rb

@@ -214,7 +214,11 @@ module Issues
       return unless old_escalation_status.present?
       return if issue.escalation_status&.slice(:status, :policy_id) == old_escalation_status
 
-      ::IncidentManagement::IssuableEscalationStatuses::AfterUpdateService.new(issue, current_user).execute
+      ::IncidentManagement::IssuableEscalationStatuses::AfterUpdateService.new(
+        issue,
+        current_user,
+        status_change_reason: @escalation_status_change_reason # Defined in IssuableBaseService before save
+      ).execute
     end
 
     # rubocop: disable CodeReuse/ActiveRecord

app/services/system_note_service.rb

@@ -319,8 +319,8 @@ module SystemNoteService
     merge_requests_service(noteable, noteable.project, user).unapprove_mr
   end
 
-  def change_alert_status(alert, author)
-    ::SystemNotes::AlertManagementService.new(noteable: alert, project: alert.project, author: author).change_alert_status(alert)
+  def change_alert_status(alert, author, reason = nil)
+    ::SystemNotes::AlertManagementService.new(noteable: alert, project: alert.project, author: author).change_alert_status(reason)
   end
 
   def new_alert_issue(alert, issue, author)
@@ -339,6 +339,10 @@ module SystemNoteService
     ::SystemNotes::IncidentService.new(noteable: incident, project: incident.project, author: author).resolve_incident_status
   end
 
+  def change_incident_status(incident, author, reason = nil)
+    ::SystemNotes::IncidentService.new(noteable: incident, project: incident.project, author: author).change_incident_status(reason)
+  end
+
   def log_resolving_alert(alert, monitoring_tool)
     ::SystemNotes::AlertManagementService.new(noteable: alert, project: alert.project).log_resolving_alert(monitoring_tool)
   end

app/services/system_notes/alert_management_service.rb

@@ -24,11 +24,12 @@ module SystemNotes
     # Example Note text:
     #
     #   "changed the status to Acknowledged"
+    #   "changed the status to Acknowledged by changing the incident status of #540"
     #
     # Returns the created Note object
-    def change_alert_status(alert)
-      status = alert.state.to_s.titleize
-      body = "changed the status to **#{status}**"
+    def change_alert_status(reason)
+      status = noteable.state.to_s.titleize
+      body = "changed the status to **#{status}**#{reason}"
 
       create_note(NoteSummary.new(noteable, project, author, body, action: 'status'))
     end

app/services/system_notes/incident_service.rb

@@ -31,5 +31,22 @@ module SystemNotes
 
       create_note(NoteSummary.new(noteable, project, author, body, action: 'status'))
     end
+
+    # Called when the status of an IncidentManagement::IssuableEscalationStatus has changed
+    #
+    # reason - String.
+    #
+    # Example Note text:
+    #
+    #   "changed the incident status to Acknowledged"
+    #   "changed the incident status to Acknowledged by changing the status of ^alert#540"
+    #
+    # Returns the created Note object
+    def change_incident_status(reason)
+      status = noteable.escalation_status.status_name.to_s.titleize
+      body = "changed the incident status to **#{status}**#{reason}"
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'status'))
+    end
   end
 end

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

@@ -473,7 +473,7 @@ RSpec.describe Issues::UpdateService do
         end
 
         it 'triggers side-effects' do
-          expect(escalation_update_class).to receive(:new).with(issue, user).and_return(service_double)
+          expect(escalation_update_class).to receive(:new).with(issue, user, status_change_reason: nil).and_return(service_double)
           expect(service_double).to receive(:execute)
 
           update_issue(opts)

spec/services/alert_management/alerts/update_service_spec.rb

@@ -28,8 +28,11 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
       specify { expect { response }.not_to change(Note, :count) }
     end
 
-    shared_examples 'adds a system note' do
-      specify { expect { response }.to change { alert.reload.notes.count }.by(1) }
+    shared_examples 'adds a system note' do |note_matcher = nil|
+      specify do
+        expect { response }.to change { alert.reload.notes.count }.by(1)
+        expect(alert.notes.last.note).to match(note_matcher) if note_matcher
+      end
     end
 
     shared_examples 'error response' do |message|
@@ -288,6 +291,12 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
           end
         end
       end
+
+      context 'when a status change reason is included' do
+        let(:params) { { status: new_status, status_change_reason: ' by changing the incident status' } }
+
+        it_behaves_like 'adds a system note', /changed the status to \*\*Acknowledged\*\* by changing the incident status/
+      end
     end
   end
 end

spec/services/incident_management/issuable_escalation_statuses/after_update_service_spec.rb

@@ -30,7 +30,15 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::AfterUpdateServic
     end
   end
 
+  shared_examples 'adds a status change system note' do
+    specify do
+      expect { result }.to change { issue.reload.notes.count }.by(1)
+    end
+  end
+
   context 'with status attributes' do
+    it_behaves_like 'adds a status change system note'
+
     it 'updates the alert with the new alert status' do
       expect(::AlertManagement::Alerts::UpdateService).to receive(:new).once.and_call_original
       expect(described_class).to receive(:new).once.and_call_original
@@ -45,12 +53,15 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::AfterUpdateServic
       end
 
       it_behaves_like 'does not attempt to update the alert'
+      it_behaves_like 'adds a status change system note'
     end
 
     context 'when new status matches the current status' do
       let(:status_event) { :trigger }
 
       it_behaves_like 'does not attempt to update the alert'
+
+      specify { expect { result }.not_to change { issue.reload.notes.count } }
     end
   end
 end

spec/services/incident_management/issuable_escalation_statuses/prepare_update_service_spec.rb

@@ -105,4 +105,10 @@ RSpec.describe IncidentManagement::IssuableEscalationStatuses::PrepareUpdateServ
       it_behaves_like 'successful response', { status_event: :acknowledge }
     end
   end
+
+  context 'with status_change_reason param' do
+    let(:params) { { status_change_reason: ' by changing the incident status' } }
+
+    it_behaves_like 'successful response', { status_change_reason: ' by changing the incident status' }
+  end
 end

spec/services/issues/update_service_spec.rb

@@ -1147,11 +1147,11 @@ RSpec.describe Issues::UpdateService, :mailer do
       let(:opts) { { escalation_status: { status: 'acknowledged' } } }
       let(:escalation_update_class) { ::IncidentManagement::IssuableEscalationStatuses::AfterUpdateService }
 
-      shared_examples 'updates the escalation status record' do |expected_status|
+      shared_examples 'updates the escalation status record' do |expected_status, expected_reason = nil|
         let(:service_double) { instance_double(escalation_update_class) }
 
         it 'has correct value' do
-          expect(escalation_update_class).to receive(:new).with(issue, user).and_return(service_double)
+          expect(escalation_update_class).to receive(:new).with(issue, user, status_change_reason: expected_reason).and_return(service_double)
           expect(service_double).to receive(:execute)
 
           update_issue(opts)
@@ -1197,6 +1197,12 @@ RSpec.describe Issues::UpdateService, :mailer do
             end
           end
 
+          context 'with a status change reason provided' do
+            let(:opts) { { escalation_status: { status: 'acknowledged', status_change_reason: ' by changing the alert status' } } }
+
+            it_behaves_like 'updates the escalation status record', :acknowledged, ' by changing the alert status'
+          end
+
           context 'with unsupported status value' do
             let(:opts) { { escalation_status: { status: 'unsupported-status' } } }
 

spec/services/system_note_service_spec.rb

@@ -572,12 +572,26 @@ RSpec.describe SystemNoteService do
   describe '.change_alert_status' do
     let(:alert) { build(:alert_management_alert) }
 
-    it 'calls AlertManagementService' do
-      expect_next_instance_of(SystemNotes::AlertManagementService) do |service|
-        expect(service).to receive(:change_alert_status).with(alert)
+    context 'with status change reason' do
+      let(:reason) { 'reason for status change' }
+
+      it 'calls AlertManagementService' do
+        expect_next_instance_of(SystemNotes::AlertManagementService) do |service|
+          expect(service).to receive(:change_alert_status).with(reason)
+        end
+
+        described_class.change_alert_status(alert, author, reason)
       end
+    end
 
-      described_class.change_alert_status(alert, author)
+    context 'without status change reason' do
+      it 'calls AlertManagementService' do
+        expect_next_instance_of(SystemNotes::AlertManagementService) do |service|
+          expect(service).to receive(:change_alert_status).with(nil)
+        end
+
+        described_class.change_alert_status(alert, author)
+      end
     end
   end
 
@@ -630,6 +644,32 @@ RSpec.describe SystemNoteService do
     end
   end
 
+  describe '.change_incident_status' do
+    let(:incident) { instance_double('Issue', project: project) }
+
+    context 'with status change reason' do
+      let(:reason) { 'reason for status change' }
+
+      it 'calls IncidentService' do
+        expect_next_instance_of(SystemNotes::IncidentService) do |service|
+          expect(service).to receive(:change_incident_status).with(reason)
+        end
+
+        described_class.change_incident_status(incident, author, reason)
+      end
+    end
+
+    context 'without status change reason' do
+      it 'calls IncidentService' do
+        expect_next_instance_of(SystemNotes::IncidentService) do |service|
+          expect(service).to receive(:change_incident_status).with(nil)
+        end
+
+        described_class.change_incident_status(incident, author)
+      end
+    end
+  end
+
   describe '.log_resolving_alert' do
     let(:alert) { build(:alert_management_alert) }
     let(:monitoring_tool) { 'Prometheus' }

spec/services/system_notes/alert_management_service_spec.rb

@@ -21,14 +21,26 @@ RSpec.describe ::SystemNotes::AlertManagementService do
   end
 
   describe '#change_alert_status' do
-    subject { described_class.new(noteable: noteable, project: project, author: author).change_alert_status(noteable) }
+    subject { described_class.new(noteable: noteable, project: project, author: author).change_alert_status(reason) }
 
-    it_behaves_like 'a system note' do
-      let(:action) { 'status' }
+    context 'with no specified reason' do
+      let(:reason) { nil }
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'status' }
+      end
+
+      it 'has the appropriate message' do
+        expect(subject.note).to eq("changed the status to **Acknowledged**")
+      end
     end
 
-    it 'has the appropriate message' do
-      expect(subject.note).to eq("changed the status to **Acknowledged**")
+    context 'with reason provided' do
+      let(:reason) { ' by changing incident status' }
+
+      it 'has the appropriate message' do
+        expect(subject.note).to eq("changed the status to **Acknowledged** by changing incident status")
+      end
     end
   end
 

spec/services/system_notes/incident_service_spec.rb

@@ -66,4 +66,28 @@ RSpec.describe ::SystemNotes::IncidentService do
       expect(noteable.notes.last.note).to eq('changed the status to **Resolved** by closing the incident')
     end
   end
+
+  describe '#change_incident_status' do
+    let_it_be(:escalation_status) { create(:incident_management_issuable_escalation_status, issue: noteable) }
+
+    let(:service) { described_class.new(noteable: noteable, project: project, author: author) }
+
+    context 'with a provided reason' do
+      subject(:change_incident_status) { service.change_incident_status(' by changing the alert status') }
+
+      it 'creates a new note for an incident status change', :aggregate_failures do
+        expect { change_incident_status }.to change { noteable.notes.count }.by(1)
+        expect(noteable.notes.last.note).to eq("changed the incident status to **Triggered** by changing the alert status")
+      end
+    end
+
+    context 'without provided reason' do
+      subject(:change_incident_status) { service.change_incident_status(nil) }
+
+      it 'creates a new note for an incident status change', :aggregate_failures do
+        expect { change_incident_status }.to change { noteable.notes.count }.by(1)
+        expect(noteable.notes.last.note).to eq("changed the incident status to **Triggered**")
+      end
+    end
+  end
 end