Commit b128cd0b authored by Adam Hegyi's avatar Adam Hegyi

Merge branch 'sy-add-user-to-escalation-rules' into 'master'

Allow user to be assigned to escalation rule

See merge request gitlab-org/gitlab!65636
parents b05e99d5 aa498b70
# frozen_string_literal: true
class RemoveNullConstraintOnScheduleFromEscalationRules < ActiveRecord::Migration[6.1]
def up
change_column_null :incident_management_escalation_rules, :oncall_schedule_id, true
end
def down
exec_query 'DELETE FROM incident_management_escalation_rules WHERE oncall_schedule_id IS NULL'
change_column_null :incident_management_escalation_rules, :oncall_schedule_id, false
end
end
# frozen_string_literal: true
class AddUserToEscalationRules < ActiveRecord::Migration[6.1]
include Gitlab::Database::MigrationHelpers
def up
with_lock_retries do
add_column :incident_management_escalation_rules, :user_id, :bigint, null: true
end
end
def down
with_lock_retries do
remove_column :incident_management_escalation_rules, :user_id
end
end
end
# frozen_string_literal: true
class AddUserIndexToEscalationRules < ActiveRecord::Migration[6.1]
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
USER_INDEX_NAME = 'index_escalation_rules_on_user'
OLD_UNIQUE_INDEX_NAME = 'index_on_policy_schedule_status_elapsed_time_escalation_rules'
NEW_UNIQUE_INDEX_NAME = 'index_escalation_rules_on_all_attributes'
def up
remove_concurrent_index_by_name :incident_management_escalation_rules, OLD_UNIQUE_INDEX_NAME
add_concurrent_index :incident_management_escalation_rules, :user_id, name: USER_INDEX_NAME
add_concurrent_index :incident_management_escalation_rules,
[:policy_id, :oncall_schedule_id, :status, :elapsed_time_seconds, :user_id],
unique: true,
name: NEW_UNIQUE_INDEX_NAME
end
def down
remove_concurrent_index_by_name :incident_management_escalation_rules, USER_INDEX_NAME
remove_concurrent_index_by_name :incident_management_escalation_rules, NEW_UNIQUE_INDEX_NAME
exec_query 'DELETE FROM incident_management_escalation_rules WHERE oncall_schedule_id IS NULL'
add_concurrent_index :incident_management_escalation_rules,
[:policy_id, :oncall_schedule_id, :status, :elapsed_time_seconds],
unique: true,
name: OLD_UNIQUE_INDEX_NAME
end
end
# frozen_string_literal: true
class AddUserFkToEscalationRules < ActiveRecord::Migration[6.1]
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
def up
add_concurrent_foreign_key :incident_management_escalation_rules, :users, column: :user_id, on_delete: :cascade
end
def down
with_lock_retries do
remove_foreign_key_if_exists :incident_management_escalation_rules, column: :user_id
end
end
end
# frozen_string_literal: true
class AddXorCheckConstraintForEscalationRules < ActiveRecord::Migration[6.1]
include Gitlab::Database::MigrationHelpers
disable_ddl_transaction!
CONSTRAINT_NAME = 'escalation_rules_one_of_oncall_schedule_or_user'
def up
add_check_constraint :incident_management_escalation_rules, 'num_nonnulls(oncall_schedule_id, user_id) = 1', CONSTRAINT_NAME
end
def down
remove_check_constraint :incident_management_escalation_rules, CONSTRAINT_NAME
end
end
cf276b9aa97fc7857499e1b103a8e09eda329a4db92d0e653cc6f7128987be39
\ No newline at end of file
5c6aff5b43a1e81e84a42f008a8a1ab90c77ee450884aa1ecc86bce551424f43
\ No newline at end of file
d49b1f48c2fa1cac8d7793f8bb025792f4bb85eed787ba3abdbaa4647523b70a
\ No newline at end of file
eab0f8488b0122ec6c5625c66ebcbd221579bdd9cc2cf670d1f26181709f23b7
\ No newline at end of file
a7a6697d86b71d59104af35a9d7d6f3caebf4ee1252e4f3e52133afb3f642e48
\ No newline at end of file
...@@ -13941,10 +13941,12 @@ ALTER SEQUENCE incident_management_escalation_policies_id_seq OWNED BY incident_ ...@@ -13941,10 +13941,12 @@ ALTER SEQUENCE incident_management_escalation_policies_id_seq OWNED BY incident_
CREATE TABLE incident_management_escalation_rules ( CREATE TABLE incident_management_escalation_rules (
id bigint NOT NULL, id bigint NOT NULL,
policy_id bigint NOT NULL, policy_id bigint NOT NULL,
oncall_schedule_id bigint NOT NULL, oncall_schedule_id bigint,
status smallint NOT NULL, status smallint NOT NULL,
elapsed_time_seconds integer NOT NULL, elapsed_time_seconds integer NOT NULL,
is_removed boolean DEFAULT false NOT NULL is_removed boolean DEFAULT false NOT NULL,
user_id bigint,
CONSTRAINT escalation_rules_one_of_oncall_schedule_or_user CHECK ((num_nonnulls(oncall_schedule_id, user_id) = 1))
); );
CREATE SEQUENCE incident_management_escalation_rules_id_seq CREATE SEQUENCE incident_management_escalation_rules_id_seq
...@@ -23723,6 +23725,10 @@ CREATE INDEX index_esc_protected_branches_on_external_status_check_id ON externa ...@@ -23723,6 +23725,10 @@ CREATE INDEX index_esc_protected_branches_on_external_status_check_id ON externa
CREATE INDEX index_esc_protected_branches_on_protected_branch_id ON external_status_checks_protected_branches USING btree (protected_branch_id); CREATE INDEX index_esc_protected_branches_on_protected_branch_id ON external_status_checks_protected_branches USING btree (protected_branch_id);
CREATE UNIQUE INDEX index_escalation_rules_on_all_attributes ON incident_management_escalation_rules USING btree (policy_id, oncall_schedule_id, status, elapsed_time_seconds, user_id);
CREATE INDEX index_escalation_rules_on_user ON incident_management_escalation_rules USING btree (user_id);
CREATE INDEX index_events_on_action ON events USING btree (action); CREATE INDEX index_events_on_action ON events USING btree (action);
CREATE INDEX index_events_on_author_id_and_created_at ON events USING btree (author_id, created_at); CREATE INDEX index_events_on_author_id_and_created_at ON events USING btree (author_id, created_at);
...@@ -24441,8 +24447,6 @@ CREATE INDEX index_on_oncall_schedule_escalation_rule ON incident_management_esc ...@@ -24441,8 +24447,6 @@ CREATE INDEX index_on_oncall_schedule_escalation_rule ON incident_management_esc
CREATE INDEX index_on_pages_metadata_not_migrated ON project_pages_metadata USING btree (project_id) WHERE ((deployed = true) AND (pages_deployment_id IS NULL)); CREATE INDEX index_on_pages_metadata_not_migrated ON project_pages_metadata USING btree (project_id) WHERE ((deployed = true) AND (pages_deployment_id IS NULL));
CREATE UNIQUE INDEX index_on_policy_schedule_status_elapsed_time_escalation_rules ON incident_management_escalation_rules USING btree (policy_id, oncall_schedule_id, status, elapsed_time_seconds);
CREATE UNIQUE INDEX index_on_project_id_escalation_policy_name_unique ON incident_management_escalation_policies USING btree (project_id, name); CREATE UNIQUE INDEX index_on_project_id_escalation_policy_name_unique ON incident_management_escalation_policies USING btree (project_id, name);
CREATE INDEX index_on_projects_lower_path ON projects USING btree (lower((path)::text)); CREATE INDEX index_on_projects_lower_path ON projects USING btree (lower((path)::text));
...@@ -25895,6 +25899,9 @@ ALTER TABLE ONLY epics ...@@ -25895,6 +25899,9 @@ ALTER TABLE ONLY epics
ALTER TABLE ONLY clusters_applications_runners ALTER TABLE ONLY clusters_applications_runners
ADD CONSTRAINT fk_02de2ded36 FOREIGN KEY (runner_id) REFERENCES ci_runners(id) ON DELETE SET NULL; ADD CONSTRAINT fk_02de2ded36 FOREIGN KEY (runner_id) REFERENCES ci_runners(id) ON DELETE SET NULL;
ALTER TABLE ONLY incident_management_escalation_rules
ADD CONSTRAINT fk_0314ee86eb FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
ALTER TABLE ONLY design_management_designs_versions ALTER TABLE ONLY design_management_designs_versions
ADD CONSTRAINT fk_03c671965c FOREIGN KEY (design_id) REFERENCES design_management_designs(id) ON DELETE CASCADE; ADD CONSTRAINT fk_03c671965c FOREIGN KEY (design_id) REFERENCES design_management_designs(id) ON DELETE CASCADE;
...@@ -9219,6 +9219,7 @@ Represents an escalation rule for an escalation policy. ...@@ -9219,6 +9219,7 @@ Represents an escalation rule for an escalation policy.
| <a id="escalationruletypeid"></a>`id` | [`IncidentManagementEscalationRuleID`](#incidentmanagementescalationruleid) | ID of the escalation policy. | | <a id="escalationruletypeid"></a>`id` | [`IncidentManagementEscalationRuleID`](#incidentmanagementescalationruleid) | ID of the escalation policy. |
| <a id="escalationruletypeoncallschedule"></a>`oncallSchedule` | [`IncidentManagementOncallSchedule`](#incidentmanagementoncallschedule) | The on-call schedule to notify. | | <a id="escalationruletypeoncallschedule"></a>`oncallSchedule` | [`IncidentManagementOncallSchedule`](#incidentmanagementoncallschedule) | The on-call schedule to notify. |
| <a id="escalationruletypestatus"></a>`status` | [`EscalationRuleStatus`](#escalationrulestatus) | The status required to prevent the rule from activating. | | <a id="escalationruletypestatus"></a>`status` | [`EscalationRuleStatus`](#escalationrulestatus) | The status required to prevent the rule from activating. |
| <a id="escalationruletypeuser"></a>`user` | [`UserCore`](#usercore) | The user to notify. |
### `Event` ### `Event`
...@@ -16737,8 +16738,9 @@ Represents an escalation rule. ...@@ -16737,8 +16738,9 @@ Represents an escalation rule.
| Name | Type | Description | | Name | Type | Description |
| ---- | ---- | ----------- | | ---- | ---- | ----------- |
| <a id="escalationruleinputelapsedtimeseconds"></a>`elapsedTimeSeconds` | [`Int!`](#int) | The time in seconds before the rule is activated. | | <a id="escalationruleinputelapsedtimeseconds"></a>`elapsedTimeSeconds` | [`Int!`](#int) | The time in seconds before the rule is activated. |
| <a id="escalationruleinputoncallscheduleiid"></a>`oncallScheduleIid` | [`ID!`](#id) | The on-call schedule to notify. | | <a id="escalationruleinputoncallscheduleiid"></a>`oncallScheduleIid` | [`ID`](#id) | The on-call schedule to notify. |
| <a id="escalationruleinputstatus"></a>`status` | [`EscalationRuleStatus!`](#escalationrulestatus) | The status required to prevent the rule from activating. | | <a id="escalationruleinputstatus"></a>`status` | [`EscalationRuleStatus!`](#escalationrulestatus) | The status required to prevent the rule from activating. |
| <a id="escalationruleinputusername"></a>`username` | [`String`](#string) | The username of the user to notify. |
### `JiraUsersMappingInputType` ### `JiraUsersMappingInputType`
......
...@@ -43,23 +43,42 @@ module Mutations ...@@ -43,23 +43,42 @@ module Mutations
def prepare_rules_attributes(project, args) def prepare_rules_attributes(project, args)
return args unless rules = args.delete(:rules) return args unless rules = args.delete(:rules)
iids = rules.collect { |rule| rule[:oncall_schedule_iid] } schedules = find_schedules(project, rules)
found_schedules = schedules_for_iids(project, iids) users = find_users(rules)
rules_attributes = rules.map { |rule| prepare_rule(found_schedules, rule.to_h) } rules_attributes = rules.map { |rule| prepare_rule(rule.to_h, schedules, users) }
args.merge(rules_attributes: rules_attributes) args.merge(rules_attributes: rules_attributes)
end end
def prepare_rule(schedules, rule) def prepare_rule(rule, schedules, users)
iid = rule.delete(:oncall_schedule_iid).to_i iid = rule.delete(:oncall_schedule_iid).to_i
username = rule.delete(:username)
rule.merge(oncall_schedule: schedules[iid]) rule.merge(
oncall_schedule: schedules[iid],
user: users[username]
)
end end
def schedules_for_iids(project, iids) def find_schedules(project, rules)
schedules = ::IncidentManagement::OncallSchedulesFinder.new(current_user, project, iid: iids).execute find_resource(rules, :oncall_schedule_iid) do |iids|
::IncidentManagement::OncallSchedulesFinder.new(current_user, project, iid: iids).execute.index_by(&:iid)
end
end
def find_users(rules)
find_resource(rules, :username) do |usernames|
UsersFinder.new(current_user, username: usernames).execute.index_by(&:username)
end
end
def find_resource(rules, attribute)
identifiers = rules.collect { |rule| rule[attribute] }.uniq.compact
resources = yield(identifiers)
return resources if resources.length == identifiers.length
schedules.index_by(&:iid) raise_resource_not_available_error!
end end
end end
end end
......
...@@ -8,7 +8,11 @@ module Types ...@@ -8,7 +8,11 @@ module Types
argument :oncall_schedule_iid, GraphQL::Types::ID, # rubocop: disable Graphql/IDType argument :oncall_schedule_iid, GraphQL::Types::ID, # rubocop: disable Graphql/IDType
description: 'The on-call schedule to notify.', description: 'The on-call schedule to notify.',
required: true required: false
argument :username, GraphQL::Types::String,
description: 'The username of the user to notify.',
required: false
argument :elapsed_time_seconds, GraphQL::Types::Int, argument :elapsed_time_seconds, GraphQL::Types::Int,
description: 'The time in seconds before the rule is activated.', description: 'The time in seconds before the rule is activated.',
...@@ -17,6 +21,18 @@ module Types ...@@ -17,6 +21,18 @@ module Types
argument :status, Types::IncidentManagement::EscalationRuleStatusEnum, argument :status, Types::IncidentManagement::EscalationRuleStatusEnum,
description: 'The status required to prevent the rule from activating.', description: 'The status required to prevent the rule from activating.',
required: true required: true
def prepare
unless schedule_iid_or_username
raise Gitlab::Graphql::Errors::ArgumentError, 'One of oncall_schedule_iid or username must be provided'
end
super
end
def schedule_iid_or_username
oncall_schedule_iid.present? ^ username.present?
end
end end
end end
end end
...@@ -15,6 +15,10 @@ module Types ...@@ -15,6 +15,10 @@ module Types
null: true, null: true,
description: 'The on-call schedule to notify.' description: 'The on-call schedule to notify.'
field :user, Types::UserType,
null: true,
description: 'The user to notify.'
field :elapsed_time_seconds, GraphQL::Types::Int, field :elapsed_time_seconds, GraphQL::Types::Int,
null: true, null: true,
description: 'The time in seconds before the rule is activated.' description: 'The time in seconds before the rule is activated.'
......
...@@ -11,7 +11,6 @@ module IncidentManagement ...@@ -11,7 +11,6 @@ module IncidentManagement
validates :project_id, uniqueness: { message: _('can only have one escalation policy') }, on: :create validates :project_id, uniqueness: { message: _('can only have one escalation policy') }, on: :create
validates :name, presence: true, uniqueness: { scope: [:project_id] }, length: { maximum: 72 } validates :name, presence: true, uniqueness: { scope: [:project_id] }, length: { maximum: 72 }
validates :description, length: { maximum: 160 } validates :description, length: { maximum: 160 }
validates :rules, presence: true
accepts_nested_attributes_for :rules accepts_nested_attributes_for :rules
end end
......
...@@ -5,19 +5,35 @@ module IncidentManagement ...@@ -5,19 +5,35 @@ module IncidentManagement
self.table_name = 'incident_management_escalation_rules' self.table_name = 'incident_management_escalation_rules'
belongs_to :policy, class_name: 'EscalationPolicy', inverse_of: 'rules', foreign_key: 'policy_id' belongs_to :policy, class_name: 'EscalationPolicy', inverse_of: 'rules', foreign_key: 'policy_id'
belongs_to :oncall_schedule, class_name: 'OncallSchedule', inverse_of: 'rotations', foreign_key: 'oncall_schedule_id' belongs_to :oncall_schedule, class_name: 'OncallSchedule', foreign_key: 'oncall_schedule_id', optional: true
belongs_to :user, optional: true
enum status: AlertManagement::Alert::STATUSES.slice(:acknowledged, :resolved) enum status: AlertManagement::Alert::STATUSES.slice(:acknowledged, :resolved)
validates :status, presence: true validates :status, presence: true
validates :oncall_schedule, presence: true
validates :elapsed_time_seconds, validates :elapsed_time_seconds,
presence: true, presence: true,
numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than_or_equal_to: 24.hours } numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than_or_equal_to: 24.hours }
validates :policy_id, uniqueness: { scope: [:oncall_schedule_id, :status, :elapsed_time_seconds], message: _('must have a unique schedule, status, and elapsed time') } validate :schedule_or_rule_present
validates :oncall_schedule_id,
uniqueness: { scope: [:policy_id, :status, :elapsed_time_seconds],
message: _('must be unique by status and elapsed time within a policy') },
allow_nil: true
validates :user_id,
uniqueness: { scope: [:policy_id, :status, :elapsed_time_seconds],
message: _('must be unique by status and elapsed time within a policy') },
allow_nil: true
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) }
private
def schedule_or_rule_present
unless oncall_schedule.present? ^ user.present?
errors.add(:base, 'must have either an on-call schedule or user')
end
end
end end
end end
...@@ -14,7 +14,13 @@ module IncidentManagement ...@@ -14,7 +14,13 @@ module IncidentManagement
end end
def invalid_schedules? def invalid_schedules?
params[:rules_attributes]&.any? { |attrs| attrs[:oncall_schedule]&.project != project } params[:rules_attributes]&.any? { |attrs| attrs[:oncall_schedule] && attrs[:oncall_schedule].project != project }
end
def users_without_permissions?
DeclarativePolicy.subject_scope do
params[:rules_attributes]&.any? { |attrs| attrs[:user] && !attrs[:user].can?(:read_project, project) }
end
end end
def error(message) def error(message)
...@@ -38,7 +44,11 @@ module IncidentManagement ...@@ -38,7 +44,11 @@ module IncidentManagement
end end
def error_bad_schedules def error_bad_schedules
error(_('All escalations rules must have a schedule in the same project as the policy')) error(_('Schedule-based escalation rules must have a schedule in the same project as the policy'))
end
def error_user_without_permission
error(_('User-based escalation rules must have a user with access to the project'))
end end
def error_in_save(policy) def error_in_save(policy)
......
...@@ -23,6 +23,7 @@ module IncidentManagement ...@@ -23,6 +23,7 @@ module IncidentManagement
return error_no_rules if params[:rules_attributes].blank? return error_no_rules if params[:rules_attributes].blank?
return error_too_many_rules if too_many_rules? return error_too_many_rules if too_many_rules?
return error_bad_schedules if invalid_schedules? return error_bad_schedules if invalid_schedules?
return error_user_without_permission if users_without_permissions?
escalation_policy = project.incident_management_escalation_policies.create(params) escalation_policy = project.incident_management_escalation_policies.create(params)
......
...@@ -28,6 +28,7 @@ module IncidentManagement ...@@ -28,6 +28,7 @@ module IncidentManagement
return error_no_rules if empty_rules? return error_no_rules if empty_rules?
return error_too_many_rules if too_many_rules? return error_too_many_rules if too_many_rules?
return error_bad_schedules if invalid_schedules? return error_bad_schedules if invalid_schedules?
return error_user_without_permission if users_without_permissions?
reconcile_rules! reconcile_rules!
...@@ -85,7 +86,7 @@ module IncidentManagement ...@@ -85,7 +86,7 @@ module IncidentManagement
end end
def unique_id(rule) def unique_id(rule)
rule.slice(:oncall_schedule_id, :elapsed_time_seconds, :status) rule.slice(:oncall_schedule_id, :user_id, :elapsed_time_seconds, :status)
end end
end end
end end
......
...@@ -55,8 +55,12 @@ module IncidentManagement ...@@ -55,8 +55,12 @@ module IncidentManagement
def oncall_notification_recipients def oncall_notification_recipients
strong_memoize(:oncall_notification_recipients) do strong_memoize(:oncall_notification_recipients) do
::IncidentManagement::OncallUsersFinder.new(project, schedule: rule.oncall_schedule).execute.to_a rule.user_id ? [rule.user] : schedule_recipients
end
end end
def schedule_recipients
::IncidentManagement::OncallUsersFinder.new(project, schedule: rule.oncall_schedule).execute.to_a
end end
def destroy_escalation! def destroy_escalation!
......
...@@ -2,7 +2,7 @@ ...@@ -2,7 +2,7 @@
FactoryBot.define do FactoryBot.define do
factory :incident_management_escalation_rule, class: 'IncidentManagement::EscalationRule' do factory :incident_management_escalation_rule, class: 'IncidentManagement::EscalationRule' do
policy { association :incident_management_escalation_policy } policy { association :incident_management_escalation_policy, rule_count: 0 }
oncall_schedule { association :incident_management_oncall_schedule, project: policy.project } oncall_schedule { association :incident_management_oncall_schedule, project: policy.project }
status { IncidentManagement::EscalationRule.statuses[:acknowledged] } status { IncidentManagement::EscalationRule.statuses[:acknowledged] }
elapsed_time_seconds { 5.minutes } elapsed_time_seconds { 5.minutes }
...@@ -15,5 +15,10 @@ FactoryBot.define do ...@@ -15,5 +15,10 @@ FactoryBot.define do
trait :removed do trait :removed do
is_removed { true } is_removed { true }
end end
trait :with_user do
oncall_schedule {}
user { association :user, developer_projects: [policy.project] }
end
end end
end end
...@@ -19,7 +19,7 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Create do ...@@ -19,7 +19,7 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Create do
status: ::IncidentManagement::EscalationRule.statuses[:acknowledged] status: ::IncidentManagement::EscalationRule.statuses[:acknowledged]
}, },
{ {
oncall_schedule_iid: oncall_schedule.iid, username: current_user&.username,
elapsed_time_seconds: 600, elapsed_time_seconds: 600,
status: ::IncidentManagement::EscalationRule.statuses[:resolved] status: ::IncidentManagement::EscalationRule.statuses[:resolved]
} }
...@@ -71,8 +71,8 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Create do ...@@ -71,8 +71,8 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Create do
expect(rules.size).to eq(2) expect(rules.size).to eq(2)
expect(rules).to match_array([ expect(rules).to match_array([
have_attributes(oncall_schedule_id: oncall_schedule.id, elapsed_time_seconds: 300, status: 'acknowledged'), have_attributes(oncall_schedule_id: oncall_schedule.id, user: nil, elapsed_time_seconds: 300, status: 'acknowledged'),
have_attributes(oncall_schedule_id: oncall_schedule.id, elapsed_time_seconds: 600, status: 'resolved') have_attributes(oncall_schedule_id: nil, user: current_user, elapsed_time_seconds: 600, status: 'resolved')
]) ])
end end
...@@ -91,7 +91,7 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Create do ...@@ -91,7 +91,7 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Create do
args[:rules][0][:oncall_schedule_iid] = other_schedule.iid args[:rules][0][:oncall_schedule_iid] = other_schedule.iid
end end
it_behaves_like 'returns a GraphQL error', 'All escalations rules must have a schedule in the same project as the policy' it_behaves_like 'raises a resource not available error'
context 'user does not have permission for project' do context 'user does not have permission for project' do
before do before do
...@@ -101,6 +101,14 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Create do ...@@ -101,6 +101,14 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Create do
it_behaves_like 'raises a resource not available error' it_behaves_like 'raises a resource not available error'
end end
end end
context 'user for rule does not exist' do
before do
args[:rules][1][:username] = 'junk-username'
end
it_behaves_like 'raises a resource not available error'
end
end end
context 'user does not have permission for project' do context 'user does not have permission for project' do
......
...@@ -110,6 +110,7 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Update do ...@@ -110,6 +110,7 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Update do
context 'with rule updates' do context 'with rule updates' do
let(:oncall_schedule_iid) { oncall_schedule.iid } let(:oncall_schedule_iid) { oncall_schedule.iid }
let(:username) { reporter.username }
let(:rule_args) do let(:rule_args) do
[ [
{ {
...@@ -119,6 +120,13 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Update do ...@@ -119,6 +120,13 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Update do
}, },
{ {
oncall_schedule_iid: oncall_schedule_iid, oncall_schedule_iid: oncall_schedule_iid,
username: nil,
elapsed_time_seconds: 800,
status: :acknowledged
},
{
oncall_schedule_iid: nil,
username: username,
elapsed_time_seconds: 800, elapsed_time_seconds: 800,
status: :acknowledged status: :acknowledged
} }
...@@ -128,22 +136,17 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Update do ...@@ -128,22 +136,17 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Update do
let(:expected_rules) do let(:expected_rules) do
[ [
first_rule, first_rule,
have_attributes(oncall_schedule_id: oncall_schedule.id, elapsed_time_seconds: 800, status: 'acknowledged') have_attributes(oncall_schedule_id: oncall_schedule.id, user: nil, elapsed_time_seconds: 800, status: 'acknowledged'),
have_attributes(oncall_schedule_id: nil, user: reporter, elapsed_time_seconds: 800, status: 'acknowledged')
] ]
end end
it_behaves_like 'successful update with no errors' it_behaves_like 'successful update with no errors'
context 'when schedule does not exist' do context 'when schedule does not exist' do
let(:error_message) { eq("The oncall schedule for iid #{non_existing_record_iid} could not be found") }
let(:oncall_schedule_iid) { non_existing_record_iid } let(:oncall_schedule_iid) { non_existing_record_iid }
it 'returns errors in the body of the response' do it_behaves_like 'failed update with a top-level access error'
expect(resolve).to eq(
escalation_policy: nil,
errors: ['All escalations rules must have a schedule in the same project as the policy']
)
end
context 'the user does not have permission to update policies regardless' do context 'the user does not have permission to update policies regardless' do
let(:current_user) { reporter } let(:current_user) { reporter }
...@@ -151,6 +154,12 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Update do ...@@ -151,6 +154,12 @@ RSpec.describe Mutations::IncidentManagement::EscalationPolicy::Update do
it_behaves_like 'failed update with a top-level access error' it_behaves_like 'failed update with a top-level access error'
end end
end end
context "when rule's user does not exist" do
let(:username) { 'invalid-username' }
it_behaves_like 'failed update with a top-level access error'
end
end end
end end
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe GitlabSchema.types['EscalationRuleInput'] do
context 'mutually exclusive arguments' do
let(:input) do
{
oncall_schedule_iid: schedule_iid,
username: username,
elapsed_time_seconds: 0,
status: 'RESOLVED'
}
end
let(:output) { input.merge(status: 'resolved', oncall_schedule_iid: schedule_iid&.to_s) }
let(:schedule_iid) {}
let(:username) {}
subject { described_class.coerce_isolated_input(input).to_h }
context 'with neither username nor schedule provided' do
specify { expect { subject }.to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'One of oncall_schedule_iid or username must be provided') }
end
context 'with both username and schedule provided' do
let(:schedule_iid) { 3 }
let(:username) { 'username' }
specify { expect { subject }.to raise_error(Gitlab::Graphql::Errors::ArgumentError, 'One of oncall_schedule_iid or username must be provided') }
end
context 'with only on-call schedule provided' do
let(:schedule_iid) { 3 }
it { is_expected.to eq(output) }
end
context 'with only user schedule provided' do
let(:username) { 'username' }
it { is_expected.to eq(output) }
end
end
it 'has specific fields' do
allowed_args = %w(oncallScheduleIid username elapsedTimeSeconds status)
expect(described_class.arguments.keys).to include(*allowed_args)
end
end
...@@ -9,6 +9,7 @@ RSpec.describe GitlabSchema.types['EscalationRuleType'] do ...@@ -9,6 +9,7 @@ RSpec.describe GitlabSchema.types['EscalationRuleType'] do
expected_fields = %i[ expected_fields = %i[
id id
oncall_schedule oncall_schedule
user
elapsed_time_seconds elapsed_time_seconds
status status
] ]
......
...@@ -25,7 +25,6 @@ RSpec.describe IncidentManagement::EscalationPolicy do ...@@ -25,7 +25,6 @@ RSpec.describe IncidentManagement::EscalationPolicy do
describe 'validations' do describe 'validations' do
it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_presence_of(:name) }
it { is_expected.to validate_presence_of(:rules) }
it { is_expected.to validate_uniqueness_of(:project_id).with_message(/can only have one escalation policy/).on(:create) } it { is_expected.to validate_uniqueness_of(:project_id).with_message(/can only have one escalation policy/).on(:create) }
it { is_expected.to validate_uniqueness_of(:name).scoped_to(:project_id) } it { is_expected.to validate_uniqueness_of(:name).scoped_to(:project_id) }
it { is_expected.to validate_length_of(:name).is_at_most(72) } it { is_expected.to validate_length_of(:name).is_at_most(72) }
......
...@@ -3,27 +3,54 @@ ...@@ -3,27 +3,54 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe IncidentManagement::EscalationRule do RSpec.describe IncidentManagement::EscalationRule do
let_it_be(:policy) { create(:incident_management_escalation_policy) } subject { build(:incident_management_escalation_rule) }
subject { build(:incident_management_escalation_rule, policy: policy) }
it { is_expected.to be_valid }
describe 'associations' do describe 'associations' do
it { is_expected.to belong_to(:policy) } it { is_expected.to belong_to(:policy) }
it { is_expected.to belong_to(:oncall_schedule) } it { is_expected.to belong_to(:oncall_schedule).optional }
it { is_expected.to belong_to(:user).optional }
end end
describe 'validations' do describe 'validations' do
it { is_expected.to be_valid }
it { is_expected.to validate_presence_of(:status) } it { is_expected.to validate_presence_of(:status) }
it { is_expected.to validate_presence_of(:elapsed_time_seconds) } it { is_expected.to validate_presence_of(:elapsed_time_seconds) }
it { is_expected.to validate_numericality_of(:elapsed_time_seconds).is_greater_than_or_equal_to(0).is_less_than_or_equal_to(24.hours) } it { is_expected.to validate_numericality_of(:elapsed_time_seconds).is_greater_than_or_equal_to(0).is_less_than_or_equal_to(24.hours) }
it { is_expected.to validate_uniqueness_of(:policy_id).scoped_to([:oncall_schedule_id, :status, :elapsed_time_seconds] ).with_message('must have a unique schedule, status, and elapsed time') } it { is_expected.to validate_uniqueness_of(:oncall_schedule_id).scoped_to([:policy_id, :status, :elapsed_time_seconds] ).with_message('must be unique by status and elapsed time within a policy') }
context 'user-based rules' do
subject { build(:incident_management_escalation_rule, :with_user) }
it { is_expected.to be_valid }
it { is_expected.to validate_uniqueness_of(:user_id).scoped_to([:policy_id, :status, :elapsed_time_seconds] ).with_message('must be unique by status and elapsed time within a policy') }
end
context 'mutually exclusive attributes' do
context 'when user and schedule are both provided' do
let_it_be(:schedule) { create(:incident_management_oncall_schedule) }
subject { build(:incident_management_escalation_rule, :with_user, oncall_schedule: schedule) }
specify do
expect(subject).to be_invalid
expect(subject.errors.messages[:base]).to eq(['must have either an on-call schedule or user'])
end
end
context 'neither user nor schedule are provided' do
subject { build(:incident_management_escalation_rule, oncall_schedule: nil) }
specify do
expect(subject).to be_invalid
expect(subject.errors.messages[:base]).to eq(['must have either an on-call schedule or user'])
end
end
end
end end
describe 'scopes' do describe 'scopes' do
let_it_be(:rule) { policy.rules.first } let_it_be(:rule) { create(:incident_management_escalation_rule) }
let_it_be(:removed_rule) { create(:incident_management_escalation_rule, :removed, policy: policy) } let_it_be(:removed_rule) { create(:incident_management_escalation_rule, :removed, policy: rule.policy) }
describe '.not_removed' do describe '.not_removed' do
subject { described_class.not_removed } subject { described_class.not_removed }
......
...@@ -118,7 +118,8 @@ RSpec.describe 'getting Incident Management escalation policies' do ...@@ -118,7 +118,8 @@ RSpec.describe 'getting Incident Management escalation policies' do
'name' => last_policy_rule.oncall_schedule.name, 'name' => last_policy_rule.oncall_schedule.name,
'description' => last_policy_rule.oncall_schedule.description, 'description' => last_policy_rule.oncall_schedule.description,
'timezone' => last_policy_rule.oncall_schedule.timezone 'timezone' => last_policy_rule.oncall_schedule.timezone
} },
'user' => nil
} }
] ]
) )
......
...@@ -10,6 +10,7 @@ RSpec.describe 'getting Incident Management escalation policies' do ...@@ -10,6 +10,7 @@ RSpec.describe 'getting Incident Management escalation policies' do
let_it_be(:policy) { create(:incident_management_escalation_policy, project: project) } let_it_be(:policy) { create(:incident_management_escalation_policy, project: project) }
let_it_be(:rule) { policy.rules.first } let_it_be(:rule) { policy.rules.first }
let_it_be(:schedule) { rule.oncall_schedule } let_it_be(:schedule) { rule.oncall_schedule }
let_it_be(:user_rule) { create(:incident_management_escalation_rule, :with_user, policy: policy) }
let(:params) { {} } let(:params) { {} }
...@@ -25,6 +26,9 @@ RSpec.describe 'getting Incident Management escalation policies' do ...@@ -25,6 +26,9 @@ RSpec.describe 'getting Incident Management escalation policies' do
iid iid
name name
} }
user {
username
}
} }
} }
QUERY QUERY
...@@ -49,15 +53,25 @@ RSpec.describe 'getting Incident Management escalation policies' do ...@@ -49,15 +53,25 @@ RSpec.describe 'getting Incident Management escalation policies' do
it 'includes expected data' do it 'includes expected data' do
post_graphql(query, current_user: current_user) post_graphql(query, current_user: current_user)
expect(escalation_rules_response).to eq([{ expect(escalation_rules_response).to eq([
{
'id' => global_id(rule), 'id' => global_id(rule),
'elapsedTimeSeconds' => rule.elapsed_time_seconds, # 5 min 'elapsedTimeSeconds' => rule.elapsed_time_seconds, # 5 min
'status' => rule.status.upcase, # 'ACKNOWLEDGED' 'status' => rule.status.upcase, # 'ACKNOWLEDGED'
'oncallSchedule' => { 'oncallSchedule' => {
'iid' => schedule.iid.to_s, 'iid' => schedule.iid.to_s,
'name' => schedule.name 'name' => schedule.name
},
'user' => nil
},
{
'id' => global_id(user_rule),
'elapsedTimeSeconds' => user_rule.elapsed_time_seconds, # 5 min
'status' => user_rule.status.upcase, # 'ACKNOWLEDGED'
'oncallSchedule' => nil,
'user' => { 'username' => user_rule.user.username }
} }
}]) ])
end end
context 'with multiple rules' do context 'with multiple rules' do
...@@ -68,10 +82,11 @@ RSpec.describe 'getting Incident Management escalation policies' do ...@@ -68,10 +82,11 @@ RSpec.describe 'getting Incident Management escalation policies' do
it 'orders rules by time and status' do it 'orders rules by time and status' do
post_graphql(query, current_user: current_user) post_graphql(query, current_user: current_user)
expect(escalation_rules_response.length).to eq(4) expect(escalation_rules_response.length).to eq(5)
expect(escalation_rules_response.map { |rule| rule['id'] }).to eq([ expect(escalation_rules_response.map { |rule| rule['id'] }).to eq([
global_id(earlier_resolved_rule), global_id(earlier_resolved_rule),
global_id(rule), global_id(rule),
global_id(user_rule),
global_id(equivalent_resolved_rule), global_id(equivalent_resolved_rule),
global_id(later_acknowledged_rule) global_id(later_acknowledged_rule)
]) ])
......
...@@ -84,20 +84,26 @@ RSpec.describe IncidentManagement::EscalationPolicies::CreateService do ...@@ -84,20 +84,26 @@ RSpec.describe IncidentManagement::EscalationPolicies::CreateService do
it_behaves_like 'error response', 'Escalation policies may not have more than 10 rules' it_behaves_like 'error response', 'Escalation policies may not have more than 10 rules'
end end
context 'oncall schedule is blank' do context 'oncall schedule is on the wrong project' do
before do before do
rule_params[0][:oncall_schedule] = nil rule_params[0][:oncall_schedule] = create(:incident_management_oncall_schedule)
end end
it_behaves_like 'error response', 'All escalations rules must have a schedule in the same project as the policy' it_behaves_like 'error response', 'Schedule-based escalation rules must have a schedule in the same project as the policy'
end end
context 'oncall schedule is on the wrong project' do context 'user for rule does not have project access' do
before do let(:rule_params) do
rule_params[0][:oncall_schedule] = create(:incident_management_oncall_schedule) [
{
user: create(:user),
elapsed_time_seconds: 60,
status: :resolved
}
]
end end
it_behaves_like 'error response', 'All escalations rules must have a schedule in the same project as the policy' it_behaves_like 'error response', 'User-based escalation rules must have a user with access to the project'
end end
context 'project has an existing escalation policy' do context 'project has an existing escalation policy' do
...@@ -115,6 +121,39 @@ RSpec.describe IncidentManagement::EscalationPolicies::CreateService do ...@@ -115,6 +121,39 @@ RSpec.describe IncidentManagement::EscalationPolicies::CreateService do
policy = execute.payload[:escalation_policy] policy = execute.payload[:escalation_policy]
expect(policy).to be_a(::IncidentManagement::EscalationPolicy) expect(policy).to be_a(::IncidentManagement::EscalationPolicy)
expect(policy.rules.length).to eq(1)
expect(policy.rules.first).to have_attributes(
oncall_schedule: oncall_schedule,
user: nil,
elapsed_time_seconds: 60,
status: 'resolved'
)
end
context 'for a user-based escalation rule' do
let(:rule_params) do
[
{
user: user_with_permissions,
elapsed_time_seconds: 60,
status: :resolved
}
]
end
it 'creates the policy and rules' do
expect(execute).to be_success
policy = execute.payload[:escalation_policy]
expect(policy).to be_a(::IncidentManagement::EscalationPolicy)
expect(policy.rules.length).to eq(1)
expect(policy.rules.first).to have_attributes(
oncall_schedule: nil,
user: user_with_permissions,
elapsed_time_seconds: 60,
status: 'resolved'
)
end
end end
end end
end end
......
...@@ -7,8 +7,11 @@ RSpec.describe IncidentManagement::EscalationPolicies::UpdateService do ...@@ -7,8 +7,11 @@ RSpec.describe IncidentManagement::EscalationPolicies::UpdateService do
let_it_be(:user_without_permissions) { create(:user) } let_it_be(:user_without_permissions) { create(:user) }
let_it_be(:project) { create(:project) } let_it_be(:project) { create(:project) }
let_it_be(:oncall_schedule) { create(:incident_management_oncall_schedule, project: project) } let_it_be(:oncall_schedule) { create(:incident_management_oncall_schedule, project: project) }
let_it_be_with_reload(:escalation_policy) { create(:incident_management_escalation_policy, project: project, rule_count: 2) }
let_it_be_with_reload(:escalation_rules) { escalation_policy.rules } let_it_be_with_reload(:escalation_policy) { create(:incident_management_escalation_policy, project: project) }
let_it_be_with_reload(:schedule_escalation_rule) { escalation_policy.rules.first }
let_it_be_with_reload(:user_escalation_rule) { create(:incident_management_escalation_rule, :with_user, policy: escalation_policy) }
let_it_be_with_reload(:escalation_rules) { escalation_policy.reload.rules }
let(:service) { described_class.new(escalation_policy, current_user, params) } let(:service) { described_class.new(escalation_policy, current_user, params) }
let(:current_user) { user_with_permissions } let(:current_user) { user_with_permissions }
...@@ -24,14 +27,16 @@ RSpec.describe IncidentManagement::EscalationPolicies::UpdateService do ...@@ -24,14 +27,16 @@ RSpec.describe IncidentManagement::EscalationPolicies::UpdateService do
let(:rule_params) { [*existing_rules_params, new_rule_params] } let(:rule_params) { [*existing_rules_params, new_rule_params] }
let(:existing_rules_params) do let(:existing_rules_params) do
escalation_rules.map do |rule| escalation_rules.map do |rule|
rule.slice(:oncall_schedule, :elapsed_time_seconds) rule.slice(:oncall_schedule, :user, :elapsed_time_seconds)
.merge(status: rule.status.to_sym) .merge(status: rule.status.to_sym)
end end
end end
let(:user_for_rule) {}
let(:new_rule_params) do let(:new_rule_params) do
{ {
oncall_schedule: oncall_schedule, oncall_schedule: oncall_schedule,
user: user_for_rule,
elapsed_time_seconds: 800, elapsed_time_seconds: 800,
status: :acknowledged status: :acknowledged
} }
...@@ -94,6 +99,13 @@ RSpec.describe IncidentManagement::EscalationPolicies::UpdateService do ...@@ -94,6 +99,13 @@ RSpec.describe IncidentManagement::EscalationPolicies::UpdateService do
let(:expected_rules) { [*escalation_rules, new_rule] } let(:expected_rules) { [*escalation_rules, new_rule] }
it_behaves_like 'successful update with no errors' it_behaves_like 'successful update with no errors'
context 'with a user-based rule' do
let(:oncall_schedule) { nil }
let(:user_for_rule) { user_with_permissions }
it_behaves_like 'successful update with no errors'
end
end end
context 'when all old rules are replaced' do context 'when all old rules are replaced' do
...@@ -166,17 +178,18 @@ RSpec.describe IncidentManagement::EscalationPolicies::UpdateService do ...@@ -166,17 +178,18 @@ RSpec.describe IncidentManagement::EscalationPolicies::UpdateService do
it_behaves_like 'error response', 'Escalation policies may not have more than 10 rules' it_behaves_like 'error response', 'Escalation policies may not have more than 10 rules'
end end
context 'when the on-call schedule is not present on the rule' do
let(:rule_params) { [new_rule_params.except(:oncall_schedule)] }
it_behaves_like 'error response', 'All escalations rules must have a schedule in the same project as the policy'
end
context 'when the on-call schedule is not on the project' do context 'when the on-call schedule is not on the project' do
let(:other_schedule) { create(:incident_management_oncall_schedule) } let(:other_schedule) { create(:incident_management_oncall_schedule) }
let(:rule_params) { [new_rule_params.merge(oncall_schedule: other_schedule)] } let(:rule_params) { [new_rule_params.merge(oncall_schedule: other_schedule)] }
it_behaves_like 'error response', 'All escalations rules must have a schedule in the same project as the policy' it_behaves_like 'error response', 'Schedule-based escalation rules must have a schedule in the same project as the policy'
end
context "when the rule's user does not have access to the project" do
let(:oncall_schedule) { nil }
let(:user_for_rule) { user_without_permissions }
it_behaves_like 'error response', 'User-based escalation rules must have a user with access to the project'
end end
context 'when an error occurs during update' do context 'when an error occurs during update' do
......
...@@ -7,7 +7,7 @@ RSpec.describe IncidentManagement::PendingEscalations::ProcessService do ...@@ -7,7 +7,7 @@ RSpec.describe IncidentManagement::PendingEscalations::ProcessService do
let_it_be(:schedule_1) { create(:incident_management_oncall_schedule, :with_rotation, project: project) } let_it_be(:schedule_1) { create(:incident_management_oncall_schedule, :with_rotation, project: project) }
let_it_be(:schedule_1_users) { schedule_1.participants.map(&:user) } let_it_be(:schedule_1_users) { schedule_1.participants.map(&:user) }
let(:escalation_rule) { build(:incident_management_escalation_rule, oncall_schedule: schedule_1 ) } let(:escalation_rule) { build(:incident_management_escalation_rule, oncall_schedule: schedule_1) }
let!(:escalation_policy) { create(:incident_management_escalation_policy, project: project, rules: [escalation_rule]) } let!(:escalation_policy) { create(:incident_management_escalation_policy, project: project, rules: [escalation_rule]) }
let(:alert) { create(:alert_management_alert, project: project, **alert_params) } let(:alert) { create(:alert_management_alert, project: project, **alert_params) }
...@@ -55,6 +55,14 @@ RSpec.describe IncidentManagement::PendingEscalations::ProcessService do ...@@ -55,6 +55,14 @@ RSpec.describe IncidentManagement::PendingEscalations::ProcessService do
expect { execute }.to change(Note, :count).by(1) expect { execute }.to change(Note, :count).by(1)
end end
context 'when escalation rule is for a user' do
let(:escalation_rule) { build(:incident_management_escalation_rule, :with_user) }
let(:users) { [escalation_rule.user] }
it_behaves_like 'sends on-call notification'
it_behaves_like 'deletes the escalation'
end
end end
context 'target is already resolved' do context 'target is already resolved' do
......
...@@ -3252,9 +3252,6 @@ msgstr "" ...@@ -3252,9 +3252,6 @@ msgstr ""
msgid "All epics" msgid "All epics"
msgstr "" msgstr ""
msgid "All escalations rules must have a schedule in the same project as the policy"
msgstr ""
msgid "All groups and projects" msgid "All groups and projects"
msgstr "" msgstr ""
...@@ -28823,6 +28820,9 @@ msgstr "" ...@@ -28823,6 +28820,9 @@ msgstr ""
msgid "Schedule a new pipeline" msgid "Schedule a new pipeline"
msgstr "" msgstr ""
msgid "Schedule-based escalation rules must have a schedule in the same project as the policy"
msgstr ""
msgid "Scheduled" msgid "Scheduled"
msgstr "" msgstr ""
...@@ -35992,6 +35992,9 @@ msgstr "" ...@@ -35992,6 +35992,9 @@ msgstr ""
msgid "User was successfully updated." msgid "User was successfully updated."
msgstr "" msgstr ""
msgid "User-based escalation rules must have a user with access to the project"
msgstr ""
msgid "UserAvailability|%{author} %{spanStart}(Busy)%{spanEnd}" msgid "UserAvailability|%{author} %{spanStart}(Busy)%{spanEnd}"
msgstr "" msgstr ""
...@@ -39604,7 +39607,7 @@ msgstr "" ...@@ -39604,7 +39607,7 @@ msgstr ""
msgid "must be inside the fork network" msgid "must be inside the fork network"
msgstr "" msgstr ""
msgid "must have a unique schedule, status, and elapsed time" msgid "must be unique by status and elapsed time within a policy"
msgstr "" msgstr ""
msgid "my-awesome-group" msgid "my-awesome-group"
......
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