-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(ACI): Accept fallthrough type in actions #103694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/sentry/notifications/notification_action/action_handler_registry/email_handler.py
Show resolved
Hide resolved
| "sentry.notifications.notification_action.registry.action_validator_registry.get", | ||
| return_value=MockActionValidatorTranslator, | ||
| ) | ||
| def test_create_workflow__with_fallthrough_type_action( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank youuuu!!
tests/sentry/workflow_engine/endpoints/test_organization_workflow_index.py
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #103694 +/- ##
===========================================
+ Coverage 80.51% 80.61% +0.10%
===========================================
Files 9274 9278 +4
Lines 395967 396089 +122
Branches 25250 25250
===========================================
+ Hits 318819 319325 +506
+ Misses 76688 76304 -384
Partials 460 460 |
|
Do we need a backfill migration to fix this for existing actions? All existing actions will have the old camelCase key |
10f0042 to
f14233e
Compare
| action_data = action.data.copy() | ||
| if action.data.get("fallthroughType"): | ||
| del action_data["fallthroughType"] | ||
| action_data["fallthrough_type"] = action.data["fallthroughType"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: CamelCase key silently overwrites snake_case key
When both fallthroughType and fallthrough_type keys exist in action.data, the camelCase version silently overwrites the snake_case version. The schema allows both keys simultaneously without mutual exclusivity constraints, and the conversion logic on line 58 unconditionally overwrites fallthrough_type with the value from fallthroughType, potentially discarding the user's intended value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is the point
mifu67
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but you might want to wait for frontend review too
malwilley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Companion PR to #103694 to migrate `fallthroughType` to `fallthrough_type` in `Action.data`. Other PR must be merged first.
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