Fix duplicate orders on routes and escalation policies (#2568)

# What this PR does

Fix duplicate `order` values for models `EscalationPolicy` and
`ChannelFilter` using the same approach as
#2278.

- Make internal API action `move_to_position` a part of
[OrderedModelViewSet](https://github.com/grafana/oncall/pull/2568/files#diff-eb62521ccbcb072d1bd2156adeadae3b5895bce6d0d54432a23db3948b0ada54R11-R34),
so all ordered model views use the same logic.
- Make public API serializers for ordered models inherit from
[OrderedModelSerializer](https://github.com/grafana/oncall/pull/2568/files#diff-d749f94af5a49adaf5074325cdfad10ddd5a52dbfd78b49582867ebb9c92fae5R6-R38),
so ordered model views are consistent with each other in public API.
- Remove `order` from plugin fronted, since it's not being used
anywhere. The frontend uses step indices & `move_to_position` action
instead.
- Make escalation snapshot use step indices instead of orders, since
orders are not guaranteed to be sequential (+fix a minor off-by-one bug)

## Which issue(s) this PR fixes

grafana/oncall-private#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)

CHANGELOG.md

@@ -13,6 +13,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Update Slack "invite" feature to use direct paging by @vadimkerr ([#2562](https://github.com/grafana/oncall/pull/2562))
 - Change "Current responders" to "Additional Responders" in web UI by @vadimkerr ([#2567](https://github.com/grafana/oncall/pull/2567))
 
+### Fixed
+
+- Fix duplicate orders on routes and escalation policies by @vadimkerr ([#2568](https://github.com/grafana/oncall/pull/2568))
+
 ## v1.3.14 (2023-07-17)
 
 ### Changed

engine/apps/alerts/escalation_snapshot/snapshot_classes/escalation_snapshot.py

@@ -88,9 +88,7 @@ def executed_escalation_policy_snapshots(self) -> typing.List["EscalationPolicyS
         """
         if self.last_active_escalation_policy_order is None:
             return []
-        elif self.last_active_escalation_policy_order == 0:
-            return [self.escalation_policies_snapshots[0]]
-        return self.escalation_policies_snapshots[: self.last_active_escalation_policy_order]
+        return self.escalation_policies_snapshots[: self.last_active_escalation_policy_order + 1]
 
     def next_step_eta_is_valid(self) -> typing.Optional[bool]:
         """
@@ -147,7 +145,8 @@ def execute_actual_escalation_step(self) -> None:
             self.stop_escalation = execution_result.stop_escalation  # result of STEP_FINAL_RESOLVE
             self.pause_escalation = execution_result.pause_escalation  # result of STEP_NOTIFY_IF_NUM_ALERTS_IN_WINDOW
 
-            last_active_escalation_policy_order = escalation_policy_snapshot.order
+            # use the index of last escalation policy snapshot, since orders are not guaranteed to be sequential
+            last_active_escalation_policy_order = self.escalation_policies_snapshots.index(escalation_policy_snapshot)
 
             if execution_result.start_from_beginning:  # result of STEP_REPEAT_ESCALATION_N_TIMES
                 last_active_escalation_policy_order = None

engine/apps/alerts/migrations/0024_auto_20230718_0953.py

@@ -0,0 +1,54 @@
+# Generated by Django 3.2.20 on 2023-07-18 09:53
+
+from django.db import migrations, models
+import django_migration_linter as linter
+from django.db.models import Count
+
+from common.database import get_random_readonly_database_key_if_present_otherwise_default
+
+
+def fix_duplicate_orders(apps, schema_editor):
+    EscalationPolicy = apps.get_model('alerts', 'EscalationPolicy')
+
+    # 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 (escalation_chain_id, order) tuples that have more than one entry (meaning duplicates)
+    items_with_duplicate_orders = EscalationPolicy.objects.using(db).values(
+        "escalation_chain_id", "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 escalation chain more than once
+    escalation_chain_ids = set(item["escalation_chain_id"] for item in items_with_duplicate_orders)
+
+    for escalation_chain_id in escalation_chain_ids:
+        policies = EscalationPolicy.objects.filter(escalation_chain_id=escalation_chain_id).order_by("order", "id")
+        # assign correct sequential order for each policy starting from 0
+        for idx, policy in enumerate(policies):
+            policy.order = idx
+        EscalationPolicy.objects.bulk_update(policies, fields=["order"])
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('alerts', '0023_auto_20230718_0952'),
+    ]
+
+    operations = [
+        linter.IgnoreMigration(),  # adding a unique constraint after fixing duplicates should be fine
+        migrations.AlterModelOptions(
+            name='escalationpolicy',
+            options={'ordering': ['order']},
+        ),
+        migrations.AlterField(
+            model_name='escalationpolicy',
+            name='order',
+            field=models.PositiveIntegerField(db_index=True, editable=False, null=True),
+        ),
+        migrations.RunPython(fix_duplicate_orders, migrations.RunPython.noop),
+        migrations.AddConstraint(
+            model_name='escalationpolicy',
+            constraint=models.UniqueConstraint(fields=('escalation_chain_id', 'order'), name='unique_escalation_policy_order'),
+        ),
+    ]

engine/apps/alerts/migrations/0025_auto_20230718_1042.py

@@ -0,0 +1,56 @@
+# Generated by Django 3.2.20 on 2023-07-18 10:42
+
+from django.db import migrations, models
+import django_migration_linter as linter
+from django.db.models import Count
+
+from common.database import get_random_readonly_database_key_if_present_otherwise_default
+
+
+def fix_duplicate_orders(apps, schema_editor):
+    ChannelFilter = apps.get_model('alerts', 'ChannelFilter')
+
+    # 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 (alert_receive_channel_id, is_default, order) tuples that have more than one entry (meaning duplicates)
+    items_with_duplicate_orders = ChannelFilter.objects.using(db).values(
+        "alert_receive_channel_id", "is_default", "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 (alert_receive_channel_id, is_default) pair more than once
+    values_to_fix = set((item["alert_receive_channel_id"], item["is_default"]) for item in items_with_duplicate_orders)
+
+    for alert_receive_channel_id, is_default in values_to_fix:
+        channel_filters = ChannelFilter.objects.filter(
+            alert_receive_channel_id=alert_receive_channel_id, is_default=is_default
+        ).order_by("order", "id")
+        # assign correct sequential order for each route starting from 0
+        for idx, channel_filter in enumerate(channel_filters):
+            channel_filter.order = idx
+        ChannelFilter.objects.bulk_update(channel_filters, fields=["order"])
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('alerts', '0024_auto_20230718_0953'),
+    ]
+
+    operations = [
+        linter.IgnoreMigration(),  # adding a unique constraint after fixing duplicates should be fine
+        migrations.AlterModelOptions(
+            name='channelfilter',
+            options={'ordering': ['alert_receive_channel_id', 'is_default', 'order']},
+        ),
+        migrations.AlterField(
+            model_name='channelfilter',
+            name='order',
+            field=models.PositiveIntegerField(db_index=True, editable=False, null=True),
+        ),
+        migrations.RunPython(fix_duplicate_orders, migrations.RunPython.noop),
+        migrations.AddConstraint(
+            model_name='channelfilter',
+            constraint=models.UniqueConstraint(fields=('alert_receive_channel_id', 'is_default', 'order'), name='unique_channel_filter_order'),
+        ),
+    ]

engine/apps/alerts/models/channel_filter.py

@@ -6,10 +6,10 @@
 from django.conf import settings
 from django.core.validators import MinLengthValidator
 from django.db import models
-from ordered_model.models import OrderedModel
 
 from common.jinja_templater import apply_jinja_template
 from common.jinja_templater.apply_jinja_template import JinjaTemplateError, JinjaTemplateWarning
+from common.ordered_model.ordered_model import OrderedModel
 from common.public_primary_keys import generate_public_primary_key, increase_public_primary_key_length
 
 logger = logging.getLogger(__name__)
@@ -34,7 +34,7 @@ class ChannelFilter(OrderedModel):
     Actually it's a Router based on terms now. Not a Filter.
     """
 
-    order_with_respect_to = ("alert_receive_channel", "is_default")
+    order_with_respect_to = ["alert_receive_channel_id", "is_default"]
 
     public_primary_key = models.CharField(
         max_length=20,
@@ -82,11 +82,12 @@ class ChannelFilter(OrderedModel):
     is_default = models.BooleanField(default=False)
 
     class Meta:
-        ordering = (
-            "alert_receive_channel",
-            "is_default",
-            "order",
-        )
+        ordering = ["alert_receive_channel_id", "is_default", "order"]
+        constraints = [
+            models.UniqueConstraint(
+                fields=["alert_receive_channel_id", "is_default", "order"], name="unique_channel_filter_order"
+            )
+        ]
 
     def __str__(self):
         return f"{self.pk}: {self.filtering_term or 'default'}"

engine/apps/alerts/models/escalation_policy.py

@@ -3,8 +3,8 @@
 from django.conf import settings
 from django.core.validators import MinLengthValidator
 from django.db import models
-from ordered_model.models import OrderedModel
 
+from common.ordered_model.ordered_model import OrderedModel
 from common.public_primary_keys import generate_public_primary_key, increase_public_primary_key_length
 
 
@@ -23,7 +23,7 @@ def generate_public_primary_key_for_escalation_policy():
 
 
 class EscalationPolicy(OrderedModel):
-    order_with_respect_to = "escalation_chain"
+    order_with_respect_to = ["escalation_chain_id"]
 
     MAX_TIMES_REPEAT = 5
 
@@ -312,6 +312,12 @@ class EscalationPolicy(OrderedModel):
     num_alerts_in_window = models.PositiveIntegerField(null=True, default=None)
     num_minutes_in_window = models.PositiveIntegerField(null=True, default=None)
 
+    class Meta:
+        ordering = ["order"]
+        constraints = [
+            models.UniqueConstraint(fields=["escalation_chain_id", "order"], name="unique_escalation_policy_order")
+        ]
+
     def __str__(self):
         return f"{self.pk}: {self.step_type_verbal}"
 

engine/apps/alerts/tests/test_escalation_snapshot.py

@@ -213,5 +213,56 @@ def test_executed_escalation_policy_snapshots(escalation_snapshot_test_setup):
         escalation_snapshot.escalation_policies_snapshots[0]
     ]
 
-    escalation_snapshot.last_active_escalation_policy_order = len(escalation_snapshot.escalation_policies_snapshots)
+    escalation_snapshot.last_active_escalation_policy_order = len(escalation_snapshot.escalation_policies_snapshots) - 1
     assert escalation_snapshot.executed_escalation_policy_snapshots == escalation_snapshot.escalation_policies_snapshots
+
+
+@pytest.mark.django_db
+def test_escalation_snapshot_non_sequential_orders(
+    make_organization,
+    make_alert_receive_channel,
+    make_escalation_chain,
+    make_channel_filter,
+    make_escalation_policy,
+    make_alert_group,
+):
+    organization = make_organization()
+
+    alert_receive_channel = make_alert_receive_channel(organization)
+
+    escalation_chain = make_escalation_chain(organization)
+    channel_filter = make_channel_filter(
+        alert_receive_channel,
+        escalation_chain=escalation_chain,
+        notification_backends={"BACKEND": {"channel_id": "abc123"}},
+    )
+
+    step_1 = make_escalation_policy(
+        escalation_chain=channel_filter.escalation_chain,
+        escalation_policy_step=EscalationPolicy.STEP_WAIT,
+        order=12,
+    )
+    step_2 = make_escalation_policy(
+        escalation_chain=channel_filter.escalation_chain,
+        escalation_policy_step=EscalationPolicy.STEP_WAIT,
+        order=42,
+    )
+
+    alert_group = make_alert_group(alert_receive_channel, channel_filter=channel_filter)
+    alert_group.raw_escalation_snapshot = alert_group.build_raw_escalation_snapshot()
+    alert_group.save()
+
+    escalation_snapshot = alert_group.escalation_snapshot
+    assert escalation_snapshot.last_active_escalation_policy_order is None
+    assert escalation_snapshot.next_active_escalation_policy_snapshot.id == step_1.id
+
+    escalation_snapshot.execute_actual_escalation_step()
+    assert escalation_snapshot.last_active_escalation_policy_order == 0
+    assert escalation_snapshot.next_active_escalation_policy_snapshot.id == step_2.id
+
+    escalation_snapshot.execute_actual_escalation_step()
+    assert escalation_snapshot.last_active_escalation_policy_order == 1
+    assert escalation_snapshot.next_active_escalation_policy_snapshot is None
+
+    policy_ids = [p.id for p in escalation_snapshot.executed_escalation_policy_snapshots]
+    assert policy_ids == [step_1.id, step_2.id]

engine/apps/api/serializers/channel_filter.py

@@ -7,12 +7,12 @@
 from apps.telegram.models import TelegramToOrganizationConnector
 from common.api_helpers.custom_fields import OrganizationFilteredPrimaryKeyRelatedField
 from common.api_helpers.exceptions import BadRequest
-from common.api_helpers.mixins import EagerLoadingMixin, OrderedModelSerializerMixin
+from common.api_helpers.mixins import EagerLoadingMixin
 from common.jinja_templater.apply_jinja_template import JinjaTemplateError
 from common.utils import is_regex_valid
 
 
-class ChannelFilterSerializer(OrderedModelSerializerMixin, EagerLoadingMixin, serializers.ModelSerializer):
+class ChannelFilterSerializer(EagerLoadingMixin, serializers.ModelSerializer):
     id = serializers.CharField(read_only=True, source="public_primary_key")
     alert_receive_channel = OrganizationFilteredPrimaryKeyRelatedField(queryset=AlertReceiveChannel.objects)
     escalation_chain = OrganizationFilteredPrimaryKeyRelatedField(
@@ -27,7 +27,6 @@ class ChannelFilterSerializer(OrderedModelSerializerMixin, EagerLoadingMixin, se
         queryset=TelegramToOrganizationConnector.objects, filter_field="organization", allow_null=True, required=False
     )
     telegram_channel_details = serializers.SerializerMethodField()
-    order = serializers.IntegerField(required=False)
     filtering_term_as_jinja2 = serializers.SerializerMethodField()
     filtering_term = serializers.CharField(required=False, allow_null=True, allow_blank=True)
 
@@ -37,7 +36,6 @@ class Meta:
         model = ChannelFilter
         fields = [
             "id",
-            "order",
             "alert_receive_channel",
             "escalation_chain",
             "slack_channel",
@@ -148,7 +146,6 @@ class Meta:
         model = ChannelFilter
         fields = [
             "id",
-            "order",
             "alert_receive_channel",
             "escalation_chain",
             "slack_channel",
@@ -181,14 +178,8 @@ def to_representation(self, obj):
         return result
 
     def create(self, validated_data):
-        order = validated_data.pop("order", None)
-        if order is not None:
-            alert_receive_channel_id = validated_data.get("alert_receive_channel")
-            self._validate_order(order, {"alert_receive_channel_id": alert_receive_channel_id, "is_default": False})
-            instance = super().create(validated_data)
-            self._change_position(order, instance)
-        else:
-            instance = super().create(validated_data)
+        instance = super().create(validated_data)
+        instance.to_index(0)  # the new route should be the first one
         return instance
 
 
@@ -200,18 +191,8 @@ class Meta(ChannelFilterCreateSerializer.Meta):
         extra_kwargs = {"filtering_term": {"required": False}}
 
     def update(self, instance, validated_data):
-        order = validated_data.get("order")
         filtering_term = validated_data.get("filtering_term")
-
-        if instance.is_default and order is not None and instance.order != order:
-            raise BadRequest(detail="The order of default channel filter cannot be changed")
-
         if instance.is_default and filtering_term is not None:
             raise BadRequest(detail="Filtering term of default channel filter cannot be changed")
 
-        if order is not None:
-            self._validate_order(
-                order, {"alert_receive_channel_id": instance.alert_receive_channel_id, "is_default": False}
-            )
-            self._change_position(order, instance)
         return super().update(instance, validated_data)

engine/apps/api/serializers/escalation_policy.py

@@ -85,7 +85,6 @@ class Meta:
         model = EscalationPolicy
         fields = [
             "id",
-            "order",
             "step",
             "wait_delay",
             "escalation_chain",
@@ -101,7 +100,6 @@ class Meta:
             "notify_to_group",
             "important",
         ]
-        read_only_fields = ["order"]
 
     SELECT_RELATED = [
         "escalation_chain",
@@ -199,7 +197,6 @@ def _convert_to_important_step_if_needed(validated_data):
 
 class EscalationPolicyCreateSerializer(EscalationPolicySerializer):
     class Meta(EscalationPolicySerializer.Meta):
-        read_only_fields = ["order"]
         extra_kwargs = {"escalation_chain": {"required": True, "allow_null": False}}
 
     def create(self, validated_data):
@@ -212,7 +209,7 @@ class EscalationPolicyUpdateSerializer(EscalationPolicySerializer):
     escalation_chain = serializers.CharField(read_only=True, source="escalation_chain.public_primary_key")
 
     class Meta(EscalationPolicySerializer.Meta):
-        read_only_fields = ["order", "escalation_chain"]
+        read_only_fields = ["escalation_chain"]
 
     def update(self, instance, validated_data):
         step = validated_data.get("step", instance.step)