ref(aci): dual write workflow group action status #92522
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #92522 +/- ##
==========================================
+ Coverage 86.61% 87.89% +1.28%
==========================================
Files 10236 10236
Lines 586994 587131 +137
Branches 22806 22806
==========================================
+ Hits 508398 516084 +7686
+ Misses 78166 70617 -7549
Partials 430 430 |
| all_statuses = WorkflowActionGroupStatus.objects.filter( | ||
| group=group, action_id__in=action_to_workflows_ids.keys() | ||
| ) |
There was a problem hiding this comment.
wasn't sure how to query for specific action+workflow combos without making a long query with a ton of Q()'s, so opted for iterating through a big query
There was a problem hiding this comment.
any reason to not also do workflow_id__in just to ensure that results are as narrow as we can simply get them?
Then the filtering below can use [] instead of get too.
There was a problem hiding this comment.
good point, will update
| workflow_action_statuses = WorkflowActionGroupStatus.objects.filter( | ||
| id__in=status_ids, date_updated__lt=now | ||
| ) | ||
| action_ids = {status.action_id for status in workflow_action_statuses} |
There was a problem hiding this comment.
using a set will dedupe actions if multiple workflows would fire the same action. in a follow up we should know which workflow is firing it (out of all the workflows that could have done so here)
cathteng
force-pushed
the
cathy/aci/dual-write-workflow-group-action-status
branch
from
9f0ffee to
441484a
Compare
| all_statuses = WorkflowActionGroupStatus.objects.filter( | ||
| group=group, action_id__in=action_to_workflows_ids.keys() | ||
| ) |
There was a problem hiding this comment.
any reason to not also do workflow_id__in just to ensure that results are as narrow as we can simply get them?
Then the filtering below can use [] instead of get too.
| create_workflow_fire_histories(filtered_actions, event_data) | ||
|
|
||
| return filtered_actions | ||
|
|
||
|
|
||
| def get_workflow_group_action_statuses( | ||
| action_to_workflows_ids: dict[int, set[int]], group: Group | ||
| ) -> dict[int, list[WorkflowActionGroupStatus]]: |
There was a problem hiding this comment.
Docstring.. "returns them grouped by Action ID" as I'm not sure the key is obvious enough from name or type.
| @@ -127,11 +130,122 @@ def filter_recently_fired_workflow_actions( | |||
| actions_without_statuses_ids = {action.id for action in actions_without_statuses} | |||
| filtered_actions = actions.filter(id__in=actions_to_include | actions_without_statuses_ids) | |||
|
|
|||
| # dual write to WorkflowActionGroupStatus | |||
| filter_recently_fired_workflow_actions(filtered_action_groups, event_data) | |||
There was a problem hiding this comment.
Maybe
# dual write to ... ignoring results for now until they are canonical
_ = filter_recently_fired_workflow_action(...)
just to make our discarding of the return value explicit and obviously intentional.
| return actions_with_statuses | ||
|
|
||
|
|
||
| def update_workflow_action_group_statuses( |
| # TODO: write this in a single spot | ||
| # create_workflow_fire_histories | ||
|
|
||
| return Action.objects.filter(id__in=action_ids).distinct() |
There was a problem hiding this comment.
aren't these guaranteed to be distinct?
| @@ -94,7 +97,7 @@ def create_workflow_fire_histories( | |||
|
|
|||
|
|
|||
| # TODO(cathy): only reinforce workflow frequency for certain issue types | |||
| def filter_recently_fired_workflow_actions( | |||
| def filter_recently_fired_actions( | |||
There was a problem hiding this comment.
By name, this just filters what we pass in, but it also does status and history updating.
I think a docstring with "Returns actions associated with the provided DataConditionGroups, excluding those that ... whatever. Also updates ...".
I do wonder if some of the book-keeping could be separated from the filtering to simplify that.
| ) | ||
|
|
||
| # TODO: need to know the exact workflow that fired the action | ||
| return action_ids |
There was a problem hiding this comment.
So, action_ids are the actions not filtered out based on workflow config and previous status? Rather than constructing action_ids, I wonder if it'd be simpler to note action_ids filtered out, and return <full set of action ids> - filtered_action_ids, and leave the WAGS book-keeping as its own thing?
There was a problem hiding this comment.
i will look into whether this can be cleaned up a bit in a follow up, i'm also not a huge fan how i've mixed updating the statuses with figuring out which actions to fire
| ) | ||
|
|
||
| # TODO: write this in a single spot | ||
| # create_workflow_fire_histories |
There was a problem hiding this comment.
kinda think this should be lifted out of here instead; noting which workflows fire isn't really a job for action filtering; the higher level stuff needs to know "we have our list of workflow actions to fire". If we bump this to a higher level, this TODO doesn't need to exist and I think the correctness of it all is clearer.
That said, not a change for this PR.
Must merge #92478 first
Dual write
WorkflowGroupActionStatusalongsideActionGroupStatus. We need a new function to do this because each action can be associated with its own set of workflows, and we want to enforce each workflow's frequency independently for each workflow+action combo. It's also difficult to query specific combinations of workflow+action statuses, so I query all the statuses for the actions+group and iterate to find the valid ones.I renamed the old function to
filter_recently_fired_actionsand the new function is calledfilter_recently_fired_workflow_actions:)