Merge branch '262862-switch-rotation-params' into 'master'

Allow rotations to gracefully switch between parameters

See merge request gitlab-org/gitlab!55570

ee/app/services/concerns/incident_management/oncall_rotations/shared_rotation_logic.rb

@@ -4,17 +4,16 @@ module IncidentManagement
   module OncallRotations
     module SharedRotationLogic
       MAXIMUM_PARTICIPANTS = 100
-      InsufficientParticipantPermissionsError = Class.new(StandardError)
 
-      # Merges existing participants with API-provided
-      # participants instead of using just the API-provided ones
-      def participants_for(oncall_rotation)
-        # Exit early if the participants that the caller
-        # wants on the rotation don't have permissions.
-        return if expected_participants_by_user.nil?
+      def save_participants!
+        participants = participants_for(oncall_rotation).each(&:validate!)
+
+        upsert_participants(participants)
+      end
 
-        # Merge the new expected attributes over the existing .
-        # participant's attributes to apply any changes
+      # Merge the new expected attributes over the existing
+      # participant's attributes to apply any changes.
+      def participants_for(oncall_rotation)
         existing_participants_by_user.merge(expected_participants_by_user) do |_, existing_participant, expected_participant|
           existing_participant.assign_attributes(expected_participant.attributes.except('id'))
           existing_participant
@@ -34,8 +33,6 @@ module IncidentManagement
 
       def expected_participants_by_user
         participants_params.to_h do |participant|
-          break unless participant[:user].can?(:read_project, project)
-
           [
             participant[:user].id,
             OncallParticipant.new(
@@ -69,12 +66,35 @@ module IncidentManagement
         participant_users != participant_users.uniq
       end
 
+      def users_without_permissions?
+        DeclarativePolicy.subject_scope do
+          participant_users.any? { |user| !user.can?(:read_project, project) }
+        end
+      end
+
       def participant_users
         @participant_users ||= participants_params.map { |participant| participant[:user] }
       end
 
-      def participant_has_no_permission
-        'A participant has insufficient permissions to access the project'
+      # Used to accurately record shift history when rotations
+      # are created or edited. Any currently running shift will
+      # be cut short and a new shift will be saved starting
+      # at the creation/update time.
+      def save_current_shift!
+        existing_shift&.update!(ends_at: oncall_rotation.updated_at)
+        new_shift&.update!(starts_at: oncall_rotation.updated_at)
+      end
+
+      def existing_shift
+        oncall_rotation.shifts.for_timestamp(oncall_rotation.updated_at).first
+      end
+
+      def new_shift
+        IncidentManagement::OncallShiftGenerator.new(oncall_rotation).for_timestamp(oncall_rotation.updated_at)
+      end
+
+      def error_participants_without_permission
+        error('A participant has insufficient permissions to access the project')
       end
 
       def error_too_many_participants

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

@@ -33,22 +33,17 @@ module IncidentManagement
         return error_no_permissions unless allowed?
         return error_too_many_participants if participants_params.size > MAXIMUM_PARTICIPANTS
         return error_duplicate_participants if duplicated_users?
+        return error_participants_without_permission if users_without_permissions?
 
         OncallRotation.transaction do
           @oncall_rotation = schedule.rotations.create!(rotation_params)
 
-          participants = participants_for(oncall_rotation)
-          raise InsufficientParticipantPermissionsError.new(participant_has_no_permission) if participants.nil?
-
-          participants.each(&:validate!)
-
-          upsert_participants(participants)
+          save_participants!
+          save_current_shift!
 
           success(oncall_rotation.reset)
         end
 
-      rescue InsufficientParticipantPermissionsError => err
-        error(err.message)
       rescue ActiveRecord::RecordInvalid => err
         error_in_validation(err.record)
       end

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

@@ -27,25 +27,25 @@ module IncidentManagement
       def execute
         return error_no_license unless available?
         return error_no_permissions unless allowed?
-        return error_too_many_participants if participants_params && participants_params.size > MAXIMUM_PARTICIPANTS
-        return error_duplicate_participants if !participants_params.nil? && duplicated_users?
+
+        if participants_params
+          return error_too_many_participants if participants_params.size > MAXIMUM_PARTICIPANTS
+          return error_duplicate_participants if duplicated_users?
+          return error_participants_without_permission if users_without_permissions?
+        end
 
         # Ensure shift history is up to date before saving new params
         IncidentManagement::OncallRotations::PersistShiftsJob.new.perform(oncall_rotation.id)
 
         OncallRotation.transaction do
-          update_and_remove_participants
-
-          # TODO Recalculate rotation with new params
-          # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55570
-
           oncall_rotation.update!(params)
 
+          save_participants!
+          save_current_shift!
+
           success(oncall_rotation.reset)
         end
 
-      rescue InsufficientParticipantPermissionsError => err
-        error(err.message)
       rescue ActiveRecord::RecordInvalid => err
         error_in_validation(err.record)
       end
@@ -54,15 +54,10 @@ module IncidentManagement
 
       attr_reader :oncall_rotation, :user, :project, :params, :participants_params
 
-      def update_and_remove_participants
+      def save_participants!
         return if participants_params.nil?
 
-        participants = participants_for(oncall_rotation)
-        raise InsufficientParticipantPermissionsError.new(participant_has_no_permission) if participants.nil?
-
-        participants.each(&:validate!)
-
-        upsert_participants(participants)
+        super
 
         oncall_rotation.touch
       end

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

@@ -186,6 +186,18 @@ RSpec.describe IncidentManagement::OncallRotations::CreateService do
           it_behaves_like 'error response', "Active period end must be later than active period start"
         end
       end
+
+      context 'for an in-progress rotation' do
+        it 'trims & saves the current shift' do
+          oncall_rotation = execute.payload[:oncall_rotation]
+
+          expect(oncall_rotation.shifts.length).to eq(1)
+          expect(oncall_rotation.shifts.first).to have_attributes(
+            starts_at: oncall_rotation.reload.created_at,
+            ends_at: oncall_rotation.starts_at.next_day
+          )
+        end
+      end
     end
   end
 end

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

@@ -7,7 +7,7 @@ RSpec.describe IncidentManagement::OncallRotations::EditService do
   let_it_be(:user_without_permissions) { create(:user) }
   let_it_be_with_refind(:project) { create(:project) }
 
-  let_it_be_with_refind(:oncall_schedule) { create(:incident_management_oncall_schedule, project: project) }
+  let_it_be_with_refind(:oncall_schedule) { create(:incident_management_oncall_schedule, :utc, project: project) }
   let_it_be_with_refind(:oncall_rotation) { create(:incident_management_oncall_rotation, :with_participants, schedule: oncall_schedule, participants_count: 2) }
   let(:current_user) { user_with_permissions }
   let(:params) { rotation_params }
@@ -43,14 +43,6 @@ RSpec.describe IncidentManagement::OncallRotations::EditService do
       it_behaves_like 'error response', 'You have insufficient permissions to edit an on-call rotation in this project'
     end
 
-    it 'runs the persist shift job before editing' do
-      expect_next_instance_of(IncidentManagement::OncallRotations::PersistShiftsJob) do |persist_job|
-        expect(persist_job).to receive(:perform).with(oncall_rotation.id)
-      end
-
-      subject
-    end
-
     context 'adding one participant' do
       let(:participant_to_add) { build(:incident_management_oncall_participant, rotation: oncall_rotation, user: user_with_permissions) }
       let(:params) { rotation_params(participants: oncall_rotation.participants.to_a.push(participant_to_add)) }
@@ -179,6 +171,79 @@ RSpec.describe IncidentManagement::OncallRotations::EditService do
         end
       end
     end
+
+    context 'for an already-started rotation' do
+      let(:active_period_shift) { { starts_at: oncall_rotation.starts_at.change(hour: 8), ends_at: oncall_rotation.starts_at.change(hour: 17) } }
+
+      around do |example|
+        travel_to(updated_at) { example.run }
+      end
+
+      context 'when the "current" shift and new "current" shift would conflict' do
+        let(:updated_at) { 8.days.after(oncall_rotation.starts_at) }
+        let(:params) { { length: 1, length_unit: 'weeks' } }
+
+        let(:previous_completed_shift) { { starts_at: oncall_rotation.starts_at, ends_at: 5.days.after(oncall_rotation.starts_at) } }
+        let(:previous_current_shift) { { starts_at: 5.days.after(oncall_rotation.starts_at), ends_at: updated_at } }
+        let(:new_current_shift) { { starts_at: updated_at, ends_at: 2.weeks.after(oncall_rotation.starts_at) } }
+
+        it 'ensures the shift history is up-to-date, ends the current shift, and starts the new shift partway' do
+          expect(execute).to be_success
+
+          first_shift, second_shift, third_shift = oncall_rotation.shifts
+          expect(oncall_rotation.shifts.length).to eq(3)
+          expect(first_shift).to have_attributes(previous_completed_shift)
+          expect(second_shift).to have_attributes(previous_current_shift)
+          expect(third_shift).to have_attributes(new_current_shift)
+        end
+      end
+
+      context 'when the next shift has not started' do
+        let(:updated_at) { 3.days.after(oncall_rotation.starts_at).change(hour: 20) }
+        let(:params) { { active_period_start: active_period_shift[:starts_at], active_period_end: active_period_shift[:ends_at] } }
+
+        let(:previous_current_shift) { { starts_at: oncall_rotation.starts_at, ends_at: updated_at } }
+
+        it 'ends the original "current" shift and does not save a new shift' do
+          expect(execute).to be_success
+
+          first_shift = oncall_rotation.shifts.first
+          expect(oncall_rotation.shifts.length).to eq(1)
+          expect(first_shift).to have_attributes(previous_current_shift)
+        end
+      end
+
+      context 'when all previous shifts have already ended' do
+        let_it_be(:starts_at) { Time.current.next_day.change(hour: 3, usec: 0) }
+        let_it_be_with_refind(:oncall_rotation) { create(:incident_management_oncall_rotation, :with_participants, :with_active_period, schedule: oncall_schedule, starts_at: starts_at) }
+
+        let(:updated_at) { starts_at.next_day }
+        let(:params) { { active_period_start: nil, active_period_end: nil } }
+
+        let(:new_current_shift) { { starts_at: updated_at, ends_at: 5.days.after(oncall_rotation.starts_at) } }
+
+        it 'starts the new "current" shift partway' do
+          expect(execute).to be_success
+
+          first_shift, second_shift = oncall_rotation.shifts
+          expect(oncall_rotation.shifts.length).to eq(2)
+          expect(first_shift).to have_attributes(active_period_shift)
+          expect(second_shift).to have_attributes(new_current_shift)
+        end
+
+        context 'when there is not a new shift' do
+          let(:params) { { ends_at: updated_at } }
+
+          it 'does not modify or save any shifts' do
+            expect(execute).to be_success
+
+            first_shift = oncall_rotation.shifts.first
+            expect(oncall_rotation.shifts.length).to eq(1)
+            expect(first_shift).to have_attributes(active_period_shift)
+          end
+        end
+      end
+    end
   end
 
   private