Merge branch 'remove-create-issue-service-dependency-from-create-alert-issue-service' into 'master'

Remove create issue service dependency from create alert issue service

See merge request gitlab-org/gitlab!33969

app/services/alert_management/create_alert_issue_service.rb

@@ -2,6 +2,10 @@
 
 module AlertManagement
   class CreateAlertIssueService
+    include Gitlab::Utils::StrongMemoize
+
+    INCIDENT_LABEL = ::IncidentManagement::CreateIssueService::INCIDENT_LABEL
+
     # @param alert [AlertManagement::Alert]
     # @param user [User]
     def initialize(alert, user)
@@ -13,10 +17,10 @@ module AlertManagement
       return error_no_permissions unless allowed?
       return error_issue_already_exists if alert.issue
 
-      result = create_issue(alert, user, alert_payload)
-      @issue = result[:issue]
+      result = create_issue(user, alert_payload)
+      @issue = result.payload[:issue]
 
-      return error(result[:message]) if result[:status] == :error
+      return error(result.message) if result.error?
       return error(alert.errors.full_messages.to_sentence) unless update_alert_issue_id
 
       success
@@ -32,10 +36,21 @@ module AlertManagement
       user.can?(:create_issue, project)
     end
 
-    def create_issue(alert, user, alert_payload)
-      ::IncidentManagement::CreateIssueService
-        .new(project, alert_payload, user)
-        .execute(skip_settings_check: true)
+    def create_issue(user, alert_payload)
+      issue = do_create_issue(label_ids: issue_label_ids)
+
+      # Create an unlabelled issue if we couldn't create the issue
+      # due to labels errors.
+      # See https://gitlab.com/gitlab-org/gitlab-foss/issues/65042
+      if issue.errors.include?(:labels)
+        log_label_error(issue)
+        issue = do_create_issue
+      end
+
+      return error(issue_errors(issue)) unless issue.valid?
+
+      @issue = issue
+      success
     end
 
     def alert_payload
@@ -65,5 +80,81 @@ module AlertManagement
     def error_no_permissions
       error(_('You have no permissions'))
     end
+
+    def do_create_issue(**params)
+      Issues::CreateService.new(
+        project,
+        user,
+        title: issue_title,
+        description: issue_description,
+        **params
+      ).execute
+    end
+
+    def issue_title
+      alert_presenter.full_title
+    end
+
+    def issue_description
+      horizontal_line = "\n\n---\n\n"
+
+      [
+        alert_summary,
+        alert_markdown,
+        issue_template_content
+      ].compact.join(horizontal_line)
+    end
+
+    def issue_label_ids
+      [
+        find_or_create_label(**INCIDENT_LABEL)
+      ].compact.map(&:id)
+    end
+
+    def find_or_create_label(**params)
+      Labels::FindOrCreateService
+        .new(user, project, **params)
+        .execute
+    end
+
+    def alert_summary
+      alert_presenter.issue_summary_markdown
+    end
+
+    def alert_markdown
+      alert_presenter.alert_markdown
+    end
+
+    def alert_presenter
+      strong_memoize(:alert_presenter) do
+        Gitlab::Alerting::Alert.new(project: project, payload: alert_payload).present
+      end
+    end
+
+    def issue_template_content
+      incident_management_setting.issue_template_content
+    end
+
+    def incident_management_setting
+      strong_memoize(:incident_management_setting) do
+        project.incident_management_setting ||
+          project.build_incident_management_setting
+      end
+    end
+
+    def issue_errors(issue)
+      issue.errors.full_messages.to_sentence
+    end
+
+    def log_label_error(issue)
+      Gitlab::AppLogger.info(
+        <<~TEXT.chomp
+          Cannot create incident issue with labels \
+          #{issue.labels.map(&:title).inspect} \
+          for "#{project.full_name}": #{issue.errors.full_messages.to_sentence}.
+          Retrying without labels.
+        TEXT
+      )
+    end
   end
 end

app/services/incident_management/create_issue_service.rb

@@ -13,12 +13,12 @@ module IncidentManagement
       DESCRIPTION
     }.freeze
 
-    def initialize(project, params, user = User.alert_bot)
-      super(project, user, params)
+    def initialize(project, params)
+      super(project, User.alert_bot, params)
     end
 
-    def execute(skip_settings_check: false)
-      return error_with('setting disabled') unless skip_settings_check || incident_management_setting.create_issue?
+    def execute
+      return error_with('setting disabled') unless incident_management_setting.create_issue?
       return error_with('invalid alert') unless alert.valid?
 
       issue = create_issue

spec/services/alert_management/create_alert_issue_service_spec.rb

@@ -4,9 +4,11 @@ require 'spec_helper'
 
 RSpec.describe AlertManagement::CreateAlertIssueService do
   let_it_be(:user) { create(:user) }
-  let_it_be(:project) { create(:project) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, group: group) }
   let_it_be(:payload) do
     {
+      'title' => 'Alert title',
       'annotations' => {
         'title' => 'Alert title'
       },
@@ -15,7 +17,7 @@ RSpec.describe AlertManagement::CreateAlertIssueService do
     }
   end
   let_it_be(:generic_alert, reload: true) { create(:alert_management_alert, :triggered, project: project, payload: payload) }
-  let_it_be(:prometheus_alert) { create(:alert_management_alert, :triggered, :prometheus, project: project, payload: payload) }
+  let_it_be(:prometheus_alert, reload: true) { create(:alert_management_alert, :triggered, :prometheus, project: project, payload: payload) }
   let(:alert) { generic_alert }
   let(:created_issue) { Issue.last! }
 
@@ -29,7 +31,7 @@ RSpec.describe AlertManagement::CreateAlertIssueService do
         .and_return(can_create)
     end
 
-    shared_examples 'creating an alert' do
+    shared_examples 'creating an alert issue' do
       it 'creates an issue' do
         expect { execute }.to change { project.issues.count }.by(1)
       end
@@ -47,12 +49,106 @@ RSpec.describe AlertManagement::CreateAlertIssueService do
 
         expect(alert.reload.issue_id).to eq(created_issue.id)
       end
+    end
 
-      it 'sets issue author to the current user' do
+    shared_examples 'setting an issue attributes' do
+      before do
         execute
+      end
 
+      it 'sets issue author to the current user' do
         expect(created_issue.author).to eq(user)
       end
+
+      it 'sets the issue title' do
+        expect(created_issue.title).to eq(alert_presenter.title)
+      end
+
+      it 'sets the issue description' do
+        expect(created_issue.description).to include(alert_presenter.issue_summary_markdown.strip)
+      end
+    end
+
+    shared_examples 'sets issue labels' do
+      let(:title) { 'incident' }
+      let(:color) { '#CC0033' }
+      let(:description) do
+        <<~DESCRIPTION.chomp
+          Denotes a disruption to IT services and \
+          the associated issues require immediate attention
+        DESCRIPTION
+      end
+
+      shared_examples 'existing label' do
+        it 'does not create new label' do
+          expect { execute }.not_to change(Label, :count)
+        end
+
+        it 'adds the existing label' do
+          execute
+
+          expect(created_issue.labels).to eq([label])
+        end
+      end
+
+      shared_examples 'new label' do
+        it 'adds newly created label' do
+          expect { execute }.to change(Label, :count).by(1)
+        end
+
+        it 'sets label attributes' do
+          execute
+
+          created_label = project.reload.labels.last!
+          expect(created_issue.labels).to eq([created_label])
+          expect(created_label.title).to eq(title)
+          expect(created_label.color).to eq(color)
+          expect(created_label.description).to eq(description)
+        end
+      end
+
+      context 'with predefined project label' do
+        it_behaves_like 'existing label' do
+          let!(:label) { create(:label, project: project, title: title) }
+        end
+      end
+
+      context 'with predefined group label' do
+        it_behaves_like 'existing label' do
+          let!(:label) { create(:group_label, group: group, title: title) }
+        end
+      end
+
+      context 'without label' do
+        it_behaves_like 'new label'
+      end
+
+      context 'with duplicate labels', issue: 'https://gitlab.com/gitlab-org/gitlab-foss/issues/65042' do
+        before do
+          # Replicate race condition to create duplicates
+          build(:label, project: project, title: title).save!(validate: false)
+          build(:label, project: project, title: title).save!(validate: false)
+        end
+
+        it 'create an issue without labels' do
+          # Verify we have duplicates
+          expect(project.labels.size).to eq(2)
+          expect(project.labels.map(&:title)).to all(eq(title))
+
+          message = <<~MESSAGE.chomp
+            Cannot create incident issue with labels ["#{title}"] for \
+            "#{project.full_name}": Labels is invalid.
+            Retrying without labels.
+          MESSAGE
+
+          expect(Gitlab::AppLogger)
+            .to receive(:info)
+            .with(message)
+
+          expect(execute).to be_success
+          expect(created_issue.labels).to be_empty
+        end
+      end
     end
 
     context 'when a user is allowed to create an issue' do
@@ -69,14 +165,25 @@ RSpec.describe AlertManagement::CreateAlertIssueService do
 
       context 'when the alert is prometheus alert' do
         let(:alert) { prometheus_alert }
+        let(:alert_presenter) do
+          Gitlab::Alerting::Alert.new(project: project, payload: alert.payload).present
+        end
 
-        it_behaves_like 'creating an alert'
+        it_behaves_like 'creating an alert issue'
+        it_behaves_like 'setting an issue attributes'
+        it_behaves_like 'sets issue labels'
       end
 
       context 'when the alert is generic' do
         let(:alert) { generic_alert }
+        let(:alert_presenter) do
+          alert_payload = Gitlab::Alerting::NotificationPayloadParser.call(alert.payload.to_h)
+          Gitlab::Alerting::Alert.new(project: project, payload: alert_payload).present
+        end
 
-        it_behaves_like 'creating an alert'
+        it_behaves_like 'creating an alert issue'
+        it_behaves_like 'setting an issue attributes'
+        it_behaves_like 'sets issue labels'
       end
 
       context 'when issue cannot be created' do
@@ -89,7 +196,7 @@ RSpec.describe AlertManagement::CreateAlertIssueService do
 
         it 'has an unsuccessful status' do
           expect(execute).to be_error
-          expect(execute.message).to eq('invalid alert')
+          expect(execute.message).to eq("Title can't be blank")
         end
       end
 

spec/services/incident_management/create_issue_service_spec.rb

@@ -281,22 +281,12 @@ describe IncidentManagement::CreateIssueService do
       setting.update!(create_issue: false)
     end
 
-    context 'when skip_settings_check is false (default)' do
-      it 'returns an error' do
-        expect(service)
-          .to receive(:log_error)
-          .with(error_message('setting disabled'))
-
-        expect(subject).to eq(status: :error, message: 'setting disabled')
-      end
-    end
-
-    context 'when skip_settings_check is true' do
-      subject { service.execute(skip_settings_check: true) }
+    it 'returns an error' do
+      expect(service)
+        .to receive(:log_error)
+        .with(error_message('setting disabled'))
 
-      it 'creates an issue' do
-        expect { subject }.to change(Issue, :count).by(1)
-      end
+      expect(subject).to eq(status: :error, message: 'setting disabled')
     end
   end