oncall-engine/engine/apps/api/serializers/user_notification_policy.py

112 lines
4.4 KiB
Python
Raw Permalink Normal View History

from datetime import timedelta
from rest_framework import serializers
from apps.base.models import UserNotificationPolicy
from apps.base.models.user_notification_policy import NotificationChannelAPIOptions
from apps.user_management.models import User
from common.api_helpers.custom_fields import DurationSecondsField, OrganizationFilteredPrimaryKeyRelatedField
Fix duplicate orders for user notification policies (#2278) # What this PR does Fixes an issue when multiple user notification policies have duplicated order values, leading to the following unexpected behaviours: 1. Not possible to rearrange notification policies that have duplicated orders. 2. The notification system only executes the first policy from each order group. For example, if there are policies with orders `[0, 0, 0, 0]`, only the first policy will be executed, and all others will be skipped. So the user will see four policies in the UI, while only one of them will be actually executed. This PR fixes the issue by adding a unique index on `(user_id, important, order)` for `UserNotificationPolicy` model. However, it's not possible to add that unique index using the ordering library that we use due to it's implementation details. I added a new abstract Django model `OrderedModel` that's able to work with such unique indices + under concurrent load. Important info on this new `OrderedModel` abstract model: - Orders are unique on the DB level - Orders are allowed to be non-consecutive, for example order sequence `[100, 150, 400]` is valid - When deleting an instance, orders of other instances don't change. This is a notable difference from the library we use. I think it's better to only delete the instance without changing any other orders, because it reduces the number of dependencies between instances (e.g. Terraform drift will be much smaller this way if a policy is deleted via the web UI). ## Which issue(s) this PR fixes Related to https://github.com/grafana/oncall-private/issues/1680 ## 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] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required)
2023-06-21 12:13:56 +01:00
from common.api_helpers.exceptions import Forbidden
from common.api_helpers.mixins import EagerLoadingMixin
# This serializer should not be user directly
class UserNotificationPolicyBaseSerializer(EagerLoadingMixin, serializers.ModelSerializer):
id = serializers.CharField(read_only=True, source="public_primary_key")
notify_by = serializers.ChoiceField(
read_only=False,
required=False,
default=UserNotificationPolicy.NotificationChannel.SLACK,
choices=NotificationChannelAPIOptions.AVAILABLE_FOR_USE,
)
step = serializers.ChoiceField(
read_only=False,
required=False,
default=UserNotificationPolicy.Step.NOTIFY,
choices=UserNotificationPolicy.Step.choices,
)
wait_delay = DurationSecondsField(
required=False,
allow_null=True,
min_value=timedelta(minutes=1),
max_value=timedelta(hours=24),
)
SELECT_RELATED = [
"user",
]
class Meta:
model = UserNotificationPolicy
fields = ["id", "step", "notify_by", "wait_delay", "important", "user"]
# Field "order" is not consumed by the plugin frontend, but is used by the mobile app
# TODO: remove this field when the mobile app is updated
fields += ["order"]
Fix duplicate orders for user notification policies (#2278) # What this PR does Fixes an issue when multiple user notification policies have duplicated order values, leading to the following unexpected behaviours: 1. Not possible to rearrange notification policies that have duplicated orders. 2. The notification system only executes the first policy from each order group. For example, if there are policies with orders `[0, 0, 0, 0]`, only the first policy will be executed, and all others will be skipped. So the user will see four policies in the UI, while only one of them will be actually executed. This PR fixes the issue by adding a unique index on `(user_id, important, order)` for `UserNotificationPolicy` model. However, it's not possible to add that unique index using the ordering library that we use due to it's implementation details. I added a new abstract Django model `OrderedModel` that's able to work with such unique indices + under concurrent load. Important info on this new `OrderedModel` abstract model: - Orders are unique on the DB level - Orders are allowed to be non-consecutive, for example order sequence `[100, 150, 400]` is valid - When deleting an instance, orders of other instances don't change. This is a notable difference from the library we use. I think it's better to only delete the instance without changing any other orders, because it reduces the number of dependencies between instances (e.g. Terraform drift will be much smaller this way if a policy is deleted via the web UI). ## Which issue(s) this PR fixes Related to https://github.com/grafana/oncall-private/issues/1680 ## 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] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required)
2023-06-21 12:13:56 +01:00
read_only_fields = ["order"]
def to_internal_value(self, data):
data = self._notify_by_to_internal_value(data)
return super().to_internal_value(data)
def to_representation(self, instance):
result = super().to_representation(instance)
result = self._notify_by_to_representation(instance, result)
return result
# _notify_by_to_internal_value and _notify_by_to_representation are exists because of in EscalationPolicy model
# notify_by field has default value NotificationChannel.SLACK and not nullable
# We don't want any notify_by value in response if step != Step.NOTIFY
def _notify_by_to_internal_value(self, data):
if not data.get("notify_by", None):
data["notify_by"] = UserNotificationPolicy.NotificationChannel.SLACK
return data
def _notify_by_to_representation(self, instance, result):
if instance.step != UserNotificationPolicy.Step.NOTIFY:
result["notify_by"] = None
return result
class UserNotificationPolicySerializer(UserNotificationPolicyBaseSerializer):
user = OrganizationFilteredPrimaryKeyRelatedField(
queryset=User.objects,
required=False,
allow_null=True,
many=False,
display_func=lambda instance: instance.username,
)
notify_by = serializers.ChoiceField(
choices=NotificationChannelAPIOptions.AVAILABLE_FOR_USE,
default=NotificationChannelAPIOptions.DEFAULT_NOTIFICATION_CHANNEL,
)
def create(self, validated_data):
Fix duplicate orders for user notification policies (#2278) # What this PR does Fixes an issue when multiple user notification policies have duplicated order values, leading to the following unexpected behaviours: 1. Not possible to rearrange notification policies that have duplicated orders. 2. The notification system only executes the first policy from each order group. For example, if there are policies with orders `[0, 0, 0, 0]`, only the first policy will be executed, and all others will be skipped. So the user will see four policies in the UI, while only one of them will be actually executed. This PR fixes the issue by adding a unique index on `(user_id, important, order)` for `UserNotificationPolicy` model. However, it's not possible to add that unique index using the ordering library that we use due to it's implementation details. I added a new abstract Django model `OrderedModel` that's able to work with such unique indices + under concurrent load. Important info on this new `OrderedModel` abstract model: - Orders are unique on the DB level - Orders are allowed to be non-consecutive, for example order sequence `[100, 150, 400]` is valid - When deleting an instance, orders of other instances don't change. This is a notable difference from the library we use. I think it's better to only delete the instance without changing any other orders, because it reduces the number of dependencies between instances (e.g. Terraform drift will be much smaller this way if a policy is deleted via the web UI). ## Which issue(s) this PR fixes Related to https://github.com/grafana/oncall-private/issues/1680 ## 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] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required)
2023-06-21 12:13:56 +01:00
user = validated_data.get("user") or self.context["request"].user
if not user.self_or_has_user_settings_admin_permission(
user_to_check=self.context["request"].user, organization=self.context["request"].auth.organization
):
raise Forbidden()
return UserNotificationPolicy.objects.create(**validated_data)
class UserNotificationPolicyUpdateSerializer(UserNotificationPolicyBaseSerializer):
user = OrganizationFilteredPrimaryKeyRelatedField(
many=False,
read_only=True,
display_func=lambda instance: instance.username,
)
class Meta(UserNotificationPolicyBaseSerializer.Meta):
read_only_fields = UserNotificationPolicyBaseSerializer.Meta.read_only_fields + ["user", "important"]
def update(self, instance, validated_data):
if not instance.user.self_or_has_user_settings_admin_permission(
user_to_check=self.context["request"].user, organization=self.context["request"].user.organization
):
raise Forbidden()
if validated_data.get("step") == UserNotificationPolicy.Step.WAIT and not validated_data.get("wait_delay"):
validated_data["wait_delay"] = UserNotificationPolicy.FIVE_MINUTES
return super().update(instance, validated_data)