From 51ac8f6896665bc6167ce0914203d2e46e44eb68 Mon Sep 17 00:00:00 2001 From: Kai Date: Thu, 27 Aug 2026 12:12:02 +0000 Subject: [PATCH] refactor(core): Split workflow review repositories by use case (no-changelog) (#37188) --- ...s => workflow-review.repositories.test.ts} | 120 ++--- packages/@n8n/db/src/repositories/index.ts | 8 +- .../workflow-review-inbox.repository.ts | 224 +++++++++ .../workflow-review-lifecycle.repository.ts | 147 ++++++ .../workflow-review-request.repository.ts | 454 +----------------- ...rkflow-review-inbox.detail.service.test.ts | 17 +- ...workflow-review-inbox.list.service.test.ts | 24 +- ...kflow-review-lifecycle.integration.test.ts | 5 +- .../workflow-review-lifecycle.service.test.ts | 101 ++-- .../workflow-review-inbox.service.ts | 8 +- .../workflow-review-lifecycle.service.ts | 22 +- 11 files changed, 557 insertions(+), 573 deletions(-) rename packages/@n8n/db/src/repositories/__tests__/{workflow-review-request.repository.test.ts => workflow-review.repositories.test.ts} (86%) create mode 100644 packages/@n8n/db/src/repositories/workflow-review-inbox.repository.ts create mode 100644 packages/@n8n/db/src/repositories/workflow-review-lifecycle.repository.ts diff --git a/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts b/packages/@n8n/db/src/repositories/__tests__/workflow-review.repositories.test.ts similarity index 86% rename from packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts rename to packages/@n8n/db/src/repositories/__tests__/workflow-review.repositories.test.ts index f9f5cb3362d..2ecbf5c1511 100644 --- a/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts +++ b/packages/@n8n/db/src/repositories/__tests__/workflow-review.repositories.test.ts @@ -11,9 +11,11 @@ import { WorkflowReviewRequest } from '../../entities/workflow-review-request.ee import { TypeOrmTransaction } from '../../services/typeorm-transaction'; import { mockEntityManager } from '../../utils/test-utils/mock-entity-manager'; import { - WorkflowReviewRequestRepository, + WorkflowReviewInboxRepository, type InboxVisibility, -} from '../workflow-review-request.repository'; +} from '../workflow-review-inbox.repository'; +import { WorkflowReviewLifecycleRepository } from '../workflow-review-lifecycle.repository'; +import { WorkflowReviewRequestRepository } from '../workflow-review-request.repository'; /** Stand-ins for the SQL TypeORM would render for the correlated junction subqueries. */ const AUTHOR_SUBQUERY_SQL = '(SELECT 1 FROM workflow_review_request_authors author WHERE ...)'; @@ -50,9 +52,11 @@ function involvedVisibility( }; } -describe('WorkflowReviewRequestRepository', () => { +describe('workflow review repositories', () => { const entityManager = mockEntityManager(WorkflowReviewRequest); - const repo = Container.get(WorkflowReviewRequestRepository); + const requestRepository = Container.get(WorkflowReviewRequestRepository); + const inboxRepository = Container.get(WorkflowReviewInboxRepository); + const lifecycleRepository = Container.get(WorkflowReviewLifecycleRepository); let queryBuilder: Mocked>; let subQueryBuilders: Array>>; @@ -92,17 +96,17 @@ describe('WorkflowReviewRequestRepository', () => { queryBuilder.getMany.mockResolvedValue([]); queryBuilder.getRawMany.mockResolvedValue([]); - vi.spyOn(repo, 'createQueryBuilder').mockReturnValue(queryBuilder); + vi.spyOn(inboxRepository, 'createQueryBuilder').mockReturnValue(queryBuilder); }); - describe('createRequest', () => { + describe('WorkflowReviewRequestRepository.createRequest', () => { it('persists an open pending request with audit fields initialised', async () => { (entityManager.create as Mock).mockImplementation( (_target: unknown, entityLike: unknown) => entityLike as WorkflowReviewRequest, ); entityManager.save.mockImplementationOnce(async (_target, entity) => entity); - await repo.createRequest( + await requestRepository.createRequest( { id: 'req-1', projectId: 'proj-1', @@ -134,7 +138,7 @@ describe('WorkflowReviewRequestRepository', () => { ); entityManager.save.mockImplementationOnce(async (_target, entity) => entity); - await repo.createRequest( + await requestRepository.createRequest( { projectId: 'proj-1', title: 'Review title', @@ -151,7 +155,7 @@ describe('WorkflowReviewRequestRepository', () => { }); }); - describe('findRequestsForWorkflow', () => { + describe('WorkflowReviewRequestRepository.findRequestsForWorkflow', () => { let queryBuilder: Mocked>; beforeEach(() => { @@ -171,7 +175,7 @@ describe('WorkflowReviewRequestRepository', () => { }); it('scopes to the requested workflow and orders newest first, ties broken by id', async () => { - await repo.findRequestsForWorkflow('workflow-1'); + await requestRepository.findRequestsForWorkflow('workflow-1'); expect(queryBuilder.where).toHaveBeenCalledWith('requestWorkflow.workflowId = :workflowId', { workflowId: 'workflow-1', @@ -184,7 +188,7 @@ describe('WorkflowReviewRequestRepository', () => { }); it.each(['open', 'closed'] as const)('narrows to state %s when given', async (state) => { - await repo.findRequestsForWorkflow('workflow-1', { state }); + await requestRepository.findRequestsForWorkflow('workflow-1', { state }); expect(queryBuilder.andWhere).toHaveBeenCalledWith('request.state = :state', { state }); }); @@ -197,7 +201,7 @@ describe('WorkflowReviewRequestRepository', () => { }); queryBuilder.getCount.mockResolvedValue(5); - const [data, count] = await repo.findRequestsForWorkflow('workflow-1', { + const [data, count] = await requestRepository.findRequestsForWorkflow('workflow-1', { skip: 1, take: 1, }); @@ -232,7 +236,7 @@ describe('WorkflowReviewRequestRepository', () => { ], }); - const [data] = await repo.findRequestsForWorkflow('workflow-1'); + const [data] = await requestRepository.findRequestsForWorkflow('workflow-1'); expect(data[0]).toEqual({ id: 'req-1', @@ -247,7 +251,7 @@ describe('WorkflowReviewRequestRepository', () => { }); it('applies skip and take when they are zero', async () => { - await repo.findRequestsForWorkflow('workflow-1', { skip: 0, take: 0 }); + await requestRepository.findRequestsForWorkflow('workflow-1', { skip: 0, take: 0 }); expect(queryBuilder.skip).toHaveBeenCalledWith(0); expect(queryBuilder.take).toHaveBeenCalledWith(0); @@ -270,7 +274,7 @@ describe('WorkflowReviewRequestRepository', () => { }); queryBuilder.getCount.mockResolvedValue(2); - const [data] = await repo.findRequestsForWorkflow('workflow-1'); + const [data] = await requestRepository.findRequestsForWorkflow('workflow-1'); expect(queryBuilder.addSelect).toHaveBeenCalledWith( 'requestWorkflow.workflowVersionId', @@ -293,7 +297,7 @@ describe('WorkflowReviewRequestRepository', () => { }); }); - describe('findOpenRequestsForWorkflows', () => { + describe('WorkflowReviewLifecycleRepository.findOpenRequestsAffectedByWorkflows', () => { let queryBuilder: Mocked>; beforeEach(() => { @@ -307,14 +311,17 @@ describe('WorkflowReviewRequestRepository', () => { }); it('returns an empty list without querying when no workflow ids are given', async () => { - const result = await repo.findOpenRequestsForWorkflows([], {}); + const result = await lifecycleRepository.findOpenRequestsAffectedByWorkflows([], {}); expect(result).toEqual([]); expect(entityManager.createQueryBuilder).not.toHaveBeenCalled(); }); it('scopes to the given workflows and to open requests only', async () => { - await repo.findOpenRequestsForWorkflows(['workflow-1', 'workflow-2'], {}); + await lifecycleRepository.findOpenRequestsAffectedByWorkflows( + ['workflow-1', 'workflow-2'], + {}, + ); expect(queryBuilder.where).toHaveBeenCalledWith( 'requestWorkflow.workflowId IN (:...workflowIds)', @@ -338,7 +345,10 @@ describe('WorkflowReviewRequestRepository', () => { ], }); - const result = await repo.findOpenRequestsForWorkflows(['workflow-1', 'workflow-2'], {}); + const result = await lifecycleRepository.findOpenRequestsAffectedByWorkflows( + ['workflow-1', 'workflow-2'], + {}, + ); expect(queryBuilder.addSelect).toHaveBeenCalledWith( 'requestWorkflow.workflowVersionId', @@ -362,7 +372,7 @@ describe('WorkflowReviewRequestRepository', () => { const transactionManager = mock(); (transactionManager.createQueryBuilder as Mock).mockReturnValue(queryBuilder); - await repo.findOpenRequestsForWorkflows(['workflow-1'], { + await lifecycleRepository.findOpenRequestsAffectedByWorkflows(['workflow-1'], { trx: new TypeOrmTransaction(transactionManager), }); @@ -371,7 +381,7 @@ describe('WorkflowReviewRequestRepository', () => { }); }); - describe('findUnreviewableOpenRequestIds', () => { + describe('WorkflowReviewLifecycleRepository.findUnreviewableOpenRequestIds', () => { let queryBuilder: Mocked>; beforeEach(() => { @@ -418,23 +428,23 @@ describe('WorkflowReviewRequestRepository', () => { }, ]); - expect(await repo.findUnreviewableOpenRequestIds({})).toEqual(['req-orphan']); + expect(await lifecycleRepository.findUnreviewableOpenRequestIds({})).toEqual(['req-orphan']); expect(queryBuilder.andWhere).not.toHaveBeenCalled(); - await repo.findUnreviewableOpenRequestIds({}, ['req-1', 'req-2']); + await lifecycleRepository.findUnreviewableOpenRequestIds({}, ['req-1', 'req-2']); expect(queryBuilder.andWhere).toHaveBeenCalledWith('review.id IN (:...candidateRequestIds)', { candidateRequestIds: ['req-1', 'req-2'], }); (entityManager.createQueryBuilder as Mock).mockClear(); - expect(await repo.findUnreviewableOpenRequestIds({}, [])).toEqual([]); + expect(await lifecycleRepository.findUnreviewableOpenRequestIds({}, [])).toEqual([]); expect(entityManager.createQueryBuilder).not.toHaveBeenCalled(); }); }); - describe('closeRequests', () => { + describe('WorkflowReviewRequestRepository.closeRequests', () => { it('bulk-closes the given requests, clearing the closing user and bumping updatedAt', async () => { - await repo.closeRequests(['req-1', 'req-2'], {}); + await requestRepository.closeRequests(['req-1', 'req-2'], {}); expect(entityManager.update).toHaveBeenCalledWith(WorkflowReviewRequest, ['req-1', 'req-2'], { state: 'closed', @@ -443,18 +453,18 @@ describe('WorkflowReviewRequestRepository', () => { }); entityManager.update.mockClear(); - await repo.closeRequests([], {}); + await requestRepository.closeRequests([], {}); expect(entityManager.update).not.toHaveBeenCalled(); }); }); - describe('findById', () => { + describe('WorkflowReviewRequestRepository.findById', () => { it("reads through the context's transaction manager", async () => { const transactionManager = mock(); const request = mock({ id: 'req-1' }); transactionManager.findOne.mockResolvedValue(request); - const result = await repo.findById('req-1', { + const result = await requestRepository.findById('req-1', { trx: new TypeOrmTransaction(transactionManager), }); @@ -466,19 +476,19 @@ describe('WorkflowReviewRequestRepository', () => { }); }); - describe('findManyForInbox', () => { + describe('WorkflowReviewInboxRepository.findRequests', () => { it('skips visibility filtering entirely for the whole-inbox scope', async () => { const rows = [mock({ id: 'req-1' })]; queryBuilder.getMany.mockResolvedValueOnce(rows); - const result = await repo.findManyForInbox({ + const result = await inboxRepository.findRequests({ visibility: allVisibility, state: 'open', limit: 15, }); expect(result).toBe(rows); - expect(repo.createQueryBuilder).toHaveBeenCalledWith('review'); + expect(inboxRepository.createQueryBuilder).toHaveBeenCalledWith('review'); expect(queryBuilder.andWhere).not.toHaveBeenCalledWith( expect.stringContaining('review.projectId'), expect.anything(), @@ -495,7 +505,7 @@ describe('WorkflowReviewRequestRepository', () => { const rows = [mock({ id: 'req-1' })]; queryBuilder.getMany.mockResolvedValueOnce(rows); - const result = await repo.findManyForInbox({ + const result = await inboxRepository.findRequests({ visibility: involvedVisibility({ adminProjectIds: ['admin-proj'], readableProjectIds: ['read-1', 'read-2'], @@ -516,7 +526,7 @@ describe('WorkflowReviewRequestRepository', () => { }); it('resolves workflow readability through shared_workflow, not the stored project', async () => { - await repo.findManyForInbox({ + await inboxRepository.findRequests({ visibility: involvedVisibility({ readableProjectIds: ['read-1'] }), limit: 15, }); @@ -539,7 +549,7 @@ describe('WorkflowReviewRequestRepository', () => { }); it('drops the readability conjunct entirely when the caller is unrestricted', async () => { - await repo.findManyForInbox({ + await inboxRepository.findRequests({ visibility: involvedVisibility({ readableProjectIds: null }), limit: 15, }); @@ -555,7 +565,7 @@ describe('WorkflowReviewRequestRepository', () => { }); it('correlates both involvement subqueries through their entities, never literal table names', async () => { - await repo.findManyForInbox({ visibility: involvedVisibility(), limit: 15 }); + await inboxRepository.findRequests({ visibility: involvedVisibility(), limit: 15 }); const [authorSubQuery, reviewerSubQuery] = subQueryBuilders; expect(authorSubQuery.from).toHaveBeenCalledWith( @@ -577,7 +587,7 @@ describe('WorkflowReviewRequestRepository', () => { }); it('returns nothing when no project is readable, admin projects included', async () => { - await repo.findManyForInbox({ + await inboxRepository.findRequests({ visibility: involvedVisibility({ adminProjectIds: ['admin-proj'], readableProjectIds: [] }), limit: 15, }); @@ -587,7 +597,7 @@ describe('WorkflowReviewRequestRepository', () => { }); it('matches nothing when the caller administers no project and can read none', async () => { - const result = await repo.findManyForInbox({ + const result = await inboxRepository.findRequests({ visibility: involvedVisibility({ adminProjectIds: [], readableProjectIds: [] }), limit: 15, }); @@ -597,11 +607,11 @@ describe('WorkflowReviewRequestRepository', () => { }); it('applies the keyset boundary carried in the cursor without an anchor lookup', async () => { - const findOneSpy = vi.spyOn(repo, 'findOne'); + const findOneSpy = vi.spyOn(inboxRepository, 'findOne'); queryBuilder.getMany.mockResolvedValueOnce([]); const createdAt = new Date('2024-01-02T00:00:00.000Z'); - await repo.findManyForInbox({ + await inboxRepository.findRequests({ visibility: involvedVisibility(), limit: 10, cursor: { createdAt, id: 'req-cursor' }, @@ -616,7 +626,7 @@ describe('WorkflowReviewRequestRepository', () => { describe('category filter', () => { it('leaves the query untouched when no category is requested', async () => { - await repo.findManyForInbox({ + await inboxRepository.findRequests({ visibility: allVisibility, limit: 15, }); @@ -625,7 +635,7 @@ describe('WorkflowReviewRequestRepository', () => { }); it('correlates both junction subqueries through their entities, never literal table names', async () => { - await repo.findManyForInbox({ + await inboxRepository.findRequests({ visibility: allVisibility, category: { userId: 'user-1', category: 'authored' }, limit: 15, @@ -647,7 +657,7 @@ describe('WorkflowReviewRequestRepository', () => { // The requester always has an author row, so neither predicate needs // the nullable `createdById`. it('matches a non-reviewing author for the authored section', async () => { - await repo.findManyForInbox({ + await inboxRepository.findRequests({ visibility: allVisibility, category: { userId: 'user-1', category: 'authored' }, limit: 15, @@ -660,7 +670,7 @@ describe('WorkflowReviewRequestRepository', () => { }); it('matches assigned reviewers first for the waiting section', async () => { - await repo.findManyForInbox({ + await inboxRepository.findRequests({ visibility: allVisibility, category: { userId: 'user-1', category: 'waiting' }, limit: 15, @@ -673,7 +683,7 @@ describe('WorkflowReviewRequestRepository', () => { }); it('narrows the visibility predicate instead of replacing it', async () => { - await repo.findManyForInbox({ + await inboxRepository.findRequests({ visibility: involvedVisibility(), category: { userId: 'user-1', category: 'waiting' }, limit: 15, @@ -689,7 +699,7 @@ describe('WorkflowReviewRequestRepository', () => { it('applies the category filter before the limit and the cursor boundary', async () => { const createdAt = new Date('2024-01-02T00:00:00.000Z'); - await repo.findManyForInbox({ + await inboxRepository.findRequests({ visibility: allVisibility, category: { userId: 'user-1', category: 'authored' }, state: 'open', @@ -706,16 +716,16 @@ describe('WorkflowReviewRequestRepository', () => { }); }); - describe('countByStateForInbox', () => { + describe('WorkflowReviewInboxRepository.countRequestsByState', () => { it('groups by state under the involvement visibility predicate', async () => { queryBuilder.getRawMany.mockResolvedValueOnce([{ state: 'open', count: '2' }]); - const result = await repo.countByStateForInbox({ - visibility: involvedVisibility({ readableProjectIds: ['proj-1', 'proj-2'] }), - }); + const result = await inboxRepository.countRequestsByState( + involvedVisibility({ readableProjectIds: ['proj-1', 'proj-2'] }), + ); expect(result).toEqual({ open: 2, closed: 0 }); - expect(repo.createQueryBuilder).toHaveBeenCalledWith('review'); + expect(inboxRepository.createQueryBuilder).toHaveBeenCalledWith('review'); expect(queryBuilder.select).toHaveBeenCalledWith('review.state', 'state'); expect(queryBuilder.addSelect).toHaveBeenCalledWith('COUNT(*)', 'count'); expect(queryBuilder.groupBy).toHaveBeenCalledWith('review.state'); @@ -735,7 +745,7 @@ describe('WorkflowReviewRequestRepository', () => { { state: 'closed', count: '12' }, ]); - const result = await repo.countByStateForInbox({ visibility: allVisibility }); + const result = await inboxRepository.countRequestsByState(allVisibility); expect(result).toEqual({ open: 3, closed: 12 }); expect(queryBuilder.andWhere).not.toHaveBeenCalled(); @@ -745,9 +755,9 @@ describe('WorkflowReviewRequestRepository', () => { it('counts nothing when the caller administers no project and can read none', async () => { queryBuilder.getRawMany.mockResolvedValueOnce([]); - const result = await repo.countByStateForInbox({ - visibility: involvedVisibility({ adminProjectIds: [], readableProjectIds: [] }), - }); + const result = await inboxRepository.countRequestsByState( + involvedVisibility({ adminProjectIds: [], readableProjectIds: [] }), + ); expect(result).toEqual({ open: 0, closed: 0 }); expect(queryBuilder.andWhere).toHaveBeenCalledWith('1 = 0'); diff --git a/packages/@n8n/db/src/repositories/index.ts b/packages/@n8n/db/src/repositories/index.ts index 0182d4a4e56..c9df293c847 100644 --- a/packages/@n8n/db/src/repositories/index.ts +++ b/packages/@n8n/db/src/repositories/index.ts @@ -85,10 +85,14 @@ export { export { WorkflowPublishHistoryRepository } from './workflow-publish-history.repository'; export { WorkflowReviewRequestRepository, - type InboxCursor, - type InboxVisibility, type WorkflowReviewRequestForWorkflowRow, } from './workflow-review-request.repository'; +export { WorkflowReviewLifecycleRepository } from './workflow-review-lifecycle.repository'; +export { + WorkflowReviewInboxRepository, + type InboxCursor, + type InboxVisibility, +} from './workflow-review-inbox.repository'; export { WorkflowReviewRequestWorkflowRepository, type WorkflowReviewRequestLinkedWorkflow, diff --git a/packages/@n8n/db/src/repositories/workflow-review-inbox.repository.ts b/packages/@n8n/db/src/repositories/workflow-review-inbox.repository.ts new file mode 100644 index 00000000000..82f9fbf5de8 --- /dev/null +++ b/packages/@n8n/db/src/repositories/workflow-review-inbox.repository.ts @@ -0,0 +1,224 @@ +import { Service } from '@n8n/di'; +import { DataSource, type SelectQueryBuilder } from '@n8n/typeorm'; + +import { BaseRepository } from './base-repository'; +import { SharedWorkflow } from '../entities/shared-workflow'; +import { WorkflowReviewRequestAuthor } from '../entities/workflow-review-request-author.ee'; +import { WorkflowReviewRequestReviewer } from '../entities/workflow-review-request-reviewer.ee'; +import { WorkflowReviewRequestWorkflow } from '../entities/workflow-review-request-workflow.ee'; +import { + WorkflowReviewRequest, + type WorkflowReviewRequestState, +} from '../entities/workflow-review-request.ee'; +import { TransactionRunner } from '../services/transaction'; + +/** The cursor carries its boundary values so deleting the previous page's last row is safe. */ +export type InboxCursor = { + createdAt: Date; + id: string; +}; + +/** + * Reviewers belong in `waiting`, even when they are also authors. All other authors belong in + * `authored`. The requester always has an author row, so this does not use `createdById`. + */ +type InboxCategoryFilter = { + userId: string; + category: 'waiting' | 'authored'; +}; + +export type InboxVisibility = + | { scope: 'all' } + | { + scope: 'involved'; + userId: string; + adminProjectIds: string[]; + /** Null means the user can read workflows in every project. */ + readableProjectIds: string[] | null; + readableWorkflowRoles: string[]; + }; + +type FindInboxRequestsOptions = { + visibility: InboxVisibility; + state?: WorkflowReviewRequestState; + category?: InboxCategoryFilter; + limit: number; + cursor?: InboxCursor; +}; + +export type InboxStateCounts = { + open: number; + closed: number; +}; + +function participantExistsSubquery( + queryBuilder: SelectQueryBuilder, + junctionEntity: typeof WorkflowReviewRequestAuthor | typeof WorkflowReviewRequestReviewer, + alias: string, + userParameter: string, +): string { + return queryBuilder + .subQuery() + .select('1') + .from(junctionEntity, alias) + .where(`${alias}.workflowReviewRequestId = review.id`) + .andWhere(`${alias}.userId = :${userParameter}`) + .getQuery(); +} + +function linkedWorkflowExistsSubquery( + queryBuilder: SelectQueryBuilder, + alias: string, +): string { + return queryBuilder + .subQuery() + .select('1') + .from(WorkflowReviewRequestWorkflow, alias) + .where(`${alias}.workflowReviewRequestId = review.id`) + .getQuery(); +} + +function readableLinkedWorkflowExistsSubquery( + queryBuilder: SelectQueryBuilder, +): string { + return queryBuilder + .subQuery() + .select('1') + .from(SharedWorkflow, 'visibilityShared') + .innerJoin( + WorkflowReviewRequestWorkflow, + 'visibilityLink', + 'visibilityLink.workflowId = visibilityShared.workflowId', + ) + .where('visibilityLink.workflowReviewRequestId = review.id') + .andWhere('visibilityShared.role IN (:...readableWorkflowRoles)') + .andWhere('visibilityShared.projectId IN (:...readableProjectIds)') + .getQuery(); +} + +function applyInboxVisibility( + queryBuilder: SelectQueryBuilder, + visibility: InboxVisibility, +): void { + if (visibility.scope === 'all') return; + + const { userId, adminProjectIds, readableProjectIds, readableWorkflowRoles } = visibility; + const parameters: Record = { involvedUserId: userId }; + const clauses: string[] = []; + + if (adminProjectIds.length > 0) { + clauses.push('review.projectId IN (:...adminProjectIds)'); + parameters.adminProjectIds = adminProjectIds; + } + + const authorExists = participantExistsSubquery( + queryBuilder, + WorkflowReviewRequestAuthor, + 'visibilityAuthor', + 'involvedUserId', + ); + const reviewerExists = participantExistsSubquery( + queryBuilder, + WorkflowReviewRequestReviewer, + 'visibilityReviewer', + 'involvedUserId', + ); + clauses.push(`(EXISTS ${authorExists} OR EXISTS ${reviewerExists})`); + + if (readableProjectIds === null) { + queryBuilder.andWhere(`(${clauses.join(' OR ')})`, parameters); + return; + } + + if (readableProjectIds.length === 0 || readableWorkflowRoles.length === 0) { + queryBuilder.andWhere('1 = 0'); + return; + } + + parameters.readableProjectIds = readableProjectIds; + parameters.readableWorkflowRoles = readableWorkflowRoles; + + // Use current workflow access because the request's stored project can become stale. + const anyLinkExists = linkedWorkflowExistsSubquery(queryBuilder, 'visibilityAnyLink'); + const readableLinkExists = readableLinkedWorkflowExistsSubquery(queryBuilder); + queryBuilder.andWhere( + `(${clauses.join(' OR ')}) AND (NOT EXISTS ${anyLinkExists} OR EXISTS ${readableLinkExists})`, + parameters, + ); +} + +function applyCategoryFilter( + queryBuilder: SelectQueryBuilder, + { userId, category }: InboxCategoryFilter, +): void { + const authorExists = participantExistsSubquery( + queryBuilder, + WorkflowReviewRequestAuthor, + 'author', + 'categoryUserId', + ); + const reviewerExists = participantExistsSubquery( + queryBuilder, + WorkflowReviewRequestReviewer, + 'reviewer', + 'categoryUserId', + ); + + if (category === 'authored') { + queryBuilder.andWhere(`(EXISTS ${authorExists} AND NOT EXISTS ${reviewerExists})`, { + categoryUserId: userId, + }); + return; + } + + queryBuilder.andWhere(`(EXISTS ${reviewerExists} OR NOT EXISTS ${authorExists})`, { + categoryUserId: userId, + }); +} + +@Service() +export class WorkflowReviewInboxRepository extends BaseRepository { + constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { + super(WorkflowReviewRequest, dataSource.manager, transactionRunner); + } + + async findRequests(options: FindInboxRequestsOptions): Promise { + const { visibility, state, category, limit, cursor } = options; + const queryBuilder = this.createQueryBuilder('review') + .orderBy('review.createdAt', 'DESC') + .addOrderBy('review.id', 'ASC'); + + applyInboxVisibility(queryBuilder, visibility); + if (category) applyCategoryFilter(queryBuilder, category); + if (state !== undefined) { + queryBuilder.andWhere('review.state = :state', { state }); + } + if (cursor) { + queryBuilder.andWhere( + '(review.createdAt < :createdAt OR (review.createdAt = :createdAt AND review.id > :id))', + { createdAt: cursor.createdAt, id: cursor.id }, + ); + } + + queryBuilder.take(limit); + return await queryBuilder.getMany(); + } + + async countRequestsByState(visibility: InboxVisibility): Promise { + const queryBuilder = this.createQueryBuilder('review') + .select('review.state', 'state') + .addSelect('COUNT(*)', 'count') + .groupBy('review.state'); + + applyInboxVisibility(queryBuilder, visibility); + const rows = await queryBuilder.getRawMany<{ + state: WorkflowReviewRequestState; + count: string | number; + }>(); + + return { + open: Number(rows.find((row) => row.state === 'open')?.count ?? 0), + closed: Number(rows.find((row) => row.state === 'closed')?.count ?? 0), + }; + } +} diff --git a/packages/@n8n/db/src/repositories/workflow-review-lifecycle.repository.ts b/packages/@n8n/db/src/repositories/workflow-review-lifecycle.repository.ts new file mode 100644 index 00000000000..f6f9fea6b96 --- /dev/null +++ b/packages/@n8n/db/src/repositories/workflow-review-lifecycle.repository.ts @@ -0,0 +1,147 @@ +import { Service } from '@n8n/di'; +import type { WorkflowSharingRole } from '@n8n/permissions'; +import { DataSource } from '@n8n/typeorm'; + +import { BaseRepository } from './base-repository'; +import { SharedWorkflow } from '../entities/shared-workflow'; +import { WorkflowEntity } from '../entities/workflow-entity'; +import { WorkflowReviewRequestWorkflow } from '../entities/workflow-review-request-workflow.ee'; +import { + WorkflowReviewRequest, + type WorkflowReviewRequestState, +} from '../entities/workflow-review-request.ee'; +import { type OperationContext, TransactionRunner } from '../services/transaction'; + +type OpenRequestWorkflowRow = { + requestId: string; + requestProjectId: string; + linkedWorkflowId: string | null; + /** Raw database boolean: `1` on SQLite, `true` on Postgres. */ + isArchived: boolean | number | null; + owningProjectId: string | null; +}; + +type OpenRequestAffectedByWorkflows = { + request: WorkflowReviewRequest; + links: Array<{ workflowId: string; workflowVersionId: string | null }>; +}; + +function isReviewable(row: OpenRequestWorkflowRow): boolean { + if (row.linkedWorkflowId === null) return false; + if (row.isArchived) return false; + + // A missing owner row is broken data, not evidence that the workflow moved. + return row.owningProjectId === null || row.owningProjectId === row.requestProjectId; +} + +@Service() +export class WorkflowReviewLifecycleRepository extends BaseRepository { + constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { + super(WorkflowReviewRequest, dataSource.manager, transactionRunner); + } + + /** + * Finds open requests linked to the workflows and returns only the matching links. + * Lifecycle handlers use the links to record one activity for each affected workflow. + */ + async findOpenRequestsAffectedByWorkflows( + workflowIds: string[], + ctx: OperationContext, + ): Promise { + if (workflowIds.length === 0) return []; + + const state: WorkflowReviewRequestState = 'open'; + const { entities, raw } = await this.managerFor(ctx) + .createQueryBuilder(WorkflowReviewRequest, 'request') + .innerJoin( + WorkflowReviewRequestWorkflow, + 'requestWorkflow', + 'requestWorkflow.workflowReviewRequestId = request.id', + ) + .addSelect('requestWorkflow.workflowId', 'linkedWorkflowId') + .addSelect('requestWorkflow.workflowVersionId', 'linkedWorkflowVersionId') + .where('requestWorkflow.workflowId IN (:...workflowIds)', { workflowIds }) + .andWhere('request.state = :state', { state }) + .getRawAndEntities<{ + request_id: string; + linkedWorkflowId: string; + linkedWorkflowVersionId: string | null; + }>(); + + const linksByRequestId = new Map< + string, + Array<{ workflowId: string; workflowVersionId: string | null }> + >(); + for (const row of raw) { + const links = linksByRequestId.get(row.request_id) ?? []; + links.push({ + workflowId: row.linkedWorkflowId, + workflowVersionId: row.linkedWorkflowVersionId ?? null, + }); + linksByRequestId.set(row.request_id, links); + } + + return entities.map((request) => ({ + request, + links: linksByRequestId.get(request.id) ?? [], + })); + } + + /** + * Finds open requests that have no workflow left to review. A workflow is reviewable when it + * exists, is not archived, and still belongs to the request's project. + * + * This checks current workflow state instead of the mutation that changed it. It catches links + * removed by delete cascades, mutations that bypass lifecycle hooks, and close operations that + * rolled back after the workflow mutation committed. + * + * This method only reads. The caller must hold the review mutation lock until it closes the + * returned requests with the same transaction context. + */ + async findUnreviewableOpenRequestIds( + ctx: OperationContext, + candidateRequestIds?: string[], + ): Promise { + if (candidateRequestIds?.length === 0) return []; + + const openState: WorkflowReviewRequestState = 'open'; + const ownerRole: WorkflowSharingRole = 'workflow:owner'; + const qb = this.managerFor(ctx) + .createQueryBuilder(WorkflowReviewRequest, 'review') + .select('review.id', 'requestId') + .addSelect('review.projectId', 'requestProjectId') + .addSelect('workflow.id', 'linkedWorkflowId') + .addSelect('workflow.isArchived', 'isArchived') + .addSelect('shared.projectId', 'owningProjectId') + // Keep requests whose link or workflow was removed so they can be closed. + .leftJoin(WorkflowReviewRequestWorkflow, 'link', 'link.workflowReviewRequestId = review.id') + .leftJoin(WorkflowEntity, 'workflow', 'workflow.id = link.workflowId') + .leftJoin( + SharedWorkflow, + 'shared', + 'shared.workflowId = link.workflowId AND shared.role = :ownerRole', + { ownerRole }, + ) + .where('review.state = :openState', { openState }); + + if (candidateRequestIds !== undefined) { + qb.andWhere('review.id IN (:...candidateRequestIds)', { candidateRequestIds }); + } + + const rows = await qb.getRawMany(); + const closableRequestIds = new Set(); + const requestIdsWithReviewableWorkflow = new Set(); + for (const row of rows) { + if (isReviewable(row)) { + requestIdsWithReviewableWorkflow.add(row.requestId); + } else { + closableRequestIds.add(row.requestId); + } + } + for (const requestId of requestIdsWithReviewableWorkflow) { + closableRequestIds.delete(requestId); + } + + return [...closableRequestIds]; + } +} diff --git a/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts b/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts index ef2ea0db23a..6dbff53d3ea 100644 --- a/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts @@ -1,14 +1,8 @@ import { Service } from '@n8n/di'; -import type { WorkflowSharingRole } from '@n8n/permissions'; -import type { SelectQueryBuilder } from '@n8n/typeorm'; import { DataSource } from '@n8n/typeorm'; import { BaseRepository } from './base-repository'; -import { SharedWorkflow } from '../entities/shared-workflow'; -import { WorkflowEntity } from '../entities/workflow-entity'; import { WorkflowHistory } from '../entities/workflow-history'; -import { WorkflowReviewRequestAuthor } from '../entities/workflow-review-request-author.ee'; -import { WorkflowReviewRequestReviewer } from '../entities/workflow-review-request-reviewer.ee'; import { WorkflowReviewRequestWorkflow } from '../entities/workflow-review-request-workflow.ee'; import { WorkflowReviewRequest, @@ -17,69 +11,6 @@ import { } from '../entities/workflow-review-request.ee'; import { type OperationContext, TransactionRunner } from '../services/transaction'; -/** - * Keyset pagination boundary. The caller carries `createdAt`/`id` in the cursor - * itself so pagination never depends on the anchor row still existing. - */ -export type InboxCursor = { - createdAt: Date; - id: string; -}; - -/** - * Splits the visible reviews into the two groups a caller can ask for: - * `waiting` = assigned reviewer, or not an author; `authored` = an author who is - * not assigned to review it. Being a reviewer wins, so a review sits where the - * caller's pending action is. Only narrows what visibility already allowed. - * - * Authorship alone decides the split. The requester always has an author row, so - * neither group needs the nullable `createdById`. - */ -export type InboxCategoryFilter = { - userId: string; - category: 'waiting' | 'authored'; -}; - -/** - * Who may see which reviews in the inbox. - * - * `all` — every review (global admins/owners). `involved` — reviews in projects the - * caller administers, plus reviews they participate in (author or assigned - * reviewer; the requester is always an author) — in both cases only while they can - * still read one of the workflows the review covers. - */ -export type InboxVisibility = - | { scope: 'all' } - | { - scope: 'involved'; - userId: string; - /** Projects the caller administers: every review in them is visible. */ - adminProjectIds: string[]; - /** - * Projects the caller reads workflows through: their personal project, - * where directly shared workflows land, plus team projects granting - * `workflow:read`. `null` means every project — listing them all would - * bind one parameter per project on every inbox query. - */ - readableProjectIds: string[] | null; - /** Workflow sharing roles that grant `workflow:read`. */ - readableWorkflowRoles: string[]; - }; - -export type FindManyForInboxOptions = { - visibility: InboxVisibility; - state?: WorkflowReviewRequestState; - /** Omitted means no category filter. */ - category?: InboxCategoryFilter; - limit: number; - cursor?: InboxCursor; -}; - -/** - * Projection for the workflow-scoped list: the request fields the use case - * needs plus the version pinned for the workflow the query was scoped to, and - * the name that version was given. - */ export type WorkflowReviewRequestForWorkflowRow = Pick< WorkflowReviewRequest, | 'id' @@ -95,37 +26,6 @@ export type WorkflowReviewRequestForWorkflowRow = Pick< workflowVersionName: string | null; }; -export type CountByStateForInboxOptions = { - visibility: InboxVisibility; -}; - -export type InboxStateCounts = { - open: number; - closed: number; -}; - -/** One row per (open request, linked workflow); a request with no link left yields one empty row. */ -type OpenRequestWorkflowRow = { - requestId: string; - requestProjectId: string; - linkedWorkflowId: string | null; - /** Raw, so the driver's own boolean: `1` on sqlite and mysql, `true` on postgres. */ - isArchived: boolean | number | null; - owningProjectId: string | null; -}; - -/** Reviewable: the linked workflow exists, is not archived, and still belongs to the request's project. */ -function isReviewable(row: OpenRequestWorkflowRow): boolean { - // Nothing behind the link: either the request has no link row left, or it points at a - // workflow that is gone. Both mean the delete cascade got there first. - if (row.linkedWorkflowId === null) return false; - - if (row.isArchived) return false; - - // A workflow with no owning project at all is a broken row, not a move — leave it alone. - return row.owningProjectId === null || row.owningProjectId === row.requestProjectId; -} - @Service() export class WorkflowReviewRequestRepository extends BaseRepository { constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { @@ -161,10 +61,7 @@ export class WorkflowReviewRequestRepository extends BaseRepository { - if (candidateRequestIds?.length === 0) return []; - - const openState: WorkflowReviewRequestState = 'open'; - const ownerRole: WorkflowSharingRole = 'workflow:owner'; - - const qb = this.managerFor(ctx) - .createQueryBuilder(WorkflowReviewRequest, 'review') - .select('review.id', 'requestId') - .addSelect('review.projectId', 'requestProjectId') - .addSelect('workflow.id', 'linkedWorkflowId') - .addSelect('workflow.isArchived', 'isArchived') - .addSelect('shared.projectId', 'owningProjectId') - // Left joins throughout: a request with no link, or a link with no workflow, is - // precisely the orphan case, and dropping those rows would hide it. - .leftJoin(WorkflowReviewRequestWorkflow, 'link', 'link.workflowReviewRequestId = review.id') - .leftJoin(WorkflowEntity, 'workflow', 'workflow.id = link.workflowId') - .leftJoin( - SharedWorkflow, - 'shared', - 'shared.workflowId = link.workflowId AND shared.role = :ownerRole', - { ownerRole }, - ) - .where('review.state = :openState', { openState }); - - if (candidateRequestIds !== undefined) { - qb.andWhere('review.id IN (:...candidateRequestIds)', { candidateRequestIds }); - } - - const rows = await qb.getRawMany(); - - // One reviewable workflow keeps the request open, however many of its siblings are gone. - const closableRequestIds = new Set(); - const requestIdsWithReviewableWorkflow = new Set(); - for (const row of rows) { - if (isReviewable(row)) { - requestIdsWithReviewableWorkflow.add(row.requestId); - } else { - closableRequestIds.add(row.requestId); - } - } - for (const requestId of requestIdsWithReviewableWorkflow) { - closableRequestIds.delete(requestId); - } - - return [...closableRequestIds]; - } - - /** - * Bulk-closes the given requests. A system close has no closing user, and the decision stays - * as-is. `updatedAt` is set explicitly because `manager.update` skips `@BeforeUpdate`. - * - * Keys off the given ids rather than a state predicate, so the caller must hold the - * review-request lock across selecting them ({@link findUnreviewableOpenRequestIds}) and - * closing them here. - */ + /** The caller selects the request IDs and holds the review mutation lock. */ async closeRequests(requestIds: string[], ctx: OperationContext): Promise { if (requestIds.length === 0) return; @@ -252,6 +77,7 @@ export class WorkflowReviewRequestRepository extends BaseRepository(); const count = await qb.getCount(); - // Raw rows are 1:1 with entities — the (requestId, workflowId) pair is unique — - // but key by id instead of index to stay independent of entity deduplication. const pinnedByRequestId = new Map< string, { workflowVersionId: string | null; workflowVersionName: string | null } @@ -350,260 +168,4 @@ export class WorkflowReviewRequestRepository extends BaseRepository; - }> - > { - if (workflowIds.length === 0) return []; - - const state: WorkflowReviewRequestState = 'open'; - - const { entities, raw } = await this.managerFor(ctx) - .createQueryBuilder(WorkflowReviewRequest, 'request') - .innerJoin( - WorkflowReviewRequestWorkflow, - 'requestWorkflow', - 'requestWorkflow.workflowReviewRequestId = request.id', - ) - .addSelect('requestWorkflow.workflowId', 'linkedWorkflowId') - .addSelect('requestWorkflow.workflowVersionId', 'linkedWorkflowVersionId') - .where('requestWorkflow.workflowId IN (:...workflowIds)', { workflowIds }) - .andWhere('request.state = :state', { state }) - .getRawAndEntities<{ - request_id: string; - linkedWorkflowId: string; - linkedWorkflowVersionId: string | null; - }>(); - - // Raw rows are per (request, workflow) pair; entities are deduplicated. - const linksByRequestId = new Map< - string, - Array<{ workflowId: string; workflowVersionId: string | null }> - >(); - for (const row of raw) { - const links = linksByRequestId.get(row.request_id) ?? []; - links.push({ - workflowId: row.linkedWorkflowId, - workflowVersionId: row.linkedWorkflowVersionId ?? null, - }); - linksByRequestId.set(row.request_id, links); - } - - return entities.map((request) => ({ - request, - links: linksByRequestId.get(request.id) ?? [], - })); - } - - async findManyForInbox(options: FindManyForInboxOptions): Promise { - const { visibility, state, category, limit, cursor } = options; - - const queryBuilder = this.createQueryBuilder('review') - .orderBy('review.createdAt', 'DESC') - .addOrderBy('review.id', 'ASC'); - - this.applyInboxVisibility(queryBuilder, visibility); - - if (category) { - this.applyCategoryFilter(queryBuilder, category); - } - - if (state !== undefined) { - queryBuilder.andWhere('review.state = :state', { state }); - } - - if (cursor) { - queryBuilder.andWhere( - '(review.createdAt < :createdAt OR (review.createdAt = :createdAt AND review.id > :id))', - { createdAt: cursor.createdAt, id: cursor.id }, - ); - } - - queryBuilder.take(limit); - - return await queryBuilder.getMany(); - } - - async countByStateForInbox(options: CountByStateForInboxOptions): Promise { - const queryBuilder = this.createQueryBuilder('review') - .select('review.state', 'state') - .addSelect('COUNT(*)', 'count') - .groupBy('review.state'); - - this.applyInboxVisibility(queryBuilder, options.visibility); - - const rows = await queryBuilder.getRawMany<{ - state: WorkflowReviewRequestState; - count: string | number; - }>(); - - return { - open: Number(rows.find((row) => row.state === 'open')?.count ?? 0), - closed: Number(rows.find((row) => row.state === 'closed')?.count ?? 0), - }; - } - - /** - * Inbox visibility — see {@link InboxVisibility}. A review is visible when the - * caller administers its project or takes part in it, and can still read one of - * the workflows it covers. Neither means no rows. - */ - private applyInboxVisibility( - queryBuilder: SelectQueryBuilder, - visibility: InboxVisibility, - ): void { - if (visibility.scope === 'all') { - return; - } - - const { userId, adminProjectIds, readableProjectIds, readableWorkflowRoles } = visibility; - - const parameters: Record = { involvedUserId: userId }; - const clauses: string[] = []; - - if (adminProjectIds.length > 0) { - clauses.push('review.projectId IN (:...adminProjectIds)'); - parameters.adminProjectIds = adminProjectIds; - } - - const authorExists = this.participantExistsSubquery( - queryBuilder, - WorkflowReviewRequestAuthor, - 'visibilityAuthor', - 'involvedUserId', - ); - const reviewerExists = this.participantExistsSubquery( - queryBuilder, - WorkflowReviewRequestReviewer, - 'visibilityReviewer', - 'involvedUserId', - ); - // No separate term for the requester: they always have an author row. - clauses.push(`(EXISTS ${authorExists} OR EXISTS ${reviewerExists})`); - - // This caller reads every workflow, so the check below is always true. Skip it. - if (readableProjectIds === null) { - queryBuilder.andWhere(`(${clauses.join(' OR ')})`, parameters); - return; - } - - // A caller who can read no workflow sees no reviews, admins included. - if (readableProjectIds.length === 0 || readableWorkflowRoles.length === 0) { - queryBuilder.andWhere('1 = 0'); - return; - } - - parameters.readableProjectIds = readableProjectIds; - parameters.readableWorkflowRoles = readableWorkflowRoles; - - // Check the workflows the caller can read now, not the review's stored project, - // which goes stale. Same rule as the detail gate, so a listed row always opens. - const anyLinkExists = this.linkedWorkflowExistsSubquery(queryBuilder, 'visibilityAnyLink'); - const readableLinkExists = this.readableLinkedWorkflowExistsSubquery(queryBuilder); - - queryBuilder.andWhere( - `(${clauses.join(' OR ')}) AND (NOT EXISTS ${anyLinkExists} OR EXISTS ${readableLinkExists})`, - parameters, - ); - } - - /** `EXISTS`-ready subquery: the current `review` row covers at least one workflow. */ - private linkedWorkflowExistsSubquery( - queryBuilder: SelectQueryBuilder, - alias: string, - ): string { - return queryBuilder - .subQuery() - .select('1') - .from(WorkflowReviewRequestWorkflow, alias) - .where(`${alias}.workflowReviewRequestId = review.id`) - .getQuery(); - } - - /** - * `EXISTS`-ready subquery: the caller can read one of the workflows the `review` - * row covers, looked up through current `shared_workflow` rows. - */ - private readableLinkedWorkflowExistsSubquery( - queryBuilder: SelectQueryBuilder, - ): string { - return queryBuilder - .subQuery() - .select('1') - .from(SharedWorkflow, 'visibilityShared') - .innerJoin( - WorkflowReviewRequestWorkflow, - 'visibilityLink', - 'visibilityLink.workflowId = visibilityShared.workflowId', - ) - .where('visibilityLink.workflowReviewRequestId = review.id') - .andWhere('visibilityShared.role IN (:...readableWorkflowRoles)') - .andWhere('visibilityShared.projectId IN (:...readableProjectIds)') - .getQuery(); - } - - /** - * `EXISTS`-ready subquery probing a participant junction table (authors or - * reviewers) for the current `review` row and the user bound to `userParameter`. - */ - private participantExistsSubquery( - queryBuilder: SelectQueryBuilder, - junctionEntity: typeof WorkflowReviewRequestAuthor | typeof WorkflowReviewRequestReviewer, - alias: string, - userParameter: string, - ): string { - return queryBuilder - .subQuery() - .select('1') - .from(junctionEntity, alias) - .where(`${alias}.workflowReviewRequestId = review.id`) - .andWhere(`${alias}.userId = :${userParameter}`) - .getQuery(); - } - - /** - * Narrows the visible rows to one category — see {@link InboxCategoryFilter}. - * The two predicates are opposites, so every review lands in exactly one. - * Always `andWhere`: {@link applyInboxVisibility} runs first. - */ - private applyCategoryFilter( - queryBuilder: SelectQueryBuilder, - { userId, category }: InboxCategoryFilter, - ): void { - const authorExists = this.participantExistsSubquery( - queryBuilder, - WorkflowReviewRequestAuthor, - 'author', - 'categoryUserId', - ); - const reviewerExists = this.participantExistsSubquery( - queryBuilder, - WorkflowReviewRequestReviewer, - 'reviewer', - 'categoryUserId', - ); - - if (category === 'authored') { - queryBuilder.andWhere(`(EXISTS ${authorExists} AND NOT EXISTS ${reviewerExists})`, { - categoryUserId: userId, - }); - return; - } - - queryBuilder.andWhere(`(EXISTS ${reviewerExists} OR NOT EXISTS ${authorExists})`, { - categoryUserId: userId, - }); - } } diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts index 29cbe6c6ce0..1f84786cb60 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts @@ -2,8 +2,8 @@ import type { LicenseState } from '@n8n/backend-common'; import type { User, WorkflowHistory, + WorkflowReviewInboxRepository, WorkflowReviewRequest, - WorkflowReviewRequestRepository, WorkflowReviewRequestState, WorkflowReviewRequestWorkflowDetailRow, WorkflowReviewRequestWorkflowRepository, @@ -62,7 +62,7 @@ describe('WorkflowReviewInboxService.getDetail', () => { const workflowReviewPolicyService = mock(); const authorizationService = mock(); const workflowHistoryService = mock(); - const requestRepository = mock(); + const inboxRepository = mock(); const workflowRepository = mock(); const participantResolver = mock(); const licenseState = mock(); @@ -71,7 +71,7 @@ describe('WorkflowReviewInboxService.getDetail', () => { new WorkflowReviewFeatureGate(licenseState, workflowReviewPolicyService), authorizationService, workflowHistoryService, - requestRepository, + inboxRepository, workflowRepository, participantResolver, ); @@ -173,11 +173,12 @@ describe('WorkflowReviewInboxService.getDetail', () => { // A covered workflow is removed along with the workflow itself, so a closed // review — history of a deleted workflow — can legitimately cover none it('returns a closed review with no workflows when its workflow was deleted', async () => { - requestRepository.findById.mockResolvedValue(reviewRequest({ state: 'closed' })); + mockGate([], reviewRequest({ state: 'closed' })); workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]); const detail = await service.getDetail(requester, requestId); + expect(detail.state).toBe('closed'); expect(detail.workflows).toEqual([]); }); @@ -264,14 +265,18 @@ describe('WorkflowReviewInboxService.getDetail', () => { }); it('passes empty coverage when a closed review no longer covers any workflow', async () => { - requestRepository.findById.mockResolvedValue(reviewRequest({ state: 'closed' })); + mockGate([], reviewRequest({ state: 'closed' })); workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]); await service.getDetail(requester, requestId); expect(authorizationService.resolveViewerEligibility).toHaveBeenCalledWith( requester, - expect.objectContaining({ workflowRows: [], readableWorkflowRows: [] }), + expect.objectContaining({ + request: expect.objectContaining({ state: 'closed' }), + workflowRows: [], + readableWorkflowRows: [], + }), ); }); }); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.list.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.list.service.test.ts index f97fca4527f..e8f28a369b5 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.list.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.list.service.test.ts @@ -1,7 +1,7 @@ import { LicenseState } from '@n8n/backend-common'; import { mockInstance } from '@n8n/backend-test-utils'; import type { InboxVisibility, User, WorkflowReviewRequest } from '@n8n/db'; -import { WorkflowReviewRequestRepository, WorkflowReviewRequestWorkflowRepository } from '@n8n/db'; +import { WorkflowReviewInboxRepository, WorkflowReviewRequestWorkflowRepository } from '@n8n/db'; import { mock } from 'vitest-mock-extended'; import type { WorkflowReviewAuthorizationService } from '../workflow-review-authorization.service'; @@ -19,7 +19,7 @@ describe('WorkflowReviewInboxService', () => { const workflowReviewPolicyService = mockInstance(WorkflowReviewPolicyService); const authorizationService = mock(); const workflowHistoryService = mock(); - const workflowReviewRequestRepository = mockInstance(WorkflowReviewRequestRepository); + const workflowReviewInboxRepository = mockInstance(WorkflowReviewInboxRepository); const workflowReviewRequestWorkflowRepository = mockInstance( WorkflowReviewRequestWorkflowRepository, ); @@ -47,7 +47,7 @@ describe('WorkflowReviewInboxService', () => { new WorkflowReviewFeatureGate(licenseState, workflowReviewPolicyService), authorizationService, workflowHistoryService, - workflowReviewRequestRepository, + workflowReviewInboxRepository, workflowReviewRequestWorkflowRepository, participantResolver, ); @@ -88,14 +88,14 @@ describe('WorkflowReviewInboxService', () => { updatedAt: new Date('2024-01-01T00:00:00.000Z'), }), ]; - workflowReviewRequestRepository.findManyForInbox.mockResolvedValue(rows); + workflowReviewInboxRepository.findRequests.mockResolvedValue(rows); workflowReviewRequestWorkflowRepository.findLinkedWorkflowsByRequestIds.mockResolvedValue( new Map([['req-2', { workflowName: 'Linked workflow', workflowVersionId: 'ver-2' }]]), ); const result = await service.listForInbox(user, { limit: 1 }); - expect(workflowReviewRequestRepository.findManyForInbox).toHaveBeenCalledWith({ + expect(workflowReviewInboxRepository.findRequests).toHaveBeenCalledWith({ visibility: involvedVisibility, state: 'open', limit: 2, @@ -116,7 +116,7 @@ describe('WorkflowReviewInboxService', () => { it('decodes the incoming cursor into a keyset boundary', async () => { mockVisibility(); - workflowReviewRequestRepository.findManyForInbox.mockResolvedValue([]); + workflowReviewInboxRepository.findRequests.mockResolvedValue([]); workflowReviewRequestWorkflowRepository.findLinkedWorkflowsByRequestIds.mockResolvedValue( new Map(), ); @@ -124,7 +124,7 @@ describe('WorkflowReviewInboxService', () => { await service.listForInbox(user, { limit: 15, cursor }); - expect(workflowReviewRequestRepository.findManyForInbox).toHaveBeenCalledWith( + expect(workflowReviewInboxRepository.findRequests).toHaveBeenCalledWith( expect.objectContaining({ cursor: { createdAt: new Date('2024-01-02T00:00:00.000Z'), id: 'req-2' }, }), @@ -142,7 +142,7 @@ describe('WorkflowReviewInboxService', () => { describe('category', () => { beforeEach(() => { - workflowReviewRequestRepository.findManyForInbox.mockResolvedValue([]); + workflowReviewInboxRepository.findRequests.mockResolvedValue([]); workflowReviewRequestWorkflowRepository.findLinkedWorkflowsByRequestIds.mockResolvedValue( new Map(), ); @@ -155,7 +155,7 @@ describe('WorkflowReviewInboxService', () => { await service.listForInbox(user, { limit: 15, category }); - expect(workflowReviewRequestRepository.findManyForInbox).toHaveBeenCalledWith( + expect(workflowReviewInboxRepository.findRequests).toHaveBeenCalledWith( expect.objectContaining({ category: { userId: 'user-1', category } }), ); }, @@ -167,7 +167,7 @@ describe('WorkflowReviewInboxService', () => { await service.listForInbox(otherUser, { limit: 15, category: 'authored' }); - expect(workflowReviewRequestRepository.findManyForInbox).toHaveBeenCalledWith( + expect(workflowReviewInboxRepository.findRequests).toHaveBeenCalledWith( expect.objectContaining({ category: { userId: 'user-2', category: 'authored' } }), ); }); @@ -177,7 +177,7 @@ describe('WorkflowReviewInboxService', () => { await service.listForInbox(user, { limit: 15 }); - expect(workflowReviewRequestRepository.findManyForInbox).toHaveBeenCalledWith( + expect(workflowReviewInboxRepository.findRequests).toHaveBeenCalledWith( expect.objectContaining({ category: undefined }), ); }); @@ -197,7 +197,7 @@ describe('WorkflowReviewInboxService', () => { beforeEach(() => { authorizationService.resolveInboxVisibility.mockResolvedValue(involvedVisibility); - workflowReviewRequestRepository.findManyForInbox.mockResolvedValue([inboxRow]); + workflowReviewInboxRepository.findRequests.mockResolvedValue([inboxRow]); workflowReviewRequestWorkflowRepository.findLinkedWorkflowsByRequestIds.mockResolvedValue( new Map(), ); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-lifecycle.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-lifecycle.integration.test.ts index 2597a1e9fbe..e954f24b370 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-lifecycle.integration.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-lifecycle.integration.test.ts @@ -14,6 +14,7 @@ import { UserRepository, WorkflowRepository, WorkflowReviewActivityRepository, + WorkflowReviewLifecycleRepository, WorkflowReviewRequestAuthorRepository, WorkflowReviewRequestRepository, WorkflowReviewRequestWorkflowRepository, @@ -65,6 +66,7 @@ let ownerProject: Project; let ownerAgent: SuperAgentTest; let requestRepository: WorkflowReviewRequestRepository; +let lifecycleRepository: WorkflowReviewLifecycleRepository; let linkRepository: WorkflowReviewRequestWorkflowRepository; let authorRepository: WorkflowReviewRequestAuthorRepository; let activityRepository: WorkflowReviewActivityRepository; @@ -72,6 +74,7 @@ let activityRepository: WorkflowReviewActivityRepository; beforeAll(async () => { await utils.initNodeTypes(); requestRepository = Container.get(WorkflowReviewRequestRepository); + lifecycleRepository = Container.get(WorkflowReviewLifecycleRepository); linkRepository = Container.get(WorkflowReviewRequestWorkflowRepository); authorRepository = Container.get(WorkflowReviewRequestAuthorRepository); activityRepository = Container.get(WorkflowReviewActivityRepository); @@ -363,7 +366,7 @@ describe('auto-close on workflow hard delete', () => { test('a failed capture degrades to a sweep close without a cause entry', async () => { const { workflow, versionId } = await createReviewableWorkflow({ isArchived: true }); const request = await createOpenReview(workflow.id, versionId); - vi.spyOn(requestRepository, 'findOpenRequestsForWorkflows').mockRejectedValueOnce( + vi.spyOn(lifecycleRepository, 'findOpenRequestsAffectedByWorkflows').mockRejectedValueOnce( new Error('read failed'), ); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-lifecycle.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-lifecycle.service.test.ts index d0dcd9f0e93..659d5952dd8 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-lifecycle.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-lifecycle.service.test.ts @@ -6,6 +6,7 @@ import type { Transaction, WorkflowReviewActivity, WorkflowReviewActivityRepository, + WorkflowReviewLifecycleRepository, WorkflowReviewRequest, WorkflowReviewRequestRepository, WorkflowReviewRequestWorkflowRepository, @@ -21,6 +22,7 @@ import { WorkflowReviewStateNotifier } from '../workflow-review-state-notifier.s describe('WorkflowReviewLifecycleService', () => { const logger = mock(); const requestRepository = mock(); + const lifecycleRepository = mock(); const requestWorkflowRepository = mock(); const activityRepository = mock(); const dbLockService = mock(); @@ -46,6 +48,7 @@ describe('WorkflowReviewLifecycleService', () => { service = new WorkflowReviewLifecycleService( logger, requestRepository, + lifecycleRepository, requestWorkflowRepository, activityRepository, dbLockService, @@ -54,7 +57,7 @@ describe('WorkflowReviewLifecycleService', () => { ); dbLockService.withLockContext.mockImplementation(async (_id, fn) => await fn(ctx)); requestRepository.closeRequests.mockResolvedValue(undefined); - requestRepository.findUnreviewableOpenRequestIds.mockImplementation( + lifecycleRepository.findUnreviewableOpenRequestIds.mockImplementation( async (_ctx, candidateRequestIds) => candidateRequestIds ?? [], ); activityRepository.createActivity.mockResolvedValue(mock()); @@ -64,7 +67,7 @@ describe('WorkflowReviewLifecycleService', () => { describe('archive', () => { it('records the cause entry and the close entry together, in the lock transaction', async () => { const request = openRequest(); - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([ { request, links: [{ workflowId: 'wf-1', workflowVersionId: 'wfv-1' }] }, ]); @@ -74,7 +77,10 @@ describe('WorkflowReviewLifecycleService', () => { DbLock.WORKFLOW_REVIEW_MUTATION, expect.any(Function), ); - expect(requestRepository.findOpenRequestsForWorkflows).toHaveBeenCalledWith(['wf-1'], ctx); + expect(lifecycleRepository.findOpenRequestsAffectedByWorkflows).toHaveBeenCalledWith( + ['wf-1'], + ctx, + ); expect(activityRepository.createActivity).toHaveBeenCalledWith( { workflowReviewRequestId: 'req-1', @@ -94,7 +100,9 @@ describe('WorkflowReviewLifecycleService', () => { ctx, ); // Evaluate the linked request, then close it by ID. - expect(requestRepository.findUnreviewableOpenRequestIds).toHaveBeenCalledWith(ctx, ['req-1']); + expect(lifecycleRepository.findUnreviewableOpenRequestIds).toHaveBeenCalledWith(ctx, [ + 'req-1', + ]); expect(requestRepository.closeRequests).toHaveBeenCalledWith(['req-1'], ctx); expect( collaborationService.broadcastWorkflowReviewStateChanged, @@ -106,7 +114,7 @@ describe('WorkflowReviewLifecycleService', () => { }); it('attributes a system archive (no user) as a system actor', async () => { - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([ { request: openRequest(), links: [{ workflowId: 'wf-1', workflowVersionId: 'wfv-1' }], @@ -130,15 +138,17 @@ describe('WorkflowReviewLifecycleService', () => { }); it('leaves the request open while a reviewable workflow remains outside the affected set', async () => { - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([ { request: openRequest(), links: [{ workflowId: 'wf-1', workflowVersionId: 'wfv-1' }] }, ]); // Keep the request open while it has a reviewable workflow. - requestRepository.findUnreviewableOpenRequestIds.mockResolvedValue([]); + lifecycleRepository.findUnreviewableOpenRequestIds.mockResolvedValue([]); await service.afterWorkflowArchived('wf-1', 'user-9'); - expect(requestRepository.findUnreviewableOpenRequestIds).toHaveBeenCalledWith(ctx, ['req-1']); + expect(lifecycleRepository.findUnreviewableOpenRequestIds).toHaveBeenCalledWith(ctx, [ + 'req-1', + ]); // The cause entry is still recorded; only the close is withheld. expect(activityRepository.createActivity).toHaveBeenCalledExactlyOnceWith( expect.objectContaining({ type: 'workflow.archived' }), @@ -149,7 +159,7 @@ describe('WorkflowReviewLifecycleService', () => { }); it('does nothing when no open request is linked — no write, no broadcast', async () => { - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([]); + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([]); await service.afterWorkflowArchived('wf-1', 'user-9'); @@ -160,7 +170,9 @@ describe('WorkflowReviewLifecycleService', () => { }); it('swallows and logs repository errors instead of failing the workflow mutation', async () => { - requestRepository.findOpenRequestsForWorkflows.mockRejectedValue(new Error('db down')); + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockRejectedValue( + new Error('db down'), + ); await expect(service.afterWorkflowArchived('wf-1', 'user-9')).resolves.toBeUndefined(); @@ -170,11 +182,11 @@ describe('WorkflowReviewLifecycleService', () => { // Reporting happens after commit, so listener failures cannot undo the archive. it('archives anyway when reporting the close throws, and still runs the sweep', async () => { - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([ { request: openRequest(), links: [{ workflowId: 'wf-1', workflowVersionId: 'wfv-1' }] }, ]); // Reconciliation also finds req-9. - requestRepository.findUnreviewableOpenRequestIds.mockImplementation( + lifecycleRepository.findUnreviewableOpenRequestIds.mockImplementation( async (_ctx, ids) => ids ?? ['req-9'], ); eventService.emit.mockImplementation(() => { @@ -193,7 +205,7 @@ describe('WorkflowReviewLifecycleService', () => { it('records workflow.moved for each open request and broadcasts once per affected workflow', async () => { const first = openRequest({ id: 'req-1' }); const second = openRequest({ id: 'req-2' }); - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([ { request: first, links: [ @@ -206,7 +218,7 @@ describe('WorkflowReviewLifecycleService', () => { await service.afterWorkflowsTransferred(['wf-1', 'wf-2', 'wf-3'], 'user-9'); - expect(requestRepository.findOpenRequestsForWorkflows).toHaveBeenCalledWith( + expect(lifecycleRepository.findOpenRequestsAffectedByWorkflows).toHaveBeenCalledWith( ['wf-1', 'wf-2', 'wf-3'], ctx, ); @@ -238,7 +250,9 @@ describe('WorkflowReviewLifecycleService', () => { }); it('swallows repository errors on transfer too — the move already committed', async () => { - requestRepository.findOpenRequestsForWorkflows.mockRejectedValue(new Error('db down')); + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockRejectedValue( + new Error('db down'), + ); await expect(service.afterWorkflowsTransferred(['wf-1'], 'user-9')).resolves.toBeUndefined(); @@ -248,7 +262,7 @@ describe('WorkflowReviewLifecycleService', () => { describe('delete', () => { it('captures before the delete without writing anything', async () => { - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([ { request: openRequest(), links: [{ workflowId: 'wf-1', workflowVersionId: 'wfv-1' }], @@ -257,7 +271,10 @@ describe('WorkflowReviewLifecycleService', () => { await service.beforeWorkflowDeleted('wf-1', 'user-9'); - expect(requestRepository.findOpenRequestsForWorkflows).toHaveBeenCalledWith(['wf-1'], {}); + expect(lifecycleRepository.findOpenRequestsAffectedByWorkflows).toHaveBeenCalledWith( + ['wf-1'], + {}, + ); expect(activityRepository.createActivity).not.toHaveBeenCalled(); expect(requestRepository.closeRequests).not.toHaveBeenCalled(); expect(dbLockService.withLockContext).not.toHaveBeenCalled(); @@ -265,7 +282,9 @@ describe('WorkflowReviewLifecycleService', () => { // Review bookkeeping must not block deletion. Reconciliation handles missed captures. it('never throws from the capture, even when the repository fails', async () => { - requestRepository.findOpenRequestsForWorkflows.mockRejectedValue(new Error('db down')); + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockRejectedValue( + new Error('db down'), + ); await expect(service.beforeWorkflowDeleted('wf-1', 'user-9')).resolves.toBeUndefined(); @@ -274,7 +293,7 @@ describe('WorkflowReviewLifecycleService', () => { it('records the captured deletion and closes, after the delete committed', async () => { const request = openRequest(); - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([ { request, links: [{ workflowId: 'wf-1', workflowVersionId: 'wfv-1' }] }, ]); await service.beforeWorkflowDeleted('wf-1', 'user-9'); @@ -307,9 +326,11 @@ describe('WorkflowReviewLifecycleService', () => { it('evaluates a batch as one affected set: per-workflow cause entries, one close', async () => { const request = openRequest(); - requestRepository.findOpenRequestsForWorkflows.mockImplementation(async ([workflowId]) => [ - { request, links: [{ workflowId, workflowVersionId: `wfv-${workflowId}` }] }, - ]); + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockImplementation( + async ([workflowId]) => [ + { request, links: [{ workflowId, workflowVersionId: `wfv-${workflowId}` }] }, + ], + ); await service.beforeWorkflowDeleted('wf-1', 'user-9'); await service.beforeWorkflowDeleted('wf-2', 'user-9'); requestRepository.findById.mockResolvedValue(request); @@ -331,7 +352,9 @@ describe('WorkflowReviewLifecycleService', () => { ctx, ); // Evaluate and close the request once. - expect(requestRepository.findUnreviewableOpenRequestIds).toHaveBeenCalledWith(ctx, ['req-1']); + expect(lifecycleRepository.findUnreviewableOpenRequestIds).toHaveBeenCalledWith(ctx, [ + 'req-1', + ]); expect( activityRepository.createActivity.mock.calls.filter( ([input]) => input.type === 'review.closed', @@ -345,7 +368,7 @@ describe('WorkflowReviewLifecycleService', () => { it('records nothing for a request that closed between capture and delete', async () => { const request = openRequest(); - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([ { request, links: [{ workflowId: 'wf-1', workflowVersionId: 'wfv-1' }] }, ]); await service.beforeWorkflowDeleted('wf-1', 'user-9'); @@ -359,7 +382,7 @@ describe('WorkflowReviewLifecycleService', () => { it('consumes the capture: a second after-hook for the same workflow records nothing', async () => { const request = openRequest(); - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([ { request, links: [{ workflowId: 'wf-1', workflowVersionId: 'wfv-1' }] }, ]); await service.beforeWorkflowDeleted('wf-1', 'user-9'); @@ -376,7 +399,7 @@ describe('WorkflowReviewLifecycleService', () => { }); it('degrades to the sweep when nothing was captured', async () => { - requestRepository.findUnreviewableOpenRequestIds.mockResolvedValue(['req-9']); + lifecycleRepository.findUnreviewableOpenRequestIds.mockResolvedValue(['req-9']); await service.afterWorkflowsDeleted(['wf-1']); @@ -394,7 +417,7 @@ describe('WorkflowReviewLifecycleService', () => { // The delete is already committed and cannot be undone here. it('swallows repository errors after a delete', async () => { - requestRepository.findUnreviewableOpenRequestIds.mockRejectedValue(new Error('db down')); + lifecycleRepository.findUnreviewableOpenRequestIds.mockRejectedValue(new Error('db down')); await expect(service.afterWorkflowsDeleted(['wf-1', 'wf-2'])).resolves.toBeUndefined(); @@ -468,9 +491,9 @@ describe('WorkflowReviewLifecycleService', () => { describe('reconciliation sweep', () => { it('closes the requests the mutation stranded and explains each of them', async () => { - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([]); + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([]); // Reconciliation finds req-9 and req-10. - requestRepository.findUnreviewableOpenRequestIds.mockImplementation( + lifecycleRepository.findUnreviewableOpenRequestIds.mockImplementation( async (_ctx, ids) => ids ?? ['req-9', 'req-10'], ); @@ -502,7 +525,7 @@ describe('WorkflowReviewLifecycleService', () => { }); it('stays quiet when nothing is left unreviewable', async () => { - requestRepository.findUnreviewableOpenRequestIds.mockResolvedValue([]); + lifecycleRepository.findUnreviewableOpenRequestIds.mockResolvedValue([]); await service.afterWorkflowsDeleted(['wf-1']); @@ -512,7 +535,7 @@ describe('WorkflowReviewLifecycleService', () => { // Activity and closing share a transaction, so both roll back on failure. it('leaves a review it cannot explain to the next sweep', async () => { - requestRepository.findUnreviewableOpenRequestIds.mockResolvedValue(['req-9']); + lifecycleRepository.findUnreviewableOpenRequestIds.mockResolvedValue(['req-9']); activityRepository.createActivity.mockRejectedValue(new Error('db down')); await expect(service.afterWorkflowsDeleted(['wf-1'])).resolves.toBeUndefined(); @@ -525,8 +548,8 @@ describe('WorkflowReviewLifecycleService', () => { // Reconciliation retries closes that rolled back after an archive or move committed. it('runs after the targeted close on archive, and again on transfer', async () => { - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([]); - requestRepository.findUnreviewableOpenRequestIds.mockImplementation( + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([]); + lifecycleRepository.findUnreviewableOpenRequestIds.mockImplementation( async (_ctx, ids) => ids ?? ['req-9'], ); @@ -542,8 +565,10 @@ describe('WorkflowReviewLifecycleService', () => { // A failed targeted close must not skip reconciliation. it('still runs when the targeted close on archive failed', async () => { - requestRepository.findOpenRequestsForWorkflows.mockRejectedValue(new Error('db down')); - requestRepository.findUnreviewableOpenRequestIds.mockImplementation( + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockRejectedValue( + new Error('db down'), + ); + lifecycleRepository.findUnreviewableOpenRequestIds.mockImplementation( async (_ctx, ids) => ids ?? ['req-9'], ); @@ -557,16 +582,16 @@ describe('WorkflowReviewLifecycleService', () => { // The pre-delete hook only captures; reconciliation waits for the delete to commit. it('does not run before a delete', async () => { - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([]); + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([]); await service.beforeWorkflowDeleted('wf-1', 'user-9'); - expect(requestRepository.findUnreviewableOpenRequestIds).not.toHaveBeenCalled(); + expect(lifecycleRepository.findUnreviewableOpenRequestIds).not.toHaveBeenCalled(); }); }); it('a failed broadcast is only warned about, never thrown', async () => { - requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([ + lifecycleRepository.findOpenRequestsAffectedByWorkflows.mockResolvedValue([ { request: openRequest(), links: [{ workflowId: 'wf-1', workflowVersionId: 'wfv-1' }], diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts index 92021cf5fdf..b0e549ca1bf 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts @@ -8,7 +8,7 @@ import type { WorkflowReviewVersionSnapshot, } from '@n8n/api-types'; import { - WorkflowReviewRequestRepository, + WorkflowReviewInboxRepository, WorkflowReviewRequestWorkflowRepository, type InboxCursor, type User, @@ -41,7 +41,7 @@ export class WorkflowReviewInboxService { private readonly featureGate: WorkflowReviewFeatureGate, private readonly authorizationService: WorkflowReviewAuthorizationService, private readonly workflowHistoryService: WorkflowHistoryService, - private readonly workflowReviewRequestRepository: WorkflowReviewRequestRepository, + private readonly workflowReviewInboxRepository: WorkflowReviewInboxRepository, private readonly workflowReviewRequestWorkflowRepository: WorkflowReviewRequestWorkflowRepository, private readonly participantResolver: WorkflowReviewParticipantResolver, ) {} @@ -54,7 +54,7 @@ export class WorkflowReviewInboxService { const visibility = await this.authorizationService.resolveInboxVisibility(user); const { limit } = query; - const rows = await this.workflowReviewRequestRepository.findManyForInbox({ + const rows = await this.workflowReviewInboxRepository.findRequests({ visibility, state: query.state ?? 'open', category: @@ -91,7 +91,7 @@ export class WorkflowReviewInboxService { await this.featureGate.assertAvailable(); const visibility = await this.authorizationService.resolveInboxVisibility(user); - return await this.workflowReviewRequestRepository.countByStateForInbox({ visibility }); + return await this.workflowReviewInboxRepository.countRequestsByState(visibility); } async getDetail( diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-lifecycle.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-lifecycle.service.ts index 48a8d8b7304..edbc17542e3 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-lifecycle.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-lifecycle.service.ts @@ -5,6 +5,7 @@ import { DbLock, DbLockService, WorkflowReviewActivityRepository, + WorkflowReviewLifecycleRepository, WorkflowReviewRequestRepository, WorkflowReviewRequestWorkflowRepository, } from '@n8n/db'; @@ -50,6 +51,7 @@ export class WorkflowReviewLifecycleService implements WorkflowMutationHooks { constructor( private readonly logger: Logger, private readonly workflowReviewRequestRepository: WorkflowReviewRequestRepository, + private readonly workflowReviewLifecycleRepository: WorkflowReviewLifecycleRepository, private readonly workflowReviewRequestWorkflowRepository: WorkflowReviewRequestWorkflowRepository, private readonly activityRepository: WorkflowReviewActivityRepository, private readonly dbLockService: DbLockService, @@ -70,10 +72,11 @@ export class WorkflowReviewLifecycleService implements WorkflowMutationHooks { /** Capture request data before deletion without writing activity or blocking the delete. */ async beforeWorkflowDeleted(workflowId: string, userId: string | null): Promise { try { - const openRequests = await this.workflowReviewRequestRepository.findOpenRequestsForWorkflows( - [workflowId], - {}, - ); + const openRequests = + await this.workflowReviewLifecycleRepository.findOpenRequestsAffectedByWorkflows( + [workflowId], + {}, + ); // Keep only the latest state captured before deletion. this.pendingDeleteCaptures.delete(workflowId); @@ -205,10 +208,11 @@ export class WorkflowReviewLifecycleService implements WorkflowMutationHooks { await this.recordCauseEventsAndClose(type, workflowIds, async (ctx) => { // Read under the lock to avoid racing a decision or version update. - const openRequests = await this.workflowReviewRequestRepository.findOpenRequestsForWorkflows( - workflowIds, - ctx, - ); + const openRequests = + await this.workflowReviewLifecycleRepository.findOpenRequestsAffectedByWorkflows( + workflowIds, + ctx, + ); const affected = new Set(); const candidateRequestIds: string[] = []; @@ -326,7 +330,7 @@ export class WorkflowReviewLifecycleService implements WorkflowMutationHooks { candidateRequestIds?: string[], ): Promise { const closableRequestIds = - await this.workflowReviewRequestRepository.findUnreviewableOpenRequestIds( + await this.workflowReviewLifecycleRepository.findUnreviewableOpenRequestIds( ctx, candidateRequestIds, );