Commit 5ff033b9 authored by Mayra Cabrera's avatar Mayra Cabrera

Merge branch 'sy-delete-esc-rules-on-user-change' into 'master'

Delete escalation rules and notify project owners on user removal

See merge request gitlab-org/gitlab!73127
parents f42c2ac8 643358f2
# frozen_string_literal: true
module IncidentManagement
class EscalationRulesFinder
# @param project [Project, Array<Project>, Integer(project_id), Array<Integer>, Project::ActiveRecord_Relation]
# Limit rules to within these projects
# @param user [Project, Array<Project>, Integer(user_id), Array<Integer>, User::ActiveRecord_Relation]
# Limit rules to those which notify these users directly
# @param include_removed [Boolean] Include rules which have been deleted in the UI but may correspond to existing pending escalations.
# @param member [GroupMember, ProjectMember] A member which will be disambiguated into project/user params
def initialize(user: nil, project: nil, include_removed: false, member: nil)
@user = user
@project = project
@include_removed = include_removed
disambiguate_member(member)
end
def execute
rules = by_project(IncidentManagement::EscalationRule)
rules = by_user(rules)
with_removed(rules)
end
private
attr_reader :member, :user, :project, :include_removed
def by_project(rules)
return rules unless project
rules.for_project(project)
end
def by_user(rules)
return rules unless user
rules.for_user(user)
end
def with_removed(rules)
return rules if include_removed
rules.not_removed
end
def disambiguate_member(member)
return unless member
raise ArgumentError, 'Member param cannot be used with project or user params' if user || project
raise ArgumentError, 'Member does not correspond to a user' unless member.user
@user = member.user
@project = member.source_type == 'Project' ? member.source_id : member.source.projects
end
end
end
......@@ -30,6 +30,17 @@ module EE
mail(to: user.notification_email_for(@project.group),
subject: subject('Mirror user changed'))
end
def user_escalation_rule_deleted_email(user, project, rules, recipient)
@user = user
@project = project
@rules = rules
mail(to: recipient.notification_email_for(@project.group), subject: subject('User removed from escalation policy')) do |format|
format.html { render layout: 'mailer' }
format.text { render layout: 'mailer' }
end
end
end
end
end
......@@ -7,6 +7,7 @@ module IncidentManagement
belongs_to :policy, class_name: 'EscalationPolicy', inverse_of: 'rules', foreign_key: 'policy_id'
belongs_to :oncall_schedule, class_name: 'OncallSchedule', foreign_key: 'oncall_schedule_id', optional: true
belongs_to :user, optional: true
has_one :project, through: :policy, source: :project
enum status: ::IncidentManagement::Escalatable::STATUSES.slice(:acknowledged, :resolved)
......@@ -27,6 +28,10 @@ module IncidentManagement
scope :not_removed, -> { where(is_removed: false) }
scope :removed, -> { where(is_removed: true) }
scope :for_user, -> (user) { where(user: user) }
scope :for_project, -> (project) { where(policy: { project: project }).joins(:policy).references(:policy) }
scope :load_project_with_routes, -> { preload(project: [:route, { namespace: :route }]) }
scope :load_policy, -> { includes(:policy) }
private
......
......@@ -38,6 +38,7 @@ module IncidentManagement
# Note! If changing the order of participants, also change the :with_shift_generation_associations scope.
has_many :active_participants, -> { not_removed.order(id: :asc) }, class_name: 'OncallParticipant', inverse_of: :rotation
has_many :users, through: :participants
has_many :participating_users, through: :active_participants, source: :user
has_many :shifts, class_name: 'OncallShift', inverse_of: :rotation, foreign_key: :rotation_id
validates :name, presence: true, uniqueness: { scope: :oncall_schedule_id }, length: { maximum: NAME_LENGTH }
......
......@@ -15,6 +15,7 @@ module EE
cleanup_group_identity(member)
cleanup_group_deletion_schedule(member) if member.source.is_a?(Group)
cleanup_oncall_rotations(member)
cleanup_escalation_rules(member) if member.user
end
private
......@@ -65,6 +66,12 @@ module EE
user
).execute
end
def cleanup_escalation_rules(member)
rules = ::IncidentManagement::EscalationRulesFinder.new(member: member, include_removed: true).execute
::IncidentManagement::EscalationRules::DestroyService.new(escalation_rules: rules, user: member.user).execute
end
end
end
end
......@@ -77,21 +77,41 @@ module EE
end
def oncall_user_removed(rotation, user, async_notification = true)
project_owners_and_participants(rotation, user).each do |recipient|
oncall_user_removed_recipients(rotation, user).each do |recipient|
email = mailer.user_removed_from_rotation_email(user, rotation, [recipient])
async_notification ? email.deliver_later : email.deliver_now
end
end
def user_escalation_rule_deleted(project, user, rules)
user_escalation_rule_deleted_recipients(project, user).map do |recipient|
# Deliver now as rules (& maybe user) are being deleted
mailer.user_escalation_rule_deleted_email(user, project, rules, recipient).deliver_now
end
end
private
def project_owners_and_participants(rotation, user)
project = rotation.project
def oncall_user_removed_recipients(rotation, removed_user)
incident_management_owners(rotation.project)
.including(rotation.participating_users)
.excluding(removed_user)
.uniq
end
def user_escalation_rule_deleted_recipients(project, removed_user)
incident_management_owners(project).excluding(removed_user)
end
owners = project.owner.is_a?(Group) ? project.owner.owners : [project.owner]
member_owners = project.members.owners
def incident_management_owners(project)
return [project.owner] unless project.group
(owners + member_owners + rotation.participants.map(&:user) - [user]).compact.uniq
MembersFinder
.new(project, nil, params: { active_without_invites_and_requests: true })
.execute
.owners
.map(&:user)
end
def add_mr_approvers_email(merge_request, approvers, current_user)
......
......@@ -10,6 +10,7 @@ module EE
result = super(user, options) do |delete_user|
mirror_cleanup(delete_user)
oncall_rotations_cleanup(delete_user)
escalation_rules_cleanup(delete_user)
end
log_audit_event(user) if result.try(:destroyed?)
......@@ -40,6 +41,12 @@ module EE
).execute
end
def escalation_rules_cleanup(user)
rules = ::IncidentManagement::EscalationRulesFinder.new(user: user, include_removed: true).execute
::IncidentManagement::EscalationRules::DestroyService.new(escalation_rules: rules, user: user).execute
end
private
def first_mirror_owner(user, mirror)
......
# frozen_string_literal: true
module IncidentManagement
module EscalationRules
# Permanently deletes escalation rules in bulk. To remove
# a rule but allow it continue notifying for existing
# escalations, prefer updating EscalationRule#is_removed.
class DestroyService
# @param escalation_rules [ActiveRecord::Relation<EscalationRule>] The rules to be deleted
# @param user [User] User corresponding to escalation rules
def initialize(escalation_rules:, user:)
@escalation_rules = escalation_rules
@user = user
end
def execute
preload_associations
send_user_deleted_emails
# Records are already loaded, so `#ids` does not incur extra query & simplifies deletion
IncidentManagement::EscalationRule.id_in(escalation_rules.ids).delete_all # rubocop: disable CodeReuse/ActiveRecord
end
private
attr_reader :escalation_rules, :user
def preload_associations
@escalation_rules = escalation_rules.load_policy.load_project_with_routes
end
def send_user_deleted_emails
escalation_rules
.group_by(&:project)
.each { |project, rules| send_user_rule_deleted_email(project, rules) }
end
def send_user_rule_deleted_email(project, rules)
NotificationService.new.user_escalation_rule_deleted(project, user, rules)
end
end
end
end
%p
= html_escape(_("%{user_name} (%{user_username}) was removed from the following escalation policies in %{project_link}: ")) % { user_name: @user.name, user_username: @user.username, project_link: link_to(@project.name, project_url(@project).html_safe) }
%ul
- @rules.each do |rule|
- policy_link = link_to(rule.policy.name, project_incident_management_escalation_policies_url(@project).html_safe)
%li= html_escape(_("%{policy_link} (notifying after %{elapsed_time} minutes unless %{status})")) % { policy_link: policy_link, elapsed_time: rule.elapsed_time_seconds / 60, status: rule.status }
%p
= html_escape(_("Please review the updated escalation policies for %{project_link}. It is recommended that you reach out to the current on-call responder to ensure continuity of on-call coverage.")) % { project_link: link_to(@project.name, project_url(@project).html_safe) }
<%= _("%{user_name} (%{user_username}) was removed from the following escalation policies in %{project}:") % { user_name: @user.name, user_username: @user.username, project: @project.name } %>
<% @rules.each do |rule| %>
<%= _("- %{policy_name} (notifying after %{elapsed_time} minutes unless %{status})") % { policy_name: rule.policy.name, elapsed_time: rule.elapsed_time_seconds / 60, status: rule.status } %>
<% end %>
<%= _("Please review the updated escalation policies for %{project}. It is recommended that you reach out to the current on-call responder to ensure continuity of on-call coverage.") % { project: @project.name } %>
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe IncidentManagement::EscalationRulesFinder do
let_it_be(:group) { create(:group) }
let_it_be(:project) { create(:project, group: group) }
let_it_be(:other_project) { create(:project, group: group) }
let_it_be(:user) { create(:user) }
let_it_be(:policy) { create(:incident_management_escalation_policy, project: project) }
let_it_be(:other_policy) { create(:incident_management_escalation_policy, project: other_project) }
let_it_be(:schedule_rule) { policy.rules.first }
let_it_be(:schedule_rule_for_other_project) { other_policy.rules.first }
let_it_be(:removed_rule) { create(:incident_management_escalation_rule, :removed, policy: policy) }
let_it_be(:rule_for_user) { create(:incident_management_escalation_rule, :with_user, policy: policy, user: user) }
let_it_be(:rule_for_other_user) { create(:incident_management_escalation_rule, :with_user, policy: policy) }
let_it_be(:rule_for_other_policy) { create(:incident_management_escalation_rule, :with_user, policy: other_policy, user: user) }
let_it_be(:project_member) { project.members.first }
let_it_be(:group_member) { create(:group_member, :developer, group: group, user: user) }
let(:params) { {} }
describe '#execute' do
subject(:execute) { described_class.new(**params).execute }
context 'when project is given' do
let(:project_rules) { [rule_for_user, rule_for_other_user, schedule_rule] }
it 'returns the rules in the project for different types of project inputs' do
[
project,
project.id,
[project],
[project.id],
Project.where(id: project.id)
].each do |project_param|
expect(described_class.new(project: project_param).execute).to contain_exactly(*project_rules)
end
end
context 'when removed rules should be included' do
let(:params) { { include_removed: true, project: project } }
it { is_expected.to contain_exactly(removed_rule, *project_rules) }
end
end
context 'when user is given' do
it 'returns the user rules for different types of user inputs' do
[
user,
user.id,
[user],
[user.id],
User.where(id: user.id)
].each do |user_param|
expect(described_class.new(user: user_param).execute).to contain_exactly(
rule_for_user,
rule_for_other_policy
)
end
end
end
context 'when group member is given' do
let(:params) { { member: group_member } }
it { is_expected.to contain_exactly(rule_for_user, rule_for_other_policy) }
context 'when member does not belong to a user' do
let(:params) { { member: create(:group_member, :invited, group: group) } }
specify { expect { execute }.to raise_error(ArgumentError, 'Member does not correspond to a user') }
end
end
context 'when project member is given' do
let(:params) { { member: project_member } }
it { is_expected.to contain_exactly(rule_for_other_user) }
context 'when user is also given' do
let(:params) { { member: project_member, user: user } }
specify { expect { execute }.to raise_error(ArgumentError, 'Member param cannot be used with project or user params') }
end
context 'when project is also given' do
let(:params) { { member: project_member, project: project } }
specify { expect { execute }.to raise_error(ArgumentError, 'Member param cannot be used with project or user params') }
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
require 'email_spec'
RSpec.describe Emails::Projects do
include EmailSpec::Matchers
describe '#user_escalation_rule_deleted_email' do
let(:rule) { create(:incident_management_escalation_rule, :with_user) }
let(:user) { rule.user }
let(:project) { rule.project }
let(:recipient) { build(:user) }
let(:elapsed_time) { (rule.elapsed_time_seconds / 60).to_s }
subject { Notify.user_escalation_rule_deleted_email(user, project, [rule], recipient) }
it 'has the correct email content', :aggregate_failures do
is_expected.to have_subject("#{project.name} | User removed from escalation policy")
is_expected.to have_body_text(user.name)
is_expected.to have_body_text(user.username)
is_expected.to have_body_text('was removed from the following escalation policies')
is_expected.to have_body_text(rule.policy.name)
is_expected.to have_body_text(elapsed_time)
is_expected.to have_body_text(rule.status.to_s)
is_expected.to have_body_text("Please review the updated escalation policies for")
is_expected.to have_body_text(project.name)
is_expected.to have_body_text("It is recommended that you reach out to the current on-call responder to ensure continuity of on-call coverage")
end
end
end
......@@ -9,6 +9,7 @@ RSpec.describe IncidentManagement::EscalationRule do
it { is_expected.to belong_to(:policy) }
it { is_expected.to belong_to(:oncall_schedule).optional }
it { is_expected.to belong_to(:user).optional }
it { is_expected.to have_one(:project).through(:policy) }
end
describe 'validations' do
......@@ -51,11 +52,12 @@ RSpec.describe IncidentManagement::EscalationRule do
describe 'scopes' do
let_it_be(:rule) { create(:incident_management_escalation_rule) }
let_it_be(:removed_rule) { create(:incident_management_escalation_rule, :removed, policy: rule.policy) }
let_it_be(:other_project_rule) { create(:incident_management_escalation_rule) }
describe '.not_removed' do
subject { described_class.not_removed }
it { is_expected.to contain_exactly(rule) }
it { is_expected.to contain_exactly(rule, other_project_rule) }
end
describe '.removed' do
......@@ -63,5 +65,13 @@ RSpec.describe IncidentManagement::EscalationRule do
it { is_expected.to contain_exactly(removed_rule) }
end
describe '.for_project' do
let(:project) { other_project_rule.project }
subject { described_class.for_project(project) }
it { is_expected.to contain_exactly(other_project_rule) }
end
end
end
......@@ -10,6 +10,7 @@ RSpec.describe IncidentManagement::OncallRotation do
it { is_expected.to have_many(:participants).order(id: :asc).class_name('OncallParticipant').inverse_of(:rotation) }
it { is_expected.to have_many(:active_participants).order(id: :asc).class_name('OncallParticipant').inverse_of(:rotation) }
it { is_expected.to have_many(:users).through(:participants) }
it { is_expected.to have_many(:participating_users).through(:active_participants).source(:user) }
it { is_expected.to have_many(:shifts).class_name('OncallShift').inverse_of(:rotation) }
describe '.active_participants' do
......
......@@ -133,6 +133,46 @@ RSpec.describe Members::DestroyService do
end
end
end
context 'user escalation rules' do
let(:project) { create(:project, group: group) }
let(:project_2) { create(:project, group: group) }
let(:project_1_policy) { create(:incident_management_escalation_policy, project: project) }
let(:project_2_policy) { create(:incident_management_escalation_policy, project: project_2) }
let!(:project_1_rule) { create(:incident_management_escalation_rule, :with_user, user: member_user, policy: project_1_policy) }
let!(:project_2_rule) { create(:incident_management_escalation_rule, :with_user, user: member_user, policy: project_2_policy) }
shared_examples_for 'calls the destroy service' do |scope, *rules|
let(:rules_to_delete) { rules.map { |rule_name| send(rule_name) } }
let(:rules_to_preserve) { IncidentManagement::EscalationRule.all - rules_to_delete }
it "calls the destroy service #{scope}" do
expect(IncidentManagement::EscalationRules::DestroyService)
.to receive(:new)
.with({ escalation_rules: rules_to_delete, user: member_user })
.and_call_original
subject.execute(member)
rules_to_delete.each { |rule| expect { rule.reload }.to raise_error(ActiveRecord::RecordNotFound) }
rules_to_preserve.each { |rule| expect { rule.reload }.not_to raise_error }
end
end
context 'group member is removed' do
let(:other_user) { create(:user, developer_projects: [group]) }
let!(:other_user_rule) { create(:incident_management_escalation_rule, :with_user, user: other_user, policy: project_1_policy) }
let!(:other_namespace_rule) { create(:incident_management_escalation_rule, :with_user, user: member_user) }
include_examples 'calls the destroy service', 'with rules each project in the group', :project_1_rule, :project_2_rule
end
context 'project member is removed' do
let!(:member) { create(:project_member, source: project, user: member_user) }
include_examples 'calls the destroy service', 'with rules for the project', :project_1_rule
end
end
end
context 'when current user is not present' do # ie, when the system initiates the destroy
......
......@@ -895,7 +895,7 @@ RSpec.describe EE::NotificationService, :mailer do
end
end
context 'IncidentManagement::Oncall' do
context 'IncidentManagement' do
let_it_be(:user) { create(:user) }
describe '#notify_oncall_users_of_alert' do
......@@ -939,5 +939,51 @@ RSpec.describe EE::NotificationService, :mailer do
expect { subject.oncall_user_removed(rotation, user, false) }.to change(ActionMailer::Base.deliveries, :size).by(2)
end
end
describe '#user_escalation_rule_deleted' do
let(:project) { create(:project) }
let(:user) { create(:user) }
let(:rules) { [rule_1, rule_2] }
let!(:rule_1) { create(:incident_management_escalation_rule, :with_user, project: project, user: user) }
let!(:rule_2) { create(:incident_management_escalation_rule, :with_user, :resolved, project: project, user: user) }
it 'immediately sends an email to the project owner' do
expect(Notify).to receive(:user_escalation_rule_deleted_email).with(user, project, rules, project.owner).once.and_call_original
expect(Notify).not_to receive(:user_escalation_rule_deleted_email).with(user, project, rules, user)
expect { subject.user_escalation_rule_deleted(project, user, rules) }.to change(ActionMailer::Base.deliveries, :size).by(1)
end
context 'when project owner is the removed user' do
let(:user) { project.owner }
it 'does not send an email' do
expect(Notify).not_to receive(:user_escalation_rule_deleted_email)
subject.user_escalation_rule_deleted(project, user, rules)
end
end
context 'with a group' do
let(:group) { create(:group) }
let(:project) { create(:project, group: group) }
let(:owner) { create(:user) }
let(:blocked_owner) { create(:user, :blocked) }
before do
group.add_owner(owner)
group.add_owner(blocked_owner)
group.add_owner(user)
end
it 'immediately sends an email to the eligable project owners' do
expect(Notify).to receive(:user_escalation_rule_deleted_email).with(user, project, rules, owner).once.and_call_original
expect(Notify).not_to receive(:user_escalation_rule_deleted_email).with(user, project, rules, blocked_owner)
expect(Notify).not_to receive(:user_escalation_rule_deleted_email).with(user, project, rules, user)
expect { subject.user_escalation_rule_deleted(project, user, rules) }.to change(ActionMailer::Base.deliveries, :size).by(1)
end
end
end
end
end
......@@ -81,6 +81,32 @@ RSpec.describe Users::DestroyService do
end
end
context 'when user has escalation rules' do
let(:project) { create(:project) }
let(:user) { project.owner }
let(:project_policy) { create(:incident_management_escalation_policy, project: project) }
let!(:project_rule) { create(:incident_management_escalation_rule, :with_user, policy: project_policy, user: user) }
let(:group) { create(:group) }
let(:group_project) { create(:project, group: group) }
let(:group_policy) { create(:incident_management_escalation_policy, project: group_project) }
let!(:group_rule) { create(:incident_management_escalation_rule, :with_user, policy: group_policy, user: user) }
let!(:group_owner) { create(:user) }
before do
group.add_developer(user)
group.add_owner(group_owner)
end
it 'deletes the escalation rules and notifies owners of group projects' do
expect { operation }.to change(ActionMailer::Base.deliveries, :size).by(1)
expect { project.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect { project_rule.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect { group_rule.reload }.to raise_error(ActiveRecord::RecordNotFound)
end
end
describe 'audit events' do
include_examples 'audit event logging' do
let(:fail_condition!) do
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe IncidentManagement::EscalationRules::DestroyService do
let!(:user) { create(:user) }
let(:project) { create(:project) }
let(:policy) { create(:incident_management_escalation_policy, project: project) }
let!(:rule_1) { create(:incident_management_escalation_rule, :with_user, user: user, policy: policy) }
let!(:rule_2) { create(:incident_management_escalation_rule, :with_user, :resolved, user: user, policy: policy) }
let!(:excluded_rule) { create(:incident_management_escalation_rule, :with_user, user: user, policy: policy, elapsed_time_seconds: 60) }
let(:other_project) { create(:project) }
let(:other_policy) { create(:incident_management_escalation_policy, project: other_project) }
let!(:other_project_rule) { create(:incident_management_escalation_rule, :with_user, user: user, policy: other_policy) }
let(:escalation_rules) { IncidentManagement::EscalationRule.id_in([rule_1, rule_2, other_project_rule]) }
let(:notification_service) { NotificationService.new }
let(:service) { described_class.new(escalation_rules: escalation_rules, user: user) }
subject(:execute) { service.execute }
before do
stub_licensed_features(oncall_schedules: true, escalation_policies: true)
end
it 'sends an email for each project and deletes the provided escalation rules' do
allow(NotificationService).to receive(:new).and_return(notification_service)
expect(notification_service).to receive(:user_escalation_rule_deleted).with(project, user, [rule_1, rule_2]).once
expect(notification_service).to receive(:user_escalation_rule_deleted).with(other_project, user, [other_project_rule]).once
execute
expect { rule_1.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect { rule_2.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect { other_project_rule.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect { excluded_rule.reload }.not_to raise_error
end
end
......@@ -838,6 +838,9 @@ msgstr ""
msgid "%{placeholder} is not a valid theme"
msgstr ""
msgid "%{policy_link} (notifying after %{elapsed_time} minutes unless %{status})"
msgstr ""
msgid "%{primary} (%{secondary})"
msgstr ""
......@@ -1035,6 +1038,12 @@ msgstr ""
msgid "%{user_name} (%{user_username}) was removed from %{rotation} in %{schedule} in %{project}. "
msgstr ""
msgid "%{user_name} (%{user_username}) was removed from the following escalation policies in %{project_link}: "
msgstr ""
msgid "%{user_name} (%{user_username}) was removed from the following escalation policies in %{project}:"
msgstr ""
msgid "%{user_name} profile page"
msgstr ""
......@@ -1218,6 +1227,9 @@ msgstr ""
msgid ", or "
msgstr ""
msgid "- %{policy_name} (notifying after %{elapsed_time} minutes unless %{status})"
msgstr ""
msgid "- Available to run jobs."
msgstr ""
......@@ -26198,6 +26210,12 @@ msgstr ""
msgid "Please refer to %{docs_url}"
msgstr ""
msgid "Please review the updated escalation policies for %{project_link}. It is recommended that you reach out to the current on-call responder to ensure continuity of on-call coverage."
msgstr ""
msgid "Please review the updated escalation policies for %{project}. It is recommended that you reach out to the current on-call responder to ensure continuity of on-call coverage."
msgstr ""
msgid "Please select"
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