Fix duplicate orders on routes and escalation policies #2568

vstpme · 2023-07-18T11:09:34Z

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, so all ordered model views use the same logic.
Make public API serializers for ordered models inherit from OrderedModelSerializer, 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

https://github.com/grafana/oncall-private/issues/1680

Checklist

Unit, integration, and e2e (if applicable) tests updated
Documentation added (or pr:no public docs PR label added if not required)
CHANGELOG.md updated (or pr:no changelog PR label added if not required)

vstpme · 2023-07-18T15:07:58Z

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

This is similar to user notification migration from the previous PR.

vstpme · 2023-07-18T15:08:05Z

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

This is similar to user notification migration from the previous PR.

vstpme · 2023-07-18T15:08:40Z

engine/apps/alerts/models/channel_filter.py

-        )
+        ordering = ["alert_receive_channel_id", "is_default", "order"]
+        constraints = [
+            models.UniqueConstraint(


Adding a unique constraint to enforce order uniqueness on DB level for ChannelFilter

vstpme · 2023-07-18T15:09:15Z

engine/apps/alerts/models/escalation_policy.py

+    class Meta:
+        ordering = ["order"]
+        constraints = [
+            models.UniqueConstraint(fields=["escalation_chain_id", "order"], name="unique_escalation_policy_order")


Adding a unique constraint to enforce order uniqueness on DB level for EscalationPolicy

vstpme · 2023-07-18T15:11:08Z

engine/apps/api/serializers/channel_filter.py

@@ -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)


order is not used by plugin frontend, removing it from the serializer

vstpme · 2023-07-18T15:22:32Z

engine/apps/public_api/serializers/personal_notification_rules.py

    id = serializers.CharField(read_only=True, source="public_primary_key")
    user_id = UserIdField(required=True, source="user")
-    position = serializers.IntegerField(required=False, source="order")


position and manual_order are now handled by parent class OrderedModelSerializer

vstpme · 2023-07-18T15:22:40Z

engine/apps/public_api/serializers/routes.py

@@ -148,7 +148,6 @@ class ChannelFilterSerializer(BaseChannelFilterSerializer):
    telegram = serializers.DictField(required=False)
    routing_type = RoutingTypeField(allow_null=False, required=False, source="filtering_term_type")
    routing_regex = serializers.CharField(allow_null=False, required=True, source="filtering_term")
-    position = serializers.IntegerField(required=False, source="order")


position and manual_order are now handled by parent class OrderedModelSerializer

vstpme · 2023-07-18T15:23:25Z

engine/common/api_helpers/mixins.py

@@ -141,38 +141,6 @@ def handle_exception(self, exc):
        return super().handle_exception(exc)


-class OrderedModelSerializerMixin:


This mixin is superseded by OrderedModelSerializer

vstpme · 2023-07-18T15:24:55Z

grafana-plugin/src/containers/ChannelFilterForm/ChannelFilterForm.tsx

@@ -64,7 +64,6 @@ const ChannelFilterForm = observer((props: ChannelFilterFormProps) => {
  const onUpdateClickCallback = useCallback(() => {
    (id === 'new'
      ? alertReceiveChannelStore.createChannelFilter({
-          order: 0,


order is set by backend here

vstpme · 2023-07-18T15:25:51Z

grafana-plugin/src/models/channel_filter.ts

@@ -4,7 +4,6 @@ import { TelegramChannel } from 'models/telegram_channel/telegram_channel.types'

 export interface ChannelFilter {
  id: string;
-  order: number;


Plugin frontend doesn't use order anywhere (it uses step indices and move_to_position action instead)

vstpme · 2023-07-18T16:27:44Z

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

-        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]


Off-by-one bug making executed_escalation_policy_snapshots smaller than it should be (it probably affects the escalation auditor task)

good catch 👍 thanks for patching this

joeyorlando

nice work! the new classes in common/ordered_model deduplicate a lot of logic shared between channel filters, escalation policies, and user notification policies 🙌

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

engine/apps/alerts/tests/test_escalation_snapshot.py

joeyorlando · 2023-07-18T16:50:55Z

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

-        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]


good catch 👍 thanks for patching this

engine/apps/public_api/serializers/personal_notification_rules.py

engine/apps/base/tests/test_ordered_model.py

# 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)

vstpme added 2 commits July 18, 2023 11:37

EscalationPolicy + internal API

174132a

unify internal API move_to_position

2dce985

vstpme added the pr:no public docs Added to a PR that does not require public documentation updates label Jul 18, 2023

vstpme added 13 commits July 18, 2023 13:00

remove order from internal API and frontend

97b23a2

bring back order for mobile

ebf9df8

add todos

dc3e169

public API

ecc0457

add todo

02a3776

move ordered_model to common

c0b8541

migrations

7c4fa65

test

2118323

tests

80c43dc

tests

6fcd51a

Merge branch 'dev' into vadimkerr/fix-duplicate-order

0642b17

changelog

3b531ee

migrations

5a27101

vstpme commented Jul 18, 2023

View reviewed changes

vstpme marked this pull request as ready for review July 18, 2023 15:32

vstpme requested review from a team July 18, 2023 15:32

fix escalation snapshot

7520963

vstpme commented Jul 18, 2023

View reviewed changes

fix

7ad4284

joeyorlando approved these changes Jul 18, 2023

View reviewed changes

vstpme added 2 commits July 18, 2023 18:11

use OrderedModelSerializer.Meta.fields

2d169a2

move tests to common dir

ef336bc

vstpme enabled auto-merge July 18, 2023 17:16

vstpme added this pull request to the merge queue Jul 18, 2023

Merged via the queue into dev with commit 602ed53 Jul 18, 2023

vstpme deleted the vadimkerr/fix-duplicate-order branch July 18, 2023 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix duplicate orders on routes and escalation policies #2568

Fix duplicate orders on routes and escalation policies #2568

vstpme commented Jul 18, 2023 •

edited

Loading

vstpme Jul 18, 2023

vstpme Jul 18, 2023

vstpme Jul 18, 2023

vstpme Jul 18, 2023

vstpme Jul 18, 2023

vstpme Jul 18, 2023

vstpme Jul 18, 2023

vstpme Jul 18, 2023

vstpme Jul 18, 2023

vstpme Jul 18, 2023

vstpme Jul 18, 2023

joeyorlando Jul 18, 2023

joeyorlando left a comment

joeyorlando Jul 18, 2023

		@@ -141,38 +141,6 @@ def handle_exception(self, exc):
		return super().handle_exception(exc)


		class OrderedModelSerializerMixin:

Fix duplicate orders on routes and escalation policies #2568

Fix duplicate orders on routes and escalation policies #2568

Conversation

vstpme commented Jul 18, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyorlando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstpme commented Jul 18, 2023 •

edited

Loading