diff --git a/db/migrate/20210722074220_remove_null_constraint_on_schedule_from_escalation_rules.rb b/db/migrate/20210722074220_remove_null_constraint_on_schedule_from_escalation_rules.rb new file mode 100644 index 0000000000000000000000000000000000000000..7146c6c953729142a03bfe7628c115699a80e75c --- /dev/null +++ b/db/migrate/20210722074220_remove_null_constraint_on_schedule_from_escalation_rules.rb @@ -0,0 +1,13 @@ +# 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 diff --git a/db/migrate/20210722074242_add_user_to_escalation_rules.rb b/db/migrate/20210722074242_add_user_to_escalation_rules.rb new file mode 100644 index 0000000000000000000000000000000000000000..061dd6194f70bf4217a099ee64f0f26089058780 --- /dev/null +++ b/db/migrate/20210722074242_add_user_to_escalation_rules.rb @@ -0,0 +1,17 @@ +# 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 diff --git a/db/migrate/20210722074256_add_user_index_to_escalation_rules.rb b/db/migrate/20210722074256_add_user_index_to_escalation_rules.rb new file mode 100644 index 0000000000000000000000000000000000000000..047149d6e7c19305bcd1e48f6adf85e16bbe0c6a --- /dev/null +++ b/db/migrate/20210722074256_add_user_index_to_escalation_rules.rb @@ -0,0 +1,33 @@ +# 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 diff --git a/db/migrate/20210722074309_add_user_fk_to_escalation_rules.rb b/db/migrate/20210722074309_add_user_fk_to_escalation_rules.rb new file mode 100644 index 0000000000000000000000000000000000000000..acdfc1ed83590539a09ade0922107531626b10a8 --- /dev/null +++ b/db/migrate/20210722074309_add_user_fk_to_escalation_rules.rb @@ -0,0 +1,17 @@ +# 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 diff --git a/db/migrate/20210722074339_add_xor_check_constraint_for_escalation_rules.rb b/db/migrate/20210722074339_add_xor_check_constraint_for_escalation_rules.rb new file mode 100644 index 0000000000000000000000000000000000000000..bd140e1da566b360c4765fec21b6400c9e4f44dc --- /dev/null +++ b/db/migrate/20210722074339_add_xor_check_constraint_for_escalation_rules.rb @@ -0,0 +1,17 @@ +# 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 diff --git a/db/schema_migrations/20210722074220 b/db/schema_migrations/20210722074220 new file mode 100644 index 0000000000000000000000000000000000000000..d0bc4133883f08139e8e16b6d4e92a6b938c1b94 --- /dev/null +++ b/db/schema_migrations/20210722074220 @@ -0,0 +1 @@ +cf276b9aa97fc7857499e1b103a8e09eda329a4db92d0e653cc6f7128987be39 \ No newline at end of file diff --git a/db/schema_migrations/20210722074242 b/db/schema_migrations/20210722074242 new file mode 100644 index 0000000000000000000000000000000000000000..2fc61b36ed2451ee6e1e3cebf7019cda8ea88ad1 --- /dev/null +++ b/db/schema_migrations/20210722074242 @@ -0,0 +1 @@ +5c6aff5b43a1e81e84a42f008a8a1ab90c77ee450884aa1ecc86bce551424f43 \ No newline at end of file diff --git a/db/schema_migrations/20210722074256 b/db/schema_migrations/20210722074256 new file mode 100644 index 0000000000000000000000000000000000000000..cedf6a7419baaf28c555c86f8463bb26dfc93a41 --- /dev/null +++ b/db/schema_migrations/20210722074256 @@ -0,0 +1 @@ +d49b1f48c2fa1cac8d7793f8bb025792f4bb85eed787ba3abdbaa4647523b70a \ No newline at end of file diff --git a/db/schema_migrations/20210722074309 b/db/schema_migrations/20210722074309 new file mode 100644 index 0000000000000000000000000000000000000000..27b1bb9e493e8759c80e6eeb734f61fdf90858af --- /dev/null +++ b/db/schema_migrations/20210722074309 @@ -0,0 +1 @@ +eab0f8488b0122ec6c5625c66ebcbd221579bdd9cc2cf670d1f26181709f23b7 \ No newline at end of file diff --git a/db/schema_migrations/20210722074339 b/db/schema_migrations/20210722074339 new file mode 100644 index 0000000000000000000000000000000000000000..dc269e5486895cfb86942dd9c57e912215ec727d --- /dev/null +++ b/db/schema_migrations/20210722074339 @@ -0,0 +1 @@ +a7a6697d86b71d59104af35a9d7d6f3caebf4ee1252e4f3e52133afb3f642e48 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 4121e3eb8b64dbea1bab2aab217fb8284b5d4c56..ce2d5acb585a3d66d3194e15d0b934f3bef74be8 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -13941,10 +13941,12 @@ ALTER SEQUENCE incident_management_escalation_policies_id_seq OWNED BY incident_ CREATE TABLE incident_management_escalation_rules ( id bigint NOT NULL, policy_id bigint NOT NULL, - oncall_schedule_id bigint NOT NULL, + oncall_schedule_id bigint, status smallint 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 @@ -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 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_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 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 INDEX index_on_projects_lower_path ON projects USING btree (lower((path)::text)); @@ -25895,6 +25899,9 @@ ALTER TABLE ONLY epics ALTER TABLE ONLY clusters_applications_runners 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 ADD CONSTRAINT fk_03c671965c FOREIGN KEY (design_id) REFERENCES design_management_designs(id) ON DELETE CASCADE; diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 111863185410712a3f6cf71374012c4e978bd11a..33ac276afe750c23329e86530f77967cc63f14c3 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -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="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="escalationruletypeuser"></a>`user` | [`UserCore`](#usercore) | The user to notify. | ### `Event` @@ -16737,8 +16738,9 @@ Represents an escalation rule. | Name | Type | Description | | ---- | ---- | ----------- | | <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="escalationruleinputusername"></a>`username` | [`String`](#string) | The username of the user to notify. | ### `JiraUsersMappingInputType` diff --git a/ee/app/graphql/mutations/incident_management/escalation_policy/base.rb b/ee/app/graphql/mutations/incident_management/escalation_policy/base.rb index e9d05db7f6971494f1b8a8f22921b4711031a955..a39c0b579f69b2faf0f09f8d004ee4f2238f05b2 100644 --- a/ee/app/graphql/mutations/incident_management/escalation_policy/base.rb +++ b/ee/app/graphql/mutations/incident_management/escalation_policy/base.rb @@ -43,23 +43,42 @@ def escalation_policies_available?(policy) def prepare_rules_attributes(project, args) return args unless rules = args.delete(:rules) - iids = rules.collect { |rule| rule[:oncall_schedule_iid] } - found_schedules = schedules_for_iids(project, iids) - rules_attributes = rules.map { |rule| prepare_rule(found_schedules, rule.to_h) } + schedules = find_schedules(project, rules) + users = find_users(rules) + rules_attributes = rules.map { |rule| prepare_rule(rule.to_h, schedules, users) } args.merge(rules_attributes: rules_attributes) end - def prepare_rule(schedules, rule) + def prepare_rule(rule, schedules, users) 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 - def schedules_for_iids(project, iids) - schedules = ::IncidentManagement::OncallSchedulesFinder.new(current_user, project, iid: iids).execute + def find_schedules(project, rules) + 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 diff --git a/ee/app/graphql/types/incident_management/escalation_rule_input_type.rb b/ee/app/graphql/types/incident_management/escalation_rule_input_type.rb index 5f549fa44114982169701d1475676564b306eebf..5bbc5e31b70595fc3a53551902510ad781d4a5be 100644 --- a/ee/app/graphql/types/incident_management/escalation_rule_input_type.rb +++ b/ee/app/graphql/types/incident_management/escalation_rule_input_type.rb @@ -8,7 +8,11 @@ class EscalationRuleInputType < BaseInputObject argument :oncall_schedule_iid, GraphQL::Types::ID, # rubocop: disable Graphql/IDType 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, description: 'The time in seconds before the rule is activated.', @@ -17,6 +21,18 @@ class EscalationRuleInputType < BaseInputObject argument :status, Types::IncidentManagement::EscalationRuleStatusEnum, description: 'The status required to prevent the rule from activating.', 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 diff --git a/ee/app/graphql/types/incident_management/escalation_rule_type.rb b/ee/app/graphql/types/incident_management/escalation_rule_type.rb index 0f9bec82cea92688a3b522249d062089fb5d9aeb..3bd2e9a3f7a454f1c4ef4e973d8b49b3f1541b12 100644 --- a/ee/app/graphql/types/incident_management/escalation_rule_type.rb +++ b/ee/app/graphql/types/incident_management/escalation_rule_type.rb @@ -15,6 +15,10 @@ class EscalationRuleType < BaseObject null: true, 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, null: true, description: 'The time in seconds before the rule is activated.' diff --git a/ee/app/models/incident_management/escalation_policy.rb b/ee/app/models/incident_management/escalation_policy.rb index 1e8b390c7f0d43eda38b0e35ac4e63f56ea37bbe..88986840276d00ed392bca81379b41ec15b02dc0 100644 --- a/ee/app/models/incident_management/escalation_policy.rb +++ b/ee/app/models/incident_management/escalation_policy.rb @@ -11,7 +11,6 @@ class EscalationPolicy < ApplicationRecord 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 :description, length: { maximum: 160 } - validates :rules, presence: true accepts_nested_attributes_for :rules end diff --git a/ee/app/models/incident_management/escalation_rule.rb b/ee/app/models/incident_management/escalation_rule.rb index de13223ce2a26a59e9de354ebfa9f749ca462ff1..960a95f04867c911b7bb9861070b5da3d9b5fc6a 100644 --- a/ee/app/models/incident_management/escalation_rule.rb +++ b/ee/app/models/incident_management/escalation_rule.rb @@ -5,19 +5,35 @@ class EscalationRule < ApplicationRecord self.table_name = 'incident_management_escalation_rules' 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) validates :status, presence: true - validates :oncall_schedule, presence: true validates :elapsed_time_seconds, presence: true, 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 :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 diff --git a/ee/app/services/incident_management/escalation_policies/base_service.rb b/ee/app/services/incident_management/escalation_policies/base_service.rb index ebffb8244572853230093eba76e7c074716dd3ca..0e1971f97b521ebecf990acf823e3be0744e6e3a 100644 --- a/ee/app/services/incident_management/escalation_policies/base_service.rb +++ b/ee/app/services/incident_management/escalation_policies/base_service.rb @@ -14,7 +14,13 @@ def too_many_rules? end 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 def error(message) @@ -38,7 +44,11 @@ def error_too_many_rules end 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 def error_in_save(policy) diff --git a/ee/app/services/incident_management/escalation_policies/create_service.rb b/ee/app/services/incident_management/escalation_policies/create_service.rb index 0ad2c84d229d2c57e97d31a14cbb978b94c1970b..2f73321b891d7a5e51c199c9b298972e6d503efe 100644 --- a/ee/app/services/incident_management/escalation_policies/create_service.rb +++ b/ee/app/services/incident_management/escalation_policies/create_service.rb @@ -23,6 +23,7 @@ def execute return error_no_rules if params[:rules_attributes].blank? return error_too_many_rules if too_many_rules? 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) diff --git a/ee/app/services/incident_management/escalation_policies/update_service.rb b/ee/app/services/incident_management/escalation_policies/update_service.rb index ac57055ccf99f277e5dbeb344302e758d91afa99..169345e9fb39179d76d03f351ba2d21480a9d633 100644 --- a/ee/app/services/incident_management/escalation_policies/update_service.rb +++ b/ee/app/services/incident_management/escalation_policies/update_service.rb @@ -28,6 +28,7 @@ def execute return error_no_rules if empty_rules? return error_too_many_rules if too_many_rules? return error_bad_schedules if invalid_schedules? + return error_user_without_permission if users_without_permissions? reconcile_rules! @@ -85,7 +86,7 @@ def expected_rules_by_uniq_id end 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 diff --git a/ee/app/services/incident_management/pending_escalations/process_service.rb b/ee/app/services/incident_management/pending_escalations/process_service.rb index 6b234825bbfc172a6ff552de315866c24571c837..0ea2aa1b288af3a18c8c2868e082f6eb407a3b9f 100644 --- a/ee/app/services/incident_management/pending_escalations/process_service.rb +++ b/ee/app/services/incident_management/pending_escalations/process_service.rb @@ -55,10 +55,14 @@ def create_system_notes def oncall_notification_recipients 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 + def schedule_recipients + ::IncidentManagement::OncallUsersFinder.new(project, schedule: rule.oncall_schedule).execute.to_a + end + def destroy_escalation! escalation.destroy! end diff --git a/ee/spec/factories/incident_management/escalation_rules.rb b/ee/spec/factories/incident_management/escalation_rules.rb index b5a54a2bd5c38ec1c7185f5215da7dcb2bef3879..94e43fe4c9a5b06a96c04fb3b4702f02c8df9f03 100644 --- a/ee/spec/factories/incident_management/escalation_rules.rb +++ b/ee/spec/factories/incident_management/escalation_rules.rb @@ -2,7 +2,7 @@ FactoryBot.define 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 } status { IncidentManagement::EscalationRule.statuses[:acknowledged] } elapsed_time_seconds { 5.minutes } @@ -15,5 +15,10 @@ trait :removed do is_removed { true } end + + trait :with_user do + oncall_schedule {} + user { association :user, developer_projects: [policy.project] } + end end end diff --git a/ee/spec/graphql/mutations/incident_management/escalation_policy/create_spec.rb b/ee/spec/graphql/mutations/incident_management/escalation_policy/create_spec.rb index d6b819f0d0a06b5cf41c95067fecef45b492b211..7dccfa7a0f3975090db95deb19489efb58c1f1c2 100644 --- a/ee/spec/graphql/mutations/incident_management/escalation_policy/create_spec.rb +++ b/ee/spec/graphql/mutations/incident_management/escalation_policy/create_spec.rb @@ -19,7 +19,7 @@ status: ::IncidentManagement::EscalationRule.statuses[:acknowledged] }, { - oncall_schedule_iid: oncall_schedule.iid, + username: current_user&.username, elapsed_time_seconds: 600, status: ::IncidentManagement::EscalationRule.statuses[:resolved] } @@ -71,8 +71,8 @@ expect(rules.size).to eq(2) 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, elapsed_time_seconds: 600, status: 'resolved') + have_attributes(oncall_schedule_id: oncall_schedule.id, user: nil, elapsed_time_seconds: 300, status: 'acknowledged'), + have_attributes(oncall_schedule_id: nil, user: current_user, elapsed_time_seconds: 600, status: 'resolved') ]) end @@ -91,7 +91,7 @@ args[:rules][0][:oncall_schedule_iid] = other_schedule.iid 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 before do @@ -101,6 +101,14 @@ it_behaves_like 'raises a resource not available error' 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 context 'user does not have permission for project' do diff --git a/ee/spec/graphql/mutations/incident_management/escalation_policy/update_spec.rb b/ee/spec/graphql/mutations/incident_management/escalation_policy/update_spec.rb index 4ccc49726297fd81926121f49d55ae67c9dab5a5..6c3cdda84c32b178d6b1b0479dd4f80c2b7151ba 100644 --- a/ee/spec/graphql/mutations/incident_management/escalation_policy/update_spec.rb +++ b/ee/spec/graphql/mutations/incident_management/escalation_policy/update_spec.rb @@ -110,6 +110,7 @@ context 'with rule updates' do let(:oncall_schedule_iid) { oncall_schedule.iid } + let(:username) { reporter.username } let(:rule_args) do [ { @@ -119,6 +120,13 @@ }, { oncall_schedule_iid: oncall_schedule_iid, + username: nil, + elapsed_time_seconds: 800, + status: :acknowledged + }, + { + oncall_schedule_iid: nil, + username: username, elapsed_time_seconds: 800, status: :acknowledged } @@ -128,22 +136,17 @@ let(:expected_rules) do [ 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 it_behaves_like 'successful update with no errors' 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 } - it 'returns errors in the body of the response' do - expect(resolve).to eq( - escalation_policy: nil, - errors: ['All escalations rules must have a schedule in the same project as the policy'] - ) - end + it_behaves_like 'failed update with a top-level access error' context 'the user does not have permission to update policies regardless' do let(:current_user) { reporter } @@ -151,6 +154,12 @@ it_behaves_like 'failed update with a top-level access error' 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 diff --git a/ee/spec/graphql/types/incident_management/escalation_rule_input_type_spec.rb b/ee/spec/graphql/types/incident_management/escalation_rule_input_type_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..fdc1ef1a0e29c9a5ef0c8c8886b9cb0a1e14129a --- /dev/null +++ b/ee/spec/graphql/types/incident_management/escalation_rule_input_type_spec.rb @@ -0,0 +1,51 @@ +# 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 diff --git a/ee/spec/graphql/types/incident_management/escalation_rule_type_spec.rb b/ee/spec/graphql/types/incident_management/escalation_rule_type_spec.rb index 35dd8f02598bf8a1a6462e7414ad9458f7614cec..a3e23d60497185feba5293c4dc2ac6041f915927 100644 --- a/ee/spec/graphql/types/incident_management/escalation_rule_type_spec.rb +++ b/ee/spec/graphql/types/incident_management/escalation_rule_type_spec.rb @@ -9,6 +9,7 @@ expected_fields = %i[ id oncall_schedule + user elapsed_time_seconds status ] diff --git a/ee/spec/models/incident_management/escalation_policy_spec.rb b/ee/spec/models/incident_management/escalation_policy_spec.rb index da406b15761894d8a524a2fbcc5ae8473330a00e..ec4a296d035c84a1a788ea418baae905d1d0dc08 100644 --- a/ee/spec/models/incident_management/escalation_policy_spec.rb +++ b/ee/spec/models/incident_management/escalation_policy_spec.rb @@ -25,7 +25,6 @@ describe 'validations' do 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(:name).scoped_to(:project_id) } it { is_expected.to validate_length_of(:name).is_at_most(72) } diff --git a/ee/spec/models/incident_management/escalation_rule_spec.rb b/ee/spec/models/incident_management/escalation_rule_spec.rb index f4815ca47f0616a481e630ac7944b97a912fca8a..b61c3febbabc4d628720ce926991b178575db096 100644 --- a/ee/spec/models/incident_management/escalation_rule_spec.rb +++ b/ee/spec/models/incident_management/escalation_rule_spec.rb @@ -3,27 +3,54 @@ require 'spec_helper' RSpec.describe IncidentManagement::EscalationRule do - let_it_be(:policy) { create(:incident_management_escalation_policy) } - - subject { build(:incident_management_escalation_rule, policy: policy) } - - it { is_expected.to be_valid } + subject { build(:incident_management_escalation_rule) } describe 'associations' do 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 describe 'validations' do + it { is_expected.to be_valid } it { is_expected.to validate_presence_of(:status) } 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_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 describe 'scopes' do - let_it_be(:rule) { policy.rules.first } - let_it_be(:removed_rule) { create(:incident_management_escalation_rule, :removed, policy: policy) } + let_it_be(:rule) { create(:incident_management_escalation_rule) } + let_it_be(:removed_rule) { create(:incident_management_escalation_rule, :removed, policy: rule.policy) } describe '.not_removed' do subject { described_class.not_removed } diff --git a/ee/spec/requests/api/graphql/project/incident_management/escalation_policies_spec.rb b/ee/spec/requests/api/graphql/project/incident_management/escalation_policies_spec.rb index 8f323aba2d768755c25c8f55db71bff93db2fc19..479a14336040cdf83218daac977e5b5b46669075 100644 --- a/ee/spec/requests/api/graphql/project/incident_management/escalation_policies_spec.rb +++ b/ee/spec/requests/api/graphql/project/incident_management/escalation_policies_spec.rb @@ -118,7 +118,8 @@ 'name' => last_policy_rule.oncall_schedule.name, 'description' => last_policy_rule.oncall_schedule.description, 'timezone' => last_policy_rule.oncall_schedule.timezone - } + }, + 'user' => nil } ] ) diff --git a/ee/spec/requests/api/graphql/project/incident_management/escalation_policy/rules_spec.rb b/ee/spec/requests/api/graphql/project/incident_management/escalation_policy/rules_spec.rb index ce2f97948b5bdd1ba7d2737d7089581209676bdb..71b9b7c4a80d873f75b44cc5bbc3a681f9162649 100644 --- a/ee/spec/requests/api/graphql/project/incident_management/escalation_policy/rules_spec.rb +++ b/ee/spec/requests/api/graphql/project/incident_management/escalation_policy/rules_spec.rb @@ -10,6 +10,7 @@ let_it_be(:policy) { create(:incident_management_escalation_policy, project: project) } let_it_be(:rule) { policy.rules.first } let_it_be(:schedule) { rule.oncall_schedule } + let_it_be(:user_rule) { create(:incident_management_escalation_rule, :with_user, policy: policy) } let(:params) { {} } @@ -25,6 +26,9 @@ iid name } + user { + username + } } } QUERY @@ -49,15 +53,25 @@ it 'includes expected data' do post_graphql(query, current_user: current_user) - expect(escalation_rules_response).to eq([{ - 'id' => global_id(rule), - 'elapsedTimeSeconds' => rule.elapsed_time_seconds, # 5 min - 'status' => rule.status.upcase, # 'ACKNOWLEDGED' - 'oncallSchedule' => { - 'iid' => schedule.iid.to_s, - 'name' => schedule.name + expect(escalation_rules_response).to eq([ + { + 'id' => global_id(rule), + 'elapsedTimeSeconds' => rule.elapsed_time_seconds, # 5 min + 'status' => rule.status.upcase, # 'ACKNOWLEDGED' + 'oncallSchedule' => { + 'iid' => schedule.iid.to_s, + '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 context 'with multiple rules' do @@ -68,10 +82,11 @@ it 'orders rules by time and status' do 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([ global_id(earlier_resolved_rule), global_id(rule), + global_id(user_rule), global_id(equivalent_resolved_rule), global_id(later_acknowledged_rule) ]) diff --git a/ee/spec/services/incident_management/escalation_policies/create_service_spec.rb b/ee/spec/services/incident_management/escalation_policies/create_service_spec.rb index 2bf2edbecedfb44be46a0dbf1fe51401e0df9f3d..2f9fc4f5b3c1def5aaa1a3473bcde954f28de7ed 100644 --- a/ee/spec/services/incident_management/escalation_policies/create_service_spec.rb +++ b/ee/spec/services/incident_management/escalation_policies/create_service_spec.rb @@ -84,20 +84,26 @@ it_behaves_like 'error response', 'Escalation policies may not have more than 10 rules' end - context 'oncall schedule is blank' do + context 'oncall schedule is on the wrong project' do before do - rule_params[0][:oncall_schedule] = nil + rule_params[0][:oncall_schedule] = create(:incident_management_oncall_schedule) 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 - context 'oncall schedule is on the wrong project' do - before do - rule_params[0][:oncall_schedule] = create(:incident_management_oncall_schedule) + context 'user for rule does not have project access' do + let(:rule_params) do + [ + { + user: create(:user), + elapsed_time_seconds: 60, + status: :resolved + } + ] 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 context 'project has an existing escalation policy' do @@ -115,6 +121,39 @@ 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: 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 diff --git a/ee/spec/services/incident_management/escalation_policies/update_service_spec.rb b/ee/spec/services/incident_management/escalation_policies/update_service_spec.rb index d2e8d4f170fa0d0138f24af34bfa2827c689cfd1..70f2013c1f4ba3f7e9f6b701d8ee89fc7e00df01 100644 --- a/ee/spec/services/incident_management/escalation_policies/update_service_spec.rb +++ b/ee/spec/services/incident_management/escalation_policies/update_service_spec.rb @@ -7,8 +7,11 @@ let_it_be(:user_without_permissions) { create(:user) } let_it_be(:project) { create(: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(:current_user) { user_with_permissions } @@ -24,14 +27,16 @@ let(:rule_params) { [*existing_rules_params, new_rule_params] } let(:existing_rules_params) do 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) end end + let(:user_for_rule) {} let(:new_rule_params) do { oncall_schedule: oncall_schedule, + user: user_for_rule, elapsed_time_seconds: 800, status: :acknowledged } @@ -94,6 +99,13 @@ let(:expected_rules) { [*escalation_rules, new_rule] } 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 context 'when all old rules are replaced' do @@ -166,17 +178,18 @@ it_behaves_like 'error response', 'Escalation policies may not have more than 10 rules' 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 let(:other_schedule) { create(:incident_management_oncall_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 context 'when an error occurs during update' do diff --git a/ee/spec/services/incident_management/pending_escalations/process_service_spec.rb b/ee/spec/services/incident_management/pending_escalations/process_service_spec.rb index bdaa037aa700942d2f24e0b33ad464e2e152c80d..32debb9972dfeaa1986136dda055b92b59703c44 100644 --- a/ee/spec/services/incident_management/pending_escalations/process_service_spec.rb +++ b/ee/spec/services/incident_management/pending_escalations/process_service_spec.rb @@ -7,7 +7,7 @@ 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(: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(:alert) { create(:alert_management_alert, project: project, **alert_params) } @@ -55,6 +55,14 @@ expect { execute }.to change(Note, :count).by(1) 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 context 'target is already resolved' do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 67014d741d0b19fd78349b01d092d00263de3844..c3897dad297e5a859bdbc5a753dcc9570afbc816 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3252,9 +3252,6 @@ msgstr "" msgid "All epics" msgstr "" -msgid "All escalations rules must have a schedule in the same project as the policy" -msgstr "" - msgid "All groups and projects" msgstr "" @@ -28823,6 +28820,9 @@ msgstr "" msgid "Schedule a new pipeline" msgstr "" +msgid "Schedule-based escalation rules must have a schedule in the same project as the policy" +msgstr "" + msgid "Scheduled" msgstr "" @@ -35992,6 +35992,9 @@ msgstr "" msgid "User was successfully updated." msgstr "" +msgid "User-based escalation rules must have a user with access to the project" +msgstr "" + msgid "UserAvailability|%{author} %{spanStart}(Busy)%{spanEnd}" msgstr "" @@ -39604,7 +39607,7 @@ msgstr "" msgid "must be inside the fork network" msgstr "" -msgid "must have a unique schedule, status, and elapsed time" +msgid "must be unique by status and elapsed time within a policy" msgstr "" msgid "my-awesome-group"