Commit f280faa7 authored by Sarah Yasonik's avatar Sarah Yasonik Committed by Peter Leitzen

Allow rotations to gracefully switch between parameters

parent 3a589001
...@@ -4,17 +4,16 @@ module IncidentManagement ...@@ -4,17 +4,16 @@ module IncidentManagement
module OncallRotations module OncallRotations
module SharedRotationLogic module SharedRotationLogic
MAXIMUM_PARTICIPANTS = 100 MAXIMUM_PARTICIPANTS = 100
InsufficientParticipantPermissionsError = Class.new(StandardError)
# Merges existing participants with API-provided def save_participants!
# participants instead of using just the API-provided ones participants = participants_for(oncall_rotation).each(&:validate!)
def participants_for(oncall_rotation)
# Exit early if the participants that the caller upsert_participants(participants)
# wants on the rotation don't have permissions. end
return if expected_participants_by_user.nil?
# Merge the new expected attributes over the existing . # Merge the new expected attributes over the existing
# participant's attributes to apply any changes # 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_participants_by_user.merge(expected_participants_by_user) do |_, existing_participant, expected_participant|
existing_participant.assign_attributes(expected_participant.attributes.except('id')) existing_participant.assign_attributes(expected_participant.attributes.except('id'))
existing_participant existing_participant
...@@ -34,8 +33,6 @@ module IncidentManagement ...@@ -34,8 +33,6 @@ module IncidentManagement
def expected_participants_by_user def expected_participants_by_user
participants_params.to_h do |participant| participants_params.to_h do |participant|
break unless participant[:user].can?(:read_project, project)
[ [
participant[:user].id, participant[:user].id,
OncallParticipant.new( OncallParticipant.new(
...@@ -69,12 +66,35 @@ module IncidentManagement ...@@ -69,12 +66,35 @@ module IncidentManagement
participant_users != participant_users.uniq participant_users != participant_users.uniq
end end
def users_without_permissions?
DeclarativePolicy.subject_scope do
participant_users.any? { |user| !user.can?(:read_project, project) }
end
end
def participant_users def participant_users
@participant_users ||= participants_params.map { |participant| participant[:user] } @participant_users ||= participants_params.map { |participant| participant[:user] }
end end
def participant_has_no_permission # Used to accurately record shift history when rotations
'A participant has insufficient permissions to access the project' # 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 end
def error_too_many_participants def error_too_many_participants
......
...@@ -33,22 +33,17 @@ module IncidentManagement ...@@ -33,22 +33,17 @@ module IncidentManagement
return error_no_permissions unless allowed? return error_no_permissions unless allowed?
return error_too_many_participants if participants_params.size > MAXIMUM_PARTICIPANTS return error_too_many_participants if participants_params.size > MAXIMUM_PARTICIPANTS
return error_duplicate_participants if duplicated_users? return error_duplicate_participants if duplicated_users?
return error_participants_without_permission if users_without_permissions?
OncallRotation.transaction do OncallRotation.transaction do
@oncall_rotation = schedule.rotations.create!(rotation_params) @oncall_rotation = schedule.rotations.create!(rotation_params)
participants = participants_for(oncall_rotation) save_participants!
raise InsufficientParticipantPermissionsError.new(participant_has_no_permission) if participants.nil? save_current_shift!
participants.each(&:validate!)
upsert_participants(participants)
success(oncall_rotation.reset) success(oncall_rotation.reset)
end end
rescue InsufficientParticipantPermissionsError => err
error(err.message)
rescue ActiveRecord::RecordInvalid => err rescue ActiveRecord::RecordInvalid => err
error_in_validation(err.record) error_in_validation(err.record)
end end
......
...@@ -27,25 +27,25 @@ module IncidentManagement ...@@ -27,25 +27,25 @@ module IncidentManagement
def execute def execute
return error_no_license unless available? return error_no_license unless available?
return error_no_permissions unless allowed? 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 # Ensure shift history is up to date before saving new params
IncidentManagement::OncallRotations::PersistShiftsJob.new.perform(oncall_rotation.id) IncidentManagement::OncallRotations::PersistShiftsJob.new.perform(oncall_rotation.id)
OncallRotation.transaction do 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) oncall_rotation.update!(params)
save_participants!
save_current_shift!
success(oncall_rotation.reset) success(oncall_rotation.reset)
end end
rescue InsufficientParticipantPermissionsError => err
error(err.message)
rescue ActiveRecord::RecordInvalid => err rescue ActiveRecord::RecordInvalid => err
error_in_validation(err.record) error_in_validation(err.record)
end end
...@@ -54,15 +54,10 @@ module IncidentManagement ...@@ -54,15 +54,10 @@ module IncidentManagement
attr_reader :oncall_rotation, :user, :project, :params, :participants_params attr_reader :oncall_rotation, :user, :project, :params, :participants_params
def update_and_remove_participants def save_participants!
return if participants_params.nil? return if participants_params.nil?
participants = participants_for(oncall_rotation) super
raise InsufficientParticipantPermissionsError.new(participant_has_no_permission) if participants.nil?
participants.each(&:validate!)
upsert_participants(participants)
oncall_rotation.touch oncall_rotation.touch
end end
......
...@@ -186,6 +186,18 @@ RSpec.describe IncidentManagement::OncallRotations::CreateService do ...@@ -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" it_behaves_like 'error response', "Active period end must be later than active period start"
end end
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 end
end end
...@@ -7,7 +7,7 @@ RSpec.describe IncidentManagement::OncallRotations::EditService do ...@@ -7,7 +7,7 @@ RSpec.describe IncidentManagement::OncallRotations::EditService do
let_it_be(:user_without_permissions) { create(:user) } let_it_be(:user_without_permissions) { create(:user) }
let_it_be_with_refind(:project) { create(:project) } 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_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(:current_user) { user_with_permissions }
let(:params) { rotation_params } let(:params) { rotation_params }
...@@ -43,14 +43,6 @@ RSpec.describe IncidentManagement::OncallRotations::EditService do ...@@ -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' it_behaves_like 'error response', 'You have insufficient permissions to edit an on-call rotation in this project'
end 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 context 'adding one participant' do
let(:participant_to_add) { build(:incident_management_oncall_participant, rotation: oncall_rotation, user: user_with_permissions) } 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)) } let(:params) { rotation_params(participants: oncall_rotation.participants.to_a.push(participant_to_add)) }
...@@ -179,6 +171,79 @@ RSpec.describe IncidentManagement::OncallRotations::EditService do ...@@ -179,6 +171,79 @@ RSpec.describe IncidentManagement::OncallRotations::EditService do
end end
end 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 end
private private
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment