Merge branch 'sy-alert-status-system-notes' into 'master'

Add system notes for status updates

See merge request gitlab-org/gitlab!35467

app/assets/javascripts/alert_management/graphql/mutations/update_alert_status.mutation.graphql

+#import "../fragments/alert_note.fragment.graphql"
+
 mutation updateAlertStatus($projectPath: ID!, $status: AlertManagementStatus!, $iid: String!) {
   updateAlertStatus(input: { iid: $iid, status: $status, projectPath: $projectPath }) {
     errors
     alert {
       iid,
       status,
-      endedAt
+      endedAt,
+      notes {
+        nodes {
+          ...AlertNote
+        }
+      }
     }
   }
 }

app/graphql/mutations/alert_management/update_alert_status.rb

@@ -19,8 +19,8 @@ module Mutations
       private
 
       def update_status(alert, status)
-        ::AlertManagement::UpdateAlertStatusService
-          .new(alert, current_user, status)
+        ::AlertManagement::Alerts::UpdateService
+          .new(alert, current_user, status: status)
           .execute
       end
 

app/helpers/system_note_helper.rb

@@ -31,7 +31,8 @@ module SystemNoteHelper
     'designs_added' => 'doc-image',
     'designs_modified' => 'doc-image',
     'designs_removed' => 'doc-image',
-    'designs_discussion_added' => 'doc-image'
+    'designs_discussion_added' => 'doc-image',
+    'status' => 'status'
   }.freeze
 
   def system_note_icon_name(note)

app/models/system_note_metadata.rb

@@ -19,6 +19,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
   ].freeze
 
   validates :note, presence: true

app/services/alert_management/alerts/update_service.rb

@@ -12,19 +12,20 @@ module AlertManagement
         @alert = alert
         @current_user = current_user
         @params = params
+        @param_errors = []
       end
 
       def execute
         return error_no_permissions unless allowed?
-        return error_no_updates if params.empty?
 
-        filter_assignees
-        return error_no_assignee_permissions if unauthorized_assignees?
+        filter_params
+        return error_invalid_params if param_errors.any?
 
+        # Save old assignees for system notes
         old_assignees = alert.assignees.to_a
 
         if alert.update(params)
-          process_assignement(old_assignees)
+          handle_changes(old_assignees: old_assignees)
 
           success
         else
@@ -34,7 +35,8 @@ module AlertManagement
 
       private
 
-      attr_reader :alert, :current_user, :params
+      attr_reader :alert, :current_user, :params, :param_errors
+      delegate :resolved?, to: :alert
 
       def allowed?
         current_user&.can?(:update_alert_management_alert, alert)
@@ -58,40 +60,80 @@ module AlertManagement
         error(_('You have no permissions'))
       end
 
-      def error_no_updates
-        error(_('Please provide attributes to update'))
+      def error_invalid_params
+        error(param_errors.to_sentence)
       end
 
-      def error_no_assignee_permissions
-        error(_('Assignee has no permissions'))
+      def add_param_error(message)
+        param_errors << message
       end
 
-      # ----- Assignee-related behavior ------
-      def unauthorized_assignees?
-        params[:assignees]&.any? { |user| !user.can?(:read_alert_management_alert, alert) }
+      def filter_params
+        param_errors << _('Please provide attributes to update') if params.empty?
+
+        filter_status
+        filter_assignees
+      end
+
+      def handle_changes(old_assignees:)
+        handle_assignement(old_assignees) if params[:assignees]
+        handle_status_change if params[:status_event]
       end
 
+      # ----- Assignee-related behavior ------
       def filter_assignees
         return if params[:assignees].nil?
 
         # Always take first assignee while multiple are not currently supported
         params[:assignees] = Array(params[:assignees].first)
+
+        param_errors << _('Assignee has no permissions') if unauthorized_assignees?
+      end
+
+      def unauthorized_assignees?
+        params[:assignees]&.any? { |user| !user.can?(:read_alert_management_alert, alert) }
       end
 
-      def process_assignement(old_assignees)
+      def handle_assignement(old_assignees)
         assign_todo
         add_assignee_system_note(old_assignees)
       end
 
       def assign_todo
-        return if alert.assignees.empty?
-
         todo_service.assign_alert(alert, current_user)
       end
 
       def add_assignee_system_note(old_assignees)
         SystemNoteService.change_issuable_assignees(alert, alert.project, current_user, old_assignees)
       end
+
+      # ------ Status-related behavior -------
+      def filter_status
+        return unless status = params.delete(:status)
+
+        status_key = AlertManagement::Alert::STATUSES.key(status)
+        status_event = AlertManagement::Alert::STATUS_EVENTS[status_key]
+
+        unless status_event
+          param_errors << _('Invalid status')
+          return
+        end
+
+        params[:status_event] = status_event
+      end
+
+      def handle_status_change
+        add_status_change_system_note
+        resolve_todos if resolved?
+      end
+
+      def add_status_change_system_note
+        SystemNoteService.change_alert_status(alert, current_user)
+      end
+
+      def resolve_todos
+        todo_service.resolve_todos_for_target(alert, current_user)
+      end
     end
   end
 end

app/services/alert_management/update_alert_status_service.rb

-# frozen_string_literal: true
-
-module AlertManagement
-  class UpdateAlertStatusService
-    include Gitlab::Utils::StrongMemoize
-
-    # @param alert [AlertManagement::Alert]
-    # @param user [User]
-    # @param status [Integer] Must match a value from AlertManagement::Alert::STATUSES
-    def initialize(alert, user, status)
-      @alert = alert
-      @user = user
-      @status = status
-    end
-
-    def execute
-      return error_no_permissions unless allowed?
-      return error_invalid_status unless status_key
-
-      if alert.update(status_event: status_event)
-        resolve_todos if resolved?
-        success
-      else
-        error(alert.errors.full_messages.to_sentence)
-      end
-    end
-
-    private
-
-    attr_reader :alert, :user, :status
-
-    delegate :project, :resolved?, to: :alert
-
-    def allowed?
-      user.can?(:update_alert_management_alert, project)
-    end
-
-    def resolve_todos
-      TodoService.new.resolve_todos_for_target(alert, user)
-    end
-
-    def status_key
-      strong_memoize(:status_key) do
-        AlertManagement::Alert::STATUSES.key(status)
-      end
-    end
-
-    def status_event
-      AlertManagement::Alert::STATUS_EVENTS[status_key]
-    end
-
-    def success
-      ServiceResponse.success(payload: { alert: alert })
-    end
-
-    def error_no_permissions
-      error(_('You have no permissions'))
-    end
-
-    def error_invalid_status
-      error(_('Invalid status'))
-    end
-
-    def error(message)
-      ServiceResponse.error(payload: { alert: alert }, message: message)
-    end
-  end
-end

app/services/system_note_service.rb

@@ -292,6 +292,10 @@ 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)
+  end
+
   private
 
   def merge_requests_service(noteable, project, author)

app/services/system_notes/alert_management_service.rb

+# frozen_string_literal: true
+
+module SystemNotes
+  class AlertManagementService < ::SystemNotes::BaseService
+    # Called when the status of an AlertManagement::Alert has changed
+    #
+    # alert - AlertManagement::Alert object.
+    #
+    # Example Note text:
+    #
+    #   "changed the status to Acknowledged"
+    #
+    # Returns the created Note object
+    def change_alert_status(alert)
+      status = AlertManagement::Alert::STATUSES.key(alert.status).to_s.titleize
+      body = "changed the status to **#{status}**"
+
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'status'))
+    end
+  end
+end

changelogs/unreleased/sy-alert-status-system-notes.yml

+---
+title: Add system notes for status updates on alerts
+merge_request: 35467
+author:
+type: added

spec/services/alert_management/alerts/update_service_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
   let_it_be(:user_with_permissions) { create(:user) }
   let_it_be(:other_user_with_permissions) { create(:user) }
   let_it_be(:user_without_permissions) { create(:user) }
-  let_it_be(:alert, reload: true) { create(:alert_management_alert) }
+  let_it_be(:alert, reload: true) { create(:alert_management_alert, :triggered) }
   let_it_be(:project) { alert.project }
 
   let(:current_user) { user_with_permissions }
@@ -28,6 +28,10 @@ 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) }
+    end
+
     shared_examples 'error response' do |message|
       it_behaves_like 'does not add a todo'
       it_behaves_like 'does not add a system note'
@@ -86,10 +90,6 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
         end
       end
 
-      shared_examples 'adds a system note' do
-        specify { expect { response }.to change { alert.reload.notes.count }.by(1) }
-      end
-
       shared_examples 'successful assignment' do
         it_behaves_like 'adds a system note'
         it_behaves_like 'adds a todo'
@@ -143,5 +143,38 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
         it_behaves_like 'successful assignment'
       end
     end
+
+    context 'when a status is included' do
+      let(:params) { { status: new_status } }
+      let(:new_status) { AlertManagement::Alert::STATUSES[:acknowledged] }
+
+      it 'successfully changes the status' do
+        expect { response }.to change { alert.acknowledged? }.to(true)
+        expect(response).to be_success
+        expect(response.payload[:alert]).to eq(alert)
+      end
+
+      it_behaves_like 'adds a system note'
+
+      context 'with unknown status' do
+        let(:new_status) { -1 }
+
+        it_behaves_like 'error response', 'Invalid status'
+      end
+
+      context 'with resolving status' do
+        let(:new_status) { AlertManagement::Alert::STATUSES[:resolved] }
+
+        it 'changes the status' do
+          expect { response }.to change { alert.resolved? }.to(true)
+        end
+
+        it "resolves the current user's related todos" do
+          todo = create(:todo, :pending, target: alert, user: current_user, project: alert.project)
+
+          expect { response }.to change { todo.reload.state }.from('pending').to('done')
+        end
+      end
+    end
   end
 end

spec/services/alert_management/update_alert_status_service_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe AlertManagement::UpdateAlertStatusService do
-  let(:project) { alert.project }
-  let_it_be(:user) { build(:user) }
-
-  let_it_be(:alert, reload: true) do
-    create(:alert_management_alert, :triggered)
-  end
-
-  let(:service) { described_class.new(alert, user, new_status) }
-
-  describe '#execute' do
-    shared_examples 'update failure' do |error_message|
-      it 'returns an error' do
-        expect(response).to be_error
-        expect(response.message).to eq(error_message)
-        expect(response.payload[:alert]).to eq(alert)
-      end
-
-      it 'does not update the status' do
-        expect { response }.not_to change { alert.status }
-      end
-    end
-
-    let(:new_status) { AlertManagement::Alert::STATUSES[:acknowledged] }
-    let(:can_update) { true }
-
-    subject(:response) { service.execute }
-
-    before do
-      allow(user).to receive(:can?)
-        .with(:update_alert_management_alert, project)
-        .and_return(can_update)
-    end
-
-    it 'returns success' do
-      expect(response).to be_success
-      expect(response.payload[:alert]).to eq(alert)
-    end
-
-    it 'updates the status' do
-      expect { response }.to change { alert.acknowledged? }.to(true)
-    end
-
-    context 'resolving status' do
-      let(:new_status) { AlertManagement::Alert::STATUSES[:resolved] }
-
-      it 'updates the status' do
-        expect { response }.to change { alert.resolved? }.to(true)
-      end
-
-      context 'user has a pending todo' do
-        let(:user) { create(:user) }
-        let!(:todo) { create(:todo, :pending, target: alert, user: user, project: alert.project) }
-
-        it 'resolves the todo' do
-          expect { response }.to change { todo.reload.state }.from('pending').to('done')
-        end
-      end
-    end
-
-    context 'when user has no permissions' do
-      let(:can_update) { false }
-
-      include_examples 'update failure', _('You have no permissions')
-    end
-
-    context 'with no status' do
-      let(:new_status) { nil }
-
-      include_examples 'update failure', _('Invalid status')
-    end
-
-    context 'with unknown status' do
-      let(:new_status) { -1 }
-
-      include_examples 'update failure', _('Invalid status')
-    end
-  end
-end

spec/services/system_note_service_spec.rb

@@ -681,4 +681,16 @@ RSpec.describe SystemNoteService do
       described_class.unapprove_mr(noteable, author)
     end
   end
+
+  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)
+      end
+
+      described_class.change_alert_status(alert, author)
+    end
+  end
 end

spec/services/system_notes/alert_management_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::SystemNotes::AlertManagementService do
+  let_it_be(:author)   { create(:user) }
+  let_it_be(:project)  { create(:project, :repository) }
+
+  let(:noteable) { create(:alert_management_alert, :acknowledged, project: project) }
+
+  describe '#change_alert_status' do
+    subject { described_class.new(noteable: noteable, project: project, author: author).change_alert_status(noteable) }
+
+    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
+end