Merge branch 'sy-swap-alert-email-api' into 'master'

Swap alert emails to trigger at the same point

See merge request gitlab-org/gitlab!43054

app/mailers/emails/projects.rb

@@ -56,12 +56,9 @@ module Emails
            subject:   @message.subject)
     end
 
-    def prometheus_alert_fired_email(project_id, user_id, alert_attributes)
-      @project = ::Project.find(project_id)
-      user = ::User.find(user_id)
-
-      @alert = AlertManagement::Alert.new(alert_attributes.with_indifferent_access).present
-      return unless @alert.parsed_payload.has_required_attributes?
+    def prometheus_alert_fired_email(project, user, alert)
+      @project = project
+      @alert = alert.present
 
       subject_text = "Alert: #{@alert.email_title}"
       mail(to: user.notification_email_for(@project.group), subject: subject(subject_text))

app/services/alert_management/process_prometheus_alert_service.rb

@@ -9,6 +9,10 @@ module AlertManagement
       return bad_request unless incoming_payload.has_required_attributes?
 
       process_alert_management_alert
+      return bad_request unless alert.persisted?
+
+      process_incident_issues if process_issues?
+      send_alert_email if send_email?
 
       ServiceResponse.success
     end
@@ -30,8 +34,6 @@ module AlertManagement
       else
         create_alert_management_alert
       end
-
-      process_incident_issues if process_issues?
     end
 
     def reset_alert_management_alert_status
@@ -85,12 +87,17 @@ module AlertManagement
     end
 
     def process_incident_issues
-      return unless alert.persisted?
-      return if alert.issue
+      return if alert.issue || alert.resolved?
 
       IncidentManagement::ProcessAlertWorker.perform_async(nil, nil, alert.id)
     end
 
+    def send_alert_email
+      notification_service
+        .async
+        .prometheus_alerts_fired(project, [alert])
+    end
+
     def logger
       @logger ||= Gitlab::AppLogger
     end

app/services/notification_service.rb

@@ -601,7 +601,7 @@ class NotificationService
     return if project.emails_disabled?
 
     owners_and_maintainers_without_invites(project).to_a.product(alerts).each do |recipient, alert|
-      mailer.prometheus_alert_fired_email(project.id, recipient.user.id, alert).deliver_later
+      mailer.prometheus_alert_fired_email(project, recipient.user, alert).deliver_later
     end
   end
 

app/services/projects/alerting/notify_service.rb

@@ -73,7 +73,7 @@ module Projects
       end
 
       def process_incident_issues
-        return if alert.issue
+        return if alert.issue || alert.resolved?
 
         ::IncidentManagement::ProcessAlertWorker.perform_async(nil, nil, alert.id)
       end
@@ -81,7 +81,7 @@ module Projects
       def send_alert_email
         notification_service
           .async
-          .prometheus_alerts_fired(project, [alert.attributes])
+          .prometheus_alerts_fired(project, [alert])
       end
 
       def alert

app/services/projects/prometheus/alerts/notify_service.rb

@@ -23,7 +23,6 @@ module Projects
           return unauthorized unless valid_alert_manager_token?(token)
 
           process_prometheus_alerts
-          send_alert_email if send_email?
 
           ServiceResponse.success
         end
@@ -120,14 +119,6 @@ module Projects
           ActiveSupport::SecurityUtils.secure_compare(expected, actual)
         end
 
-        def send_alert_email
-          return unless firings.any?
-
-          notification_service
-            .async
-            .prometheus_alerts_fired(project, alerts_attributes)
-        end
-
         def process_prometheus_alerts
           alerts.each do |alert|
             AlertManagement::ProcessPrometheusAlertService
@@ -136,18 +127,6 @@ module Projects
           end
         end
 
-        def alerts_attributes
-          firings.map do |payload|
-            alert_params = Gitlab::AlertManagement::Payload.parse(
-              project,
-              payload,
-              monitoring_tool: Gitlab::AlertManagement::Payload::MONITORING_TOOLS[:prometheus]
-            ).alert_params
-
-            AlertManagement::Alert.new(alert_params).attributes
-          end
-        end
-
         def bad_request
           ServiceResponse.error(message: 'Bad Request', http_status: :bad_request)
         end

app/views/notify/prometheus_alert_fired_email.html.haml

+- body = @alert.resolved? ? _('An alert has been resolved in %{project_path}.') : _('An alert has been triggered in %{project_path}.')
+
+%p
+  = body % { project_path: @alert.project.full_path }
 %p
-  = _('An alert has been triggered in %{project_path}.') % { project_path: @alert.project.full_path }
+  = link_to(_('View alert details.'), @alert.details_url)
 
 - if description = @alert.description
   %p

app/views/notify/prometheus_alert_fired_email.text.erb

-<%= _('An alert has been triggered in %{project_path}.') % { project_path: @alert.project.full_path } %>.
+<% body = @alert.resolved? ? _('An alert has been resolved in %{project_path}.') : _('An alert has been triggered in %{project_path}.') %>
+
+<%= body % { project_path: @alert.project.full_path } %>
+<%= _('View alert details at') %> <%= @alert.details_url %>
 
 <% if description = @alert.description %>
 <%= _('Description:') %> <%= description %>

changelogs/unreleased/sy-swap-alert-email-api.yml

+---
+title: Improve messaging for emails from alerts
+merge_request: 43054
+author:
+type: changed

locale/gitlab.pot

@@ -2821,6 +2821,9 @@ msgstr ""
 msgid "An administrator changed the password for your GitLab account on %{link_to}."
 msgstr ""
 
+msgid "An alert has been resolved in %{project_path}."
+msgstr ""
+
 msgid "An alert has been triggered in %{project_path}."
 msgstr ""
 
@@ -29435,6 +29438,12 @@ msgstr ""
 msgid "View Documentation"
 msgstr ""
 
+msgid "View alert details at"
+msgstr ""
+
+msgid "View alert details."
+msgstr ""
+
 msgid "View all issues"
 msgstr ""
 

spec/mailers/emails/projects_spec.rb

@@ -32,19 +32,13 @@ RSpec.describe Emails::Projects do
   describe '#prometheus_alert_fired_email' do
     let(:default_title) { Gitlab::AlertManagement::Payload::Generic::DEFAULT_TITLE }
     let(:payload) { { 'startsAt' => Time.now.rfc3339 } }
-    let(:alert_attributes) { build(:alert_management_alert, :from_payload, payload: payload, project: project).attributes }
+    let(:alert) { create(:alert_management_alert, :from_payload, payload: payload, project: project) }
 
     subject do
-      Notify.prometheus_alert_fired_email(project.id, user.id, alert_attributes)
+      Notify.prometheus_alert_fired_email(project, user, alert)
     end
 
-    context 'missing required attributes' do
-      let(:alert_attributes) { build(:alert_management_alert, :prometheus, :from_payload, payload: payload, project: project).attributes }
-
-      it_behaves_like 'no email'
-    end
-
-    context 'with minimum required attributes' do
+    context 'with empty payload' do
       let(:payload) { {} }
 
       it_behaves_like 'an email sent from GitLab'
@@ -58,6 +52,7 @@ RSpec.describe Emails::Projects do
       it 'has expected content' do
         is_expected.to have_body_text('An alert has been triggered')
         is_expected.to have_body_text(project.full_path)
+        is_expected.to have_body_text(alert.details_url)
         is_expected.not_to have_body_text('Description:')
         is_expected.not_to have_body_text('Environment:')
         is_expected.not_to have_body_text('Metric:')
@@ -78,6 +73,7 @@ RSpec.describe Emails::Projects do
       it 'has expected content' do
         is_expected.to have_body_text('An alert has been triggered')
         is_expected.to have_body_text(project.full_path)
+        is_expected.to have_body_text(alert.details_url)
         is_expected.to have_body_text('Description:')
         is_expected.to have_body_text('alert description')
         is_expected.not_to have_body_text('Environment:')
@@ -101,6 +97,7 @@ RSpec.describe Emails::Projects do
       it 'has expected content' do
         is_expected.to have_body_text('An alert has been triggered')
         is_expected.to have_body_text(project.full_path)
+        is_expected.to have_body_text(alert.details_url)
         is_expected.to have_body_text('Environment:')
         is_expected.to have_body_text(environment.name)
         is_expected.not_to have_body_text('Description:')
@@ -112,7 +109,7 @@ RSpec.describe Emails::Projects do
       let_it_be(:prometheus_alert) { create(:prometheus_alert, project: project) }
       let_it_be(:environment) { prometheus_alert.environment }
 
-      let(:alert_attributes) { build(:alert_management_alert, :prometheus, :from_payload, payload: payload, project: project).attributes }
+      let(:alert) { create(:alert_management_alert, :prometheus, :from_payload, payload: payload, project: project) }
       let(:title) { "#{prometheus_alert.title} #{prometheus_alert.computed_operator} #{prometheus_alert.threshold}" }
       let(:metrics_url) { metrics_project_environment_url(project, environment) }
 
@@ -135,6 +132,7 @@ RSpec.describe Emails::Projects do
       it 'has expected content' do
         is_expected.to have_body_text('An alert has been triggered')
         is_expected.to have_body_text(project.full_path)
+        is_expected.to have_body_text(alert.details_url)
         is_expected.to have_body_text('Environment:')
         is_expected.to have_body_text(environment.name)
         is_expected.to have_body_text('Metric:')
@@ -143,5 +141,23 @@ RSpec.describe Emails::Projects do
         is_expected.not_to have_body_text('Description:')
       end
     end
+
+    context 'resolved' do
+      let_it_be(:alert) { create(:alert_management_alert, :resolved, project: project) }
+
+      it_behaves_like 'an email sent from GitLab'
+      it_behaves_like 'it should not have Gmail Actions links'
+      it_behaves_like 'a user cannot unsubscribe through footer link'
+
+      it 'has expected subject' do
+        is_expected.to have_subject("#{project.name} | Alert: #{alert.title}")
+      end
+
+      it 'has expected content' do
+        is_expected.to have_body_text('An alert has been resolved')
+        is_expected.to have_body_text(project.full_path)
+        is_expected.to have_body_text(alert.details_url)
+      end
+    end
   end
 end

spec/services/alert_management/process_prometheus_alert_service_spec.rb

@@ -11,9 +11,16 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
 
   describe '#execute' do
     let(:service) { described_class.new(project, nil, payload) }
-    let(:incident_management_setting) { double(auto_close_incident?: auto_close_incident, create_issue?: create_issue) }
     let(:auto_close_incident) { true }
     let(:create_issue) { true }
+    let(:send_email) { true }
+    let(:incident_management_setting) do
+      double(
+        auto_close_incident?: auto_close_incident,
+        create_issue?: create_issue,
+        send_email?: send_email
+      )
+    end
 
     before do
       allow(service)
@@ -55,6 +62,7 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
 
           it_behaves_like 'adds an alert management alert event'
           it_behaves_like 'processes incident issues'
+          it_behaves_like 'Alert Notification Service sends notification email'
 
           context 'existing alert is resolved' do
             let!(:alert) { create(:alert_management_alert, :resolved, project: project, fingerprint: fingerprint) }
@@ -92,28 +100,48 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
             end
           end
 
-          context 'when auto-alert creation is disabled' do
+          context 'when auto-creation of issues is disabled' do
             let(:create_issue) { false }
 
             it_behaves_like 'does not process incident issues'
           end
+
+          context 'when emails are disabled' do
+            let(:send_email) { false }
+
+            it 'does not send notification' do
+              expect(NotificationService).not_to receive(:new)
+
+              expect(subject).to be_success
+            end
+          end
         end
 
         context 'when alert does not exist' do
           context 'when alert can be created' do
             it_behaves_like 'creates an alert management alert'
+            it_behaves_like 'Alert Notification Service sends notification email'
+            it_behaves_like 'processes incident issues'
 
             it 'creates a system note corresponding to alert creation' do
               expect { subject }.to change(Note, :count).by(1)
             end
 
-            it_behaves_like 'processes incident issues'
-
             context 'when auto-alert creation is disabled' do
               let(:create_issue) { false }
 
               it_behaves_like 'does not process incident issues'
             end
+
+            context 'when emails are disabled' do
+              let(:send_email) { false }
+
+              it 'does not send notification' do
+                expect(NotificationService).not_to receive(:new)
+
+                expect(subject).to be_success
+              end
+            end
           end
 
           context 'when alert cannot be created' do
@@ -125,6 +153,9 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
               allow(service).to receive_message_chain(:alert, :errors).and_return(errors)
             end
 
+            it_behaves_like 'Alert Notification Service sends no notifications', http_status: :bad_request
+            it_behaves_like 'does not process incident issues due to error', http_status: :bad_request
+
             it 'writes a warning to the log' do
               expect(Gitlab::AppLogger).to receive(:warn).with(
                 message: 'Unable to create AlertManagement::Alert',
@@ -134,8 +165,6 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
 
               execute
             end
-
-            it_behaves_like 'does not process incident issues'
           end
 
           it { is_expected.to be_success }
@@ -148,6 +177,9 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
 
         context 'when auto_resolve_incident set to true' do
           context 'when status can be changed' do
+            it_behaves_like 'Alert Notification Service sends notification email'
+            it_behaves_like 'does not process incident issues'
+
             it 'resolves an existing alert' do
               expect { execute }.to change { alert.reload.resolved? }.to(true)
             end
@@ -185,6 +217,8 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
 
               execute
             end
+
+            it_behaves_like 'Alert Notification Service sends notification email'
           end
 
           it { is_expected.to be_success }
@@ -197,6 +231,16 @@ RSpec.describe AlertManagement::ProcessPrometheusAlertService do
             expect { execute }.not_to change { alert.reload.resolved? }
           end
         end
+
+        context 'when emails are disabled' do
+          let(:send_email) { false }
+
+          it 'does not send notification' do
+            expect(NotificationService).not_to receive(:new)
+
+            expect(subject).to be_success
+          end
+        end
       end
 
       context 'environment given' do

spec/services/notification_service_spec.rb

@@ -3100,26 +3100,26 @@ RSpec.describe NotificationService, :mailer do
   end
 
   describe '#prometheus_alerts_fired' do
-    let!(:project) { create(:project) }
-    let!(:master) { create(:user) }
-    let!(:developer) { create(:user) }
-    let(:alert_attributes) { build(:alert_management_alert, project: project).attributes }
+    let_it_be(:project) { create(:project) }
+    let_it_be(:master) { create(:user) }
+    let_it_be(:developer) { create(:user) }
+    let_it_be(:alert) { create(:alert_management_alert, project: project) }
 
     before do
       project.add_maintainer(master)
     end
 
     it 'sends the email to owners and masters' do
-      expect(Notify).to receive(:prometheus_alert_fired_email).with(project.id, master.id, alert_attributes).and_call_original
-      expect(Notify).to receive(:prometheus_alert_fired_email).with(project.id, project.owner.id, alert_attributes).and_call_original
-      expect(Notify).not_to receive(:prometheus_alert_fired_email).with(project.id, developer.id, alert_attributes)
+      expect(Notify).to receive(:prometheus_alert_fired_email).with(project, master, alert).and_call_original
+      expect(Notify).to receive(:prometheus_alert_fired_email).with(project, project.owner, alert).and_call_original
+      expect(Notify).not_to receive(:prometheus_alert_fired_email).with(project, developer, alert)
 
-      subject.prometheus_alerts_fired(project, [alert_attributes])
+      subject.prometheus_alerts_fired(project, [alert])
     end
 
     it_behaves_like 'project emails are disabled' do
       let(:notification_target)  { project }
-      let(:notification_trigger) { subject.prometheus_alerts_fired(project, [alert_attributes]) }
+      let(:notification_trigger) { subject.prometheus_alerts_fired(project, [alert]) }
 
       around do |example|
         perform_enqueued_jobs { example.run }

spec/services/projects/alerting/notify_service_spec.rb

@@ -129,6 +129,12 @@ RSpec.describe Projects::Alerting::NotifyService do
                   it { expect { subject }.to change { issue.reload.state }.from('opened').to('closed') }
                   it { expect { subject }.to change(ResourceStateEvent, :count).by(1) }
                 end
+
+                context 'with issue enabled' do
+                  let(:issue_enabled) { true }
+
+                  it_behaves_like 'does not process incident issues'
+                end
               end
             end