Commit ca16b26f authored by Sean Arnold's avatar Sean Arnold Committed by Bob Van Landuyt

Add removed_at to oncall participants table

- Add migrations
- Add specs for new methods
- Update code to use new scopes
parent 6131d5c5
......@@ -23,9 +23,5 @@ module IncidentManagement
scope :not_removed, -> { where(is_removed: false) }
scope :removed, -> { where(is_removed: true) }
def mark_as_removed
update_column(:is_removed, true)
end
end
end
......@@ -85,6 +85,13 @@ module IncidentManagement
!hours? && active_period.present?
end
def upsert_participants!(new_participants)
::IncidentManagement::OncallParticipant.upsert_all(
new_participants,
unique_by: :index_inc_mgmnt_oncall_participants_on_user_id_and_rotation_id
)
end
private
def valid_ends_at
......
# frozen_string_literal: true
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?
# Merge the new expected attributes over the existing .
# participant's attributes to apply any changes
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
end.values
end
def existing_participants_by_user
oncall_rotation.participants.to_h do |participant|
# Setting the `is_removed` flag on the AR object
# means we don't have to write the removal to the DB
# unless the participant was actually removed
participant.is_removed = true
[participant.user_id, participant]
end
end
def expected_participants_by_user
participants_params.to_h do |participant|
break unless participant[:user].can?(:read_project, project)
[
participant[:user].id,
OncallParticipant.new(
rotation: oncall_rotation,
user: participant[:user],
color_palette: participant[:color_palette],
color_weight: participant[:color_weight],
is_removed: false
)
]
end
end
def upsert_participants(participants)
oncall_rotation.upsert_participants!(participant_rows(participants))
end
def participant_rows(participants)
participants.map do |participant|
{
oncall_rotation_id: participant.oncall_rotation_id,
user_id: participant.user_id,
color_palette: OncallParticipant.color_palettes[participant.color_palette],
color_weight: OncallParticipant.color_weights[participant.color_weight],
is_removed: participant.is_removed
}
end
end
def duplicated_users?
participant_users != participant_users.uniq
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'
end
def error_too_many_participants
error('A maximum of %{count} participants can be added' % { count: MAXIMUM_PARTICIPANTS })
end
def error_duplicate_participants
error('A user can only participate in a rotation once')
end
def error_in_validation(object)
error(object.errors.full_messages.to_sentence)
end
end
end
end
......@@ -3,7 +3,7 @@
module IncidentManagement
module OncallRotations
class CreateService < OncallRotations::BaseService
MAXIMUM_PARTICIPANTS = 100
include IncidentManagement::OncallRotations::SharedRotationLogic
# @param schedule [IncidentManagement::OncallSchedule]
# @param project [Project]
......@@ -35,82 +35,30 @@ module IncidentManagement
return error_duplicate_participants if duplicated_users?
OncallRotation.transaction do
oncall_rotation = schedule.rotations.create(rotation_params)
break error_in_validation(oncall_rotation) unless oncall_rotation.persisted?
@oncall_rotation = schedule.rotations.create!(rotation_params)
participants = participants_for(oncall_rotation)
break error_participant_has_no_permission if participants.nil?
raise InsufficientParticipantPermissionsError.new(participant_has_no_permission) if participants.nil?
first_invalid_participant = participants.find(&:invalid?)
break error_in_validation(first_invalid_participant) if first_invalid_participant
participants.each(&:validate!)
insert_participants(participants)
upsert_participants(participants)
success(oncall_rotation)
success(oncall_rotation.reset)
end
end
private
attr_reader :schedule, :project, :user, :rotation_params, :participants_params
def duplicated_users?
users = participants_params.map { |participant| participant[:user] }
users != users.uniq
rescue InsufficientParticipantPermissionsError => err
error(err.message)
rescue ActiveRecord::RecordInvalid => err
error_in_validation(err.record)
end
def participants_for(rotation)
participants_params.map do |participant|
break unless participant[:user].can?(:read_project, project)
OncallParticipant.new(
rotation: rotation,
user: participant[:user],
color_palette: participant[:color_palette],
color_weight: participant[:color_weight]
)
end
end
def participant_rows(participants)
participants.map do |participant|
{
oncall_rotation_id: participant.oncall_rotation_id,
user_id: participant.user_id,
color_palette: OncallParticipant.color_palettes[participant.color_palette],
color_weight: OncallParticipant.color_weights[participant.color_weight]
}
end
end
# BulkInsertSafe cannot be used here while OncallParticipant
# has a has_many association. https://gitlab.com/gitlab-org/gitlab/-/issues/247718
# We still want to bulk insert to avoid up to MAXIMUM_PARTICIPANTS
# consecutive insertions, but .insert_all
# does not include validations. Warning!
def insert_participants(participants)
OncallParticipant.insert_all(participant_rows(participants))
end
def error_participant_has_no_permission
error('A participant has insufficient permissions to access the project')
end
def error_too_many_participants
error(_('A maximum of %{count} participants can be added') % { count: MAXIMUM_PARTICIPANTS })
end
private
def error_duplicate_participants
error(_('A user can only participate in a rotation once'))
end
attr_reader :schedule, :project, :user, :rotation_params, :participants_params, :oncall_rotation
def error_no_permissions
error(_('You have insufficient permissions to create an on-call rotation for this project'))
end
def error_in_validation(object)
error(object.errors.full_messages.to_sentence)
error('You have insufficient permissions to create an on-call rotation for this project')
end
end
end
......
# frozen_string_literal: true
module IncidentManagement
module OncallRotations
class EditService < OncallRotations::BaseService
include IncidentManagement::OncallRotations::SharedRotationLogic
# @param rotation [IncidentManagement::OncallRotation]
# @param user [User]
# @param params [Hash<Symbol,Any>]
# @param params - name [String] The name of the on-call rotation.
# @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 - user [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".
# @option opts - color_weight [String] The color weight to assign to for the on-call user, for example "500". Max 4 chars.
def initialize(oncall_rotation, user, params)
@oncall_rotation = oncall_rotation
@user = user
@project = oncall_rotation.project
@params = params
@participants_params = params.delete(:participants)
end
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?
# 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)
success(oncall_rotation.reset)
end
rescue InsufficientParticipantPermissionsError => err
error(err.message)
rescue ActiveRecord::RecordInvalid => err
error_in_validation(err.record)
end
private
attr_reader :oncall_rotation, :user, :project, :params, :participants_params
def update_and_remove_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)
oncall_rotation.touch
end
def error_no_permissions
error('You have insufficient permissions to edit an on-call rotation in this project')
end
end
end
end
......@@ -14,11 +14,17 @@ FactoryBot.define do
active_period_end { '17:00' }
end
trait :with_participant do
after(:create) do |rotation|
user = create(:user)
rotation.project.add_reporter(user)
create(:incident_management_oncall_participant, rotation: rotation, user: user)
trait :with_participants do
transient do
participants_count { 1 }
end
after(:create) do |rotation, evaluator|
evaluator.participants_count.times do
user = create(:user)
rotation.project.add_reporter(user)
create(:incident_management_oncall_participant, rotation: rotation, user: user)
end
end
end
......
......@@ -17,13 +17,13 @@ RSpec.describe IncidentManagement::OncallUsersFinder do
# Schedule 1 / Rotation 1
let_it_be(:s1) { create(:incident_management_oncall_schedule, project: project) }
let_it_be(:s1_r1) { create(:incident_management_oncall_rotation, :with_participant, schedule: s1) }
let_it_be(:s1_r1) { create(:incident_management_oncall_rotation, :with_participants, schedule: s1) }
let_it_be(:s1_r1_p1) { s1_r1.participants.first }
let_it_be(:user_1) { s1_r1_p1.user }
let_it_be(:s1_r1_shift1) { create(:incident_management_oncall_shift, participant: s1_r1_p1) }
# Schedule 1 / Rotation 2
let_it_be(:s1_r2) { create(:incident_management_oncall_rotation, :with_participant, schedule: s1) }
let_it_be(:s1_r2) { create(:incident_management_oncall_rotation, :with_participants, schedule: s1) }
let_it_be(:s1_r2_p1) { s1_r2.participants.first }
let_it_be(:user_2) { s1_r2_p1.user }
let_it_be(:s1_r2_shift1) { create(:incident_management_oncall_shift, participant: s1_r2_p1) }
......@@ -33,7 +33,7 @@ RSpec.describe IncidentManagement::OncallUsersFinder do
# Schedule 2 / Rotation 1
let_it_be(:s2) { create(:incident_management_oncall_schedule, project: project) }
let_it_be(:s2_r1) { create(:incident_management_oncall_rotation, :with_participant, schedule: s2) }
let_it_be(:s2_r1) { create(:incident_management_oncall_rotation, :with_participants, schedule: s2) }
let_it_be(:s2_r1_p1) { s2_r1.participants.first }
let_it_be(:user_4) { s2_r1_p1.user }
let_it_be(:s2_r1_shift1) { create(:incident_management_oncall_shift, participant: s2_r1_p1) }
......
......@@ -6,7 +6,7 @@ RSpec.describe Resolvers::IncidentManagement::OncallShiftsResolver do
include GraphqlHelpers
let_it_be(:current_user) { create(:user) }
let_it_be(:rotation) { create(:incident_management_oncall_rotation, :with_participant, :utc) }
let_it_be(:rotation) { create(:incident_management_oncall_rotation, :with_participants, :utc) }
let_it_be(:project) { rotation.project }
let(:args) { { start_time: rotation.starts_at, end_time: rotation.starts_at + rotation.shift_cycle_duration } }
......
......@@ -55,14 +55,6 @@ RSpec.describe IncidentManagement::OncallParticipant do
end
end
describe '#mark_as_removed' do
subject { participant.mark_as_removed }
it 'updates is_removed to true' do
expect { subject }.to change { participant.reload.is_removed }.to(true)
end
end
private
def remove_user_from_project(user, project)
......
......@@ -33,7 +33,8 @@ RSpec.describe IncidentManagement::OncallRotations::CreateService do
describe '#execute' do
shared_examples 'error response' do |message|
it 'has an informative message' do
it 'does not save the rotation and has an informative message' do
expect { execute }.not_to change(IncidentManagement::OncallRotation, :count)
expect(execute).to be_error
expect(execute.message).to eq(message)
end
......@@ -82,7 +83,7 @@ RSpec.describe IncidentManagement::OncallRotations::CreateService do
it 'has an informative error message' do
expect(execute).to be_error
expect(execute.message).to eq("A maximum of #{IncidentManagement::OncallRotations::CreateService::MAXIMUM_PARTICIPANTS} participants can be added")
expect(execute.message).to eq("A maximum of #{IncidentManagement::OncallRotations::SharedRotationLogic::MAXIMUM_PARTICIPANTS} participants can be added")
end
end
......@@ -134,7 +135,7 @@ RSpec.describe IncidentManagement::OncallRotations::CreateService do
expect(oncall_rotation.length).to eq(1)
expect(oncall_rotation.length_unit).to eq('days')
expect(oncall_rotation.participants.length).to eq(1)
expect(oncall_rotation.participants.reload.length).to eq(1)
expect(oncall_rotation.participants.first).to have_attributes(
**participants.first,
rotation: oncall_rotation,
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe IncidentManagement::OncallRotations::EditService do
let_it_be(:user_with_permissions) { create(:user) }
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_rotation) { create(:incident_management_oncall_rotation, :with_participants, schedule: oncall_schedule, participants_count: 2) }
let(:current_user) { user_with_permissions }
let(:params) { rotation_params }
let(:service) { described_class.new(oncall_rotation, current_user, params) }
before do
stub_licensed_features(oncall_schedules: true)
project.add_maintainer(user_with_permissions)
end
describe '#execute' do
subject(:execute) { service.execute }
shared_examples 'error response' do |message|
it 'has an informative message' do
expect { execute }.not_to change { oncall_rotation.reload.updated_at }
expect(execute).to be_error
expect(execute.message).to eq(message)
end
end
context 'no license' do
before do
stub_licensed_features(oncall_schedules: false)
end
it_behaves_like 'error response', 'Your license does not support on-call rotations'
end
context 'user does not have permission' do
let(:current_user) { user_without_permissions }
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)) }
it 'adds the participant to the rotation' do
subject
attributes_to_match = participant_to_add.attributes.except('id')
expect(oncall_rotation.participants.not_removed).to include(an_object_having_attributes(attributes_to_match))
end
it 'updates the rotation updated_at' do
expect { subject }.to change { oncall_rotation.updated_at }
end
context 'new participant has a validation error' do
# Participant with nil color palette
let(:participant_to_add) { build(:incident_management_oncall_participant, rotation: oncall_rotation, user: user_with_permissions, color_palette: nil) }
it_behaves_like 'error response', "Color palette can't be blank"
end
context 'rotation params have a validation error' do
let(:rotation_edit_params) { { name: '' } }
let(:params) { rotation_params(edit_params: rotation_edit_params, participants: oncall_rotation.participants.to_a.push(participant_to_add)) }
it 'does not add the participant' do
expect { subject }.not_to change(IncidentManagement::OncallParticipant, :count)
end
it_behaves_like 'error response', "Name can't be blank"
end
end
context 'adding too many participants' do
let(:participant_to_add) { build(:incident_management_oncall_participant, rotation: oncall_rotation, user: user_with_permissions) }
let(:params) { rotation_params(participants: Array.new(described_class::MAXIMUM_PARTICIPANTS + 1, participant_to_add)) }
it 'has an informative error message' do
expect { execute }.not_to change { oncall_rotation.reload.updated_at }
expect(execute).to be_error
expect(execute.message).to eq("A maximum of #{described_class::MAXIMUM_PARTICIPANTS} participants can be added")
end
end
context 'when adding a duplicate user' do
let(:existing_participant_user) { oncall_rotation.participants.first.user }
let(:participant_to_add) { build(:incident_management_oncall_participant, rotation: oncall_rotation, user: existing_participant_user) }
let(:params) { rotation_params(participants: oncall_rotation.participants.to_a.push(participant_to_add)) }
it_behaves_like 'error response', 'A user can only participate in a rotation once'
end
context 'when adding a user that do not have permissions' do
let(:another_user_with_permission) do
new_user = create(:user)
project.add_maintainer(new_user)
new_user
end
let(:participant_to_add) { build(:incident_management_oncall_participant, rotation: oncall_rotation, user: another_user_with_permission) }
let(:participant_without_permissions_to_add) { build(:incident_management_oncall_participant, rotation: oncall_rotation, user: user_without_permissions) }
let(:params) { rotation_params(participants: oncall_rotation.participants.to_a.push(participant_to_add, participant_without_permissions_to_add)) }
it_behaves_like 'error response', 'A participant has insufficient permissions to access the project'
it 'does not modify the rotation' do
expect { subject }.not_to change { oncall_rotation.participants.reload }
end
end
context 'removing one participant' do
let(:participant_to_keep) { oncall_rotation.participants.first }
let(:participant_to_remove) { oncall_rotation.participants.last }
let(:params) { rotation_params(participants: [participant_to_keep]) }
it 'soft-removes the participant from the rotation' do
subject
expect(participant_to_remove.reload.is_removed).to eq(true)
expect(participant_to_keep.reload.is_removed).to eq(false)
end
end
context 'removing all participants' do
let(:params) { rotation_params(participants: []) }
it 'soft-deletes all the rotation participants' do
subject
expect(oncall_rotation.participants.not_removed).to be_empty
expect(oncall_rotation.participants.removed).to eq(oncall_rotation.participants)
end
end
context 'participant param is nil' do
let(:params) { rotation_params(participants: nil) }
it 'does not modify the participants' do
subject
expect(oncall_rotation.participants.not_removed).to eq(oncall_rotation.participants)
expect(oncall_rotation.participants.removed).to be_empty
end
end
context 'editing rotation attributes' do
let(:params) { { name: 'Changed rotation', length: 7, length_unit: 'days', starts_at: 1.week.from_now.change(sec: 0), ends_at: 1.month.from_now.change(sec: 0) } }
it 'updates the rotation to match the params' do
subject
expect(oncall_rotation.reload).to have_attributes(params)
end
context 'with a validation error' do
let(:params) { { name: '', starts_at: 1.week.from_now } }
it_behaves_like 'error response', "Name can't be blank"
it 'updates the rotation to match the params' do
subject
expect(oncall_rotation.reload).not_to have_attributes(params)
end
end
end
end
private
def rotation_params(participants: nil, edit_params: {})
# if participant params given, generate them
# otherwise use saved params
edit_params.merge(participants: participant_params(participants))
end
def participant_params(participants)
return unless participants
participants.map do |participant|
{
user: participant.user,
color_palette: participant.color_palette,
color_weight: participant.color_weight
}
end
end
end
......@@ -27,7 +27,7 @@ RSpec.describe IncidentManagement::OncallRotations::PersistShiftsJob do
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) }
let_it_be(:rotation) { create(:incident_management_oncall_rotation, :with_participants, created_at: 1.day.ago) }
it 'creates shift' do
expect { perform }.to change { rotation.shifts.count }.by(1)
......@@ -36,7 +36,7 @@ RSpec.describe IncidentManagement::OncallRotations::PersistShiftsJob do
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) }
let_it_be(:rotation) { create(:incident_management_oncall_rotation, :with_participants, starts_at: 1.month.ago) }
it 'creates shift without backfilling' do
expect { perform }.to change { rotation.shifts.count }.by(1)
......@@ -58,7 +58,7 @@ RSpec.describe IncidentManagement::OncallRotations::PersistShiftsJob do
create(
:incident_management_oncall_rotation,
:with_active_period, # 8:00 - 17:00
:with_participant,
:with_participants,
:utc,
created_at: created_at, # Monday @ 5:00
starts_at: starts_at, # Tuesday @ 00:00
......@@ -175,8 +175,8 @@ RSpec.describe IncidentManagement::OncallRotations::PersistShiftsJob do
let_it_be_with_reload(:rotation) do
create(
:incident_management_oncall_rotation,
:with_participants,
:utc,
:with_participant,
:with_active_period, # 8:00-17:00
starts_at: starts_at
)
......
......@@ -1348,9 +1348,6 @@ msgstr ""
msgid "A limit of %{ci_project_subscriptions_limit} subscriptions to or from a project applies."
msgstr ""
msgid "A maximum of %{count} participants can be added"
msgstr ""
msgid "A member of the abuse team will review your report as soon as possible."
msgstr ""
......@@ -1426,9 +1423,6 @@ msgstr ""
msgid "A title is required"
msgstr ""
msgid "A user can only participate in a rotation once"
msgstr ""
msgid "A user with write access to the source branch selected this option"
msgstr ""
......@@ -34581,9 +34575,6 @@ msgstr ""
msgid "You have insufficient permissions to create an HTTP integration for this project"
msgstr ""
msgid "You have insufficient permissions to create an on-call rotation for this project"
msgstr ""
msgid "You have insufficient permissions to create an on-call schedule for this project"
msgstr ""
......
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