Merge branch 'ck3g-extract-create-incident-management-label' into 'master'

Extract `IncidentManagement::CreateIncidentLabelService` from `IncidentManagement::CreateIssueService`

Closes #222310

See merge request gitlab-org/gitlab!35050

app/presenters/projects/prometheus/alert_presenter.rb

@@ -6,7 +6,7 @@ module Projects
       RESERVED_ANNOTATIONS = %w(gitlab_incident_markdown gitlab_y_label title).freeze
       GENERIC_ALERT_SUMMARY_ANNOTATIONS = %w(monitoring_tool service hosts).freeze
       MARKDOWN_LINE_BREAK = "  \n".freeze
-      INCIDENT_LABEL_NAME = IncidentManagement::CreateIssueService::INCIDENT_LABEL[:title].freeze
+      INCIDENT_LABEL_NAME = IncidentManagement::CreateIncidentLabelService::LABEL_PROPERTIES[:title].freeze
       METRIC_TIME_WINDOW = 30.minutes
 
       def full_title

app/services/alert_management/create_alert_issue_service.rb

@@ -4,8 +4,6 @@ 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)
@@ -18,17 +16,17 @@ module AlertManagement
       return error_issue_already_exists if alert.issue
 
       result = create_issue
-      @issue = result.payload[:issue]
+      issue = result.payload[:issue]
 
-      return error(result.message) if result.error?
-      return error(alert.errors.full_messages.to_sentence) unless update_alert_issue_id
+      return error(result.message, issue) if result.error?
+      return error(object_errors(alert), issue) unless associate_alert_with_issue(issue)
 
-      success
+      result
     end
 
     private
 
-    attr_reader :alert, :user, :issue
+    attr_reader :alert, :user
 
     delegate :project, to: :alert
 
@@ -37,31 +35,35 @@ module AlertManagement
     end
 
     def create_issue
-      issue = do_create_issue(label_ids: issue_label_ids)
+      label_result = find_or_create_incident_label
 
-      # Create an unlabelled issue if we couldn't create the issue
-      # due to labels errors.
+      # Create an unlabelled issue if we couldn't create the label
+      # due to a race condition.
       # See https://gitlab.com/gitlab-org/gitlab-foss/issues/65042
-      if issue.errors.include?(:labels)
-        log_label_error(issue)
-        issue = do_create_issue
-      end
+      extra_params = label_result.success? ? { label_ids: [label_result.payload[:label].id] } : {}
+
+      issue = Issues::CreateService.new(
+        project,
+        user,
+        title: alert_presenter.title,
+        description: alert_presenter.issue_description,
+        **extra_params
+      ).execute
 
-      return error(issue_errors(issue)) unless issue.valid?
+      return error(object_errors(issue), issue) unless issue.valid?
 
-      @issue = issue
-      success
+      success(issue)
     end
 
-    def update_alert_issue_id
+    def associate_alert_with_issue(issue)
       alert.update(issue_id: issue.id)
     end
 
-    def success
+    def success(issue)
       ServiceResponse.success(payload: { issue: issue })
     end
 
-    def error(message)
+    def error(message, issue = nil)
       ServiceResponse.error(payload: { issue: issue }, message: message)
     end
 
@@ -73,47 +75,18 @@ module AlertManagement
       error(_('You have no permissions'))
     end
 
-    def do_create_issue(**params)
-      Issues::CreateService.new(
-        project,
-        user,
-        title: alert_presenter.title,
-        description: alert_presenter.issue_description,
-        **params
-      ).execute
-    end
-
     def alert_presenter
       strong_memoize(:alert_presenter) do
         alert.present
       end
     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 issue_errors(issue)
-      issue.errors.full_messages.to_sentence
+    def find_or_create_incident_label
+      IncidentManagement::CreateIncidentLabelService.new(project, user).execute
     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
-      )
+    def object_errors(object)
+      object.errors.full_messages.to_sentence
     end
   end
 end

app/services/incident_management/create_incident_label_service.rb

+# frozen_string_literal: true
+
+module IncidentManagement
+  class CreateIncidentLabelService < BaseService
+    LABEL_PROPERTIES = {
+      title: 'incident',
+      color: '#CC0033',
+      description: <<~DESCRIPTION.chomp
+        Denotes a disruption to IT services and \
+        the associated issues require immediate attention
+      DESCRIPTION
+    }.freeze
+
+    def execute
+      label = Labels::FindOrCreateService
+        .new(current_user, project, **LABEL_PROPERTIES)
+        .execute
+
+      if label.invalid?
+        log_invalid_label_info(label)
+        return ServiceResponse.error(payload: { label: label }, message: full_error_message(label))
+      end
+
+      ServiceResponse.success(payload: { label: label })
+    end
+
+    private
+
+    def log_invalid_label_info(label)
+      log_info <<~TEXT.chomp
+        Cannot create incident label "#{label.title}" \
+        for "#{label.project.full_name}": #{full_error_message(label)}.
+      TEXT
+    end
+
+    def full_error_message(label)
+      label.errors.full_messages.to_sentence
+    end
+  end
+end

app/services/incident_management/create_issue_service.rb

@@ -4,15 +4,6 @@ module IncidentManagement
   class CreateIssueService < BaseService
     include Gitlab::Utils::StrongMemoize
 
-    INCIDENT_LABEL = {
-      title: 'incident',
-      color: '#CC0033',
-      description: <<~DESCRIPTION.chomp
-        Denotes a disruption to IT services and \
-        the associated issues require immediate attention
-      DESCRIPTION
-    }.freeze
-
     def initialize(project, params)
       super(project, User.alert_bot, params)
     end
@@ -30,26 +21,19 @@ module IncidentManagement
     private
 
     def create_issue
-      issue = do_create_issue(label_ids: issue_label_ids)
+      label_result = find_or_create_incident_label
 
-      # Create an unlabelled issue if we couldn't create the issue
-      # due to labels errors.
+      # Create an unlabelled issue if we couldn't create the label
+      # due to a race condition.
       # See https://gitlab.com/gitlab-org/gitlab-foss/issues/65042
-      if issue.errors.include?(:labels)
-        log_label_error(issue)
-        issue = do_create_issue
-      end
+      extra_params = label_result.success? ? { label_ids: [label_result.payload[:label].id] } : {}
 
-      issue
-    end
-
-    def do_create_issue(**params)
       Issues::CreateService.new(
         project,
         current_user,
         title: issue_title,
         description: issue_description,
-        **params
+        **extra_params
       ).execute
     end
 
@@ -67,16 +51,8 @@ module IncidentManagement
       ].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(current_user, project, **params)
-        .execute
+    def find_or_create_incident_label
+      IncidentManagement::CreateIncidentLabelService.new(project, current_user).execute
     end
 
     def alert_summary
@@ -108,15 +84,6 @@ module IncidentManagement
       issue.errors.full_messages.to_sentence
     end
 
-    def log_label_error(issue)
-      log_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
-
     def error_with(message)
       log_error(%{Cannot create incident issue for "#{project.full_name}": #{message}})
 

lib/gitlab/usage_data.rb

@@ -124,7 +124,7 @@ module Gitlab
             issues_created_manually_from_alerts: issues_created_manually_from_alerts,
             incident_issues: alert_bot_incident_count,
             alert_bot_incident_issues: alert_bot_incident_count,
-            incident_labeled_issues: count(::Issue.with_label_attributes(IncidentManagement::CreateIssueService::INCIDENT_LABEL)),
+            incident_labeled_issues: count(::Issue.with_label_attributes(IncidentManagement::CreateIncidentLabelService::LABEL_PROPERTIES)),
             keys: count(Key),
             label_lists: count(List.label),
             lfs_objects: count(LfsObject),

spec/factories/usage_data.rb

@@ -46,7 +46,7 @@ FactoryBot.define do
       create(:sentry_issue, issue: projects[0].issues[0])
 
       # Incident Labeled Issues
-      incident_label_attrs = IncidentManagement::CreateIssueService::INCIDENT_LABEL
+      incident_label_attrs = IncidentManagement::CreateIncidentLabelService::LABEL_PROPERTIES
       incident_label = create(:label, project: projects[0], **incident_label_attrs)
       create(:labeled_issue, project: projects[0], labels: [incident_label])
       incident_group = create(:group)

spec/migrations/add_incident_settings_to_all_existing_projects_spec.rb

@@ -67,7 +67,7 @@ describe AddIncidentSettingsToAllExistingProjects, :migration do
     context 'when project has incident labels' do
       before do
         issue = issues.create!(project_id: project.id)
-        incident_label_attrs = IncidentManagement::CreateIssueService::INCIDENT_LABEL
+        incident_label_attrs = IncidentManagement::CreateIncidentLabelService::LABEL_PROPERTIES
         incident_label = labels.create!(project_id: project.id, **incident_label_attrs)
         label_links.create!(target_id: issue.id, label_id: incident_label.id, target_type: 'Issue')
       end

spec/services/alert_management/create_alert_issue_service_spec.rb

@@ -66,88 +66,6 @@ RSpec.describe AlertManagement::CreateAlertIssueService do
       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
       let(:can_create) { true }
 
@@ -162,18 +80,20 @@ RSpec.describe AlertManagement::CreateAlertIssueService do
 
       context 'when the alert is prometheus alert' do
         let(:alert) { prometheus_alert }
+        let(:issue) { subject.payload[:issue] }
 
         it_behaves_like 'creating an alert issue'
         it_behaves_like 'setting an issue attributes'
-        it_behaves_like 'sets issue labels'
+        it_behaves_like 'create alert issue sets issue labels'
       end
 
       context 'when the alert is generic' do
         let(:alert) { generic_alert }
+        let(:issue) { subject.payload[:issue] }
 
         it_behaves_like 'creating an alert issue'
         it_behaves_like 'setting an issue attributes'
-        it_behaves_like 'sets issue labels'
+        it_behaves_like 'create alert issue sets issue labels'
       end
 
       context 'when issue cannot be created' do

spec/services/incident_management/create_incident_label_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe IncidentManagement::CreateIncidentLabelService do
+  let_it_be(:project) { create(:project, :private) }
+  let_it_be(:user) { User.alert_bot }
+  let(:service) { described_class.new(project, user) }
+
+  subject(:execute) { service.execute }
+
+  describe 'execute' do
+    let(:title) { described_class::LABEL_PROPERTIES[:title] }
+    let(:color) { described_class::LABEL_PROPERTIES[:color] }
+    let(:description) { described_class::LABEL_PROPERTIES[:description] }
+
+    shared_examples 'existing label' do
+      it 'returns the existing label' do
+        expect { execute }.not_to change(Label, :count)
+
+        expect(execute).to be_success
+        expect(execute.payload).to eq(label: label)
+      end
+    end
+
+    shared_examples 'new label' do
+      it 'creates a new label' do
+        expect { execute }.to change(Label, :count).by(1)
+
+        label = project.reload.labels.last
+        expect(execute).to be_success
+        expect(execute.payload).to eq(label: label)
+        expect(label.title).to eq(title)
+        expect(label.color).to eq(color)
+        expect(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
+      let(:project) { create(:project, group: group) }
+      let(:group) { create(:group) }
+
+      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 label "incident" \
+          for "#{project.full_name}": Title has already been taken.
+        MESSAGE
+
+        expect(service).to receive(:log_info).with(message)
+        expect(execute).to be_error
+        expect(execute.payload[:label]).to be_kind_of(Label)
+        expect(execute.message).to eq('Title has already been taken')
+      end
+    end
+  end
+end

spec/services/incident_management/create_issue_service_spec.rb

@@ -199,80 +199,7 @@ RSpec.describe IncidentManagement::CreateIssueService do
     end
 
     describe "label `incident`" 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 'adds the existing label' do
-          expect { subject }.not_to change(Label, :count)
-
-          expect(issue.labels).to eq([label])
-        end
-      end
-
-      shared_examples 'new label' do
-        it 'adds newly created label' do
-          expect { subject }.to change(Label, :count).by(1)
-
-          label = project.reload.labels.last
-          expect(issue.labels).to eq([label])
-          expect(label.title).to eq(title)
-          expect(label.color).to eq(color)
-          expect(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
-        let(:project) { create(:project, group: group) }
-        let(:group) { create(:group) }
-
-        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(service)
-            .to receive(:log_info)
-            .with(message)
-
-          expect(subject).to include(status: :success)
-          expect(issue.labels).to be_empty
-        end
-      end
+      it_behaves_like 'create alert issue sets issue labels'
     end
   end
 

spec/support/shared_examples/create_alert_issue_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'create alert issue sets issue labels' do
+  let(:title) { IncidentManagement::CreateIncidentLabelService::LABEL_PROPERTIES[:title] }
+  let!(:label) { create(:label, project: project, title: title) }
+  let(:label_service) { instance_double(IncidentManagement::CreateIncidentLabelService, execute: label_service_response) }
+
+  before do
+    allow(IncidentManagement::CreateIncidentLabelService).to receive(:new).with(project, user).and_return(label_service)
+  end
+
+  context 'when create incident label responds with success' do
+    let(:label_service_response) { ServiceResponse.success(payload: { label: label }) }
+
+    it 'adds label to issue' do
+      expect(issue.labels).to eq([label])
+    end
+  end
+
+  context 'when create incident label responds with error' do
+    let(:label_service_response) { ServiceResponse.error(payload: { label: label }, message: 'label error') }
+
+    it 'creates an issue without labels' do
+      expect(issue.labels).to be_empty
+    end
+  end
+end