Add more specs + refine service

- Add with_active_period scope to Oncall rotation
- Use transaction in update

ee/app/models/incident_management/oncall_rotation.rb

@@ -50,6 +50,7 @@ module IncidentManagement
 
     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_active_period, -> { where.not(active_period_start: nil) }
     scope :with_shift_generation_associations, -> do
       joins(:active_participants, :schedule)
         .distinct

ee/app/services/incident_management/oncall_schedules/update_service.rb

@@ -8,8 +8,7 @@ module IncidentManagement
       # @param params [Hash]
       def initialize(oncall_schedule, user, params)
         @oncall_schedule = oncall_schedule
-        @original_schedule_timezone = oncall_schedule&.timezone
-        @oncall_rotations = oncall_schedule&.rotations
+        @original_schedule_timezone = oncall_schedule.timezone
         @user = user
         @params = params
         @project = oncall_schedule.project
@@ -19,47 +18,52 @@ module IncidentManagement
         return error_no_license unless available?
         return error_no_permissions unless allowed?
 
-        oncall_schedule.update!(params)
-        update_rotation_result = update_rotation_active_periods
-
-        if update_rotation_result.respond_to?(:error?) && update_rotation_result.error?
-          return error(update_rotation_result.message)
+        IncidentManagement::OncallSchedule.transaction do
+          oncall_schedule.update!(params)
+          update_rotations!
         end
 
         success(oncall_schedule)
       rescue ActiveRecord::RecordInvalid => e
         error(e.record.errors.full_messages.to_sentence)
+      rescue StandardError => e
+        error(e.message)
       end
 
       private
 
-      attr_reader :oncall_schedule, :original_schedule_timezone, :oncall_rotations, :user, :params, :project
+      attr_reader :oncall_schedule, :original_schedule_timezone, :user, :params, :project
+
+      def update_rotations!
+        return unless original_schedule_timezone != oncall_schedule.timezone
 
+        update_rotation_active_periods
+      end
+
+      # Converts & updates the active period to the new timezone
+      # Ex: 8:00 - 17:00 Europe/Berlin becomes 6:00 - 15:00 UTC
       def update_rotation_active_periods
-        oncall_schedule.rotations.select(&:has_shift_active_period?).each do |rotation|
+        original_schedule_current_time = Time.current.in_time_zone(original_schedule_timezone)
+
+        oncall_schedule.rotations.with_active_period.each do |rotation|
+          active_period = rotation.active_period.for_date(original_schedule_current_time)
+          new_start_time, new_end_time = active_period.map { |time| time.in_time_zone(oncall_schedule.timezone).strftime('%H:%M') }
+
           service = IncidentManagement::OncallRotations::EditService.new(
             rotation,
             user,
             {
-              active_period_start: new_rotation_active_period(rotation.active_period_start),
-              active_period_end: new_rotation_active_period(rotation.active_period_end)
+              active_period_start: new_start_time,
+              active_period_end: new_end_time
             }
           )
 
           response = service.execute
 
-          break(response) if response.error?
+          raise response.message if response.error?
         end
       end
 
-      def new_rotation_active_period(existing_time)
-        existing_timezone_time = Time.find_zone!(original_schedule_timezone)
-                                  .now
-                                  .change(hour: existing_time.hour, min: existing_time.min)
-
-        existing_timezone_time.in_time_zone(oncall_schedule.timezone).strftime('%H:%M')
-      end
-
       def error_no_permissions
         error(_('You have insufficient permissions to update an on-call schedule for this project'))
       end

ee/spec/models/incident_management/oncall_rotation_spec.rb

@@ -107,6 +107,18 @@ RSpec.describe IncidentManagement::OncallRotation do
 
       it { is_expected.to contain_exactly(rotation_1, rotation_2) }
     end
+
+    describe '.with_active_period' do
+      subject { described_class.with_active_period }
+
+      it { is_expected.to be_empty }
+
+      context 'rotation has active period' do
+        let(:rotation) { create(:incident_management_oncall_rotation, :with_active_period, schedule: schedule) }
+
+        it { is_expected.to contain_exactly(rotation) }
+      end
+    end
   end
 
   describe '#shift_cycle_duration' do

ee/spec/services/incident_management/oncall_schedules/update_service_spec.rb

@@ -9,7 +9,8 @@ RSpec.describe IncidentManagement::OncallSchedules::UpdateService do
   let_it_be_with_reload(:oncall_schedule) { create(:incident_management_oncall_schedule, :utc, project: project) }
 
   let(:current_user) { user_with_permissions }
-  let(:params) { { name: 'Updated name', description: 'Updated description', timezone: 'America/New_York' } }
+  let(:new_timezone) { 'America/New_York' }
+  let(:params) { { name: 'Updated name', description: 'Updated description', timezone: new_timezone } }
   let(:service) { described_class.new(oncall_schedule, current_user, params) }
 
   before do
@@ -72,19 +73,70 @@ RSpec.describe IncidentManagement::OncallSchedules::UpdateService do
       context 'schedule has a rotation' do
         # Setting fixed timezone for rotation active period updates
         around do |example|
-          freeze_time do
-            travel_to Time.utc(2021, 03, 22, 0, 0)
+          travel_to Time.utc(2021, 03, 22, 0, 0)
 
-            example.run
-          end
+          example.run
         end
 
         let_it_be_with_reload(:oncall_rotation) { create(:incident_management_oncall_rotation, :with_active_period, schedule: oncall_schedule) }
 
+        shared_examples 'updates the rotation active periods' do |new_start_time, new_end_time|
+          it 'updates the rotation active periods with new timezone' do
+            initial_start_time = oncall_rotation.reload.attributes_before_type_cast['active_period_start']
+            initial_end_time = oncall_rotation.attributes_before_type_cast['active_period_end']
+
+            expect { execute }.to change { oncall_rotation.reload.attributes_before_type_cast['active_period_start'] }.from(initial_start_time).to("#{new_start_time}:00")
+             .and change { oncall_rotation.reload.attributes_before_type_cast['active_period_end'] }.from(initial_end_time).to("#{new_end_time}:00")
+             .and change { oncall_schedule.reload.timezone }.to(new_timezone)
+          end
+        end
+
         # This expects the active periods are updated according to the date above (22nd March, 2021 in the new timezone).
-        it 'updates the rotation active periods with new timezone' do
-          expect { execute }.to change { time_from_time_column(oncall_rotation.reload.active_period_start) }.from('08:00').to('04:00')
-           .and change { time_from_time_column(oncall_rotation.active_period_end) }.from('17:00').to('13:00')
+        it_behaves_like 'updates the rotation active periods', '04:00', '13:00'
+
+        context 'from non-overnight shifts to overnight' do
+          let(:new_timezone) { 'Pacific/Auckland' }
+
+          it_behaves_like 'updates the rotation active periods', '21:00', '06:00'
+        end
+
+        context 'from overnight shifts to non-overnight' do
+          before do
+            oncall_rotation.update!(active_period_start: '21:00', active_period_end: '06:00')
+          end
+
+          let(:new_timezone) { 'Pacific/Auckland' }
+
+          it_behaves_like 'updates the rotation active periods', '10:00', '19:00'
+        end
+
+        context 'new timezone has non-whole-hour change' do
+          let(:new_timezone) { 'Asia/Kolkata' }
+
+          it_behaves_like 'updates the rotation active periods', '13:30', '22:30'
+        end
+
+        context 'new timezone but same offset' do
+          let(:new_timezone) { 'Europe/London' }
+
+          it 'updates the timezone' do
+            expect { execute }.to change { oncall_schedule.reload.timezone }.to(new_timezone)
+          end
+
+          it 'does not update the active period times' do
+            expect { execute }.to not_change { time_from_time_column(oncall_rotation.reload.active_period_start) }
+             .and not_change { time_from_time_column(oncall_rotation.active_period_end) }
+          end
+        end
+
+        context 'timezone is not changed' do
+          before do
+            params.delete(:timezone)
+          end
+
+          it 'does not update rotations' do
+            expect { execute }.to not_change { oncall_rotation.updated_at }
+          end
         end
 
         context 'error updating' do