Skip to content

Commit 0560103

Browse files
ceorourkeceorourke
authored
fix(ACI): Accept fallthrough type in actions (#103694)
If "suggested assignees" are selected when creating an action that uses the fallthrough type the schema was incorrectly expecting it to be camel case instead of snake case which prevented the action from being created. This updates that and adds a test case around that particular action. For now we must support both types until we can run a migration, after which I can go back and remove support for the incorrect camel case type. Fixes https://getsentry.atlassian.net/browse/ACI-504 Related migration #103764
1 parent e1e8ef4 commit 0560103

File tree

8 files changed

+127
-50
lines changed

8 files changed

+127
-50
lines changed

‎src/sentry/notifications/notification_action/action_handler_registry/email_handler.py‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,13 @@ class EmailActionHandler(ActionHandler):
4242
"$schema": "https://json-schema.org/draft/2020-12/schema",
4343
"type": "object",
4444
"properties": {
45-
"fallthroughType": { # TODO: migrate to snake_case
45+
"fallthrough_type": {
46+
"type": "string",
47+
"description": "The fallthrough type for issue owners email notifications",
48+
"enum": [*FallthroughChoiceType],
49+
},
50+
# XXX(CEO): temporarily support this incorrect camel case
51+
"fallthroughType": {
4652
"type": "string",
4753
"description": "The fallthrough type for issue owners email notifications",
4854
"enum": [*FallthroughChoiceType],

‎src/sentry/notifications/notification_action/issue_alert_registry/handlers/email_issue_alert_handler.py‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,13 @@ def get_additional_fields(cls, action: Action, mapping: ActionFieldMapping) -> d
5151
}
5252

5353
if target_type == ActionTarget.ISSUE_OWNERS.value:
54-
blob = EmailDataBlob(**action.data)
55-
final_blob[EmailFieldMappingKeys.FALLTHROUGH_TYPE_KEY.value] = blob.fallthroughType
54+
# XXX(CEO): temporarily handle both fallthroughType and fallthrough_type
55+
action_data = action.data.copy()
56+
if action.data.get("fallthroughType"):
57+
del action_data["fallthroughType"]
58+
action_data["fallthrough_type"] = action.data["fallthroughType"]
59+
60+
blob = EmailDataBlob(**action_data)
61+
final_blob[EmailFieldMappingKeys.FALLTHROUGH_TYPE_KEY.value] = blob.fallthrough_type
5662

5763
return final_blob

‎src/sentry/testutils/helpers/data_blobs.py‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -761,37 +761,37 @@
761761
"targetType": "IssueOwners",
762762
"id": "sentry.mail.actions.NotifyEmailAction",
763763
"targetIdentifier": "None",
764-
"fallthroughType": "ActiveMembers",
764+
"fallthrough_type": "ActiveMembers",
765765
"uuid": "2e8847d7-8fe4-44d2-8a16-e25040329790",
766766
},
767767
# NoOne Fallthrough (targetIdentifier is "")
768768
{
769769
"targetType": "IssueOwners",
770770
"targetIdentifier": "",
771771
"id": "sentry.mail.actions.NotifyEmailAction",
772-
"fallthroughType": "NoOne",
772+
"fallthrough_type": "NoOne",
773773
"uuid": "fb039430-0848-4fc4-89b4-bc7689a9f851",
774774
},
775775
# AllMembers Fallthrough (targetIdentifier is None)
776776
{
777777
"targetType": "IssueOwners",
778778
"id": "sentry.mail.actions.NotifyEmailAction",
779779
"targetIdentifier": None,
780-
"fallthroughType": "AllMembers",
780+
"fallthrough_type": "AllMembers",
781781
"uuid": "41f13756-8f90-4afe-b162-55268c6e3cdb",
782782
},
783783
# NoOne Fallthrough (targetIdentifier is "None")
784784
{
785785
"targetType": "IssueOwners",
786786
"id": "sentry.mail.actions.NotifyEmailAction",
787787
"targetIdentifier": "None",
788-
"fallthroughType": "NoOne",
788+
"fallthrough_type": "NoOne",
789789
"uuid": "99c9b517-0a0f-47f0-b3ff-2a9cd2fd9c49",
790790
},
791791
# ActiveMembers Fallthrough
792792
{
793793
"targetType": "Member",
794-
"fallthroughType": "ActiveMembers",
794+
"fallthrough_type": "ActiveMembers",
795795
"id": "sentry.mail.actions.NotifyEmailAction",
796796
"targetIdentifier": 3234013,
797797
"uuid": "6e83337b-9561-4167-a208-27d6bdf5e613",
@@ -807,7 +807,7 @@
807807
{
808808
"targetType": "Team",
809809
"id": "sentry.mail.actions.NotifyEmailAction",
810-
"fallthroughType": "AllMembers",
810+
"fallthrough_type": "AllMembers",
811811
"uuid": "71b445cf-573b-4e0c-86bc-8dfbad93c480",
812812
"targetIdentifier": 188022,
813813
},

‎src/sentry/workflow_engine/typings/notification_action.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ class EmailFieldMappingKeys(StrEnum):
9898
EmailFieldMappingKeys is an enum that represents the keys of an email field mapping.
9999
"""
100100

101-
FALLTHROUGH_TYPE_KEY = "fallthroughType"
101+
FALLTHROUGH_TYPE_KEY = "fallthrough_type"
102102
TARGET_TYPE_KEY = "targetType"
103103

104104

@@ -577,7 +577,7 @@ def get_sanitized_data(self) -> dict[str, Any]:
577577
):
578578
return dataclasses.asdict(
579579
EmailDataBlob(
580-
fallthroughType=self.action.get(
580+
fallthrough_type=self.action.get(
581581
EmailFieldMappingKeys.FALLTHROUGH_TYPE_KEY.value,
582582
FallthroughChoiceType.ACTIVE_MEMBERS.value,
583583
),
@@ -740,7 +740,7 @@ class EmailDataBlob(DataBlob):
740740
EmailDataBlob represents the data blob for an email notification action.
741741
"""
742742

743-
fallthroughType: str = ""
743+
fallthrough_type: str = ""
744744

745745

746746
issue_alert_action_translator_mapping: dict[str, type[BaseActionTranslator]] = {

‎tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py‎

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
BaseIssueAlertHandler,
2626
TicketingIssueAlertHandler,
2727
)
28+
from sentry.notifications.types import ActionTargetType, FallthroughChoiceType
2829
from sentry.testutils.helpers.data_blobs import (
2930
AZURE_DEVOPS_ACTION_DATA_BLOBS,
3031
EMAIL_ACTION_DATA_BLOBS,
@@ -538,47 +539,47 @@ def setUp(self) -> None:
538539
self.detector = self.create_detector(project=self.project)
539540
# These are the actions that are healed from the old email action data blobs
540541
# It removes targetIdentifier for IssueOwner targets (since that shouldn't be set for those)
541-
# It also removes the fallthroughType for Team and Member targets (since that shouldn't be set for those)
542+
# It also removes the fallthrough_type for Team and Member targets (since that shouldn't be set for those)
542543
self.HEALED_EMAIL_ACTION_DATA_BLOBS = [
543544
# IssueOwners (targetIdentifier is "None")
544545
{
545-
"targetType": "IssueOwners",
546+
"targetType": ActionTargetType.ISSUE_OWNERS.value,
546547
"id": "sentry.mail.actions.NotifyEmailAction",
547-
"fallthroughType": "ActiveMembers",
548+
"fallthrough_type": FallthroughChoiceType.ACTIVE_MEMBERS,
548549
},
549550
# NoOne Fallthrough (targetIdentifier is "")
550551
{
551-
"targetType": "IssueOwners",
552+
"targetType": ActionTargetType.ISSUE_OWNERS.value,
552553
"id": "sentry.mail.actions.NotifyEmailAction",
553-
"fallthroughType": "NoOne",
554+
"fallthrough_type": FallthroughChoiceType.NO_ONE,
554555
},
555556
# AllMembers Fallthrough (targetIdentifier is None)
556557
{
557-
"targetType": "IssueOwners",
558+
"targetType": ActionTargetType.ISSUE_OWNERS.value,
558559
"id": "sentry.mail.actions.NotifyEmailAction",
559-
"fallthroughType": "AllMembers",
560+
"fallthrough_type": "AllMembers",
560561
},
561562
# NoOne Fallthrough (targetIdentifier is "None")
562563
{
563-
"targetType": "IssueOwners",
564+
"targetType": ActionTargetType.ISSUE_OWNERS.value,
564565
"id": "sentry.mail.actions.NotifyEmailAction",
565-
"fallthroughType": "NoOne",
566+
"fallthrough_type": FallthroughChoiceType.NO_ONE,
566567
},
567568
# ActiveMembers Fallthrough
568569
{
569-
"targetType": "Member",
570+
"targetType": ActionTargetType.MEMBER.value,
570571
"id": "sentry.mail.actions.NotifyEmailAction",
571572
"targetIdentifier": "3234013",
572573
},
573574
# Member Email
574575
{
575576
"id": "sentry.mail.actions.NotifyEmailAction",
576577
"targetIdentifier": "2160509",
577-
"targetType": "Member",
578+
"targetType": ActionTargetType.MEMBER.value,
578579
},
579580
# Team Email
580581
{
581-
"targetType": "Team",
582+
"targetType": ActionTargetType.TEAM.value,
582583
"id": "sentry.mail.actions.NotifyEmailAction",
583584
"targetIdentifier": "188022",
584585
},
@@ -587,10 +588,8 @@ def setUp(self) -> None:
587588
def test_build_rule_action_blob(self) -> None:
588589
for expected, healed in zip(EMAIL_ACTION_DATA_BLOBS, self.HEALED_EMAIL_ACTION_DATA_BLOBS):
589590
action_data = pop_keys_from_data_blob(expected, Action.Type.EMAIL)
590-
591591
# pop the targetType from the action_data
592592
target_type = EmailActionHelper.get_target_type_object(action_data.pop("targetType"))
593-
594593
# Handle all possible targetIdentifier formats
595594
target_identifier: str | None = str(expected["targetIdentifier"])
596595
if target_identifier in ("None", "", None):
@@ -605,9 +604,28 @@ def test_build_rule_action_blob(self) -> None:
605604
},
606605
)
607606
blob = self.handler.build_rule_action_blob(action, self.organization.id)
608-
609607
assert blob == healed
610608

609+
def test_build_rule_action_blob_fallthrough_type_camel_case(self) -> None:
610+
"""
611+
Test that while we are temporarily allowing both fallthroughType and fallthrough_type in Action.data
612+
that both work when building the action data blob and result in the snake case version
613+
"""
614+
action = Action.objects.create(
615+
type=Action.Type.EMAIL,
616+
data={"fallthroughType": FallthroughChoiceType.ACTIVE_MEMBERS},
617+
config={
618+
"target_type": ActionTarget.ISSUE_OWNERS,
619+
"target_identifier": None,
620+
},
621+
)
622+
blob = self.handler.build_rule_action_blob(action, self.organization.id)
623+
assert blob == {
624+
"fallthrough_type": FallthroughChoiceType.ACTIVE_MEMBERS,
625+
"id": "sentry.mail.actions.NotifyEmailAction",
626+
"targetType": "IssueOwners",
627+
}
628+
611629

612630
class TestPluginIssueAlertHandler(BaseWorkflowTest):
613631
def setUp(self) -> None:

‎tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py‎

Lines changed: 61 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -10,17 +10,20 @@
1010
from sentry.grouping.grouptype import ErrorGroupType
1111
from sentry.incidents.grouptype import MetricIssue
1212
from sentry.notifications.models.notificationaction import ActionTarget
13+
from sentry.notifications.types import FallthroughChoiceType
1314
from sentry.testutils.asserts import assert_org_audit_log_exists
1415
from sentry.testutils.cases import APITestCase
1516
from sentry.testutils.outbox import outbox_runner
1617
from sentry.testutils.silo import region_silo_test
1718
from sentry.workflow_engine.models import (
1819
Action,
20+
DataConditionGroupAction,
1921
DetectorWorkflow,
2022
Workflow,
2123
WorkflowDataConditionGroup,
2224
WorkflowFireHistory,
2325
)
26+
from sentry.workflow_engine.models.data_condition import Condition
2427
from tests.sentry.workflow_engine.test_base import MockActionValidatorTranslator
2528

2629

@@ -394,6 +397,13 @@ def setUp(self) -> None:
394397
role="member",
395398
organization=self.organization,
396399
)
400+
self.basic_condition = [
401+
{
402+
"type": Condition.EQUAL.value,
403+
"comparison": 1,
404+
"conditionResult": True,
405+
}
406+
]
397407

398408
@mock.patch("sentry.workflow_engine.endpoints.validators.base.workflow.create_audit_entry")
399409
def test_create_workflow__basic(self, mock_audit: mock.MagicMock) -> None:
@@ -428,13 +438,7 @@ def test_create_workflow__with_config(self) -> None:
428438
def test_create_workflow__with_triggers(self) -> None:
429439
self.valid_workflow["triggers"] = {
430440
"logicType": "any",
431-
"conditions": [
432-
{
433-
"type": "eq",
434-
"comparison": 1,
435-
"conditionResult": True,
436-
}
437-
],
441+
"conditions": self.basic_condition,
438442
}
439443

440444
response = self.get_success_response(
@@ -457,13 +461,7 @@ def test_create_workflow__with_actions(self, mock_action_validator: mock.MagicMo
457461
self.valid_workflow["actionFilters"] = [
458462
{
459463
"logicType": "any",
460-
"conditions": [
461-
{
462-
"type": "eq",
463-
"comparison": 1,
464-
"conditionResult": True,
465-
}
466-
],
464+
"conditions": self.basic_condition,
467465
"actions": [
468466
{
469467
"type": Action.Type.SLACK,
@@ -492,6 +490,55 @@ def test_create_workflow__with_actions(self, mock_action_validator: mock.MagicMo
492490
"actionFilters", []
493491
)[0].get("id")
494492

493+
@mock.patch(
494+
"sentry.notifications.notification_action.registry.action_validator_registry.get",
495+
return_value=MockActionValidatorTranslator,
496+
)
497+
def test_create_workflow__with_fallthrough_type_action(
498+
self, mock_action_validator: mock.MagicMock
499+
) -> None:
500+
self.valid_workflow["actionFilters"] = [
501+
{
502+
"logicType": "any",
503+
"conditions": self.basic_condition,
504+
"actions": [
505+
{
506+
"type": Action.Type.EMAIL,
507+
"config": {
508+
"targetType": "issue_owners",
509+
},
510+
"data": {"fallthroughType": "ActiveMembers"},
511+
},
512+
],
513+
}
514+
]
515+
516+
response = self.get_success_response(
517+
self.organization.slug,
518+
raw_data=self.valid_workflow,
519+
)
520+
521+
assert response.status_code == 201
522+
new_workflow = Workflow.objects.get(id=response.data["id"])
523+
new_action_filters = WorkflowDataConditionGroup.objects.filter(workflow=new_workflow)
524+
assert len(new_action_filters) == len(response.data.get("actionFilters", []))
525+
dcga = DataConditionGroupAction.objects.filter(
526+
condition_group=new_action_filters[0].condition_group
527+
).first()
528+
assert dcga
529+
assert str(new_action_filters[0].condition_group.id) == response.data.get(
530+
"actionFilters", []
531+
)[0].get("id")
532+
assert (
533+
response.data.get("actionFilters")[0]
534+
.get("actions")[0]
535+
.get("data")
536+
.get("fallthrough_type")
537+
== FallthroughChoiceType.ACTIVE_MEMBERS.value
538+
)
539+
assert dcga.action.type == Action.Type.EMAIL
540+
assert dcga.action.data == {"fallthrough_type": "ActiveMembers"}
541+
495542
def test_create_invalid_workflow(self) -> None:
496543
self.valid_workflow["name"] = ""
497544
response = self.get_response(

‎tests/sentry/workflow_engine/migration_helpers/test_migrate_rule_action.py‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,8 @@ def assert_action_data_blob(
114114
assert action.data.get(field) == source_value
115115
else:
116116
# For unmapped fields, check directly with empty string default
117-
if action.type == Action.Type.EMAIL and field == "fallthroughType":
118-
# for email actions, the default value for fallthroughType should be "ActiveMembers"
117+
if action.type == Action.Type.EMAIL and field == "fallthrough_type":
118+
# for email actions, the default value for fallthrough_type should be "ActiveMembers"
119119
assert action.data.get(field) == compare_dict.get(
120120
field, "ActiveMembers"
121121
)
@@ -131,10 +131,10 @@ def assert_action_data_blob(
131131
if key not in exclude_keys:
132132
if (
133133
action.type == Action.Type.EMAIL
134-
and key == "fallthroughType"
134+
and key == "fallthrough_type"
135135
and action.config.get("target_type") != ActionTarget.ISSUE_OWNERS
136136
):
137-
# for email actions, fallthroughType should only be set for when targetType is ISSUE_OWNERS
137+
# for email actions, fallthrough_type should only be set for when targetType is ISSUE_OWNERS
138138
continue
139139
else:
140140
assert compare_dict[key] == action.data[key]
@@ -626,9 +626,9 @@ def test_email_migration_malformed(self) -> None:
626626
{
627627
"uuid": "12345678-90ab-cdef-0123-456789abcdef",
628628
"id": "sentry.mail.actions.NotifyEmailAction",
629-
"fallthroughType": "NoOne",
629+
"fallthrough_type": "NoOne",
630630
},
631-
# This should be ok since we have a default value for fallthroughType
631+
# This should be ok since we have a default value for fallthrough_type
632632
{
633633
"uuid": "12345678-90ab-cdef-0123-456789abcdef",
634634
"id": "sentry.mail.actions.NotifyEmailAction",

0 commit comments

Comments
 (0)