-
Notifications
You must be signed in to change notification settings - Fork 297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
notify user task patch + small update to user notification rules public API docs #4628
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will update tests in #4629
@@ -35,7 +35,7 @@ The above command returns JSON structured in the following way: | |||
| ----------- | :------: | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | |||
| `user_id` | Yes | User ID | | |||
| `position` | Optional | Personal notification rules execute one after another starting from `position=0`. `Position=-1` will put the escalation policy to the end of the list. A new escalation policy created with a position of an existing escalation policy will move the old one (and all following) down on the list. | | |||
| `type` | Yes | One of: `wait`, `notify_by_slack`, `notify_by_sms`, `notify_by_phone_call`, `notify_by_telegram`, `notify_by_email`, `notify_by_mobile_app`, `notify_by_mobile_app_critical`. | | |||
| `type` | Yes | One of: `wait`, `notify_by_slack`, `notify_by_sms`, `notify_by_phone_call`, `notify_by_telegram`, `notify_by_email`, `notify_by_mobile_app`, `notify_by_mobile_app_critical`, or `notify_by_msteams` (**NOTE** `notify_by_msteams` is only available on Grafana Cloud). | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
…#4635) # What this PR does This is a follow-up PR to #4628. As @Ferril pointed out, there was a slight issue in `apps.alerts.tasks.notify_user.perform_notification` method when using a "fallback"/default user notification policy. This is because the `log_record_pk` arg passed into `perform_notification` will fetch the `UserNotificationPolicyLogRecord` object, but that object will have a `notification_policy` set to `None` (because there's no persistent `UserNotificationPolicy` object to refer to). Instead we now pass in a second argument to `perform_notification`, `use_default_notification_policy_fallback`. If this is true, simply grab the transient/in-memory `UserNotificationPolicy` and use that inside of this task Related to #4410 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] Added the relevant release notes label (see labels prefixed w/ `release:`). These labels dictate how your PR will show up in the autogenerated release notes.
What this PR does
Patches a small bug noticed (locally) by @Ferril 🙏 + updates our user notification rules public API docs to include
notify_by_msteams
as a validtype
value (cloud only)Checklist
pr:no public docs
PR label added if not required)release:
). These labels dictate how your PR willshow up in the autogenerated release notes.