From abedea72bfca62673babbbdf604c6cd3bdde2db5 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Fri, 5 Jul 2024 15:08:17 -0400 Subject: [PATCH] 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. --- .../personal_notification_rules.md | 4 +- .../incident_log_builder.py | 13 +++- engine/apps/alerts/tasks/notify_group.py | 10 +-- engine/apps/alerts/tasks/notify_user.py | 16 ++-- .../api/views/user_notification_policy.py | 8 +- .../base/models/user_notification_policy.py | 34 +------- .../tests/test_user_notification_policy.py | 6 +- .../views/personal_notifications.py | 6 +- engine/apps/user_management/models/user.py | 77 +++++-------------- .../apps/user_management/tests/test_sync.py | 12 --- engine/common/exceptions/__init__.py | 1 - engine/common/exceptions/exceptions.py | 4 - 12 files changed, 50 insertions(+), 141 deletions(-) diff --git a/docs/sources/oncall-api-reference/personal_notification_rules.md b/docs/sources/oncall-api-reference/personal_notification_rules.md index 225ce57f..422749b2 100644 --- a/docs/sources/oncall-api-reference/personal_notification_rules.md +++ b/docs/sources/oncall-api-reference/personal_notification_rules.md @@ -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** diff --git a/engine/apps/alerts/incident_log_builder/incident_log_builder.py b/engine/apps/alerts/incident_log_builder/incident_log_builder.py index d2d88db4..4dcf586a 100644 --- a/engine/apps/alerts/incident_log_builder/incident_log_builder.py +++ b/engine/apps/alerts/incident_log_builder/incident_log_builder.py @@ -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 diff --git a/engine/apps/alerts/tasks/notify_group.py b/engine/apps/alerts/tasks/notify_group.py index 0d95e528..6787c040 100644 --- a/engine/apps/alerts/tasks/notify_group.py +++ b/engine/apps/alerts/tasks/notify_group.py @@ -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 User Group" diff --git a/engine/apps/alerts/tasks/notify_user.py b/engine/apps/alerts/tasks/notify_user.py index 972f2033..2a197338 100644 --- a/engine/apps/alerts/tasks/notify_user.py +++ b/engine/apps/alerts/tasks/notify_user.py @@ -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 ) diff --git a/engine/apps/api/views/user_notification_policy.py b/engine/apps/api/views/user_notification_policy.py index 92e1a151..6a1311c2 100644 --- a/engine/apps/api/views/user_notification_policy.py +++ b/engine/apps/api/views/user_notification_policy.py @@ -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, diff --git a/engine/apps/base/models/user_notification_policy.py b/engine/apps/base/models/user_notification_policy.py index df854a82..1593b3ca 100644 --- a/engine/apps/base/models/user_notification_policy.py +++ b/engine/apps/base/models/user_notification_policy.py @@ -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: """ diff --git a/engine/apps/base/tests/test_user_notification_policy.py b/engine/apps/base/tests/test_user_notification_policy.py index 41354491..7997f2bd 100644 --- a/engine/apps/base/tests/test_user_notification_policy.py +++ b/engine/apps/base/tests/test_user_notification_policy.py @@ -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() diff --git a/engine/apps/public_api/views/personal_notifications.py b/engine/apps/public_api/views/personal_notifications.py index b1288b4b..44b251a3 100644 --- a/engine/apps/public_api/views/personal_notifications.py +++ b/engine/apps/public_api/views/personal_notifications.py @@ -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, diff --git a/engine/apps/user_management/models/user.py b/engine/apps/user_management/models/user.py index d121719e..b7d99200 100644 --- a/engine/apps/user_management/models/user.py +++ b/engine/apps/user_management/models/user.py @@ -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,), ) diff --git a/engine/apps/user_management/tests/test_sync.py b/engine/apps/user_management/tests/test_sync.py index 7502557c..3cfd875a 100644 --- a/engine/apps/user_management/tests/test_sync.py +++ b/engine/apps/user_management/tests/test_sync.py @@ -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): diff --git a/engine/common/exceptions/__init__.py b/engine/common/exceptions/__init__.py index ec543ab8..75622cd2 100644 --- a/engine/common/exceptions/__init__.py +++ b/engine/common/exceptions/__init__.py @@ -3,5 +3,4 @@ from .exceptions import ( # noqa: F401 MaintenanceCouldNotBeStartedError, TeamCanNotBeChangedError, UnableToSendDemoAlert, - UserNotificationPolicyCouldNotBeDeleted, ) diff --git a/engine/common/exceptions/exceptions.py b/engine/common/exceptions/exceptions.py index f176812b..9908e3bb 100644 --- a/engine/common/exceptions/exceptions.py +++ b/engine/common/exceptions/exceptions.py @@ -19,10 +19,6 @@ class UnableToSendDemoAlert(OperationCouldNotBePerformedError): pass -class UserNotificationPolicyCouldNotBeDeleted(OperationCouldNotBePerformedError): - pass - - class BacksyncIntegrationRequestError(Exception): """Error making request to alert receive channel backsync connection."""