diff --git a/packages/@n8n/api-types/src/dto/workflow-reviews/__tests__/update-workflow-review-request-version.dto.test.ts b/packages/@n8n/api-types/src/dto/workflow-reviews/__tests__/update-workflow-review-request-version.dto.test.ts index 17d33f5786d..1063d81c318 100644 --- a/packages/@n8n/api-types/src/dto/workflow-reviews/__tests__/update-workflow-review-request-version.dto.test.ts +++ b/packages/@n8n/api-types/src/dto/workflow-reviews/__tests__/update-workflow-review-request-version.dto.test.ts @@ -29,6 +29,24 @@ describe('UpdateWorkflowReviewRequestVersionDto', () => { workflowVersionDescription: '', }, }, + { + name: 'an optional review description', + request: { + workflowId: 'workflow-1', + workflowVersionId: 'version-1', + workflowVersionName: 'Release candidate', + description: 'Review the retry behavior', + }, + }, + { + name: 'an empty review description', + request: { + workflowId: 'workflow-1', + workflowVersionId: 'version-1', + workflowVersionName: 'Release candidate', + description: '', + }, + }, ])('should validate $name', ({ request }) => { const result = UpdateWorkflowReviewRequestVersionDto.safeParse(request); expect(result.success).toBe(true); @@ -111,6 +129,16 @@ describe('UpdateWorkflowReviewRequestVersionDto', () => { }, expectedErrorPath: ['workflowVersionDescription'], }, + { + name: 'a review description longer than 512 characters', + request: { + workflowId: 'workflow-1', + workflowVersionId: 'version-1', + workflowVersionName: 'Release candidate', + description: 'a'.repeat(513), + }, + expectedErrorPath: ['description'], + }, ])('should fail validation for $name', ({ request, expectedErrorPath }) => { const result = UpdateWorkflowReviewRequestVersionDto.safeParse(request); expect(result.success).toBe(false); diff --git a/packages/@n8n/api-types/src/dto/workflow-reviews/list-workflow-review-requests-query.dto.ts b/packages/@n8n/api-types/src/dto/workflow-reviews/list-workflow-review-requests-query.dto.ts index d7d65dd08e6..0397d267eb4 100644 --- a/packages/@n8n/api-types/src/dto/workflow-reviews/list-workflow-review-requests-query.dto.ts +++ b/packages/@n8n/api-types/src/dto/workflow-reviews/list-workflow-review-requests-query.dto.ts @@ -37,6 +37,8 @@ export type WorkflowReviewApprovedPublicationState = * stay on the minimal summary. */ export interface WorkflowReviewRequestForWorkflow extends WorkflowReviewRequestSummary { + /** The review description, but only for a requester who can act on the review */ + description: string | null; /** Who made the current decision; `null` when there is none or the user is gone. */ decisionBy: WorkflowReviewEligibleReviewer | null; /** Only derived for an approved review; `null` otherwise. */ diff --git a/packages/@n8n/api-types/src/dto/workflow-reviews/update-workflow-review-request-version.dto.ts b/packages/@n8n/api-types/src/dto/workflow-reviews/update-workflow-review-request-version.dto.ts index 512f99478d6..4a2e4e64263 100644 --- a/packages/@n8n/api-types/src/dto/workflow-reviews/update-workflow-review-request-version.dto.ts +++ b/packages/@n8n/api-types/src/dto/workflow-reviews/update-workflow-review-request-version.dto.ts @@ -1,3 +1,5 @@ +import { z } from 'zod'; + import { n8nIdSchema } from '../../schemas/id.schema'; import { requiredWorkflowVersionNameSchema, @@ -11,4 +13,6 @@ export class UpdateWorkflowReviewRequestVersionDto extends Z.class({ workflowVersionName: requiredWorkflowVersionNameSchema, // When present, an empty/whitespace string clears the version description workflowVersionDescription: workflowVersionDescriptionSchema, + // When present, an empty/whitespace string clears the review description + description: z.string().max(512).optional(), }) {} 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 e264032e227..588f5c6c3ec 100644 --- a/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow-review-request.repository.ts @@ -36,7 +36,7 @@ export type FindManyForInboxOptions = { */ export type WorkflowReviewRequestForWorkflowRow = Pick< WorkflowReviewRequest, - 'id' | 'state' | 'decision' | 'updatedById' | 'createdAt' | 'updatedAt' + 'id' | 'state' | 'decision' | 'description' | 'updatedById' | 'createdAt' | 'updatedAt' > & { workflowVersionId: string | null; }; @@ -187,6 +187,7 @@ export class WorkflowReviewRequestRepository extends BaseRepository({ id: 'user-1' }); /** Build a real `User` with `isPending` computed, as TypeORM does after load. */ @@ -570,6 +569,7 @@ describe('WorkflowReviewRequestService', () => { id: 'req-1', state: 'open', decision: 'pending', + description: null, updatedById: 'user-2', workflowVersionId: 'ver-1', createdAt: new Date('2024-01-01T00:00:00.000Z'), @@ -625,6 +625,7 @@ describe('WorkflowReviewRequestService', () => { id: 'req-1', state: 'open', decision: 'changes_requested', + description: null, workflowVersionId: 'ver-1', createdAt: '2024-01-01T00:00:00.000Z', updatedAt: '2024-01-02T00:00:00.000Z', @@ -700,7 +701,6 @@ describe('WorkflowReviewRequestService', () => { }); }); - // R1 (P3): enriching row by row made this list an N+1 — see LIGO-607_review.md it('enriches many rows with one lookup per derived field', async () => { workflowFinderService.findWorkflowForUser.mockResolvedValue(mock()); requestRepository.findRequestsForWorkflow.mockResolvedValue([ 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 8559b97cf01..a475c56b1a9 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 @@ -94,6 +94,7 @@ describe('WorkflowReviewRequestService.updateVersion', () => { id: requestId, state: 'open', decision: 'pending', + description: null, createdAt: new Date('2026-07-20T10:00:00.000Z'), updatedAt: new Date('2026-07-20T11:00:00.000Z'), ...overrides, @@ -358,6 +359,20 @@ describe('WorkflowReviewRequestService.updateVersion', () => { ); }); + it('updates the review description in the same transaction as the re-pin', async () => { + mockSuccessfulUpdatePath(); + + await service.updateVersion(user, requestId, { + ...dto, + description: ' Updated review context ', + }); + + expect(requestRepository.saveRequest).toHaveBeenCalledWith( + expect.objectContaining({ description: 'Updated review context' }), + ctx, + ); + }); + it('throws BadRequestError when the version was pruned before the naming write', async () => { mockSuccessfulUpdatePath(); workflowHistoryRepository.updateVersionMetadata.mockResolvedValue(0); @@ -442,6 +457,63 @@ describe('WorkflowReviewRequestService.updateVersion', () => { expect(dbLockService.withLockContext).not.toHaveBeenCalled(); }); + it('updates the review description under the lock when the version is already pinned', async () => { + mockAlreadyPinned('Same name'); + requestRepository.findById.mockResolvedValue( + openRequest({ description: 'Original review description' }), + ); + + await service.updateVersion(user, requestId, { + ...dto, + workflowVersionName: 'Same name', + description: ' Updated review description ', + }); + + expect(dbLockService.withLockContext).toHaveBeenCalledOnce(); + expect(requestRepository.saveRequest).toHaveBeenCalledWith( + expect.objectContaining({ + description: 'Updated review description', + updatedById: user.id, + }), + ctx, + ); + expect(workflowRepository.updateWorkflowVersion).not.toHaveBeenCalled(); + expect(collaborationService.broadcastWorkflowReviewStateChanged).toHaveBeenCalledWith('wf-1'); + }); + + it('clears the review description when an empty string is sent', async () => { + mockAlreadyPinned('Same name'); + requestRepository.findById.mockResolvedValue( + openRequest({ description: 'Original review description' }), + ); + + await service.updateVersion(user, requestId, { + ...dto, + workflowVersionName: 'Same name', + description: ' ', + }); + + expect(requestRepository.saveRequest).toHaveBeenCalledWith( + expect.objectContaining({ description: null }), + ctx, + ); + }); + + it('preserves the review description when it is omitted', async () => { + mockAlreadyPinned('Same name'); + requestRepository.findById.mockResolvedValue( + openRequest({ description: 'Original review description' }), + ); + + await service.updateVersion(user, requestId, { + ...dto, + workflowVersionName: 'Same name', + }); + + expect(requestRepository.saveRequest).not.toHaveBeenCalled(); + expect(dbLockService.withLockContext).not.toHaveBeenCalled(); + }); + it('still names the version when a concurrent sync re-pinned it first', async () => { mockSuccessfulUpdatePath(); workflowHistoryService.findVersion.mockResolvedValue( 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 a312d1506be..ad020005980 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 @@ -22,15 +22,15 @@ import { WorkflowReviewRequestWorkflowRepository, } from '@n8n/db'; import { Container } from '@n8n/di'; +import { createAdmin, createMember, createOwner, createUser } from '@test-integration/db/users'; +import { createWorkflowHistoryItem } from '@test-integration/db/workflow-history'; +import type { SuperAgentTest } from '@test-integration/types'; +import * as utils from '@test-integration/utils'; import { v4 as uuid } from 'uuid'; import { ActiveWorkflowManager } from '@/active-workflow-manager'; import { WorkflowReviewPolicyService } from '@/services/workflow-review-policy.service'; import { WorkflowValidationService } from '@/workflows/workflow-validation.service'; -import { createAdmin, createMember, createOwner, createUser } from '@test-integration/db/users'; -import { createWorkflowHistoryItem } from '@test-integration/db/workflow-history'; -import type { SuperAgentTest } from '@test-integration/types'; -import * as utils from '@test-integration/utils'; const activeWorkflowManager = mockInstance(ActiveWorkflowManager); const workflowValidationService = mockInstance(WorkflowValidationService); @@ -936,7 +936,11 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/update-version versionId: string, author: User, projectId = ownerProject.id, - overrides: { state?: 'open' | 'closed'; decision?: 'pending' | 'changes_requested' } = {}, + overrides: { + state?: 'open' | 'closed'; + decision?: 'pending' | 'changes_requested'; + description?: string | null; + } = {}, ) { const request = await requestRepository.createRequest( { @@ -1066,6 +1070,70 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/update-version expect(unchanged?.decision).toBe('changes_requested'); }); + test('updates the review description when re-pinning the version', async () => { + const { workflow } = await createReviewableWorkflow('version-1'); + await createWorkflowHistoryItem(workflow.id, { versionId: 'version-2' }); + const request = await seedOpenRequest(workflow.id, 'version-1', owner, ownerProject.id, { + description: 'Original review description', + }); + + await ownerAgent + .post(`/workflow-review-requests/${request.id}/update-version`) + .send({ + workflowId: workflow.id, + workflowVersionId: 'version-2', + workflowVersionName: 'Release candidate', + description: ' Updated review description ', + }) + .expect(200); + + const updated = await requestRepository.findById(request.id, {}); + expect(updated?.description).toBe('Updated review description'); + }); + + test('updates the review description when the version is already pinned', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await seedOpenRequest(workflow.id, versionId, owner, ownerProject.id, { + decision: 'changes_requested', + description: 'Original review description', + }); + + await ownerAgent + .post(`/workflow-review-requests/${request.id}/update-version`) + .send({ + workflowId: workflow.id, + workflowVersionId: versionId, + workflowVersionName: 'Release candidate', + description: 'Updated review description', + }) + .expect(200); + + const updated = await requestRepository.findById(request.id, {}); + expect(updated).toMatchObject({ + description: 'Updated review description', + decision: 'changes_requested', + }); + }); + + test('clears the review description when an empty string is sent', async () => { + const { workflow, versionId } = await createReviewableWorkflow(); + const request = await seedOpenRequest(workflow.id, versionId, owner, ownerProject.id, { + description: 'Original review description', + }); + + await ownerAgent + .post(`/workflow-review-requests/${request.id}/update-version`) + .send({ + workflowId: workflow.id, + workflowVersionId: versionId, + workflowVersionName: 'Release candidate', + description: '', + }) + .expect(200); + + expect((await requestRepository.findById(request.id, {}))?.description).toBeNull(); + }); + test('returns 404 for an unknown review request id', async () => { const { workflow, versionId } = await createReviewableWorkflow(); @@ -1940,6 +2008,9 @@ describe('GET /workflow-review-requests', () => { state: 'open', decision: 'pending', workflowVersionId: versionId, + // The owner can act on the review, so the description rides along; the + // title stays off the workflow-scoped list entirely. + description: 'Confidential description', createdAt: expect.any(String), updatedAt: expect.any(String), // Neither applies to a pending review @@ -1948,6 +2019,34 @@ describe('GET /workflow-review-requests', () => { }); }); + test('withholds the description from a requester who cannot act on the review', async () => { + const workflow = await createWorkflow({}, teamProject); + const versionId = uuid(); + await createWorkflowHistoryItem(workflow.id, { versionId }); + const request = await requestRepository.createRequest( + { + projectId: teamProject.id, + title: 'Confidential title', + description: 'Confidential description', + createdById: owner.id, + }, + {}, + ); + await linkRequestToWorkflow(request.id, workflow.id, versionId); + + const viewerResponse = await viewerAgent + .get('/workflow-review-requests') + .query({ workflowId: workflow.id, take: 1 }) + .expect(200); + expect(viewerResponse.body.data.data[0].description).toBeNull(); + + const editorResponse = await memberAgent + .get('/workflow-review-requests') + .query({ workflowId: workflow.id, take: 1 }) + .expect(200); + expect(editorResponse.body.data.data[0].description).toBe('Confidential description'); + }); + test('returns the newest review, closed included, with take=1 and no state filter', async () => { const { workflow, versionId } = await createReviewableWorkflow(); const older = await requestRepository.createRequest( 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 8c8aa6f2e89..836235ff9ea 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 @@ -47,13 +47,17 @@ import { WorkflowService } from '@/workflows/workflow.service'; import { WorkflowReviewEligibilityService } from './workflow-review-eligibility.service'; import { WorkflowReviewFeatureGate } from './workflow-review-feature-gate.service'; import { toEligibleReviewer } from './workflow-review.mapper'; - /** Omitted stays omitted (column untouched); an empty/whitespace string clears to null. */ function normalizeVersionDescription(description: string | undefined): string | null | undefined { if (description === undefined) return undefined; return description.trim() || null; } +function normalizeReviewDescription(description: string | undefined): string | null | undefined { + if (description === undefined) return undefined; + return description.trim() || null; +} + /** * The workflow-scoped review request lifecycle: listing a workflow's reviews, * resolving its eligible reviewers, opening a review, re-pinning it to a new @@ -113,20 +117,28 @@ export class WorkflowReviewRequestService { throw new NotFoundError('Could not find workflow'); } - const [requests, count] = await this.workflowReviewRequestRepository.findRequestsForWorkflow( - query.workflowId, - { state: query.state, skip: query.skip, take: query.take }, - ); + const [[requests, count], canPublish] = await Promise.all([ + this.workflowReviewRequestRepository.findRequestsForWorkflow(query.workflowId, { + state: query.state, + skip: query.skip, + take: query.take, + }), + // the description is only for whoever can act on the review + this.workflowFinderService + .findWorkflowForUser(query.workflowId, user, ['workflow:publish']) + .then((found) => found !== null), + ]); return { count, - data: await this.toWorkflowScopedItems(query.workflowId, requests), + data: await this.toWorkflowScopedItems(query.workflowId, requests, canPublish), }; } private async toWorkflowScopedItems( workflowId: string, requests: WorkflowReviewRequestForWorkflowRow[], + canPublish: boolean, ): Promise { const [decisionActors, publicationStates] = await Promise.all([ this.resolveDecisionActors(requests), @@ -137,6 +149,7 @@ export class WorkflowReviewRequestService { id: request.id, state: request.state, decision: request.decision, + description: canPublish ? request.description : null, workflowVersionId: request.workflowVersionId, createdAt: request.createdAt.toISOString(), updatedAt: request.updatedAt.toISOString(), @@ -481,12 +494,18 @@ export class WorkflowReviewRequestService { const versionName = dto.workflowVersionName.trim(); const versionDescription = normalizeVersionDescription(dto.workflowVersionDescription); + const reviewDescription = normalizeReviewDescription(dto.description); const metadataChanged = (current: { name: string | null; description: string | null }) => versionName !== current.name || (versionDescription !== undefined && versionDescription !== current.description); + const hasReviewDescriptionChanged = (current: WorkflowReviewRequest) => + reviewDescription !== undefined && reviewDescription !== current.description; - // Nothing new to review: skip the lock, write nothing, broadcast nothing. - if (workflowRow.workflowVersionId === dto.workflowVersionId) { + // Nothing new to review: skip the lock unless the review request itself must change. + if ( + workflowRow.workflowVersionId === dto.workflowVersionId && + !hasReviewDescriptionChanged(request) + ) { // A rename or re-description is the one thing that can still be pending // here, and a lone UPDATE is atomic, so apply it rather than silently // dropping it. @@ -542,7 +561,15 @@ export class WorkflowReviewRequestService { ); } - return { request: current, changed: false }; + if (reviewDescription === undefined || reviewDescription === current.description) { + return { request: current, changed: false }; + } + + current.description = reviewDescription; + current.updatedById = user.id; + const saved = await this.workflowReviewRequestRepository.saveRequest(current, ctx); + + return { request: saved, changed: true }; } await this.workflowReviewRequestWorkflowRepository.updateWorkflowVersion( @@ -563,6 +590,9 @@ export class WorkflowReviewRequestService { ); current.decision = 'pending'; + if (reviewDescription !== undefined) { + current.description = reviewDescription; + } current.updatedById = user.id; const saved = await this.workflowReviewRequestRepository.saveRequest(current, ctx); diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index 1e68876dec9..d86b4988ebd 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -4602,7 +4602,7 @@ "workflowReviews.submitForReview.step": "Step {step} of {total}", "workflowReviews.submitForReview.ariaDescription": "Name and describe the workflow version, then add a review title, a reviewer, and an optional description.", "workflowReviews.submitForReview.reviewTitle.label": "Review title", - "workflowReviews.submitForReview.description.label": "Description", + "workflowReviews.submitForReview.description.label": "Review description", "workflowReviews.submitForReview.reviewer.label": "Reviewer", "workflowReviews.submitForReview.reviewer.placeholder": "Search users...", "workflowReviews.submitForReview.submit": "Submit", diff --git a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.test.ts b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.test.ts index 5b84c2b856d..0878edc0b2a 100644 --- a/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.test.ts +++ b/packages/frontend/editor-ui/src/app/components/MainHeader/WorkflowHeaderDraftPublishActions.test.ts @@ -587,6 +587,7 @@ describe('WorkflowHeaderDraftPublishActions', () => { state: 'open', decision: 'pending', workflowVersionId: 'version-0', + description: null, createdAt: '2026-07-20T10:00:00.000Z', updatedAt: '2026-07-20T10:00:00.000Z', decisionBy: null, @@ -694,6 +695,9 @@ describe('WorkflowHeaderDraftPublishActions', () => { const updateDialog = await findByRole('dialog', { name: 'Submit latest changes to existing review', }); + // The update dialog is a two-step flow: version metadata, then the review + // description. + await userEvent.click(within(updateDialog).getByTestId('workflow-update-review-next-button')); await userEvent.click( within(updateDialog).getByTestId('workflow-update-review-submit-button'), ); @@ -1274,6 +1278,7 @@ describe('WorkflowHeaderDraftPublishActions', () => { decision: 'pending', // Differs from the saved 'version-1', so the review is out of date workflowVersionId: 'version-0', + description: null, createdAt: '2026-07-20T10:00:00.000Z', updatedAt: '2026-07-20T10:00:00.000Z', decisionBy: null, 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 e4adff3c3cc..e1f5e4ea0fe 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 @@ -175,8 +175,6 @@ const statusSummary = computed(() => { min-width: 0; } -/* No card header here, so the role labels carry the grouping — they need more air - between sections than between a label and its own rows. */ .peopleCard { --n8n--card-body--gap: var(--spacing--sm); } @@ -197,8 +195,6 @@ const statusSummary = computed(() => { > span > span { display: flex; flex: 1; - /* Long names wrap, so the icon tracks the first line rather than centring - itself against the whole block. */ align-items: flex-start; min-width: 0; } diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewRequiredToggle.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewRequiredToggle.test.ts index 809708d6aed..2eba1525f94 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewRequiredToggle.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewRequiredToggle.test.ts @@ -112,6 +112,7 @@ describe('WorkflowReviewRequiredToggle', () => { state: 'open', decision: 'pending', workflowVersionId: 'ver-1', + description: null, createdAt: '2026-07-20T10:00:00.000Z', updatedAt: '2026-07-20T10:00:00.000Z', decisionBy: null, diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewStatusBanner.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewStatusBanner.test.ts index ecacc39ad08..2819c997390 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewStatusBanner.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewStatusBanner.test.ts @@ -16,6 +16,7 @@ const review = ( state: 'open', decision: 'pending', workflowVersionId: PINNED_VERSION, + description: null, createdAt: '2026-07-20T10:00:00.000Z', updatedAt: '2026-07-20T10:00:00.000Z', decisionBy: null, diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowSubmitForReviewDialog.vue b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowSubmitForReviewDialog.vue index 0248cc16cf3..90b1613a961 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowSubmitForReviewDialog.vue +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowSubmitForReviewDialog.vue @@ -203,7 +203,7 @@ const submit = async () => { // install the response before clearing the local flag so the // publish gate never opens while a refetch is in flight - reviewStatusStore.setOpenReview(workflowId, reviewRequest); + reviewStatusStore.setOpenReview(workflowId, reviewRequest, trimmedDescription || null); reviewRequiredStore.setReviewRequired(workflowId, false); emit('update:open', false); emit('submitted', reviewRequest.id); diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowUpdateReviewDialog.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowUpdateReviewDialog.test.ts index f1cac42f7e4..c18086cf4bc 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowUpdateReviewDialog.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowUpdateReviewDialog.test.ts @@ -1,7 +1,7 @@ import { ResponseError, type WorkflowVersionData } from '@n8n/rest-api-client'; import { createPinia, setActivePinia } from 'pinia'; import userEvent from '@testing-library/user-event'; -import { fireEvent, waitFor } from '@testing-library/vue'; +import { waitFor } from '@testing-library/vue'; import { createMemoryHistory, createRouter } from 'vue-router'; import { createComponentRenderer } from '@/__tests__/render'; @@ -38,6 +38,7 @@ const openReview = { state: 'open' as const, decision: 'pending' as const, workflowVersionId: 'version-1', + description: 'Original review description' as string | null, createdAt: '2024-01-01T00:00:00.000Z', updatedAt: '2024-01-01T00:00:00.000Z', decisionBy: null, @@ -95,8 +96,14 @@ const renderDialog = async ({ }; const result = renderComponent({ pinia, props }); await result.rerender({ ...props, open: true }); + const goToStep2 = async () => { + await waitFor(() => + expect(result.getByTestId('workflow-update-review-next-button')).toBeEnabled(), + ); + await userEvent.click(result.getByTestId('workflow-update-review-next-button')); + }; - return { ...result, flushSave, documentStore, fetchStatusSpy }; + return { ...result, flushSave, documentStore, fetchStatusSpy, goToStep2 }; }; describe('WorkflowUpdateReviewDialog', () => { @@ -127,17 +134,32 @@ describe('WorkflowUpdateReviewDialog', () => { ); }); - it('keeps the description intact without a link when the open review is unknown', async () => { - const { baseElement, queryByRole } = await renderDialog({ seedOpenReview: false }); + it('stays open without prefilling when no open review is known yet', async () => { + const { queryByRole, emitted, fetchStatusSpy, getByTestId } = await renderDialog({ + seedOpenReview: false, + }); - expect(baseElement.textContent?.replace(/\s+/g, ' ')).toContain( - 'This workflow already has an open review.', - ); + await waitFor(() => expect(fetchStatusSpy).toHaveBeenCalledWith('workflow-1')); expect(queryByRole('link', { name: 'open review' })).not.toBeInTheDocument(); + expect(mockShowError).not.toHaveBeenCalled(); + expect(emitted('update:open')).toBeUndefined(); + await waitFor(() => expect(getByTestId('workflow-update-review-next-button')).toBeEnabled()); + }); + + it('reports a review that no longer exists at submit time', async () => { + const { getByTestId, emitted, goToStep2 } = await renderDialog({ seedOpenReview: false }); + await goToStep2(); + + await userEvent.click(getByTestId('workflow-update-review-submit-button')); + + await waitFor(() => expect(mockShowError).toHaveBeenCalledOnce()); + expect(updateWorkflowReviewRequestVersion).not.toHaveBeenCalled(); + expect(emitted('update:open')).toContainEqual([false]); }); it('submits the flushed version to the open review and refetches the status', async () => { - const { getByTestId, flushSave, fetchStatusSpy, emitted } = await renderDialog(); + const { getByTestId, flushSave, fetchStatusSpy, emitted, goToStep2 } = await renderDialog(); + await goToStep2(); await userEvent.click(getByTestId('workflow-update-review-submit-button')); @@ -150,6 +172,7 @@ describe('WorkflowUpdateReviewDialog', () => { workflowVersionId: SAVED_VERSION_ID, workflowVersionName: GENERATED_VERSION_NAME, workflowVersionDescription: undefined, + description: undefined, }, ); }); @@ -163,14 +186,15 @@ describe('WorkflowUpdateReviewDialog', () => { it('disables submission when the caller knows the review is up to date', async () => { const { getByTestId, flushSave } = await renderDialog({ canSubmit: false }); - expect(getByTestId('workflow-update-review-submit-button')).toBeDisabled(); + expect(getByTestId('workflow-update-review-next-button')).toBeDisabled(); expect(flushSave).not.toHaveBeenCalled(); }); it('closes without updating when saving resolves to the reviewed version', async () => { - const { getByTestId, flushSave, emitted } = await renderDialog({ + const { getByTestId, flushSave, emitted, goToStep2 } = await renderDialog({ reviewData: { ...openReview, workflowVersionId: SAVED_VERSION_ID }, }); + await goToStep2(); await userEvent.click(getByTestId('workflow-update-review-submit-button')); @@ -183,7 +207,8 @@ describe('WorkflowUpdateReviewDialog', () => { it('shows an error and keeps the dialog open when saving fails', async () => { const flushSave = vi.fn().mockResolvedValue(undefined); - const { getByTestId, emitted } = await renderDialog({ flushSave }); + const { getByTestId, emitted, goToStep2 } = await renderDialog({ flushSave }); + await goToStep2(); await userEvent.click(getByTestId('workflow-update-review-submit-button')); @@ -200,7 +225,8 @@ describe('WorkflowUpdateReviewDialog', () => { resolveSave = resolve; }), ); - const { getByTestId, rerender, emitted } = await renderDialog({ flushSave }); + const { getByTestId, rerender, emitted, goToStep2 } = await renderDialog({ flushSave }); + await goToStep2(); await userEvent.click(getByTestId('workflow-update-review-submit-button')); await waitFor(() => expect(flushSave).toHaveBeenCalledOnce()); @@ -210,25 +236,98 @@ describe('WorkflowUpdateReviewDialog', () => { await rerender({ open: true, workflowId: 'workflow-2', flushSave }); resolveSave('version-2'); - await waitFor(() => expect(getByTestId('workflow-update-review-submit-button')).toBeEnabled()); + await waitFor(() => expect(getByTestId('workflow-update-review-next-button')).toBeEnabled()); expect(updateWorkflowReviewRequestVersion).not.toHaveBeenCalled(); expect(mockShowError).not.toHaveBeenCalled(); expect(emitted('updated')).toBeUndefined(); }); - it('shows an error and closes when no open review is found after a refetch', async () => { - const { getByTestId, fetchStatusSpy, emitted } = await renderDialog({ - seedOpenReview: false, + describe('review description', () => { + it('uses the same two-step flow as review creation and prefills the description', async () => { + const { getByTestId, goToStep2 } = await renderDialog(); + + expect(getByTestId('workflow-update-review-dialog-step')).toHaveTextContent('Step 1 of 2'); + + await goToStep2(); + + expect(getByTestId('workflow-update-review-dialog-step')).toHaveTextContent('Step 2 of 2'); + expect(getByTestId('workflow-update-review-description-input')).toHaveValue( + 'Original review description', + ); + expect(getByTestId('workflow-update-review-description-input')).toHaveAttribute( + 'maxlength', + '512', + ); }); - await userEvent.click(getByTestId('workflow-update-review-submit-button')); + it('returns to the version step without losing the review description', async () => { + const { getByTestId, goToStep2 } = await renderDialog(); + await goToStep2(); + await userEvent.clear(getByTestId('workflow-update-review-description-input')); + await userEvent.type( + getByTestId('workflow-update-review-description-input'), + 'Updated context', + ); - await waitFor(() => expect(mockShowError).toHaveBeenCalledOnce()); - // One refetch attempt before giving up. - expect(fetchStatusSpy).toHaveBeenCalledWith('workflow-1'); - expect(updateWorkflowReviewRequestVersion).not.toHaveBeenCalled(); - expect(emitted('updated')).toBeUndefined(); - expect(emitted('update:open')).toContainEqual([false]); + await userEvent.click(getByTestId('workflow-update-review-back-button')); + expect(getByTestId('workflow-update-review-version-name-input')).toBeInTheDocument(); + await userEvent.click(getByTestId('workflow-update-review-next-button')); + + expect(getByTestId('workflow-update-review-description-input')).toHaveValue( + 'Updated context', + ); + }); + + it('submits a changed review description', async () => { + const { getByTestId, goToStep2 } = await renderDialog(); + await goToStep2(); + await userEvent.clear(getByTestId('workflow-update-review-description-input')); + await userEvent.type( + getByTestId('workflow-update-review-description-input'), + ' Updated review context ', + ); + + await userEvent.click(getByTestId('workflow-update-review-submit-button')); + + await waitFor(() => + expect(updateWorkflowReviewRequestVersion).toHaveBeenCalledWith( + expect.any(Object), + 'review-1', + expect.objectContaining({ description: 'Updated review context' }), + ), + ); + }); + + it('omits an unchanged review description', async () => { + const { getByTestId, goToStep2 } = await renderDialog(); + await goToStep2(); + + await userEvent.click(getByTestId('workflow-update-review-submit-button')); + + await waitFor(() => + expect(updateWorkflowReviewRequestVersion).toHaveBeenCalledWith( + expect.any(Object), + 'review-1', + expect.objectContaining({ description: undefined }), + ), + ); + }); + + it('sends an empty review description when the prefilled value is cleared', async () => { + const { getByTestId, goToStep2 } = await renderDialog(); + await goToStep2(); + await userEvent.clear(getByTestId('workflow-update-review-description-input')); + + await userEvent.click(getByTestId('workflow-update-review-submit-button')); + + await waitFor(() => + expect(updateWorkflowReviewRequestVersion).toHaveBeenCalledWith( + expect.any(Object), + 'review-1', + expect.objectContaining({ description: '' }), + ), + ); + }); }); describe('version name and description', () => { @@ -272,22 +371,23 @@ describe('WorkflowUpdateReviewDialog', () => { ); }); - it('blocks submission while the name is empty', async () => { + it('blocks advancing while the name is empty', async () => { const { getByTestId } = await renderDialog(); await userEvent.clear(getByTestId('workflow-update-review-version-name-input')); - expect(getByTestId('workflow-update-review-submit-button')).toBeDisabled(); + expect(getByTestId('workflow-update-review-next-button')).toBeDisabled(); }); it('submits the trimmed name and mirrors it into the editor', async () => { - const { getByTestId, documentStore } = await renderDialog(); + const { getByTestId, documentStore, goToStep2 } = await renderDialog(); await userEvent.clear(getByTestId('workflow-update-review-version-name-input')); await userEvent.type( getByTestId('workflow-update-review-version-name-input'), ' Release 3 ', ); + await goToStep2(); await userEvent.click(getByTestId('workflow-update-review-submit-button')); await waitFor(() => { @@ -299,6 +399,7 @@ describe('WorkflowUpdateReviewDialog', () => { workflowVersionId: SAVED_VERSION_ID, workflowVersionName: 'Release 3', workflowVersionDescription: undefined, + description: undefined, }, ); }); @@ -310,12 +411,13 @@ describe('WorkflowUpdateReviewDialog', () => { }); it('submits the trimmed description and mirrors it into the editor', async () => { - const { getByTestId, documentStore } = await renderDialog(); + const { getByTestId, documentStore, goToStep2 } = await renderDialog(); await userEvent.type( getByTestId('workflow-update-review-version-description-input'), ' What changed ', ); + await goToStep2(); await userEvent.click(getByTestId('workflow-update-review-submit-button')); await waitFor(() => { @@ -329,7 +431,7 @@ describe('WorkflowUpdateReviewDialog', () => { }); it('sends an empty description when the prefilled one is cleared', async () => { - const { getByTestId, documentStore } = await renderDialog({ + const { getByTestId, documentStore, goToStep2 } = await renderDialog({ versionData: { versionId: SAVED_VERSION_ID, name: 'Release candidate', @@ -338,6 +440,7 @@ describe('WorkflowUpdateReviewDialog', () => { }); await userEvent.clear(getByTestId('workflow-update-review-version-description-input')); + await goToStep2(); await userEvent.click(getByTestId('workflow-update-review-submit-button')); await waitFor(() => { @@ -350,25 +453,22 @@ describe('WorkflowUpdateReviewDialog', () => { expect(documentStore.versionData).toMatchObject({ description: null }); }); - // The name is read before `flushSave()` is awaited, so a mid-save change can't - // reach the request. `fireEvent` bypasses the disabled input the way a stray - // programmatic write would, keeping the snapshot covered on its own. - it('locks the name while submitting and sends the one validated at click time', async () => { + it('sends the name validated before advancing to the review step', async () => { let resolveSave!: (versionId: string | undefined) => void; const flushSave = vi.fn().mockReturnValue( new Promise((resolve) => { resolveSave = resolve; }), ); - const { getByTestId, documentStore } = await renderDialog({ flushSave }); + const { getByTestId, documentStore, goToStep2 } = await renderDialog({ flushSave }); const input = getByTestId('workflow-update-review-version-name-input'); await userEvent.clear(input); await userEvent.type(input, 'Validated name'); + await goToStep2(); await userEvent.click(getByTestId('workflow-update-review-submit-button')); - await waitFor(() => expect(input).toBeDisabled()); - await fireEvent.update(input, ''); + await waitFor(() => expect(getByTestId('workflow-update-review-back-button')).toBeDisabled()); resolveSave(SAVED_VERSION_ID); await waitFor(() => { @@ -380,6 +480,7 @@ describe('WorkflowUpdateReviewDialog', () => { workflowVersionId: SAVED_VERSION_ID, workflowVersionName: 'Validated name', workflowVersionDescription: undefined, + description: undefined, }, ); }); @@ -391,7 +492,8 @@ describe('WorkflowUpdateReviewDialog', () => { vi.mocked(updateWorkflowReviewRequestVersion).mockRejectedValue( new ResponseError('Conflict', { httpStatusCode: 409 }), ); - const { getByTestId, fetchStatusSpy, emitted } = await renderDialog(); + const { getByTestId, fetchStatusSpy, emitted, goToStep2 } = await renderDialog(); + await goToStep2(); await userEvent.click(getByTestId('workflow-update-review-submit-button')); @@ -399,4 +501,35 @@ describe('WorkflowUpdateReviewDialog', () => { expect(fetchStatusSpy).toHaveBeenCalledWith('workflow-1'); expect(emitted('updated')).toBeUndefined(); }); + + it('retries against the review that replaced the one open when the dialog was opened', async () => { + vi.mocked(updateWorkflowReviewRequestVersion).mockRejectedValueOnce( + new ResponseError('Conflict', { httpStatusCode: 409 }), + ); + const { getByTestId, goToStep2 } = await renderDialog(); + await goToStep2(); + + // The first attempt conflicts, and the status refetch reports a new review. + vi.mocked(fetchWorkflowReviewRequests).mockResolvedValue({ + count: 1, + data: [{ ...openReview, id: 'review-2' }], + }); + await userEvent.click(getByTestId('workflow-update-review-submit-button')); + await waitFor(() => expect(mockShowError).toHaveBeenCalledOnce()); + + vi.mocked(updateWorkflowReviewRequestVersion).mockResolvedValue({ + ...openReview, + id: 'review-2', + workflowVersionId: SAVED_VERSION_ID, + }); + await userEvent.click(getByTestId('workflow-update-review-submit-button')); + + await waitFor(() => + expect(updateWorkflowReviewRequestVersion).toHaveBeenLastCalledWith( + expect.any(Object), + 'review-2', + expect.any(Object), + ), + ); + }); }); diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowUpdateReviewDialog.vue b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowUpdateReviewDialog.vue index 8974e6199f7..e9b3d87d293 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowUpdateReviewDialog.vue +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowUpdateReviewDialog.vue @@ -1,11 +1,17 @@