mirror of
https://github.com/n8n-io/n8n.git
synced 2026-08-30 18:01:23 +08:00
feat(core): Auto-close workflow review requests when a workflow is archived or moved (#35772)
This commit is contained in:
+100
-6
@@ -207,6 +207,73 @@ describe('WorkflowReviewRequestRepository', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('findOpenRequestsForWorkflows', () => {
|
||||
let queryBuilder: Mocked<SelectQueryBuilder<WorkflowReviewRequest>>;
|
||||
|
||||
beforeEach(() => {
|
||||
queryBuilder = mock<SelectQueryBuilder<WorkflowReviewRequest>>();
|
||||
queryBuilder.innerJoin.mockReturnThis();
|
||||
queryBuilder.addSelect.mockReturnThis();
|
||||
queryBuilder.where.mockReturnThis();
|
||||
queryBuilder.andWhere.mockReturnThis();
|
||||
queryBuilder.getRawAndEntities.mockResolvedValue({ entities: [], raw: [] });
|
||||
(entityManager.createQueryBuilder as Mock).mockReturnValue(queryBuilder);
|
||||
});
|
||||
|
||||
it('returns an empty list without querying when no workflow ids are given', async () => {
|
||||
const result = await repo.findOpenRequestsForWorkflows([], {});
|
||||
|
||||
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'], {});
|
||||
|
||||
expect(queryBuilder.where).toHaveBeenCalledWith(
|
||||
'requestWorkflow.workflowId IN (:...workflowIds)',
|
||||
{ workflowIds: ['workflow-1', 'workflow-2'] },
|
||||
);
|
||||
expect(queryBuilder.andWhere).toHaveBeenCalledWith('request.state = :state', {
|
||||
state: 'open',
|
||||
});
|
||||
});
|
||||
|
||||
it('maps each request to the linked workflows it was matched by', async () => {
|
||||
queryBuilder.getRawAndEntities.mockResolvedValue({
|
||||
entities: [
|
||||
mock<WorkflowReviewRequest>({ id: 'req-1' }),
|
||||
mock<WorkflowReviewRequest>({ id: 'req-2' }),
|
||||
],
|
||||
raw: [
|
||||
{ request_id: 'req-1', linkedWorkflowId: 'workflow-1' },
|
||||
{ request_id: 'req-1', linkedWorkflowId: 'workflow-2' },
|
||||
{ request_id: 'req-2', linkedWorkflowId: 'workflow-2' },
|
||||
],
|
||||
});
|
||||
|
||||
const result = await repo.findOpenRequestsForWorkflows(['workflow-1', 'workflow-2'], {});
|
||||
|
||||
expect(result).toHaveLength(2);
|
||||
expect(result[0]).toMatchObject({ workflowIds: ['workflow-1', 'workflow-2'] });
|
||||
expect(result[0].request.id).toBe('req-1');
|
||||
expect(result[1]).toMatchObject({ workflowIds: ['workflow-2'] });
|
||||
expect(result[1].request.id).toBe('req-2');
|
||||
});
|
||||
|
||||
it("reads through the context's transaction manager", async () => {
|
||||
const transactionManager = mock<EntityManager>();
|
||||
(transactionManager.createQueryBuilder as Mock).mockReturnValue(queryBuilder);
|
||||
|
||||
await repo.findOpenRequestsForWorkflows(['workflow-1'], {
|
||||
trx: new TypeOrmTransaction(transactionManager),
|
||||
});
|
||||
|
||||
expect(transactionManager.createQueryBuilder).toHaveBeenCalled();
|
||||
expect(entityManager.createQueryBuilder).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('findById', () => {
|
||||
it("reads through the context's transaction manager", async () => {
|
||||
const transactionManager = mock<EntityManager>();
|
||||
@@ -238,7 +305,7 @@ describe('WorkflowReviewRequestRepository', () => {
|
||||
|
||||
expect(result).toBe(rows);
|
||||
expect(repo.createQueryBuilder).toHaveBeenCalledWith('review');
|
||||
expect(queryBuilder.where).toHaveBeenCalledWith('review.createdById = :requesterId', {
|
||||
expect(queryBuilder.andWhere).toHaveBeenCalledWith('review.createdById = :requesterId', {
|
||||
requesterId: 'user-1',
|
||||
});
|
||||
expect(queryBuilder.take).toHaveBeenCalledWith(15);
|
||||
@@ -257,7 +324,10 @@ describe('WorkflowReviewRequestRepository', () => {
|
||||
|
||||
expect(result).toBe(rows);
|
||||
expect(repo.createQueryBuilder).toHaveBeenCalledWith('review');
|
||||
expect(queryBuilder.where).not.toHaveBeenCalled();
|
||||
expect(queryBuilder.andWhere).not.toHaveBeenCalledWith(
|
||||
expect.stringContaining('review.createdById'),
|
||||
expect.anything(),
|
||||
);
|
||||
expect(queryBuilder.orderBy).toHaveBeenCalledWith('review.createdAt', 'DESC');
|
||||
expect(queryBuilder.addOrderBy).toHaveBeenCalledWith('review.id', 'ASC');
|
||||
expect(queryBuilder.andWhere).toHaveBeenCalledWith('review.state = :state', {
|
||||
@@ -279,7 +349,7 @@ describe('WorkflowReviewRequestRepository', () => {
|
||||
|
||||
expect(result).toBe(rows);
|
||||
expect(repo.createQueryBuilder).toHaveBeenCalledWith('review');
|
||||
expect(queryBuilder.where).toHaveBeenCalledWith(
|
||||
expect(queryBuilder.andWhere).toHaveBeenCalledWith(
|
||||
'(review.projectId IN (:...projectIds) OR review.createdById = :requesterId)',
|
||||
{ projectIds: ['proj-1', 'proj-2'], requesterId: 'user-1' },
|
||||
);
|
||||
@@ -291,6 +361,17 @@ 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([]);
|
||||
@@ -322,7 +403,7 @@ describe('WorkflowReviewRequestRepository', () => {
|
||||
expect(queryBuilder.select).toHaveBeenCalledWith('review.state', 'state');
|
||||
expect(queryBuilder.addSelect).toHaveBeenCalledWith('COUNT(*)', 'count');
|
||||
expect(queryBuilder.groupBy).toHaveBeenCalledWith('review.state');
|
||||
expect(queryBuilder.where).toHaveBeenCalledWith('review.createdById = :requesterId', {
|
||||
expect(queryBuilder.andWhere).toHaveBeenCalledWith('review.createdById = :requesterId', {
|
||||
requesterId: 'user-1',
|
||||
});
|
||||
});
|
||||
@@ -336,7 +417,10 @@ describe('WorkflowReviewRequestRepository', () => {
|
||||
const result = await repo.countByStateForInbox({ projectIds: null, requesterId: 'user-1' });
|
||||
|
||||
expect(result).toEqual({ open: 3, closed: 12 });
|
||||
expect(queryBuilder.where).not.toHaveBeenCalled();
|
||||
expect(queryBuilder.andWhere).not.toHaveBeenCalledWith(
|
||||
expect.stringContaining('review.createdById'),
|
||||
expect.anything(),
|
||||
);
|
||||
expect(queryBuilder.groupBy).toHaveBeenCalledWith('review.state');
|
||||
});
|
||||
|
||||
@@ -352,12 +436,22 @@ describe('WorkflowReviewRequestRepository', () => {
|
||||
});
|
||||
|
||||
expect(result).toEqual({ open: 1, closed: 4 });
|
||||
expect(queryBuilder.where).toHaveBeenCalledWith(
|
||||
expect(queryBuilder.andWhere).toHaveBeenCalledWith(
|
||||
'(review.projectId IN (:...projectIds) OR review.createdById = :requesterId)',
|
||||
{ projectIds: ['proj-1', 'proj-2'], requesterId: 'user-1' },
|
||||
);
|
||||
});
|
||||
|
||||
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([]);
|
||||
|
||||
|
||||
@@ -5,7 +5,7 @@ import {
|
||||
type Scope,
|
||||
type WorkflowSharingRole,
|
||||
} from '@n8n/permissions';
|
||||
import { DataSource, Repository, In, Not } from '@n8n/typeorm';
|
||||
import { DataSource, In, Not } from '@n8n/typeorm';
|
||||
import type {
|
||||
EntityManager,
|
||||
FindManyOptions,
|
||||
@@ -13,11 +13,13 @@ import type {
|
||||
SelectQueryBuilder,
|
||||
} from '@n8n/typeorm';
|
||||
|
||||
import { BaseRepository } from './base-repository';
|
||||
import type { User } from '../entities';
|
||||
import { Project, ProjectRelation, SharedWorkflow } from '../entities';
|
||||
import type { OperationContext } from '../services/transaction';
|
||||
|
||||
@Service()
|
||||
export class SharedWorkflowRepository extends Repository<SharedWorkflow> {
|
||||
export class SharedWorkflowRepository extends BaseRepository<SharedWorkflow> {
|
||||
constructor(dataSource: DataSource) {
|
||||
super(SharedWorkflow, dataSource.manager);
|
||||
}
|
||||
@@ -136,9 +138,14 @@ export class SharedWorkflowRepository extends Repository<SharedWorkflow> {
|
||||
return [...new Set(projectIds)];
|
||||
}
|
||||
|
||||
async getWorkflowOwningProject(workflowId: string) {
|
||||
/**
|
||||
* Pass `ctx` when calling from inside a transaction — the read then runs on that
|
||||
* transaction's connection instead of checking out a second one, which would
|
||||
* deadlock a single-connection pool.
|
||||
*/
|
||||
async getWorkflowOwningProject(workflowId: string, ctx: OperationContext = {}) {
|
||||
return (
|
||||
await this.findOne({
|
||||
await this.managerFor(ctx).findOne(SharedWorkflow, {
|
||||
where: { workflowId, role: 'workflow:owner' },
|
||||
relations: { project: true },
|
||||
})
|
||||
|
||||
@@ -5,6 +5,8 @@ import { DiffMetaData, DiffRule, groupWorkflows, SKIP_RULES } from 'n8n-workflow
|
||||
import { WorkflowHistory, WorkflowEntity, WorkflowPublishedVersion } from '../entities';
|
||||
import { BaseRepository } from './base-repository';
|
||||
import { WorkflowPublishHistoryRepository } from './workflow-publish-history.repository';
|
||||
import { WorkflowReviewRequestWorkflow } from '../entities/workflow-review-request-workflow.ee';
|
||||
import { WorkflowReviewRequest } from '../entities/workflow-review-request.ee';
|
||||
import type { OperationContext } from '../services/transaction';
|
||||
|
||||
@Service()
|
||||
@@ -73,6 +75,18 @@ export class WorkflowHistoryRepository extends BaseRepository<WorkflowHistory> {
|
||||
.from(WorkflowPublishedVersion, 'wpv')
|
||||
.getQuery();
|
||||
|
||||
// Versions pinned by an open review request must stay reviewable and
|
||||
// publishable-on-approval. Closed reviews don't need it.
|
||||
const openReviewPinnedVersionIdsSubquery = this.manager
|
||||
.createQueryBuilder()
|
||||
.subQuery()
|
||||
.select('wrrw.workflowVersionId')
|
||||
.from(WorkflowReviewRequestWorkflow, 'wrrw')
|
||||
.innerJoin(WorkflowReviewRequest, 'wrr', 'wrr.id = wrrw.workflowReviewRequestId')
|
||||
.where("wrr.state = 'open'")
|
||||
.andWhere('wrrw.workflowVersionId IS NOT NULL')
|
||||
.getQuery();
|
||||
|
||||
const query = this.manager
|
||||
.createQueryBuilder()
|
||||
.delete()
|
||||
@@ -80,7 +94,8 @@ export class WorkflowHistoryRepository extends BaseRepository<WorkflowHistory> {
|
||||
.where('createdAt < :date', { date })
|
||||
.andWhere(`versionId NOT IN (${currentVersionIdsSubquery})`)
|
||||
.andWhere(`versionId NOT IN (${activeVersionIdsSubquery})`)
|
||||
.andWhere(`versionId NOT IN (${publishedVersionIdsSubquery})`);
|
||||
.andWhere(`versionId NOT IN (${publishedVersionIdsSubquery})`)
|
||||
.andWhere(`versionId NOT IN (${openReviewPinnedVersionIdsSubquery})`);
|
||||
|
||||
if (preserveNamedVersions) {
|
||||
query.andWhere('name IS NULL');
|
||||
|
||||
@@ -41,14 +41,6 @@ export type WorkflowReviewRequestForWorkflowRow = Pick<
|
||||
workflowVersionId: string | null;
|
||||
};
|
||||
|
||||
export type ExistsAnyForInboxOptions = {
|
||||
/** `null` means all projects (no filter); `[]` means no publish-scoped projects. */
|
||||
projectIds: string[] | null;
|
||||
/** Requesters always see the reviews they created, regardless of project scope. */
|
||||
requesterId: string;
|
||||
state?: WorkflowReviewRequestState;
|
||||
};
|
||||
|
||||
export type CountByStateForInboxOptions = {
|
||||
/** `null` means all projects (no filter); `[]` means no publish-scoped projects. */
|
||||
projectIds: string[] | null;
|
||||
@@ -107,6 +99,47 @@ export class WorkflowReviewRequestRepository extends BaseRepository<WorkflowRevi
|
||||
return await this.managerFor(ctx).save(WorkflowReviewRequest, request);
|
||||
}
|
||||
|
||||
/**
|
||||
* Closes every open request left with no linked workflow, returning the ids closed.
|
||||
*
|
||||
* A workflow hard delete cascades the link rows away, so an open request that has
|
||||
* lost its last one covers nothing and can never be acted on again. `create` writes
|
||||
* the request and its link row in one transaction, so a request is only ever visible
|
||||
* without links once the workflow behind it is gone — the two steps here cannot see
|
||||
* a half-written create and so need no lock.
|
||||
*/
|
||||
async closeOrphanedOpenRequests(ctx: OperationContext): Promise<string[]> {
|
||||
const openState: WorkflowReviewRequestState = 'open';
|
||||
const closedState: WorkflowReviewRequestState = 'closed';
|
||||
const manager = this.managerFor(ctx);
|
||||
|
||||
const orphans = await manager
|
||||
.createQueryBuilder(WorkflowReviewRequest, 'review')
|
||||
.select('review.id', 'id')
|
||||
.where('review.state = :openState', { openState })
|
||||
.andWhere((qb) => {
|
||||
const linkedWorkflowExists = qb
|
||||
.subQuery()
|
||||
.select('1')
|
||||
.from(WorkflowReviewRequestWorkflow, 'requestWorkflow')
|
||||
.where('requestWorkflow.workflowReviewRequestId = review.id')
|
||||
.getQuery();
|
||||
return `NOT EXISTS ${linkedWorkflowExists}`;
|
||||
})
|
||||
.getRawMany<{ id: string }>();
|
||||
|
||||
if (orphans.length === 0) return [];
|
||||
|
||||
const ids = orphans.map(({ id }) => id);
|
||||
// A system close has no closing user; the decision stays as-is.
|
||||
await manager.update(WorkflowReviewRequest, ids, {
|
||||
state: closedState,
|
||||
closedById: null,
|
||||
});
|
||||
|
||||
return ids;
|
||||
}
|
||||
|
||||
async findById(id: string, ctx: OperationContext): Promise<WorkflowReviewRequest | null> {
|
||||
return await this.managerFor(ctx).findOne(WorkflowReviewRequest, { where: { id } });
|
||||
}
|
||||
@@ -182,6 +215,45 @@ export class WorkflowReviewRequestRepository extends BaseRepository<WorkflowRevi
|
||||
.getOne();
|
||||
}
|
||||
|
||||
/**
|
||||
* All open requests linked to any of the given workflows, each with the
|
||||
* subset of those workflows it is linked to — so a lifecycle cleanup can
|
||||
* close a request once while still knowing which workflows were affected.
|
||||
*/
|
||||
async findOpenRequestsForWorkflows(
|
||||
workflowIds: string[],
|
||||
ctx: OperationContext,
|
||||
): Promise<Array<{ request: WorkflowReviewRequest; workflowIds: string[] }>> {
|
||||
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')
|
||||
.where('requestWorkflow.workflowId IN (:...workflowIds)', { workflowIds })
|
||||
.andWhere('request.state = :state', { state })
|
||||
.getRawAndEntities<{ request_id: string; linkedWorkflowId: string }>();
|
||||
|
||||
// Raw rows are per (request, workflow) pair; entities are deduplicated.
|
||||
const workflowIdsByRequestId = new Map<string, string[]>();
|
||||
for (const row of raw) {
|
||||
const linked = workflowIdsByRequestId.get(row.request_id) ?? [];
|
||||
linked.push(row.linkedWorkflowId);
|
||||
workflowIdsByRequestId.set(row.request_id, linked);
|
||||
}
|
||||
|
||||
return entities.map((request) => ({
|
||||
request,
|
||||
workflowIds: workflowIdsByRequestId.get(request.id) ?? [],
|
||||
}));
|
||||
}
|
||||
|
||||
async findManyForInbox(options: FindManyForInboxOptions): Promise<WorkflowReviewRequest[]> {
|
||||
const { projectIds, requesterId, state, limit, cursor } = options;
|
||||
|
||||
@@ -190,6 +262,7 @@ export class WorkflowReviewRequestRepository extends BaseRepository<WorkflowRevi
|
||||
.addOrderBy('review.id', 'ASC');
|
||||
|
||||
this.applyInboxVisibility(queryBuilder, projectIds, requesterId);
|
||||
this.excludeOpenOrphans(queryBuilder);
|
||||
|
||||
if (state !== undefined) {
|
||||
queryBuilder.andWhere('review.state = :state', { state });
|
||||
@@ -216,6 +289,7 @@ export class WorkflowReviewRequestRepository extends BaseRepository<WorkflowRevi
|
||||
.groupBy('review.state');
|
||||
|
||||
this.applyInboxVisibility(queryBuilder, projectIds, requesterId);
|
||||
this.excludeOpenOrphans(queryBuilder);
|
||||
|
||||
const rows = await queryBuilder.getRawMany<{
|
||||
state: WorkflowReviewRequestState;
|
||||
@@ -244,13 +318,38 @@ export class WorkflowReviewRequestRepository extends BaseRepository<WorkflowRevi
|
||||
}
|
||||
|
||||
if (projectIds.length === 0) {
|
||||
queryBuilder.where('review.createdById = :requesterId', { requesterId });
|
||||
queryBuilder.andWhere('review.createdById = :requesterId', { requesterId });
|
||||
return;
|
||||
}
|
||||
|
||||
queryBuilder.where(
|
||||
queryBuilder.andWhere(
|
||||
'(review.projectId IN (:...projectIds) OR review.createdById = :requesterId)',
|
||||
{ projectIds, requesterId },
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Hide open requests with no remaining link rows. A workflow hard delete
|
||||
* cascades the link rows away; when the auto-close hook is bypassed (folder
|
||||
* cascade, create/delete race) the parent is left open with nothing to act
|
||||
* on — decide and update-version 404 — so the inbox must not offer it.
|
||||
* Closed requests legitimately keep zero link rows: a hard delete closes
|
||||
* the request and preserves it as history.
|
||||
*/
|
||||
private excludeOpenOrphans(queryBuilder: SelectQueryBuilder<WorkflowReviewRequest>): 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 },
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { GlobalConfig } from '@n8n/config';
|
||||
import { Service } from '@n8n/di';
|
||||
import type { Scope } from '@n8n/permissions';
|
||||
import { DataSource, Repository, In, Like, Not, IsNull } from '@n8n/typeorm';
|
||||
import { DataSource, In, Like, Not, IsNull } from '@n8n/typeorm';
|
||||
import type {
|
||||
SelectQueryBuilder,
|
||||
UpdateResult,
|
||||
@@ -13,6 +13,7 @@ import type {
|
||||
} from '@n8n/typeorm';
|
||||
import { PROJECT_ROOT, UserError } from 'n8n-workflow';
|
||||
|
||||
import { BaseRepository } from './base-repository';
|
||||
import { FolderRepository } from './folder.repository';
|
||||
import { SharedWorkflowRepository } from './shared-workflow.repository';
|
||||
import { WorkflowHistoryRepository } from './workflow-history.repository';
|
||||
@@ -30,6 +31,7 @@ import type {
|
||||
FolderWithWorkflowAndSubFolderCount,
|
||||
ListQuery,
|
||||
} from '../entities/types-db';
|
||||
import type { OperationContext } from '../services/transaction';
|
||||
import { applyWorkflowBooleanSettingFilter } from '../utils/apply-workflow-boolean-setting-filter';
|
||||
import { isStringArray } from '../utils/is-string-array';
|
||||
import { TimedQuery } from '../utils/timed-query';
|
||||
@@ -59,7 +61,7 @@ type WorkflowListResult = {
|
||||
};
|
||||
|
||||
@Service()
|
||||
export class WorkflowRepository extends Repository<WorkflowEntity> {
|
||||
export class WorkflowRepository extends BaseRepository<WorkflowEntity> {
|
||||
constructor(
|
||||
dataSource: DataSource,
|
||||
private readonly globalConfig: GlobalConfig,
|
||||
@@ -80,6 +82,23 @@ export class WorkflowRepository extends Repository<WorkflowEntity> {
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Archived state of a workflow, or `null` if it no longer exists.
|
||||
*
|
||||
* Pass `ctx` when calling from inside a transaction — the read then runs on that
|
||||
* transaction's connection instead of checking out a second one, which would
|
||||
* deadlock a single-connection pool.
|
||||
*/
|
||||
async findArchivedState(
|
||||
workflowId: string,
|
||||
ctx: OperationContext = {},
|
||||
): Promise<{ isArchived: boolean } | null> {
|
||||
return await this.managerFor(ctx).findOne(WorkflowEntity, {
|
||||
select: { isArchived: true },
|
||||
where: { id: workflowId },
|
||||
});
|
||||
}
|
||||
|
||||
async getAllActiveIds() {
|
||||
const result = await this.find({
|
||||
select: { id: true },
|
||||
|
||||
+232
@@ -0,0 +1,232 @@
|
||||
process.env.N8N_ENV_FEAT_WORKFLOW_REVIEWS = 'true';
|
||||
|
||||
import {
|
||||
createTeamProject,
|
||||
createWorkflow,
|
||||
getPersonalProject,
|
||||
mockInstance,
|
||||
testDb,
|
||||
} from '@n8n/backend-test-utils';
|
||||
import type { Project, User } from '@n8n/db';
|
||||
import {
|
||||
WorkflowRepository,
|
||||
WorkflowReviewRequestAuthorRepository,
|
||||
WorkflowReviewRequestRepository,
|
||||
WorkflowReviewRequestWorkflowRepository,
|
||||
} from '@n8n/db';
|
||||
import { Container } from '@n8n/di';
|
||||
import { v4 as uuid } from 'uuid';
|
||||
|
||||
import { ActiveWorkflowManager } from '@/active-workflow-manager';
|
||||
import { WorkflowReviewPolicyService } from '@/services/workflow-review-policy.service';
|
||||
import { EnterpriseWorkflowService } from '@/workflows/workflow.service.ee';
|
||||
import { WorkflowService } from '@/workflows/workflow.service';
|
||||
import { createOwner } from '@test-integration/db/users';
|
||||
import { createWorkflowHistoryItem } from '@test-integration/db/workflow-history';
|
||||
import type { SuperAgentTest } from '@test-integration/types';
|
||||
import * as utils from '@test-integration/utils';
|
||||
|
||||
mockInstance(ActiveWorkflowManager);
|
||||
|
||||
const testServer = utils.setupTestServer({
|
||||
endpointGroups: ['workflow-reviews', 'workflows'],
|
||||
enabledFeatures: ['feat:workflowReviews'],
|
||||
modules: ['workflow-reviews'],
|
||||
});
|
||||
|
||||
let owner: User;
|
||||
let ownerProject: Project;
|
||||
let ownerAgent: SuperAgentTest;
|
||||
|
||||
let requestRepository: WorkflowReviewRequestRepository;
|
||||
let linkRepository: WorkflowReviewRequestWorkflowRepository;
|
||||
let authorRepository: WorkflowReviewRequestAuthorRepository;
|
||||
|
||||
beforeAll(async () => {
|
||||
await utils.initNodeTypes();
|
||||
requestRepository = Container.get(WorkflowReviewRequestRepository);
|
||||
linkRepository = Container.get(WorkflowReviewRequestWorkflowRepository);
|
||||
authorRepository = Container.get(WorkflowReviewRequestAuthorRepository);
|
||||
});
|
||||
|
||||
beforeEach(async () => {
|
||||
process.env.N8N_ENV_FEAT_WORKFLOW_REVIEWS = 'true';
|
||||
testServer.license.enable('feat:workflowReviews');
|
||||
|
||||
await testDb.truncate([
|
||||
'WorkflowReviewRequestAuthor',
|
||||
'WorkflowReviewRequestReviewer',
|
||||
'WorkflowReviewRequestWorkflow',
|
||||
'WorkflowReviewRequest',
|
||||
'SharedWorkflow',
|
||||
'WorkflowPublishedVersion',
|
||||
'WorkflowPublicationOutbox',
|
||||
'WorkflowPublishHistory',
|
||||
'WorkflowEntity',
|
||||
'WorkflowHistory',
|
||||
'ProjectRelation',
|
||||
'Project',
|
||||
'User',
|
||||
]);
|
||||
|
||||
await Container.get(WorkflowReviewPolicyService).set(true);
|
||||
|
||||
owner = await createOwner();
|
||||
ownerProject = await getPersonalProject(owner);
|
||||
ownerAgent = testServer.authAgentFor(owner);
|
||||
});
|
||||
|
||||
/** Create a workflow owned by `owner` with a pinned history version. */
|
||||
async function createReviewableWorkflow() {
|
||||
const versionId = uuid();
|
||||
const workflow = await createWorkflow({ versionId }, owner);
|
||||
await createWorkflowHistoryItem(workflow.id, { versionId });
|
||||
return { workflow, versionId };
|
||||
}
|
||||
|
||||
async function createOpenReview(
|
||||
workflowId: string,
|
||||
versionId: string,
|
||||
overrides: {
|
||||
state?: 'open' | 'closed';
|
||||
decision?: 'pending' | 'changes_requested' | 'approved';
|
||||
} = {},
|
||||
) {
|
||||
const request = await requestRepository.createRequest(
|
||||
{
|
||||
projectId: ownerProject.id,
|
||||
title: 'Review before publishing',
|
||||
createdById: owner.id,
|
||||
state: overrides.state,
|
||||
decision: overrides.decision,
|
||||
},
|
||||
{},
|
||||
);
|
||||
await linkRepository.createWorkflowRow(
|
||||
{ workflowReviewRequestId: request.id, workflowId, workflowVersionId: versionId },
|
||||
{},
|
||||
);
|
||||
await authorRepository.addAuthor({ workflowReviewRequestId: request.id, userId: owner.id }, {});
|
||||
return request;
|
||||
}
|
||||
|
||||
describe('auto-close on workflow archive', () => {
|
||||
test('archiving closes the open review, leaving the decision unchanged', async () => {
|
||||
const { workflow, versionId } = await createReviewableWorkflow();
|
||||
const request = await createOpenReview(workflow.id, versionId, {
|
||||
decision: 'changes_requested',
|
||||
});
|
||||
|
||||
await ownerAgent.post(`/workflows/${workflow.id}/archive`).expect(200);
|
||||
|
||||
const closed = await requestRepository.findById(request.id, {});
|
||||
expect(closed?.state).toBe('closed');
|
||||
expect(closed?.decision).toBe('changes_requested');
|
||||
expect(closed?.closedById).toBeNull();
|
||||
expect(closed?.approvedAt).toBeNull();
|
||||
});
|
||||
|
||||
test('unarchiving does not reopen the review, and the workflow is no longer publish-blocked', async () => {
|
||||
const { workflow, versionId } = await createReviewableWorkflow();
|
||||
const request = await createOpenReview(workflow.id, versionId);
|
||||
|
||||
await ownerAgent.post(`/workflows/${workflow.id}/archive`).expect(200);
|
||||
await ownerAgent.post(`/workflows/${workflow.id}/unarchive`).expect(200);
|
||||
|
||||
const closed = await requestRepository.findById(request.id, {});
|
||||
expect(closed?.state).toBe('closed');
|
||||
expect(await requestRepository.findOpenRequestForWorkflow(workflow.id, {})).toBeNull();
|
||||
});
|
||||
|
||||
test('an already-closed (approved) review is untouched by archiving', async () => {
|
||||
const { workflow, versionId } = await createReviewableWorkflow();
|
||||
const request = await createOpenReview(workflow.id, versionId, {
|
||||
state: 'closed',
|
||||
decision: 'approved',
|
||||
});
|
||||
|
||||
await ownerAgent.post(`/workflows/${workflow.id}/archive`).expect(200);
|
||||
|
||||
const untouched = await requestRepository.findById(request.id, {});
|
||||
expect(untouched?.state).toBe('closed');
|
||||
expect(untouched?.decision).toBe('approved');
|
||||
expect(untouched?.updatedAt).toEqual(request.updatedAt);
|
||||
});
|
||||
});
|
||||
|
||||
describe('auto-close on workflow transfer', () => {
|
||||
test('moving the workflow to another project closes the open review', async () => {
|
||||
const { workflow, versionId } = await createReviewableWorkflow();
|
||||
const request = await createOpenReview(workflow.id, versionId);
|
||||
const destination = await createTeamProject('Destination', owner);
|
||||
|
||||
await Container.get(EnterpriseWorkflowService).transferWorkflow(
|
||||
owner,
|
||||
workflow.id,
|
||||
destination.id,
|
||||
);
|
||||
|
||||
const closed = await requestRepository.findById(request.id, {});
|
||||
expect(closed?.state).toBe('closed');
|
||||
expect(closed?.decision).toBe('pending');
|
||||
expect(await requestRepository.findOpenRequestForWorkflow(workflow.id, {})).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('auto-close on workflow hard delete', () => {
|
||||
test('force-deleting a non-archived workflow closes the review instead of orphaning it open', async () => {
|
||||
const { workflow, versionId } = await createReviewableWorkflow();
|
||||
const request = await createOpenReview(workflow.id, versionId);
|
||||
|
||||
await Container.get(WorkflowService).delete(owner, workflow.id, true);
|
||||
|
||||
const closed = await requestRepository.findById(request.id, {});
|
||||
expect(closed?.state).toBe('closed');
|
||||
// The link row cascaded away with the workflow; the request itself remains, closed.
|
||||
expect(await linkRepository.findByRequestId(request.id, {})).toHaveLength(0);
|
||||
});
|
||||
|
||||
// A review opened after the pre-delete hook ran loses its link row to the cascade and
|
||||
// can no longer be found by workflow id. The post-delete sweep is what catches it.
|
||||
test('a review orphaned by a delete that skipped the hooks is closed by the next delete', async () => {
|
||||
const orphaned = await createReviewableWorkflow();
|
||||
const request = await createOpenReview(orphaned.workflow.id, orphaned.versionId);
|
||||
|
||||
// Delete the row straight from the repository, as a folder-hierarchy cascade does:
|
||||
// no hook fires, so the request is left open with its link row cascaded away.
|
||||
await Container.get(WorkflowRepository).delete(orphaned.workflow.id);
|
||||
expect((await requestRepository.findById(request.id, {}))?.state).toBe('open');
|
||||
expect(await linkRepository.findByRequestId(request.id, {})).toHaveLength(0);
|
||||
|
||||
const unrelated = await createReviewableWorkflow();
|
||||
await Container.get(WorkflowService).delete(owner, unrelated.workflow.id, true);
|
||||
|
||||
const closed = await requestRepository.findById(request.id, {});
|
||||
expect(closed?.state).toBe('closed');
|
||||
expect(closed?.closedById).toBeNull();
|
||||
});
|
||||
|
||||
test('leaves a review whose workflow still exists open', async () => {
|
||||
const live = await createReviewableWorkflow();
|
||||
const request = await createOpenReview(live.workflow.id, live.versionId);
|
||||
|
||||
const other = await createReviewableWorkflow();
|
||||
await Container.get(WorkflowService).delete(owner, other.workflow.id, true);
|
||||
|
||||
expect((await requestRepository.findById(request.id, {}))?.state).toBe('open');
|
||||
});
|
||||
});
|
||||
|
||||
describe('auto-close with the instance policy disabled', () => {
|
||||
test('cleanup still runs when the policy toggle is off', async () => {
|
||||
const { workflow, versionId } = await createReviewableWorkflow();
|
||||
const request = await createOpenReview(workflow.id, versionId);
|
||||
|
||||
await Container.get(WorkflowReviewPolicyService).set(false);
|
||||
|
||||
await ownerAgent.post(`/workflows/${workflow.id}/archive`).expect(200);
|
||||
|
||||
const closed = await requestRepository.findById(request.id, {});
|
||||
expect(closed?.state).toBe('closed');
|
||||
});
|
||||
});
|
||||
+181
@@ -0,0 +1,181 @@
|
||||
import type { Logger } from '@n8n/backend-common';
|
||||
import type {
|
||||
DbLockService,
|
||||
OperationContext,
|
||||
Transaction,
|
||||
WorkflowReviewRequest,
|
||||
WorkflowReviewRequestRepository,
|
||||
} from '@n8n/db';
|
||||
import { mock } from 'vitest-mock-extended';
|
||||
|
||||
import type { CollaborationService } from '@/collaboration/collaboration.service';
|
||||
|
||||
import { WorkflowReviewAutoCloseService } from '../workflow-review-auto-close.service';
|
||||
|
||||
describe('WorkflowReviewAutoCloseService', () => {
|
||||
const logger = mock<Logger>();
|
||||
const requestRepository = mock<WorkflowReviewRequestRepository>();
|
||||
const dbLockService = mock<DbLockService>();
|
||||
const collaborationService = mock<CollaborationService>();
|
||||
/** The lock's context. Distinct from the root `{}` so tests can tell the two apart. */
|
||||
const ctx: OperationContext = { trx: mock<Transaction>() };
|
||||
|
||||
const service = new WorkflowReviewAutoCloseService(
|
||||
logger,
|
||||
requestRepository,
|
||||
dbLockService,
|
||||
collaborationService,
|
||||
);
|
||||
|
||||
const openRequest = (overrides: Partial<WorkflowReviewRequest> = {}) =>
|
||||
mock<WorkflowReviewRequest>({
|
||||
id: 'req-1',
|
||||
state: 'open',
|
||||
decision: 'pending',
|
||||
closedById: null,
|
||||
...overrides,
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
vi.resetAllMocks();
|
||||
dbLockService.withLockContext.mockImplementation(async (_id, fn) => await fn(ctx));
|
||||
requestRepository.saveRequest.mockImplementation(async (request) => request);
|
||||
collaborationService.broadcastWorkflowReviewStateChanged.mockResolvedValue(undefined);
|
||||
});
|
||||
|
||||
it('closes an open request on archive, leaving the decision and audit fields intact', async () => {
|
||||
const request = openRequest({ decision: 'changes_requested', updatedById: 'user-2' });
|
||||
requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([
|
||||
{ request, workflowIds: ['wf-1'] },
|
||||
]);
|
||||
|
||||
await service.afterWorkflowArchived('wf-1');
|
||||
|
||||
expect(requestRepository.findOpenRequestsForWorkflows).toHaveBeenCalledWith(['wf-1'], ctx);
|
||||
expect(requestRepository.saveRequest).toHaveBeenCalledExactlyOnceWith(request, ctx);
|
||||
expect(request.state).toBe('closed');
|
||||
expect(request.decision).toBe('changes_requested');
|
||||
expect(request.closedById).toBeNull();
|
||||
expect(request.updatedById).toBe('user-2');
|
||||
expect(
|
||||
collaborationService.broadcastWorkflowReviewStateChanged,
|
||||
).toHaveBeenCalledExactlyOnceWith('wf-1');
|
||||
});
|
||||
|
||||
it('closes each open request on transfer and broadcasts once per affected workflow', async () => {
|
||||
const first = openRequest({ id: 'req-1' });
|
||||
const second = openRequest({ id: 'req-2' });
|
||||
requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([
|
||||
{ request: first, workflowIds: ['wf-1'] },
|
||||
{ request: second, workflowIds: ['wf-2'] },
|
||||
]);
|
||||
|
||||
await service.afterWorkflowsTransferred(['wf-1', 'wf-2', 'wf-3']);
|
||||
|
||||
expect(requestRepository.findOpenRequestsForWorkflows).toHaveBeenCalledWith(
|
||||
['wf-1', 'wf-2', 'wf-3'],
|
||||
ctx,
|
||||
);
|
||||
expect(first.state).toBe('closed');
|
||||
expect(second.state).toBe('closed');
|
||||
expect(collaborationService.broadcastWorkflowReviewStateChanged).toHaveBeenCalledTimes(2);
|
||||
expect(collaborationService.broadcastWorkflowReviewStateChanged).toHaveBeenCalledWith('wf-1');
|
||||
expect(collaborationService.broadcastWorkflowReviewStateChanged).toHaveBeenCalledWith('wf-2');
|
||||
});
|
||||
|
||||
it('closes an open request before its workflow is deleted', async () => {
|
||||
const request = openRequest();
|
||||
requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([
|
||||
{ request, workflowIds: ['wf-1'] },
|
||||
]);
|
||||
|
||||
await service.beforeWorkflowDeleted('wf-1');
|
||||
|
||||
expect(request.state).toBe('closed');
|
||||
});
|
||||
|
||||
it('does nothing when no open request is linked — no save, no broadcast', async () => {
|
||||
requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([]);
|
||||
|
||||
await service.afterWorkflowArchived('wf-1');
|
||||
|
||||
expect(requestRepository.saveRequest).not.toHaveBeenCalled();
|
||||
expect(collaborationService.broadcastWorkflowReviewStateChanged).not.toHaveBeenCalled();
|
||||
expect(logger.error).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('swallows and logs repository errors instead of failing the workflow mutation', async () => {
|
||||
requestRepository.findOpenRequestsForWorkflows.mockRejectedValue(new Error('db down'));
|
||||
|
||||
await expect(service.afterWorkflowArchived('wf-1')).resolves.toBeUndefined();
|
||||
|
||||
expect(logger.error).toHaveBeenCalled();
|
||||
expect(collaborationService.broadcastWorkflowReviewStateChanged).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('swallows repository errors on transfer too — the move already committed', async () => {
|
||||
requestRepository.findOpenRequestsForWorkflows.mockRejectedValue(new Error('db down'));
|
||||
|
||||
await expect(service.afterWorkflowsTransferred(['wf-1'])).resolves.toBeUndefined();
|
||||
|
||||
expect(logger.error).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// The pre-delete hook must not swallow: the link rows would cascade away and strand the
|
||||
// still-open request.
|
||||
it('rethrows repository errors before a delete, so the delete is called off', async () => {
|
||||
const error = new Error('db down');
|
||||
requestRepository.findOpenRequestsForWorkflows.mockRejectedValue(error);
|
||||
|
||||
await expect(service.beforeWorkflowDeleted('wf-1')).rejects.toThrow(error);
|
||||
|
||||
expect(logger.error).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
describe('afterWorkflowDeleted', () => {
|
||||
it('closes the requests the delete orphaned, outside any lock', async () => {
|
||||
requestRepository.closeOrphanedOpenRequests.mockResolvedValue(['req-9']);
|
||||
|
||||
await service.afterWorkflowDeleted('wf-1');
|
||||
|
||||
// A single atomic statement pair, so it needs no lock — and must not take one
|
||||
// after a delete, where camping on the create lock would serialize submissions.
|
||||
expect(requestRepository.closeOrphanedOpenRequests).toHaveBeenCalledExactlyOnceWith({});
|
||||
expect(dbLockService.withLockContext).not.toHaveBeenCalled();
|
||||
expect(logger.info).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('stays quiet when the delete orphaned nothing', async () => {
|
||||
requestRepository.closeOrphanedOpenRequests.mockResolvedValue([]);
|
||||
|
||||
await service.afterWorkflowDeleted('wf-1');
|
||||
|
||||
expect(logger.info).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// The delete already committed, so there is nothing left to abort.
|
||||
it('swallows repository errors, unlike the pre-delete hook', async () => {
|
||||
requestRepository.closeOrphanedOpenRequests.mockRejectedValue(new Error('db down'));
|
||||
|
||||
await expect(service.afterWorkflowDeleted('wf-1')).resolves.toBeUndefined();
|
||||
|
||||
expect(logger.error).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
it('a failed broadcast is only warned about, never thrown', async () => {
|
||||
requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([
|
||||
{ request: openRequest(), workflowIds: ['wf-1'] },
|
||||
]);
|
||||
collaborationService.broadcastWorkflowReviewStateChanged.mockRejectedValue(
|
||||
new Error('push down'),
|
||||
);
|
||||
|
||||
await expect(service.afterWorkflowArchived('wf-1')).resolves.toBeUndefined();
|
||||
|
||||
// Let the fire-and-forget rejection settle before asserting.
|
||||
await new Promise(process.nextTick);
|
||||
expect(logger.warn).toHaveBeenCalled();
|
||||
expect(logger.error).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
+22
-5
@@ -96,9 +96,11 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
licenseState.isWorkflowReviewsLicensed.mockReturnValue(true);
|
||||
workflowReviewPolicyService.get.mockResolvedValue({ enabled: true });
|
||||
requestRepository.findById.mockResolvedValue(reviewRequest());
|
||||
// By default the caller can still read every workflow the review covers
|
||||
// By default the review covers one workflow the caller can still read
|
||||
workflowFinderService.findWorkflowForUser.mockResolvedValue(mock<WorkflowEntity>());
|
||||
workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]);
|
||||
workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([
|
||||
{ workflowId, workflowName: 'My workflow', workflowVersionId: null },
|
||||
]);
|
||||
reviewerRepository.findByRequestIds.mockResolvedValue([]);
|
||||
userRepository.findManyByIds.mockResolvedValue([]);
|
||||
publishedVersionRepository.getPublishedVersionId.mockResolvedValue(null);
|
||||
@@ -220,14 +222,26 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
expect(detail.workflows[0]).toMatchObject({ workflowId, workflowName: 'My workflow' });
|
||||
});
|
||||
|
||||
// A covered workflow is removed along with the workflow itself, so a review can end up with none
|
||||
it('returns no workflows when the review no longer covers any', async () => {
|
||||
// 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' }));
|
||||
workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]);
|
||||
|
||||
const detail = await service.getDetail(requester, requestId);
|
||||
|
||||
expect(detail.workflows).toEqual([]);
|
||||
expect(detail.workflowName).toBeNull();
|
||||
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 () => {
|
||||
workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]);
|
||||
|
||||
await expect(service.getDetail(requester, requestId)).rejects.toThrow(NotFoundError);
|
||||
});
|
||||
});
|
||||
|
||||
describe('viewer decision eligibility', () => {
|
||||
@@ -272,7 +286,10 @@ describe('WorkflowReviewInboxService.getDetail', () => {
|
||||
expect(detail.viewerDecisionIneligibilityReason).toBe('missing_publish_permission');
|
||||
});
|
||||
|
||||
it('passes no workflow id when the review no longer covers any workflow', async () => {
|
||||
it('passes no workflow id when a closed review no longer covers any workflow', async () => {
|
||||
requestRepository.findById.mockResolvedValue(reviewRequest({ state: 'closed' }));
|
||||
workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]);
|
||||
|
||||
await service.getDetail(requester, requestId);
|
||||
|
||||
expect(decisionEligibilityService.resolveViewerEligibility).toHaveBeenCalledWith(
|
||||
|
||||
+2
@@ -15,6 +15,7 @@ import type {
|
||||
WorkflowReviewRequestReviewerRepository,
|
||||
WorkflowReviewRequestWorkflow,
|
||||
WorkflowReviewRequestWorkflowRepository,
|
||||
WorkflowRepository,
|
||||
Transaction,
|
||||
OperationContext,
|
||||
} from '@n8n/db';
|
||||
@@ -70,6 +71,7 @@ describe('WorkflowReviewRequestService.decide', () => {
|
||||
workflowFinderService,
|
||||
workflowHistoryService,
|
||||
workflowHistoryRepository,
|
||||
mock<WorkflowRepository>(),
|
||||
sharedWorkflowRepository,
|
||||
publishHistoryRepository,
|
||||
requestRepository,
|
||||
|
||||
+54
@@ -20,6 +20,7 @@ import type {
|
||||
WorkflowReviewRequestForWorkflowRow,
|
||||
WorkflowReviewRequestReviewerRepository,
|
||||
WorkflowReviewRequestWorkflowRepository,
|
||||
WorkflowRepository,
|
||||
Transaction,
|
||||
OperationContext,
|
||||
} from '@n8n/db';
|
||||
@@ -62,6 +63,8 @@ describe('WorkflowReviewRequestService', () => {
|
||||
const workflowFinderService = mock<WorkflowFinderService>();
|
||||
const workflowHistoryService = mock<WorkflowHistoryService>();
|
||||
const workflowHistoryRepository = mock<WorkflowHistoryRepository>();
|
||||
/** The `workflow_entity` repository. `workflowRepository` below is the review's link table. */
|
||||
const workflowEntityRepository = mock<WorkflowRepository>();
|
||||
const sharedWorkflowRepository = mock<SharedWorkflowRepository>();
|
||||
const publishHistoryRepository = mock<WorkflowPublishHistoryRepository>();
|
||||
const requestRepository = mock<WorkflowReviewRequestRepository>();
|
||||
@@ -85,6 +88,7 @@ describe('WorkflowReviewRequestService', () => {
|
||||
workflowFinderService,
|
||||
workflowHistoryService,
|
||||
workflowHistoryRepository,
|
||||
workflowEntityRepository,
|
||||
sharedWorkflowRepository,
|
||||
publishHistoryRepository,
|
||||
requestRepository,
|
||||
@@ -116,6 +120,8 @@ describe('WorkflowReviewRequestService', () => {
|
||||
mock<WorkflowEntity>({ isArchived: false }),
|
||||
);
|
||||
workflowHistoryService.findVersion.mockResolvedValue(mock());
|
||||
// The in-lock re-check reads archived state on the lock's own connection.
|
||||
workflowEntityRepository.findArchivedState.mockResolvedValue({ isArchived: false });
|
||||
sharedWorkflowRepository.getWorkflowOwningProject.mockResolvedValue(
|
||||
mock<Project>({ id: 'project-1' }),
|
||||
);
|
||||
@@ -144,6 +150,7 @@ describe('WorkflowReviewRequestService', () => {
|
||||
mock<WorkflowEntity>({ isArchived: false }),
|
||||
);
|
||||
workflowHistoryService.findVersion.mockResolvedValue(mock());
|
||||
workflowEntityRepository.findArchivedState.mockResolvedValue({ isArchived: false });
|
||||
sharedWorkflowRepository.getWorkflowOwningProject.mockResolvedValue(
|
||||
mock<Project>({ id: 'project-1' }),
|
||||
);
|
||||
@@ -228,6 +235,7 @@ describe('WorkflowReviewRequestService', () => {
|
||||
mock<WorkflowEntity>({ isArchived: false }),
|
||||
);
|
||||
workflowHistoryService.findVersion.mockResolvedValue(mock());
|
||||
workflowEntityRepository.findArchivedState.mockResolvedValue({ isArchived: false });
|
||||
sharedWorkflowRepository.getWorkflowOwningProject.mockResolvedValue(
|
||||
mock<Project>({ id: 'project-1' }),
|
||||
);
|
||||
@@ -244,6 +252,52 @@ describe('WorkflowReviewRequestService', () => {
|
||||
expect(authorRepository.addAuthor).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('rejects a workflow archived between the pre-lock check and the lock', async () => {
|
||||
mockSuccessfulCreatePath();
|
||||
workflowEntityRepository.findArchivedState.mockResolvedValue({ isArchived: true });
|
||||
|
||||
await expect(service.create(user, dto)).rejects.toThrow(BadRequestError);
|
||||
|
||||
expect(dbLockService.withLockContext).toHaveBeenCalled();
|
||||
expect(requestRepository.createRequest).not.toHaveBeenCalled();
|
||||
expect(workflowRepository.createWorkflowRow).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('rejects a workflow deleted between the pre-lock check and the lock', async () => {
|
||||
mockSuccessfulCreatePath();
|
||||
workflowEntityRepository.findArchivedState.mockResolvedValue(null);
|
||||
|
||||
await expect(service.create(user, dto)).rejects.toThrow(NotFoundError);
|
||||
|
||||
expect(requestRepository.createRequest).not.toHaveBeenCalled();
|
||||
expect(workflowRepository.createWorkflowRow).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// A read that checks out a second connection here deadlocks a single-connection pool.
|
||||
it('runs both in-lock re-check reads on the lock transaction', async () => {
|
||||
mockSuccessfulCreatePath();
|
||||
|
||||
await service.create(user, dto);
|
||||
|
||||
expect(workflowEntityRepository.findArchivedState).toHaveBeenCalledWith('wf-1', ctx);
|
||||
expect(sharedWorkflowRepository.getWorkflowOwningProject).toHaveBeenLastCalledWith(
|
||||
'wf-1',
|
||||
ctx,
|
||||
);
|
||||
});
|
||||
|
||||
it('rejects a workflow moved to another project between the pre-lock check and the lock', async () => {
|
||||
mockSuccessfulCreatePath();
|
||||
sharedWorkflowRepository.getWorkflowOwningProject
|
||||
.mockResolvedValueOnce(mock<Project>({ id: 'project-1' }))
|
||||
.mockResolvedValueOnce(mock<Project>({ id: 'project-2' }));
|
||||
|
||||
await expect(service.create(user, dto)).rejects.toThrow(ConflictError);
|
||||
|
||||
expect(dbLockService.withLockContext).toHaveBeenCalled();
|
||||
expect(requestRepository.createRequest).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
describe('reviewer assignment', () => {
|
||||
const mockEligibleReviewers = (...ids: string[]) => {
|
||||
roleService.rolesWithScope.mockResolvedValue(['some-role']);
|
||||
|
||||
+2
@@ -16,6 +16,7 @@ import type {
|
||||
WorkflowReviewRequestReviewerRepository,
|
||||
WorkflowReviewRequestWorkflow,
|
||||
WorkflowReviewRequestWorkflowRepository,
|
||||
WorkflowRepository,
|
||||
Transaction,
|
||||
OperationContext,
|
||||
} from '@n8n/db';
|
||||
@@ -73,6 +74,7 @@ describe('WorkflowReviewRequestService.updateVersion', () => {
|
||||
workflowFinderService,
|
||||
workflowHistoryService,
|
||||
workflowHistoryRepository,
|
||||
mock<WorkflowRepository>(),
|
||||
sharedWorkflowRepository,
|
||||
publishHistoryRepository,
|
||||
requestRepository,
|
||||
|
||||
+132
-9
@@ -2109,6 +2109,16 @@ describe('GET /workflow-review-requests', () => {
|
||||
});
|
||||
});
|
||||
|
||||
/** An open request only surfaces in the inbox while it covers a live workflow. */
|
||||
async function linkToNewWorkflow(workflowReviewRequestId: string, project = teamProject) {
|
||||
const workflow = await createWorkflow({}, project);
|
||||
await workflowRepository.createWorkflowRow(
|
||||
{ workflowReviewRequestId, workflowId: workflow.id },
|
||||
{},
|
||||
);
|
||||
return workflow;
|
||||
}
|
||||
|
||||
async function seedInboxRequests() {
|
||||
const openRequest = await requestRepository.createRequest(
|
||||
{
|
||||
@@ -2119,6 +2129,8 @@ async function seedInboxRequests() {
|
||||
},
|
||||
{},
|
||||
);
|
||||
const openWorkflow = await linkToNewWorkflow(openRequest.id);
|
||||
// No link row: a hard-deleted workflow leaves closed requests exactly like this
|
||||
const closedRequest = await requestRepository.createRequest(
|
||||
{
|
||||
projectId: teamProject.id,
|
||||
@@ -2128,7 +2140,7 @@ async function seedInboxRequests() {
|
||||
},
|
||||
{},
|
||||
);
|
||||
return { openRequest, closedRequest };
|
||||
return { openRequest, closedRequest, openWorkflow };
|
||||
}
|
||||
|
||||
describe('GET /workflow-review-requests/summary', () => {
|
||||
@@ -2157,7 +2169,7 @@ describe('GET /workflow-review-requests/summary', () => {
|
||||
});
|
||||
|
||||
test('counts a requester their own review regardless of project scope', async () => {
|
||||
await requestRepository.createRequest(
|
||||
const ownRequest = await requestRepository.createRequest(
|
||||
{
|
||||
projectId: teamProject.id,
|
||||
title: 'Review submitted by viewer',
|
||||
@@ -2166,12 +2178,38 @@ describe('GET /workflow-review-requests/summary', () => {
|
||||
},
|
||||
{},
|
||||
);
|
||||
await linkToNewWorkflow(ownRequest.id);
|
||||
|
||||
const response = await viewerAgent.get('/workflow-review-requests/summary').expect(200);
|
||||
|
||||
expect(response.body.data).toEqual({ open: 1, closed: 0 });
|
||||
});
|
||||
|
||||
test('does not count an open review orphaned by a workflow hard delete', async () => {
|
||||
await seedInboxRequests();
|
||||
const orphan = await requestRepository.createRequest(
|
||||
{
|
||||
projectId: teamProject.id,
|
||||
title: 'Orphaned review',
|
||||
createdById: owner.id,
|
||||
state: 'open',
|
||||
},
|
||||
{},
|
||||
);
|
||||
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
|
||||
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.
|
||||
const ownerResponse = await ownerAgent.get('/workflow-review-requests/summary').expect(200);
|
||||
expect(ownerResponse.body.data).toEqual({ open: 1, closed: 1 });
|
||||
|
||||
const memberResponse = await memberAgent.get('/workflow-review-requests/summary').expect(200);
|
||||
expect(memberResponse.body.data).toEqual({ open: 1, closed: 1 });
|
||||
});
|
||||
|
||||
test('returns 403 when feature is disabled', async () => {
|
||||
await policyService.set(false);
|
||||
|
||||
@@ -2181,7 +2219,7 @@ describe('GET /workflow-review-requests/summary', () => {
|
||||
|
||||
describe('GET /workflow-review-requests/inbox', () => {
|
||||
test('returns reviews for instance owner', async () => {
|
||||
const { openRequest } = await seedInboxRequests();
|
||||
const { openRequest, openWorkflow } = await seedInboxRequests();
|
||||
|
||||
const response = await ownerAgent
|
||||
.get('/workflow-review-requests/inbox')
|
||||
@@ -2193,7 +2231,7 @@ describe('GET /workflow-review-requests/inbox', () => {
|
||||
id: openRequest.id,
|
||||
title: 'Open review request',
|
||||
state: 'open',
|
||||
workflowName: null,
|
||||
workflowName: openWorkflow.name,
|
||||
workflowVersionId: null,
|
||||
});
|
||||
expect(response.body.data.hasMore).toBe(false);
|
||||
@@ -2209,6 +2247,54 @@ 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 () => {
|
||||
const { openRequest } = await seedInboxRequests();
|
||||
const orphan = await requestRepository.createRequest(
|
||||
{
|
||||
projectId: teamProject.id,
|
||||
title: 'Orphaned review',
|
||||
createdById: owner.id,
|
||||
state: 'open',
|
||||
},
|
||||
{},
|
||||
);
|
||||
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
|
||||
await workflowEntityRepository.delete({ id: workflow.id });
|
||||
|
||||
// Owner exercises the global scope, member the project-scoped filter
|
||||
const ownerResponse = await ownerAgent
|
||||
.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,
|
||||
]);
|
||||
|
||||
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,
|
||||
]);
|
||||
});
|
||||
|
||||
test('still lists a closed review whose workflow was hard-deleted', async () => {
|
||||
const { closedRequest } = await seedInboxRequests();
|
||||
|
||||
const response = await ownerAgent
|
||||
.get('/workflow-review-requests/inbox')
|
||||
.query({ state: 'closed', limit: 15 })
|
||||
.expect(200);
|
||||
|
||||
// The closed seed request has no link rows — deleted-workflow history stays visible
|
||||
expect(response.body.data.data).toEqual([
|
||||
expect.objectContaining({ id: closedRequest.id, state: 'closed', workflowName: null }),
|
||||
]);
|
||||
});
|
||||
|
||||
test('returns 403 when license is disabled', async () => {
|
||||
testServer.license.disable('feat:workflowReviews');
|
||||
|
||||
@@ -2217,7 +2303,7 @@ describe('GET /workflow-review-requests/inbox', () => {
|
||||
|
||||
test('returns cursor pagination metadata', async () => {
|
||||
await seedInboxRequests();
|
||||
await requestRepository.createRequest(
|
||||
const secondRequest = await requestRepository.createRequest(
|
||||
{
|
||||
projectId: teamProject.id,
|
||||
title: 'Second open review',
|
||||
@@ -2226,6 +2312,7 @@ describe('GET /workflow-review-requests/inbox', () => {
|
||||
},
|
||||
{},
|
||||
);
|
||||
await linkToNewWorkflow(secondRequest.id);
|
||||
|
||||
const firstPage = await ownerAgent
|
||||
.get('/workflow-review-requests/inbox')
|
||||
@@ -2288,7 +2375,7 @@ describe('GET /workflow-review-requests/inbox', () => {
|
||||
|
||||
test('hides reviews from projects the member cannot access', async () => {
|
||||
const otherProject = await createTeamProject('Other Reviews Project', owner);
|
||||
await requestRepository.createRequest(
|
||||
const privateRequest = await requestRepository.createRequest(
|
||||
{
|
||||
projectId: otherProject.id,
|
||||
title: 'Private other-project review',
|
||||
@@ -2297,6 +2384,7 @@ describe('GET /workflow-review-requests/inbox', () => {
|
||||
},
|
||||
{},
|
||||
);
|
||||
await linkToNewWorkflow(privateRequest.id, otherProject);
|
||||
|
||||
const memberResponse = await memberAgent.get('/workflow-review-requests/inbox').expect(200);
|
||||
expect(memberResponse.body.data.data).toEqual([]);
|
||||
@@ -2322,6 +2410,7 @@ describe('GET /workflow-review-requests/inbox', () => {
|
||||
},
|
||||
{},
|
||||
);
|
||||
await linkToNewWorkflow(ownRequest.id, otherProject);
|
||||
|
||||
const response = await memberAgent.get('/workflow-review-requests/inbox').expect(200);
|
||||
|
||||
@@ -2332,7 +2421,7 @@ describe('GET /workflow-review-requests/inbox', () => {
|
||||
|
||||
test('does not truncate pagination when the cursor row is deleted', async () => {
|
||||
await seedInboxRequests();
|
||||
await requestRepository.createRequest(
|
||||
const secondRequest = await requestRepository.createRequest(
|
||||
{
|
||||
projectId: teamProject.id,
|
||||
title: 'Second open review',
|
||||
@@ -2341,6 +2430,7 @@ describe('GET /workflow-review-requests/inbox', () => {
|
||||
},
|
||||
{},
|
||||
);
|
||||
await linkToNewWorkflow(secondRequest.id);
|
||||
|
||||
const firstPage = await ownerAgent
|
||||
.get('/workflow-review-requests/inbox')
|
||||
@@ -2372,6 +2462,7 @@ describe('GET /workflow-review-requests/inbox', () => {
|
||||
},
|
||||
{},
|
||||
);
|
||||
await linkToNewWorkflow(request.id);
|
||||
await reviewerRepository.addReviewers(
|
||||
{
|
||||
workflowReviewRequestId: request.id,
|
||||
@@ -2412,6 +2503,7 @@ describe('GET /workflow-review-requests/inbox', () => {
|
||||
},
|
||||
{},
|
||||
);
|
||||
await linkToNewWorkflow(request.id);
|
||||
|
||||
const response = await ownerAgent
|
||||
.get('/workflow-review-requests/inbox')
|
||||
@@ -2437,6 +2529,7 @@ describe('GET /workflow-review-requests/inbox', () => {
|
||||
},
|
||||
{},
|
||||
);
|
||||
await linkToNewWorkflow(request.id);
|
||||
await reviewerRepository.addReviewers(
|
||||
{
|
||||
workflowReviewRequestId: request.id,
|
||||
@@ -2591,12 +2684,42 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => {
|
||||
});
|
||||
});
|
||||
|
||||
test('still opens the review after its workflow was deleted', async () => {
|
||||
test('no longer 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);
|
||||
|
||||
// Deleting the workflow removes the review's reference to it as well
|
||||
// Bypasses the auto-close hook: the cascade removes the link row and
|
||||
// leaves the request open — a dead leftover nothing can decide or update
|
||||
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);
|
||||
});
|
||||
|
||||
test('still opens a closed review after its workflow was deleted', async () => {
|
||||
const workflow = await createWorkflow({}, teamProject);
|
||||
await createWorkflowHistoryItem(workflow.id, { versionId: 'version-pinned' });
|
||||
const request = await requestRepository.createRequest(
|
||||
{
|
||||
projectId: teamProject.id,
|
||||
title: 'Please review',
|
||||
createdById: owner.id,
|
||||
state: 'closed',
|
||||
decision: 'approved',
|
||||
},
|
||||
{},
|
||||
);
|
||||
await workflowRepository.createWorkflowRow(
|
||||
{
|
||||
workflowReviewRequestId: request.id,
|
||||
workflowId: workflow.id,
|
||||
workflowVersionId: 'version-pinned',
|
||||
},
|
||||
{},
|
||||
);
|
||||
|
||||
// Deleting the workflow removes the review's reference, not its history
|
||||
await workflowEntityRepository.delete({ id: workflow.id });
|
||||
|
||||
const response = await ownerAgent.get(`/workflow-review-requests/${request.id}`).expect(200);
|
||||
|
||||
@@ -0,0 +1,128 @@
|
||||
import { Logger } from '@n8n/backend-common';
|
||||
import { DbLock, DbLockService, WorkflowReviewRequestRepository } from '@n8n/db';
|
||||
import { Service } from '@n8n/di';
|
||||
|
||||
import { CollaborationService } from '@/collaboration/collaboration.service';
|
||||
import type { WorkflowMutationHooks } from '@/workflows/workflow-mutation-hooks-proxy.service';
|
||||
|
||||
type AutoCloseReason = 'workflow-archived' | 'workflow-moved' | 'workflow-deleted';
|
||||
|
||||
/**
|
||||
* Closes open review requests when their workflow stops being reviewable:
|
||||
* archived, moved to another project, or deleted.
|
||||
*
|
||||
* Deliberately not feature-gated beyond module load: the instance policy
|
||||
* toggle guards user actions, but a review left open while the policy is off
|
||||
* would still block publishing once the policy is re-enabled, so cleanup must
|
||||
* run regardless.
|
||||
*/
|
||||
@Service()
|
||||
export class WorkflowReviewAutoCloseService implements WorkflowMutationHooks {
|
||||
constructor(
|
||||
private readonly logger: Logger,
|
||||
private readonly workflowReviewRequestRepository: WorkflowReviewRequestRepository,
|
||||
private readonly dbLockService: DbLockService,
|
||||
private readonly collaborationService: CollaborationService,
|
||||
) {}
|
||||
|
||||
async afterWorkflowArchived(workflowId: string): Promise<void> {
|
||||
await this.closeOpenRequestsForWorkflows([workflowId], 'workflow-archived');
|
||||
}
|
||||
|
||||
async afterWorkflowsTransferred(workflowIds: string[]): Promise<void> {
|
||||
await this.closeOpenRequestsForWorkflows(workflowIds, 'workflow-moved');
|
||||
}
|
||||
|
||||
async beforeWorkflowDeleted(workflowId: string): Promise<void> {
|
||||
// The delete hasn't happened yet, so aborting is recoverable — whereas swallowing
|
||||
// here would let the link rows cascade away and strand an open request.
|
||||
await this.closeOpenRequestsForWorkflows([workflowId], 'workflow-deleted', {
|
||||
rethrow: true,
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Closes reviews whose workflow is gone: the cascade took their link row, so they can
|
||||
* only be found by "open with nothing linked", not by workflow id. Catches reviews
|
||||
* opened after the pre-delete hook ran, and any left by a delete that skips the hooks.
|
||||
*/
|
||||
async afterWorkflowDeleted(workflowId: string): Promise<void> {
|
||||
try {
|
||||
const closedRequestIds = await this.workflowReviewRequestRepository.closeOrphanedOpenRequests(
|
||||
{},
|
||||
);
|
||||
|
||||
if (closedRequestIds.length === 0) return;
|
||||
|
||||
this.logger.info('Closed workflow review request(s) left without a workflow', {
|
||||
reason: 'workflow-deleted',
|
||||
workflowId,
|
||||
workflowReviewRequestIds: closedRequestIds,
|
||||
});
|
||||
} 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.
|
||||
this.logger.error('Failed to close workflow review request(s) left without a workflow', {
|
||||
workflowId,
|
||||
error,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
private async closeOpenRequestsForWorkflows(
|
||||
workflowIds: string[],
|
||||
reason: AutoCloseReason,
|
||||
options: { rethrow?: boolean } = {},
|
||||
): Promise<void> {
|
||||
try {
|
||||
const affectedWorkflowIds = await this.dbLockService.withLockContext(
|
||||
DbLock.WORKFLOW_REVIEW_REQUEST_CREATE,
|
||||
async (ctx) => {
|
||||
// Fetched under the lock so the close can't race a concurrent
|
||||
// decide/version sync on the same request.
|
||||
const openRequests =
|
||||
await this.workflowReviewRequestRepository.findOpenRequestsForWorkflows(
|
||||
workflowIds,
|
||||
ctx,
|
||||
);
|
||||
|
||||
const affected = new Set<string>();
|
||||
for (const { request, workflowIds: linkedWorkflowIds } of openRequests) {
|
||||
request.state = 'closed';
|
||||
// A system close has no closing user; the decision stays as-is.
|
||||
request.closedById = null;
|
||||
await this.workflowReviewRequestRepository.saveRequest(request, ctx);
|
||||
for (const linkedWorkflowId of linkedWorkflowIds) affected.add(linkedWorkflowId);
|
||||
}
|
||||
|
||||
return [...affected];
|
||||
},
|
||||
);
|
||||
|
||||
if (affectedWorkflowIds.length === 0) return;
|
||||
|
||||
this.logger.info('Closed open workflow review request(s)', {
|
||||
reason,
|
||||
workflowIds: affectedWorkflowIds,
|
||||
});
|
||||
|
||||
for (const workflowId of affectedWorkflowIds) {
|
||||
// Fire-and-forget: viewers heal via focus/reconnect refetch.
|
||||
this.collaborationService
|
||||
.broadcastWorkflowReviewStateChanged(workflowId)
|
||||
.catch((error) =>
|
||||
this.logger.warn('Failed to broadcast review state change', { workflowId, error }),
|
||||
);
|
||||
}
|
||||
} catch (error) {
|
||||
this.logger.error('Failed to close open workflow review request(s)', {
|
||||
reason,
|
||||
workflowIds,
|
||||
error,
|
||||
});
|
||||
// Cleanup must never fail a mutation that already committed; only the
|
||||
// pre-delete hook is still in a position to call its mutation off.
|
||||
if (options.rethrow) throw error;
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -136,6 +136,15 @@ 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
|
||||
|
||||
@@ -23,6 +23,7 @@ import {
|
||||
WorkflowPublishHistoryRepository,
|
||||
WorkflowReviewRequestAuthorRepository,
|
||||
WorkflowReviewRequestRepository,
|
||||
WorkflowRepository,
|
||||
WorkflowReviewRequestReviewerRepository,
|
||||
WorkflowReviewRequestWorkflowRepository,
|
||||
type OperationContext,
|
||||
@@ -67,6 +68,7 @@ export class WorkflowReviewRequestService {
|
||||
private readonly workflowFinderService: WorkflowFinderService,
|
||||
private readonly workflowHistoryService: WorkflowHistoryService,
|
||||
private readonly workflowHistoryRepository: WorkflowHistoryRepository,
|
||||
private readonly workflowRepository: WorkflowRepository,
|
||||
private readonly sharedWorkflowRepository: SharedWorkflowRepository,
|
||||
private readonly workflowPublishHistoryRepository: WorkflowPublishHistoryRepository,
|
||||
private readonly workflowReviewRequestRepository: WorkflowReviewRequestRepository,
|
||||
@@ -273,6 +275,41 @@ export class WorkflowReviewRequestService {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Re-asserts under the lock what `create`'s pre-lock checks established: the
|
||||
* workflow still exists, is unarchived, and still belongs to the same project.
|
||||
* Access is not re-checked — the races this guards against are archive and
|
||||
* transfer, both of which these two reads cover.
|
||||
*
|
||||
* Every read is threaded with `ctx` so it runs on the lock transaction's own
|
||||
* connection. A read that checks out a second connection here deadlocks against
|
||||
* the transaction holding the first one (see the same note in `decide`).
|
||||
*/
|
||||
private async assertWorkflowStillReviewable(
|
||||
workflowId: string,
|
||||
expectedProjectId: string,
|
||||
ctx: OperationContext,
|
||||
): Promise<void> {
|
||||
const workflow = await this.workflowRepository.findArchivedState(workflowId, ctx);
|
||||
if (!workflow) {
|
||||
throw new NotFoundError('Could not find workflow');
|
||||
}
|
||||
|
||||
if (workflow.isArchived) {
|
||||
throw new BadRequestError(
|
||||
`The workflow '${workflowId}' is archived and cannot be submitted for review`,
|
||||
);
|
||||
}
|
||||
|
||||
const project = await this.sharedWorkflowRepository.getWorkflowOwningProject(workflowId, ctx);
|
||||
if (project?.id !== expectedProjectId) {
|
||||
throw new ConflictError(
|
||||
`The workflow '${workflowId}' moved to another project and cannot be submitted for review here`,
|
||||
'Retry from the project that now owns the workflow',
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
async create(
|
||||
user: User,
|
||||
dto: CreateWorkflowReviewRequestDto,
|
||||
@@ -330,6 +367,10 @@ export class WorkflowReviewRequestService {
|
||||
const request = await this.dbLockService.withLockContext(
|
||||
DbLock.WORKFLOW_REVIEW_REQUEST_CREATE,
|
||||
async (ctx) => {
|
||||
// without this, a create that lost the race opens a review on an archived
|
||||
// or moved workflow.
|
||||
await this.assertWorkflowStillReviewable(workflowId, project.id, ctx);
|
||||
|
||||
const existing = await this.workflowReviewRequestRepository.findOpenRequestForWorkflow(
|
||||
workflowId,
|
||||
ctx,
|
||||
|
||||
@@ -4,6 +4,7 @@ import { BackendModule } from '@n8n/decorators';
|
||||
import { Container } from '@n8n/di';
|
||||
|
||||
import { isWorkflowReviewsEnvFeatureFlagEnabled } from '@/constants/workflow-reviews';
|
||||
import { WorkflowMutationHooksProxy } from '@/workflows/workflow-mutation-hooks-proxy.service';
|
||||
import { WorkflowPublishGuardProxy } from '@/workflows/workflow-publish-guard-proxy.service';
|
||||
|
||||
@BackendModule({ name: 'workflow-reviews', licenseFlag: LICENSE_FEATURES.WORKFLOW_REVIEWS })
|
||||
@@ -18,5 +19,12 @@ export class WorkflowReviewsModule implements ModuleInterface {
|
||||
Container.get(WorkflowPublishGuardProxy).registerProvider(
|
||||
Container.get(WorkflowReviewPublishGuard),
|
||||
);
|
||||
|
||||
const { WorkflowReviewAutoCloseService } = await import(
|
||||
'./workflow-review-auto-close.service.js'
|
||||
);
|
||||
Container.get(WorkflowMutationHooksProxy).registerProvider(
|
||||
Container.get(WorkflowReviewAutoCloseService),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -67,7 +67,7 @@
|
||||
{
|
||||
"name": "WorkflowReviewRequest",
|
||||
"path": "packages/@n8n/db/src/entities/workflow-review-request.ee.ts",
|
||||
"reason": "Not transferred with the project. Open reviews should be closed when their workflows move to another project; that close-on-move behaviour is deferred."
|
||||
"reason": "Not transferred with the project. Open reviews are closed when their workflow is archived, moved to another project, or deleted (workflow lifecycle hooks); rows are removed via the projectId FK cascade when the source project itself is deleted, as in ownership-transfer flows."
|
||||
}
|
||||
]
|
||||
}
|
||||
|
||||
@@ -0,0 +1,36 @@
|
||||
import { mock } from 'vitest-mock-extended';
|
||||
|
||||
import {
|
||||
type WorkflowMutationHooks,
|
||||
WorkflowMutationHooksProxy,
|
||||
} from '../workflow-mutation-hooks-proxy.service';
|
||||
|
||||
describe('WorkflowMutationHooksProxy', () => {
|
||||
test('every hook is a no-op when no provider is registered', async () => {
|
||||
const proxy = new WorkflowMutationHooksProxy();
|
||||
|
||||
await expect(proxy.afterWorkflowArchived('workflow-1')).resolves.toBeUndefined();
|
||||
await expect(proxy.afterWorkflowsTransferred(['workflow-1'])).resolves.toBeUndefined();
|
||||
await expect(proxy.beforeWorkflowDeleted('workflow-1')).resolves.toBeUndefined();
|
||||
await expect(proxy.afterWorkflowDeleted('workflow-1')).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
test('forwards each hook to the registered provider', async () => {
|
||||
const proxy = new WorkflowMutationHooksProxy();
|
||||
const provider = mock<WorkflowMutationHooks>();
|
||||
proxy.registerProvider(provider);
|
||||
|
||||
await proxy.afterWorkflowArchived('workflow-1');
|
||||
await proxy.afterWorkflowsTransferred(['workflow-1', 'workflow-2']);
|
||||
await proxy.beforeWorkflowDeleted('workflow-3');
|
||||
await proxy.afterWorkflowDeleted('workflow-4');
|
||||
|
||||
expect(provider.afterWorkflowArchived).toHaveBeenCalledExactlyOnceWith('workflow-1');
|
||||
expect(provider.afterWorkflowsTransferred).toHaveBeenCalledExactlyOnceWith([
|
||||
'workflow-1',
|
||||
'workflow-2',
|
||||
]);
|
||||
expect(provider.beforeWorkflowDeleted).toHaveBeenCalledExactlyOnceWith('workflow-3');
|
||||
expect(provider.afterWorkflowDeleted).toHaveBeenCalledExactlyOnceWith('workflow-4');
|
||||
});
|
||||
});
|
||||
@@ -1,14 +1,18 @@
|
||||
import type {
|
||||
CredentialsEntity,
|
||||
Project,
|
||||
SharedWorkflow,
|
||||
WorkflowEntity,
|
||||
WorkflowPublishHistoryRepository,
|
||||
WorkflowRepository,
|
||||
} from '@n8n/db';
|
||||
import type { UpdateResult } from '@n8n/typeorm';
|
||||
import type { EntityManager, UpdateResult } from '@n8n/typeorm';
|
||||
import type { IWorkflowBase } from 'n8n-workflow';
|
||||
import { WorkflowActivationError } from 'n8n-workflow';
|
||||
import { mock } from 'vitest-mock-extended';
|
||||
|
||||
import type { ActiveWorkflowManager } from '@/active-workflow-manager';
|
||||
import type { WorkflowMutationHooksProxy } from '@/workflows/workflow-mutation-hooks-proxy.service';
|
||||
import { EnterpriseWorkflowService } from '@/workflows/workflow.service.ee';
|
||||
|
||||
describe('EnterpriseWorkflowService', () => {
|
||||
@@ -16,6 +20,7 @@ describe('EnterpriseWorkflowService', () => {
|
||||
const workflowRepository = mock<WorkflowRepository>();
|
||||
const activeWorkflowManager = mock<ActiveWorkflowManager>();
|
||||
const workflowPublishHistoryRepository = mock<WorkflowPublishHistoryRepository>();
|
||||
const workflowMutationHooks = mock<WorkflowMutationHooksProxy>();
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
@@ -33,6 +38,7 @@ describe('EnterpriseWorkflowService', () => {
|
||||
mock(), // workflowFinderService
|
||||
mock(), // folderRepository
|
||||
workflowPublishHistoryRepository,
|
||||
workflowMutationHooks,
|
||||
);
|
||||
});
|
||||
|
||||
@@ -162,4 +168,50 @@ describe('EnterpriseWorkflowService', () => {
|
||||
expect(workflowRepository.updateActiveState).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('transferWorkflowOwnership', () => {
|
||||
const destinationProject = mock<Project>({ id: 'proj-dest' });
|
||||
|
||||
const makeWorkflow = (id: string, ownerProjectId: string) =>
|
||||
mock<WorkflowEntity>({
|
||||
id,
|
||||
shared: [
|
||||
mock<SharedWorkflow>({
|
||||
role: 'workflow:owner',
|
||||
project: mock<Project>({ id: ownerProjectId }),
|
||||
}),
|
||||
],
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
const entityManager = mock<EntityManager>();
|
||||
entityManager.transaction.mockImplementation(
|
||||
// @ts-expect-error transaction() has multiple overloads; tests use the single-callback one
|
||||
async (cb: (trx: EntityManager) => Promise<void>) => await cb(mock<EntityManager>()),
|
||||
);
|
||||
Object.defineProperty(workflowRepository, 'manager', {
|
||||
value: entityManager,
|
||||
configurable: true,
|
||||
});
|
||||
});
|
||||
|
||||
it('notifies the mutation hook only for workflows whose owning project changed', async () => {
|
||||
const moved = makeWorkflow('wf-moved', 'proj-source');
|
||||
const folderMoveOnly = makeWorkflow('wf-same-project', 'proj-dest');
|
||||
|
||||
await service['transferWorkflowOwnership']([moved, folderMoveOnly], destinationProject);
|
||||
|
||||
expect(workflowMutationHooks.afterWorkflowsTransferred).toHaveBeenCalledExactlyOnceWith([
|
||||
'wf-moved',
|
||||
]);
|
||||
});
|
||||
|
||||
it('does not notify the mutation hook for a same-project folder move', async () => {
|
||||
const folderMoveOnly = makeWorkflow('wf-same-project', 'proj-dest');
|
||||
|
||||
await service['transferWorkflowOwnership']([folderMoveOnly], destinationProject);
|
||||
|
||||
expect(workflowMutationHooks.afterWorkflowsTransferred).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -43,6 +43,7 @@ import * as WorkflowHelpers from '@/workflow-helpers';
|
||||
import type { WorkflowHookContextService } from '@/workflow-hook-context.service';
|
||||
import type { WorkflowFinderService } from '@/workflows/workflow-finder.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 { WorkflowValidationService } from '@/workflows/workflow-validation.service';
|
||||
import { WorkflowService } from '@/workflows/workflow.service';
|
||||
@@ -110,6 +111,7 @@ describe('WorkflowService', () => {
|
||||
mock(), // workflowPublishedVersionRepository
|
||||
mock(), // workflowHookContextService
|
||||
mock(), // workflowPublishGuard
|
||||
mock(), // workflowMutationHooks
|
||||
);
|
||||
});
|
||||
|
||||
@@ -379,6 +381,7 @@ describe('WorkflowService', () => {
|
||||
mock(), // workflowPublishedVersionRepository
|
||||
workflowHookContextServiceMock, // workflowHookContextService
|
||||
mock(), // workflowPublishGuard
|
||||
mock(), // workflowMutationHooks
|
||||
);
|
||||
|
||||
vi.clearAllMocks();
|
||||
@@ -1119,6 +1122,7 @@ describe('WorkflowService', () => {
|
||||
mock(), // workflowPublishedVersionRepository
|
||||
workflowHookContextServiceMock, // workflowHookContextService
|
||||
workflowPublishGuardMock, // workflowPublishGuard
|
||||
mock(), // workflowMutationHooks
|
||||
);
|
||||
|
||||
// Bypass validation internals
|
||||
@@ -1621,6 +1625,7 @@ describe('WorkflowService', () => {
|
||||
mock(), // workflowPublishedVersionRepository
|
||||
mock(), // workflowHookContextService
|
||||
mock(), // workflowPublishGuard
|
||||
mock(), // workflowMutationHooks
|
||||
);
|
||||
});
|
||||
|
||||
@@ -1679,6 +1684,7 @@ describe('WorkflowService', () => {
|
||||
let activeWorkflowManagerMock: MockProxy<ActiveWorkflowManager>;
|
||||
let externalHooksMock: MockProxy<ExternalHooks>;
|
||||
let workflowPublishedVersionRepositoryMock: MockProxy<WorkflowPublishedVersionRepository>;
|
||||
let workflowMutationHooksMock: MockProxy<WorkflowMutationHooksProxy>;
|
||||
|
||||
const WORKFLOW_ID = 'workflow-1';
|
||||
|
||||
@@ -1699,6 +1705,7 @@ describe('WorkflowService', () => {
|
||||
executionPersistenceMock = mock();
|
||||
activeWorkflowManagerMock = mock();
|
||||
externalHooksMock = mock<ExternalHooks>();
|
||||
workflowMutationHooksMock = mock<WorkflowMutationHooksProxy>();
|
||||
workflowPublishedVersionRepositoryMock = mock<WorkflowPublishedVersionRepository>();
|
||||
workflowPublishedVersionRepositoryMock.getPublishedVersionId.mockResolvedValue(null);
|
||||
globalConfigMock = mock<GlobalConfig>({
|
||||
@@ -1736,6 +1743,7 @@ describe('WorkflowService', () => {
|
||||
workflowPublishedVersionRepositoryMock, // workflowPublishedVersionRepository
|
||||
mock(), // workflowHookContextService
|
||||
mock(), // workflowPublishGuard
|
||||
workflowMutationHooksMock, // workflowMutationHooks
|
||||
);
|
||||
});
|
||||
|
||||
@@ -1783,6 +1791,88 @@ describe('WorkflowService', () => {
|
||||
expect(workflowRepositoryMock.delete).toHaveBeenCalledWith(WORKFLOW_ID);
|
||||
});
|
||||
|
||||
test('runs the beforeWorkflowDeleted lifecycle hook before the row delete', async () => {
|
||||
const workflow = makeWorkflowEntity({ isArchived: true, activeVersionId: null });
|
||||
workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow);
|
||||
|
||||
await workflowService.delete(mock<User>(), WORKFLOW_ID, true);
|
||||
|
||||
expect(workflowMutationHooksMock.beforeWorkflowDeleted).toHaveBeenCalledExactlyOnceWith(
|
||||
WORKFLOW_ID,
|
||||
);
|
||||
expect(
|
||||
workflowMutationHooksMock.beforeWorkflowDeleted.mock.invocationCallOrder[0],
|
||||
).toBeLessThan(workflowRepositoryMock.delete.mock.invocationCallOrder[0]);
|
||||
});
|
||||
|
||||
test('does not run the beforeWorkflowDeleted lifecycle hook when deletion is rejected', async () => {
|
||||
const workflow = makeWorkflowEntity({ activeVersionId: 'v1' });
|
||||
workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow);
|
||||
|
||||
await expect(workflowService.delete(mock<User>(), WORKFLOW_ID, true)).rejects.toBeInstanceOf(
|
||||
ConflictError,
|
||||
);
|
||||
|
||||
expect(workflowMutationHooksMock.beforeWorkflowDeleted).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// Deactivation is not rolled back when a later step fails, so an aborted delete
|
||||
// must not have torn the triggers down.
|
||||
test('aborts the deletion, leaving the workflow running, when the hook throws', async () => {
|
||||
globalConfigMock.workflows.useWorkflowPublicationService = false;
|
||||
const workflow = makeWorkflowEntity({ active: true, activeVersionId: 'v1' });
|
||||
workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow);
|
||||
workflowMutationHooksMock.beforeWorkflowDeleted.mockRejectedValue(new Error('db down'));
|
||||
|
||||
await expect(workflowService.delete(mock<User>(), WORKFLOW_ID, true)).rejects.toThrow(
|
||||
'db down',
|
||||
);
|
||||
|
||||
expect(activeWorkflowManagerMock.remove).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// The hook may throw to abort the delete, so it has to run before the executions are purged,
|
||||
// not just before the row.
|
||||
test('aborts the deletion, leaving executions and the row intact, when the hook throws', async () => {
|
||||
const workflow = makeWorkflowEntity({ isArchived: true, activeVersionId: null });
|
||||
workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow);
|
||||
workflowMutationHooksMock.beforeWorkflowDeleted.mockRejectedValue(new Error('db down'));
|
||||
|
||||
await expect(workflowService.delete(mock<User>(), WORKFLOW_ID, true)).rejects.toThrow(
|
||||
'db down',
|
||||
);
|
||||
|
||||
expect(executionPersistenceMock.hardDeleteByWorkflowId).not.toHaveBeenCalled();
|
||||
expect(workflowRepositoryMock.delete).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// It cleans up rows the cascade orphaned, which cannot be found until the row is gone.
|
||||
test('runs the afterWorkflowDeleted lifecycle hook once the row is deleted', async () => {
|
||||
const workflow = makeWorkflowEntity({ isArchived: true, activeVersionId: null });
|
||||
workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow);
|
||||
|
||||
await workflowService.delete(mock<User>(), WORKFLOW_ID, true);
|
||||
|
||||
expect(workflowMutationHooksMock.afterWorkflowDeleted).toHaveBeenCalledExactlyOnceWith(
|
||||
WORKFLOW_ID,
|
||||
);
|
||||
expect(
|
||||
workflowMutationHooksMock.afterWorkflowDeleted.mock.invocationCallOrder[0],
|
||||
).toBeGreaterThan(workflowRepositoryMock.delete.mock.invocationCallOrder[0]);
|
||||
});
|
||||
|
||||
test('does not run the afterWorkflowDeleted lifecycle hook when the deletion is aborted', async () => {
|
||||
const workflow = makeWorkflowEntity({ isArchived: true, activeVersionId: null });
|
||||
workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow);
|
||||
workflowMutationHooksMock.beforeWorkflowDeleted.mockRejectedValue(new Error('db down'));
|
||||
|
||||
await expect(workflowService.delete(mock<User>(), WORKFLOW_ID, true)).rejects.toThrow(
|
||||
'db down',
|
||||
);
|
||||
|
||||
expect(workflowMutationHooksMock.afterWorkflowDeleted).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test('deletes the workflow executions before the workflow itself', async () => {
|
||||
const workflow = makeWorkflowEntity({ isArchived: true, activeVersionId: null });
|
||||
workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow);
|
||||
@@ -1881,6 +1971,7 @@ describe('WorkflowService', () => {
|
||||
mock(), // workflowPublishedVersionRepository
|
||||
mock(), // workflowHookContextService
|
||||
mock(), // workflowPublishGuard
|
||||
mock(), // workflowMutationHooks
|
||||
);
|
||||
});
|
||||
|
||||
@@ -1933,6 +2024,7 @@ describe('WorkflowService', () => {
|
||||
let workflowFinderServiceMock: MockProxy<WorkflowFinderService>;
|
||||
let workflowRepositoryMock: MockProxy<WorkflowRepository>;
|
||||
let externalHooksMock: MockProxy<ExternalHooks>;
|
||||
let workflowMutationHooksMock: MockProxy<WorkflowMutationHooksProxy>;
|
||||
|
||||
const WORKFLOW_ID = 'workflow-1';
|
||||
|
||||
@@ -1969,6 +2061,7 @@ describe('WorkflowService', () => {
|
||||
workflowFinderServiceMock = mock<WorkflowFinderService>();
|
||||
workflowRepositoryMock = mock();
|
||||
externalHooksMock = mock<ExternalHooks>();
|
||||
workflowMutationHooksMock = mock<WorkflowMutationHooksProxy>();
|
||||
|
||||
workflowService = new WorkflowService(
|
||||
mock(), // logger
|
||||
@@ -2001,6 +2094,7 @@ describe('WorkflowService', () => {
|
||||
mock(), // workflowPublishedVersionRepository
|
||||
mock(), // workflowHookContextService
|
||||
mock(), // workflowPublishGuard
|
||||
workflowMutationHooksMock, // workflowMutationHooks
|
||||
);
|
||||
});
|
||||
|
||||
@@ -2027,6 +2121,29 @@ describe('WorkflowService', () => {
|
||||
expectedActor,
|
||||
]);
|
||||
});
|
||||
|
||||
test('runs the afterWorkflowArchived lifecycle hook on archive', async () => {
|
||||
const workflow = makeWorkflowEntity({ isArchived: false, activeVersionId: null });
|
||||
workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow);
|
||||
|
||||
await workflowService.archive(makeActingUser(), WORKFLOW_ID);
|
||||
|
||||
expect(workflowMutationHooksMock.afterWorkflowArchived).toHaveBeenCalledExactlyOnceWith(
|
||||
WORKFLOW_ID,
|
||||
);
|
||||
});
|
||||
|
||||
test('runs no lifecycle hook when archiving is skipped or on unarchive', async () => {
|
||||
const archived = makeWorkflowEntity({ isArchived: true });
|
||||
workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(archived);
|
||||
|
||||
await workflowService.archive(makeActingUser(), WORKFLOW_ID, { skipArchived: true });
|
||||
await workflowService.unarchive(makeActingUser(), WORKFLOW_ID);
|
||||
|
||||
expect(workflowMutationHooksMock.afterWorkflowArchived).not.toHaveBeenCalled();
|
||||
expect(workflowMutationHooksMock.beforeWorkflowDeleted).not.toHaveBeenCalled();
|
||||
expect(workflowMutationHooksMock.afterWorkflowsTransferred).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('updateWorkflowTags()', () => {
|
||||
@@ -2074,6 +2191,7 @@ describe('WorkflowService', () => {
|
||||
mock(), // workflowPublishedVersionRepository
|
||||
mock(), // workflowHookContextService
|
||||
mock(), // workflowPublishGuard
|
||||
mock(), // workflowMutationHooks
|
||||
);
|
||||
});
|
||||
|
||||
|
||||
@@ -0,0 +1,58 @@
|
||||
import { Service } from '@n8n/di';
|
||||
|
||||
/**
|
||||
* Lets a module react to workflow mutations without core depending on the
|
||||
* module: it registers an implementation on {@link WorkflowMutationHooksProxy}.
|
||||
* With no provider registered, every hook is a no-op.
|
||||
*
|
||||
* Distinct from the `workflow.afterArchive` / `workflow.afterDelete` external
|
||||
* hooks, which notify code outside n8n rather than modules inside it.
|
||||
*
|
||||
* The `after*` hooks observe an already-committed mutation and must not throw —
|
||||
* there is nothing left to abort. `beforeWorkflowDeleted` is the exception:
|
||||
* it runs while the delete can still be called off, so it may throw to stop it.
|
||||
*/
|
||||
export interface WorkflowMutationHooks {
|
||||
afterWorkflowArchived(workflowId: string): Promise<void>;
|
||||
|
||||
/** Called only for workflows whose owning project actually changed. */
|
||||
afterWorkflowsTransferred(workflowIds: string[]): Promise<void>;
|
||||
|
||||
/**
|
||||
* Called before anything about the workflow is destroyed, while rows
|
||||
* referencing it still exist. Throwing here aborts the deletion.
|
||||
*/
|
||||
beforeWorkflowDeleted(workflowId: string): Promise<void>;
|
||||
|
||||
/**
|
||||
* Called once the workflow row is gone, for cleanup that can only be done
|
||||
* after the delete cascades — rows orphaned by it, which by definition cannot
|
||||
* be found while the workflow still exists.
|
||||
*/
|
||||
afterWorkflowDeleted(workflowId: string): Promise<void>;
|
||||
}
|
||||
|
||||
@Service()
|
||||
export class WorkflowMutationHooksProxy implements WorkflowMutationHooks {
|
||||
private provider: WorkflowMutationHooks | null = null;
|
||||
|
||||
registerProvider(provider: WorkflowMutationHooks): void {
|
||||
this.provider = provider;
|
||||
}
|
||||
|
||||
async afterWorkflowArchived(workflowId: string): Promise<void> {
|
||||
await this.provider?.afterWorkflowArchived(workflowId);
|
||||
}
|
||||
|
||||
async afterWorkflowsTransferred(workflowIds: string[]): Promise<void> {
|
||||
await this.provider?.afterWorkflowsTransferred(workflowIds);
|
||||
}
|
||||
|
||||
async beforeWorkflowDeleted(workflowId: string): Promise<void> {
|
||||
await this.provider?.beforeWorkflowDeleted(workflowId);
|
||||
}
|
||||
|
||||
async afterWorkflowDeleted(workflowId: string): Promise<void> {
|
||||
await this.provider?.afterWorkflowDeleted(workflowId);
|
||||
}
|
||||
}
|
||||
@@ -43,6 +43,7 @@ import { OwnershipService } from '@/services/ownership.service';
|
||||
import { ProjectService } from '@/services/project.service.ee';
|
||||
|
||||
import { WorkflowFinderService } from './workflow-finder.service';
|
||||
import { WorkflowMutationHooksProxy } from './workflow-mutation-hooks-proxy.service';
|
||||
|
||||
@Service()
|
||||
export class EnterpriseWorkflowService {
|
||||
@@ -60,6 +61,7 @@ export class EnterpriseWorkflowService {
|
||||
private readonly workflowFinderService: WorkflowFinderService,
|
||||
private readonly folderRepository: FolderRepository,
|
||||
private readonly workflowPublishHistoryRepository: WorkflowPublishHistoryRepository,
|
||||
private readonly workflowMutationHooks: WorkflowMutationHooksProxy,
|
||||
) {}
|
||||
|
||||
async shareWithProjects(
|
||||
@@ -648,6 +650,16 @@ export class EnterpriseWorkflowService {
|
||||
workflows: WorkflowEntity[],
|
||||
destinationProject: Project,
|
||||
) {
|
||||
// Resolved before the transaction rewrites the sharings. Workflows already
|
||||
// owned by the destination are folder moves, not project transfers.
|
||||
const movedWorkflowIds = workflows
|
||||
.filter(
|
||||
(workflow) =>
|
||||
workflow.shared.find((s) => s.role === 'workflow:owner')?.project.id !==
|
||||
destinationProject.id,
|
||||
)
|
||||
.map((workflow) => workflow.id);
|
||||
|
||||
await this.workflowRepository.manager.transaction(async (trx) => {
|
||||
for (const workflow of workflows) {
|
||||
// Remove all sharings
|
||||
@@ -668,6 +680,10 @@ export class EnterpriseWorkflowService {
|
||||
for (const workflow of workflows) {
|
||||
await this.ownershipService.setWorkflowProjectCacheEntry(workflow.id, destinationProject);
|
||||
}
|
||||
|
||||
if (movedWorkflowIds.length > 0) {
|
||||
await this.workflowMutationHooks.afterWorkflowsTransferred(movedWorkflowIds);
|
||||
}
|
||||
}
|
||||
|
||||
private async shareCredentialsWithProject(
|
||||
|
||||
@@ -31,6 +31,7 @@ import { WorkflowPublicationNotifier } from './publication/workflow-publication-
|
||||
import { getErrorDescription, getErrorNodeId, getRequiredRedactionScopes } from './utils';
|
||||
import { WorkflowFinderService } from './workflow-finder.service';
|
||||
import { WorkflowHistoryService } from './workflow-history/workflow-history.service';
|
||||
import { WorkflowMutationHooksProxy } from './workflow-mutation-hooks-proxy.service';
|
||||
import { WorkflowPublishGuardProxy } from './workflow-publish-guard-proxy.service';
|
||||
import { WorkflowValidationService } from './workflow-validation.service';
|
||||
|
||||
@@ -98,6 +99,7 @@ export class WorkflowService {
|
||||
private readonly workflowPublishedVersionRepository: WorkflowPublishedVersionRepository,
|
||||
private readonly workflowHookContextService: WorkflowHookContextService,
|
||||
private readonly workflowPublishGuard: WorkflowPublishGuardProxy,
|
||||
private readonly workflowMutationHooks: WorkflowMutationHooksProxy,
|
||||
) {}
|
||||
|
||||
async getMany(
|
||||
@@ -1176,6 +1178,12 @@ export class WorkflowService {
|
||||
throw new BadRequestError('Workflow must be archived before it can be deleted.');
|
||||
}
|
||||
|
||||
// Ahead of every destructive step, including the trigger teardown below: the
|
||||
// hook may throw to abort the delete, and deactivation is not rolled back, so
|
||||
// running it later would strand the workflow as active in the DB but no longer
|
||||
// running.
|
||||
await this.workflowMutationHooks.beforeWorkflowDeleted(workflowId);
|
||||
|
||||
if (workflow.active) {
|
||||
// deactivate before deleting
|
||||
await this.activeWorkflowManager.remove(workflowId);
|
||||
@@ -1188,6 +1196,10 @@ export class WorkflowService {
|
||||
|
||||
await this.workflowRepository.delete(workflowId);
|
||||
|
||||
// After the cascade, so it can see the rows the delete orphaned. Observes a
|
||||
// committed delete, so it must not throw — the module swallows its own errors.
|
||||
await this.workflowMutationHooks.afterWorkflowDeleted(workflowId);
|
||||
|
||||
this.eventService.emit('workflow-deleted', { user, workflowId, publicApi: false });
|
||||
await this.externalHooks.run('workflow.afterDelete', [
|
||||
workflowId,
|
||||
@@ -1254,6 +1266,8 @@ export class WorkflowService {
|
||||
|
||||
await this.workflowHistoryService.saveVersion(user, workflow, workflowId);
|
||||
|
||||
await this.workflowMutationHooks.afterWorkflowArchived(workflowId);
|
||||
|
||||
this.eventService.emit('workflow-archived', {
|
||||
user,
|
||||
workflowId,
|
||||
|
||||
+94
-1
@@ -1,10 +1,16 @@
|
||||
import {
|
||||
createTeamProject,
|
||||
createWorkflow,
|
||||
createWorkflowHistory,
|
||||
createWorkflowWithHistory,
|
||||
testDb,
|
||||
} from '@n8n/backend-test-utils';
|
||||
import { WorkflowHistoryRepository, WorkflowPublishedVersionRepository } from '@n8n/db';
|
||||
import {
|
||||
WorkflowHistoryRepository,
|
||||
WorkflowPublishedVersionRepository,
|
||||
WorkflowReviewRequestRepository,
|
||||
WorkflowReviewRequestWorkflowRepository,
|
||||
} from '@n8n/db';
|
||||
import { Container } from '@n8n/di';
|
||||
import { RULES, type INode } from 'n8n-workflow';
|
||||
import { v4 as uuid } from 'uuid';
|
||||
@@ -27,10 +33,13 @@ describe('WorkflowHistoryRepository', () => {
|
||||
|
||||
beforeEach(async () => {
|
||||
await testDb.truncate([
|
||||
'WorkflowReviewRequestWorkflow',
|
||||
'WorkflowReviewRequest',
|
||||
'WorkflowPublishedVersion',
|
||||
'WorkflowPublishHistory',
|
||||
'WorkflowHistory',
|
||||
'WorkflowEntity',
|
||||
'Project',
|
||||
'User',
|
||||
]);
|
||||
});
|
||||
@@ -338,6 +347,90 @@ describe('WorkflowHistoryRepository', () => {
|
||||
expect(remainingIds).toContain(vCurrent); // preserved: current version
|
||||
expect(remainingIds).not.toContain(vOther); // pruned: old and unreferenced
|
||||
});
|
||||
|
||||
// The open-review exclusion must hold even when named-version preservation
|
||||
// is off (unlicensed), because review pins are named versions and would
|
||||
// otherwise be pruned mid-review. A closed review no longer protects its pin.
|
||||
it('should preserve versions pinned by an open review but not by a closed one', async () => {
|
||||
const vCurrent = uuid();
|
||||
const vOpenPinned = uuid();
|
||||
const vClosedPinned = uuid();
|
||||
|
||||
const tenDaysAgo = new Date();
|
||||
tenDaysAgo.setDate(tenDaysAgo.getDate() - 10);
|
||||
|
||||
const oneDayAgo = new Date();
|
||||
oneDayAgo.setDate(oneDayAgo.getDate() - 1);
|
||||
|
||||
const workflow = await createWorkflow({
|
||||
versionId: vCurrent,
|
||||
nodes: [{ ...testNode1, parameters: { a: 'current' } }],
|
||||
});
|
||||
await createWorkflowHistory(
|
||||
{
|
||||
...workflow,
|
||||
versionId: vOpenPinned,
|
||||
nodes: [{ ...testNode1, parameters: { a: 'open-pinned' } }],
|
||||
},
|
||||
undefined,
|
||||
undefined,
|
||||
{ createdAt: tenDaysAgo, name: 'Open review pin' },
|
||||
);
|
||||
await createWorkflowHistory(
|
||||
{
|
||||
...workflow,
|
||||
versionId: vClosedPinned,
|
||||
nodes: [{ ...testNode1, parameters: { a: 'closed-pinned' } }],
|
||||
},
|
||||
undefined,
|
||||
undefined,
|
||||
{ createdAt: tenDaysAgo, name: 'Closed review pin' },
|
||||
);
|
||||
await createWorkflowHistory(workflow);
|
||||
|
||||
const project = await createTeamProject('Reviews Project');
|
||||
const requestRepository = Container.get(WorkflowReviewRequestRepository);
|
||||
const linkRepository = Container.get(WorkflowReviewRequestWorkflowRepository);
|
||||
|
||||
const openRequest = await requestRepository.createRequest(
|
||||
{ projectId: project.id, title: 'Open review', createdById: null },
|
||||
{},
|
||||
);
|
||||
await linkRepository.createWorkflowRow(
|
||||
{
|
||||
workflowReviewRequestId: openRequest.id,
|
||||
workflowId: workflow.id,
|
||||
workflowVersionId: vOpenPinned,
|
||||
},
|
||||
{},
|
||||
);
|
||||
|
||||
const closedRequest = await requestRepository.createRequest(
|
||||
{ projectId: project.id, title: 'Closed review', state: 'closed', createdById: null },
|
||||
{},
|
||||
);
|
||||
await linkRepository.createWorkflowRow(
|
||||
{
|
||||
workflowReviewRequestId: closedRequest.id,
|
||||
workflowId: workflow.id,
|
||||
workflowVersionId: vClosedPinned,
|
||||
},
|
||||
{},
|
||||
);
|
||||
|
||||
const repository = Container.get(WorkflowHistoryRepository);
|
||||
|
||||
// preserveNamedVersions = false: the name on the pins must not save them here
|
||||
await repository.deleteEarlierThanExceptCurrentAndActive(oneDayAgo, false);
|
||||
|
||||
const remainingIds = (await repository.find()).map((r) => r.versionId);
|
||||
expect(remainingIds).toContain(vOpenPinned); // preserved: pinned by an open review
|
||||
expect(remainingIds).not.toContain(vClosedPinned); // pruned: its review is closed
|
||||
|
||||
// The pin was nulled by the FK, not left dangling
|
||||
const closedLinkRows = await linkRepository.findByRequestId(closedRequest.id, {});
|
||||
expect(closedLinkRows[0]?.workflowVersionId).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('getWorkflowIdsInRange', () => {
|
||||
|
||||
@@ -39,6 +39,7 @@ describe('EnterpriseWorkflowService', () => {
|
||||
mock(), // workflowFinderService
|
||||
mock(), // folderRepository
|
||||
mock(), // workflowPublishHistoryRepository
|
||||
mock(), // workflowMutationHooks
|
||||
);
|
||||
});
|
||||
|
||||
|
||||
@@ -99,6 +99,7 @@ beforeAll(async () => {
|
||||
workflowPublishedVersionRepository,
|
||||
Container.get(WorkflowHookContextService), // workflowHookContextService
|
||||
workflowPublishGuard,
|
||||
mock(), // workflowMutationHooks
|
||||
);
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user