refactor(core): Continue moving typeorm operators to repositories (no-changelog)
#8186
Conversation
n8n-assistant
bot
added
core
| @@ -106,17 +106,24 @@ export = { | |||
|
|
|||
| if (['owner', 'admin'].includes(req.user.globalRole.name)) { | |||
| if (tags) { | |||
| const workflowIds = await getWorkflowIdsViaTags(parseTagNames(tags)); | |||
| const workflowIds = await Container.get(TagRepository).getWorkflowIdsViaTags( | |||
There was a problem hiding this comment.
Once we are done moving all DB stuff into repositories, we should start moving all logic into services, and make sure that the public api and the rest api reuse as much code as possible. we have currently too much duplication in the public api.
There was a problem hiding this comment.
I'd love to know why the Public API required so much duplication.
There was a problem hiding this comment.
our code used to be a lot less re-usable because of how it was structured in the past.
packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts
Outdated
Show resolved
Hide resolved
| @@ -89,7 +91,10 @@ export class CollaborationService { | |||
| if (workflowUserIds.length === 0) { | |||
| return; | |||
| } | |||
| const users = await this.userService.getByIds(this.userService.getManager(), workflowUserIds); | |||
| const users = await this.userRepository.getByIds( | |||
| this.userService.getManager(), | |||
There was a problem hiding this comment.
not sure why UserService.getManager was added, but this is unnecessary, and should be removed. if userRepository.getByIds wants to accept a optional transaction, then we should update it to
async getByIds(ids: string[], transaction: EntityManager = this.manager) instead of passing around entity-managers like this.
There was a problem hiding this comment.
getByIds requires a transaction because I'm moving all persistence logic to the persistence layer and that is how this method existed originally. We should refactor to create more generalized reusable methods in the repositories, but for now I'd like to focus on finishing moving all persistence logic to the persistence layer, which is a ton of work as it is 🙈
| @@ -138,7 +138,7 @@ export class AuthController { | |||
| } | |||
|
|
|||
| try { | |||
| user = await this.userService.findOneOrFail({ where: {} }); | |||
| user = await this.userRepository.findOneOrFail({ where: {}, relations: ['globalRole'] }); | |||
There was a problem hiding this comment.
in the long run we should aim to move more code out of these controllers, and avoid having repositories as direct dependencies in them.
There was a problem hiding this comment.
Here I wasn't 100% sure what to do since the service method was a paper-thin wrapper for the repository method.
3 flaky tests on run #3553 ↗︎
Details:
|
|||||||||||||||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Sharing > should work for admin role on credentials created by others (also can share it with themselves) |
Screenshots
Video
|
|
24-ndv-paired-item.cy.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| NDV > resolves expression with default item when input node is not parent, while still pairing items |
Screenshots
Video
|
|
29-templates.cy.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| Templates > should save template id with the workflow |
Screenshots
Video
|
|
Review all test suite changes for PR #8186 ↗︎
|
✅ All Cypress E2E specs passed |
|
Got released with |
Follow-up to: #8163