Merge branch '262860-persist-shift-job' into 'master'

Oncall-Rotations: Persist shifts job

See merge request gitlab-org/gitlab!50239

changelogs/unreleased/262860-persist-shift-job.yml

+---
+title: Add job to persist On-call shifts
+merge_request: 50239
+author:
+type: added

config/initializers/1_settings.rb

@@ -600,6 +600,9 @@ Gitlab.ee do
   Settings.cron_jobs['incident_sla_exceeded_check_worker'] ||= Settingslogic.new({})
   Settings.cron_jobs['incident_sla_exceeded_check_worker']['cron'] ||= '*/2 * * * *'
   Settings.cron_jobs['incident_sla_exceeded_check_worker']['job_class'] = 'IncidentManagement::IncidentSlaExceededCheckWorker'
+  Settings.cron_jobs['incident_management_persist_oncall_rotation_worker'] ||= Settingslogic.new({})
+  Settings.cron_jobs['incident_management_persist_oncall_rotation_worker']['cron'] ||= '*/5 * * * *'
+  Settings.cron_jobs['incident_management_persist_oncall_rotation_worker']['job_class'] = 'IncidentManagement::OncallRotations::PersistAllRotationsShiftsJob'
   Settings.cron_jobs['import_software_licenses_worker'] ||= Settingslogic.new({})
   Settings.cron_jobs['import_software_licenses_worker']['cron'] ||= '0 3 * * 0'
   Settings.cron_jobs['import_software_licenses_worker']['job_class'] = 'ImportSoftwareLicensesWorker'

config/sidekiq_queues.yml

@@ -168,6 +168,8 @@
   - 2
 - - incident_management_apply_incident_sla_exceeded_label
   - 1
+- - incident_management_oncall_rotations_persist_shifts_job
+  - 1
 - - invalid_gpg_signature_update
   - 2
 - - irker

ee/app/models/incident_management/oncall_rotation.rb

@@ -22,6 +22,8 @@ module IncidentManagement
     validates :length, presence: true, numericality: true
     validates :length_unit, presence: true
 
+    scope :started, -> { where('starts_at < ?', Time.current) }
+
     delegate :project, to: :schedule
 
     def shift_duration

ee/app/models/incident_management/oncall_shift.rb

@@ -2,6 +2,8 @@
 
 module IncidentManagement
   class OncallShift < ApplicationRecord
+    include BulkInsertSafe
+
     self.table_name = 'incident_management_oncall_shifts'
 
     belongs_to :rotation, class_name: 'OncallRotation', inverse_of: :shifts, foreign_key: :rotation_id
@@ -13,7 +15,10 @@ module IncidentManagement
     validates :ends_at, presence: true
     validate :timeframes_do_not_overlap, if: :rotation
 
+    scope :order_starts_at_desc, -> { order(starts_at: :desc) }
     scope :for_timeframe, -> (starts_at, ends_at) do
+      return none unless starts_at.to_i < ends_at.to_i
+
       where("tstzrange(starts_at, ends_at, '[)') && tstzrange(?, ?, '[)')", starts_at, ends_at)
     end
 

ee/app/services/incident_management/oncall_shifts/read_service.rb

@@ -15,6 +15,7 @@ module IncidentManagement
         @current_user = current_user
         @start_time = start_time
         @end_time = end_time
+        @current_time = Time.current
       end
 
       def execute
@@ -23,16 +24,42 @@ module IncidentManagement
         return error_invalid_range unless start_before_end?
         return error_excessive_range unless under_max_timeframe?
 
-        success(
-          ::IncidentManagement::OncallShiftGenerator
-          .new(rotation)
-          .for_timeframe(starts_at: start_time, ends_at: end_time)
-        )
+        persisted_shifts = find_shifts
+        generated_shifts = generate_shifts
+        shifts = combine_shifts(persisted_shifts, generated_shifts)
+
+        success(shifts)
       end
 
       private
 
-      attr_reader :rotation, :current_user, :start_time, :end_time
+      attr_reader :rotation, :current_user, :start_time, :end_time, :current_time
+
+      def find_shifts
+        rotation
+          .shifts
+          .for_timeframe(start_time, [end_time, current_time].min)
+          .order_starts_at_desc
+      end
+
+      def generate_shifts
+        ::IncidentManagement::OncallShiftGenerator
+          .new(rotation)
+          .for_timeframe(
+            starts_at: [start_time, current_time].max,
+            ends_at: end_time
+          )
+      end
+
+      def combine_shifts(persisted_shifts, generated_shifts)
+        return generated_shifts unless persisted_shifts.present?
+
+        # Remove duplicate or overlapping shifts
+        min_start_time = persisted_shifts.last.ends_at
+        generated_shifts.reject! { |shift| shift.starts_at < min_start_time }
+
+        persisted_shifts + generated_shifts
+      end
 
       def available?
         ::Gitlab::IncidentManagement.oncall_schedules_available?(rotation.project)

ee/app/workers/all_queues.yml

@@ -235,6 +235,14 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: cronjob:incident_management_oncall_rotations_persist_all_rotations_shifts_job
+  :feature_category: :incident_management
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: cronjob:ingress_modsecurity_counter_metrics
   :feature_category: :web_firewall
   :has_external_dependencies: true
@@ -773,6 +781,14 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: incident_management_oncall_rotations_persist_shifts_job
+  :feature_category: :incident_management
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: ldap_group_sync
   :feature_category: :authentication_and_authorization
   :has_external_dependencies: true

ee/app/workers/incident_management/oncall_rotations/persist_all_rotations_shifts_job.rb

+# frozen_string_literal: true
+
+module IncidentManagement
+  module OncallRotations
+    class PersistAllRotationsShiftsJob
+      include ApplicationWorker
+
+      idempotent!
+      feature_category :incident_management
+      queue_namespace :cronjob
+
+      def perform
+        IncidentManagement::OncallRotation.started.pluck(:id).each do |rotation_id| # rubocop: disable CodeReuse/ActiveRecord
+          IncidentManagement::OncallRotations::PersistShiftsJob.perform_async(rotation_id)
+        end
+      end
+    end
+  end
+end

ee/app/workers/incident_management/oncall_rotations/persist_shifts_job.rb

+# frozen_string_literal: true
+
+module IncidentManagement
+  module OncallRotations
+    class PersistShiftsJob
+      include ApplicationWorker
+
+      idempotent!
+      feature_category :incident_management
+
+      def perform(rotation_id)
+        @rotation = ::IncidentManagement::OncallRotation.find_by_id(rotation_id)
+        return unless rotation && Gitlab::IncidentManagement.oncall_schedules_available?(rotation.project)
+
+        generated_shifts = generate_shifts
+        return unless generated_shifts.present?
+
+        IncidentManagement::OncallShift.bulk_insert!(generated_shifts)
+      end
+
+      private
+
+      attr_reader :rotation
+
+      def generate_shifts
+        ::IncidentManagement::OncallShiftGenerator
+          .new(rotation)
+          .for_timeframe(
+            starts_at: shift_generation_start_time,
+            ends_at: Time.current
+          )
+      end
+
+      # To avoid generating shifts in the past, which could lead to unnecessary processing,
+      # we get the latest of rotation created time, rotation start time,
+      # or the most recent shift.
+      def shift_generation_start_time
+        [
+          rotation.created_at,
+          rotation.starts_at,
+          rotation.shifts.order_starts_at_desc.first&.ends_at
+        ].compact.max
+      end
+    end
+  end
+end

ee/spec/factories/incident_management/oncall_rotations.rb

@@ -4,7 +4,7 @@ FactoryBot.define do
   factory :incident_management_oncall_rotation, class: 'IncidentManagement::OncallRotation' do
     association :schedule, factory: :incident_management_oncall_schedule
     sequence(:name) { |n| "On-call Rotation ##{n}" }
-    starts_at { Time.current }
+    starts_at { Time.current.floor }
     length { 5 }
     length_unit { :days }
 

ee/spec/models/incident_management/oncall_rotation_spec.rb

@@ -35,6 +35,17 @@ RSpec.describe IncidentManagement::OncallRotation do
     end
   end
 
+  describe 'scopes' do
+    describe '.started' do
+      subject { described_class.started }
+
+      let_it_be(:rotation_1) { create(:incident_management_oncall_rotation, schedule: schedule) }
+      let_it_be(:rotation_2) { create(:incident_management_oncall_rotation, schedule: schedule, starts_at: 1.week.from_now) }
+
+      it { is_expected.to contain_exactly(rotation_1) }
+    end
+  end
+
   describe '#shift_duration' do
     let_it_be(:rotation) { create(:incident_management_oncall_rotation, schedule: schedule, length: 5, length_unit: :days) }
 

ee/spec/models/incident_management/oncall_shift_spec.rb

@@ -91,6 +91,21 @@ RSpec.describe IncidentManagement::OncallShift do
         # tue_to_wed - Ends as timeframe starts
         # sat_to_sun - Starts as timeframe ends
       end
+
+      context 'for invalid timeframe' do
+        subject { described_class.for_timeframe(saturday, friday) }
+
+        it { is_expected.to eq described_class.none }
+      end
+    end
+
+    describe '.order_starts_at_desc' do
+      subject { described_class.order_starts_at_desc }
+
+      let_it_be(:shift1) { create_shift(Time.current, Time.current + 1.hour, participant) }
+      let_it_be(:shift2) { create_shift(Time.current + 2.hours, Time.current + 3.hours, participant) }
+
+      it { is_expected.to eq [shift2, shift1]}
     end
   end
 

ee/spec/services/incident_management/oncall_shifts/read_service_spec.rb

@@ -3,14 +3,23 @@
 require 'spec_helper'
 
 RSpec.describe ::IncidentManagement::OncallShifts::ReadService do
-  let_it_be_with_refind(:rotation) { create(:incident_management_oncall_rotation) }
-  let_it_be(:participant) { create(:incident_management_oncall_participant, :with_developer_access, rotation: rotation) }
-  let_it_be(:project) { rotation.project }
   let_it_be(:user_with_permissions) { create(:user) }
   let_it_be(:user_without_permissions) { create(:user) }
   let_it_be(:current_user) { user_with_permissions }
 
-  let(:params) { { start_time: 15.minutes.since(rotation.starts_at), end_time: 3.weeks.since(rotation.starts_at) } }
+  let_it_be_with_refind(:rotation) { create(:incident_management_oncall_rotation, length: 1, length_unit: :days) }
+  let_it_be(:participant) { create(:incident_management_oncall_participant, :with_developer_access, rotation: rotation) }
+  let_it_be(:project) { rotation.project }
+
+  let_it_be(:persisted_first_shift) { create(:incident_management_oncall_shift, participant: participant) }
+  let_it_be(:first_shift) { build(:incident_management_oncall_shift, participant: participant) }
+  let_it_be(:second_shift) { build(:incident_management_oncall_shift, participant: participant, starts_at: first_shift.ends_at) }
+  let_it_be(:third_shift) { build(:incident_management_oncall_shift, participant: participant, starts_at: second_shift.ends_at) }
+
+  let(:start_time) { rotation.starts_at }
+  let(:end_time) { 3.days.after(start_time) }
+  let(:params) { { start_time: start_time, end_time: end_time } }
+
   let(:service) { described_class.new(rotation, current_user, **params) }
 
   before_all do
@@ -29,6 +38,18 @@ RSpec.describe ::IncidentManagement::OncallShifts::ReadService do
       end
     end
 
+    shared_examples 'returns expected shifts' do
+      it 'successfully returns a sorted collection of IncidentManagement::OncallShifts' do
+        expect(execute).to be_success
+
+        shifts = execute.payload[:shifts]
+
+        expect(shifts).to all(be_a(::IncidentManagement::OncallShift))
+        expect(shifts.sort_by(&:starts_at)).to eq(shifts)
+        expect(shifts.map(&:attributes)).to eq(expected_shifts.map(&:attributes))
+      end
+    end
+
     subject(:execute) { service.execute }
 
     context 'when the current_user is anonymous' do
@@ -43,18 +64,6 @@ RSpec.describe ::IncidentManagement::OncallShifts::ReadService do
       it_behaves_like 'error response', 'You have insufficient permissions to view shifts for this rotation'
     end
 
-    context 'when the start time is after the end time' do
-      let(:params) { { start_time: rotation.starts_at, end_time: rotation.starts_at - 1.day } }
-
-      it_behaves_like 'error response', '`start_time` should precede `end_time`'
-    end
-
-    context 'when timeframe exceeds one month' do
-      let(:params) { { start_time: rotation.starts_at, end_time: rotation.starts_at + 1.month + 1.day } }
-
-      it_behaves_like 'error response', '`end_time` should not exceed one month after `start_time`'
-    end
-
     context 'when feature is not available' do
       before do
         stub_licensed_features(oncall_schedules: false)
@@ -71,23 +80,49 @@ RSpec.describe ::IncidentManagement::OncallShifts::ReadService do
       it_behaves_like 'error response', 'Your license does not support on-call rotations'
     end
 
-    context 'with valid params' do
-      it 'successfully returns a sorted collection of IncidentManagement::OncallShifts' do
-        expect(execute).to be_success
+    context 'when the start time is after the end time' do
+      let(:end_time) { 1.day.before(start_time) }
 
-        shifts = execute.payload[:shifts]
+      it_behaves_like 'error response', '`start_time` should precede `end_time`'
+    end
 
-        expect(shifts).to all(be_a(::IncidentManagement::OncallShift))
-        expect(shifts).to all(be_valid)
-        expect(shifts.sort_by(&:starts_at)).to eq(shifts)
-        expect(shifts.first.starts_at).to be <= params[:start_time]
-        expect(shifts.last.ends_at).to be >= params[:end_time]
+    context 'when timeframe exceeds one month' do
+      let(:end_time) { 2.months.after(start_time) }
+
+      it_behaves_like 'error response', '`end_time` should not exceed one month after `start_time`'
+    end
+
+    context 'when timeframe is exactly 1 month' do
+      let(:start_time) { rotation.starts_at.beginning_of_day }
+      let(:end_time) { 1.month.after(start_time).end_of_day }
+
+      it { is_expected.to be_success }
+    end
+
+    context 'with time frozen' do
+      around do |example|
+        travel_to(current_time) { example.run }
+      end
+
+      context 'when timeframe spans the current time' do
+        let(:current_time) { 5.minutes.after(start_time) }
+        let(:expected_shifts) { [persisted_first_shift, second_shift, third_shift] }
+
+        include_examples 'returns expected shifts'
+      end
+
+      context 'when timeframe is entirely in the past' do
+        let(:current_time) { 5.minutes.after(end_time) }
+        let(:expected_shifts) { [persisted_first_shift] }
+
+        include_examples 'returns expected shifts'
       end
 
-      context 'when timeframe is exactly 1 month' do
-        let(:params) { { start_time: rotation.starts_at.beginning_of_day, end_time: (rotation.starts_at + 1.month).end_of_day } }
+      context 'when timeframe is entirely in the future' do
+        let(:current_time) { 5.minutes.before(start_time) }
+        let(:expected_shifts) { [first_shift, second_shift, third_shift] }
 
-        it { is_expected.to be_success }
+        include_examples 'returns expected shifts'
       end
     end
   end

ee/spec/workers/incident_management/oncall_rotations/persist_all_rotations_shifts_job_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe IncidentManagement::OncallRotations::PersistAllRotationsShiftsJob do
+  let(:worker) { described_class.new }
+
+  let_it_be(:schedule) { create(:incident_management_oncall_schedule) }
+
+  let_it_be(:rotation) { create(:incident_management_oncall_rotation, :with_participant, schedule: schedule) }
+  let_it_be(:rotation_2) { create(:incident_management_oncall_rotation, :with_participant, schedule: schedule) }
+  let_it_be(:not_started_rotation) { create(:incident_management_oncall_rotation, :with_participant, schedule: schedule, starts_at: 1.day.from_now) }
+
+  describe '.perform' do
+    subject(:perform) { worker.perform }
+
+    it 'creates a PersistOncallShiftsJob for each started rotation' do
+      expect(::IncidentManagement::OncallRotations::PersistShiftsJob).to receive(:perform_async).with(rotation.id)
+      expect(::IncidentManagement::OncallRotations::PersistShiftsJob).to receive(:perform_async).with(rotation_2.id)
+      expect(::IncidentManagement::OncallRotations::PersistShiftsJob).not_to receive(:perform_async).with(not_started_rotation.id)
+
+      perform
+    end
+  end
+end

ee/spec/workers/incident_management/oncall_rotations/persist_shifts_job_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe IncidentManagement::OncallRotations::PersistShiftsJob do
+  let(:worker) { described_class.new }
+  let(:rotation_id) { rotation.id }
+
+  before do
+    stub_licensed_features(oncall_schedules: true)
+  end
+
+  describe '#perform' do
+    subject(:perform) { worker.perform(rotation_id) }
+
+    context 'unknown rotation' do
+      let(:rotation_id) { non_existing_record_id }
+
+      it { is_expected.to be_nil }
+
+      it 'does not create shifts' do
+        expect { perform }.not_to change { IncidentManagement::OncallShift.count }
+      end
+    end
+
+    context 'when rotation has no saved shifts' do
+      context 'and rotation was created before it "started"' do
+        let_it_be(:rotation) { create(:incident_management_oncall_rotation, :with_participant, created_at: 1.day.ago) }
+
+        it 'creates shift' do
+          expect { perform }.to change { rotation.shifts.count }.by(1)
+          expect(rotation.shifts.first.starts_at).to eq(rotation.starts_at)
+        end
+      end
+
+      context 'and rotation "started" before it was created' do
+        let_it_be(:rotation) { create(:incident_management_oncall_rotation, :with_participant, starts_at: 1.month.ago) }
+
+        it 'creates shift without backfilling' do
+          expect { perform }.to change { rotation.shifts.count }.by(1)
+
+          first_shift = rotation.shifts.first
+
+          expect(first_shift.starts_at).to be > rotation.starts_at
+          expect(rotation.created_at).to be_between(first_shift.starts_at, first_shift.ends_at)
+        end
+      end
+    end
+
+    context 'when rotation has saved shifts' do
+      let_it_be(:existing_shift) { create(:incident_management_oncall_shift) }
+      let_it_be(:rotation) { existing_shift.rotation }
+
+      context 'when current time is during a saved shift' do
+        it 'does not create shifts' do
+          expect { perform }.not_to change { IncidentManagement::OncallShift.count }
+        end
+      end
+
+      context 'when current time is not during a saved shift' do
+        around do |example|
+          travel_to(5.minutes.after(existing_shift.ends_at)) { example.run }
+        end
+
+        it 'creates shift' do
+          expect { perform }.to change { rotation.shifts.count }.by(1)
+          expect(rotation.shifts.first).to eq(existing_shift)
+          expect(rotation.shifts.second.starts_at).to eq(existing_shift.ends_at)
+        end
+      end
+
+      # Unexpected case. If the job is delayed, we'll still
+      # fill in the correct shift history.
+      context 'when current time is several shifts after the last saved shift' do
+        around do |example|
+          travel_to(existing_shift.ends_at + (3 * rotation.shift_duration)) { example.run }
+        end
+
+        context 'when feature flag is not enabled' do
+          before do
+            stub_feature_flags(oncall_schedules_mvc: false)
+          end
+
+          it 'does not create shifts' do
+            expect { perform }.not_to change { IncidentManagement::OncallShift.count }
+          end
+        end
+
+        it 'creates multiple shifts' do
+          expect { perform }.to change { rotation.shifts.count }.by(3)
+
+          first_shift,
+          second_shift,
+          third_shift,
+          fourth_shift = rotation.shifts.order(:starts_at)
+
+          expect(rotation.shifts.length).to eq(4)
+          expect(first_shift).to eq(existing_shift)
+          expect(second_shift.starts_at).to eq(existing_shift.ends_at)
+          expect(third_shift.starts_at).to eq(existing_shift.ends_at + rotation.shift_duration)
+          expect(fourth_shift.starts_at).to eq(existing_shift.ends_at + (2 * rotation.shift_duration))
+        end
+      end
+    end
+  end
+end