Refactor workflows endpoints for user management #2676
Conversation
| return { | ||
| id: id.toString(), | ||
| ...rest, | ||
| tags: tags?.map(({ id, ...rest }) => ({ id: id.toString(), ...rest })) ?? [], | ||
| }; |
There was a problem hiding this comment.
Copying instead of mutating here is inefficient, but TS is right to complain about forcing the field type change without adjusting the type in the model, and the model's field type should not be adjusted - it should reflect the column type.
Hence I think we need to have workflow ID and tag ID types in the FE match exactly our class models to stop doing this needless copy or mutation work - this applies to other workflow endpoints as well.
| if (userId) { | ||
| queryBuilder.innerJoin('w.shared', 'shared'); | ||
| queryBuilder.andWhere('shared.user = :userId', { userId }); | ||
| async getActiveWorkflows(user?: User): Promise<IWorkflowDb[]> { |
There was a problem hiding this comment.
For now, optional user since getActiveWorkflows() is used without arg by removeAll(), which is used in start.ts, ActiveWorkflowRunner.ts, and TestWebhooks.ts. Delving there would increase the scope of the PR.
ivov
added
feature
| } else { | ||
| const shared = await Db.collections.SharedWorkflow!.find({ | ||
| relations: ['workflow'], | ||
| where: whereClause({ |
There was a problem hiding this comment.
I think it's better to have possibly additional filters to this whereClause allowing us to use active = true as part of it. This would reduce the need for the accumulator below without sacrificing re-usability.
I know the whereClause function already expects a third optional argument so maybe we should rethink how this can work.
Let me know if you think we should change this now or merge first and then make all changes at once when refactoring code to centralize these helpers into a model-like class.
There was a problem hiding this comment.
Agree, let's do away with reduce() and keep the filters flexible, ideally merging first and then refactoring, so that we have a full view of all the call variants that we need to account for in UserManagementModel.
krynble
force-pushed
the
user-management-backend-workflows-endpoints
branch
from
c206a9e to
6a753a3
Compare
Pinned chokidar to fix build: #2696