diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ac91fbd4b..ddcb7573eb 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/engine/apps/alerts/escalation_snapshot/snapshot_classes/escalation_snapshot.py b/engine/apps/alerts/escalation_snapshot/snapshot_classes/escalation_snapshot.py index aac6a5ecc7..71bb385488 100644 --- a/engine/apps/alerts/escalation_snapshot/snapshot_classes/escalation_snapshot.py +++ b/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 diff --git a/engine/apps/alerts/migrations/0024_auto_20230718_0953.py b/engine/apps/alerts/migrations/0024_auto_20230718_0953.py new file mode 100644 index 0000000000..dfba4a1ebe --- /dev/null +++ b/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'), + ), + ] diff --git a/engine/apps/alerts/migrations/0025_auto_20230718_1042.py b/engine/apps/alerts/migrations/0025_auto_20230718_1042.py new file mode 100644 index 0000000000..83d51dfeea --- /dev/null +++ b/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'), + ), + ] diff --git a/engine/apps/alerts/models/channel_filter.py b/engine/apps/alerts/models/channel_filter.py index 915aeafff1..59a2feca9b 100644 --- a/engine/apps/alerts/models/channel_filter.py +++ b/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'}" diff --git a/engine/apps/alerts/models/escalation_policy.py b/engine/apps/alerts/models/escalation_policy.py index 9c9b13386d..1ec36727e9 100644 --- a/engine/apps/alerts/models/escalation_policy.py +++ b/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}" diff --git a/engine/apps/alerts/tests/test_escalation_snapshot.py b/engine/apps/alerts/tests/test_escalation_snapshot.py index b3ddb82805..66003eebca 100644 --- a/engine/apps/alerts/tests/test_escalation_snapshot.py +++ b/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] diff --git a/engine/apps/api/serializers/channel_filter.py b/engine/apps/api/serializers/channel_filter.py index d6053192e3..ea534cac6e 100644 --- a/engine/apps/api/serializers/channel_filter.py +++ b/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) diff --git a/engine/apps/api/serializers/escalation_policy.py b/engine/apps/api/serializers/escalation_policy.py index 58f822ae5b..ee8fdf4def 100644 --- a/engine/apps/api/serializers/escalation_policy.py +++ b/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) diff --git a/engine/apps/api/serializers/user_notification_policy.py b/engine/apps/api/serializers/user_notification_policy.py index c9acb7f09a..e936d0daba 100644 --- a/engine/apps/api/serializers/user_notification_policy.py +++ b/engine/apps/api/serializers/user_notification_policy.py @@ -33,7 +33,11 @@ class UserNotificationPolicyBaseSerializer(EagerLoadingMixin, serializers.ModelS class Meta: model = UserNotificationPolicy - fields = ["id", "step", "order", "notify_by", "wait_delay", "important", "user"] + 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"] read_only_fields = ["order"] def to_internal_value(self, data): @@ -100,7 +104,7 @@ class UserNotificationPolicyUpdateSerializer(UserNotificationPolicyBaseSerialize ) class Meta(UserNotificationPolicyBaseSerializer.Meta): - read_only_fields = ["order", "user", "important"] + read_only_fields = UserNotificationPolicyBaseSerializer.Meta.read_only_fields + ["user", "important"] def update(self, instance, validated_data): self_or_admin = instance.user.self_or_admin( diff --git a/engine/apps/api/tests/test_channel_filter.py b/engine/apps/api/tests/test_channel_filter.py index 484090c894..f979a4267f 100644 --- a/engine/apps/api/tests/test_channel_filter.py +++ b/engine/apps/api/tests/test_channel_filter.py @@ -6,6 +6,7 @@ from rest_framework.response import Response from rest_framework.test import APIClient +from apps.alerts.models import ChannelFilter from apps.api.permissions import LegacyAccessControlRole @@ -220,7 +221,7 @@ def test_channel_filter_move_to_position_permissions( @pytest.mark.django_db -def test_channel_filter_create_with_order( +def test_channel_filter_create_order( make_organization_and_user_with_plugin_token, make_alert_receive_channel, make_escalation_chain, @@ -230,7 +231,6 @@ def test_channel_filter_create_with_order( organization, user, token = make_organization_and_user_with_plugin_token() alert_receive_channel = make_alert_receive_channel(organization) make_escalation_chain(organization) - # create default channel filter make_channel_filter(alert_receive_channel, is_default=True) channel_filter = make_channel_filter(alert_receive_channel, filtering_term="a", is_default=False) client = APIClient() @@ -239,44 +239,16 @@ def test_channel_filter_create_with_order( data_for_creation = { "alert_receive_channel": alert_receive_channel.public_primary_key, "filtering_term": "b", - "order": 0, } response = client.post(url, data=data_for_creation, format="json", **make_user_auth_headers(user, token)) channel_filter.refresh_from_db() assert response.status_code == status.HTTP_201_CREATED - assert response.json()["order"] == 0 - assert channel_filter.order == 1 - - -@pytest.mark.django_db -def test_channel_filter_create_without_order( - make_organization_and_user_with_plugin_token, - make_alert_receive_channel, - make_escalation_chain, - make_channel_filter, - make_user_auth_headers, -): - organization, user, token = make_organization_and_user_with_plugin_token() - alert_receive_channel = make_alert_receive_channel(organization) - make_escalation_chain(organization) - make_channel_filter(alert_receive_channel, is_default=True) - channel_filter = make_channel_filter(alert_receive_channel, filtering_term="a", is_default=False) - client = APIClient() - - url = reverse("api-internal:channel_filter-list") - data_for_creation = { - "alert_receive_channel": alert_receive_channel.public_primary_key, - "filtering_term": "b", - } - - response = client.post(url, data=data_for_creation, format="json", **make_user_auth_headers(user, token)) - channel_filter.refresh_from_db() - assert response.status_code == status.HTTP_201_CREATED - assert response.json()["order"] == 1 - assert channel_filter.order == 0 + # check that orders are correct + assert ChannelFilter.objects.get(public_primary_key=response.json()["id"]).order == 0 + assert channel_filter.order == 1 @pytest.mark.django_db @@ -297,7 +269,7 @@ def test_move_to_position( url = reverse( "api-internal:channel_filter-move-to-position", kwargs={"pk": first_channel_filter.public_primary_key} ) - url += f"?position=2" + url += f"?position=1" response = client.put(url, **make_user_auth_headers(user, token)) assert response.status_code == status.HTTP_200_OK @@ -308,7 +280,7 @@ def test_move_to_position( @pytest.mark.django_db -def test_move_to_position_cant_move_default( +def test_move_to_position_invalid_index( make_organization_and_user_with_plugin_token, make_alert_receive_channel, make_channel_filter, @@ -317,21 +289,22 @@ def test_move_to_position_cant_move_default( organization, user, token = make_organization_and_user_with_plugin_token() alert_receive_channel = make_alert_receive_channel(organization) # create default channel filter - default_channel_filter = make_channel_filter(alert_receive_channel, is_default=True, order=0) - make_channel_filter(alert_receive_channel, filtering_term="b", is_default=False, order=1) + make_channel_filter(alert_receive_channel, is_default=True, order=0) + first_channel_filter = make_channel_filter(alert_receive_channel, filtering_term="a", is_default=False, order=1) + make_channel_filter(alert_receive_channel, filtering_term="b", is_default=False, order=2) client = APIClient() url = reverse( - "api-internal:channel_filter-move-to-position", kwargs={"pk": default_channel_filter.public_primary_key} + "api-internal:channel_filter-move-to-position", kwargs={"pk": first_channel_filter.public_primary_key} ) - url += f"?position=1" + url += f"?position=2" response = client.put(url, **make_user_auth_headers(user, token)) assert response.status_code == status.HTTP_400_BAD_REQUEST @pytest.mark.django_db -def test_channel_filter_update_with_order( +def test_move_to_position_cant_move_default( make_organization_and_user_with_plugin_token, make_alert_receive_channel, make_channel_filter, @@ -340,33 +313,21 @@ def test_channel_filter_update_with_order( organization, user, token = make_organization_and_user_with_plugin_token() alert_receive_channel = make_alert_receive_channel(organization) # create default channel filter - make_channel_filter(alert_receive_channel, is_default=True) - first_channel_filter = make_channel_filter(alert_receive_channel, filtering_term="a", is_default=False) - second_channel_filter = make_channel_filter(alert_receive_channel, filtering_term="b", is_default=False) + default_channel_filter = make_channel_filter(alert_receive_channel, is_default=True, order=0) + make_channel_filter(alert_receive_channel, filtering_term="b", is_default=False, order=1) client = APIClient() + url = reverse( + "api-internal:channel_filter-move-to-position", kwargs={"pk": default_channel_filter.public_primary_key} + ) + url += f"?position=1" + response = client.put(url, **make_user_auth_headers(user, token)) - url = reverse("api-internal:channel_filter-detail", kwargs={"pk": first_channel_filter.public_primary_key}) - data_for_update = { - "id": first_channel_filter.public_primary_key, - "alert_receive_channel": alert_receive_channel.public_primary_key, - "order": 1, - "filtering_term": first_channel_filter.filtering_term, - } - - response = client.put(url, data=data_for_update, format="json", **make_user_auth_headers(user, token)) - - first_channel_filter.refresh_from_db() - second_channel_filter.refresh_from_db() - - assert response.status_code == status.HTTP_200_OK - assert response.json()["order"] == 1 - assert first_channel_filter.order == 1 - assert second_channel_filter.order == 0 + assert response.status_code == status.HTTP_400_BAD_REQUEST @pytest.mark.django_db -def test_channel_filter_update_without_order( +def test_channel_filter_update( make_organization_and_user_with_plugin_token, make_alert_receive_channel, make_channel_filter, @@ -394,7 +355,6 @@ def test_channel_filter_update_without_order( second_channel_filter.refresh_from_db() assert response.status_code == status.HTTP_200_OK - assert response.json()["order"] == 0 assert first_channel_filter.order == 0 assert second_channel_filter.order == 1 diff --git a/engine/apps/api/tests/test_escalation_policy.py b/engine/apps/api/tests/test_escalation_policy.py index 4b8613f82c..07ec8d396a 100644 --- a/engine/apps/api/tests/test_escalation_policy.py +++ b/engine/apps/api/tests/test_escalation_policy.py @@ -53,7 +53,7 @@ def test_create_escalation_policy(escalation_policy_internal_api_setup, make_use response = client.post(url, data, format="json", **make_user_auth_headers(user, token)) assert response.status_code == status.HTTP_201_CREATED - assert response.data["order"] == max_order + 1 + assert EscalationPolicy.objects.get(public_primary_key=response.data["id"]).order == max_order + 1 @pytest.mark.django_db @@ -77,9 +77,9 @@ def test_create_escalation_policy_webhook( response = client.post(url, data, format="json", **make_user_auth_headers(user, token)) assert response.status_code == status.HTTP_201_CREATED - assert response.data["order"] == max_order + 1 assert response.data["custom_webhook"] == webhook.public_primary_key escalation_policy = EscalationPolicy.objects.get(public_primary_key=response.data["id"]) + assert escalation_policy.order == max_order + 1 assert escalation_policy.custom_webhook == webhook @@ -107,7 +107,7 @@ def test_move_to_position(escalation_policy_internal_api_setup, make_user_auth_h token, _, escalation_policy, user, _ = escalation_policy_internal_api_setup client = APIClient() - position_to_move = 1 + position_to_move = 0 url = reverse( "api-internal:escalation_policy-move-to-position", kwargs={"pk": escalation_policy.public_primary_key} ) @@ -119,6 +119,22 @@ def test_move_to_position(escalation_policy_internal_api_setup, make_user_auth_h assert escalation_policy.order == position_to_move +@pytest.mark.django_db +def test_move_to_position_invalid_index(escalation_policy_internal_api_setup, make_user_auth_headers): + token, _, escalation_policy, user, _ = escalation_policy_internal_api_setup + client = APIClient() + + position_to_move = 1 + url = reverse( + "api-internal:escalation_policy-move-to-position", kwargs={"pk": escalation_policy.public_primary_key} + ) + response = client.put( + f"{url}?position={position_to_move}", content_type="application/json", **make_user_auth_headers(user, token) + ) + escalation_policy.refresh_from_db() + assert response.status_code == status.HTTP_400_BAD_REQUEST + + @pytest.mark.django_db @pytest.mark.parametrize( "role,expected_status", @@ -736,7 +752,6 @@ def test_escalation_policy_switch_importance( data_for_update = { "id": escalation_policy.public_primary_key, "step": escalation_policy.step, - "order": escalation_policy.order, "escalation_chain": escalation_chain.public_primary_key, "notify_to_users_queue": [], "from_time": None, @@ -792,7 +807,6 @@ def test_escalation_policy_filter_by_user( expected_payload = [ { "id": escalation_policy_with_one_user.public_primary_key, - "order": 0, "step": 13, "wait_delay": None, "escalation_chain": escalation_chain.public_primary_key, @@ -810,7 +824,6 @@ def test_escalation_policy_filter_by_user( }, { "id": escalation_policy_with_two_users.public_primary_key, - "order": 1, "step": 13, "wait_delay": None, "escalation_chain": escalation_chain.public_primary_key, @@ -873,7 +886,6 @@ def test_escalation_policy_filter_by_slack_channel( expected_payload = [ { "id": escalation_policy_from_alert_receive_channel_with_slack_channel.public_primary_key, - "order": 0, "step": 0, "wait_delay": None, "escalation_chain": escalation_chain.public_primary_key, diff --git a/engine/apps/api/views/channel_filter.py b/engine/apps/api/views/channel_filter.py index 18d72066b2..25eeda60ec 100644 --- a/engine/apps/api/views/channel_filter.py +++ b/engine/apps/api/views/channel_filter.py @@ -3,7 +3,6 @@ from rest_framework.decorators import action from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response -from rest_framework.viewsets import ModelViewSet from apps.alerts.models import ChannelFilter from apps.api.permissions import RBACPermission @@ -21,12 +20,16 @@ TeamFilteringMixin, UpdateSerializerMixin, ) -from common.api_helpers.serializers import get_move_to_position_param from common.insight_log import EntityEvent, write_resource_insight_log +from common.ordered_model.viewset import OrderedModelViewSet class ChannelFilterView( - TeamFilteringMixin, PublicPrimaryKeyMixin, CreateSerializerMixin, UpdateSerializerMixin, ModelViewSet + TeamFilteringMixin, + PublicPrimaryKeyMixin, + CreateSerializerMixin, + UpdateSerializerMixin, + OrderedModelViewSet, ): authentication_classes = (PluginAuthentication,) permission_classes = (IsAuthenticated, RBACPermission) @@ -109,24 +112,10 @@ def perform_update(self, serializer): @action(detail=True, methods=["put"]) def move_to_position(self, request, pk): instance = self.get_object() - position = get_move_to_position_param(request) - if instance.is_default: raise BadRequest(detail="Unable to change position for default filter") - prev_state = instance.insight_logs_serialized - instance.to(position) - new_state = instance.insight_logs_serialized - - write_resource_insight_log( - instance=instance, - author=self.request.user, - event=EntityEvent.UPDATED, - prev_state=prev_state, - new_state=new_state, - ) - - return Response(status=status.HTTP_200_OK) + return super().move_to_position(request, pk) @action(detail=True, methods=["post"]) def convert_from_regex_to_jinja2(self, request, pk): diff --git a/engine/apps/api/views/escalation_policy.py b/engine/apps/api/views/escalation_policy.py index 611285c4c3..fd9f5d6d16 100644 --- a/engine/apps/api/views/escalation_policy.py +++ b/engine/apps/api/views/escalation_policy.py @@ -1,10 +1,8 @@ from django.conf import settings from django.db.models import Q -from rest_framework import status from rest_framework.decorators import action from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response -from rest_framework.viewsets import ModelViewSet from apps.alerts.models import EscalationPolicy from apps.api.permissions import RBACPermission @@ -20,12 +18,16 @@ TeamFilteringMixin, UpdateSerializerMixin, ) -from common.api_helpers.serializers import get_move_to_position_param from common.insight_log import EntityEvent, write_resource_insight_log +from common.ordered_model.viewset import OrderedModelViewSet class EscalationPolicyView( - TeamFilteringMixin, PublicPrimaryKeyMixin, CreateSerializerMixin, UpdateSerializerMixin, ModelViewSet + TeamFilteringMixin, + PublicPrimaryKeyMixin, + CreateSerializerMixin, + UpdateSerializerMixin, + OrderedModelViewSet, ): authentication_classes = (PluginAuthentication,) permission_classes = (IsAuthenticated, RBACPermission) @@ -108,25 +110,6 @@ def perform_destroy(self, instance): ) instance.delete() - @action(detail=True, methods=["put"]) - def move_to_position(self, request, pk): - instance = self.get_object() - position = get_move_to_position_param(request) - - prev_state = instance.insight_logs_serialized - instance.to(position) - new_state = instance.insight_logs_serialized - - write_resource_insight_log( - instance=instance, - author=self.request.user, - event=EntityEvent.UPDATED, - prev_state=prev_state, - new_state=new_state, - ) - - return Response(status=status.HTTP_200_OK) - @action(detail=False, methods=["get"]) def escalation_options(self, request): choices = [] diff --git a/engine/apps/api/views/user_notification_policy.py b/engine/apps/api/views/user_notification_policy.py index a7efb87fcc..26805c6549 100644 --- a/engine/apps/api/views/user_notification_policy.py +++ b/engine/apps/api/views/user_notification_policy.py @@ -1,10 +1,8 @@ from django.conf import settings from django.http import Http404 -from rest_framework import status from rest_framework.decorators import action from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response -from rest_framework.viewsets import ModelViewSet from apps.api.permissions import IsOwnerOrHasRBACPermissions, RBACPermission from apps.api.serializers.user_notification_policy import ( @@ -19,12 +17,12 @@ from apps.user_management.models import User from common.api_helpers.exceptions import BadRequest from common.api_helpers.mixins import UpdateSerializerMixin -from common.api_helpers.serializers import get_move_to_position_param from common.exceptions import UserNotificationPolicyCouldNotBeDeleted from common.insight_log import EntityEvent, write_resource_insight_log +from common.ordered_model.viewset import OrderedModelViewSet -class UserNotificationPolicyView(UpdateSerializerMixin, ModelViewSet): +class UserNotificationPolicyView(UpdateSerializerMixin, OrderedModelViewSet): authentication_classes = ( MobileAppAuthTokenAuthentication, PluginAuthentication, @@ -78,9 +76,7 @@ def get_queryset(self): queryset = self.model.objects.filter(user=target_user, important=important) - queryset = self.serializer_class.setup_eager_loading(queryset) - - return queryset.order_by("order") + return self.serializer_class.setup_eager_loading(queryset) def get_object(self): # we need overriden get object, because original one call get_queryset first and raise 404 trying to access @@ -138,18 +134,6 @@ def perform_destroy(self, instance): new_state=new_state, ) - @action(detail=True, methods=["put"]) - def move_to_position(self, request, pk): - instance = self.get_object() - position = get_move_to_position_param(request) - - try: - instance.to_index(position) - except IndexError: - raise BadRequest(detail="Invalid position") - - return Response(status=status.HTTP_200_OK) - @action(detail=False, methods=["get"]) def delay_options(self, request): choices = [] diff --git a/engine/apps/base/models/user_notification_policy.py b/engine/apps/base/models/user_notification_policy.py index 3a39b96fd6..a772a74f0d 100644 --- a/engine/apps/base/models/user_notification_policy.py +++ b/engine/apps/base/models/user_notification_policy.py @@ -9,9 +9,9 @@ from django.db.models import Q from apps.base.messaging import get_messaging_backends -from apps.base.models.ordered_model import OrderedModel 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 diff --git a/engine/apps/public_api/serializers/escalation_policies.py b/engine/apps/public_api/serializers/escalation_policies.py index ca7e9f1447..0db6797c31 100644 --- a/engine/apps/public_api/serializers/escalation_policies.py +++ b/engine/apps/public_api/serializers/escalation_policies.py @@ -14,7 +14,8 @@ UsersFilteredByOrganizationField, ) from common.api_helpers.exceptions import BadRequest -from common.api_helpers.mixins import EagerLoadingMixin, OrderedModelSerializerMixin +from common.api_helpers.mixins import EagerLoadingMixin +from common.ordered_model.serializer import OrderedModelSerializer class EscalationPolicyTypeField(fields.CharField): @@ -35,12 +36,11 @@ def to_internal_value(self, data): return step_type -class EscalationPolicySerializer(EagerLoadingMixin, OrderedModelSerializerMixin, serializers.ModelSerializer): +class EscalationPolicySerializer(EagerLoadingMixin, OrderedModelSerializer): id = serializers.CharField(read_only=True, source="public_primary_key") escalation_chain_id = OrganizationFilteredPrimaryKeyRelatedField( queryset=EscalationChain.objects, source="escalation_chain" ) - position = serializers.IntegerField(required=False, source="order") type = EscalationPolicyTypeField(source="step", allow_null=True) duration = serializers.ChoiceField(required=False, source="wait_delay", choices=EscalationPolicy.DURATION_CHOICES) persons_to_notify = UsersFilteredByOrganizationField( @@ -67,17 +67,15 @@ class EscalationPolicySerializer(EagerLoadingMixin, OrderedModelSerializerMixin, required=False, source="custom_button_trigger", ) - manual_order = serializers.BooleanField(default=False, write_only=True) important = serializers.BooleanField(required=False) notify_if_time_from = CustomTimeField(required=False, source="from_time") notify_if_time_to = CustomTimeField(required=False, source="to_time") class Meta: model = EscalationPolicy - fields = [ + fields = OrderedModelSerializer.Meta.fields + [ "id", "escalation_chain_id", - "position", "type", "duration", "important", @@ -86,7 +84,6 @@ class Meta: "notify_on_call_from_schedule", "group_to_notify", "action_to_trigger", - "manual_order", "notify_if_time_from", "notify_if_time_to", "num_alerts_in_window", @@ -126,21 +123,7 @@ def validate_notify_on_call_from_schedule(self, schedule): def create(self, validated_data): validated_data = self._correct_validated_data(validated_data) - manual_order = validated_data.pop("manual_order") - if not manual_order: - order = validated_data.pop("order", None) - escalation_chain_id = validated_data.get("escalation_chain") - # validate 'order' value before creation - self._validate_order(order, {"escalation_chain_id": escalation_chain_id}) - - instance = super().create(validated_data) - self._change_position(order, instance) - else: - # validate will raise if there is a duplicated order - self._validate_manual_order(None, validated_data) - instance = super().create(validated_data) - - return instance + return super().create(validated_data) def to_representation(self, instance): step = instance.step @@ -211,18 +194,6 @@ def _get_field_to_represent(self, step, result): result.pop(field, None) return result - def _validate_manual_order(self, instance, validated_data): - order = validated_data.get("order") - if order is None: - return - - policies_with_order = self.escalation_chain.escalation_policies.filter(order=order) - if instance and instance.id: - policies_with_order = policies_with_order.exclude(id=instance.id) - - if policies_with_order.exists(): - raise BadRequest(detail="Steps cannot have duplicated positions") - def _correct_validated_data(self, validated_data): validated_data_fields_to_remove = [ "notify_to_users_queue", @@ -310,14 +281,4 @@ def update(self, instance, validated_data): instance.num_alerts_in_window = None instance.num_minutes_in_window = None - manual_order = validated_data.pop("manual_order") - - if not manual_order: - order = validated_data.pop("order", None) - self._validate_order(order, {"escalation_chain_id": instance.escalation_chain_id}) - self._change_position(order, instance) - else: - # validate will raise if there is a duplicated order - self._validate_manual_order(instance, validated_data) - return super().update(instance, validated_data) diff --git a/engine/apps/public_api/serializers/personal_notification_rules.py b/engine/apps/public_api/serializers/personal_notification_rules.py index 3d0e7df318..a284202015 100644 --- a/engine/apps/public_api/serializers/personal_notification_rules.py +++ b/engine/apps/public_api/serializers/personal_notification_rules.py @@ -8,20 +8,18 @@ from common.api_helpers.custom_fields import UserIdField from common.api_helpers.exceptions import BadRequest from common.api_helpers.mixins import EagerLoadingMixin +from common.ordered_model.serializer import OrderedModelSerializer -class PersonalNotificationRuleSerializer(EagerLoadingMixin, serializers.ModelSerializer): +class PersonalNotificationRuleSerializer(EagerLoadingMixin, OrderedModelSerializer): id = serializers.CharField(read_only=True, source="public_primary_key") user_id = UserIdField(required=True, source="user") - position = serializers.IntegerField(required=False, source="order") type = serializers.CharField( required=False, ) - duration = serializers.ChoiceField( required=False, source="wait_delay", choices=UserNotificationPolicy.DURATION_CHOICES ) - manual_order = serializers.BooleanField(default=False, write_only=True) SELECT_RELATED = ["user"] @@ -31,7 +29,7 @@ class PersonalNotificationRuleSerializer(EagerLoadingMixin, serializers.ModelSer class Meta: model = UserNotificationPolicy - fields = ["id", "user_id", "position", "type", "duration", "manual_order", "important"] + fields = OrderedModelSerializer.Meta.fields + ["id", "user_id", "type", "duration", "important"] def create(self, validated_data): if "type" not in validated_data: @@ -44,17 +42,7 @@ def create(self, validated_data): if "wait_delay" in validated_data and validated_data["step"] != UserNotificationPolicy.Step.WAIT: raise exceptions.ValidationError({"duration": "Duration can't be set"}) - # Remove "manual_order" and "order" fields from validated_data, so they are not passed to create method. - # Policies are always created at the end of the list, and then moved to the desired position by _adjust_order. - manual_order = validated_data.pop("manual_order") - order = validated_data.pop("order", None) - - instance = UserNotificationPolicy.objects.create(**validated_data) - - if order is not None: - self._adjust_order(instance, manual_order, order, created=True) - - return instance + return super().create(validated_data) def to_internal_value(self, data): if "duration" in data: @@ -119,33 +107,6 @@ def _type_to_step_and_notification_channel(cls, rule_type): raise exceptions.ValidationError({"type": "Invalid type"}) - @staticmethod - def _adjust_order(instance, manual_order, order, created): - # Passing order=-1 means that the policy should be moved to the end of the list. - if order == -1: - if created: - # The policy was just created, so it is already at the end of the list. - return - - order = instance.max_order() - # max_order() can't be None here because at least one instance exists – the one we are moving. - assert order is not None - - # Negative order is not allowed. - if order < 0: - raise BadRequest(detail="Invalid value for position field") - - # manual_order=True is intended for use by Terraform provider only, and is not a documented feature. - # Orders are swapped instead of moved when using Terraform, because Terraform may issue concurrent requests - # to create / update / delete multiple policies. "Move to" operation is not deterministic in this case, and - # final order of policies may be different depending on the order in which requests are processed. On the other - # hand, the result of concurrent "swap" operations is deterministic and does not depend on the order in - # which requests are processed. - if manual_order: - instance.swap(order) - else: - instance.to(order) - class PersonalNotificationRuleUpdateSerializer(PersonalNotificationRuleSerializer): user_id = UserIdField(read_only=True, source="user") @@ -165,10 +126,4 @@ def update(self, instance, validated_data): if "wait_delay" in validated_data and instance.step != UserNotificationPolicy.Step.WAIT: raise exceptions.ValidationError({"duration": "Duration can't be set"}) - # Remove "manual_order" and "order" fields from validated_data, so they are not passed to update method. - manual_order = validated_data.pop("manual_order") - order = validated_data.pop("order", None) - if order is not None: - self._adjust_order(instance, manual_order, order, created=False) - return super().update(instance, validated_data) diff --git a/engine/apps/public_api/serializers/routes.py b/engine/apps/public_api/serializers/routes.py index 1125b4479e..de15f448dc 100644 --- a/engine/apps/public_api/serializers/routes.py +++ b/engine/apps/public_api/serializers/routes.py @@ -6,12 +6,12 @@ from apps.base.messaging import get_messaging_backend_from_id, get_messaging_backends from common.api_helpers.custom_fields import OrganizationFilteredPrimaryKeyRelatedField from common.api_helpers.exceptions import BadRequest -from common.api_helpers.mixins import OrderedModelSerializerMixin from common.jinja_templater.apply_jinja_template import JinjaTemplateError +from common.ordered_model.serializer import OrderedModelSerializer from common.utils import is_regex_valid -class BaseChannelFilterSerializer(OrderedModelSerializerMixin, serializers.ModelSerializer): +class BaseChannelFilterSerializer(OrderedModelSerializer): """Base Channel Filter serializer with validation methods""" def __init__(self, *args, **kwargs): @@ -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") integration_id = OrganizationFilteredPrimaryKeyRelatedField( queryset=AlertReceiveChannel.objects, source="alert_receive_channel" ) @@ -159,39 +158,24 @@ class ChannelFilterSerializer(BaseChannelFilterSerializer): ) is_the_last_route = serializers.BooleanField(read_only=True, source="is_default") - manual_order = serializers.BooleanField(default=False, write_only=True) class Meta: model = ChannelFilter - fields = [ + fields = OrderedModelSerializer.Meta.fields + [ "id", "integration_id", "escalation_chain_id", "routing_type", "routing_regex", - "position", "is_the_last_route", "slack", "telegram", - "manual_order", ] read_only_fields = ["is_the_last_route"] def create(self, validated_data): validated_data = self._correct_validated_data(validated_data) - manual_order = validated_data.pop("manual_order") - if manual_order: - self._validate_manual_order(validated_data.get("order", None)) - instance = super().create(validated_data) - else: - order = validated_data.pop("order", None) - alert_receive_channel_id = validated_data.get("alert_receive_channel") - # validate 'order' value before creation - 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) - - return instance + return super().create(validated_data) def validate(self, data): filtering_term = data.get("routing_regex") @@ -224,18 +208,6 @@ class Meta(ChannelFilterSerializer.Meta): def update(self, instance, validated_data): validated_data = self._correct_validated_data(validated_data) - - manual_order = validated_data.pop("manual_order") - if manual_order: - self._validate_manual_order(validated_data.get("order", None)) - else: - order = validated_data.pop("order", None) - self._validate_order( - order, - {"alert_receive_channel_id": instance.alert_receive_channel_id, "is_default": instance.is_default}, - ) - self._change_position(order, instance) - if validated_data.get("notification_backends"): validated_data["notification_backends"] = self._update_notification_backends( validated_data["notification_backends"] diff --git a/engine/apps/public_api/tests/test_escalation_policies.py b/engine/apps/public_api/tests/test_escalation_policies.py index 728fc3837f..01d05e30ab 100644 --- a/engine/apps/public_api/tests/test_escalation_policies.py +++ b/engine/apps/public_api/tests/test_escalation_policies.py @@ -154,7 +154,7 @@ def test_create_escalation_policy_manual_order_duplicated_position( escalation_policies_setup, ): organization, user, token = make_organization_and_user_with_token() - escalation_chain, _, _ = escalation_policies_setup(organization, user) + escalation_chain, escalation_policies, _ = escalation_policies_setup(organization, user) data_for_create = { "escalation_chain_id": escalation_chain.public_primary_key, @@ -168,7 +168,43 @@ def test_create_escalation_policy_manual_order_duplicated_position( url = reverse("api-public:escalation_policies-list") response = client.post(url, data=data_for_create, format="json", HTTP_AUTHORIZATION=token) - assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.status_code == status.HTTP_201_CREATED + assert response.data["position"] == 0 + + for escalation_policy in escalation_policies: + escalation_policy.refresh_from_db() + + orders = [escalation_policy.order for escalation_policy in escalation_policies] + assert orders == [3, 1, 2] # Check orders are swapped when manual_order is True + + +@pytest.mark.django_db +def test_create_escalation_policy_no_manual_order_duplicated_position( + make_organization_and_user_with_token, + escalation_policies_setup, +): + organization, user, token = make_organization_and_user_with_token() + escalation_chain, escalation_policies, _ = escalation_policies_setup(organization, user) + + data_for_create = { + "escalation_chain_id": escalation_chain.public_primary_key, + "type": "notify_person_next_each_time", + "position": 0, + "persons_to_notify_next_each_time": [user.public_primary_key], + } + + client = APIClient() + url = reverse("api-public:escalation_policies-list") + response = client.post(url, data=data_for_create, format="json", HTTP_AUTHORIZATION=token) + + assert response.status_code == status.HTTP_201_CREATED + assert response.data["position"] == 0 + + for escalation_policy in escalation_policies: + escalation_policy.refresh_from_db() + + orders = [escalation_policy.order for escalation_policy in escalation_policies] + assert orders == [1, 2, 3] # Check policies are moved down manual_order is False @pytest.mark.django_db @@ -265,4 +301,10 @@ def test_update_escalation_policy_manual_order_duplicated_position( data_to_change = {"position": 0, "manual_order": True} response = client.put(url, data=data_to_change, format="json", HTTP_AUTHORIZATION=token) - assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.status_code == status.HTTP_200_OK + + for escalation_policy in escalation_policies: + escalation_policy.refresh_from_db() + + orders = [escalation_policy.order for escalation_policy in escalation_policies] + assert orders == [1, 0, 2] # Check orders are swapped when manual_order is True diff --git a/engine/apps/public_api/tests/test_routes.py b/engine/apps/public_api/tests/test_routes.py index c77ee531ed..6aa8cdedb8 100644 --- a/engine/apps/public_api/tests/test_routes.py +++ b/engine/apps/public_api/tests/test_routes.py @@ -444,7 +444,7 @@ def test_update_route_with_manual_ordering( url = reverse("api-public:routes-detail", kwargs={"pk": channel_filter.public_primary_key}) - # Test negative value. Note, that for "manual_order"=False, -1 is valud option (It will move route to the bottom) + # Test negative value. Note, that for "manual_order"=False, -1 is valid option (It will move route to the bottom) data_to_update = {"position": -1, "manual_order": True} response = client.put(url, format="json", HTTP_AUTHORIZATION=token, data=data_to_update) diff --git a/engine/common/api_helpers/mixins.py b/engine/common/api_helpers/mixins.py index 0e41111dca..13620ce790 100644 --- a/engine/common/api_helpers/mixins.py +++ b/engine/common/api_helpers/mixins.py @@ -141,38 +141,6 @@ def handle_exception(self, exc): return super().handle_exception(exc) -class OrderedModelSerializerMixin: - def _change_position(self, order, instance): - if order is not None: - if order >= 0: - instance.to(order) - elif order == -1: - instance.bottom() - else: - raise BadRequest(detail="Invalid value for position field") - - def _validate_order(self, order, filter_kwargs): - if order is not None and (self.instance is None or self.instance.order != order): - last_instance = self.Meta.model.objects.filter(**filter_kwargs).order_by("order").last() - max_order = last_instance.order if last_instance else -1 - if self.instance is None: - max_order += 1 - if order > max_order: - raise BadRequest(detail="Invalid value for position field") - - def _validate_manual_order(self, order): - """ - For manual ordering validate just that order is valid PositiveIntegrer. - User of manual ordering is responsible for correct ordering. - However, manual ordering not intended for use somewhere, except terraform provider. - """ - - # https://docs.djangoproject.com/en/4.1/ref/models/fields/#positiveintegerfield - MAX_POSITIVE_INTEGER = 2147483647 - if order is not None and order < 0 or order > MAX_POSITIVE_INTEGER: - raise BadRequest(detail="Invalid value for position field") - - class PublicPrimaryKeyMixin: def get_object(self): pk = self.kwargs["pk"] diff --git a/engine/common/ordered_model/__init__.py b/engine/common/ordered_model/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/engine/apps/base/models/ordered_model.py b/engine/common/ordered_model/ordered_model.py similarity index 100% rename from engine/apps/base/models/ordered_model.py rename to engine/common/ordered_model/ordered_model.py diff --git a/engine/common/ordered_model/serializer.py b/engine/common/ordered_model/serializer.py new file mode 100644 index 0000000000..3e1022c76d --- /dev/null +++ b/engine/common/ordered_model/serializer.py @@ -0,0 +1,69 @@ +from rest_framework import serializers + +from common.api_helpers.exceptions import BadRequest + + +class OrderedModelSerializer(serializers.ModelSerializer): + """Ordered model serializer to be used in public API.""" + + position = serializers.IntegerField(required=False, source="order") + # manual_order=True is intended for use by Terraform provider only, and is not a documented feature. + manual_order = serializers.BooleanField(default=False, write_only=True) + + class Meta: + fields = ["position", "manual_order"] + + def create(self, validated_data): + # Remove "manual_order" and "order" fields from validated_data, so they are not passed to create method. + manual_order = validated_data.pop("manual_order") + order = validated_data.pop("order", None) + + # Create the instance. + # Instances are always created at the end of the list, and then moved to the desired position by _adjust_order. + instance = super().create(validated_data) + + # Adjust order of the instance if necessary. + if order is not None: + self._adjust_order(instance, manual_order, order, created=True) + + return instance + + def update(self, instance, validated_data): + # Remove "manual_order" and "order" fields from validated_data, so they are not passed to update method. + manual_order = validated_data.pop("manual_order") + order = validated_data.pop("order", None) + + # Adjust order of the instance if necessary. + if order is not None: + self._adjust_order(instance, manual_order, order, created=False) + + # Proceed with the update. + return super().update(instance, validated_data) + + @staticmethod + def _adjust_order(instance, manual_order, order, created): + # Passing order=-1 means that the policy should be moved to the end of the list. + # Works only for public API but not for Terraform provider. + if order == -1 and not manual_order: + if created: + # The policy was just created, so it is already at the end of the list. + return + + order = instance.max_order() + # max_order() can't be None here because at least one instance exists – the one we are moving. + assert order is not None + + # Check the order is in the valid range. + # https://docs.djangoproject.com/en/4.1/ref/models/fields/#positiveintegerfield + if order < 0 or order > 2147483647: + raise BadRequest(detail="Invalid value for position field") + + # Orders are swapped instead of moved when using Terraform, because Terraform may issue concurrent requests + # to create / update / delete multiple policies. "Move to" operation is not deterministic in this case, and + # final order of policies may be different depending on the order in which requests are processed. On the other + # hand, the result of concurrent "swap" operations is deterministic and does not depend on the order in + # which requests are processed. + if manual_order: + instance.swap(order) + else: + instance.to(order) diff --git a/engine/common/ordered_model/viewset.py b/engine/common/ordered_model/viewset.py new file mode 100644 index 0000000000..b00f749cab --- /dev/null +++ b/engine/common/ordered_model/viewset.py @@ -0,0 +1,56 @@ +from rest_framework import serializers, status +from rest_framework.decorators import action +from rest_framework.request import Request +from rest_framework.response import Response +from rest_framework.viewsets import ModelViewSet + +from common.api_helpers.exceptions import BadRequest +from common.insight_log import EntityEvent, write_resource_insight_log + + +class OrderedModelViewSet(ModelViewSet): + """Ordered model viewset to be used in internal API.""" + + @action(detail=True, methods=["put"]) + def move_to_position(self, request: Request, pk: int) -> Response: + instance = self.get_object() + position = self._get_move_to_position_param(request) + + prev_state = self._get_insight_logs_serialized(instance) + try: + instance.to_index(position) + except IndexError: + raise BadRequest(detail="Invalid position") + new_state = self._get_insight_logs_serialized(instance) + + write_resource_insight_log( + instance=instance, + author=self.request.user, + event=EntityEvent.UPDATED, + prev_state=prev_state, + new_state=new_state, + ) + + return Response(status=status.HTTP_200_OK) + + @staticmethod + def _get_insight_logs_serialized(instance): + try: + return instance.insight_logs_serialized + except AttributeError: + return instance.user.insight_logs_serialized # workaround for UserNotificationPolicy + + @staticmethod + def _get_move_to_position_param(request: Request) -> int: + """ + Get "position" parameter from query params + validate it. + Used by actions on ordered models (e.g. move_to_position). + """ + + class MoveToPositionQueryParamsSerializer(serializers.Serializer): + position = serializers.IntegerField() + + serializer = MoveToPositionQueryParamsSerializer(data=request.query_params) + serializer.is_valid(raise_exception=True) + + return serializer.validated_data["position"] diff --git a/engine/apps/base/tests/test_ordered_model.py b/engine/common/tests/test_ordered_model.py similarity index 99% rename from engine/apps/base/tests/test_ordered_model.py rename to engine/common/tests/test_ordered_model.py index b312d23891..d5376f272f 100644 --- a/engine/apps/base/tests/test_ordered_model.py +++ b/engine/common/tests/test_ordered_model.py @@ -4,7 +4,7 @@ import pytest from django.db import models -from apps.base.models.ordered_model import OrderedModel +from common.ordered_model.ordered_model import OrderedModel class TestOrderedModel(OrderedModel): diff --git a/engine/requirements.txt b/engine/requirements.txt index 2d56ea9136..7a456061b4 100644 --- a/engine/requirements.txt +++ b/engine/requirements.txt @@ -4,6 +4,7 @@ slackclient==1.3.0 whitenoise==5.3.0 twilio~=6.37.0 phonenumbers==8.10.0 +# TODO: remove django-ordered-model after migration to custom OrderModel django-ordered-model==3.1.1 celery[amqp,redis]==5.3.1 redis==4.6.0 diff --git a/grafana-plugin/src/containers/ChannelFilterForm/ChannelFilterForm.tsx b/grafana-plugin/src/containers/ChannelFilterForm/ChannelFilterForm.tsx index b4e641f50a..a0403aa2dc 100644 --- a/grafana-plugin/src/containers/ChannelFilterForm/ChannelFilterForm.tsx +++ b/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, alert_receive_channel: alertReceiveChannelId, filtering_term: filteringTerm, filtering_term_type: filteringTermType, diff --git a/grafana-plugin/src/models/channel_filter.ts b/grafana-plugin/src/models/channel_filter.ts index 0fb6754d41..34ac528f3b 100644 --- a/grafana-plugin/src/models/channel_filter.ts +++ b/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; alert_receive_channel: AlertReceiveChannel['id']; slack_channel_id?: SlackChannel['id']; telegram_channel?: TelegramChannel['id']; diff --git a/grafana-plugin/src/models/channel_filter/channel_filter.types.ts b/grafana-plugin/src/models/channel_filter/channel_filter.types.ts index 67835515fb..e842f7a0e0 100644 --- a/grafana-plugin/src/models/channel_filter/channel_filter.types.ts +++ b/grafana-plugin/src/models/channel_filter/channel_filter.types.ts @@ -10,7 +10,6 @@ export enum FilteringTermType { export interface ChannelFilter { id: string; - order: number; alert_receive_channel: AlertReceiveChannel['id']; slack_channel_id?: SlackChannel['id']; slack_channel?: SlackChannel; diff --git a/grafana-plugin/src/models/escalation_policy.ts b/grafana-plugin/src/models/escalation_policy.ts index 01e118dd08..a8f501a389 100644 --- a/grafana-plugin/src/models/escalation_policy.ts +++ b/grafana-plugin/src/models/escalation_policy.ts @@ -10,8 +10,7 @@ import { UserDTO as User } from './user'; export interface EscalationPolicyType { id: string; notify_to_user: User['pk'] | null; - order: number; - // is't option value from api/internal/v1/escalation_policies/escalation_options/ + // it's option value from api/internal/v1/escalation_policies/escalation_options/ step: number; wait_delay: string | null; is_final: boolean; diff --git a/grafana-plugin/src/models/escalation_policy/escalation_policy.types.ts b/grafana-plugin/src/models/escalation_policy/escalation_policy.types.ts index d3faa65bee..5e757d5bb7 100644 --- a/grafana-plugin/src/models/escalation_policy/escalation_policy.types.ts +++ b/grafana-plugin/src/models/escalation_policy/escalation_policy.types.ts @@ -8,8 +8,7 @@ import { UserGroup } from 'models/user_group/user_group.types'; export interface EscalationPolicy { id: string; notify_to_user: User['pk'] | null; - order: number; - // is't option value from api/internal/v1/escalation_policies/escalation_options/ + // it's option value from api/internal/v1/escalation_policies/escalation_options/ step: EscalationPolicyOption['value']; wait_delay: string | null; is_final: boolean; diff --git a/grafana-plugin/src/models/notification_policy.ts b/grafana-plugin/src/models/notification_policy.ts index dce97f216d..ad0e7d71fd 100644 --- a/grafana-plugin/src/models/notification_policy.ts +++ b/grafana-plugin/src/models/notification_policy.ts @@ -2,7 +2,6 @@ import { UserDTO as User } from './user'; export interface NotificationPolicyType { id: string; - order: number; step: number; notify_by: User['pk'] | null; wait_delay: string | null; diff --git a/grafana-plugin/src/pages/integration/Integration.tsx b/grafana-plugin/src/pages/integration/Integration.tsx index 146ae58508..447c11f298 100644 --- a/grafana-plugin/src/pages/integration/Integration.tsx +++ b/grafana-plugin/src/pages/integration/Integration.tsx @@ -406,7 +406,6 @@ class Integration extends React.Component { () => { alertReceiveChannelStore .createChannelFilter({ - order: 0, alert_receive_channel: id, filtering_term: NEW_ROUTE_DEFAULT, filtering_term_type: 1, // non-regex