Skip to content

Commit 9dbd2c0

Browse files
authored
fix(alerts): Fix trigger ordering in workflow-based AlertRule serializer (#112727)
We'd previously assumed order of triggers wasn't significant given that they are labeled with their level. This preserves the old order, and really makes our tests simpler in the process. Fixes ISWF-2430.
1 parent 3ac568e commit 9dbd2c0

4 files changed

Lines changed: 21 additions & 20 deletions

File tree

src/sentry/incidents/endpoints/serializers/workflow_engine_detector.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,7 @@ def get_attrs(
242242
detector_trigger_data_conditions = DataCondition.objects.filter(
243243
condition_group__in=detector_workflow_condition_group_ids,
244244
condition_result__in=[DetectorPriorityLevel.HIGH, DetectorPriorityLevel.MEDIUM],
245-
)
245+
).order_by("-condition_result")
246246
workflow_dcg_ids = DataConditionGroup.objects.filter(
247247
workflowdataconditiongroup__workflow__in=Subquery(
248248
DetectorWorkflow.objects.filter(detector__in=detector_ids).values_list(

tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@
6363
from sentry.testutils.abstract import Abstract
6464
from sentry.testutils.helpers.datetime import freeze_time
6565
from sentry.testutils.helpers.features import with_feature
66+
from sentry.testutils.helpers.serializer_parity import assert_serializer_parity
6667
from sentry.testutils.outbox import outbox_runner
6768
from sentry.testutils.silo import assume_test_silo_mode
6869
from sentry.testutils.skips import requires_snuba
@@ -369,6 +370,21 @@ def test_workflow_engine_serializer_matches_old_serializer(self) -> None:
369370

370371
self.assert_alert_detail_results_match(old_data, new_data)
371372

373+
@with_feature("organizations:incidents")
374+
@freeze_time("2024-12-11 03:21:34")
375+
def test_workflow_engine_trigger_order_matches_legacy(self) -> None:
376+
"""The frontend uses array position to determine critical vs warning.
377+
Verify the workflow engine returns triggers in the same order as legacy."""
378+
self.create_team(organization=self.organization, members=[self.user])
379+
self.login_as(self.user)
380+
381+
old_resp = self.get_success_response(self.organization.slug, self.alert_rule.id)
382+
383+
with self.feature("organizations:workflow-engine-rule-serializers"):
384+
new_resp = self.get_success_response(self.organization.slug, self.alert_rule.id)
385+
386+
assert_serializer_parity(old=old_resp.data, new=new_resp.data)
387+
372388
@with_feature("organizations:incidents")
373389
@freeze_time("2024-12-11 03:21:34")
374390
def test_workflow_engine_serializer_snoozed_alert_rule(self) -> None:

tests/sentry/incidents/endpoints/test_organization_alert_rule_index.py

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -391,17 +391,9 @@ def test_workflow_engine_serializer_matches_old_serializer(self) -> None:
391391
assert len(new_data) == len(old_data)
392392

393393
for old_rule, new_rule in zip(old_data, new_data):
394-
old_sorted = {
395-
**old_rule,
396-
"triggers": sorted(old_rule.get("triggers", []), key=lambda t: t.get("label", "")),
397-
}
398-
new_sorted = {
399-
**new_rule,
400-
"triggers": sorted(new_rule.get("triggers", []), key=lambda t: t.get("label", "")),
401-
}
402394
assert_serializer_parity(
403-
old=old_sorted,
404-
new=new_sorted,
395+
old=old_rule,
396+
new=new_rule,
405397
known_differences={
406398
# resolveThreshold: Old serializer checked AlertRule.resolve_threshold for None,
407399
# but workflow engine always creates a resolve condition during migration.

tests/sentry/incidents/serializers/test_workflow_engine_detector.py

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,19 +28,12 @@ class TestDetectorSerializer(TestWorkflowEngineSerializer):
2828
def setUp(self) -> None:
2929
super().setUp()
3030

31-
@staticmethod
32-
def _sort_triggers(data: dict[str, Any]) -> dict[str, Any]:
33-
"""Sort triggers by id so ordering doesn't affect comparison."""
34-
result = data.copy()
35-
result["triggers"] = sorted(result.get("triggers", []), key=lambda t: t["id"])
36-
return result
37-
3831
def test_simple(self) -> None:
3932
self.add_warning_trigger()
4033
serialized_detector = serialize(
4134
self.detector, self.user, WorkflowEngineDetectorSerializer()
4235
)
43-
assert self._sort_triggers(serialized_detector) == self._sort_triggers(self.expected)
36+
assert serialized_detector == self.expected
4437

4538
def test_latest_incident(self) -> None:
4639
self.add_warning_trigger()
@@ -188,7 +181,7 @@ def test_sentry_app(self, mock_sentry_app_components_preparer: Any) -> None:
188181
self.user,
189182
WorkflowEngineDetectorSerializer(prepare_component_fields=True),
190183
)
191-
assert self._sort_triggers(serialized_detector) == self._sort_triggers(sentry_app_expected)
184+
assert serialized_detector == sentry_app_expected
192185

193186
def test_snooze_enabled_detector(self) -> None:
194187
serialized = serialize(self.detector, self.user, WorkflowEngineDetectorSerializer())

0 commit comments

Comments
 (0)