Merge branch '335008-add-label-applied-to-issuable-sla' into 'master'

Add label_applied and scope to Issuable Sla

See merge request gitlab-org/gitlab!66017

app/services/issuable_base_service.rb

@@ -527,6 +527,12 @@ class IssuableBaseService < ::BaseProjectService
   def allowed_update_params(params)
     params
   end
+
+  def update_issuable_sla(issuable)
+    return unless issuable_sla = issuable.issuable_sla
+
+    issuable_sla.update(issuable_closed: issuable.closed?)
+  end
 end
 
 IssuableBaseService.prepend_mod_with('IssuableBaseService')

app/services/issues/close_service.rb

@@ -32,7 +32,7 @@ module Issues
 
         notification_service.async.close_issue(issue, current_user, { closed_via: closed_via }) if notifications
         todo_service.close_issue(issue, current_user)
-        resolve_alert(issue)
+        perform_incident_management_actions(issue)
         execute_hooks(issue, 'close')
         invalidate_cache_counts(issue, users: issue.assignees)
         issue.update_project_counter_caches
@@ -51,6 +51,10 @@ module Issues
 
     private
 
+    def perform_incident_management_actions(issue)
+      resolve_alert(issue)
+    end
+
     def close_external_issue(issue, closed_via)
       return unless project.external_issue_tracker&.support_close_issue?
 
@@ -89,3 +93,5 @@ module Issues
     end
   end
 end
+
+Issues::CloseService.prepend_mod_with('Issues::CloseService')

app/services/issues/reopen_service.rb

@@ -9,6 +9,7 @@ module Issues
         event_service.reopen_issue(issue, current_user)
         create_note(issue, 'reopened')
         notification_service.async.reopen_issue(issue, current_user)
+        perform_incident_management_actions(issue)
         execute_hooks(issue, 'reopen')
         invalidate_cache_counts(issue, users: issue.assignees)
         issue.update_project_counter_caches
@@ -21,8 +22,13 @@ module Issues
 
     private
 
+    def perform_incident_management_actions(issue)
+    end
+
     def create_note(issue, state = issue.state)
       SystemNoteService.change_status(issue, issue.project, current_user, state, nil)
     end
   end
 end
+
+Issues::ReopenService.prepend_mod_with('Issues::ReopenService')

db/migrate/20210712052519_add_label_applied_issuable_closed_to_issuable_sla.rb

+# frozen_string_literal: true
+
+class AddLabelAppliedIssuableClosedToIssuableSla < ActiveRecord::Migration[6.1]
+  def change
+    add_column :issuable_slas, :label_applied, :boolean, default: false, null: false
+    add_column :issuable_slas, :issuable_closed, :boolean, default: false, null: false
+  end
+end

db/migrate/20210714043818_add_index_for_label_applied_to_issuable_sla.rb

+# frozen_string_literal: true
+
+class AddIndexForLabelAppliedToIssuableSla < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_issuable_slas_on_due_at_id_label_applied_issuable_closed'
+
+  def up
+    add_concurrent_index :issuable_slas, [:due_at, :id], name: INDEX_NAME, where: 'label_applied = FALSE AND issuable_closed = FALSE'
+  end
+
+  def down
+    remove_concurrent_index_by_name :issuable_slas, INDEX_NAME
+  end
+end

db/post_migrate/20210722042939_update_issuable_slas_where_issue_closed.rb

+# frozen_string_literal: true
+
+class UpdateIssuableSlasWhereIssueClosed < ActiveRecord::Migration[6.1]
+  ISSUE_CLOSED_STATUS = 2
+
+  class IssuableSla < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'issuable_slas'
+
+    belongs_to :issue, class_name: 'Issue'
+  end
+
+  class Issue < ActiveRecord::Base
+    self.table_name = 'issues'
+
+    has_one :issuable_sla, class_name: 'IssuableSla'
+  end
+
+  def up
+    IssuableSla.each_batch(of: 50) do |relation|
+      relation.joins(:issue)
+              .where(issues: { state_id: ISSUE_CLOSED_STATUS } )
+              .update_all(issuable_closed: true)
+    end
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema_migrations/20210712052519

+f3959b7a6f7ac95019f2f85c6383ddd11294562e94936ef3b5704bd4de7c5910
\ No newline at end of file

db/schema_migrations/20210714043818

+344736284dc18b5f7516ec2062bef99b2444ae31720691e56b4e8687d5566b31
\ No newline at end of file

db/schema_migrations/20210722042939

+dd3b35b87c2f015895d807ede2521c9672fb41ec7a3b0b1a2f7abdc009950b6e
\ No newline at end of file

db/structure.sql

@@ -14200,7 +14200,9 @@ ALTER SEQUENCE issuable_severities_id_seq OWNED BY issuable_severities.id;
 CREATE TABLE issuable_slas (
     id bigint NOT NULL,
     issue_id bigint NOT NULL,
-    due_at timestamp with time zone NOT NULL
+    due_at timestamp with time zone NOT NULL,
+    label_applied boolean DEFAULT false NOT NULL,
+    issuable_closed boolean DEFAULT false NOT NULL
 );
 
 CREATE SEQUENCE issuable_slas_id_seq
@@ -24047,6 +24049,8 @@ CREATE INDEX index_issuable_metric_images_on_issue_id ON issuable_metric_images
 
 CREATE UNIQUE INDEX index_issuable_severities_on_issue_id ON issuable_severities USING btree (issue_id);
 
+CREATE INDEX index_issuable_slas_on_due_at_id_label_applied_issuable_closed ON issuable_slas USING btree (due_at, id) WHERE ((label_applied = false) AND (issuable_closed = false));
+
 CREATE UNIQUE INDEX index_issuable_slas_on_issue_id ON issuable_slas USING btree (issue_id);
 
 CREATE INDEX index_issue_assignees_on_user_id ON issue_assignees USING btree (user_id);

ee/app/models/issuable_sla.rb

@@ -4,5 +4,5 @@ class IssuableSla < ApplicationRecord
   belongs_to :issue, optional: false
   validates :due_at, presence: true
 
-  scope :exceeded_for_issues, -> { includes(:issue).joins(:issue).merge(Issue.opened).where('due_at < ?', Time.current) }
+  scope :exceeded, -> { where(label_applied: false, issuable_closed: false).where('due_at < ?', Time.current).order(:due_at, :id) }
 end

ee/app/services/ee/issues/close_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Issues
+    module CloseService
+      extend ::Gitlab::Utils::Override
+
+      override :perform_incident_management_actions
+      def perform_incident_management_actions(issue)
+        super
+        update_issuable_sla(issue)
+      end
+    end
+  end
+end

ee/app/services/ee/issues/reopen_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Issues
+    module ReopenService
+      extend ::Gitlab::Utils::Override
+
+      override :perform_incident_management_actions
+      def perform_incident_management_actions(issue)
+        super
+        update_issuable_sla(issue)
+      end
+    end
+  end
+end

ee/app/workers/incident_management/apply_incident_sla_exceeded_label_worker.rb

@@ -20,9 +20,13 @@ module IncidentManagement
       return unless project
 
       @label = incident_exceeded_sla_label
-      return if incident.label_ids.include?(label.id)
+      if incident.label_ids.include?(label.id)
+        set_label_applied_boolean
+        return
+      end
 
       incident.labels << label
+      set_label_applied_boolean
       add_resource_event
     end
 
@@ -30,6 +34,10 @@ module IncidentManagement
 
     attr_reader :incident, :project, :label
 
+    def set_label_applied_boolean
+      incident.issuable_sla.update(label_applied: true)
+    end
+
     def add_resource_event
       ResourceEvents::ChangeLabelsService
         .new(incident, User.alert_bot)

ee/app/workers/incident_management/incident_sla_exceeded_check_worker.rb

@@ -14,8 +14,12 @@ module IncidentManagement
     tags :exclude_from_kubernetes
 
     def perform
-      IssuableSla.exceeded_for_issues.find_each do |incident_sla|
-        ApplyIncidentSlaExceededLabelWorker.perform_async(incident_sla.issue_id)
+      iterator = Gitlab::Pagination::Keyset::Iterator.new(scope: IssuableSla.exceeded)
+
+      iterator.each_batch(of: 1000) do |records|
+        records.each do |incident_sla|
+          ApplyIncidentSlaExceededLabelWorker.perform_async(incident_sla.issue_id)
+        end
       end
     end
   end

ee/spec/factories/issuable_sla.rb

@@ -8,5 +8,13 @@ FactoryBot.define do
     trait :exceeded do
       due_at { 1.hour.ago }
     end
+
+    trait :label_applied do
+      label_applied { true }
+    end
+
+    trait :issuable_closed do
+      issuable_closed { true }
+    end
   end
 end

ee/spec/factories/issues.rb

@@ -7,6 +7,10 @@ FactoryBot.modify do
         issue.create_status_page_published_incident!
       end
     end
+
+    trait :with_sla do
+      issuable_sla
+    end
   end
 end
 

ee/spec/models/issuable_sla_spec.rb

@@ -12,33 +12,36 @@ RSpec.describe IssuableSla do
   end
 
   describe 'scopes' do
-    describe '.exceeded_for_issues' do
-      subject { described_class.exceeded_for_issues }
+    let_it_be(:project) { create(:project) }
+    let_it_be_with_reload(:issue) { create(:issue, project: project) }
+    let_it_be(:issuable_closed_sla) { create(:issuable_sla, :issuable_closed, issue: create(:issue, project: project)) }
+    let_it_be(:future_due_at_sla) { create(:issuable_sla, issue: create(:issue, project: project), due_at: 1.hour.from_now) }
 
-      let_it_be(:project) { create(:project) }
-      let_it_be_with_reload(:issue) { create(:issue, project: project) }
-      let_it_be_with_reload(:issuable_sla) { create(:issuable_sla, issue: issue, due_at: 1.hour.ago) }
+    describe '.exceeded' do
+      subject { described_class.exceeded }
 
-      context 'issue closed' do
+      let!(:issuable_sla) { create(:issuable_sla, issue: issue, due_at: 1.hour.ago) }
+
+      it { is_expected.to contain_exactly(issuable_sla) }
+
+      context 'marked as issuable closed' do
+        let!(:issuable_sla) { create(:issuable_sla, :issuable_closed, issue: issue, due_at: 1.hour.ago) }
+
+        it { is_expected.to be_empty }
+      end
+
+      context 'due_at has not passed' do
         before do
-          issue.close!
+          issuable_sla.update!(due_at: 1.hour.from_now)
         end
 
         it { is_expected.to be_empty }
       end
 
-      context 'issue opened' do
-        context 'due_at has not passed' do
-          before do
-            issuable_sla.update!(due_at: 1.hour.from_now)
-          end
-
-          it { is_expected.to be_empty }
-        end
+      context 'label applied' do
+        let!(:issuable_sla) { create(:issuable_sla, :label_applied, issue: issue, due_at: 1.hour.ago) }
 
-        context 'when due date has passed' do
-          it { is_expected.to contain_exactly(issuable_sla) }
-        end
+        it { is_expected.to be_empty }
       end
     end
   end

ee/spec/workers/incident_management/apply_incident_sla_exceeded_label_worker_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe IncidentManagement::ApplyIncidentSlaExceededLabelWorker do
   let(:worker) { described_class.new }
 
-  let_it_be_with_refind(:incident) { create(:incident) }
+  let_it_be_with_refind(:incident) { create(:incident, :with_sla) }
   let_it_be(:project) { incident.project }
   let_it_be(:label) do
     ::IncidentManagement::CreateIncidentSlaExceededLabelService
@@ -36,6 +36,10 @@ RSpec.describe IncidentManagement::ApplyIncidentSlaExceededLabelWorker do
     expect(incident.labels.reload).to include(label)
   end
 
+  it 'sets the label applied boolean' do
+    expect { perform }.to change { incident.issuable_sla.reload.label_applied }.from(false).to(true)
+  end
+
   it 'adds a note that the label was added', :aggregate_failures do
     expect { subject }.to change { incident.resource_label_events.reload.count }
 
@@ -44,6 +48,21 @@ RSpec.describe IncidentManagement::ApplyIncidentSlaExceededLabelWorker do
     expect(event.label).to eq(label)
   end
 
+  context 'label is already added' do
+    before do
+      incident.labels << label
+    end
+
+    it 'does not add a label', :aggregate_failures do
+      expect { subject }.not_to change { incident.labels.reload.count }
+      expect(incident.labels.reload).to contain_exactly(label)
+    end
+
+    it 'sets the label applied boolean' do
+      expect { perform }.to change { incident.issuable_sla.reload.label_applied }.from(false).to(true)
+    end
+  end
+
   context 'for plain issues' do
     before_all do
       incident.update!(issue_type: 'issue')

ee/spec/workers/incident_management/incident_sla_exceeded_check_worker_spec.rb

@@ -8,16 +8,13 @@ RSpec.describe IncidentManagement::IncidentSlaExceededCheckWorker do
   describe '#perform' do
     subject(:perform) { worker.perform }
 
-    let_it_be(:incident_sla) { create(:issuable_sla, :exceeded) }
-    let_it_be(:other_incident_slas) { create_list(:issuable_sla, 2, :exceeded) }
+    let_it_be(:label_applied_incident_sla) { create(:issuable_sla, :exceeded, :label_applied) }
+    let_it_be(:exceeded_incident_sla) { create(:issuable_sla, :exceeded) }
 
-    let(:label_service_stub) { instance_double(IncidentManagement::ApplyIncidentSlaExceededLabelWorker) }
-
-    it 'calls the apply incident sla label service' do
+    it 'calls the apply incident sla label service where the label is not applied already' do
       expect(IncidentManagement::ApplyIncidentSlaExceededLabelWorker)
         .to receive(:perform_async)
-        .exactly(3)
-        .times
+        .with(exceeded_incident_sla.issue_id)
 
       perform
     end

spec/migrations/20210722042939_update_issuable_slas_where_issue_closed_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!('update_issuable_slas_where_issue_closed')
+
+RSpec.describe UpdateIssuableSlasWhereIssueClosed, :migration do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:issues) { table(:issues) }
+  let(:issuable_slas) { table(:issuable_slas) }
+  let(:issue_params) { { title: 'title', project_id: project.id } }
+  let(:issue_closed_state) { 2 }
+
+  let!(:namespace) { namespaces.create!(name: 'foo', path: 'foo') }
+  let!(:project) { projects.create!(namespace_id: namespace.id) }
+  let!(:issue_open) { issues.create!(issue_params) }
+  let!(:issue_closed) { issues.create!(issue_params.merge(state_id: issue_closed_state)) }
+
+  let!(:issuable_sla_open_issue) { issuable_slas.create!(issue_id: issue_open.id, due_at: Time.now) }
+  let!(:issuable_sla_closed_issue) { issuable_slas.create!(issue_id: issue_closed.id, due_at: Time.now) }
+
+  it 'sets the issuable_closed attribute to false' do
+    expect(issuable_sla_open_issue.issuable_closed).to eq(false)
+    expect(issuable_sla_closed_issue.issuable_closed).to eq(false)
+
+    migrate!
+
+    expect(issuable_sla_open_issue.reload.issuable_closed).to eq(false)
+    expect(issuable_sla_closed_issue.reload.issuable_closed).to eq(true)
+  end
+end

spec/services/issues/close_service_spec.rb

@@ -222,7 +222,7 @@ RSpec.describe Issues::CloseService do
 
       it 'verifies the number of queries' do
         recorded = ActiveRecord::QueryRecorder.new { close_issue }
-        expected_queries = 24
+        expected_queries = 25
 
         expect(recorded.count).to be <= expected_queries
         expect(recorded.cached_count).to eq(0)