don't force create default user notification policies (#4608)
# What this PR does Related to https://github.com/grafana/oncall/issues/4410 The changes in this PR are a prerequisite to https://github.com/grafana/terraform-provider-grafana/pull/1653. See the conversation [here](https://raintank-corp.slack.com/archives/C04JCU51NF8/p1719806995902499?thread_ts=1719520920.744319&cid=C04JCU51NF8) for more context on why we decided to move away from always creating default personal notification rules for users. ## 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.
This commit is contained in:
Joey Orlando
GitHub
parent
6e0bebaa11
commit
abedea72bf
12 changed files with 50 additions and 141 deletions
|
|
@ -35,8 +35,8 @@ 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`. |
|
||||
| `duration` | Optional | A time in secs when type `wait` is chosen for `type`. |
|
||||
| `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`. |
|
||||
| `duration` | Optional | A time in seconds to wait (when `type=wait`). Can be one of 60, 300, 900, 1800, or 3600. |
|
||||
| `important` | Optional | Boolean value indicates if a rule is "important". Default is `false`. |
|
||||
|
||||
**HTTP request**
|
||||
|
|
|
|||
|
|
@ -10,7 +10,8 @@ if typing.TYPE_CHECKING:
|
|||
from django.db.models.manager import RelatedManager
|
||||
|
||||
from apps.alerts.models import AlertGroup, AlertGroupLogRecord, ResolutionNote
|
||||
from apps.base.models import UserNotificationPolicyLogRecord
|
||||
from apps.base.models import UserNotificationPolicy, UserNotificationPolicyLogRecord
|
||||
from apps.user_management.models import User
|
||||
|
||||
|
||||
class IncidentLogBuilder:
|
||||
|
|
@ -578,7 +579,9 @@ class IncidentLogBuilder:
|
|||
escalation_plan_dict.setdefault(timedelta, []).append(plan)
|
||||
return escalation_plan_dict
|
||||
|
||||
def _render_user_notification_line(self, user_to_notify, notification_policy, for_slack=False):
|
||||
def _render_user_notification_line(
|
||||
self, user_to_notify: "User", notification_policy: "UserNotificationPolicy", for_slack=False
|
||||
):
|
||||
"""
|
||||
Renders user notification plan line
|
||||
:param user_to_notify:
|
||||
|
|
@ -611,7 +614,9 @@ class IncidentLogBuilder:
|
|||
result += f"inviting {user_verbal} but notification channel is unspecified"
|
||||
return result
|
||||
|
||||
def _get_notification_plan_for_user(self, user_to_notify, future_step=False, important=False, for_slack=False):
|
||||
def _get_notification_plan_for_user(
|
||||
self, user_to_notify: "User", future_step=False, important=False, for_slack=False
|
||||
):
|
||||
"""
|
||||
Renders user notification plan
|
||||
:param user_to_notify:
|
||||
|
|
@ -665,7 +670,7 @@ class IncidentLogBuilder:
|
|||
# last passed step order + 1
|
||||
notification_policy_order = last_user_log.notification_policy.order + 1
|
||||
|
||||
notification_policies = user_to_notify.get_or_create_notification_policies(important=important)
|
||||
notification_policies = user_to_notify.get_notification_policies_or_use_default_fallback(important=important)
|
||||
|
||||
for notification_policy in notification_policies:
|
||||
future_notification = notification_policy.order >= notification_policy_order
|
||||
|
|
|
|||
|
|
@ -83,22 +83,22 @@ def notify_group_task(alert_group_pk, escalation_policy_snapshot_order=None):
|
|||
continue
|
||||
|
||||
important = escalation_policy_step == EscalationPolicy.STEP_NOTIFY_GROUP_IMPORTANT
|
||||
notification_policies = user.get_or_create_notification_policies(important=important)
|
||||
notification_policies = user.get_notification_policies_or_use_default_fallback(important=important)
|
||||
|
||||
if notification_policies:
|
||||
usergroup_notification_plan += "\n_{} (".format(
|
||||
step.get_user_notification_message_for_thread_for_usergroup(user, notification_policies.first())
|
||||
step.get_user_notification_message_for_thread_for_usergroup(user, notification_policies[0])
|
||||
)
|
||||
|
||||
notification_channels = []
|
||||
if notification_policies.filter(step=UserNotificationPolicy.Step.NOTIFY).count() == 0:
|
||||
else:
|
||||
usergroup_notification_plan += "Empty notifications"
|
||||
|
||||
notification_channels = []
|
||||
for notification_policy in notification_policies:
|
||||
if notification_policy.step == UserNotificationPolicy.Step.NOTIFY:
|
||||
notification_channels.append(
|
||||
UserNotificationPolicy.NotificationChannel(notification_policy.notify_by).label
|
||||
)
|
||||
|
||||
usergroup_notification_plan += "→".join(notification_channels) + ")_"
|
||||
reason = f"Membership in <!subteam^{usergroup.slack_id}> User Group"
|
||||
|
||||
|
|
|
|||
|
|
@ -72,20 +72,20 @@ def notify_user_task(
|
|||
user_has_notification = UserHasNotification.objects.filter(pk=user_has_notification.pk).select_for_update()[0]
|
||||
|
||||
if previous_notification_policy_pk is None:
|
||||
notification_policy = user.get_or_create_notification_policies(important=important).first()
|
||||
if notification_policy is None:
|
||||
notification_policies = user.get_notification_policies_or_use_default_fallback(important=important)
|
||||
if not notification_policies:
|
||||
task_logger.info(
|
||||
f"notify_user_task: Failed to notify. No notification policies. user_id={user_pk} alert_group_id={alert_group_pk} important={important}"
|
||||
)
|
||||
return
|
||||
|
||||
# Here we collect a brief overview of notification steps configured for user to send it to thread.
|
||||
collected_steps_ids = []
|
||||
next_notification_policy = notification_policy.next()
|
||||
while next_notification_policy is not None:
|
||||
if next_notification_policy.step == UserNotificationPolicy.Step.NOTIFY:
|
||||
if next_notification_policy.notify_by not in collected_steps_ids:
|
||||
collected_steps_ids.append(next_notification_policy.notify_by)
|
||||
next_notification_policy = next_notification_policy.next()
|
||||
for notification_policy in notification_policies:
|
||||
if notification_policy.step == UserNotificationPolicy.Step.NOTIFY:
|
||||
if notification_policy.notify_by not in collected_steps_ids:
|
||||
collected_steps_ids.append(notification_policy.notify_by)
|
||||
|
||||
collected_steps = ", ".join(
|
||||
UserNotificationPolicy.NotificationChannel(step_id).label for step_id in collected_steps_ids
|
||||
)
|
||||
|
|
|
|||
|
|
@ -17,7 +17,6 @@ from apps.mobile_app.auth import MobileAppAuthTokenAuthentication
|
|||
from apps.user_management.models import User
|
||||
from common.api_helpers.exceptions import BadRequest
|
||||
from common.api_helpers.mixins import UpdateSerializerMixin
|
||||
from common.exceptions import UserNotificationPolicyCouldNotBeDeleted
|
||||
from common.insight_log import EntityEvent, write_resource_insight_log
|
||||
from common.ordered_model.viewset import OrderedModelViewSet
|
||||
|
||||
|
|
@ -73,7 +72,7 @@ class UserNotificationPolicyView(UpdateSerializerMixin, OrderedModelViewSet):
|
|||
target_user = User.objects.get(public_primary_key=user_id)
|
||||
except User.DoesNotExist:
|
||||
raise BadRequest(detail="User does not exist")
|
||||
queryset = target_user.get_or_create_notification_policies(important=important)
|
||||
queryset = UserNotificationPolicy.objects.filter(user=target_user, important=important)
|
||||
return self.serializer_class.setup_eager_loading(queryset)
|
||||
|
||||
def get_object(self):
|
||||
|
|
@ -119,10 +118,7 @@ class UserNotificationPolicyView(UpdateSerializerMixin, OrderedModelViewSet):
|
|||
def perform_destroy(self, instance):
|
||||
user = instance.user
|
||||
prev_state = user.insight_logs_serialized
|
||||
try:
|
||||
instance.delete()
|
||||
except UserNotificationPolicyCouldNotBeDeleted:
|
||||
raise BadRequest(detail="Can't delete last user notification policy")
|
||||
instance.delete()
|
||||
new_state = user.insight_logs_serialized
|
||||
write_resource_insight_log(
|
||||
instance=user,
|
||||
|
|
|
|||
|
|
@ -5,12 +5,11 @@ from typing import Tuple
|
|||
from django.conf import settings
|
||||
from django.core.exceptions import ValidationError
|
||||
from django.core.validators import MinLengthValidator
|
||||
from django.db import IntegrityError, models
|
||||
from django.db import models
|
||||
from django.db.models import Q
|
||||
|
||||
from apps.base.messaging import get_messaging_backends
|
||||
from apps.user_management.models import User
|
||||
from common.exceptions import UserNotificationPolicyCouldNotBeDeleted
|
||||
from common.ordered_model.ordered_model import OrderedModel
|
||||
from common.public_primary_keys import generate_public_primary_key, increase_public_primary_key_length
|
||||
|
||||
|
|
@ -66,32 +65,7 @@ def validate_channel_choice(value):
|
|||
raise ValidationError("%(value)s is not a valid option", params={"value": value})
|
||||
|
||||
|
||||
class UserNotificationPolicyQuerySet(models.QuerySet):
|
||||
def create_default_policies_for_user(self, user: User) -> None:
|
||||
if user.notification_policies.filter(important=False).exists():
|
||||
return
|
||||
|
||||
policies_to_create = user.default_notification_policies_defaults
|
||||
|
||||
try:
|
||||
super().bulk_create(policies_to_create)
|
||||
except IntegrityError:
|
||||
pass
|
||||
|
||||
def create_important_policies_for_user(self, user: User) -> None:
|
||||
if user.notification_policies.filter(important=True).exists():
|
||||
return
|
||||
|
||||
policies_to_create = user.important_notification_policies_defaults
|
||||
|
||||
try:
|
||||
super().bulk_create(policies_to_create)
|
||||
except IntegrityError:
|
||||
pass
|
||||
|
||||
|
||||
class UserNotificationPolicy(OrderedModel):
|
||||
objects = UserNotificationPolicyQuerySet.as_manager()
|
||||
order_with_respect_to = ("user_id", "important")
|
||||
|
||||
public_primary_key = models.CharField(
|
||||
|
|
@ -171,12 +145,6 @@ class UserNotificationPolicy(OrderedModel):
|
|||
else:
|
||||
return "Not set"
|
||||
|
||||
def delete(self):
|
||||
if UserNotificationPolicy.objects.filter(important=self.important, user=self.user).count() == 1:
|
||||
raise UserNotificationPolicyCouldNotBeDeleted("Can't delete last user notification policy")
|
||||
else:
|
||||
super().delete()
|
||||
|
||||
|
||||
class NotificationChannelOptions:
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -9,7 +9,6 @@ from apps.base.models.user_notification_policy import (
|
|||
validate_channel_choice,
|
||||
)
|
||||
from apps.base.tests.messaging_backend import TestOnlyBackend
|
||||
from common.exceptions import UserNotificationPolicyCouldNotBeDeleted
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
|
|
@ -84,7 +83,7 @@ def test_extra_messaging_backends_details():
|
|||
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_unable_to_delete_last_notification_policy(
|
||||
def test_can_delete_last_notification_policy(
|
||||
make_organization,
|
||||
make_user_for_organization,
|
||||
make_user_notification_policy,
|
||||
|
|
@ -101,5 +100,4 @@ def test_unable_to_delete_last_notification_policy(
|
|||
)
|
||||
|
||||
first_policy.delete()
|
||||
with pytest.raises(UserNotificationPolicyCouldNotBeDeleted):
|
||||
second_policy.delete()
|
||||
second_policy.delete()
|
||||
|
|
|
|||
|
|
@ -12,7 +12,6 @@ from apps.user_management.models import User
|
|||
from common.api_helpers.exceptions import BadRequest
|
||||
from common.api_helpers.mixins import RateLimitHeadersMixin, UpdateSerializerMixin
|
||||
from common.api_helpers.paginators import FiftyPageSizePaginator
|
||||
from common.exceptions import UserNotificationPolicyCouldNotBeDeleted
|
||||
from common.insight_log import EntityEvent, write_resource_insight_log
|
||||
|
||||
|
||||
|
|
@ -75,10 +74,7 @@ class PersonalNotificationView(RateLimitHeadersMixin, UpdateSerializerMixin, Mod
|
|||
def perform_destroy(self, instance):
|
||||
user = self.request.user
|
||||
prev_state = user.insight_logs_serialized
|
||||
try:
|
||||
instance.delete()
|
||||
except UserNotificationPolicyCouldNotBeDeleted:
|
||||
raise BadRequest(detail="Can't delete last user notification policy")
|
||||
instance.delete()
|
||||
new_state = user.insight_logs_serialized
|
||||
write_resource_insight_log(
|
||||
instance=user,
|
||||
|
|
|
|||
|
|
@ -9,7 +9,7 @@ import pytz
|
|||
from django.conf import settings
|
||||
from django.core.exceptions import ObjectDoesNotExist
|
||||
from django.core.validators import MinLengthValidator
|
||||
from django.db import models, transaction
|
||||
from django.db import models
|
||||
from django.db.models import Q
|
||||
from django.db.models.signals import post_save
|
||||
from django.dispatch import receiver
|
||||
|
|
@ -84,8 +84,6 @@ class UserManager(models.Manager["User"]):
|
|||
|
||||
@staticmethod
|
||||
def sync_for_organization(organization, api_users: list[dict]):
|
||||
from apps.base.models import UserNotificationPolicy
|
||||
|
||||
grafana_users = {user["userId"]: user for user in api_users}
|
||||
existing_user_ids = set(organization.users.all().values_list("user_id", flat=True))
|
||||
|
||||
|
|
@ -105,21 +103,7 @@ class UserManager(models.Manager["User"]):
|
|||
if user["userId"] not in existing_user_ids
|
||||
)
|
||||
|
||||
with transaction.atomic():
|
||||
organization.users.bulk_create(users_to_create, batch_size=5000)
|
||||
# Retrieve primary keys for the newly created users
|
||||
#
|
||||
# If the model’s primary key is an AutoField, the primary key attribute can only be retrieved
|
||||
# on certain databases (currently PostgreSQL, MariaDB 10.5+, and SQLite 3.35+).
|
||||
# On other databases, it will not be set.
|
||||
# https://docs.djangoproject.com/en/4.1/ref/models/querysets/#django.db.models.query.QuerySet.bulk_create
|
||||
created_users = organization.users.exclude(user_id__in=existing_user_ids)
|
||||
|
||||
policies_to_create = ()
|
||||
for user in created_users:
|
||||
policies_to_create = policies_to_create + user.default_notification_policies_defaults
|
||||
policies_to_create = policies_to_create + user.important_notification_policies_defaults
|
||||
UserNotificationPolicy.objects.bulk_create(policies_to_create, batch_size=5000)
|
||||
organization.users.bulk_create(users_to_create, batch_size=5000)
|
||||
|
||||
# delete excess users
|
||||
user_ids_to_delete = existing_user_ids - grafana_users.keys()
|
||||
|
|
@ -429,43 +413,25 @@ class User(models.Model):
|
|||
return PermissionsQuery(permissions__contains=[required_permission])
|
||||
return RoleInQuery(role__lte=permission.fallback_role.value)
|
||||
|
||||
def get_or_create_notification_policies(self, important=False):
|
||||
def get_notification_policies_or_use_default_fallback(
|
||||
self, important=False
|
||||
) -> typing.List["UserNotificationPolicy"]:
|
||||
"""
|
||||
If the user has no notification policies defined, fallback to using e-mail as the notification channel.
|
||||
"""
|
||||
from apps.base.models import UserNotificationPolicy
|
||||
|
||||
if not self.notification_policies.filter(important=important).exists():
|
||||
if important:
|
||||
self.notification_policies.create_important_policies_for_user(self)
|
||||
else:
|
||||
self.notification_policies.create_default_policies_for_user(self)
|
||||
notification_policies = self.notification_policies.filter(important=important)
|
||||
return notification_policies
|
||||
|
||||
@property
|
||||
def default_notification_policies_defaults(self):
|
||||
from apps.base.models import UserNotificationPolicy
|
||||
|
||||
print(self)
|
||||
|
||||
return (
|
||||
UserNotificationPolicy(
|
||||
user=self,
|
||||
step=UserNotificationPolicy.Step.NOTIFY,
|
||||
notify_by=settings.EMAIL_BACKEND_INTERNAL_ID,
|
||||
order=0,
|
||||
),
|
||||
)
|
||||
|
||||
@property
|
||||
def important_notification_policies_defaults(self):
|
||||
from apps.base.models import UserNotificationPolicy
|
||||
|
||||
return (
|
||||
UserNotificationPolicy(
|
||||
user=self,
|
||||
step=UserNotificationPolicy.Step.NOTIFY,
|
||||
notify_by=settings.EMAIL_BACKEND_INTERNAL_ID,
|
||||
important=True,
|
||||
order=0,
|
||||
),
|
||||
)
|
||||
return [
|
||||
UserNotificationPolicy(
|
||||
user=self,
|
||||
step=UserNotificationPolicy.Step.NOTIFY,
|
||||
notify_by=settings.EMAIL_BACKEND_INTERNAL_ID,
|
||||
important=important,
|
||||
order=0,
|
||||
),
|
||||
]
|
||||
return list(self.notification_policies.filter(important=important).all())
|
||||
|
||||
def update_alert_group_table_selected_columns(self, columns: typing.List[AlertGroupTableColumn]) -> None:
|
||||
if self.alert_group_table_selected_columns != columns:
|
||||
|
|
@ -498,9 +464,6 @@ class User(models.Model):
|
|||
# TODO: check whether this signal can be moved to save method of the model
|
||||
@receiver(post_save, sender=User)
|
||||
def listen_for_user_model_save(sender: User, instance: User, created: bool, *args, **kwargs) -> None:
|
||||
if created:
|
||||
instance.notification_policies.create_default_policies_for_user(instance)
|
||||
instance.notification_policies.create_important_policies_for_user(instance)
|
||||
drop_cached_ical_for_custom_events_for_organization.apply_async(
|
||||
(instance.organization_id,),
|
||||
)
|
||||
|
|
|
|||
|
|
@ -118,18 +118,6 @@ def test_sync_users_for_organization(make_organization, make_user_for_organizati
|
|||
assert created_user.name == api_users[1]["name"]
|
||||
assert created_user.avatar_full_url == "https://test.test/test/1234"
|
||||
|
||||
assert created_user.notification_policies.filter(important=False).count() == 1
|
||||
assert (
|
||||
created_user.notification_policies.filter(important=False).first().notify_by
|
||||
== settings.EMAIL_BACKEND_INTERNAL_ID
|
||||
)
|
||||
|
||||
assert created_user.notification_policies.filter(important=True).count() == 1
|
||||
assert (
|
||||
created_user.notification_policies.filter(important=True).first().notify_by
|
||||
== settings.EMAIL_BACKEND_INTERNAL_ID
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.django_db
|
||||
def test_sync_users_for_organization_role_none(make_organization, make_user_for_organization):
|
||||
|
|
|
|||
|
|
@ -3,5 +3,4 @@ from .exceptions import ( # noqa: F401
|
|||
MaintenanceCouldNotBeStartedError,
|
||||
TeamCanNotBeChangedError,
|
||||
UnableToSendDemoAlert,
|
||||
UserNotificationPolicyCouldNotBeDeleted,
|
||||
)
|
||||
|
|
|
|||
|
|
@ -19,10 +19,6 @@ class UnableToSendDemoAlert(OperationCouldNotBePerformedError):
|
|||
pass
|
||||
|
||||
|
||||
class UserNotificationPolicyCouldNotBeDeleted(OperationCouldNotBePerformedError):
|
||||
pass
|
||||
|
||||
|
||||
class BacksyncIntegrationRequestError(Exception):
|
||||
"""Error making request to alert receive channel backsync connection."""
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue