Fix duplicate orders for user notification policies

CHANGELOG.md

@@ -20,6 +20,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Change mobile shift notifications title and subtitle by @imtoori ([#2288](https://github.com/grafana/oncall/pull/2288))
 
+## Fixed
+
+- Fix duplicate orders for user notification policies by @vadimkerr ([#2278](https://github.com/grafana/oncall/pull/2278))
+
 ## v1.2.45 (2023-06-19)
 
 ### Changed

docker-compose-developer.yml

@@ -208,7 +208,7 @@ services:
     container_name: mysql
     labels: *oncall-labels
     image: mysql:8.0.32
-    command: --default-authentication-plugin=mysql_native_password --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci
+    command: --default-authentication-plugin=mysql_native_password --character-set-server=utf8mb4 --collation-server=utf8mb4_unicode_ci --max_connections=1024
     restart: always
     environment:
       MYSQL_ROOT_PASSWORD: empty

engine/apps/api/serializers/user_notification_policy.py

@@ -7,7 +7,7 @@
 from apps.base.models.user_notification_policy import NotificationChannelAPIOptions
 from apps.user_management.models import User
 from common.api_helpers.custom_fields import OrganizationFilteredPrimaryKeyRelatedField
-from common.api_helpers.exceptions import BadRequest, Forbidden
+from common.api_helpers.exceptions import Forbidden
 from common.api_helpers.mixins import EagerLoadingMixin
 
 
@@ -34,6 +34,7 @@ class UserNotificationPolicyBaseSerializer(EagerLoadingMixin, serializers.ModelS
     class Meta:
         model = UserNotificationPolicy
         fields = ["id", "step", "order", "notify_by", "wait_delay", "important", "user"]
+        read_only_fields = ["order"]
 
     def to_internal_value(self, data):
         if data.get("wait_delay", None):
@@ -67,7 +68,6 @@ def _notify_by_to_representation(self, instance, result):
 
 
 class UserNotificationPolicySerializer(UserNotificationPolicyBaseSerializer):
-    prev_step = serializers.CharField(required=False, write_only=True, allow_null=True)
     user = OrganizationFilteredPrimaryKeyRelatedField(
         queryset=User.objects,
         required=False,
@@ -80,36 +80,16 @@ class UserNotificationPolicySerializer(UserNotificationPolicyBaseSerializer):
         default=NotificationChannelAPIOptions.DEFAULT_NOTIFICATION_CHANNEL,
     )
 
-    class Meta(UserNotificationPolicyBaseSerializer.Meta):
-        fields = [*UserNotificationPolicyBaseSerializer.Meta.fields, "prev_step"]
-        read_only_fields = ("order",)
-
     def create(self, validated_data):
-        prev_step = validated_data.pop("prev_step", None)
-
-        user = validated_data.get("user")
+        user = validated_data.get("user") or self.context["request"].user
         organization = self.context["request"].auth.organization
 
-        if not user:
-            user = self.context["request"].user
-
         self_or_admin = user.self_or_admin(user_to_check=self.context["request"].user, organization=organization)
         if not self_or_admin:
             raise Forbidden()
 
-        if prev_step is not None:
-            try:
-                prev_step = UserNotificationPolicy.objects.get(public_primary_key=prev_step)
-            except UserNotificationPolicy.DoesNotExist:
-                raise BadRequest(detail="Prev step does not exist")
-            if prev_step.user != user or prev_step.important != validated_data.get("important", False):
-                raise BadRequest(detail="UserNotificationPolicy can be created only with the same user and importance")
-            instance = UserNotificationPolicy.objects.create(**validated_data)
-            instance.to(prev_step.order + 1)
-            return instance
-        else:
-            instance = UserNotificationPolicy.objects.create(**validated_data)
-            return instance
+        instance = UserNotificationPolicy.objects.create(**validated_data)
+        return instance
 
 
 class UserNotificationPolicyUpdateSerializer(UserNotificationPolicyBaseSerializer):

engine/apps/api/tests/test_user_notification_policy.py

@@ -110,7 +110,7 @@ def test_user_cant_create_notification_policy_for_user(
 
 
 @pytest.mark.django_db
-def test_create_notification_policy_from_step(
+def test_create_notification_policy_order_is_ignored(
     user_notification_policy_internal_api_setup,
     make_user_auth_headers,
 ):
@@ -121,7 +121,7 @@ def test_create_notification_policy_from_step(
     url = reverse("api-internal:notification_policy-list")
 
     data = {
-        "prev_step": wait_notification_step.public_primary_key,
+        "position": 2023,
         "step": UserNotificationPolicy.Step.NOTIFY,
         "notify_by": UserNotificationPolicy.NotificationChannel.SLACK,
         "wait_delay": None,
@@ -130,26 +130,19 @@ def test_create_notification_policy_from_step(
     }
     response = client.post(url, data, format="json", **make_user_auth_headers(admin, token))
     assert response.status_code == status.HTTP_201_CREATED
-    assert response.data["order"] == 1
+    assert response.data["order"] == 2
 
 
 @pytest.mark.django_db
-def test_create_invalid_notification_policy(user_notification_policy_internal_api_setup, make_user_auth_headers):
+def test_move_to_position_position_error(user_notification_policy_internal_api_setup, make_user_auth_headers):
     token, steps, users = user_notification_policy_internal_api_setup
-    wait_notification_step, _, _, _ = steps
     admin, _ = users
+    step = steps[0]
     client = APIClient()
-    url = reverse("api-internal:notification_policy-list")
+    url = reverse("api-internal:notification_policy-move-to-position", kwargs={"pk": step.public_primary_key})
 
-    data = {
-        "prev_step": wait_notification_step.public_primary_key,
-        "step": UserNotificationPolicy.Step.NOTIFY,
-        "notify_by": UserNotificationPolicy.NotificationChannel.SLACK,
-        "wait_delay": None,
-        "important": True,
-        "user": admin.public_primary_key,
-    }
-    response = client.post(url, data, format="json", **make_user_auth_headers(admin, token))
+    # position value only can be 0 or 1 for this test setup, because there are only 2 steps
+    response = client.put(f"{url}?position=2", content_type="application/json", **make_user_auth_headers(admin, token))
     assert response.status_code == status.HTTP_400_BAD_REQUEST
 
 
@@ -221,7 +214,7 @@ def test_admin_can_move_user_step(user_notification_policy_internal_api_setup, m
         "api-internal:notification_policy-move-to-position", kwargs={"pk": second_user_step.public_primary_key}
     )
 
-    response = client.put(f"{url}?position=1", content_type="application/json", **make_user_auth_headers(admin, token))
+    response = client.put(f"{url}?position=0", content_type="application/json", **make_user_auth_headers(admin, token))
     assert response.status_code == status.HTTP_200_OK
 
 

engine/apps/api/views/user_notification_policy.py

@@ -142,7 +142,12 @@ def perform_destroy(self, instance):
     def move_to_position(self, request, pk):
         instance = self.get_object()
         position = get_move_to_position_param(request)
-        instance.to(position)
+
+        try:
+            instance.to_index(position)
+        except IndexError:
+            raise BadRequest(detail="Invalid position")
+
         return Response(status=status.HTTP_200_OK)
 
     @action(detail=False, methods=["get"])

engine/apps/base/migrations/0004_auto_20230616_1510.py

@@ -0,0 +1,50 @@
+# Generated by Django 3.2.19 on 2023-06-16 15:10
+
+from django.db import migrations, models
+from django.db.models import Count
+
+from common.database import get_random_readonly_database_key_if_present_otherwise_default
+import django_migration_linter as linter
+
+
+def fix_duplicate_order_user_notification_policy(apps, schema_editor):
+    UserNotificationPolicy = apps.get_model('base', 'UserNotificationPolicy')
+
+    # it should be safe to use a readonly database because duplicates are pretty infrequent
+    db = get_random_readonly_database_key_if_present_otherwise_default()
+
+    # find all (user_id, important, order) tuples that have more than one entry (meaning duplicates)
+    items_with_duplicate_orders = UserNotificationPolicy.objects.using(db).values(
+        "user_id", "important", "order"
+    ).annotate(count=Count("order")).order_by().filter(count__gt=1)  # use order_by() to reset any existing ordering
+
+    # make sure we don't fix the same (user_id, important) pair more than once
+    values_to_fix = set((item["user_id"], item["important"]) for item in items_with_duplicate_orders)
+
+    for user_id, important in values_to_fix:
+        policies = UserNotificationPolicy.objects.filter(user_id=user_id, important=important).order_by("order", "id")
+        # assign correct sequential order for each policy starting from 0
+        for idx, policy in enumerate(policies):
+            policy.order = idx
+        UserNotificationPolicy.objects.bulk_update(policies, fields=["order"])
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('base', '0003_delete_organizationlogrecord'),
+    ]
+
+    operations = [
+        linter.IgnoreMigration(),  # adding a unique constraint after fixing duplicates should be fine
+        migrations.AlterField(
+            model_name='usernotificationpolicy',
+            name='order',
+            field=models.PositiveIntegerField(db_index=True, editable=False, null=True),
+        ),
+        migrations.RunPython(fix_duplicate_order_user_notification_policy, migrations.RunPython.noop),
+        migrations.AddConstraint(
+            model_name='usernotificationpolicy',
+            constraint=models.UniqueConstraint(fields=('user_id', 'important', 'order'), name='unique_user_notification_policy_order'),
+        ),
+    ]