Commit 1dfa5487 authored by Luke Duncalfe's avatar Luke Duncalfe

Merge branch '323631-remove-participant-on-removal' into 'master'

Remove user from rotation if removed from project/group

## What does this MR do?

<!--

Describe in detail what your merge request does and why.

Are there any risks involved with the proposed change? What additional
test coverage is introduced to offset the risk?

Please keep this description up-to-date with any discussion that takes
place so that reviewers can understand your intent. This is especially
important if they didn't participate in the discussion.

-->


This adds a service to remove a user from an On-call Rotation.

We ensure that the rotation is up to date by running the job top persist current shifts. Then, we update the `is_removed` boolean on the Participant model associated to the user/rotation.

Deletion from rotations are scoped to the member being removed:

- If a member is a project member, only that project's rotations are affected
- If a member is a group member, all of the projects within that group are affected

**Why don't we use the existing [`EditRotationService`](https://gitlab.com/gitlab-org/gitlab/-/blob/9c35c043a9ce9d70bd30ec6dc0d30991bf15ab29/ee/app/services/incident_management/oncall_rotations/edit_service.rb) service?**
The `EditRotationService` requires you to provide all `participant` params for all remaining participants, and in a format which requires the `user`, `color_palette`, `color_weight` etc. Rather than modify the `EditRotationService` to  make this easier, I chose to make a specific service for this use case.


SQL queries for finder:

Single Project:

Explain: https://explain.depesz.com/s/QToy

SQL: 
```sql
  User Load (0.5ms)  SELECT "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1 /*application:console,line:/ee/app/finders/incident_management/member_oncall_rotations_finder.rb:11:in `initialize'*/

  Project Load (0.5ms)  SELECT "projects".* FROM "projects" WHERE "projects"."id" = 4302 LIMIT 1 /*application:console,line:/ee/app/finders/incident_management/member_oncall_rotations_finder.rb:15:in `execute'*/
  
  IncidentManagement::OncallRotation Load (4.5ms)  SELECT "incident_management_oncall_rotations".* FROM "incident_management_oncall_rotations" INNER JOIN "incident_management_oncall_participants" ON "incident_management_oncall_rotations"."id" = "incident_management_oncall_participants"."oncall_rotation_id" INNER JOIN "incident_management_oncall_schedules" ON "incident_management_oncall_schedules"."id" = "incident_management_oncall_rotations"."oncall_schedule_id" WHERE "incident_management_oncall_participants"."user_id" = 1 AND "incident_management_oncall_schedules"."project_id" = 4302 

```

Group member, multiple projects:

Explain: https://explain.depesz.com/s/3sxa

SQL: 
```sql
  User Load (0.7ms)  SELECT "users".* FROM "users" WHERE "users"."id" = 1 LIMIT 1

  Namespace Load (0.5ms)  SELECT "namespaces".* FROM "namespaces" WHERE "namespaces"."id" = 4374 LIMIT 1

  IncidentManagement::OncallRotation Load (1.1ms)  SELECT "incident_management_oncall_rotations".* FROM "incident_management_oncall_rotations" INNER JOIN "incident_management_oncall_participants" ON "incident_management_oncall_rotations"."id" = "incident_management_oncall_participants"."oncall_rotation_id" INNER JOIN "incident_management_oncall_schedules" ON "incident_management_oncall_schedules"."id" = "incident_management_oncall_rotations"."oncall_schedule_id" WHERE "incident_management_oncall_participants"."user_id" = 1 AND "incident_management_oncall_schedules"."project_id" IN (SELECT "projects"."id" FROM "projects" WHERE "projects"."namespace_id" = 4374)
```

## Screenshots  (strongly suggested)

<!-- 

Please include any relevant screenshots that will assist reviewers and
future readers. If you need help visually verifying the change, please 
leave a comment and ping a GitLab reviewer, maintainer, or MR coach.

-->

## Does this MR meet the acceptance criteria?

### Conformity

- 📋 [Does this MR need a changelog?](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry)
  - [x] I have included a changelog entry.
  - [ ] I have not included a changelog entry because _____.
- [ ] [Documentation](https://docs.gitlab.com/ee/development/documentation/workflow.html) ([if required](https://about.gitlab.com/handbook/engineering/ux/technical-writing/workflow/#when-documentation-is-required))
- [x] [Code review guidelines](https://docs.gitlab.com/ee/development/code_review.html)
- [ ] [Merge request performance guidelines](https://docs.gitlab.com/ee/development/merge_request_performance_guidelines.html)
- [x] [Style guides](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/doc/development/contributing/style_guides.md)
- [x] [Database guides](https://docs.gitlab.com/ee/development/database_review.html)
- [x] [Separation of EE specific content](https://docs.gitlab.com/ee/development/ee_features.html#separation-of-ee-code)

### Availability and Testing

<!-- What risks does this change pose? How might it affect the quality/performance of the product?
What additional test coverage or changes to tests will be needed?
Will it require cross-browser testing?
See the test engineering process for further guidelines: https://about.gitlab.com/handbook/engineering/quality/test-engineering/ -->

<!-- If cross-browser testing is not required, please remove the relevant item, or mark it as not needed: [-] -->

- [ ] [Review and add/update tests for this feature/bug](https://docs.gitlab.com/ee/development/testing_guide/index.html). Consider [all test levels](https://docs.gitlab.com/ee/development/testing_guide/testing_levels.html). See the [Test Planning Process](https://about.gitlab.com/handbook/engineering/quality/test-engineering).
- [ ] [Tested in all supported browsers](https://docs.gitlab.com/ee/install/requirements.html#supported-web-browsers)
- [ ] Informed Infrastructure department of a default or new setting change, if applicable per [definition of done](https://docs.gitlab.com/ee/development/contributing/merge_request_workflow.html#definition-of-done)

### Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in [the security review guidelines](https://about.gitlab.com/handbook/engineering/security/#when-to-request-a-security-review):

- [ ] Label as ~security and @ mention `@gitlab-com/gl-security/appsec`
- [ ] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
- [ ] Security reports checked/validated by a reviewer from the AppSec team 


Related to #323631

See merge request gitlab-org/gitlab!59427
parents 34599448 049c2ade
---
title: Remove user from rotation if they are removed from a project or group
merge_request: 59427
author:
type: added
# frozen_string_literal: true
module IncidentManagement
# A finder used to find all rotations related to a member.
# For most cases you will want to use `OncallRotationsFinder` instead.
# For a group member, finds all rotations that user is part of in the group
# For a project member, find all the rotations that user is part of in the project.
class MemberOncallRotationsFinder
def initialize(member)
@member = member
@user = member.user
end
def execute
projects = member.source.is_a?(Group) ? member.source.projects : member.source
for_projects(projects)
end
private
attr_reader :member, :user
def for_projects(projects)
user.oncall_rotations.for_project(projects)
end
end
end
...@@ -23,5 +23,6 @@ module IncidentManagement ...@@ -23,5 +23,6 @@ module IncidentManagement
scope :not_removed, -> { where(is_removed: false) } scope :not_removed, -> { where(is_removed: false) }
scope :removed, -> { where(is_removed: true) } scope :removed, -> { where(is_removed: true) }
scope :for_user, -> (user) { where(user: user) }
end end
end end
...@@ -48,6 +48,7 @@ module IncidentManagement ...@@ -48,6 +48,7 @@ module IncidentManagement
validates :active_period_end, presence: true, if: :active_period_start validates :active_period_end, presence: true, if: :active_period_start
validate :no_active_period_for_hourly_shifts, if: :hours? validate :no_active_period_for_hourly_shifts, if: :hours?
scope :for_project, -> (project) { joins(:schedule).merge(OncallSchedule.for_project(project)) }
scope :in_progress, -> { where('starts_at < :time AND (ends_at > :time OR ends_at IS NULL)', time: 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 :except_ids, -> (ids) { where.not(id: ids) }
scope :with_active_period, -> { where.not(active_period_start: nil) } scope :with_active_period, -> { where.not(active_period_start: nil) }
......
...@@ -21,6 +21,7 @@ module IncidentManagement ...@@ -21,6 +21,7 @@ module IncidentManagement
validates :timezone, presence: true, inclusion: { in: :timezones } validates :timezone, presence: true, inclusion: { in: :timezones }
scope :for_iid, -> (iid) { where(iid: iid) } scope :for_iid, -> (iid) { where(iid: iid) }
scope :for_project, -> (project) { where(project: project) }
delegate :name, to: :project, prefix: true delegate :name, to: :project, prefix: true
......
...@@ -5,6 +5,11 @@ module IncidentManagement ...@@ -5,6 +5,11 @@ module IncidentManagement
module SharedRotationLogic module SharedRotationLogic
MAXIMUM_PARTICIPANTS = 100 MAXIMUM_PARTICIPANTS = 100
def ensure_rotation_is_up_to_date
# Ensure shift history is up to date before saving new params
IncidentManagement::OncallRotations::PersistShiftsJob.new.perform(oncall_rotation.id)
end
def save_participants! def save_participants!
participants = participants_for(oncall_rotation).each(&:validate!) participants = participants_for(oncall_rotation).each(&:validate!)
......
...@@ -14,6 +14,7 @@ module EE ...@@ -14,6 +14,7 @@ module EE
cleanup_group_identity(member) cleanup_group_identity(member)
cleanup_group_deletion_schedule(member) if member.source.is_a?(Group) cleanup_group_deletion_schedule(member) if member.source.is_a?(Group)
cleanup_oncall_rotations(member)
end end
private private
...@@ -49,6 +50,21 @@ module EE ...@@ -49,6 +50,21 @@ module EE
deletion_schedule.destroy if deletion_schedule.deleting_user == member.user deletion_schedule.destroy if deletion_schedule.deleting_user == member.user
end end
def cleanup_oncall_rotations(member)
user = member.user
return unless user
user_rotations = ::IncidentManagement::MemberOncallRotationsFinder.new(member).execute
return unless user_rotations.present?
::IncidentManagement::OncallRotations::RemoveParticipantsService.new(
user_rotations,
user
).execute
end
end end
end end
end end
...@@ -9,6 +9,7 @@ module EE ...@@ -9,6 +9,7 @@ module EE
def execute(user, options = {}) def execute(user, options = {})
result = super(user, options) do |delete_user| result = super(user, options) do |delete_user|
mirror_cleanup(delete_user) mirror_cleanup(delete_user)
oncall_rotations_cleanup(delete_user)
end end
log_audit_event(user) if result.try(:destroyed?) log_audit_event(user) if result.try(:destroyed?)
...@@ -31,6 +32,13 @@ module EE ...@@ -31,6 +32,13 @@ module EE
end end
end end
def oncall_rotations_cleanup(user)
::IncidentManagement::OncallRotations::RemoveParticipantsService.new(
user.oncall_rotations,
user
).execute
end
private private
def first_mirror_owner(user, mirror) def first_mirror_owner(user, mirror)
......
...@@ -34,8 +34,7 @@ module IncidentManagement ...@@ -34,8 +34,7 @@ module IncidentManagement
return error_participants_without_permission if users_without_permissions? return error_participants_without_permission if users_without_permissions?
end end
# Ensure shift history is up to date before saving new params ensure_rotation_is_up_to_date
IncidentManagement::OncallRotations::PersistShiftsJob.new.perform(oncall_rotation.id)
OncallRotation.transaction do OncallRotation.transaction do
oncall_rotation.update!(params) oncall_rotation.update!(params)
......
# frozen_string_literal: true
module IncidentManagement
module OncallRotations
class RemoveParticipantService < OncallRotations::BaseService
include IncidentManagement::OncallRotations::SharedRotationLogic
# @param oncall_rotations [IncidentManagement::OncallRotation]
# @param user_to_remove [User]
def initialize(oncall_rotation, user_to_remove)
@oncall_rotation = oncall_rotation
@user_to_remove = user_to_remove
end
def execute
ensure_rotation_is_up_to_date
deleted = remove_user_from_rotation
if deleted
save_current_shift!
end
end
private
attr_reader :oncall_rotation, :user_to_remove
def remove_user_from_rotation
participant = oncall_rotation.participants.for_user(user_to_remove).first
return unless participant
participant.update!(is_removed: true)
oncall_rotation.touch
end
end
end
end
# frozen_string_literal: true
module IncidentManagement
module OncallRotations
class RemoveParticipantsService
# @param oncall_rotations [Array<IncidentManagement::OncallRotation>]
# @param user_to_remove [User]
def initialize(oncall_rotations, user_to_remove)
@oncall_rotations = oncall_rotations
@user_to_remove = user_to_remove
end
attr_reader :oncall_rotations, :user_to_remove
def execute
oncall_rotations.each do |oncall_rotation|
RemoveParticipantService.new(oncall_rotation, user_to_remove).execute
end
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe IncidentManagement::MemberOncallRotationsFinder do
let_it_be(:user) { create(:user) }
describe '#execute' do
subject(:execute) { described_class.new(member).execute }
# 2 Group Projects
let_it_be(:group) { create(:group) }
let_it_be(:group_project) { create(:project, group: group) }
let_it_be(:group_schedule) { create(:incident_management_oncall_schedule, project: group_project) }
let_it_be(:group_rotation) { add_rotation_for_user(user, group_schedule) }
let_it_be(:second_group_project) { create(:project, group: group) }
let_it_be(:second_group_schedule) { create(:incident_management_oncall_schedule, project: second_group_project) }
let_it_be(:second_group_project_user_rotation) { add_rotation_for_user(user, second_group_schedule) }
let_it_be(:second_group_project_other_rotation) { create(:incident_management_oncall_rotation, schedule: second_group_schedule) }
# 1 standalone Project
let_it_be(:project) { create(:project) }
let_it_be(:schedule) { create(:incident_management_oncall_schedule, project: project) }
let_it_be(:rotation) { add_rotation_for_user(user, schedule) }
context 'group member' do
let!(:member) { create(:group_member, source: group, user: user) }
it 'returns the group rotations the user is in across many projects' do
expect(execute).to contain_exactly(group_rotation, second_group_project_user_rotation)
end
end
context 'project member' do
# Member is project member
let!(:member) { create(:project_member, source: project, user: user) }
it "returns the rotations the user is in for the member's project" do
expect(execute).to contain_exactly(rotation)
end
end
end
def add_rotation_for_user(user, schedule)
rotation = create(:incident_management_oncall_rotation, schedule: schedule)
create(:incident_management_oncall_participant, rotation: rotation, user: user)
rotation
end
end
...@@ -53,6 +53,12 @@ RSpec.describe IncidentManagement::OncallParticipant do ...@@ -53,6 +53,12 @@ RSpec.describe IncidentManagement::OncallParticipant do
it { is_expected.to contain_exactly(removed_participant) } it { is_expected.to contain_exactly(removed_participant) }
end end
describe '.for_user' do
subject { described_class.for_user(participant.user) }
it { is_expected.to contain_exactly(participant) }
end
end end
private private
......
...@@ -121,6 +121,15 @@ RSpec.describe IncidentManagement::OncallRotation do ...@@ -121,6 +121,15 @@ RSpec.describe IncidentManagement::OncallRotation do
end end
end end
describe '.for_project' do
let_it_be(:schedule_rotation) { create(:incident_management_oncall_rotation, schedule: schedule) }
let_it_be(:another_rotation) { create(:incident_management_oncall_rotation) }
subject { described_class.for_project(schedule_rotation.project) }
it { is_expected.to contain_exactly(schedule_rotation) }
end
describe '#shift_cycle_duration' do describe '#shift_cycle_duration' do
let_it_be(:rotation) { create(:incident_management_oncall_rotation, schedule: schedule, length: 5, length_unit: :days) } let_it_be(:rotation) { create(:incident_management_oncall_rotation, schedule: schedule, length: 5, length_unit: :days) }
......
...@@ -35,6 +35,17 @@ RSpec.describe IncidentManagement::OncallSchedule do ...@@ -35,6 +35,17 @@ RSpec.describe IncidentManagement::OncallSchedule do
end end
end end
describe 'scopes' do
let_it_be(:schedule) { create(:incident_management_oncall_schedule, project: project) }
let_it_be(:other_schedule) { create(:incident_management_oncall_schedule) }
describe '.for_project' do
subject { described_class.for_project(project) }
it { is_expected.to contain_exactly(schedule) }
end
end
it_behaves_like 'AtomicInternalId' do it_behaves_like 'AtomicInternalId' do
let(:internal_id_attribute) { :iid } let(:internal_id_attribute) { :iid }
let(:instance) { build(:incident_management_oncall_schedule) } let(:instance) { build(:incident_management_oncall_schedule) }
......
...@@ -81,6 +81,58 @@ RSpec.describe Members::DestroyService do ...@@ -81,6 +81,58 @@ RSpec.describe Members::DestroyService do
end end
end end
end end
context 'on-call rotations' do
let!(:project) { create(:project, group: group) }
context 'when member is in an on-call rotation' do
let(:project_1_schedule) { create(:incident_management_oncall_schedule, project: project) }
let(:project_1_rotation) { create(:incident_management_oncall_rotation, schedule: project_1_schedule) }
let!(:project_1_participant) { create(:incident_management_oncall_participant, rotation: project_1_rotation, user: member_user) }
let(:project_2) { create(:project, group: group) }
let(:project_2_schedule) { create(:incident_management_oncall_schedule, project: project_2) }
let(:project_2_rotation) { create(:incident_management_oncall_rotation, schedule: project_2_schedule) }
let!(:project_2_participant) { create(:incident_management_oncall_participant, rotation: project_2_rotation, user: member_user) }
context 'when group member is removed' do
it 'calls the remove service for each project in the group' do
expect(IncidentManagement::OncallRotations::RemoveParticipantsService).to receive(:new).with([project_1_rotation, project_2_rotation], member_user).and_call_original
subject.execute(member)
expect(project_1_participant.reload.is_removed).to eq(true)
expect(project_2_participant.reload.is_removed).to eq(true)
end
end
context 'when project member is removed' do
let!(:project_member) { create(:project_member, source: project, user: member_user) }
it 'calls the remove service for that project only' do
expect(IncidentManagement::OncallRotations::RemoveParticipantsService).to receive(:new).with([project_1_rotation], member_user).and_call_original
subject.execute(project_member)
expect(project_1_participant.reload.is_removed).to eq(true)
expect(project_2_participant.reload.is_removed).to eq(false)
end
end
end
context 'when member is not part of an on-call rotation for the group' do
before do
# Creates a rotation for another project in another group
create(:incident_management_oncall_participant, user: member_user)
end
it 'does not call the remove service' do
expect(IncidentManagement::OncallRotations::RemoveParticipantsService).not_to receive(:new)
subject.execute(member)
end
end
end
end end
context 'when current user is not present' do # ie, when the system initiates the destroy context 'when current user is not present' do # ie, when the system initiates the destroy
......
...@@ -42,6 +42,40 @@ RSpec.describe Users::DestroyService do ...@@ -42,6 +42,40 @@ RSpec.describe Users::DestroyService do
end end
end end
context 'when user has oncall rotations' do
let(:schedule) { create(:incident_management_oncall_schedule, project: project) }
let(:rotation) { create(:incident_management_oncall_rotation, schedule: schedule) }
let!(:participant) { create(:incident_management_oncall_participant, rotation: rotation, user: user) }
context 'in their own project' do
let(:project) { create(:project, namespace: user.namespace) }
it 'deletes the project and the schedule' do
operation
expect { project.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect { schedule.reload }.to raise_error(ActiveRecord::RecordNotFound)
end
end
context 'in a group project' do
let(:group) { create(:group) }
let(:project) { create(:project, namespace: group) }
before do
project.add_developer(user)
end
it 'deletes the participant from the rotation' do
expect(rotation.participants.reload).to include(participant)
operation
expect(rotation.participants.reload).not_to include(participant)
end
end
end
describe 'audit events' do describe 'audit events' do
include_examples 'audit event logging' do include_examples 'audit event logging' do
let(:fail_condition!) do let(:fail_condition!) do
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe IncidentManagement::OncallRotations::RemoveParticipantService do
let_it_be(:user) { create(:user) }
let_it_be(:project) { create(:project) }
let_it_be(:schedule) { create(:incident_management_oncall_schedule, project: project) }
let_it_be(:rotation) { create(:incident_management_oncall_rotation, schedule: schedule, length: 5, length_unit: :days) }
let_it_be(:other_participant) { create(:incident_management_oncall_participant, rotation: rotation) }
let_it_be(:participant) { create(:incident_management_oncall_participant, rotation: rotation, user: user) }
let(:service) { described_class.new(rotation, user) }
subject(:execute) { service.execute }
before do
stub_licensed_features(oncall_schedules: true)
end
context 'user is not a participant' do
let(:other_user) { create(:user) }
let(:service) { described_class.new(rotation, other_user) }
it 'does not send a notification' do
expect(NotificationService).not_to receive(:oncall_user_removed)
execute
end
end
it 'marks the participant as removed' do
expect { execute }.to change { participant.reload.is_removed }.to(true)
end
context 'with existing shift by other participant, and current shift by user to be removed' do
let(:current_date) { 1.week.after(rotation.starts_at) }
around do |example|
travel_to(current_date) { example.run }
end
# Create an historial shift (other participant)
let!(:historical_shift) { create(:incident_management_oncall_shift, rotation: rotation, participant: other_participant, starts_at: rotation.starts_at, ends_at: ends_at(rotation.starts_at)) }
context 'with historial and current shift' do
# Create a current shift (particpant being removed)
let!(:current_shift) { create(:incident_management_oncall_shift, rotation: rotation, participant: participant, starts_at: historical_shift.ends_at, ends_at: ends_at(historical_shift.ends_at)) }
it 'does not affect existing shifts, ends the current shift, and starts the new shift', :aggregate_failures do
historical_shift, current_shift = rotation.shifts.order(starts_at: :asc)
expect(historical_shift.participant).to eq(other_participant)
expect(current_shift.participant).to eq(participant)
expect(current_shift.ends_at).not_to be_like_time(Time.current)
expect { execute }.not_to change { historical_shift.reload }
new_shift = rotation.shifts.order(starts_at: :asc).last
expect(current_shift.reload.ends_at).to be_like_time(Time.current)
expect(new_shift.participant).to eq(other_participant)
expect(new_shift.starts_at).to be_like_time(Time.current)
expect(new_shift.ends_at).to be_like_time(ends_at(historical_shift.ends_at))
end
end
context 'when current shift has not been created' do
it 'creates the current shift and cuts it short' do
expect { execute }.to change { rotation.shifts.count }.from(1).to(3)
current_shift, new_current_shift = rotation.shifts.order(starts_at: :asc).last(2)
expect(current_shift.participant).to eq(participant)
expect(current_shift.ends_at).to be_like_time(Time.current)
expect(new_current_shift.participant).to eq(other_participant)
expect(new_current_shift.starts_at).to be_like_time(Time.current)
expect(new_current_shift.ends_at).to be_like_time(ends_at(current_shift.starts_at))
end
end
def ends_at(starts_at)
starts_at + rotation.shift_cycle_duration
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe IncidentManagement::OncallRotations::RemoveParticipantsService do
let!(:user) { instance_double(User) }
let!(:rotation) { instance_double(IncidentManagement::OncallRotation) }
let!(:rotation_2) { instance_double(IncidentManagement::OncallRotation) }
let(:service) { described_class.new([rotation, rotation_2], user) }
subject(:execute) { service.execute }
before do
stub_licensed_features(oncall_schedules: true)
end
it 'calls the RemoveParticipantService for each rotation' do
remove_service = instance_spy(IncidentManagement::OncallRotations::RemoveParticipantService)
expect(IncidentManagement::OncallRotations::RemoveParticipantService)
.to receive(:new)
.with(rotation, user)
.and_return(remove_service)
expect(IncidentManagement::OncallRotations::RemoveParticipantService)
.to receive(:new)
.with(rotation_2, user)
.and_return(remove_service)
expect(remove_service).to receive(:execute).twice
execute
end
end
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