From 92339bb2b067689224bf4eae4447f772bcf9540f Mon Sep 17 00:00:00 2001 From: Jan Kalkan Date: Sun, 16 Aug 2026 14:58:24 +0000 Subject: [PATCH] feat(core): Complete the review activity feed and add the decision note (no-changelog) (#36137) Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: kgrhartlage --- packages/@n8n/api-types/src/dto/index.ts | 2 +- ...decide-workflow-review-request.dto.test.ts | 34 ++ .../decide-workflow-review-request.dto.ts | 6 + .../workflow-review-activity.dto.ts | 25 +- packages/@n8n/api-types/src/index.ts | 2 +- .../api-types/src/workflow-review-activity.ts | 77 ++++- .../entities/workflow-review-activity.ee.ts | 16 +- packages/@n8n/db/src/repositories/index.ts | 1 + .../workflow-review-activity.repository.ts | 33 +- .../workflow-review-request.repository.ts | 112 +++++-- ...rkflow-review-activity.integration.test.ts | 245 ++++++++++++++- ...flow-review-auto-close.integration.test.ts | 133 +++++++- ...workflow-review-auto-close.service.test.ts | 120 ++++++- ...flow-review-request.decide.service.test.ts | 59 +++- .../workflow-review-request.service.test.ts | 9 +- ...flow-review-request.update.service.test.ts | 32 +- ...ew-requests.controller.integration.test.ts | 8 +- .../__tests__/workflow-review.mapper.test.ts | 49 +++ .../workflow-review-activity.service.ts | 13 +- .../workflow-review-auto-close.service.ts | 102 ++++-- .../workflow-review-request.service.ts | 98 +++++- .../workflow-review.mapper.ts | 101 ++++-- .../frontend/@n8n/i18n/src/locales/en.json | 16 +- .../WorkflowReviewActivityFeed.test.ts | 294 ++++++++++++++++-- .../components/WorkflowReviewActivityFeed.vue | 71 ++++- .../WorkflowReviewCommentComposer.test.ts | 33 +- .../WorkflowReviewCommentComposer.vue | 18 +- .../WorkflowReviewDecisionPopover.test.ts | 272 ++++++++++++++++ .../WorkflowReviewDecisionPopover.vue | 196 ++++++++++++ .../WorkflowReviewDetailMetadata.vue | 27 +- ...kflowReviewDetailTabs.decisionNote.test.ts | 92 ++++++ .../WorkflowReviewDetailTabs.test.ts | 153 ++++----- .../components/WorkflowReviewDetailTabs.vue | 112 +++---- .../WorkflowReviewRequestsSidebar.vue | 2 +- .../components/_activity-card.scss | 36 +++ .../WorkflowReviewActivityComment.vue | 39 ++- .../WorkflowReviewActivityEventEntry.vue | 203 ++++++++++++ .../components/activityEntryRegistry.ts | 8 + .../reviewActivity.store.test.ts | 32 +- .../workflow-reviews/reviewActivity.store.ts | 25 +- .../reviewInbox.store.test.ts | 18 +- .../workflow-reviews/reviewInbox.store.ts | 4 +- .../views/WorkflowReviewRequestsView.test.ts | 258 +++++++++------ .../views/WorkflowReviewRequestsView.vue | 41 ++- .../workflow-reviews/workflowReviews.api.ts | 4 +- .../workflow-reviews/workflowReviews.utils.ts | 5 + 46 files changed, 2764 insertions(+), 472 deletions(-) create mode 100644 packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review.mapper.test.ts create mode 100644 packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDecisionPopover.test.ts create mode 100644 packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDecisionPopover.vue create mode 100644 packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.decisionNote.test.ts create mode 100644 packages/frontend/editor-ui/src/features/workflow-reviews/components/_activity-card.scss create mode 100644 packages/frontend/editor-ui/src/features/workflow-reviews/components/activity-entries/WorkflowReviewActivityEventEntry.vue diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index 2fc17d8d776..e0ddf584990 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -339,7 +339,7 @@ export type * from './workflow-reviews/get-workflow-review-request-detail.dto'; export { ListWorkflowReviewActivityQueryDto, CreateWorkflowReviewCommentDto, - WORKFLOW_REVIEW_COMMENT_MAX_LENGTH, + WORKFLOW_REVIEW_TEXT_MAX_LENGTH, type ListWorkflowReviewActivityResponse, } from './workflow-reviews/workflow-review-activity.dto'; diff --git a/packages/@n8n/api-types/src/dto/workflow-reviews/__tests__/decide-workflow-review-request.dto.test.ts b/packages/@n8n/api-types/src/dto/workflow-reviews/__tests__/decide-workflow-review-request.dto.test.ts index 7c311e8535d..0449424c06a 100644 --- a/packages/@n8n/api-types/src/dto/workflow-reviews/__tests__/decide-workflow-review-request.dto.test.ts +++ b/packages/@n8n/api-types/src/dto/workflow-reviews/__tests__/decide-workflow-review-request.dto.test.ts @@ -5,11 +5,30 @@ describe('DecideWorkflowReviewRequestDto', () => { test.each([ { name: 'approved decision', request: { decision: 'approved' } }, { name: 'changes_requested decision', request: { decision: 'changes_requested' } }, + { + name: 'approved decision with a note', + request: { decision: 'approved', note: 'Ships as is' }, + }, + { + name: 'changes_requested decision with a note', + request: { decision: 'changes_requested', note: 'Please rename the node' }, + }, ])('should validate $name', ({ request }) => { const result = DecideWorkflowReviewRequestDto.safeParse(request); expect(result.success).toBe(true); expect(result.data).toMatchObject(request); }); + + // Its own test because the table above asserts `toMatchObject(request)`, which a row + // whose point is that the output differs from the input cannot satisfy. + test('should trim the note', () => { + const result = DecideWorkflowReviewRequestDto.safeParse({ + decision: 'approved', + note: ' first line\nsecond line ', + }); + + expect(result.data?.note).toBe('first line\nsecond line'); + }); }); describe('Invalid requests', () => { @@ -34,6 +53,21 @@ describe('DecideWorkflowReviewRequestDto', () => { request: { decision: 1 }, expectedErrorPath: ['decision'], }, + { + name: 'a note that is only whitespace', + request: { decision: 'changes_requested', note: ' \n ' }, + expectedErrorPath: ['note'], + }, + { + name: 'a note over the length limit', + request: { decision: 'changes_requested', note: 'x'.repeat(10_001) }, + expectedErrorPath: ['note'], + }, + { + name: 'a note containing a control character', + request: { decision: 'changes_requested', note: 'oops \x00 here' }, + expectedErrorPath: ['note'], + }, ])('should fail validation for $name', ({ request, expectedErrorPath }) => { const result = DecideWorkflowReviewRequestDto.safeParse(request); expect(result.success).toBe(false); diff --git a/packages/@n8n/api-types/src/dto/workflow-reviews/decide-workflow-review-request.dto.ts b/packages/@n8n/api-types/src/dto/workflow-reviews/decide-workflow-review-request.dto.ts index 85a04bfc6a1..b2d2e8aa1a9 100644 --- a/packages/@n8n/api-types/src/dto/workflow-reviews/decide-workflow-review-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/workflow-reviews/decide-workflow-review-request.dto.ts @@ -1,6 +1,12 @@ +import { reviewTextSchema } from './workflow-review-activity.dto'; import { workflowReviewRequestDecisionSchema } from '../../workflow-review-request-summary'; import { Z } from '../../zod-class'; export class DecideWorkflowReviewRequestDto extends Z.class({ decision: workflowReviewRequestDecisionSchema.exclude(['pending']), + /** + * Required for `changes_requested`, optional for `approved`. The requirement is enforced + * service-side: `Z.class` wraps a plain `z.object` with no `superRefine` hook. + */ + note: reviewTextSchema.optional(), }) {} diff --git a/packages/@n8n/api-types/src/dto/workflow-reviews/workflow-review-activity.dto.ts b/packages/@n8n/api-types/src/dto/workflow-reviews/workflow-review-activity.dto.ts index d6bf8c4748b..4504e9b1430 100644 --- a/packages/@n8n/api-types/src/dto/workflow-reviews/workflow-review-activity.dto.ts +++ b/packages/@n8n/api-types/src/dto/workflow-reviews/workflow-review-activity.dto.ts @@ -18,17 +18,20 @@ export interface ListWorkflowReviewActivityResponse { hasMore: boolean; } -export const WORKFLOW_REVIEW_COMMENT_MAX_LENGTH = 10_000; +export const WORKFLOW_REVIEW_TEXT_MAX_LENGTH = 10_000; + +/** Shared by the comment body and the decision note: same cap, same control-character rule. */ +export const reviewTextSchema = z + .string() + .trim() + .min(1) + .max(WORKFLOW_REVIEW_TEXT_MAX_LENGTH) + // NUL cannot be stored in a Postgres text column at all, so it would turn user input + // into a 500. The rest of C0 is non-printing junk with no place in a comment body, and + // is rejected in the same pass. \n, \r and \t are deliberately allowed through. + // eslint-disable-next-line no-control-regex + .refine((v) => !/[\x00-\x08\x0B\x0C\x0E-\x1F]/.test(v), 'Contains control characters'); export class CreateWorkflowReviewCommentDto extends Z.class({ - body: z - .string() - .trim() - .min(1) - .max(WORKFLOW_REVIEW_COMMENT_MAX_LENGTH) - // NUL cannot be stored in a Postgres text column at all, so it would turn user input - // into a 500. The rest of C0 is non-printing junk with no place in a comment body, and - // is rejected in the same pass. \n, \r and \t are deliberately allowed through. - // eslint-disable-next-line no-control-regex - .refine((v) => !/[\x00-\x08\x0B\x0C\x0E-\x1F]/.test(v), 'Body contains control characters'), + body: reviewTextSchema, }) {} diff --git a/packages/@n8n/api-types/src/index.ts b/packages/@n8n/api-types/src/index.ts index 74a3e20bdd1..873be0407b6 100644 --- a/packages/@n8n/api-types/src/index.ts +++ b/packages/@n8n/api-types/src/index.ts @@ -14,7 +14,7 @@ export * from './instance-registry-types'; export * from './redaction-enforcement'; export * from './redaction-enforcement-floor'; export * from './workflow-reviews-policy'; -export type * from './workflow-review-activity'; +export * from './workflow-review-activity'; export type * from './workflow-review-eligible-reviewer'; export * from './workflow-review-request-summary'; export * from './workflow-publish-blocked-details'; diff --git a/packages/@n8n/api-types/src/workflow-review-activity.ts b/packages/@n8n/api-types/src/workflow-review-activity.ts index 5af23693857..2d05329a267 100644 --- a/packages/@n8n/api-types/src/workflow-review-activity.ts +++ b/packages/@n8n/api-types/src/workflow-review-activity.ts @@ -1,4 +1,4 @@ -import type { IDataObject } from 'n8n-workflow'; +import { z } from 'zod'; import type { Iso8601DateTimeString } from './datetime'; import type { WorkflowReviewEligibleReviewer } from './workflow-review-eligible-reviewer'; @@ -22,6 +22,59 @@ export type WorkflowReviewActivityType = * approval writes `review.approved` instead, never both. */ | 'review.closed'; +// These are the typeVersion 1 shapes. A typeVersion 2 gets its own schema, its own +// union member and its own mapper case; never widen one of these in place. +const workflowReviewClosedReasonSchema = z.enum([ + 'workflow-archived', + 'workflow-moved', + 'workflow-deleted', +]); +export type WorkflowReviewClosedReason = z.infer; + +/** + * Pairs the version with its workflow: the entry is the immutable record, and the live pin it was + * built from is prunable, so a bare version id becomes unresolvable once pruning removes the + * history row. + */ +const workflowReviewActivityWorkflowVersionSchema = z.object({ + workflowId: z.string(), + workflowVersionId: z.string(), +}); +export const workflowReviewOpenedActivityDataSchema = z.object({ + workflowVersions: z.array(workflowReviewActivityWorkflowVersionSchema), +}); +export type WorkflowReviewOpenedActivityData = z.infer< + typeof workflowReviewOpenedActivityDataSchema +>; + +export const workflowReviewDecisionActivityDataSchema = z.object({ + workflowVersions: z.array(workflowReviewActivityWorkflowVersionSchema), + note: z.string().nullable(), +}); +export type WorkflowReviewDecisionActivityData = z.infer< + typeof workflowReviewDecisionActivityDataSchema +>; + +export const workflowReviewVersionUpdatedActivityDataSchema = z.object({ + /** + * Here rather than in the entity's `workflowId` column: that column's FK cascades, so a + * workflow delete would silently take these entries out of an append-only feed. + */ + workflowId: z.string(), + fromWorkflowVersionId: z.string().nullable(), + toWorkflowVersionId: z.string().nullable(), +}); +export type WorkflowReviewVersionUpdatedActivityData = z.infer< + typeof workflowReviewVersionUpdatedActivityDataSchema +>; + +export const workflowReviewClosedActivityDataSchema = z.object({ + reason: workflowReviewClosedReasonSchema, +}); +export type WorkflowReviewClosedActivityData = z.infer< + typeof workflowReviewClosedActivityDataSchema +>; + type WorkflowReviewActivityBase = { /** Int in the database, string on the wire. */ id: string; @@ -43,6 +96,8 @@ export type WorkflowReviewActivityMessage = { /** * Total over every type the CHECK constraint allows, so no stored row is ever unmappable. + * `data: null` on a non-comment entry means the stored payload did not parse; the renderer + * degrades rather than dropping the entry. */ export type WorkflowReviewActivityEntry = | (WorkflowReviewActivityBase & { @@ -51,6 +106,22 @@ export type WorkflowReviewActivityEntry = messages: WorkflowReviewActivityMessage[]; }) | (WorkflowReviewActivityBase & { - type: Exclude; - data: IDataObject | null; + type: 'review.opened'; + data: WorkflowReviewOpenedActivityData | null; + }) + | (WorkflowReviewActivityBase & { + type: 'review.changes_requested' | 'review.approved'; + data: WorkflowReviewDecisionActivityData | null; + }) + | (WorkflowReviewActivityBase & { + type: 'review.version_updated'; + data: WorkflowReviewVersionUpdatedActivityData | null; + }) + | (WorkflowReviewActivityBase & { + type: 'review.closed'; + data: WorkflowReviewClosedActivityData | null; + }) + | (WorkflowReviewActivityBase & { + type: 'workflow.published'; + data: null; }); diff --git a/packages/@n8n/db/src/entities/workflow-review-activity.ee.ts b/packages/@n8n/db/src/entities/workflow-review-activity.ee.ts index e9d64039834..d8184f4b40e 100644 --- a/packages/@n8n/db/src/entities/workflow-review-activity.ee.ts +++ b/packages/@n8n/db/src/entities/workflow-review-activity.ee.ts @@ -31,8 +31,10 @@ export class WorkflowReviewActivity extends WithCreatedAt { typeVersion: number; /** - * Immutable per-type detail. Ids only: a user id stored here would survive user deletion, - * unlike `createdById`, so a type needing another actor gets its own `SET NULL` column. + * Immutable per-type detail: ids, plus a reviewer's note on a decision. Nothing redacts it — + * unlike a comment body, which is nulled on delete, it is frozen once written. No user ids: one + * stored here would survive user deletion, unlike `createdById`, so a type needing another actor + * gets its own `SET NULL` column. */ @JsonColumn({ nullable: true }) data: IDataObject | null; @@ -42,9 +44,13 @@ export class WorkflowReviewActivity extends WithCreatedAt { createdById: string | null; /** - * Scopes an entry to one workflow; `null` for review-level entries such as comments. A column - * rather than a key in `data` so the feed can query and filter on it directly, for instance to - * leave out entries about workflows the reader may not open. Nothing writes it yet. + * Scopes an entry to one workflow; `null` for review-level entries such as comments and + * decisions. Nothing writes it yet, deliberately: the FK is `ON DELETE CASCADE`, so setting it + * means accepting that an ordinary workflow delete removes those entries from an append-only + * feed, which is why `review.version_updated` keeps its `workflowId` in `data`. Filtering the + * feed on this column is the precondition for relaxing + * `CreateWorkflowReviewRequestDto.workflows.length(1)`, since a multi-workflow review would + * otherwise show entries about workflows the reader may not open. */ @Column({ type: 'varchar', length: 36, nullable: true }) workflowId: string | null; diff --git a/packages/@n8n/db/src/repositories/index.ts b/packages/@n8n/db/src/repositories/index.ts index 16ffba7e90d..39057183392 100644 --- a/packages/@n8n/db/src/repositories/index.ts +++ b/packages/@n8n/db/src/repositories/index.ts @@ -83,6 +83,7 @@ export { export { WorkflowPublishHistoryRepository } from './workflow-publish-history.repository'; export { WorkflowReviewRequestRepository, + type ClosedUnreviewableRequest, type InboxCursor, type InboxVisibility, type WorkflowReviewRequestForWorkflowRow, diff --git a/packages/@n8n/db/src/repositories/workflow-review-activity.repository.ts b/packages/@n8n/db/src/repositories/workflow-review-activity.repository.ts index 54ed6f0a0fa..1460763b99a 100644 --- a/packages/@n8n/db/src/repositories/workflow-review-activity.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow-review-activity.repository.ts @@ -1,7 +1,11 @@ -import type { WorkflowReviewActivityType } from '@n8n/api-types'; +import type { + WorkflowReviewClosedActivityData, + WorkflowReviewDecisionActivityData, + WorkflowReviewOpenedActivityData, + WorkflowReviewVersionUpdatedActivityData, +} from '@n8n/api-types'; import { Service } from '@n8n/di'; import { DataSource, In, LessThan } from '@n8n/typeorm'; -import type { IDataObject } from 'n8n-workflow'; import { BaseRepository } from './base-repository'; import { WorkflowReviewActivityComment } from '../entities/workflow-review-activity-comment.ee'; @@ -13,6 +17,22 @@ export type WorkflowReviewActivityFeedEntry = { messages: WorkflowReviewActivityComment[]; }; +/** + * Write-side counterpart of `WorkflowReviewActivityEntry`, reusing its data types so the read + * and write shapes cannot drift. Repository input only, never serialized, hence not in + * `@n8n/api-types`. `changes_requested` and `approved` share a member on purpose, so a + * conditional decision type still typechecks at the call site. + */ +export type WorkflowReviewActivityPayload = + | { type: 'review.opened'; data: WorkflowReviewOpenedActivityData } + | { type: 'comment.created' | 'workflow.published'; data: null } + | { + type: 'review.changes_requested' | 'review.approved'; + data: WorkflowReviewDecisionActivityData; + } + | { type: 'review.version_updated'; data: WorkflowReviewVersionUpdatedActivityData } + | { type: 'review.closed'; data: WorkflowReviewClosedActivityData }; + @Service() export class WorkflowReviewActivityRepository extends BaseRepository { constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { @@ -20,11 +40,14 @@ export class WorkflowReviewActivityRepository extends BaseRepository { diff --git a/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts b/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts index 5f6a6a4a7d6..f3132ddf896 100644 --- a/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts @@ -1,9 +1,12 @@ +import type { WorkflowReviewClosedReason } from '@n8n/api-types'; import { Service } from '@n8n/di'; +import type { WorkflowSharingRole } from '@n8n/permissions'; import type { SelectQueryBuilder } from '@n8n/typeorm'; import { DataSource } from '@n8n/typeorm'; import { BaseRepository } from './base-repository'; import { SharedWorkflow } from '../entities/shared-workflow'; +import { WorkflowEntity } from '../entities/workflow-entity'; import { WorkflowReviewRequestAuthor } from '../entities/workflow-review-request-author.ee'; import { WorkflowReviewRequestReviewer } from '../entities/workflow-review-request-reviewer.ee'; import { WorkflowReviewRequestWorkflow } from '../entities/workflow-review-request-workflow.ee'; @@ -92,6 +95,44 @@ export type InboxStateCounts = { closed: number; }; +/** An open request the reconciliation sweep closed, and what made its workflow unreviewable. */ +export type ClosedUnreviewableRequest = { + id: string; + reason: WorkflowReviewClosedReason; +}; + +/** One row per (open request, linked workflow); a request with no link left yields one empty row. */ +type OpenRequestWorkflowRow = { + requestId: string; + requestProjectId: string; + linkedWorkflowId: string | null; + /** Raw, so the driver's own boolean: `1` on sqlite and mysql, `true` on postgres. */ + isArchived: boolean | number | null; + owningProjectId: string | null; +}; + +/** Most to least definitive: one workflow can be several of these at once. */ +const CLOSE_REASON_PRECEDENCE: WorkflowReviewClosedReason[] = [ + 'workflow-deleted', + 'workflow-archived', + 'workflow-moved', +]; + +function closeReasonFor(row: OpenRequestWorkflowRow): WorkflowReviewClosedReason | null { + // Nothing behind the link: either the request has no link row left, or it points at a + // workflow that is gone. Both mean the delete cascade got there first. + if (row.linkedWorkflowId === null) return 'workflow-deleted'; + + if (row.isArchived) return 'workflow-archived'; + + // A workflow with no owning project at all is a broken row, not a move — leave it alone. + if (row.owningProjectId !== null && row.owningProjectId !== row.requestProjectId) { + return 'workflow-moved'; + } + + return null; +} + @Service() export class WorkflowReviewRequestRepository extends BaseRepository { constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { @@ -139,44 +180,69 @@ export class WorkflowReviewRequestRepository extends BaseRepository { + async closeUnreviewableOpenRequests(ctx: OperationContext): Promise { const openState: WorkflowReviewRequestState = 'open'; const closedState: WorkflowReviewRequestState = 'closed'; + const ownerRole: WorkflowSharingRole = 'workflow:owner'; const manager = this.managerFor(ctx); - const orphans = await manager + const rows = await manager .createQueryBuilder(WorkflowReviewRequest, 'review') - .select('review.id', 'id') + .select('review.id', 'requestId') + .addSelect('review.projectId', 'requestProjectId') + .addSelect('workflow.id', 'linkedWorkflowId') + .addSelect('workflow.isArchived', 'isArchived') + .addSelect('shared.projectId', 'owningProjectId') + // Left joins throughout: a request with no link, or a link with no workflow, is + // precisely the orphan case, and dropping those rows would hide it. + .leftJoin(WorkflowReviewRequestWorkflow, 'link', 'link.workflowReviewRequestId = review.id') + .leftJoin(WorkflowEntity, 'workflow', 'workflow.id = link.workflowId') + .leftJoin( + SharedWorkflow, + 'shared', + 'shared.workflowId = link.workflowId AND shared.role = :ownerRole', + { ownerRole }, + ) .where('review.state = :openState', { openState }) - .andWhere((qb) => { - const linkedWorkflowExists = qb - .subQuery() - .select('1') - .from(WorkflowReviewRequestWorkflow, 'requestWorkflow') - .where('requestWorkflow.workflowReviewRequestId = review.id') - .getQuery(); - return `NOT EXISTS ${linkedWorkflowExists}`; - }) - .getRawMany<{ id: string }>(); + .getRawMany(); - if (orphans.length === 0) return []; + const reasonByRequestId = new Map(); + for (const row of rows) { + const reason = closeReasonFor(row); + if (reason === null) continue; + + // A request linked to several workflows gets one row each, so keep the most + // definitive reason rather than whichever row the database returned last. + const current = reasonByRequestId.get(row.requestId); + if ( + current !== undefined && + CLOSE_REASON_PRECEDENCE.indexOf(current) <= CLOSE_REASON_PRECEDENCE.indexOf(reason) + ) { + continue; + } + reasonByRequestId.set(row.requestId, reason); + } + + if (reasonByRequestId.size === 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, { + await manager.update(WorkflowReviewRequest, [...reasonByRequestId.keys()], { state: closedState, closedById: null, }); - return ids; + return [...reasonByRequestId].map(([id, reason]) => ({ id, reason })); } async findById(id: string, ctx: OperationContext): Promise { diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-activity.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-activity.integration.test.ts index 7d2e598716c..119c4be6b16 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-activity.integration.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-activity.integration.test.ts @@ -352,6 +352,244 @@ describe('Commenting on a review', () => { }); }); +describe('Recording the review lifecycle in the feed', () => { + /** Two history versions so the review can be re-pinned from one to the other. */ + async function createReviewableWorkflow() { + const workflow = await createWorkflow({ name: 'Reviewed workflow' }, teamProject); + await createWorkflowHistoryItem(workflow.id, { versionId: 'version-1' }); + await createWorkflowHistoryItem(workflow.id, { versionId: 'version-2' }); + return workflow; + } + + /** Opened by `member`, so `owner` is free to decide on it without an admin override. */ + async function openReview(workflowId: string, workflowVersionId = 'version-1') { + const response = await memberAgent + .post('/workflow-review-requests') + .send({ + title: 'Please review', + workflows: [{ workflowId, workflowVersionId, workflowVersionName: 'Release candidate' }], + reviewerUserIds: [owner.id], + }) + .expect(201); + return response.body.data.id as string; + } + + const entryTypes = (feed: { data: FeedEntry[] }) => feed.data.map((entry) => entry.type); + + test('shows who opened the review and which version they submitted', async () => { + const workflow = await createReviewableWorkflow(); + + const requestId = await openReview(workflow.id); + + const feed = await getActivity(memberAgent, requestId); + expect(feed.data).toHaveLength(1); + expect(feed.data[0]).toMatchObject({ + type: 'review.opened', + typeVersion: 1, + createdBy: expect.objectContaining({ id: member.id }), + }); + expect(feed.data[0].data).toEqual({ + workflowVersions: [{ workflowId: workflow.id, workflowVersionId: 'version-1' }], + }); + }); + + test('records the note and the reviewed version when a reviewer requests changes', async () => { + const workflow = await createReviewableWorkflow(); + const requestId = await openReview(workflow.id); + + await ownerAgent + .post(`/workflow-review-requests/${requestId}/decision`) + .send({ decision: 'changes_requested', note: 'Please rename the node' }) + .expect(200); + + const feed = await getActivity(ownerAgent, requestId); + expect(entryTypes(feed)).toEqual(['review.opened', 'review.changes_requested']); + expect(feed.data[1]).toMatchObject({ + createdBy: expect.objectContaining({ id: owner.id }), + }); + expect(feed.data[1].data).toEqual({ + workflowVersions: [{ workflowId: workflow.id, workflowVersionId: 'version-1' }], + note: 'Please rename the node', + }); + }); + + test('refuses to request changes without a note, and records nothing', async () => { + const workflow = await createReviewableWorkflow(); + const requestId = await openReview(workflow.id); + + await ownerAgent + .post(`/workflow-review-requests/${requestId}/decision`) + .send({ decision: 'changes_requested' }) + .expect(400); + + expect(entryTypes(await getActivity(ownerAgent, requestId))).toEqual(['review.opened']); + expect(await requestRepository.findById(requestId, {})).toMatchObject({ + state: 'open', + decision: 'pending', + }); + }); + + test('records the note a reviewer leaves when they approve', async () => { + const workflow = await createReviewableWorkflow(); + const requestId = await openReview(workflow.id); + + await ownerAgent + .post(`/workflow-review-requests/${requestId}/decision`) + .send({ decision: 'approved', note: 'Ships as is' }) + .expect(200); + + const feed = await getActivity(ownerAgent, requestId); + expect(entryTypes(feed)).toEqual(['review.opened', 'review.approved']); + expect(feed.data[1].data).toEqual({ + workflowVersions: [{ workflowId: workflow.id, workflowVersionId: 'version-1' }], + note: 'Ships as is', + }); + }); + + test('records an approval left without a note as having none', async () => { + const workflow = await createReviewableWorkflow(); + const requestId = await openReview(workflow.id); + + await ownerAgent + .post(`/workflow-review-requests/${requestId}/decision`) + .send({ decision: 'approved' }) + .expect(200); + + const feed = await getActivity(ownerAgent, requestId); + // `null`, not a missing key: "no note given" and "the payload did not parse" are + // different things on an audit record. + expect(feed.data[1].data).toEqual({ + workflowVersions: [{ workflowId: workflow.id, workflowVersionId: 'version-1' }], + note: null, + }); + }); + + test('still names the workflow an approved version came from after that workflow is deleted', async () => { + const workflow = await createReviewableWorkflow(); + const requestId = await openReview(workflow.id); + + await ownerAgent + .post(`/workflow-review-requests/${requestId}/decision`) + .send({ decision: 'approved' }) + .expect(200); + + await workflowEntityRepository.delete(workflow.id); + + // The pin cascades away with the workflow, so the entry is the only record left of which + // version was approved. + expect(await workflowRepository.findByRequestId(requestId, {})).toEqual([]); + + const feed = await getActivity(ownerAgent, requestId); + expect(feed.data[1].data).toEqual({ + workflowVersions: [{ workflowId: workflow.id, workflowVersionId: 'version-1' }], + note: null, + }); + }); + + test('records the version a re-pinned review moved from and to, scoped to that workflow', async () => { + const workflow = await createReviewableWorkflow(); + const requestId = await openReview(workflow.id); + + await memberAgent + .post(`/workflow-review-requests/${requestId}/update-version`) + .send({ + workflowId: workflow.id, + workflowVersionId: 'version-2', + workflowVersionName: 'Second attempt', + }) + .expect(200); + + const feed = await getActivity(memberAgent, requestId); + expect(entryTypes(feed)).toEqual(['review.opened', 'review.version_updated']); + expect(feed.data[1].data).toEqual({ + workflowId: workflow.id, + fromWorkflowVersionId: 'version-1', + toWorkflowVersionId: 'version-2', + }); + + // The scoping column stays unwritten: its FK cascades, so a workflow delete would take + // the entry with it. + const rows = await activityRepository.findBy({ type: 'review.version_updated' }); + expect(rows).toHaveLength(1); + expect(rows[0].workflowId).toBeNull(); + }); + + test('keeps a version update in the feed after its workflow is deleted', async () => { + const workflow = await createReviewableWorkflow(); + const requestId = await openReview(workflow.id); + + await memberAgent + .post(`/workflow-review-requests/${requestId}/update-version`) + .send({ + workflowId: workflow.id, + workflowVersionId: 'version-2', + workflowVersionName: 'Second attempt', + }) + .expect(200); + + // Straight from the repository, as a folder-hierarchy cascade does, so nothing but the + // database's own cascades decides what survives. + await workflowEntityRepository.delete(workflow.id); + + const feed = await getActivity(memberAgent, requestId); + expect(entryTypes(feed)).toEqual(['review.opened', 'review.version_updated']); + expect(feed.data[1].data).toMatchObject({ workflowId: workflow.id }); + }); + + test('records nothing when a review is re-pinned to the version it already covers', async () => { + const workflow = await createReviewableWorkflow(); + const requestId = await openReview(workflow.id); + + await memberAgent + .post(`/workflow-review-requests/${requestId}/update-version`) + .send({ + workflowId: workflow.id, + workflowVersionId: 'version-1', + workflowVersionName: 'Renamed only', + }) + .expect(200); + + expect(entryTypes(await getActivity(memberAgent, requestId))).toEqual(['review.opened']); + }); + + test('leaves the review undecided when its feed entry cannot be written', async () => { + const workflow = await createReviewableWorkflow(); + const requestId = await openReview(workflow.id); + vi.spyOn(activityRepository, 'createActivity').mockRejectedValueOnce(new Error('write failed')); + + await ownerAgent + .post(`/workflow-review-requests/${requestId}/decision`) + .send({ decision: 'approved' }) + .expect(500); + + expect(await requestRepository.findById(requestId, {})).toMatchObject({ + state: 'open', + decision: 'pending', + }); + expect(entryTypes(await getActivity(ownerAgent, requestId))).toEqual(['review.opened']); + }); + + // Only a stored row can be malformed, so these are seeded past the write union. + test.each([ + ['a decision whose payload is not an object', 'review.changes_requested', 'oops'], + ['a close with a reason this version does not know', 'review.closed', { reason: 'nope' }], + ['a version update missing its version ids', 'review.version_updated', { from: 1 }], + ['an opening entry written in an older shape', 'review.opened', { index: 1 }], + ])('serves %s without its details rather than failing the feed', async (_label, type, data) => { + const { request } = await seedReviewInTeamProject(owner); + await activityRepository.createActivity( + { workflowReviewRequestId: request.id, type, data, createdById: owner.id } as never, + {}, + ); + + const feed = await getActivity(ownerAgent, request.id); + + expect(feed.data).toHaveLength(1); + expect(feed.data[0].type).toBe(type); + expect(feed.data[0].data).toBeNull(); + }); +}); + describe('Reading the activity feed', () => { /** Non-comment entries, cheap to seed and enough to pin the paging arithmetic. */ async function seedEntries(workflowReviewRequestId: string, count: number) { @@ -361,7 +599,7 @@ describe('Reading the activity feed', () => { { workflowReviewRequestId, type: 'review.opened', - data: { index }, + data: { workflowVersions: [] }, createdById: owner.id, }, {}, @@ -502,7 +740,10 @@ describe('Reading the activity feed', () => { test('shows a non-comment activity entry with its details intact and no messages', async () => { const { request } = await seedReviewInTeamProject(owner); - const data = { workflowVersionIds: ['version-pinned'], note: 'needs work' }; + const data = { + workflowVersions: [{ workflowId: 'workflow-reviewed', workflowVersionId: 'version-pinned' }], + note: 'needs work', + }; await activityRepository.createActivity( { workflowReviewRequestId: request.id, diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.integration.test.ts index bd3a891fbcb..85a83db5f94 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.integration.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.integration.test.ts @@ -15,6 +15,7 @@ import { SharedWorkflowRepository, UserRepository, WorkflowRepository, + WorkflowReviewActivityRepository, WorkflowReviewRequestAuthorRepository, WorkflowReviewRequestRepository, WorkflowReviewRequestWorkflowRepository, @@ -62,12 +63,14 @@ let ownerAgent: SuperAgentTest; let requestRepository: WorkflowReviewRequestRepository; let linkRepository: WorkflowReviewRequestWorkflowRepository; let authorRepository: WorkflowReviewRequestAuthorRepository; +let activityRepository: WorkflowReviewActivityRepository; beforeAll(async () => { await utils.initNodeTypes(); requestRepository = Container.get(WorkflowReviewRequestRepository); linkRepository = Container.get(WorkflowReviewRequestWorkflowRepository); authorRepository = Container.get(WorkflowReviewRequestAuthorRepository); + activityRepository = Container.get(WorkflowReviewActivityRepository); }); beforeEach(async () => { @@ -75,6 +78,8 @@ beforeEach(async () => { testServer.license.enable('feat:workflowReviews'); await testDb.truncate([ + 'WorkflowReviewActivityComment', + 'WorkflowReviewActivity', 'WorkflowReviewRequestAuthor', 'WorkflowReviewRequestReviewer', 'WorkflowReviewRequestWorkflow', @@ -98,10 +103,18 @@ beforeEach(async () => { ownerAgent = testServer.authAgentFor(owner); }); +/** Read through the endpoint, so the entries are asserted exactly as a reader sees them. */ +async function getActivityEntries(requestId: string) { + const response = await ownerAgent + .get(`/workflow-review-requests/${requestId}/activity`) + .expect(200); + return response.body.data.data as Array<{ type: string; data: { reason?: string } | null }>; +} + /** Create a workflow owned by `owner` with a pinned history version. */ -async function createReviewableWorkflow() { +async function createReviewableWorkflow(attributes: { isArchived?: boolean } = {}) { const versionId = uuid(); - const workflow = await createWorkflow({ versionId }, owner); + const workflow = await createWorkflow({ versionId, ...attributes }, owner); await createWorkflowHistoryItem(workflow.id, { versionId }); return { workflow, versionId }; } @@ -146,6 +159,54 @@ describe('auto-close on workflow archive', () => { expect(closed?.decision).toBe('changes_requested'); expect(closed?.closedById).toBeNull(); expect(closed?.approvedAt).toBeNull(); + + expect(await getActivityEntries(request.id)).toEqual([ + expect.objectContaining({ + type: 'review.closed', + data: { reason: 'workflow-archived' }, + }), + ]); + }); + + // The close and its explanation share one transaction, so an unwritable entry rolls the + // close back — and archiving still succeeds, because cleanup never fails the mutation. The + // reconciliation sweep that follows the hook picks the rolled-back review straight back up. + test('archives the workflow anyway when the review cannot be explained, and the sweep closes it', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await createOpenReview(workflow.id, versionId); + vi.spyOn(activityRepository, 'createActivity').mockRejectedValueOnce(new Error('write failed')); + + await ownerAgent.post(`/workflows/${workflow.id}/archive`).expect(200); + + expect((await requestRepository.findById(request.id, {}))?.state).toBe('closed'); + expect(await getActivityEntries(request.id)).toEqual([ + expect.objectContaining({ type: 'review.closed', data: { reason: 'workflow-archived' } }), + ]); + }); + + // Both the targeted close and the sweep that follows it are down, so the review is stranded + // open on a workflow that is already archived — the state the next sweep has to repair. + test('a review stranded open on an archived workflow is closed by the next sweep', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await createOpenReview(workflow.id, versionId); + vi.spyOn(activityRepository, 'createActivity') + .mockRejectedValueOnce(new Error('write failed')) + .mockRejectedValueOnce(new Error('write failed')); + + await ownerAgent.post(`/workflows/${workflow.id}/archive`).expect(200); + expect((await requestRepository.findById(request.id, {}))?.state).toBe('open'); + + // Any later lifecycle mutation runs the sweep again — this one touches an unrelated + // workflow, so only the sweep can reach the stranded review. + const unrelated = await createReviewableWorkflow(); + await ownerAgent.post(`/workflows/${unrelated.workflow.id}/archive`).expect(200); + + const closed = await requestRepository.findById(request.id, {}); + expect(closed?.state).toBe('closed'); + expect(closed?.closedById).toBeNull(); + expect(await getActivityEntries(request.id)).toEqual([ + expect.objectContaining({ type: 'review.closed', data: { reason: 'workflow-archived' } }), + ]); }); test('unarchiving does not reopen the review, and the workflow is no longer publish-blocked', async () => { @@ -192,6 +253,48 @@ describe('auto-close on workflow transfer', () => { expect(closed?.state).toBe('closed'); expect(closed?.decision).toBe('pending'); expect(await requestRepository.findOpenRequestForWorkflow(workflow.id, {})).toBeNull(); + + expect(await getActivityEntries(request.id)).toEqual([ + expect.objectContaining({ type: 'review.closed', data: { reason: 'workflow-moved' } }), + ]); + }); + + // The move stays committed when its close rolls back, leaving the review open on a workflow + // that now belongs to another project — the sweep still has the link row to find it by. + test('a review stranded open on a moved workflow is closed by the next sweep', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await createOpenReview(workflow.id, versionId); + const destination = await createTeamProject('Destination', owner); + vi.spyOn(activityRepository, 'createActivity') + .mockRejectedValueOnce(new Error('write failed')) + .mockRejectedValueOnce(new Error('write failed')); + + await Container.get(EnterpriseWorkflowService).transferWorkflow( + owner, + workflow.id, + destination.id, + ); + expect((await requestRepository.findById(request.id, {}))?.state).toBe('open'); + + const unrelated = await createReviewableWorkflow(); + await ownerAgent.post(`/workflows/${unrelated.workflow.id}/archive`).expect(200); + + const closed = await requestRepository.findById(request.id, {}); + expect(closed?.state).toBe('closed'); + expect(await linkRepository.findByRequestId(request.id, {})).toHaveLength(1); + expect(await getActivityEntries(request.id)).toEqual([ + expect.objectContaining({ type: 'review.closed', data: { reason: 'workflow-moved' } }), + ]); + }); + + test('a review whose workflow is still in its project and unarchived is left open', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await createOpenReview(workflow.id, versionId); + + const unrelated = await createReviewableWorkflow(); + await ownerAgent.post(`/workflows/${unrelated.workflow.id}/archive`).expect(200); + + expect((await requestRepository.findById(request.id, {}))?.state).toBe('open'); }); }); @@ -206,6 +309,26 @@ describe('auto-close on workflow hard delete', () => { 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); + + // The pre-delete hook and the post-delete sweep both run here, so the count matters + // as much as the reason: the review must be explained exactly once. + expect(await getActivityEntries(request.id)).toEqual([ + expect.objectContaining({ type: 'review.closed', data: { reason: 'workflow-deleted' } }), + ]); + }); + + // The pre-delete hook shares one transaction with its explanation and rethrows, so an + // unwritable entry calls the delete off rather than leaving a closed review unexplained. + test('refuses to delete the workflow when the review cannot be explained', async () => { + const { workflow, versionId } = await createReviewableWorkflow({ isArchived: true }); + const request = await createOpenReview(workflow.id, versionId); + vi.spyOn(activityRepository, 'createActivity').mockRejectedValueOnce(new Error('write failed')); + + await ownerAgent.delete(`/workflows/${workflow.id}`).expect(500); + + expect(await Container.get(WorkflowRepository).findOneBy({ id: workflow.id })).not.toBeNull(); + expect((await requestRepository.findById(request.id, {}))?.state).toBe('open'); + expect(await getActivityEntries(request.id)).toEqual([]); }); // A review opened after the pre-delete hook ran loses its link row to the cascade and @@ -220,12 +343,18 @@ describe('auto-close on workflow hard delete', () => { expect((await requestRepository.findById(request.id, {}))?.state).toBe('open'); expect(await linkRepository.findByRequestId(request.id, {})).toHaveLength(0); + // `unrelated` has no review of its own, so the pre-delete hook finds nothing to close + // and the sweep is the only thing that can explain the orphaned review. 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(); + + expect(await getActivityEntries(request.id)).toEqual([ + expect.objectContaining({ type: 'review.closed', data: { reason: 'workflow-deleted' } }), + ]); }); test('leaves a review whose workflow still exists open', async () => { diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.service.test.ts index 543067cc2ee..c279a649ff5 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-auto-close.service.test.ts @@ -1,8 +1,11 @@ import type { Logger } from '@n8n/backend-common'; +import { DbLock } from '@n8n/db'; import type { DbLockService, OperationContext, Transaction, + WorkflowReviewActivity, + WorkflowReviewActivityRepository, WorkflowReviewRequest, WorkflowReviewRequestRepository, } from '@n8n/db'; @@ -15,6 +18,7 @@ import { WorkflowReviewAutoCloseService } from '../workflow-review-auto-close.se describe('WorkflowReviewAutoCloseService', () => { const logger = mock(); const requestRepository = mock(); + const activityRepository = mock(); const dbLockService = mock(); const collaborationService = mock(); /** The lock's context. Distinct from the root `{}` so tests can tell the two apart. */ @@ -23,6 +27,7 @@ describe('WorkflowReviewAutoCloseService', () => { const service = new WorkflowReviewAutoCloseService( logger, requestRepository, + activityRepository, dbLockService, collaborationService, ); @@ -40,6 +45,7 @@ describe('WorkflowReviewAutoCloseService', () => { vi.resetAllMocks(); dbLockService.withLockContext.mockImplementation(async (_id, fn) => await fn(ctx)); requestRepository.saveRequest.mockImplementation(async (request) => request); + requestRepository.closeUnreviewableOpenRequests.mockResolvedValue([]); collaborationService.broadcastWorkflowReviewStateChanged.mockResolvedValue(undefined); }); @@ -132,38 +138,95 @@ describe('WorkflowReviewAutoCloseService', () => { expect(logger.error).toHaveBeenCalled(); }); - describe('afterWorkflowsDeleted', () => { - it('closes the requests the delete orphaned, outside any lock', async () => { - requestRepository.closeOrphanedOpenRequests.mockResolvedValue(['req-9']); + describe('reconciliation sweep', () => { + it('closes the requests the delete orphaned and explains each of them', async () => { + requestRepository.closeUnreviewableOpenRequests.mockResolvedValue([ + { id: 'req-9', reason: 'workflow-deleted' }, + ]); + activityRepository.createActivity.mockResolvedValue(mock()); await service.afterWorkflowsDeleted(['wf-1', 'wf-2']); - // 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. - // The sweep is global, so the batch never reaches the query; it is log context only. - expect(requestRepository.closeOrphanedOpenRequests).toHaveBeenCalledExactlyOnceWith({}); - expect(dbLockService.withLockContext).not.toHaveBeenCalled(); + // Same lock and same transaction as every other close path: the sweep updates the + // requests it selected by id, so two racing sweeps would otherwise explain the same + // close twice. The sweep is global, so the batch never reaches the query; it is log + // context only. + expect(dbLockService.withLockContext).toHaveBeenCalledWith( + DbLock.WORKFLOW_REVIEW_REQUEST_CREATE, + expect.any(Function), + ); + expect(requestRepository.closeUnreviewableOpenRequests).toHaveBeenCalledExactlyOnceWith(ctx); + expect(activityRepository.createActivity).toHaveBeenCalledExactlyOnceWith( + { + workflowReviewRequestId: 'req-9', + type: 'review.closed', + data: { reason: 'workflow-deleted' }, + createdById: null, + }, + ctx, + ); expect(logger.info).toHaveBeenCalledExactlyOnceWith( expect.any(String), expect.objectContaining({ - reason: 'workflow-deleted', workflowIds: ['wf-1', 'wf-2'], - workflowReviewRequestIds: ['req-9'], + closedRequests: [{ id: 'req-9', reason: 'workflow-deleted' }], }), ); }); - it('stays quiet when the delete orphaned nothing', async () => { - requestRepository.closeOrphanedOpenRequests.mockResolvedValue([]); + // The reason is per request, not per mutation: one sweep can find a review left by an + // archive next to one left by a move. + it('explains each request with the reason the sweep reported for it', async () => { + requestRepository.closeUnreviewableOpenRequests.mockResolvedValue([ + { id: 'req-1', reason: 'workflow-archived' }, + { id: 'req-2', reason: 'workflow-moved' }, + ]); + activityRepository.createActivity.mockResolvedValue(mock()); + + await service.afterWorkflowsDeleted(['wf-1']); + + expect(activityRepository.createActivity).toHaveBeenCalledWith( + expect.objectContaining({ + workflowReviewRequestId: 'req-1', + data: { reason: 'workflow-archived' }, + }), + ctx, + ); + expect(activityRepository.createActivity).toHaveBeenCalledWith( + expect.objectContaining({ + workflowReviewRequestId: 'req-2', + data: { reason: 'workflow-moved' }, + }), + ctx, + ); + }); + + it('stays quiet when nothing is left unreviewable', async () => { + requestRepository.closeUnreviewableOpenRequests.mockResolvedValue([]); await service.afterWorkflowsDeleted(['wf-1']); expect(logger.info).not.toHaveBeenCalled(); }); + // The close and its explanation share one transaction, so an unwritable entry rolls the + // close back and the next sweep picks the review up again. The delete already committed, + // so there is nothing left to fail. + it('leaves a review it cannot explain to the next sweep', async () => { + requestRepository.closeUnreviewableOpenRequests.mockResolvedValue([ + { id: 'req-9', reason: 'workflow-deleted' }, + ]); + activityRepository.createActivity.mockRejectedValue(new Error('db down')); + + await expect(service.afterWorkflowsDeleted(['wf-1'])).resolves.toBeUndefined(); + + expect(logger.error).toHaveBeenCalled(); + 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')); + requestRepository.closeUnreviewableOpenRequests.mockRejectedValue(new Error('db down')); await expect(service.afterWorkflowsDeleted(['wf-1', 'wf-2'])).resolves.toBeUndefined(); @@ -172,6 +235,37 @@ describe('WorkflowReviewAutoCloseService', () => { expect.objectContaining({ workflowIds: ['wf-1', 'wf-2'] }), ); }); + + // An archive or a move whose close rolled back stays committed, so the sweep has to run + // there too — the targeted close is done with the review by then. + it('runs after the targeted close on archive, and again on transfer', async () => { + requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([]); + + await service.afterWorkflowArchived('wf-1'); + expect(requestRepository.closeUnreviewableOpenRequests).toHaveBeenCalledExactlyOnceWith(ctx); + + await service.afterWorkflowsTransferred(['wf-2']); + expect(requestRepository.closeUnreviewableOpenRequests).toHaveBeenCalledTimes(2); + }); + + // A close that rolled back is exactly what the sweep is there to repair, so a throwing + // targeted close must not skip it. + it('still runs when the targeted close on archive failed', async () => { + requestRepository.findOpenRequestsForWorkflows.mockRejectedValue(new Error('db down')); + + await expect(service.afterWorkflowArchived('wf-1')).resolves.toBeUndefined(); + + expect(requestRepository.closeUnreviewableOpenRequests).toHaveBeenCalledExactlyOnceWith(ctx); + }); + + // The pre-delete hook aborts the delete instead, so there is nothing to reconcile. + it('does not run before a delete', async () => { + requestRepository.findOpenRequestsForWorkflows.mockResolvedValue([]); + + await service.beforeWorkflowDeleted('wf-1'); + + expect(requestRepository.closeUnreviewableOpenRequests).not.toHaveBeenCalled(); + }); }); it('a failed broadcast is only warned about, never thrown', async () => { diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.decide.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.decide.service.test.ts index dbb804df1de..677f4a13b42 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.decide.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.decide.service.test.ts @@ -2,6 +2,7 @@ import type { DecideWorkflowReviewRequestDto } from '@n8n/api-types'; import type { LicenseState, Logger } from '@n8n/backend-common'; import type { DbLockService, + Project, ProjectRelationRepository, SharedWorkflowRepository, User, @@ -12,6 +13,7 @@ import type { WorkflowReviewRequest, WorkflowReviewRequestAuthorRepository, WorkflowReviewRequestRepository, + WorkflowReviewActivityRepository, WorkflowReviewRequestReviewerRepository, WorkflowReviewRequestWorkflow, WorkflowReviewRequestWorkflowRepository, @@ -23,6 +25,7 @@ import { DbLock } from '@n8n/db'; import { mock } from 'vitest-mock-extended'; import type { CollaborationService } from '@/collaboration/collaboration.service'; +import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { ConflictError } from '@/errors/response-errors/conflict.error'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; @@ -46,19 +49,24 @@ const requesterUser = mock({ const requestId = 'req-1'; const projectId = 'proj-1'; const approveDto: DecideWorkflowReviewRequestDto = { decision: 'approved' }; -const requestChangesDto: DecideWorkflowReviewRequestDto = { decision: 'changes_requested' }; +const requestChangesDto: DecideWorkflowReviewRequestDto = { + decision: 'changes_requested', + note: 'Please rename the node', +}; describe('WorkflowReviewRequestService.decide', () => { const workflowReviewPolicyService = mock(); const workflowFinderService = mock(); const workflowHistoryService = mock(); const workflowHistoryRepository = mock(); + const workflowEntityRepository = mock(); const sharedWorkflowRepository = mock(); const publishHistoryRepository = mock(); const requestRepository = mock(); const workflowRepository = mock(); const authorRepository = mock(); const reviewerRepository = mock(); + const activityRepository = mock(); const userRepository = mock(); const projectRelationRepository = mock(); const roleService = mock(); @@ -76,13 +84,14 @@ describe('WorkflowReviewRequestService.decide', () => { workflowFinderService, workflowHistoryService, workflowHistoryRepository, - mock(), + workflowEntityRepository, sharedWorkflowRepository, publishHistoryRepository, requestRepository, workflowRepository, authorRepository, reviewerRepository, + activityRepository, userRepository, // Real service over the same mocks, so the override assertions below // exercise the actual eligibility logic decide() shares with the read side. @@ -125,6 +134,10 @@ describe('WorkflowReviewRequestService.decide', () => { workflowFinderService.findWorkflowForUser.mockResolvedValue( mock({ isArchived: false }), ); + workflowEntityRepository.findArchivedState.mockResolvedValue({ isArchived: false }); + sharedWorkflowRepository.getWorkflowOwningProject.mockResolvedValue( + mock({ id: projectId }), + ); authorRepository.isAuthor.mockResolvedValue(false); reviewerRepository.isReviewer.mockResolvedValue(true); projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]); @@ -231,6 +244,18 @@ describe('WorkflowReviewRequestService.decide', () => { expect(dbLockService.withLockContext).not.toHaveBeenCalled(); }); + // The missing note is a payload problem, and an author is not entitled to hear about it: + // they may not decide at all, whatever they sent. + it('tells an author they may not decide even when their note is missing too', async () => { + mockSuccessfulDecidePath(); + authorRepository.isAuthor.mockResolvedValue(true); + projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]); + + await expect( + service.decide(memberUser(), requestId, { decision: 'changes_requested' }), + ).rejects.toThrow(ForbiddenError); + }); + it('rejects a caller who became an author while waiting for the lock', async () => { mockSuccessfulDecidePath(); // Not an author before the lock, but a version sync won the lock first and @@ -392,6 +417,27 @@ describe('WorkflowReviewRequestService.decide', () => { expect(collaborationService.broadcastWorkflowReviewStateChanged).not.toHaveBeenCalled(); }); + it('refuses to approve a workflow archived while the decision waited for the lock', async () => { + mockSuccessfulDecidePath(); + // The pre-lock lookups still see a live workflow; only the in-lock re-read + // sees the archive that committed while this decision queued. + workflowEntityRepository.findArchivedState.mockResolvedValue({ isArchived: true }); + + const decision = service.decide(memberUser(), requestId, approveDto); + await expect(decision).rejects.toThrow(BadRequestError); + // The reviewer is told their review was refused, not that the workflow + // "cannot be submitted for review" — that is the author's action, not theirs. + await expect(decision).rejects.toThrow( + "The workflow 'wf-1' is archived and cannot be reviewed", + ); + + // Nothing may reach the activity feed: an approval entry here would durably + // assert a decision on a workflow that had already left the reviewable state. + expect(activityRepository.createActivity).not.toHaveBeenCalled(); + expect(requestRepository.saveRequest).not.toHaveBeenCalled(); + expect(workflowService.activateWorkflow).not.toHaveBeenCalled(); + }); + it('reports and publishes the version re-pinned by a concurrent sync that won the lock', async () => { mockSuccessfulDecidePath(); workflowRepository.findByRequestId @@ -549,6 +595,15 @@ describe('WorkflowReviewRequestService.decide', () => { status: 'failed', message: 'The reviewed workflow version no longer exists', }); + // A `[null]` here is rejected on read, which would take the reviewer's note down + // with it and leave an approval nobody can account for. + expect(activityRepository.createActivity).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'review.approved', + data: { workflowVersions: [], note: null }, + }), + ctx, + ); }); }); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts index 02195eea0c1..69515ac0c38 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts @@ -18,6 +18,7 @@ import type { WorkflowReviewRequestAuthorRepository, WorkflowReviewRequestRepository, WorkflowReviewRequestForWorkflowRow, + WorkflowReviewActivityRepository, WorkflowReviewRequestReviewerRepository, WorkflowReviewRequestWorkflowRepository, WorkflowRepository, @@ -71,6 +72,7 @@ describe('WorkflowReviewRequestService', () => { const workflowRepository = mock(); const authorRepository = mock(); const reviewerRepository = mock(); + const activityRepository = mock(); const userRepository = mock(); const eligibilityService = mock(); const roleService = mock(); @@ -95,6 +97,7 @@ describe('WorkflowReviewRequestService', () => { workflowRepository, authorRepository, reviewerRepository, + activityRepository, userRepository, eligibilityService, roleService, @@ -264,7 +267,11 @@ describe('WorkflowReviewRequestService', () => { mockSuccessfulCreatePath(); workflowEntityRepository.findArchivedState.mockResolvedValue({ isArchived: true }); - await expect(service.create(user, dto)).rejects.toThrow(BadRequestError); + const creation = service.create(user, dto); + await expect(creation).rejects.toThrow(BadRequestError); + await expect(creation).rejects.toThrow( + "The workflow 'wf-1' is archived and cannot be submitted for review", + ); expect(dbLockService.withLockContext).toHaveBeenCalled(); expect(requestRepository.createRequest).not.toHaveBeenCalled(); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.update.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.update.service.test.ts index a475c56b1a9..106e6bff067 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.update.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.update.service.test.ts @@ -3,6 +3,7 @@ import type { LicenseState, Logger } from '@n8n/backend-common'; import { DbLock } from '@n8n/db'; import type { DbLockService, + Project, SharedWorkflowRepository, User, UserRepository, @@ -13,6 +14,7 @@ import type { WorkflowReviewRequest, WorkflowReviewRequestAuthorRepository, WorkflowReviewRequestRepository, + WorkflowReviewActivityRepository, WorkflowReviewRequestReviewerRepository, WorkflowReviewRequestWorkflow, WorkflowReviewRequestWorkflowRepository, @@ -40,6 +42,7 @@ import { WorkflowReviewRequestService } from '../workflow-review-request.service const user = mock({ id: 'user-1' }); const requestId = 'req-1'; +const projectId = 'proj-1'; const dto: UpdateWorkflowReviewRequestVersionDto = { workflowId: 'wf-1', workflowVersionId: 'ver-2', @@ -51,12 +54,14 @@ describe('WorkflowReviewRequestService.updateVersion', () => { const workflowFinderService = mock(); const workflowHistoryService = mock(); const workflowHistoryRepository = mock(); + const workflowEntityRepository = mock(); const sharedWorkflowRepository = mock(); const publishHistoryRepository = mock(); const requestRepository = mock(); const workflowRepository = mock(); const authorRepository = mock(); const reviewerRepository = mock(); + const activityRepository = mock(); const userRepository = mock(); const eligibilityService = mock(); const roleService = mock(); @@ -74,13 +79,14 @@ describe('WorkflowReviewRequestService.updateVersion', () => { workflowFinderService, workflowHistoryService, workflowHistoryRepository, - mock(), + workflowEntityRepository, sharedWorkflowRepository, publishHistoryRepository, requestRepository, workflowRepository, authorRepository, reviewerRepository, + activityRepository, userRepository, eligibilityService, roleService, @@ -92,6 +98,7 @@ describe('WorkflowReviewRequestService.updateVersion', () => { const openRequest = (overrides: Partial = {}) => mock({ id: requestId, + projectId, state: 'open', decision: 'pending', description: null, @@ -112,6 +119,10 @@ describe('WorkflowReviewRequestService.updateVersion', () => { workflowFinderService.findWorkflowForUser.mockResolvedValue( mock({ isArchived: false }), ); + workflowEntityRepository.findArchivedState.mockResolvedValue({ isArchived: false }); + sharedWorkflowRepository.getWorkflowOwningProject.mockResolvedValue( + mock({ id: projectId }), + ); workflowHistoryService.findVersion.mockResolvedValue(mock()); workflowHistoryRepository.updateVersionMetadata.mockResolvedValue(1); requestRepository.saveRequest.mockImplementation(async (request) => request); @@ -303,6 +314,25 @@ describe('WorkflowReviewRequestService.updateVersion', () => { expect(collaborationService.broadcastWorkflowReviewStateChanged).not.toHaveBeenCalled(); }); + it('refuses to re-pin a workflow archived while the update waited for the lock', async () => { + mockSuccessfulUpdatePath(); + // The pre-lock lookups still see a live workflow; only the in-lock re-read + // sees the archive that committed while this update queued. + workflowEntityRepository.findArchivedState.mockResolvedValue({ isArchived: true }); + + const update = service.updateVersion(user, requestId, dto); + await expect(update).rejects.toThrow(BadRequestError); + await expect(update).rejects.toThrow( + "The workflow 'wf-1' is archived and cannot be submitted as a new review version", + ); + + // Nothing may reach the activity feed: a version_updated entry here would + // durably assert a re-pin on a workflow that can no longer be reviewed. + expect(activityRepository.createActivity).not.toHaveBeenCalled(); + expect(workflowRepository.updateWorkflowVersion).not.toHaveBeenCalled(); + expect(requestRepository.saveRequest).not.toHaveBeenCalled(); + }); + it('throws NotFoundError when the request disappears between check and lock', async () => { mockSuccessfulUpdatePath(); requestRepository.findById.mockResolvedValueOnce(openRequest()).mockResolvedValueOnce(null); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts index c62d3415cf5..e68fc21558f 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-requests.controller.integration.test.ts @@ -81,6 +81,8 @@ beforeEach(async () => { testServer.license.enable('feat:workflowReviews'); await testDb.truncate([ + 'WorkflowReviewActivityComment', + 'WorkflowReviewActivity', 'WorkflowReviewRequestAuthor', 'WorkflowReviewRequestReviewer', 'WorkflowReviewRequestWorkflow', @@ -1535,7 +1537,7 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', () const response = await memberAgent .post(`/workflow-review-requests/${request.id}/decision`) - .send({ decision: 'changes_requested' }) + .send({ decision: 'changes_requested', note: 'Please rename the node' }) .expect(200); expect(response.body.data).toMatchObject({ @@ -1576,7 +1578,7 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', () await memberAgent .post(`/workflow-review-requests/${request.id}/decision`) - .send({ decision: 'changes_requested' }) + .send({ decision: 'changes_requested', note: 'Please rename the node' }) .expect(200); expect(await requestRepository.findById(request.id, {})).toMatchObject({ @@ -1665,7 +1667,7 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', () await viewerAgent .post(`/workflow-review-requests/${request.id}/decision`) - .send({ decision: 'changes_requested' }) + .send({ decision: 'changes_requested', note: 'Please rename the node' }) .expect(200); expect(await requestRepository.findById(request.id, {})).toMatchObject({ diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review.mapper.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review.mapper.test.ts new file mode 100644 index 00000000000..6c02e82a92a --- /dev/null +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review.mapper.test.ts @@ -0,0 +1,49 @@ +import type { Logger } from '@n8n/backend-common'; +import type { WorkflowReviewActivity } from '@n8n/db'; +import { mock } from 'vitest-mock-extended'; + +import { toActivityEntry } from '../workflow-review.mapper'; + +describe('toActivityEntry', () => { + const logger = mock(); + + // The stored `type` is CHECK-constrained, so only a downgrade to a version that knows fewer + // types can produce this row — which is why it cannot be reached through the endpoints. + it('serves an entry of a type this version does not know rather than dropping it', () => { + const row = mock({ + id: 7, + typeVersion: 1, + type: 'review.something_newer' as WorkflowReviewActivity['type'], + data: { anything: true }, + createdById: null, + createdAt: new Date('2026-07-20T10:00:00.000Z'), + }); + + expect(toActivityEntry(row, [], new Map(), logger)).toEqual({ + id: '7', + typeVersion: 1, + type: 'review.something_newer', + data: null, + createdBy: null, + createdAt: '2026-07-20T10:00:00.000Z', + }); + }); + + // Same reason: only a downgrade writes a payload version this code does not know, and its + // shape may happen to satisfy the version 1 schema while meaning something else. + it('does not read a newer payload version as if it were the one it knows', () => { + const row = mock({ + id: 8, + typeVersion: 2, + type: 'review.approved', + data: { + workflowVersions: [{ workflowId: 'wf-1', workflowVersionId: 'ver-1' }], + note: 'Ship it', + }, + createdById: null, + createdAt: new Date('2026-07-20T10:00:00.000Z'), + }); + + expect(toActivityEntry(row, [], new Map(), logger).data).toBeNull(); + }); +}); diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-activity.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-activity.service.ts index 29d602998e0..4c75c4397f2 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-activity.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-activity.service.ts @@ -5,6 +5,7 @@ import type { WorkflowReviewActivityEntry, WorkflowReviewEligibleReviewer, } from '@n8n/api-types'; +import { Logger } from '@n8n/backend-common'; import { TransactionRunner, UserRepository, @@ -33,6 +34,7 @@ export class WorkflowReviewActivityService { private readonly activityCommentRepository: WorkflowReviewActivityCommentRepository, private readonly userRepository: UserRepository, private readonly txRunner: TransactionRunner, + private readonly logger: Logger, ) {} async listActivity( @@ -96,7 +98,12 @@ export class WorkflowReviewActivityService { return { activity, message }; }); - return toActivityEntry(activity, [message], new Map([[user.id, toEligibleReviewer(user)]])); + return toActivityEntry( + activity, + [message], + new Map([[user.id, toEligibleReviewer(user)]]), + this.logger, + ); } private async hydrate( @@ -109,7 +116,9 @@ export class WorkflowReviewActivityService { ]), ); - return entries.map((entry) => toActivityEntry(entry.activity, entry.messages, usersById)); + return entries.map((entry) => + toActivityEntry(entry.activity, entry.messages, usersById, this.logger), + ); } private async hydrateAuthors( diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-auto-close.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-auto-close.service.ts index 86448b32c0d..9107850403f 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-auto-close.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-auto-close.service.ts @@ -1,12 +1,16 @@ +import type { WorkflowReviewClosedReason } from '@n8n/api-types'; import { Logger } from '@n8n/backend-common'; -import { DbLock, DbLockService, WorkflowReviewRequestRepository } from '@n8n/db'; +import { + DbLock, + DbLockService, + WorkflowReviewActivityRepository, + 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. @@ -21,16 +25,19 @@ export class WorkflowReviewAutoCloseService implements WorkflowMutationHooks { constructor( private readonly logger: Logger, private readonly workflowReviewRequestRepository: WorkflowReviewRequestRepository, + private readonly activityRepository: WorkflowReviewActivityRepository, private readonly dbLockService: DbLockService, private readonly collaborationService: CollaborationService, ) {} async afterWorkflowArchived(workflowId: string): Promise { await this.closeOpenRequestsForWorkflows([workflowId], 'workflow-archived'); + await this.reconcileUnreviewableOpenRequests([workflowId]); } async afterWorkflowsTransferred(workflowIds: string[]): Promise { await this.closeOpenRequestsForWorkflows(workflowIds, 'workflow-moved'); + await this.reconcileUnreviewableOpenRequests(workflowIds); } async beforeWorkflowDeleted(workflowId: string): Promise { @@ -41,38 +48,71 @@ export class WorkflowReviewAutoCloseService implements WorkflowMutationHooks { }); } - /** - * Closes reviews whose workflow is gone: the cascade took their link rows, so they - * can only be found by "open with nothing linked", not by workflow id — the sweep is - * global and one call covers the whole batch. Catches reviews opened after the - * pre-delete hooks ran, and any left by a delete that skips the hooks. - */ async afterWorkflowsDeleted(workflowIds: string[]): Promise { + await this.reconcileUnreviewableOpenRequests(workflowIds); + } + + /** + * Closes every open review whose workflow is no longer reviewable, whatever left it that way. + * The sweep is global — one call covers the whole batch, and the ids are log context only. + * + * The per-mutation close above cannot cover two cases. A delete cascades the link rows away, + * so the reviews it strands are findable only as "open with nothing linked", never by workflow + * id. And an archive or a move whose close rolled back stays committed, leaving a review its + * own hook has already finished with — so the sweep runs after those hooks too, in its own + * transaction, and repairs the strand on that mutation or on the next one. + * + * No broadcast: the reviews it closes are a rare inconsistency rather than a live edit, and + * viewers heal on the next focus/reconnect refetch. + */ + private async reconcileUnreviewableOpenRequests(workflowIds: string[]): Promise { try { - const closedRequestIds = await this.workflowReviewRequestRepository.closeOrphanedOpenRequests( - {}, + const closedRequests = await this.dbLockService.withLockContext( + DbLock.WORKFLOW_REVIEW_REQUEST_CREATE, + async (ctx) => { + const requests = + await this.workflowReviewRequestRepository.closeUnreviewableOpenRequests(ctx); + + // Under the same lock and in the same transaction as every other close path: the + // sweep selects the requests and then updates them by id, so two sweeps racing + // across that gap would both explain the same close and both overwrite whatever a + // concurrent approval wrote. A failed entry rolls the close back and the next + // sweep closes it again, which is what the sweep is for. + for (const { id, reason } of requests) { + await this.activityRepository.createActivity( + { + workflowReviewRequestId: id, + type: 'review.closed', + data: { reason }, + createdById: null, + }, + ctx, + ); + } + + return requests; + }, ); - if (closedRequestIds.length === 0) return; + if (closedRequests.length === 0) return; - this.logger.info('Closed workflow review request(s) left without a workflow', { - reason: 'workflow-deleted', + this.logger.info('Closed open workflow review request(s) left on unreviewable workflows', { workflowIds, - workflowReviewRequestIds: closedRequestIds, + closedRequests, }); } catch (error) { - // The delete has already committed; failing it now would be worse than a + // The mutation has already committed; failing it now would be worse than a // request that stays open until the next sweep closes it. - this.logger.error('Failed to close workflow review request(s) left without a workflow', { - workflowIds, - error, - }); + this.logger.error( + 'Failed to close open workflow review request(s) left on unreviewable workflows', + { workflowIds, error }, + ); } } private async closeOpenRequestsForWorkflows( workflowIds: string[], - reason: AutoCloseReason, + reason: WorkflowReviewClosedReason, options: { rethrow?: boolean } = {}, ): Promise { try { @@ -93,6 +133,21 @@ export class WorkflowReviewAutoCloseService implements WorkflowMutationHooks { // A system close has no closing user; the decision stays as-is. request.closedById = null; await this.workflowReviewRequestRepository.saveRequest(request, ctx); + + // In the transaction on purpose: a review closed without an explanation is + // worse than a close that rolls back. Before a delete the rollback calls the + // delete off; for an archive or a move the mutation stands and the review + // stays open until the reconciliation sweep closes it again. + await this.activityRepository.createActivity( + { + workflowReviewRequestId: request.id, + type: 'review.closed', + data: { reason }, + createdById: null, + }, + ctx, + ); + for (const linkedWorkflowId of linkedWorkflowIds) affected.add(linkedWorkflowId); } @@ -121,8 +176,9 @@ export class WorkflowReviewAutoCloseService implements WorkflowMutationHooks { 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. + // Cleanup does not fail a mutation that already committed — archive and move end + // here, having rolled the close back. Only the pre-delete hook is still in a + // position to call its mutation off. if (options.rethrow) throw error; } } diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts index 836235ff9ea..7d467a9f1fb 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-request.service.ts @@ -24,6 +24,7 @@ import { WorkflowReviewRequestAuthorRepository, WorkflowReviewRequestRepository, WorkflowRepository, + WorkflowReviewActivityRepository, WorkflowReviewRequestReviewerRepository, WorkflowReviewRequestWorkflowRepository, type OperationContext, @@ -58,6 +59,15 @@ function normalizeReviewDescription(description: string | undefined): string | n return description.trim() || null; } +/** What the caller was trying to do, so a refusal names that action rather than another. */ +type BlockedAction = 'submit' | 'review' | 'update'; + +const BLOCKED_ACTION_TEXT: Record = { + submit: 'submitted for review', + review: 'reviewed', + update: 'submitted as a new review version', +}; + /** * The workflow-scoped review request lifecycle: listing a workflow's reviews, * resolving its eligible reviewers, opening a review, re-pinning it to a new @@ -79,6 +89,7 @@ export class WorkflowReviewRequestService { private readonly workflowReviewRequestWorkflowRepository: WorkflowReviewRequestWorkflowRepository, private readonly workflowReviewRequestAuthorRepository: WorkflowReviewRequestAuthorRepository, private readonly workflowReviewRequestReviewerRepository: WorkflowReviewRequestReviewerRepository, + private readonly activityRepository: WorkflowReviewActivityRepository, private readonly userRepository: UserRepository, private readonly eligibilityService: WorkflowReviewEligibilityService, private readonly roleService: RoleService, @@ -289,7 +300,7 @@ export class WorkflowReviewRequestService { } /** - * Re-asserts under the lock what `create`'s pre-lock checks established: the + * Re-asserts under the lock what the caller'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. @@ -302,7 +313,9 @@ export class WorkflowReviewRequestService { workflowId: string, expectedProjectId: string, ctx: OperationContext, + action: BlockedAction, ): Promise { + const blockedAction = BLOCKED_ACTION_TEXT[action]; const workflow = await this.workflowRepository.findArchivedState(workflowId, ctx); if (!workflow) { throw new NotFoundError('Could not find workflow'); @@ -310,14 +323,14 @@ export class WorkflowReviewRequestService { if (workflow.isArchived) { throw new BadRequestError( - `The workflow '${workflowId}' is archived and cannot be submitted for review`, + `The workflow '${workflowId}' is archived and cannot be ${blockedAction}`, ); } 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`, + `The workflow '${workflowId}' moved to another project and cannot be ${blockedAction} here`, 'Retry from the project that now owns the workflow', ); } @@ -384,7 +397,7 @@ export class WorkflowReviewRequestService { 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); + await this.assertWorkflowStillReviewable(workflowId, project.id, ctx, 'submit'); const existing = await this.workflowReviewRequestRepository.findOpenRequestForWorkflow( workflowId, @@ -430,6 +443,19 @@ export class WorkflowReviewRequestService { ctx, ); + // Records the opening version: the pin only ever holds the current one, and + // pruning spares it only while the review is open, so a re-pin or a close + // leaves the original prunable and its `SET NULL` erases the last trace. + await this.activityRepository.createActivity( + { + workflowReviewRequestId: created.id, + type: 'review.opened', + data: { workflowVersions: [{ workflowId, workflowVersionId }] }, + createdById: user.id, + }, + ctx, + ); + return created; }, ); @@ -547,6 +573,16 @@ export class WorkflowReviewRequestService { if (!currentRow) { throw new NotFoundError('Could not find review request'); } + + // Archive and transfer run in after hooks, so they commit before queueing + // on this lock — the pre-lock check can already be stale here. + await this.assertWorkflowStillReviewable( + currentRow.workflowId, + current.projectId, + ctx, + 'update', + ); + if (currentRow.workflowVersionId === dto.workflowVersionId) { // A concurrent sync won the lock and already re-pinned this version // but our rename or re-description can still be pending — apply it @@ -572,6 +608,9 @@ export class WorkflowReviewRequestService { return { request: saved, changed: true }; } + // Captured before the update. + const fromWorkflowVersionId = currentRow.workflowVersionId; + await this.workflowReviewRequestWorkflowRepository.updateWorkflowVersion( { workflowReviewRequestId, @@ -601,6 +640,20 @@ export class WorkflowReviewRequestService { ctx, ); + await this.activityRepository.createActivity( + { + workflowReviewRequestId, + type: 'review.version_updated', + data: { + workflowId: dto.workflowId, + fromWorkflowVersionId, + toWorkflowVersionId: dto.workflowVersionId, + }, + createdById: user.id, + }, + ctx, + ); + return { request: saved, changed: true }; }, ); @@ -680,6 +733,13 @@ export class WorkflowReviewRequestService { ? await this.resolveRequesterPublishability(request.createdById, workflowRow.workflowId) : null; + // After the lifecycle and permission checks: a state or permission error outranks a + // payload error, so an author keeps being told they may not decide at all rather than + // that their note was the problem. + if (dto.decision === 'changes_requested' && !dto.note) { + throw new BadRequestError('A note is required when requesting changes'); + } + const { request: saved, pinnedVersionId } = await this.dbLockService.withLockContext( DbLock.WORKFLOW_REVIEW_REQUEST_CREATE, async (ctx) => { @@ -719,6 +779,17 @@ export class WorkflowReviewRequestService { throw new NotFoundError('Could not find review request'); } + // Archive and transfer run in after hooks, so they commit before queueing + // on this lock — the pre-lock check can already be stale here. + // 'reviewed' covers both decisions: an approval and a change request + // are equally refused here. + await this.assertWorkflowStillReviewable( + currentRow.workflowId, + current.projectId, + ctx, + 'review', + ); + current.decision = dto.decision; current.updatedById = user.id; if (dto.decision === 'approved') { @@ -730,6 +801,25 @@ export class WorkflowReviewRequestService { } const savedRequest = await this.workflowReviewRequestRepository.saveRequest(current, ctx); + + await this.activityRepository.createActivity( + { + workflowReviewRequestId, + type: dto.decision === 'approved' ? 'review.approved' : 'review.changes_requested', + data: { + // A pruned pin drops out rather than landing as a null in the array. + workflowVersions: currentRows.flatMap((row) => + row.workflowVersionId === null + ? [] + : [{ workflowId: row.workflowId, workflowVersionId: row.workflowVersionId }], + ), + note: dto.note ?? null, + }, + createdById: user.id, + }, + ctx, + ); + return { request: savedRequest, pinnedVersionId: currentRow.workflowVersionId }; }, ); diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review.mapper.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review.mapper.ts index 824743e5f45..b88b83b0427 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review.mapper.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review.mapper.ts @@ -1,5 +1,14 @@ -import type { WorkflowReviewActivityEntry, WorkflowReviewEligibleReviewer } from '@n8n/api-types'; +import { + workflowReviewClosedActivityDataSchema, + workflowReviewDecisionActivityDataSchema, + workflowReviewOpenedActivityDataSchema, + workflowReviewVersionUpdatedActivityDataSchema, + type WorkflowReviewActivityEntry, + type WorkflowReviewEligibleReviewer, +} from '@n8n/api-types'; +import type { Logger } from '@n8n/backend-common'; import type { User, WorkflowReviewActivity, WorkflowReviewActivityComment } from '@n8n/db'; +import type { z } from 'zod'; /** * Project a user onto the boundary shape the review endpoints are allowed to expose. @@ -15,10 +24,36 @@ export function toEligibleReviewer(user: User): WorkflowReviewEligibleReviewer { }; } +/** + * A stored payload the schema rejects becomes `null` rather than an exception: one bad row must + * not take down the whole feed page. + */ +function parsePayload( + schema: z.ZodType, + row: WorkflowReviewActivity, + logger: Logger, +): T | null { + // These schemas describe typeVersion 1 only; a later version gets its own schema and case. + if (row.typeVersion === 1) { + const parsed = schema.safeParse(row.data); + if (parsed.success) { + return parsed.data; + } + } + + logger.warn('Failed to parse workflow review activity payload', { + activityId: row.id, + type: row.type, + typeVersion: row.typeVersion, + }); + return null; +} + export function toActivityEntry( row: WorkflowReviewActivity, messages: WorkflowReviewActivityComment[], usersById: Map, + logger: Logger, ): WorkflowReviewActivityEntry { const createdBy = row.createdById ? (usersById.get(row.createdById) ?? null) : null; const base = { @@ -28,23 +63,53 @@ export function toActivityEntry( createdAt: row.createdAt.toISOString(), }; - if (row.type === 'comment.created') { - return { - ...base, - type: row.type, - data: null, - messages: messages.map((message) => ({ - id: String(message.id), - // Never send the body of a deleted comment, even if the row still holds one, so a - // delete path that only sets `deletedAt` cannot leak it. - body: message.deletedAt ? null : message.body, - createdBy: message.createdById ? (usersById.get(message.createdById) ?? null) : null, - createdAt: message.createdAt.toISOString(), - updatedAt: message.updatedAt?.toISOString() ?? null, - deletedAt: message.deletedAt?.toISOString() ?? null, - })), - }; + switch (row.type) { + case 'comment.created': + return { + ...base, + type: row.type, + data: null, + messages: messages.map((message) => ({ + id: String(message.id), + // Never send the body of a deleted comment, even if the row still holds one, so a + // delete path that only sets `deletedAt` cannot leak it. + body: message.deletedAt ? null : message.body, + createdBy: message.createdById ? (usersById.get(message.createdById) ?? null) : null, + createdAt: message.createdAt.toISOString(), + updatedAt: message.updatedAt?.toISOString() ?? null, + deletedAt: message.deletedAt?.toISOString() ?? null, + })), + }; + case 'review.opened': + return { + ...base, + type: row.type, + data: parsePayload(workflowReviewOpenedActivityDataSchema, row, logger), + }; + case 'review.changes_requested': + case 'review.approved': + return { + ...base, + type: row.type, + data: parsePayload(workflowReviewDecisionActivityDataSchema, row, logger), + }; + case 'review.version_updated': + return { + ...base, + type: row.type, + data: parsePayload(workflowReviewVersionUpdatedActivityDataSchema, row, logger), + }; + case 'review.closed': + return { + ...base, + type: row.type, + data: parsePayload(workflowReviewClosedActivityDataSchema, row, logger), + }; + case 'workflow.published': + return { ...base, type: row.type, data: null }; } - return { ...base, type: row.type, data: row.data }; + // Unreachable per the union; a row whose type is outside it (e.g. after a version + // downgrade) must still render as an unknown entry rather than crash the feed. + return { ...base, type: row.type, data: null }; } diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index a2201fae663..ff3db93bfe7 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -5485,13 +5485,19 @@ "workflowReviews.decision.approved": "Approved", "workflowReviews.detail.tabs.activity": "Activity", "workflowReviews.detail.tabs.changes": "Changes", + "workflowReviews.detail.activity.description": "Description", "workflowReviews.detail.activity.noDescription": "No description provided.", "workflowReviews.detail.activity.listLabel": "Review activity", - "workflowReviews.detail.activity.empty.heading": "No activity yet", - "workflowReviews.detail.activity.empty.description": "Comments and review activity will appear here.", "workflowReviews.detail.activity.comment.deleted": "This comment was deleted.", "workflowReviews.detail.activity.unknownEntry": "This activity entry can't be displayed.", "workflowReviews.detail.activity.unknownAuthor": "Deleted user", + "workflowReviews.detail.activity.opened": "Review submitted", + "workflowReviews.detail.activity.changesRequested": "Requested changes", + "workflowReviews.detail.activity.approved": "Approved submission", + "workflowReviews.detail.activity.versionUpdated": "New workflow version submitted", + "workflowReviews.detail.activity.closed.archived": "Review closed because the workflow was archived", + "workflowReviews.detail.activity.closed.moved": "Review closed because the workflow moved to another project", + "workflowReviews.detail.activity.closed.deleted": "Review closed because the workflow was deleted", "workflowReviews.detail.activity.composer.label": "Add a comment", "workflowReviews.detail.activity.composer.placeholder": "Leave a comment...", "workflowReviews.detail.activity.composer.notAllowed": "You don't have permission to comment on this review.", @@ -5515,7 +5521,11 @@ "workflowReviews.changes.firstPublish.sourceEmpty": "This workflow hasn't been published before.", "workflowReviews.detail.notFound.title": "Review not found", "workflowReviews.detail.notFound.body": "This review doesn't exist or you don't have access to it.", - "workflowReviews.detail.decision.approve": "Approve", + "workflowReviews.detail.decision.trigger": "Review", + "workflowReviews.detail.decision.note.label": "Review note", + "workflowReviews.detail.decision.note.required": "Add a note to request changes", + "workflowReviews.detail.decision.comment": "Comment", + "workflowReviews.detail.decision.approveAndPublish": "Approve and publish", "workflowReviews.detail.decision.requestChanges": "Request changes", "workflowReviews.detail.decision.ineligible.author": "You contributed a version to this review.", "workflowReviews.emptyState.body.open": "There are no open reviews right now.", diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.test.ts index c4ad71ea5c1..860a9a6950e 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.test.ts @@ -8,10 +8,11 @@ import { useReviewActivityStore } from '../reviewActivity.store'; import WorkflowReviewActivityFeed from './WorkflowReviewActivityFeed.vue'; /** - * Scroll anchoring and the first-load scroll to bottom are deliberately untested: - * jsdom has no layout, so `scrollHeight` is always 0 and `getBoundingClientRect()` - * all zeroes. A test would have to stub those getters and then re-compute the - * production arithmetic from its own constants. Do not backfill a fake one. + * The scroll anchoring arithmetic is deliberately untested: jsdom has no layout, so + * `getBoundingClientRect()` is all zeroes and a test would have to stub those getters and then + * re-compute the production arithmetic from its own constants. Do not backfill a fake one. + * Which of the two behaviours a changed list picks — hold the reading position, or jump to the + * newest entry — is testable, and is what the reload cases below assert. */ const observer = vi.hoisted(() => ({ observe: vi.fn(), @@ -27,7 +28,9 @@ vi.mock('@/app/composables/useIntersectionObserver', () => ({ const renderComponent = createComponentRenderer(WorkflowReviewActivityFeed); -function makeEntry(overrides: Partial = {}) { +type CommentEntry = Extract; + +function makeComment(overrides: Partial = {}): WorkflowReviewActivityEntry { return { id: '1', type: 'comment.created', @@ -46,9 +49,17 @@ function makeEntry(overrides: Partial = {}) { }, ], ...overrides, - } as WorkflowReviewActivityEntry; + }; } +/** Every non-comment entry shares these fields; `type` and `data` are per row. */ +const systemEntry = { + id: '1', + typeVersion: 1, + createdBy: null, + createdAt: '2024-01-01T00:00:00.000Z', +}; + describe('WorkflowReviewActivityFeed', () => { let store: ReturnType>; @@ -66,7 +77,7 @@ describe('WorkflowReviewActivityFeed', () => { }); it('shows one entry per activity in the feed', async () => { - store.entries = Array.from({ length: 3 }, (_, index) => makeEntry({ id: String(index) })); + store.entries = Array.from({ length: 3 }, (_, index) => makeComment({ id: String(index) })); const { queryAllByTestId } = renderComponent(); await nextTick(); @@ -83,12 +94,6 @@ describe('WorkflowReviewActivityFeed', () => { expect(queryByTestId('workflow-review-activity-empty')).not.toBeInTheDocument(); }); - it('tells the viewer there is no activity yet', () => { - const { getByTestId } = renderComponent(); - - expect(getByTestId('workflow-review-activity-empty')).toHaveTextContent('No activity yet'); - }); - it('shows an error row when the first page failed', () => { store.error = new Error('boom'); @@ -112,7 +117,7 @@ describe('WorkflowReviewActivityFeed', () => { it('still reaches the earlier activity after posting onto a feed that failed to load', () => { store.error = new Error('boom'); - store.entries = [makeEntry()]; + store.entries = [makeComment()]; const { getByTestId } = renderComponent(); getByTestId('workflow-review-activity-load-more-retry').click(); @@ -122,7 +127,7 @@ describe('WorkflowReviewActivityFeed', () => { }); it('shows progress while refetching a feed that already has entries', async () => { - store.entries = [makeEntry()]; + store.entries = [makeComment()]; store.loading = true; const { container } = renderComponent(); @@ -132,7 +137,7 @@ describe('WorkflowReviewActivityFeed', () => { }); it('keeps a loaded feed and offers a retry when load-more failed', async () => { - store.entries = [makeEntry()]; + store.entries = [makeComment()]; store.hasMore = true; // A load-more failure always has a cursor to resume from. store.nextCursor = 'cursor-1'; @@ -161,7 +166,7 @@ describe('WorkflowReviewActivityFeed', () => { }); it('shows a placeholder for an activity type this frontend does not know', async () => { - store.entries = [makeEntry({ typeVersion: 2 })]; + store.entries = [makeComment({ typeVersion: 2 })]; const { getByTestId } = renderComponent(); await nextTick(); @@ -172,7 +177,7 @@ describe('WorkflowReviewActivityFeed', () => { }); it('loads older entries when the viewer scrolls to the top of the feed', async () => { - store.entries = [makeEntry()]; + store.entries = [makeComment()]; store.hasMore = true; const { getByTestId } = renderComponent(); @@ -189,7 +194,7 @@ describe('WorkflowReviewActivityFeed', () => { }); it('stops loading once the oldest entry is on screen', async () => { - store.entries = [makeEntry()]; + store.entries = [makeComment()]; store.hasMore = false; renderComponent(); @@ -199,8 +204,47 @@ describe('WorkflowReviewActivityFeed', () => { expect(observer.observe).not.toHaveBeenCalled(); }); + /** + * A reload keeps only the entries newer than the page it fetched, so its first entry + * differs without anything having been prepended. `scrollHeight` is stubbed because jsdom + * has no layout; the assertion is on the feed landing at the bottom, not on any arithmetic. + */ + async function reloadFeedAndReturnContainer(getByTestId: (id: string) => HTMLElement) { + const container = getByTestId('workflow-review-activity-feed'); + Object.defineProperty(container, 'scrollHeight', { value: 500, configurable: true }); + container.scrollTop = 120; + + store.entries = [makeComment({ id: '2' }), makeComment({ id: '3' })]; + await nextTick(); + + return container; + } + + it('shows the entry a decision just added to a feed the viewer had paged back through', async () => { + store.entries = [makeComment({ id: '1' }), makeComment({ id: '2' })]; + + const { getByTestId } = renderComponent(); + await nextTick(); + + expect((await reloadFeedAndReturnContainer(getByTestId)).scrollTop).toBe(500); + }); + + // `loadMore` returns early while another page is in flight, leaving behind the anchor it + // captured — which by the next reload points at an entry no longer on screen. + it('shows the newest entry after a reload that followed a load-more that never ran', async () => { + store.entries = [makeComment({ id: '1' }), makeComment({ id: '2' })]; + store.hasMore = true; + store.loadingMore = true; + + const { getByTestId } = renderComponent(); + await nextTick(); + observer.onIntersect(); + + expect((await reloadFeedAndReturnContainer(getByTestId)).scrollTop).toBe(500); + }); + it('does not request the same older page twice', async () => { - store.entries = [makeEntry()]; + store.entries = [makeComment()]; store.hasMore = true; store.loadingMore = true; @@ -210,4 +254,214 @@ describe('WorkflowReviewActivityFeed', () => { expect(observer.observe).not.toHaveBeenCalled(); }); + + describe('review lifecycle entries', () => { + it.each<[string, WorkflowReviewActivityEntry, { testId: string; text: string; note?: string }]>( + [ + [ + 'the review being submitted', + { + ...systemEntry, + type: 'review.opened', + data: { workflowVersions: [{ workflowId: 'wf-1', workflowVersionId: 'version-1' }] }, + }, + { testId: 'workflow-review-activity-opened', text: 'Review submitted' }, + ], + [ + 'requested changes with the note the reviewer left', + { + ...systemEntry, + type: 'review.changes_requested', + data: { + workflowVersions: [{ workflowId: 'wf-1', workflowVersionId: 'version-1' }], + note: 'Please add retries', + }, + }, + { + testId: 'workflow-review-activity-changes-requested', + text: 'Requested changes', + note: 'Please add retries', + }, + ], + [ + 'an approval with the note the reviewer left', + { + ...systemEntry, + type: 'review.approved', + data: { + workflowVersions: [{ workflowId: 'wf-1', workflowVersionId: 'version-1' }], + note: 'Ship it', + }, + }, + { + testId: 'workflow-review-activity-approved', + text: 'Approved submission', + note: 'Ship it', + }, + ], + [ + 'an approval given without a note', + { + ...systemEntry, + type: 'review.approved', + data: { + workflowVersions: [{ workflowId: 'wf-1', workflowVersionId: 'version-1' }], + note: null, + }, + }, + { testId: 'workflow-review-activity-approved', text: 'Approved submission' }, + ], + [ + 'an approval whose stored payload can no longer be read', + { ...systemEntry, type: 'review.approved', data: null }, + { testId: 'workflow-review-activity-approved', text: 'Approved submission' }, + ], + [ + 'the reviewed version being updated', + { + ...systemEntry, + type: 'review.version_updated', + data: { + workflowId: 'wf-1', + fromWorkflowVersionId: 'version-1', + toWorkflowVersionId: 'version-2', + }, + }, + { + testId: 'workflow-review-activity-version-updated', + text: 'New workflow version submitted', + }, + ], + [ + 'a review closed by archiving its workflow', + { ...systemEntry, type: 'review.closed', data: { reason: 'workflow-archived' } }, + { + testId: 'workflow-review-activity-closed', + text: 'Review closed because the workflow was archived', + }, + ], + [ + 'a review closed by moving its workflow', + { ...systemEntry, type: 'review.closed', data: { reason: 'workflow-moved' } }, + { + testId: 'workflow-review-activity-closed', + text: 'Review closed because the workflow moved to another project', + }, + ], + [ + 'a review closed by deleting its workflow', + { ...systemEntry, type: 'review.closed', data: { reason: 'workflow-deleted' } }, + { + testId: 'workflow-review-activity-closed', + text: 'Review closed because the workflow was deleted', + }, + ], + ], + )('shows %s', async (_label, entry, expected) => { + store.entries = [entry]; + + const { getByTestId, queryByTestId } = renderComponent(); + await nextTick(); + + expect(getByTestId(expected.testId)).toHaveTextContent(expected.text); + if (expected.note) { + expect(getByTestId('workflow-review-activity-note')).toHaveTextContent(expected.note); + } else { + expect(queryByTestId('workflow-review-activity-note')).not.toBeInTheDocument(); + } + }); + + // The only entry whose payload the sentence cannot do without: the close reason *is* the + // sentence, so there is nothing left to render. + it('shows a placeholder for a closed review that no longer says why', async () => { + store.entries = [{ ...systemEntry, type: 'review.closed', data: null }]; + + const { getByTestId, queryByTestId } = renderComponent(); + await nextTick(); + + expect(getByTestId('workflow-review-activity-unknown')).toHaveTextContent( + "This activity entry can't be displayed.", + ); + expect(queryByTestId('workflow-review-activity-closed')).not.toBeInTheDocument(); + }); + + // Losing the note must not cost the actor and the verb too, which are known from the type. + it('keeps a change request whose note can no longer be read', async () => { + store.entries = [{ ...systemEntry, type: 'review.changes_requested', data: null }]; + + const { getByTestId, queryByTestId } = renderComponent(); + await nextTick(); + + expect(getByTestId('workflow-review-activity-changes-requested')).toHaveTextContent( + 'Requested changes', + ); + expect(queryByTestId('workflow-review-activity-note')).not.toBeInTheDocument(); + expect(queryByTestId('workflow-review-activity-unknown')).not.toBeInTheDocument(); + }); + + // No component is registered for this type yet, which is why the registry stays partial. + it('shows a placeholder for an entry type no renderer covers yet', async () => { + store.entries = [{ ...systemEntry, type: 'workflow.published', data: null }]; + + const { getByTestId } = renderComponent(); + await nextTick(); + + expect(getByTestId('workflow-review-activity-unknown')).toBeInTheDocument(); + }); + + it('names the reviewer who acted', async () => { + store.entries = [ + { + ...systemEntry, + type: 'review.approved', + data: { + workflowVersions: [{ workflowId: 'wf-1', workflowVersionId: 'version-1' }], + note: null, + }, + createdBy: { + id: 'user-1', + email: 'ada@example.com', + firstName: 'Ada', + lastName: 'Lovelace', + }, + }, + ]; + + const { getByTestId } = renderComponent(); + await nextTick(); + + expect(getByTestId('workflow-review-activity-actor')).toHaveTextContent('Ada Lovelace'); + }); + + // The actor is missing for two different reasons, and a deleted approver must not + // read as a system event. + it('names a deleted reviewer as the actor of their decision', async () => { + store.entries = [ + { + ...systemEntry, + type: 'review.approved', + data: { + workflowVersions: [{ workflowId: 'wf-1', workflowVersionId: 'version-1' }], + note: null, + }, + }, + ]; + + const { getByTestId } = renderComponent(); + await nextTick(); + + expect(getByTestId('workflow-review-activity-actor')).toHaveTextContent('Deleted user'); + }); + + it('shows no actor at all for a review the system closed', async () => { + store.entries = [ + { ...systemEntry, type: 'review.closed', data: { reason: 'workflow-moved' } }, + ]; + + const { queryByTestId } = renderComponent(); + await nextTick(); + + expect(queryByTestId('workflow-review-activity-actor')).not.toBeInTheDocument(); + }); + }); }); diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.vue b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.vue index 8760d1f6bb3..c26b5fef20d 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.vue +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewActivityFeed.vue @@ -1,5 +1,5 @@ + + + + diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailMetadata.vue b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailMetadata.vue index f5f75640fd0..0a5a1b8987c 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailMetadata.vue +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailMetadata.vue @@ -38,13 +38,13 @@ const statusSummary = computed(() => {