Reorient ActionManager to group by teamId for practicality - #433
Conversation
| public async reloadAllActions(): Promise<void> { | ||
| this.actionCache = await this.db.fetchAllActionsMap() | ||
| this.actionCache = Object.fromEntries( | ||
| Object.entries(groupBy(await this.db.fetchAllActions(), 'team_id')).map(([teamId, actions]) => [ |
There was a problem hiding this comment.
Do you expect db.fetchAllActions() to be used elsewhere? If not, I think it makes things a lot simpler to just have db.fetchAllActionsByTeam() which returns them grouped by team?
There was a problem hiding this comment.
Sorry if I was a bit unclear: I meant fetchAllActionsGroupedByTeam to replace fetchAllActions (which means getting rid of all the groupBy logic, since we're building the grouping from scratch, just like in fetchAllActions() right now.
Thoughts?
There was a problem hiding this comment.
Ah, I see what you mean, done. Though maybe I'll leave groupBy in utils since maybe it'll come in useful and I've already written it
There was a problem hiding this comment.
"maybe it'll come in useful"
... is a dangerous thing to say :D. See for example the HTTP server (let's remove it) :).
There was a problem hiding this comment.
LOL, yeah. Well, at least it's isolated and tested. Unlike the HTTP server… pending removal in #282 for some time now
neilkakkar
left a comment
There was a problem hiding this comment.
A few red tests, but otherwise LGTM
…cticality (PostHog/plugin-server#433) * Reorient `ActionManager` to group by teamId for practicality * Make `getTeamActions()` return type more versatile * Fix PubSub's lack of teamId * Add DB.fetchAllActionsGroupedByTeam * Address feedback
Changes
Followup to #403, makes
ActionManagermore practical for the actual matching engine.Checklist