♻️ ref(aci): only fire active actions #97927
Conversation
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #97927 +/- ##
=======================================
Coverage 80.64% 80.65%
=======================================
Files 8591 8591
Lines 378981 379049 +68
Branches 24684 24684
=======================================
+ Hits 305647 305707 +60
- Misses 72955 72963 +8
Partials 379 379 |
| @@ -465,6 +466,10 @@ def process_workflows( | |||
| create_workflow_fire_histories( | |||
| detector, actions, event_data, should_trigger_actions, is_delayed=False | |||
| ) | |||
|
|
|||
| # We only want to fire active actions | |||
| actions = actions.filter(status=ObjectStatus.ACTIVE) | |||
There was a problem hiding this comment.
Should this be done in fire_actions or deduplicate_actions inside of it? fire_actions is used in both process_workflows and delayed workflow processing
There was a problem hiding this comment.
i could see an argument for fire_actions but my thought in keeping it where it is because i want fire_actions and deduplicate_actions to do exactly what they say - deduplicate them and fire them.
if we add it to either method, they have a side effect and do an extra filter that i don't think they should
There was a problem hiding this comment.
open to change it tho
There was a problem hiding this comment.
I moved deduplicate_actions inside fire_actions before. I don't think it hurts because you should only fire active actions anyway
There was a problem hiding this comment.
might be worth putting in fire_actions just to DRY up the filter in delayed workflows and make sure there aren't any footguns there. another alternative could be to just put an if statement in the action.trigger, but that's a little less ideal since it'll create all the extra overhead until executing that.
we'll also want to confirm that this won't make 2 queries; 1 for the first set of actions that we log for fire history, and a second for this filter. if there are 2 queries, it might be better to just quickly filter the array.
now that we have introduced a status field for Actions and will update the status of actins depending on integration status, lets only fire active actions.
we will still record
WorkflowFireHistoryentries for all the disabled actions, just not fire them.i also added the base manager for
Actionto filter out all actions in process of being deleted. this matchesWorkflowandDetectorbehavior (which both also have the status field)