diff --git a/packages/@n8n/api-types/src/dto/workflow-reviews/get-workflow-review-request-detail.dto.ts b/packages/@n8n/api-types/src/dto/workflow-reviews/get-workflow-review-request-detail.dto.ts index c03438ae427..8bcfb3d585a 100644 --- a/packages/@n8n/api-types/src/dto/workflow-reviews/get-workflow-review-request-detail.dto.ts +++ b/packages/@n8n/api-types/src/dto/workflow-reviews/get-workflow-review-request-detail.dto.ts @@ -39,7 +39,13 @@ export type WorkflowReviewDecisionIneligibilityReason = | 'missing_permission' | 'missing_reviewer_permission'; -export interface WorkflowReviewRequestDetail extends WorkflowReviewInboxItem { +/** + * The inbox item's flat `workflowName` / `workflowVersionId` are omitted: they + * summarize the review for the list card, which holds exactly one workflow today + * (create caps the list at one), while the detail lists every covered workflow. + */ +export interface WorkflowReviewRequestDetail + extends Omit { description: string | null; workflows: WorkflowReviewRequestWorkflowDetail[]; /** diff --git a/packages/@n8n/db/src/repositories/workflow-review-request-workflow.repository.ts b/packages/@n8n/db/src/repositories/workflow-review-request-workflow.repository.ts index d8d7724d948..3146d5dee75 100644 --- a/packages/@n8n/db/src/repositories/workflow-review-request-workflow.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow-review-request-workflow.repository.ts @@ -136,7 +136,11 @@ export class WorkflowReviewRequestWorkflowRepository extends BaseRepository> { diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-authorization.access.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-authorization.access.service.test.ts index 77d3952c9c4..e20f0ec2303 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-authorization.access.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-authorization.access.service.test.ts @@ -216,24 +216,22 @@ describe('WorkflowReviewAuthorizationService: visibility and the read gate', () ); }); - it('treats the first covered workflow as the one under review', async () => { + it('returns the covered workflows together with the ones the caller may read', async () => { mockReadableReviewProject(); mockChildRow(); const result = await service.findReadableRequestOrFail(requester, requestId); - expect(result.readableWorkflowRows).toEqual([ - { - workflowId, - workflowName: 'My workflow', - workflowVersionId: 'ver-pinned', - activeVersionId: null, - baselineVersionId: null, - requestState: 'open', - }, - ]); - expect(result.pinnedWorkflowId).toBe(workflowId); - expect(result.canReadPinnedWorkflow).toBe(true); + const expectedRow = { + workflowId, + workflowName: 'My workflow', + workflowVersionId: 'ver-pinned', + activeVersionId: null, + baselineVersionId: null, + requestState: 'open', + }; + expect(result.workflowRows).toEqual([expectedRow]); + expect(result.readableWorkflowRows).toEqual([expectedRow]); expect(workflowFinderService.findWorkflowForUser).toHaveBeenCalledWith( workflowId, requester, @@ -277,18 +275,17 @@ describe('WorkflowReviewAuthorizationService: visibility and the read gate', () requestState: 'open', }, ]); - // Eligibility still resolves against the pinned row, which they cannot read - expect(result.pinnedWorkflowId).toBe(workflowId); - expect(result.canReadPinnedWorkflow).toBe(false); + // Eligibility still sees the full coverage, unreadable rows included + expect(result.workflowRows.map((row) => row.workflowId)).toEqual([workflowId, 'wf-2']); }); - it('has no workflow under review once the review covers none', async () => { + it('returns empty row sets once the review covers no workflow', async () => { mockReadableReviewProject(); const result = await service.findReadableRequestOrFail(requester, requestId); - expect(result.pinnedWorkflowId).toBeNull(); - expect(result.canReadPinnedWorkflow).toBe(false); + expect(result.workflowRows).toEqual([]); + expect(result.readableWorkflowRows).toEqual([]); }); }); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-authorization.capabilities.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-authorization.capabilities.service.test.ts index a0ccf0d98ba..65105f3c522 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-authorization.capabilities.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-authorization.capabilities.service.test.ts @@ -6,6 +6,7 @@ import type { WorkflowReviewRequestAuthorRepository, WorkflowReviewRequestRepository, WorkflowReviewRequestReviewerRepository, + WorkflowReviewRequestWorkflowDetailRow, WorkflowReviewRequestWorkflowRepository, } from '@n8n/db'; import { mock } from 'vitest-mock-extended'; @@ -42,11 +43,15 @@ describe('WorkflowReviewAuthorizationService: viewer capabilities', () => { const request = () => mock({ id: requestId, projectId }); + const row = (id = 'wf-1') => mock({ workflowId: id }); + + /** By default the review covers one workflow and the viewer can read it. */ const readable = ( overrides: Partial[1]> = {}, ) => ({ request: request(), - canReadPinnedWorkflow: true, + workflowRows: [row()], + readableWorkflowRows: [row()], ...overrides, }); @@ -166,7 +171,7 @@ describe('WorkflowReviewAuthorizationService: viewer capabilities', () => { const eligibility = await service.resolveViewerEligibility( memberUser(), - readable({ canReadPinnedWorkflow: false }), + readable({ readableWorkflowRows: [] }), ); expect(eligibility).toEqual({ @@ -196,13 +201,28 @@ describe('WorkflowReviewAuthorizationService: viewer capabilities', () => { canComment: true, }); }); + + it('requires read access to every covered workflow, not just one of them', async () => { + const rows = [row('wf-1'), row('wf-2')]; + + const eligibility = await service.resolveViewerEligibility( + memberUser(), + readable({ workflowRows: rows, readableWorkflowRows: rows.slice(1) }), + ); + + expect(eligibility).toEqual({ + canDecide: false, + decisionIneligibilityReason: 'missing_permission', + canComment: false, + }); + }); }); describe('who may comment', () => { it('refuses commenting to a viewer who cannot open the workflow under review', async () => { const eligibility = await service.resolveViewerEligibility( memberUser(), - readable({ canReadPinnedWorkflow: false }), + readable({ readableWorkflowRows: [] }), ); expect(eligibility).toEqual({ @@ -229,7 +249,7 @@ describe('WorkflowReviewAuthorizationService: viewer capabilities', () => { const eligibility = await service.resolveViewerEligibility( memberUser(), - readable({ canReadPinnedWorkflow: false }), + readable({ readableWorkflowRows: [] }), ); expect(eligibility).toEqual({ @@ -242,7 +262,7 @@ describe('WorkflowReviewAuthorizationService: viewer capabilities', () => { it('refuses both deciding and commenting on a review whose workflow is gone', async () => { const eligibility = await service.resolveViewerEligibility( memberUser(), - readable({ canReadPinnedWorkflow: false }), + readable({ readableWorkflowRows: [] }), ); expect(eligibility).toEqual({ diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts index 38319098605..29cbe6c6ce0 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-inbox.detail.service.test.ts @@ -83,13 +83,15 @@ describe('WorkflowReviewInboxService.getDetail', () => { }); } - /** The read gate resolved: `readableWorkflowRows` are what the caller may still read. */ - function mockGate(readableWorkflowRows: WorkflowReviewRequestWorkflowDetailRow[] = []) { + /** The read gate resolved: the caller may read everything the review covers. */ + function mockGate( + workflowRows: WorkflowReviewRequestWorkflowDetailRow[] = [], + request = reviewRequest(), + ) { authorizationService.findReadableRequestOrFail.mockResolvedValue({ - request: reviewRequest(), - readableWorkflowRows, - pinnedWorkflowId: readableWorkflowRows.at(0)?.workflowId ?? null, - canReadPinnedWorkflow: readableWorkflowRows.length > 0, + request, + workflowRows, + readableWorkflowRows: workflowRows, }); } @@ -157,13 +159,15 @@ describe('WorkflowReviewInboxService.getDetail', () => { decision: 'pending', title: 'Please review', description: 'Some context', - workflowName: 'My workflow', - workflowVersionId: 'ver-pinned', createdAt: '2026-07-01T00:00:00.000Z', updatedAt: '2026-07-02T00:00:00.000Z', }); expect(detail.workflows).toHaveLength(1); expect(detail.workflows[0]).toMatchObject({ workflowId, workflowName: 'My workflow' }); + // The covered workflows live only in `workflows` — the inbox card's flat + // summary fields are not part of the detail response. + expect(detail).not.toHaveProperty('workflowName'); + expect(detail).not.toHaveProperty('workflowVersionId'); }); // A covered workflow is removed along with the workflow itself, so a closed @@ -175,8 +179,6 @@ describe('WorkflowReviewInboxService.getDetail', () => { const detail = await service.getDetail(requester, requestId); expect(detail.workflows).toEqual([]); - expect(detail.workflowName).toBeNull(); - expect(detail.workflowVersionId).toBeNull(); }); // An open review can transiently cover no workflow when a delete orphaned @@ -188,7 +190,6 @@ describe('WorkflowReviewInboxService.getDetail', () => { expect(detail.state).toBe('open'); expect(detail.workflows).toEqual([]); - expect(detail.workflowName).toBeNull(); }); }); @@ -234,14 +235,14 @@ describe('WorkflowReviewInboxService.getDetail', () => { expect(detail.viewerCanComment).toBe(true); }); - it('checks what the viewer may do against the workflow under review, even one they cannot open', async () => { + it('checks what the viewer may do against every covered workflow, even ones they cannot open', async () => { // The requester keeps their record after losing view access to the covered - // workflow — eligibility must still be checked against that pinned row. + // workflow — eligibility must still see the full coverage. + const coveredRow = mock({ workflowId }); authorizationService.findReadableRequestOrFail.mockResolvedValue({ request: reviewRequest(), + workflowRows: [coveredRow], readableWorkflowRows: [], - pinnedWorkflowId: workflowId, - canReadPinnedWorkflow: false, }); authorizationService.resolveViewerEligibility.mockResolvedValue({ canDecide: false, @@ -253,9 +254,8 @@ describe('WorkflowReviewInboxService.getDetail', () => { expect(authorizationService.resolveViewerEligibility).toHaveBeenCalledWith(requester, { request: expect.objectContaining({ id: requestId }), + workflowRows: [coveredRow], readableWorkflowRows: [], - pinnedWorkflowId: workflowId, - canReadPinnedWorkflow: false, }); expect(detail.workflows).toEqual([]); expect(detail.viewerCanDecide).toBe(false); @@ -263,7 +263,7 @@ describe('WorkflowReviewInboxService.getDetail', () => { expect(detail.viewerCanComment).toBe(false); }); - it('passes no workflow id when a closed review no longer covers any workflow', async () => { + it('passes empty coverage when a closed review no longer covers any workflow', async () => { requestRepository.findById.mockResolvedValue(reviewRequest({ state: 'closed' })); workflowRepository.findLinkedWorkflowDetailsByRequestId.mockResolvedValue([]); @@ -271,7 +271,7 @@ describe('WorkflowReviewInboxService.getDetail', () => { expect(authorizationService.resolveViewerEligibility).toHaveBeenCalledWith( requester, - expect.objectContaining({ pinnedWorkflowId: null, canReadPinnedWorkflow: false }), + expect.objectContaining({ workflowRows: [], readableWorkflowRows: [] }), ); }); }); @@ -349,9 +349,8 @@ describe('WorkflowReviewInboxService.getDetail', () => { }); it('uses the frozen baseline on a closed review, not the live published pointer', async () => { - authorizationService.findReadableRequestOrFail.mockResolvedValue({ - request: reviewRequest({ state: 'closed', decision: 'approved' }), - readableWorkflowRows: [ + mockGate( + [ { workflowId, workflowName: 'My workflow', @@ -361,9 +360,8 @@ describe('WorkflowReviewInboxService.getDetail', () => { requestState: 'closed', }, ], - pinnedWorkflowId: workflowId, - canReadPinnedWorkflow: true, - }); + reviewRequest({ state: 'closed', decision: 'approved' }), + ); workflowHistoryService.findVersion.mockImplementation(async (_workflowId, versionId) => historyVersion(versionId), ); @@ -376,9 +374,8 @@ describe('WorkflowReviewInboxService.getDetail', () => { it('uses a frozen baseline whatever state accompanies it', async () => { // Only an approval ever writes a baseline, so a frozen one can be trusted alone. - authorizationService.findReadableRequestOrFail.mockResolvedValue({ - request: reviewRequest({ state: 'open', decision: 'pending' }), - readableWorkflowRows: [ + mockGate( + [ { workflowId, workflowName: 'My workflow', @@ -388,9 +385,8 @@ describe('WorkflowReviewInboxService.getDetail', () => { requestState: 'open', }, ], - pinnedWorkflowId: workflowId, - canReadPinnedWorkflow: true, - }); + reviewRequest({ state: 'open', decision: 'pending' }), + ); workflowHistoryService.findVersion.mockImplementation(async (_workflowId, versionId) => historyVersion(versionId), ); @@ -401,9 +397,8 @@ describe('WorkflowReviewInboxService.getDetail', () => { }); it('returns no baseline for a closed review when none was captured', async () => { - authorizationService.findReadableRequestOrFail.mockResolvedValue({ - request: reviewRequest({ state: 'closed', decision: 'approved' }), - readableWorkflowRows: [ + mockGate( + [ { workflowId, workflowName: 'My workflow', @@ -413,9 +408,8 @@ describe('WorkflowReviewInboxService.getDetail', () => { requestState: 'closed', }, ], - pinnedWorkflowId: workflowId, - canReadPinnedWorkflow: true, - }); + reviewRequest({ state: 'closed', decision: 'approved' }), + ); workflowHistoryService.findVersion.mockImplementation(async (_workflowId, versionId) => historyVersion(versionId), ); @@ -429,9 +423,8 @@ describe('WorkflowReviewInboxService.getDetail', () => { // The request is fetched before its rows, so an approval landing in between // leaves the request looking open. The row's own state has to win: a frozen null // baseline would otherwise read as "still open" and resolve the live version. - authorizationService.findReadableRequestOrFail.mockResolvedValue({ - request: reviewRequest({ state: 'open', decision: 'pending' }), - readableWorkflowRows: [ + mockGate( + [ { workflowId, workflowName: 'My workflow', @@ -441,9 +434,8 @@ describe('WorkflowReviewInboxService.getDetail', () => { requestState: 'closed', }, ], - pinnedWorkflowId: workflowId, - canReadPinnedWorkflow: true, - }); + reviewRequest({ state: 'open', decision: 'pending' }), + ); workflowHistoryService.findVersion.mockImplementation(async (_workflowId, versionId) => historyVersion(versionId), ); @@ -454,9 +446,8 @@ describe('WorkflowReviewInboxService.getDetail', () => { }); it('returns no baseline for a closed review that was never approved', async () => { - authorizationService.findReadableRequestOrFail.mockResolvedValue({ - request: reviewRequest({ state: 'closed', decision: 'pending' }), - readableWorkflowRows: [ + mockGate( + [ { workflowId, workflowName: 'My workflow', @@ -466,9 +457,8 @@ describe('WorkflowReviewInboxService.getDetail', () => { requestState: 'closed', }, ], - pinnedWorkflowId: workflowId, - canReadPinnedWorkflow: true, - }); + reviewRequest({ state: 'closed', decision: 'pending' }), + ); workflowHistoryService.findVersion.mockImplementation(async (_workflowId, versionId) => historyVersion(versionId), ); 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 5c5bb0e7023..779b6af9d2b 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 @@ -216,6 +216,23 @@ describe('WorkflowReviewRequestService.decide', () => { expect(dbLockService.withLockContext).not.toHaveBeenCalled(); }); + it('throws NotFoundError when the user cannot view every workflow the request covers', async () => { + mockSuccessfulDecidePath(); + workflowRepository.findByRequestId.mockResolvedValue([ + pinnedRow('ver-1', 'wf-1'), + pinnedRow('ver-2', 'wf-2'), + ]); + workflowFinderService.findWorkflowForUser.mockImplementation(async (workflowId) => + workflowId === 'wf-1' ? mock({ isArchived: false }) : null, + ); + + await expect(service.decide(memberUser(), requestId, approveDto)).rejects.toThrow( + NotFoundError, + ); + + expect(dbLockService.withLockContext).not.toHaveBeenCalled(); + }); + it('throws NotFoundError for a non-assigned viewer without an admin override', async () => { mockSuccessfulDecidePath(); reviewerRepository.isReviewer.mockResolvedValue(false); 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 f7c19b0bd19..f399fc1007b 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 @@ -3347,11 +3347,13 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => { decision: 'pending', title: 'Please review', description: 'Some context', - workflowName: 'Reviewed workflow', - workflowVersionId: 'version-pinned', requester: { id: owner.id, email: owner.email }, reviewers: [{ id: reviewer.id, email: reviewer.email }], }); + // The covered workflows live only in `workflows` — the inbox card's flat + // summary fields are not part of the detail response. + expect(response.body.data).not.toHaveProperty('workflowName'); + expect(response.body.data).not.toHaveProperty('workflowVersionId'); expect(response.body.data.workflows).toHaveLength(1); const [child] = response.body.data.workflows; @@ -3517,7 +3519,6 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => { expect(response.body.data.id).toBe(request.id); expect(response.body.data.state).toBe('open'); expect(response.body.data.workflows).toEqual([]); - expect(response.body.data.workflowName).toBeNull(); }); test('still opens a closed review after its workflow was deleted', async () => { @@ -3549,8 +3550,6 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => { expect(response.body.data.id).toBe(request.id); expect(response.body.data.workflows).toEqual([]); - expect(response.body.data.workflowName).toBeNull(); - expect(response.body.data.workflowVersionId).toBeNull(); }); test('lets an assigned reviewer in the review project open it', async () => { diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-authorization.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-authorization.service.ts index cf8c41efc61..9fcaf9f3dee 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-authorization.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-authorization.service.ts @@ -28,9 +28,9 @@ import { resolveDecisionCapability } from './workflow-review-decision-policy'; export interface ReadableWorkflowReviewRequest { request: WorkflowReviewRequest; + workflowRows: WorkflowReviewRequestWorkflowDetailRow[]; + /** The subset of {@link workflowRows} the caller may currently read. */ readableWorkflowRows: WorkflowReviewRequestWorkflowDetailRow[]; - pinnedWorkflowId: string | null; - canReadPinnedWorkflow: boolean; } export interface WorkflowReviewViewerEligibility { @@ -164,17 +164,7 @@ export class WorkflowReviewAuthorizationService { throw new NotFoundError('Could not find review request'); } - // One workflow per review for now, so the only row is the pinned one. Ids are - // nanoids, so the query's id ordering just makes the pick deterministic. - const pinnedWorkflowId = workflowRows.at(0)?.workflowId ?? null; - return { - request, - readableWorkflowRows, - pinnedWorkflowId, - canReadPinnedWorkflow: readableWorkflowRows.some( - (row) => row.workflowId === pinnedWorkflowId, - ), - }; + return { request, workflowRows, readableWorkflowRows }; } /** @@ -278,12 +268,20 @@ export class WorkflowReviewAuthorizationService { */ async resolveViewerEligibility( user: User, - access: Pick, + access: Pick< + ReadableWorkflowReviewRequest, + 'request' | 'workflowRows' | 'readableWorkflowRows' + >, ): Promise { - const { request, canReadPinnedWorkflow } = access; + const { request, workflowRows, readableWorkflowRows } = access; - // No participation lookup is worth running without read on the pinned version. - if (!canReadPinnedWorkflow) { + // A decision or comment covers the whole review, so both need read access to + // every covered workflow. + const canReadEveryWorkflow = + workflowRows.length > 0 && readableWorkflowRows.length === workflowRows.length; + + // No participation lookup is worth running without read on every workflow. + if (!canReadEveryWorkflow) { return { canDecide: false, decisionIneligibilityReason: 'missing_permission', @@ -299,7 +297,7 @@ export class WorkflowReviewAuthorizationService { ]); const capability = resolveDecisionCapability({ - canReadPinnedWorkflow, + canReadEveryWorkflow, isAuthor, isAssignedReviewer, hasAdminOverride, diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-decision-policy.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-decision-policy.ts index 29eafe4a1ca..4c016646cbc 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-decision-policy.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-decision-policy.ts @@ -2,8 +2,11 @@ import type { WorkflowReviewDecisionIneligibilityReason } from '@n8n/api-types'; /** The facts a decision verdict is derived from, resolved by the caller. */ export interface WorkflowReviewDecisionFacts { - /** Reading the version under review is the floor for deciding it. */ - canReadPinnedWorkflow: boolean; + /** + * Reading every workflow the review covers is the floor for deciding it. A + * decision applies to all of them, and no workflow on a review outranks another. + */ + canReadEveryWorkflow: boolean; isAuthor: boolean; isAssignedReviewer: boolean; hasAdminOverride: boolean; @@ -29,9 +32,9 @@ const ALLOWED: WorkflowReviewDecisionCapability = { allowed: true }; export function resolveDecisionCapability( facts: WorkflowReviewDecisionFacts, ): WorkflowReviewDecisionCapability { - // Checked first so someone who cannot see the workflow hears about the - // permission rather than about their authorship. - if (!facts.canReadPinnedWorkflow) { + // Checked first so someone who cannot see every covered workflow hears about + // the permission rather than about their authorship. + if (!facts.canReadEveryWorkflow) { return { allowed: false, reason: 'missing_permission' }; } diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts index 5ccb9d47523..92021cf5fdf 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-inbox.service.ts @@ -109,14 +109,13 @@ export class WorkflowReviewInboxService { const [workflows, participants, eligibility] = await Promise.all([ Promise.all(readableWorkflowRows.map(async (row) => await this.toWorkflowDetail(row))), this.participantResolver.resolve([request]), - // Reuses the snapshot above: capabilities are resolved against the pinned - // row decide() authorizes against, not against every readable row. + // Reuses the snapshot above: capabilities are resolved against every covered + // row, matching what decide() authorizes against. this.authorizationService.resolveViewerEligibility(user, access), ]); return { - // One workflow per review for now, so the summary fields mirror the first row - ...this.toInboxItem(request, workflows.at(0) ?? null, participants.for(request.id)), + ...this.toReviewSummary(request, participants.for(request.id)), description: request.description, workflows, viewerCanDecide: eligibility.canDecide, @@ -219,17 +218,15 @@ export class WorkflowReviewInboxService { return { createdAt, id }; } - private toInboxItem( + /** The review fields shared by the inbox card and the detail response. */ + private toReviewSummary( entity: WorkflowReviewRequest, - linkedWorkflow: WorkflowReviewRequestLinkedWorkflow | null, { requester, authors, reviewers }: WorkflowReviewParticipants, - ): WorkflowReviewInboxItem { + ): Omit { return { id: entity.id, projectId: entity.projectId, title: entity.title, - workflowName: linkedWorkflow?.workflowName ?? null, - workflowVersionId: linkedWorkflow?.workflowVersionId ?? null, decision: entity.decision, state: entity.state, createdAt: entity.createdAt.toISOString(), @@ -239,4 +236,16 @@ export class WorkflowReviewInboxService { reviewers, }; } + + private toInboxItem( + entity: WorkflowReviewRequest, + linkedWorkflow: WorkflowReviewRequestLinkedWorkflow | null, + participants: WorkflowReviewParticipants, + ): WorkflowReviewInboxItem { + return { + ...this.toReviewSummary(entity, participants), + workflowName: linkedWorkflow?.workflowName ?? null, + workflowVersionId: linkedWorkflow?.workflowVersionId ?? null, + }; + } } 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 4046f274bc6..9052e89d8a6 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 @@ -666,20 +666,29 @@ export class WorkflowReviewRequestService { workflowReviewRequestId, {}, ); - const workflowRow = workflowRows[0]; + // Reviews hold exactly one workflow today (create caps the list at one). + // Baseline capture and activity data below already cover every row, but + // publishing, broadcasts, and events still assume this single row. + const [workflowRow] = workflowRows; if (!workflowRow) { throw new NotFoundError('Could not find review request'); } - const canReadPinnedWorkflow = Boolean( - await this.workflowFinderService.findWorkflowForUser(workflowRow.workflowId, user, [ - 'workflow:read', - ]), + // A decision covers every workflow the review holds, so all of them must be + // readable — no workflow on a review outranks another. + const readableWorkflows = await Promise.all( + workflowRows.map( + async (row) => + await this.workflowFinderService.findWorkflowForUser(row.workflowId, user, [ + 'workflow:read', + ]), + ), ); + const canReadEveryWorkflow = readableWorkflows.every((workflow) => workflow !== null); // The policy's `missing_permission` verdict, applied here rather than below: // 404 (not 403) so callers without access can't probe which requests exist, and // ahead of the lifecycle check so a closed review does not leak one either. - if (!canReadPinnedWorkflow) { + if (!canReadEveryWorkflow) { throw new NotFoundError('Could not find review request'); } @@ -703,7 +712,7 @@ export class WorkflowReviewRequestService { {}, ); this.assertDecisionAllowed({ - canReadPinnedWorkflow, + canReadEveryWorkflow, isAuthor, isAssignedReviewer, hasAdminOverride, @@ -752,7 +761,7 @@ export class WorkflowReviewRequestService { ctx, ); this.assertDecisionAllowed({ - canReadPinnedWorkflow, + canReadEveryWorkflow, isAuthor: isAuthorNow, isAssignedReviewer: isAssignedReviewerNow, hasAdminOverride, diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailMetadata.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailMetadata.test.ts index 7382fafd344..fec2c1c22b7 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailMetadata.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailMetadata.test.ts @@ -60,8 +60,6 @@ function makeDetail( id: 'req-1', projectId: 'proj-1', title: 'Update Payment Handler', - workflowName: 'Payment Handler', - workflowVersionId: 'version-1', requester, // The backend always carries the requester in `authors` too. authors: [{ ...requester }, laterAuthor], diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.decisionNote.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.decisionNote.test.ts index 98e94836938..fe88d2ec6fd 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.decisionNote.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/components/WorkflowReviewDetailTabs.decisionNote.test.ts @@ -33,8 +33,6 @@ const review: WorkflowReviewRequestDetail = { id: 'req-1', projectId: 'proj-1', title: 'Needs review', - workflowName: 'My workflow', - workflowVersionId: null, requester: null, authors: [], reviewers: [], diff --git a/packages/frontend/editor-ui/src/features/workflow-reviews/reviewInbox.store.test.ts b/packages/frontend/editor-ui/src/features/workflow-reviews/reviewInbox.store.test.ts index e614741a833..473abb4bd72 100644 --- a/packages/frontend/editor-ui/src/features/workflow-reviews/reviewInbox.store.test.ts +++ b/packages/frontend/editor-ui/src/features/workflow-reviews/reviewInbox.store.test.ts @@ -784,8 +784,6 @@ function createDetail(): WorkflowReviewRequestDetail { id: 'req-1', projectId: 'proj-1', title: 'Review', - workflowName: 'My workflow', - workflowVersionId: null, requester: null, authors: [], reviewers: [],