Skip to content
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

Merged
merged 3 commits into from
Jul 8, 2024

Conversation

joeyorlando
Copy link
Contributor

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 valid type value (cloud only)

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • 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.

@joeyorlando joeyorlando added the release:patch PR will be added to "Other Changes" section of release notes label Jul 8, 2024
@joeyorlando joeyorlando requested a review from a team as a code owner July 8, 2024 15:21
@joeyorlando joeyorlando requested review from a team July 8, 2024 15:21
Copy link
Contributor Author

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). |
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@Ferril Ferril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@joeyorlando joeyorlando merged commit 0163b58 into dev Jul 8, 2024
21 checks passed
@joeyorlando joeyorlando deleted the jorlando/notify-user-task-patch branch July 8, 2024 15:52
joeyorlando added a commit that referenced this pull request Jul 9, 2024
…#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:patch PR will be added to "Other Changes" section of release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants