-
-
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
base: master
Are you sure you want to change the base?
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #103694 +/- ##
===========================================
- Coverage 80.51% 80.50% -0.02%
===========================================
Files 9274 9278 +4
Lines 395967 397190 +1223
Branches 25250 25250
===========================================
+ Hits 318819 319745 +926
- Misses 76688 76985 +297
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
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