Merge branch 'sy-add-end-date-to-rotations' into 'master'

Add support for end time to oncall rotations

See merge request gitlab-org/gitlab!53675

changelogs/unreleased/sy-add-end-date-to-rotations.yml

+---
+title: Add suppport for an end time on on-call rotations
+merge_request: 53675
+author:
+type: added

db/migrate/20210208200914_add_ends_at_to_oncall_rotations.rb

+# frozen_string_literal: true
+
+class AddEndsAtToOncallRotations < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :incident_management_oncall_rotations, :ends_at, :datetime_with_timezone
+  end
+end

db/schema_migrations/20210208200914

+e7a0121e8e21acd356daa882d8fe83242f4db180915dd0f3c25835c6c664ce0b
\ No newline at end of file

db/structure.sql

@@ -13216,6 +13216,7 @@ CREATE TABLE incident_management_oncall_rotations (
     length_unit smallint NOT NULL,
     starts_at timestamp with time zone NOT NULL,
     name text NOT NULL,
+    ends_at timestamp with time zone,
     CONSTRAINT check_5209fb5d02 CHECK ((char_length(name) <= 200))
 );
 

doc/api/graphql/reference/index.md

@@ -2096,6 +2096,7 @@ Describes an incident management on-call rotation.
 
 | Field | Type | Description |
 | ----- | ---- | ----------- |
+| `endsAt` | Time | End date and time of the on-call rotation. |
 | `id` | IncidentManagementOncallRotationID! | ID of the on-call rotation. |
 | `length` | Int | Length of the on-call schedule, in the units specified by lengthUnit. |
 | `lengthUnit` | OncallRotationUnitEnum | Unit of the on-call rotation length. |

ee/app/graphql/mutations/incident_management/oncall_rotation/create.rb

@@ -25,6 +25,10 @@ module Mutations
                  required: true,
                  description: 'The start date and time of the on-call rotation, in the timezone of the on-call schedule.'
 
+        argument :ends_at, Types::IncidentManagement::OncallRotationDateInputType,
+                 required: false,
+                 description: 'The end date and time of the on-call rotation, in the timezone of the on-call schedule.'
+
         argument :rotation_length, Types::IncidentManagement::OncallRotationLengthInputType,
                  required: true,
                  description: 'The rotation length of the on-call rotation.'
@@ -65,18 +69,20 @@ module Mutations
         def create_service_params(schedule, participants, args)
           rotation_length = args[:rotation_length][:length]
           rotation_length_unit = args[:rotation_length][:unit]
-          starts_at = parse_start_time(schedule, args)
+          starts_at = parse_datetime(schedule, args[:starts_at])
+          ends_at = parse_datetime(schedule, args[:ends_at]) if args[:ends_at]
 
           args.slice(:name).merge(
             length: rotation_length,
             length_unit: rotation_length_unit,
             starts_at: starts_at,
+            ends_at: ends_at,
             participants: find_participants(participants)
           )
         end
 
-        def parse_start_time(schedule, args)
-          args[:starts_at].asctime.in_time_zone(schedule.timezone)
+        def parse_datetime(schedule, timestamp)
+          timestamp.asctime.in_time_zone(schedule.timezone)
         end
 
         def find_participants(user_array)

ee/app/graphql/types/incident_management/oncall_rotation_date_input_type.rb

@@ -15,15 +15,15 @@ module Types
                 required: true,
                 description: 'The time component of the date in 24hr HH:MM format.'
 
-      DATE_FORMAT = %r[\d{4}-[0123]\d-\d{2}].freeze
-      TIME_FORMAT = %r[[012]\d:\d{2}].freeze
+      DATE_FORMAT = %r[^\d{4}-[0123]\d-\d{2}$].freeze
+      TIME_FORMAT = %r[^(0\d|1\d|2[0-3]):[0-5]\d$].freeze
 
       def prepare
-        raise Gitlab::Graphql::Errors::ArgumentError, 'Date given is invalid' unless DATE_FORMAT.match(date)
-        raise Gitlab::Graphql::Errors::ArgumentError, 'Time given is invalid' unless TIME_FORMAT.match(time)
+        raise Gitlab::Graphql::Errors::ArgumentError, 'Date given is invalid' unless DATE_FORMAT.match?(date)
+        raise Gitlab::Graphql::Errors::ArgumentError, 'Time given is invalid' unless TIME_FORMAT.match?(time)
 
-        Time.parse("#{date} #{time}")
-      rescue ArgumentError, TypeError
+        DateTime.parse("#{date} #{time}")
+      rescue ArgumentError, TypeError, Date::Error
         raise Gitlab::Graphql::Errors::ArgumentError, 'Date & time is invalid'
       end
     end

ee/app/graphql/types/incident_management/oncall_rotation_type.rb

@@ -23,6 +23,11 @@ module Types
             null: true,
             description: 'Start date of the on-call rotation.'
 
+      field :ends_at,
+            Types::TimeType,
+            null: true,
+            description: 'End date and time of the on-call rotation.'
+
       field :length,
             GraphQL::INT_TYPE,
             null: true,

ee/app/models/incident_management/oncall_rotation.rb

@@ -22,8 +22,9 @@ module IncidentManagement
     validates :starts_at, presence: true
     validates :length, presence: true, numericality: true
     validates :length_unit, presence: true
+    validate :valid_ends_at, if: -> { ends_at && starts_at }
 
-    scope :started, -> { where('starts_at < ?', Time.current) }
+    scope :in_progress, -> { where('starts_at < :time AND (ends_at > :time OR ends_at IS NULL)', time: Time.current) }
     scope :except_ids, -> (ids) { where.not(id: ids) }
     scope :with_shift_generation_associations, -> do
       joins(:participants, :schedule)
@@ -42,5 +43,11 @@ module IncidentManagement
       # As length_unit is an enum, input is guaranteed to be appropriate
       length.public_send(length_unit) # rubocop:disable GitlabSecurity/PublicSend
     end
+
+    private
+
+    def valid_ends_at
+      errors.add(:ends_at, s_('must be after start')) if ends_at <= starts_at
+    end
   end
 end

ee/app/services/incident_management/oncall_rotations/create_service.rb

@@ -13,6 +13,7 @@ module IncidentManagement
       # @param params - length [Integer] The length of the rotation.
       # @param params - length_unit [String] The unit of the rotation length. (One of 'hours', days', 'weeks')
       # @param params - starts_at [DateTime] The datetime the rotation starts on.
+      # @param params - ends_at [DateTime] The datetime the rotation ends on.
       # @param params - participants [Array<hash>] An array of hashes defining participants of the on-call rotations.
       # @option opts  - participant [User] The user who is part of the rotation
       # @option opts  - color_palette [String] The color palette to assign to the on-call user, for example: "blue".

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

@@ -10,7 +10,7 @@ module IncidentManagement
       queue_namespace :cronjob
 
       def perform
-        IncidentManagement::OncallRotation.started.pluck(:id).each do |rotation_id| # rubocop: disable CodeReuse/ActiveRecord
+        IncidentManagement::OncallRotation.in_progress.pluck(:id).each do |rotation_id| # rubocop: disable CodeReuse/ActiveRecord
           IncidentManagement::OncallRotations::PersistShiftsJob.perform_async(rotation_id)
         end
       end

ee/lib/incident_management/oncall_shift_generator.rb

@@ -2,6 +2,8 @@
 
 module IncidentManagement
   class OncallShiftGenerator
+    include Gitlab::Utils::StrongMemoize
+
     # @param rotation [IncidentManagement::OncallRotation]
     def initialize(rotation)
       @rotation = rotation
@@ -14,7 +16,7 @@ module IncidentManagement
     # @return [IncidentManagement::OncallShift]
     def for_timeframe(starts_at:, ends_at:)
       starts_at = [apply_timezone(starts_at), rotation_starts_at].max
-      ends_at = apply_timezone(ends_at)
+      ends_at = limit_end_time(apply_timezone(ends_at))
 
       return [] unless starts_at < ends_at
       return [] unless rotation.participants.any?
@@ -44,6 +46,7 @@ module IncidentManagement
       timestamp = apply_timezone(timestamp)
 
       return if timestamp < rotation_starts_at
+      return if rotation_ends_at && rotation_ends_at <= timestamp
       return unless rotation.participants.any?
 
       elapsed_shift_count = elapsed_whole_shifts(timestamp)
@@ -112,7 +115,7 @@ module IncidentManagement
         rotation: rotation,
         participant: participants[participant_rank(elapsed_shift_count)],
         starts_at: shift_starts_at,
-        ends_at: shift_starts_at + shift_duration
+        ends_at: limit_end_time(shift_starts_at + shift_duration)
       )
     end
 
@@ -123,16 +126,30 @@ module IncidentManagement
       elapsed_shifts_count % participants.length
     end
 
+    def limit_end_time(expected_ends_at)
+      [expected_ends_at, rotation_ends_at].compact.min
+    end
+
     def participants
-      @participants ||= rotation.participants
+      strong_memoize(:participants) do
+        rotation.participants
+      end
     end
 
     def rotation_starts_at
-      @rotation_starts_at ||= apply_timezone(rotation.starts_at)
+      strong_memoize(:rotation_starts_at) do
+        apply_timezone(rotation.starts_at)
+      end
+    end
+
+    def rotation_ends_at
+      strong_memoize(:rotation_ends_at) do
+        apply_timezone(rotation.ends_at)
+      end
     end
 
     def apply_timezone(timestamp)
-      timestamp.in_time_zone(rotation.schedule.timezone)
+      timestamp&.in_time_zone(rotation.schedule.timezone)
     end
   end
 end

ee/spec/factories/incident_management/oncall_rotations.rb

@@ -4,7 +4,8 @@ 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.floor }
+    starts_at { Time.current.change(usec: 0) }
+    ends_at { nil }
     length { 5 }
     length_unit { :days }
 

ee/spec/graphql/mutations/incident_management/oncall_rotation/create_spec.rb

@@ -42,6 +42,28 @@ RSpec.describe Mutations::IncidentManagement::OncallRotation::Create do
             errors: be_empty
           )
         end
+
+        context 'with endsAt arg' do
+          let(:ends_at) { "2020-02-10 09:00".in_time_zone(schedule.timezone) }
+
+          before do
+            args.merge!(ends_at: ends_at)
+          end
+
+          it 'returns the on-call rotation with no errors' do
+            expect(resolve[:oncall_rotation].ends_at).to eq(ends_at)
+            expect(resolve[:errors]).to be_empty
+          end
+
+          context 'when endsAt is nil' do
+            let(:ends_at) { nil }
+
+            it 'returns the on-call rotation with no errors' do
+              expect(resolve[:oncall_rotation].ends_at).to be_nil
+              expect(resolve[:errors]).to be_empty
+            end
+          end
+        end
       end
 
       context 'when OncallRotations::CreateService responds with an error' do

ee/spec/graphql/types/incident_management/oncall_rotation_date_input_type_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Types::IncidentManagement::OncallRotationDateInputType do
+  let(:date) { '2021-02-17' }
+  let(:time) { '07:25' }
+
+  let(:input) { { date: date, time: time } }
+
+  it 'accepts date and time' do
+    expect(described_class.coerce_isolated_input(input)).to eq(DateTime.parse('2021-02-17 07:25'))
+  end
+
+  shared_examples 'invalid date format' do |date|
+    context "like #{date}" do
+      let(:date) { date }
+
+      it 'raises an argument error' do
+        expect { described_class.coerce_isolated_input(input) }
+          .to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'Date given is invalid')
+      end
+    end
+  end
+
+  shared_examples 'invalid time format' do |time|
+    context "like #{time}" do
+      let(:time) { time }
+
+      it 'raises an argument error' do
+        expect { described_class.coerce_isolated_input(input) }
+          .to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'Time given is invalid')
+      end
+    end
+  end
+
+  shared_examples 'invalid parsed datetime' do |date|
+    context "like #{date}" do
+      let(:date) { date }
+
+      it 'raises an argument error' do
+        expect { described_class.coerce_isolated_input(input) }
+          .to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'Date & time is invalid')
+      end
+    end
+  end
+
+  it_behaves_like 'invalid date format', 'YYYY-MM-DD'
+  it_behaves_like 'invalid date format', '20000-12-03'
+  it_behaves_like 'invalid date format', '19231202'
+  it_behaves_like 'invalid date format', '1923-2-02'
+  it_behaves_like 'invalid date format', '1923-02-2'
+
+  it_behaves_like 'invalid time format', '99:99'
+  it_behaves_like 'invalid time format', '23:60'
+  it_behaves_like 'invalid time format', '24:59'
+  it_behaves_like 'invalid time format', '123:00'
+  it_behaves_like 'invalid time format', '00:99'
+  it_behaves_like 'invalid time format', '00:000'
+  it_behaves_like 'invalid time format', '0725'
+
+  it_behaves_like 'invalid parsed datetime', '1923-39-02'
+  it_behaves_like 'invalid parsed datetime', '2021-02-30'
+end

ee/spec/graphql/types/incident_management/oncall_rotation_type_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe GitlabSchema.types['IncidentManagementOncallRotation'] do
       id
       name
       starts_at
+      ends_at
       length
       length_unit
       participants

ee/spec/lib/incident_management/oncall_shift_generator_spec.rb

@@ -142,6 +142,49 @@ RSpec.describe IncidentManagement::OncallShiftGenerator do
            [:participant3, '2021-01-02 00:00:00 UTC', '2021-01-07 00:00:00 UTC'],
            [:participant1, '2021-01-07 00:00:00 UTC', '2021-01-12 00:00:00 UTC']]
       end
+
+      context 'with rotation end time' do
+        let(:equal_to) { rotation_end_time }
+        let(:less_than) { 10.minutes.before(rotation_end_time) }
+        let(:greater_than) { 10.minutes.after(rotation_end_time) }
+        let(:well_past) { shift_length.after(rotation_end_time) }
+
+        before do
+          rotation.update!(ends_at: rotation_end_time)
+        end
+
+        context 'when the rotation end time coincides with a shift end' do
+          let(:rotation_end_time) { rotation_start_time + (shift_length * 3) }
+
+          [:equal_to, :less_than, :greater_than, :well_past].each do |scenario|
+            context "when end time is #{scenario} the rotation end time" do
+              let(:ends_at) { send(scenario) }
+
+              it_behaves_like 'unsaved shifts',
+                '3 shifts of 5 days which ends at the rotation end time',
+                [[:participant1, '2020-12-08 00:00:00 UTC', '2020-12-13 00:00:00 UTC'],
+                 [:participant2, '2020-12-13 00:00:00 UTC', '2020-12-18 00:00:00 UTC'],
+                 [:participant3, '2020-12-18 00:00:00 UTC', '2020-12-23 00:00:00 UTC']]
+            end
+          end
+        end
+
+        context 'when the rotation end time is partway through a shift' do
+          let(:rotation_end_time) { rotation_start_time + (shift_length * 2.5) }
+
+          [:equal_to, :less_than, :greater_than, :well_past].each do |scenario|
+            context "when end time is #{scenario} the rotation end time" do
+              let(:ends_at) { send(scenario) }
+
+              it_behaves_like 'unsaved shifts',
+                '2 shifts of 5 days and one partial shift which ends at the rotation end time',
+                [[:participant1, '2020-12-08 00:00:00 UTC', '2020-12-13 00:00:00 UTC'],
+                 [:participant2, '2020-12-13 00:00:00 UTC', '2020-12-18 00:00:00 UTC'],
+                 [:participant3, '2020-12-18 00:00:00 UTC', '2020-12-20 12:00:00 UTC']]
+            end
+          end
+        end
+      end
     end
 
     context 'in timezones with daylight-savings' do
@@ -697,6 +740,34 @@ RSpec.describe IncidentManagement::OncallShiftGenerator do
           'the shift during which the timestamp occurs',
           [:participant2, '2020-12-13 00:00:00 UTC', '2020-12-18 00:00:00 UTC']
       end
+
+      context 'with rotation end time' do
+        let(:rotation_end_time) { rotation_start_time + (shift_length * 2.5) }
+
+        before do
+          rotation.update!(ends_at: rotation_end_time)
+        end
+
+        context 'when timestamp matches rotation end time' do
+          let(:timestamp) { rotation_end_time }
+
+          it { is_expected.to be_nil }
+        end
+
+        context 'when timestamp is before rotation end time' do
+          let(:timestamp) { 10.minutes.before(rotation_end_time) }
+
+          it_behaves_like 'unsaved shift',
+            'the shift during which the timestamp occurs',
+            [:participant3, '2020-12-18 00:00:00 UTC', '2020-12-20 12:00:00 UTC']
+        end
+
+        context 'when timestamp is at rotation end time' do
+          let(:timestamp) { 10.minutes.after(rotation_end_time) }
+
+          it { is_expected.to be_nil }
+        end
+      end
     end
   end
 end

ee/spec/models/incident_management/oncall_rotation_spec.rb

@@ -33,16 +33,36 @@ RSpec.describe IncidentManagement::OncallRotation do
         expect(subject.errors.full_messages.to_sentence).to eq('Name has already been taken')
       end
     end
+
+    context 'with ends_at' do
+      let(:starts_at) { Time.current }
+      let(:ends_at) { 5.days.from_now }
+
+      subject { build(:incident_management_oncall_rotation, schedule: schedule, starts_at: starts_at, ends_at: ends_at) }
+
+      it { is_expected.to be_valid }
+
+      context 'with ends_at before starts_at' do
+        let(:ends_at) { 5.days.ago }
+
+        it 'has validation errors' do
+          expect(subject).to be_invalid
+          expect(subject.errors.full_messages.to_sentence).to eq('Ends at must be after start')
+        end
+      end
+    end
   end
 
   describe 'scopes' do
-    describe '.started' do
-      subject { described_class.started }
+    describe '.in_progress' do
+      subject { described_class.in_progress }
 
       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) }
+      let_it_be(:rotation_2) { create(:incident_management_oncall_rotation, schedule: schedule, ends_at: nil) }
+      let_it_be(:rotation_3) { create(:incident_management_oncall_rotation, schedule: schedule, starts_at: 1.week.from_now) }
+      let_it_be(:rotation_4) { create(:incident_management_oncall_rotation, schedule: schedule, starts_at: 1.week.ago, ends_at: 6.days.ago) }
 
-      it { is_expected.to contain_exactly(rotation_1) }
+      it { is_expected.to contain_exactly(rotation_1, rotation_2) }
     end
   end
 

ee/spec/services/incident_management/oncall_rotations/create_service_spec.rb

@@ -8,6 +8,7 @@ RSpec.describe IncidentManagement::OncallRotations::CreateService do
   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_it_be(:starts_at) { Time.current.change(usec: 0) }
 
   let(:participants) do
     [
@@ -19,7 +20,7 @@ RSpec.describe IncidentManagement::OncallRotations::CreateService do
     ]
   end
 
-  let(:params) { { name: 'On-call rotation', starts_at: Time.current, length: '1', length_unit: 'days' }.merge(participants: participants) }
+  let(:params) { { name: 'On-call rotation', starts_at: starts_at, ends_at: 1.month.after(starts_at), length: '1', length_unit: 'days' }.merge(participants: participants) }
   let(:service) { described_class.new(schedule, project, current_user, params) }
 
   before_all do
@@ -127,6 +128,8 @@ RSpec.describe IncidentManagement::OncallRotations::CreateService do
         oncall_rotation = execute.payload[:oncall_rotation]
         expect(oncall_rotation).to be_a(::IncidentManagement::OncallRotation)
         expect(oncall_rotation.name).to eq('On-call rotation')
+        expect(oncall_rotation.starts_at).to eq(starts_at)
+        expect(oncall_rotation.ends_at).to eq(1.month.after(starts_at))
         expect(oncall_rotation.length).to eq(1)
         expect(oncall_rotation.length_unit).to eq('days')
 

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

@@ -7,9 +7,10 @@ RSpec.describe IncidentManagement::OncallRotations::PersistAllRotationsShiftsJob
 
   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) }
+  let_it_be(:rotation) { create(:incident_management_oncall_rotation, schedule: schedule) }
+  let_it_be(:rotation_2) { create(:incident_management_oncall_rotation, schedule: schedule) }
+  let_it_be(:not_started_rotation) { create(:incident_management_oncall_rotation, schedule: schedule, starts_at: 1.day.from_now) }
+  let_it_be(:ended_rotation) { create(:incident_management_oncall_rotation, schedule: schedule, starts_at: 5.days.ago, ends_at: 1.day.ago) }
 
   describe '.perform' do
     subject(:perform) { worker.perform }
@@ -18,6 +19,7 @@ RSpec.describe IncidentManagement::OncallRotations::PersistAllRotationsShiftsJob
       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)
+      expect(::IncidentManagement::OncallRotations::PersistShiftsJob).not_to receive(:perform_async).with(ended_rotation.id)
 
       perform
     end

locale/gitlab.pot

@@ -35619,6 +35619,9 @@ msgstr ""
 msgid "must be a valid IPv4 or IPv6 address"
 msgstr ""
 
+msgid "must be after start"
+msgstr ""
+
 msgid "must be greater than start date"
 msgstr ""