diff --git a/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts b/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts index 076977e13c5..d39e6d804ed 100644 --- a/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts +++ b/packages/@n8n/db/src/repositories/__tests__/workflow-review-request.repository.test.ts @@ -361,17 +361,6 @@ describe('WorkflowReviewRequestRepository', () => { expect(queryBuilder.take).toHaveBeenCalledWith(15); }); - it('excludes open requests whose link rows are gone, even at global scope', async () => { - queryBuilder.getMany.mockResolvedValueOnce([]); - - await repo.findManyForInbox({ projectIds: null, requesterId: 'user-1', limit: 15 }); - - // SQL shape (state != open OR EXISTS) is asserted by the inbox integration tests - expect(queryBuilder.andWhere).toHaveBeenCalledWith(expect.any(Function), { - openState: 'open', - }); - }); - it('applies the keyset boundary carried in the cursor without an anchor lookup', async () => { const findOneSpy = vi.spyOn(repo, 'findOne'); queryBuilder.getMany.mockResolvedValueOnce([]); @@ -442,16 +431,6 @@ describe('WorkflowReviewRequestRepository', () => { ); }); - it('excludes open requests whose link rows are gone, even at global scope', async () => { - queryBuilder.getRawMany.mockResolvedValueOnce([]); - - await repo.countByStateForInbox({ projectIds: null, requesterId: 'user-1' }); - - expect(queryBuilder.andWhere).toHaveBeenCalledWith(expect.any(Function), { - openState: 'open', - }); - }); - it('defaults absent states to zero', async () => { queryBuilder.getRawMany.mockResolvedValueOnce([]); 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 b559911b68b..6a01b980f65 100644 --- a/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts @@ -262,7 +262,6 @@ export class WorkflowReviewRequestRepository extends BaseRepository): void { - const openState: WorkflowReviewRequestState = 'open'; - - queryBuilder.andWhere( - (qb) => { - const linkedWorkflowExists = qb - .subQuery() - .select('1') - .from(WorkflowReviewRequestWorkflow, 'requestWorkflow') - .where('requestWorkflow.workflowReviewRequestId = review.id') - .getQuery(); - return `(review.state != :openState OR EXISTS ${linkedWorkflowExists})`; - }, - { openState }, - ); - } } diff --git a/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts b/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts index f9e735e02e4..9ac44ccaee4 100644 --- a/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts +++ b/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts @@ -42,6 +42,7 @@ import type { DataTableSizeValidator } from '@/modules/data-table/data-table-siz import type { DataTableRepository } from '@/modules/data-table/data-table.repository'; import type { RedactionEnforcementService } from '@/modules/redaction/redaction-enforcement.service'; import type { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-history.service'; +import type { WorkflowMutationHooksProxy } from '@/workflows/workflow-mutation-hooks-proxy.service'; import type { WorkflowPublishGuardProxy } from '@/workflows/workflow-publish-guard-proxy.service'; import type { WorkflowService } from '@/workflows/workflow.service'; @@ -76,6 +77,7 @@ describe('SourceControlImportService', () => { const workflowService = mock(); const workflowHistoryService = mock(); const workflowPublishGuard = mock(); + const workflowMutationHooks = mock(); const dataTableRepository = mock(); const dataTableColumnRepository = mock(); const dataTableDDLService = mock(); @@ -127,6 +129,7 @@ describe('SourceControlImportService', () => { activeWorkflowManager, executionPersistence, workflowPublishGuard, + workflowMutationHooks, ); const globMock = fastGlob.default as unknown as Mock<(...args: string[]) => Promise>; @@ -792,6 +795,131 @@ describe('SourceControlImportService', () => { expect(workflowService.activateWorkflow).not.toHaveBeenCalled(); }); + describe('workflow mutation hooks on pull-archive', () => { + const mockUserId = 'user-id-123'; + const mockUser = Object.assign(new User(), { id: mockUserId }); + const mockWorkflowFile = '/mock/workflow1.json'; + + const remoteWorkflow = (overrides: Record = {}) => ({ + id: 'workflow1', + name: 'Workflow', + nodes: [], + connections: {}, + versionId: 'v2', + parentFolderId: null, + active: false, + ...overrides, + }); + + const setupPull = (options: { + existing?: Partial; + remote?: Record; + }) => { + userRepository.findOne.mockResolvedValue(mockUser); + projectRepository.getPersonalProjectForUserOrFail.mockResolvedValue( + Object.assign(new Project(), { id: 'project1', type: 'personal' }), + ); + workflowRepository.findByIds.mockResolvedValue( + options.existing + ? [ + Object.assign(new WorkflowEntity(), { + id: 'workflow1', + name: 'Workflow', + active: false, + isArchived: false, + ...options.existing, + }), + ] + : [], + ); + folderRepository.find.mockResolvedValue([]); + sharedWorkflowRepository.findWithFields.mockResolvedValue([]); + workflowRepository.upsert.mockResolvedValue({ + identifiers: [{ id: 'workflow1' }], + generatedMaps: [], + raw: [], + }); + fsReadFile.mockResolvedValue(JSON.stringify(remoteWorkflow(options.remote))); + + return [mock({ file: mockWorkflowFile, id: 'workflow1' })]; + }; + + it('should fire afterWorkflowArchived when the pull archives an existing workflow', async () => { + const candidates = setupPull({ existing: {}, remote: { isArchived: true } }); + + await service.importWorkflowFromWorkFolder(candidates, mockUserId); + + expect(workflowMutationHooks.afterWorkflowArchived).toHaveBeenCalledTimes(1); + expect(workflowMutationHooks.afterWorkflowArchived).toHaveBeenCalledWith('workflow1'); + // The hook observes a committed mutation, so it must run after the upsert + expect(workflowRepository.upsert.mock.invocationCallOrder[0]).toBeLessThan( + workflowMutationHooks.afterWorkflowArchived.mock.invocationCallOrder[0], + ); + }); + + it('should not fire for a new workflow imported already archived', async () => { + const candidates = setupPull({ remote: { isArchived: true } }); + + await service.importWorkflowFromWorkFolder(candidates, mockUserId); + + expect(workflowMutationHooks.afterWorkflowArchived).not.toHaveBeenCalled(); + }); + + it('should not fire when the workflow is already archived locally', async () => { + const candidates = setupPull({ + existing: { isArchived: true }, + remote: { isArchived: true }, + }); + + await service.importWorkflowFromWorkFolder(candidates, mockUserId); + + expect(workflowMutationHooks.afterWorkflowArchived).not.toHaveBeenCalled(); + }); + + it('should not fire for a plain update that leaves the workflow unarchived', async () => { + const candidates = setupPull({ existing: {}, remote: {} }); + + await service.importWorkflowFromWorkFolder(candidates, mockUserId); + + expect(workflowMutationHooks.afterWorkflowArchived).not.toHaveBeenCalled(); + }); + + it('should not fire when the pull unarchives a workflow', async () => { + const candidates = setupPull({ + existing: { isArchived: true }, + remote: { isArchived: false }, + }); + + await service.importWorkflowFromWorkFolder(candidates, mockUserId); + + expect(workflowMutationHooks.afterWorkflowArchived).not.toHaveBeenCalled(); + }); + + it('should not fire when the upsert fails', async () => { + const candidates = setupPull({ existing: {}, remote: { isArchived: true } }); + workflowRepository.upsert.mockResolvedValue({ + identifiers: [], + generatedMaps: [], + raw: [], + }); + + await expect(service.importWorkflowFromWorkFolder(candidates, mockUserId)).rejects.toThrow( + 'Failed to upsert workflow', + ); + + expect(workflowMutationHooks.afterWorkflowArchived).not.toHaveBeenCalled(); + }); + + it('should fire even when saving workflow history subsequently fails', async () => { + const candidates = setupPull({ existing: {}, remote: { isArchived: true } }); + workflowHistoryService.findVersion.mockRejectedValueOnce(new Error('history unavailable')); + + await service.importWorkflowFromWorkFolder(candidates, mockUserId); + + expect(workflowMutationHooks.afterWorkflowArchived).toHaveBeenCalledWith('workflow1'); + }); + }); + describe('autoPublish parameter', () => { const mockUserId = 'user-id-123'; const mockUser = Object.assign(new User(), { id: mockUserId }); @@ -2815,6 +2943,69 @@ describe('SourceControlImportService', () => { expect(executionPersistence.hardDeleteByWorkflowId).toHaveBeenCalledWith('wf-1'); expect(folderRepository.delete).toHaveBeenCalledWith({ id: In(['folder1']) }); }); + + it('should fire beforeWorkflowDeleted before trigger teardown and folder deletion', async () => { + const candidates = [mock({ id: 'folder1' })]; + const straggler = Object.assign(new WorkflowEntity(), { id: 'wf-1', active: true }); + folderRepository.getAllFolderIdsInHierarchy.mockResolvedValueOnce([]); + workflowRepository.find.mockResolvedValueOnce([straggler]); + workflowRepository.findOne.mockResolvedValueOnce(straggler); + + await service.deleteFoldersNotInWorkfolder(candidates as any); + + expect(workflowMutationHooks.beforeWorkflowDeleted).toHaveBeenCalledTimes(1); + expect(workflowMutationHooks.beforeWorkflowDeleted).toHaveBeenCalledWith('wf-1'); + // An abort must leave the workflow untouched: hook before trigger teardown + expect( + workflowMutationHooks.beforeWorkflowDeleted.mock.invocationCallOrder[0], + ).toBeLessThan(activeWorkflowManager.remove.mock.invocationCallOrder[0]); + expect( + workflowMutationHooks.beforeWorkflowDeleted.mock.invocationCallOrder[0], + ).toBeLessThan(folderRepository.delete.mock.invocationCallOrder[0]); + }); + + it('should abort folder deletion when beforeWorkflowDeleted rejects', async () => { + const candidates = [mock({ id: 'folder1', name: 'My folder' })]; + const straggler = Object.assign(new WorkflowEntity(), { id: 'wf-1', active: true }); + folderRepository.getAllFolderIdsInHierarchy.mockResolvedValueOnce([]); + workflowRepository.find.mockResolvedValueOnce([straggler]); + workflowRepository.findOne.mockResolvedValueOnce(straggler); + workflowMutationHooks.beforeWorkflowDeleted.mockRejectedValueOnce(new Error('hook failed')); + + await expect(service.deleteFoldersNotInWorkfolder(candidates as any)).rejects.toThrow( + 'Failed to delete folder(s) "My folder" (folder1) while pulling from source control: hook failed', + ); + + expect(activeWorkflowManager.remove).not.toHaveBeenCalled(); + expect(folderRepository.delete).not.toHaveBeenCalled(); + }); + + it('should fire the afterWorkflowDeleted sweep once, after the folder row delete', async () => { + const candidates = [mock({ id: 'folder1' })]; + const straggler = Object.assign(new WorkflowEntity(), { id: 'wf-1', active: false }); + folderRepository.getAllFolderIdsInHierarchy.mockResolvedValueOnce([]); + workflowRepository.find.mockResolvedValueOnce([straggler]); + workflowRepository.findOne.mockResolvedValueOnce(straggler); + + await service.deleteFoldersNotInWorkfolder(candidates as any); + + expect(workflowMutationHooks.afterWorkflowDeleted).toHaveBeenCalledTimes(1); + expect(workflowMutationHooks.afterWorkflowDeleted).toHaveBeenCalledWith('wf-1'); + // Only the row delete cascades the workflows away, so the sweep must run after it + expect( + workflowMutationHooks.afterWorkflowDeleted.mock.invocationCallOrder[0], + ).toBeGreaterThan(folderRepository.delete.mock.invocationCallOrder[0]); + }); + + it('should not fire the sweep when the deleted folders contained no workflows', async () => { + const candidates = [mock({ id: 'folder1' })]; + folderRepository.getAllFolderIdsInHierarchy.mockResolvedValueOnce([]); + workflowRepository.find.mockResolvedValueOnce([]); + + await service.deleteFoldersNotInWorkfolder(candidates as any); + + expect(workflowMutationHooks.afterWorkflowDeleted).not.toHaveBeenCalled(); + }); }); }); @@ -3376,6 +3567,111 @@ describe('SourceControlImportService', () => { expect(executionPersistence.hardDeleteByWorkflowId).toHaveBeenCalledWith('wf-inactive'); expect(projectRepository.delete).toHaveBeenCalledWith({ id: In(['project-1']) }); }); + + it('should fire beforeWorkflowDeleted for each straggler before project deletion', async () => { + const candidates = [mock({ id: 'project-1' })]; + sharedWorkflowRepository.find.mockResolvedValueOnce([ + { workflowId: 'wf-active' }, + { workflowId: 'wf-inactive' }, + ] as SharedWorkflow[]); + workflowRepository.findOne + .mockResolvedValueOnce( + Object.assign(new WorkflowEntity(), { id: 'wf-active', active: true }), + ) + .mockResolvedValueOnce( + Object.assign(new WorkflowEntity(), { id: 'wf-inactive', active: false }), + ); + + await service.deleteTeamProjectsNotInWorkfolder(candidates); + + expect(workflowMutationHooks.beforeWorkflowDeleted).toHaveBeenCalledTimes(2); + expect(workflowMutationHooks.beforeWorkflowDeleted).toHaveBeenCalledWith('wf-active'); + expect(workflowMutationHooks.beforeWorkflowDeleted).toHaveBeenCalledWith('wf-inactive'); + // An abort must leave the workflow untouched: hook before trigger teardown + expect( + workflowMutationHooks.beforeWorkflowDeleted.mock.invocationCallOrder[0], + ).toBeLessThan(activeWorkflowManager.remove.mock.invocationCallOrder[0]); + expect( + workflowMutationHooks.beforeWorkflowDeleted.mock.invocationCallOrder[0], + ).toBeLessThan(projectRepository.delete.mock.invocationCallOrder[0]); + }); + + it('should abort project deletion when beforeWorkflowDeleted rejects', async () => { + const candidates = [mock({ id: 'project-1', name: 'My project' })]; + sharedWorkflowRepository.find.mockResolvedValueOnce([ + { workflowId: 'wf-1' }, + ] as SharedWorkflow[]); + workflowRepository.findOne.mockResolvedValueOnce( + Object.assign(new WorkflowEntity(), { id: 'wf-1', active: false }), + ); + workflowMutationHooks.beforeWorkflowDeleted.mockRejectedValueOnce(new Error('hook failed')); + + await expect(service.deleteTeamProjectsNotInWorkfolder(candidates)).rejects.toThrow( + 'Failed to delete project(s) "My project" (project-1) while pulling from source control: hook failed', + ); + + expect(executionPersistence.hardDeleteByWorkflowId).not.toHaveBeenCalled(); + expect(projectRepository.delete).not.toHaveBeenCalled(); + }); + + it('should run all hooks before any teardown, so a late rejection leaves earlier workflows untouched', async () => { + const candidates = [mock({ id: 'project-1', name: 'My project' })]; + sharedWorkflowRepository.find.mockResolvedValueOnce([ + { workflowId: 'wf-1' }, + { workflowId: 'wf-2' }, + ] as SharedWorkflow[]); + workflowRepository.findOne + .mockResolvedValueOnce(Object.assign(new WorkflowEntity(), { id: 'wf-1', active: true })) + .mockResolvedValueOnce( + Object.assign(new WorkflowEntity(), { id: 'wf-2', active: false }), + ); + workflowMutationHooks.beforeWorkflowDeleted + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(new Error('hook failed')); + + await expect(service.deleteTeamProjectsNotInWorkfolder(candidates)).rejects.toThrow( + 'Failed to delete project(s) "My project" (project-1) while pulling from source control: hook failed', + ); + + // wf-1's hook already passed, but wf-2's rejection must abort before + // ANY teardown — wf-1 keeps its triggers and execution history + expect(activeWorkflowManager.remove).not.toHaveBeenCalled(); + expect(executionPersistence.hardDeleteByWorkflowId).not.toHaveBeenCalled(); + expect(projectRepository.delete).not.toHaveBeenCalled(); + }); + + it('should fire the afterWorkflowDeleted sweep once, after the project row delete', async () => { + const candidates = [mock({ id: 'project-1' })]; + sharedWorkflowRepository.find.mockResolvedValueOnce([ + { workflowId: 'wf-active' }, + { workflowId: 'wf-inactive' }, + ] as SharedWorkflow[]); + workflowRepository.findOne + .mockResolvedValueOnce( + Object.assign(new WorkflowEntity(), { id: 'wf-active', active: true }), + ) + .mockResolvedValueOnce( + Object.assign(new WorkflowEntity(), { id: 'wf-inactive', active: false }), + ); + + await service.deleteTeamProjectsNotInWorkfolder(candidates); + + // The sweep searches globally for orphaned requests, so one call covers the batch + expect(workflowMutationHooks.afterWorkflowDeleted).toHaveBeenCalledTimes(1); + expect(workflowMutationHooks.afterWorkflowDeleted).toHaveBeenCalledWith('wf-active'); + expect( + workflowMutationHooks.afterWorkflowDeleted.mock.invocationCallOrder[0], + ).toBeGreaterThan(projectRepository.delete.mock.invocationCallOrder[0]); + }); + + it('should not fire the sweep when the deleted projects contained no workflows', async () => { + const candidates = [mock({ id: 'project-1' })]; + sharedWorkflowRepository.find.mockResolvedValueOnce([]); + + await service.deleteTeamProjectsNotInWorkfolder(candidates); + + expect(workflowMutationHooks.afterWorkflowDeleted).not.toHaveBeenCalled(); + }); }); }); diff --git a/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts b/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts index 2ccd9f8f8f9..af6c6a695d4 100644 --- a/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts +++ b/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts @@ -55,6 +55,7 @@ import { TagService } from '@/services/tag.service'; import { assertNever } from '@/utils'; import { validateWorkflowNodeGroups, sanitizeNodeGroupDescriptions } from '@/workflow-helpers'; import { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-history.service'; +import { WorkflowMutationHooksProxy } from '@/workflows/workflow-mutation-hooks-proxy.service'; import { WorkflowPublishGuardProxy } from '@/workflows/workflow-publish-guard-proxy.service'; import { WorkflowService } from '@/workflows/workflow.service'; @@ -153,6 +154,7 @@ export class SourceControlImportService { private readonly activeWorkflowManager: ActiveWorkflowManager, private readonly executionPersistence: ExecutionPersistence, private readonly workflowPublishGuard: WorkflowPublishGuardProxy, + private readonly workflowMutationHooks: WorkflowMutationHooksProxy, ) { this.gitFolder = path.join(instanceSettings.n8nFolder, SOURCE_CONTROL_GIT_FOLDER); this.workflowExportFolder = path.join(this.gitFolder, SOURCE_CONTROL_WORKFLOW_EXPORT_FOLDER); @@ -848,6 +850,12 @@ export class SourceControlImportService { this.logger.debug(`Updating workflow id ${id ?? 'new'}`); + // The upsert below writes `isArchived` directly instead of going through + // `WorkflowService.archive()`, so detect the transition to run its + // side effects (e.g. closing open review requests) ourselves. + const archivedByPull = + !!existingWorkflow && !existingWorkflow.isArchived && !!importedWorkflow.isArchived; + const upsertResult = await this.workflowRepository.upsert( { ...importedWorkflow, @@ -861,6 +869,10 @@ export class SourceControlImportService { }); } + if (archivedByPull) { + await this.workflowMutationHooks.afterWorkflowArchived(id); + } + try { await this.saveOrUpdateWorkflowHistory( { id, versionId, nodes, connections, nodeGroups }, @@ -1789,13 +1801,15 @@ export class SourceControlImportService { select: ['id'], where: { parentFolder: { id: In(folderIds) } }, }); - await this.deactivateWorkflowsAndHardDeleteExecutions( + const cascadedWorkflowIds = await this.deactivateWorkflowsAndHardDeleteExecutions( workflows.map((workflow) => workflow.id), ); await this.folderRepository.delete({ id: In(candidateIds), }); + + await this.sweepAfterWorkflowCascade(cascadedWorkflowIds); } catch (error) { throw this.deletionError('folder', candidates, error); } @@ -1816,13 +1830,15 @@ export class SourceControlImportService { select: ['workflowId'], where: { projectId: In(candidateIds), role: 'workflow:owner' }, }); - await this.deactivateWorkflowsAndHardDeleteExecutions( + const cascadedWorkflowIds = await this.deactivateWorkflowsAndHardDeleteExecutions( ownedWorkflows.map((sw) => sw.workflowId), ); await this.projectRepository.delete({ id: In(candidateIds), }); + + await this.sweepAfterWorkflowCascade(cascadedWorkflowIds); } catch (error) { throw this.deletionError('project', candidates, error); } @@ -1839,29 +1855,58 @@ export class SourceControlImportService { * pulling user holds `workflow:delete` on, and the pull already ran it for * those (see `deleteWorkflowsNotInWorkfolder`). Any workflow still standing * was skipped by that permission check, yet the FK cascade below deletes it - * regardless — so we do the physical cleanup directly beforehand. Deletion - * hooks (`workflow.delete`/`workflow.afterDelete`) and the `workflow-deleted` - * event don't fire for these workflows — a pre-existing gap for any - * cascade-deleted workflow. + * regardless — so we do the physical cleanup directly beforehand. The + * `beforeWorkflowDeleted` mutation hook fires here so modules can run their + * pre-delete side effects (e.g. closing open review requests), and the + * caller fires `afterWorkflowDeleted` once the cascade has run (see + * {@link sweepAfterWorkflowCascade}) — but external hooks + * (`workflow.delete`/`workflow.afterDelete`) and the `workflow-deleted` + * event still don't fire, a pre-existing gap for any cascade-deleted + * workflow. * * REVIEW(question): should we instead extract the permission-free part of * `WorkflowService.delete` (deactivate + drain + delete row + hooks/events) - * into an internal method and call it here? That would restore hook/event - * parity, at the cost of refactoring a hot service for this edge path. + * into an internal method and call it here? That would restore full + * hook/event parity, at the cost of refactoring a hot service for this + * edge path. */ private async deactivateWorkflowsAndHardDeleteExecutions(workflowIds: string[]) { + const workflows: WorkflowEntity[] = []; for (const workflowId of workflowIds) { const workflow = await this.workflowRepository.findOne({ select: ['id', 'active'], where: { id: workflowId }, }); - if (!workflow) continue; + if (workflow) workflows.push(workflow); + } + // The hook may throw to abort the deletion, so it runs for every workflow + // before any destructive teardown — a rejection halfway through the batch + // must not leave earlier workflows deactivated with their executions gone. + for (const workflow of workflows) { + await this.workflowMutationHooks.beforeWorkflowDeleted(workflow.id); + } + + for (const workflow of workflows) { if (workflow.active) { await this.activeWorkflowManager.remove(workflow.id); } await this.executionPersistence.hardDeleteByWorkflowId(workflow.id); } + + return workflows.map((workflow) => workflow.id); + } + + /** + * Fire `afterWorkflowDeleted` once the folder/project row delete has + * cascaded the given workflows away, mirroring `WorkflowService.delete`: + * the sweep behind the hook closes review requests opened after the + * pre-delete hooks ran and now left without a workflow. It searches + * globally for such orphans, so one call covers the whole batch. + */ + private async sweepAfterWorkflowCascade(cascadedWorkflowIds: string[]) { + if (cascadedWorkflowIds.length === 0) return; + await this.workflowMutationHooks.afterWorkflowDeleted(cascadedWorkflowIds[0]); } /** Contextual error for a failed deletion during pull, so the operator learns which resource to look at. */ diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.integration.test.ts index 9d9b71a729b..bd3a891fbcb 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.integration.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.integration.test.ts @@ -1,5 +1,6 @@ process.env.N8N_ENV_FEAT_WORKFLOW_REVIEWS = 'true'; +import type { SourceControlledFile } from '@n8n/api-types'; import { createTeamProject, createWorkflow, @@ -9,18 +10,29 @@ import { } from '@n8n/backend-test-utils'; import type { Project, User } from '@n8n/db'; import { + FolderRepository, + ProjectRepository, + SharedWorkflowRepository, + UserRepository, WorkflowRepository, WorkflowReviewRequestAuthorRepository, WorkflowReviewRequestRepository, WorkflowReviewRequestWorkflowRepository, } from '@n8n/db'; import { Container } from '@n8n/di'; +import type { InstanceSettings } from 'n8n-core'; +import { readFile } from 'node:fs/promises'; import { v4 as uuid } from 'uuid'; +import { mock } from 'vitest-mock-extended'; import { ActiveWorkflowManager } from '@/active-workflow-manager'; +import { SourceControlImportService } from '@/modules/source-control.ee/source-control-import.service.ee'; import { WorkflowReviewPolicyService } from '@/services/workflow-review-policy.service'; +import { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-history.service'; +import { WorkflowMutationHooksProxy } from '@/workflows/workflow-mutation-hooks-proxy.service'; import { EnterpriseWorkflowService } from '@/workflows/workflow.service.ee'; import { WorkflowService } from '@/workflows/workflow.service'; +import { createFolder } from '@test-integration/db/folders'; import { createOwner } from '@test-integration/db/users'; import { createWorkflowHistoryItem } from '@test-integration/db/workflow-history'; import type { SuperAgentTest } from '@test-integration/types'; @@ -28,6 +40,15 @@ import * as utils from '@test-integration/utils'; mockInstance(ActiveWorkflowManager); +// `readFile` must be mocked at the module level: the source-control import service +// imports it as a named binding, which `vi.spyOn` can't intercept under Vitest. +// The default implementation passes through to the real fs. +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal(); + const mockedReadFile = vi.fn(actual.readFile); + return { ...actual, readFile: mockedReadFile, default: { ...actual, readFile: mockedReadFile } }; +}); + const testServer = utils.setupTestServer({ endpointGroups: ['workflow-reviews', 'workflows'], enabledFeatures: ['feat:workflowReviews'], @@ -64,6 +85,7 @@ beforeEach(async () => { 'WorkflowPublishHistory', 'WorkflowEntity', 'WorkflowHistory', + 'Folder', 'ProjectRelation', 'Project', 'User', @@ -230,3 +252,152 @@ describe('auto-close with the instance policy disabled', () => { expect(closed?.state).toBe('closed'); }); }); + +describe('auto-close on source-control pull', () => { + const mockFileData = new Map(); + const fsReadFile = vi.mocked(readFile); + let importService: SourceControlImportService; + + beforeAll(() => { + // Manual construction with real persistence + the real hooks proxy (whose + // provider the workflow-reviews module registered), fs and non-workflow + // dependencies mocked — same pattern as the environments integration tests. + importService = new SourceControlImportService( + mock(), // logger + mock(), // errorReporter + mock(), // variablesService + mock(), // credentialsRepository + Container.get(ProjectRepository), + mock(), // projectRelationRepository + mock(), // tagRepository + Container.get(SharedWorkflowRepository), + mock(), // sharedCredentialsRepository + Container.get(UserRepository), + mock(), // variablesRepository + Container.get(WorkflowRepository), + mock(), // workflowTagMappingRepository + mock(), // workflowService + mock(), // credentialsService + mock(), // tagService + Container.get(FolderRepository), + mock({ n8nFolder: '/mock' }), + mock(), // sourceControlContextFactory + mock(), // sourceControlScopedService + Container.get(WorkflowHistoryService), + mock(), // dataTableRepository + mock(), // dataTableColumnRepository + mock(), // dataTableDDLService + mock(), // redactionEnforcementService + mock(), // dataTableSizeValidator + mock(), // activeWorkflowManager + mock(), // executionPersistence + mock(), // workflowPublishGuard + Container.get(WorkflowMutationHooksProxy), + ); + }); + + beforeEach(() => { + mockFileData.clear(); + fsReadFile.mockImplementation(async (path) => { + const pathStr = typeof path === 'string' ? path : path.toString(); + const data = mockFileData.get(pathStr); + if (data === undefined) throw new Error(`Trying to access invalid file in test: ${pathStr}`); + return data; + }); + }); + + afterAll(() => { + // Restore the pass-through implementation given to vi.fn(actual.readFile) + fsReadFile.mockReset(); + }); + + const putWorkflowFile = (remote: { id: string }) => { + const file = `/mock/${remote.id}.json`; + mockFileData.set(file, JSON.stringify(remote)); + return mock({ id: remote.id, file }); + }; + + const remoteWorkflow = (id: string, overrides: Record = {}) => ({ + id, + name: 'Remote Workflow', + versionId: uuid(), + nodes: [], + connections: {}, + settings: {}, + parentFolderId: null, + active: false, + isArchived: false, + owner: { type: 'personal', personalEmail: owner.email }, + ...overrides, + }); + + test('a pull that archives the workflow closes the open review, decision unchanged', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await createOpenReview(workflow.id, versionId, { + decision: 'changes_requested', + }); + + const candidate = putWorkflowFile(remoteWorkflow(workflow.id, { isArchived: true })); + await importService.importWorkflowFromWorkFolder([candidate], owner.id); + + const archived = await Container.get(WorkflowRepository).findOneBy({ id: workflow.id }); + expect(archived?.isArchived).toBe(true); + + const closed = await requestRepository.findById(request.id, {}); + expect(closed?.state).toBe('closed'); + expect(closed?.decision).toBe('changes_requested'); + expect(closed?.closedById).toBeNull(); + // The publish guard keys off open requests — none left to block publishing + expect(await requestRepository.findOpenRequestForWorkflow(workflow.id, {})).toBeNull(); + }); + + test('an already-approved (closed) review is untouched by a pull-archive', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await createOpenReview(workflow.id, versionId, { + state: 'closed', + decision: 'approved', + }); + + const candidate = putWorkflowFile(remoteWorkflow(workflow.id, { isArchived: true })); + await importService.importWorkflowFromWorkFolder([candidate], owner.id); + + const untouched = await requestRepository.findById(request.id, {}); + expect(untouched?.state).toBe('closed'); + expect(untouched?.decision).toBe('approved'); + expect(untouched?.updatedAt).toEqual(request.updatedAt); + }); + + test('a pull that updates the workflow without archiving leaves the review open', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await createOpenReview(workflow.id, versionId); + + const candidate = putWorkflowFile(remoteWorkflow(workflow.id, { name: 'Updated by pull' })); + await importService.importWorkflowFromWorkFolder([candidate], owner.id); + + const updated = await Container.get(WorkflowRepository).findOneBy({ id: workflow.id }); + expect(updated?.name).toBe('Updated by pull'); + + const stillOpen = await requestRepository.findById(request.id, {}); + expect(stillOpen?.state).toBe('open'); + }); + + test('a pull that deletes a folder closes reviews of cascade-deleted workflows', async () => { + const folder = await createFolder(ownerProject, { name: 'Deleted remotely' }); + const { workflow, versionId } = await createReviewableWorkflow(); + await Container.get(WorkflowRepository).save({ id: workflow.id, parentFolder: folder }); + const request = await createOpenReview(workflow.id, versionId); + + await importService.deleteFoldersNotInWorkfolder([ + mock({ id: folder.id, name: folder.name }), + ]); + + // The workflow went with the folder cascade... + expect(await Container.get(WorkflowRepository).findOneBy({ id: workflow.id })).toBeNull(); + + // ...but its review was properly closed first, not left orphaned open + const closed = await requestRepository.findById(request.id, {}); + expect(closed?.state).toBe('closed'); + expect(closed?.closedById).toBeNull(); + expect(await linkRepository.findByRequestId(request.id, {})).toHaveLength(0); + }); +}); 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 e38a653cfc4..347573102a5 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 @@ -235,12 +235,16 @@ describe('WorkflowReviewInboxService.getDetail', () => { expect(detail.workflowVersionId).toBeNull(); }); - // An open review with no covered workflow is a dead leftover (nothing can - // decide or update it), so it is hidden — from its requester too - it('reports an open review whose workflows were all deleted as not found', async () => { + // An open review can transiently cover no workflow when a delete orphaned + // it and the sweep hasn't closed it yet — it stays readable until then + it('returns an open review with no workflows when its workflow was deleted', async () => { workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]); - await expect(service.getDetail(requester, requestId)).rejects.toThrow(NotFoundError); + const detail = await service.getDetail(requester, requestId); + + expect(detail.state).toBe('open'); + expect(detail.workflows).toEqual([]); + expect(detail.workflowName).toBeNull(); }); }); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts index b18f40f8fa3..0ffb06fdc77 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts @@ -2185,7 +2185,7 @@ describe('GET /workflow-review-requests/summary', () => { expect(response.body.data).toEqual({ open: 1, closed: 0 }); }); - test('does not count an open review orphaned by a workflow hard delete', async () => { + test('still counts an open review orphaned by a workflow hard delete until a sweep closes it', async () => { await seedInboxRequests(); const orphan = await requestRepository.createRequest( { @@ -2197,17 +2197,16 @@ describe('GET /workflow-review-requests/summary', () => { {}, ); const workflow = await linkToNewWorkflow(orphan.id); - // Bypasses the auto-close hook: the cascade removes the link row and - // leaves the request open — as the folder cascade or a create/delete race would + // Bypasses the auto-close hook and the sweep: the cascade removes the link + // row and leaves the request open — visible until the next delete sweeps it await workflowEntityRepository.delete({ id: workflow.id }); - // Owner exercises the global scope, member the project-scoped filter. - // The closed seed request has no link rows and must stay counted. + // Owner exercises the global scope, member the project-scoped filter const ownerResponse = await ownerAgent.get('/workflow-review-requests/summary').expect(200); - expect(ownerResponse.body.data).toEqual({ open: 1, closed: 1 }); + expect(ownerResponse.body.data).toEqual({ open: 2, closed: 1 }); const memberResponse = await memberAgent.get('/workflow-review-requests/summary').expect(200); - expect(memberResponse.body.data).toEqual({ open: 1, closed: 1 }); + expect(memberResponse.body.data).toEqual({ open: 2, closed: 1 }); }); test('returns 403 when feature is disabled', async () => { @@ -2247,7 +2246,7 @@ describe('GET /workflow-review-requests/inbox', () => { expect(response.body.data.hasMore).toBe(false); }); - test('omits an open review orphaned by a workflow hard delete', async () => { + test('still lists an open review orphaned by a workflow hard delete until a sweep closes it', async () => { const { openRequest } = await seedInboxRequests(); const orphan = await requestRepository.createRequest( { @@ -2259,8 +2258,8 @@ describe('GET /workflow-review-requests/inbox', () => { {}, ); const workflow = await linkToNewWorkflow(orphan.id); - // Bypasses the auto-close hook: the cascade removes the link row and - // leaves the request open — as the folder cascade or a create/delete race would + // Bypasses the auto-close hook and the sweep: the cascade removes the link + // row and leaves the request open — visible until the next delete sweeps it await workflowEntityRepository.delete({ id: workflow.id }); // Owner exercises the global scope, member the project-scoped filter @@ -2268,17 +2267,17 @@ describe('GET /workflow-review-requests/inbox', () => { .get('/workflow-review-requests/inbox') .query({ state: 'open', limit: 15 }) .expect(200); - expect(ownerResponse.body.data.data.map((row: { id: string }) => row.id)).toEqual([ - openRequest.id, - ]); + expect(ownerResponse.body.data.data.map((row: { id: string }) => row.id).sort()).toEqual( + [openRequest.id, orphan.id].sort(), + ); const memberResponse = await memberAgent .get('/workflow-review-requests/inbox') .query({ state: 'open', limit: 15 }) .expect(200); - expect(memberResponse.body.data.data.map((row: { id: string }) => row.id)).toEqual([ - openRequest.id, - ]); + expect(memberResponse.body.data.data.map((row: { id: string }) => row.id).sort()).toEqual( + [openRequest.id, orphan.id].sort(), + ); }); test('still lists a closed review whose workflow was hard-deleted', async () => { @@ -2684,17 +2683,21 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => { }); }); - test('no longer opens an open review after its workflow was hard-deleted', async () => { + test('still opens an open review after its workflow was hard-deleted', async () => { const workflow = await createWorkflow({}, teamProject); await createWorkflowHistoryItem(workflow.id, { versionId: 'version-pinned' }); const request = await seedRequest(workflow.id, 'version-pinned', owner); - // Bypasses the auto-close hook: the cascade removes the link row and - // leaves the request open — a dead leftover nothing can decide or update + // Bypasses the auto-close hook and the sweep: the cascade removes the link + // row and leaves the request open until the next delete sweeps it closed await workflowEntityRepository.delete({ id: workflow.id }); - // 404 even for the requester (owner) — their inbox no longer lists it either - await ownerAgent.get(`/workflow-review-requests/${request.id}`).expect(404); + const response = await ownerAgent.get(`/workflow-review-requests/${request.id}`).expect(200); + + expect(response.body.data.id).toBe(request.id); + expect(response.body.data.state).toBe('open'); + expect(response.body.data.workflows).toEqual([]); + expect(response.body.data.workflowName).toBeNull(); }); test('still opens a closed review after its workflow was deleted', async () => { diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-auto-close.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-auto-close.service.ts index 755a8eb3352..c3b748c4849 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-auto-close.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-auto-close.service.ts @@ -61,7 +61,7 @@ export class WorkflowReviewAutoCloseService implements WorkflowMutationHooks { }); } catch (error) { // The delete has already committed; failing it now would be worse than a - // request that stays open, which the inbox hides until the next sweep. + // request that stays open until the next sweep closes it. this.logger.error('Failed to close workflow review request(s) left without a workflow', { workflowId, error, 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 270a4058c77..9a6f03e68d1 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 @@ -136,15 +136,6 @@ export class WorkflowReviewInboxService { this.workflowReviewRequestReviewerRepository.findByRequestIds([request.id]), ]); - // An open request whose link rows all cascaded away with a workflow hard - // delete is a dead leftover: nothing can act on it and the inbox hides it, - // so hide it here too — for the requester as well, matching their inbox. - // A closed request keeps zero link rows legitimately (history of a deleted - // workflow) and stays readable. - if (request.state === 'open' && workflowRows.length === 0) { - throw new NotFoundError('Could not find review request'); - } - const readableRows = await this.filterReadableWorkflowRows(user, workflowRows); // Someone who reaches this review through its project has no reason to learn it // exists once they can read none of the workflows it covers. The requester already diff --git a/packages/cli/src/workflows/__tests__/workflow.service.test.ts b/packages/cli/src/workflows/__tests__/workflow.service.test.ts index ed7a0bc2150..a3578c47224 100644 --- a/packages/cli/src/workflows/__tests__/workflow.service.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow.service.test.ts @@ -2017,6 +2017,42 @@ describe('WorkflowService', () => { expect(updateCall?.[1]?.[2]).toEqual(expectedActor); expect(afterUpdateCall?.[1]?.[2]).toEqual(expectedActor); }); + + // Bulk import paths (e.g. the n8n-packages workflow importer) pass entities + // that may carry `isArchived` from the imported payload. Archiving must only + // happen through `archive()`, which runs its side effects (review auto-close, + // events) — so `update()` must never persist the flag. If this test breaks, + // those import paths silently gain an archive bypass. + test('does not persist isArchived from the update payload', async () => { + const workflow = mock({ + id: WORKFLOW_ID, + isArchived: false, + versionId: 'v1', + nodes: [], + connections: {}, + settings: {}, + activeVersionId: undefined as unknown as string, + tags: [], + }); + workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow); + workflowRepositoryMock.findOne.mockResolvedValue(workflow); + + const user = mock({ + id: 'user-1', + role: mock({ slug: 'global:admin' }), + }); + + await workflowService.update( + user, + { nodes: [], connections: {}, isArchived: true } as unknown as WorkflowEntity, + WORKFLOW_ID, + ); + + expect(workflowRepositoryMock.update).toHaveBeenCalledWith( + WORKFLOW_ID, + expect.not.objectContaining({ isArchived: expect.anything() }), + ); + }); }); describe('archive() and unarchive() hooks', () => { diff --git a/packages/cli/test/integration/environments/source-control-import.service.test.ts b/packages/cli/test/integration/environments/source-control-import.service.test.ts index 4f907f97e41..985e55c1527 100644 --- a/packages/cli/test/integration/environments/source-control-import.service.test.ts +++ b/packages/cli/test/integration/environments/source-control-import.service.test.ts @@ -127,6 +127,7 @@ describe('SourceControlImportService', () => { mock(), // activeWorkflowManager mock(), // executionPersistence mock(), // workflowPublishGuard + mock(), // workflowMutationHooks ); });