feat(ACI): Add error for unsupported condition#110025
Conversation
github-actions
bot
added
the
Scope: Backend
|
|
||
| for condition in trigger_conditions: | ||
| if condition.type in UNSUPPORTED_CONDITIONS: | ||
| errors.append(f"Condition not supported: {condition.type}") |
There was a problem hiding this comment.
Can someone who actually works on the front end confirm we can show a banner if this is found?
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| if workflow.when_condition_group | ||
| else [] | ||
| ) | ||
| filter_conditions = list(workflow_dcg.condition_group.conditions.all()) |
There was a problem hiding this comment.
Duplicate condition fetching logic across two locations
Low Severity
The trigger and filter conditions are fetched identically in both _generate_rule_conditions_filters (lines 470-476 of the method body) and the new unsupported-condition check (lines 645-650). Both use the same workflow.when_condition_group.conditions.all() and workflow_dcg.condition_group.conditions.all() calls with the same null-guard pattern. This duplicated logic increases the risk of the two locations diverging if one is updated without the other. The unsupported check could be integrated into _generate_rule_conditions_filters or the conditions could be fetched once and reused.
Additional Locations (1)
| result[workflow]["filters"] = filters | ||
|
|
||
| # Check for unsupported conditions | ||
| UNSUPPORTED_CONDITIONS = [ |
There was a problem hiding this comment.
I'd feel better with this as a top-level def with "# These conditions exist in Workflows, but are unsupported in ...; if we're trying to return these in legacy APIs, our best option is to error rather than being broken" or something.
Also, certainly doesn't matter in practice, but it'd be nice if this was a set.
2 participants
We have some conditions that exist in ACI that don't exist in the old world - if a
Workflowhas one / more of these we can't render it in the old UI, but we should at least let the user know they're seeing incomplete data. This PR adds the unsupported condition type to the errors in the payload which can be picked up by the front end and used to render a warning in a follow up.