Merge branch 'ck3g-add-system-note-on-change-severity' into 'master'

Add system note on change incidents severity

See merge request gitlab-org/gitlab!42358

app/helpers/system_note_helper.rb

@@ -34,7 +34,8 @@ module SystemNoteHelper
     'designs_discussion_added' => 'doc-image',
     'status' => 'status',
     'alert_issue_added' => 'issues',
-    'new_alert_added' => 'warning'
+    'new_alert_added' => 'warning',
+    'severity' => 'information-o'
   }.freeze
 
   def system_note_icon_name(note)

app/models/concerns/issuable.rb

@@ -203,15 +203,6 @@ module Issuable
       issuable_severity&.severity || IssuableSeverity::DEFAULT
     end
 
-    def update_severity(severity)
-      return unless incident?
-
-      severity = severity.to_s.downcase
-      severity = IssuableSeverity::DEFAULT unless IssuableSeverity.severities.key?(severity)
-
-      (issuable_severity || build_issuable_severity(issue_id: id)).update(severity: severity)
-    end
-
     private
 
     def description_max_length_for_new_records_is_valid

app/models/issuable_severity.rb

@@ -2,6 +2,13 @@
 
 class IssuableSeverity < ApplicationRecord
   DEFAULT = 'unknown'
+  SEVERITY_LABELS = {
+    unknown:  'Unknown',
+    low:      'Low - S4',
+    medium:   'Medium - S3',
+    high:     'High - S2',
+    critical: 'Critical - S1'
+  }.freeze
 
   belongs_to :issue
 

app/models/system_note_metadata.rb

@@ -20,7 +20,7 @@ class SystemNoteMetadata < ApplicationRecord
     title time_tracking branch milestone discussion task moved
     opened closed merged duplicate locked unlocked outdated
     tag due_date pinned_embed cherry_pick health_status approved unapproved
-    status alert_issue_added relate unrelate new_alert_added
+    status alert_issue_added relate unrelate new_alert_added severity
   ].freeze
 
   validates :note, presence: true

app/services/incident_management/incidents/create_service.rb

@@ -24,7 +24,7 @@ module IncidentManagement
 
         return error(issue.errors.full_messages.to_sentence, issue) unless issue.valid?
 
-        issue.update_severity(severity)
+        update_severity_for(issue)
 
         success(issue)
       end
@@ -40,6 +40,10 @@ module IncidentManagement
       def error(message, issue = nil)
         ServiceResponse.error(payload: { issue: issue }, message: message)
       end
+
+      def update_severity_for(issue)
+        ::IncidentManagement::Incidents::UpdateSeverityService.new(issue, current_user, severity).execute
+      end
     end
   end
 end

app/services/incident_management/incidents/update_severity_service.rb

+# frozen_string_literal: true
+
+module IncidentManagement
+  module Incidents
+    class UpdateSeverityService < BaseService
+      def initialize(issuable, current_user, severity)
+        super(issuable.project, current_user)
+
+        @issuable = issuable
+        @severity = severity.to_s.downcase
+        @severity = IssuableSeverity::DEFAULT unless IssuableSeverity.severities.key?(@severity)
+      end
+
+      def execute
+        return unless issuable.incident?
+
+        update_severity!
+        add_system_note
+      end
+
+      private
+
+      attr_reader :issuable, :severity
+
+      def issuable_severity
+        issuable.issuable_severity || issuable.build_issuable_severity(issue_id: issuable.id)
+      end
+
+      def update_severity!
+        issuable_severity.update!(severity: severity)
+      end
+
+      def add_system_note
+        ::IncidentManagement::AddSeveritySystemNoteWorker.perform_async(issuable.id, current_user.id)
+      end
+    end
+  end
+end

app/services/issuable_base_service.rb

@@ -321,7 +321,7 @@ class IssuableBaseService < BaseService
 
   def change_severity(issuable)
     if severity = params.delete(:severity)
-      issuable.update_severity(severity)
+      ::IncidentManagement::Incidents::UpdateSeverityService.new(issuable, current_user, severity).execute
     end
   end
 

app/services/system_note_service.rb

@@ -308,6 +308,10 @@ module SystemNoteService
     ::SystemNotes::AlertManagementService.new(noteable: alert, project: alert.project).create_new_alert(monitoring_tool)
   end
 
+  def change_incident_severity(incident, author)
+    ::SystemNotes::IncidentService.new(noteable: incident, project: incident.project, author: author).change_incident_severity
+  end
+
   private
 
   def merge_requests_service(noteable, project, author)

app/services/system_notes/incident_service.rb

+# frozen_string_literal: true
+
+module SystemNotes
+  class IncidentService < ::SystemNotes::BaseService
+    # Called when the severity of an Incident has changed
+    #
+    # Example Note text:
+    #
+    #   "changed the severity to Medium - S3"
+    #
+    # Returns the created Note object
+    def change_incident_severity
+      return unless Feature.enabled?(:add_severity_system_note, noteable.project)
+
+      severity = noteable.severity
+
+      if severity_label = IssuableSeverity::SEVERITY_LABELS[severity.to_sym]
+        body = "changed the severity to **#{severity_label}**"
+
+        create_note(NoteSummary.new(noteable, project, author, body, action: 'severity'))
+      else
+        Gitlab::AppLogger.error(
+          message: 'Cannot create a system note for severity change',
+          noteable_class: noteable.class.to_s,
+          noteable_id: noteable.id,
+          severity: severity
+        )
+      end
+    end
+  end
+end

app/workers/all_queues.yml

@@ -715,6 +715,14 @@
   :weight: 2
   :idempotent: true
   :tags: []
+- :name: incident_management:incident_management_add_severity_system_note
+  :feature_category: :incident_management
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 2
+  :idempotent:
+  :tags: []
 - :name: incident_management:incident_management_pager_duty_process_incident
   :feature_category: :incident_management
   :has_external_dependencies:

app/workers/incident_management/add_severity_system_note_worker.rb

+# frozen_string_literal: true
+
+module IncidentManagement
+  class AddSeveritySystemNoteWorker # rubocop:disable Scalability/IdempotentWorker
+    include ApplicationWorker
+
+    queue_namespace :incident_management
+    feature_category :incident_management
+
+    def perform(incident_id, user_id)
+      return if incident_id.blank? || user_id.blank?
+
+      incident = Issue.with_issue_type(:incident).find_by_id(incident_id)
+      return unless incident
+
+      user = User.find_by_id(user_id)
+      return unless user
+
+      SystemNoteService.change_incident_severity(incident, user)
+    end
+  end
+end

config/feature_flags/development/add_severity_system_note.yml

+---
+name: add_severity_system_note
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/42358
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/251110
+group: group::health
+type: development
+default_enabled: false

spec/models/concerns/issuable_spec.rb

@@ -926,58 +926,4 @@ RSpec.describe Issuable do
       end
     end
   end
-
-  describe '#update_severity' do
-    let(:severity) { 'low' }
-
-    subject(:update_severity) { issuable.update_severity(severity) }
-
-    context 'when issuable not an incident' do
-      %i(issue merge_request).each do |issuable_type|
-        let(:issuable) { build_stubbed(issuable_type) }
-
-        it { is_expected.to be_nil }
-
-        it 'does not set severity' do
-          expect { subject }.not_to change(IssuableSeverity, :count)
-        end
-      end
-    end
-
-    context 'when issuable is an incident' do
-      let!(:issuable) { create(:incident) }
-
-      context 'when issuable does not have issuable severity yet' do
-        it 'creates new record' do
-          expect { update_severity }.to change { IssuableSeverity.where(issue: issuable).count }.to(1)
-        end
-
-        it 'sets severity to specified value' do
-          expect { update_severity }.to change { issuable.severity }.to('low')
-        end
-      end
-
-      context 'when issuable has an issuable severity' do
-        let!(:issuable_severity) { create(:issuable_severity, issue: issuable, severity: 'medium') }
-
-        it 'does not create new record' do
-          expect { update_severity }.not_to change(IssuableSeverity, :count)
-        end
-
-        it 'updates existing issuable severity' do
-          expect { update_severity }.to change { issuable_severity.severity }.to(severity)
-        end
-      end
-
-      context 'when severity value is unsupported' do
-        let(:severity) { 'unsupported-severity' }
-
-        it 'sets the severity to default value' do
-          update_severity
-
-          expect(issuable.issuable_severity.severity).to eq(IssuableSeverity::DEFAULT)
-        end
-      end
-    end
-  end
 end

spec/services/incident_management/incidents/update_severity_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe IncidentManagement::Incidents::UpdateSeverityService do
+  let_it_be(:user) { create(:user) }
+
+  describe '#execute' do
+    let(:severity) { 'low' }
+    let(:system_note_worker) { ::IncidentManagement::AddSeveritySystemNoteWorker }
+
+    subject(:update_severity) { described_class.new(issuable, user, severity).execute }
+
+    before do
+      allow(system_note_worker).to receive(:perform_async)
+    end
+
+    shared_examples 'adds a system note' do
+      it 'calls AddSeveritySystemNoteWorker' do
+        update_severity
+
+        expect(system_note_worker).to have_received(:perform_async).with(issuable.id, user.id)
+      end
+    end
+
+    context 'when issuable not an incident' do
+      %i(issue merge_request).each do |issuable_type|
+        let(:issuable) { build_stubbed(issuable_type) }
+
+        it { is_expected.to be_nil }
+
+        it 'does not set severity' do
+          expect { update_severity }.not_to change(IssuableSeverity, :count)
+        end
+
+        it 'does not add a system note' do
+          update_severity
+
+          expect(system_note_worker).not_to have_received(:perform_async)
+        end
+      end
+    end
+
+    context 'when issuable is an incident' do
+      let!(:issuable) { create(:incident) }
+
+      context 'when issuable does not have issuable severity yet' do
+        it 'creates new record' do
+          expect { update_severity }.to change { IssuableSeverity.where(issue: issuable).count }.to(1)
+        end
+
+        it 'sets severity to specified value' do
+          expect { update_severity }.to change { issuable.severity }.to('low')
+        end
+
+        it_behaves_like 'adds a system note'
+      end
+
+      context 'when issuable has an issuable severity' do
+        let!(:issuable_severity) { create(:issuable_severity, issue: issuable, severity: 'medium') }
+
+        it 'does not create new record' do
+          expect { update_severity }.not_to change(IssuableSeverity, :count)
+        end
+
+        it 'updates existing issuable severity' do
+          expect { update_severity }.to change { issuable_severity.severity }.to(severity)
+        end
+
+        it_behaves_like 'adds a system note'
+      end
+
+      context 'when severity value is unsupported' do
+        let(:severity) { 'unsupported-severity' }
+
+        it 'sets the severity to default value' do
+          update_severity
+
+          expect(issuable.issuable_severity.severity).to eq(IssuableSeverity::DEFAULT)
+        end
+
+        it_behaves_like 'adds a system note'
+      end
+    end
+  end
+end

spec/services/system_note_service_spec.rb

@@ -741,4 +741,16 @@ RSpec.describe SystemNoteService do
       described_class.create_new_alert(alert, monitoring_tool)
     end
   end
+
+  describe '.change_incident_severity' do
+    let(:incident) { build(:incident) }
+
+    it 'calls IncidentService' do
+      expect_next_instance_of(SystemNotes::IncidentService) do |service|
+        expect(service).to receive(:change_incident_severity)
+      end
+
+      described_class.change_incident_severity(incident, author)
+    end
+  end
 end

spec/services/system_notes/incident_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::SystemNotes::IncidentService do
+  let_it_be(:author) { create(:user) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:noteable) { create(:incident, project: project) }
+  let_it_be(:issuable_severity) { create(:issuable_severity, issue: noteable, severity: :medium) }
+
+  describe '#change_incident_severity' do
+    subject(:change_severity) { described_class.new(noteable: noteable, project: project, author: author).change_incident_severity }
+
+    before do
+      allow(Gitlab::AppLogger).to receive(:error).and_call_original
+    end
+
+    context 'with add_severity_system_note feature flag enabled' do
+      before do
+        stub_feature_flags(add_severity_system_note: project)
+      end
+
+      it_behaves_like 'a system note' do
+        let(:action) { 'severity' }
+      end
+
+      IssuableSeverity.severities.keys.each do |severity|
+        context "with #{severity} severity" do
+          before do
+            issuable_severity.update!(severity: severity)
+          end
+
+          it 'has the appropriate message' do
+            severity_label = IssuableSeverity::SEVERITY_LABELS.fetch(severity.to_sym)
+
+            expect(change_severity.note).to eq("changed the severity to **#{severity_label}**")
+          end
+        end
+      end
+
+      context 'when severity is invalid' do
+        let(:invalid_severity) { 'invalid-severity' }
+
+        before do
+          allow(noteable).to receive(:severity).and_return(invalid_severity)
+        end
+
+        it 'does not create system note' do
+          expect { change_severity }.not_to change { noteable.notes.count }
+        end
+
+        it 'writes error to logs' do
+          change_severity
+
+          expect(Gitlab::AppLogger).to have_received(:error).with(
+            message: 'Cannot create a system note for severity change',
+            noteable_class: noteable.class.to_s,
+            noteable_id: noteable.id,
+            severity: invalid_severity
+          )
+        end
+      end
+    end
+
+    context 'with add_severity_system_note feature flag disabled' do
+      before do
+        stub_feature_flags(add_severity_system_note: false)
+      end
+
+      it 'does not create system note' do
+        expect { change_severity }.not_to change { noteable.notes.count }
+      end
+
+      it 'does not write error to logs' do
+        expect(Gitlab::AppLogger).not_to have_received(:error)
+      end
+    end
+  end
+end

spec/workers/incident_management/add_severity_system_note_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe IncidentManagement::AddSeveritySystemNoteWorker do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:incident) { create(:incident, project: project) }
+  let_it_be(:issuable_severity) { create(:issuable_severity, issue: incident, severity: :medium) }
+
+  describe '#perform' do
+    let(:incident_id) { incident.id }
+    let(:user_id) { user.id }
+
+    subject(:perform) { described_class.new.perform(incident_id, user_id) }
+
+    shared_examples 'does not add a system note' do
+      it 'does not change incident notes count' do
+        expect { perform }.not_to change { incident.notes.count }
+      end
+    end
+
+    context 'when incident and user exist' do
+      it 'creates a system note' do
+        expect { perform }.to change { incident.notes.where(author: user).count }.by(1)
+      end
+    end
+
+    context 'when incident does not exist' do
+      let(:incident_id) { -1 }
+
+      it_behaves_like 'does not add a system note'
+    end
+
+    context 'when incident_id is nil' do
+      let(:incident_id) { nil }
+
+      it_behaves_like 'does not add a system note'
+    end
+
+    context 'when issue is not an incident' do
+      let_it_be(:issue) { create(:issue, project: project) }
+      let(:incident_id) { issue.id }
+
+      it_behaves_like 'does not add a system note'
+    end
+
+    context 'when user does not exist' do
+      let(:user_id) { -1 }
+
+      it_behaves_like 'does not add a system note'
+    end
+
+    context 'when user_id is nil' do
+      let(:user_id) { nil }
+
+      it_behaves_like 'does not add a system note'
+    end
+  end
+end