diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-eligibility.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-eligibility.service.test.ts index 3da3da9b758..d3b667c3f2a 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-eligibility.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-eligibility.service.test.ts @@ -78,12 +78,25 @@ describe('WorkflowReviewEligibilityService', () => { }); }); - it('stops an author from approving their own review even when assigned', async () => { + it('lets an assigned reviewer decide even when they authored a version', async () => { authorRepository.isAuthor.mockResolvedValue(true); reviewerRepository.isReviewer.mockResolvedValue(true); const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); + expect(eligibility).toEqual({ + canDecide: true, + decisionIneligibilityReason: null, + canComment: true, + }); + }); + + it('stops a non-assigned author from approving their own review', async () => { + authorRepository.isAuthor.mockResolvedValue(true); + reviewerRepository.isReviewer.mockResolvedValue(false); + + const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); + expect(eligibility).toEqual({ canDecide: false, decisionIneligibilityReason: 'author', @@ -129,6 +142,7 @@ describe('WorkflowReviewEligibilityService', () => { it('still stops an author whose project-admin rights are in another project', async () => { authorRepository.isAuthor.mockResolvedValue(true); + reviewerRepository.isReviewer.mockResolvedValue(false); projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue(['other-proj']); const eligibility = await service.resolveViewerEligibility(memberUser(), readable()); 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 677f4a13b42..3bfc8d6d753 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 @@ -231,12 +231,23 @@ describe('WorkflowReviewRequestService.decide', () => { }); describe('author eligibility', () => { - it('throws ForbiddenError for an author without an admin override, even when assigned', async () => { + it('allows an assigned reviewer to decide even when they authored a version', async () => { mockSuccessfulDecidePath(); authorRepository.isAuthor.mockResolvedValue(true); reviewerRepository.isReviewer.mockResolvedValue(true); projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]); + const result = await service.decide(memberUser(), requestId, approveDto); + + expect(result.decision).toBe('approved'); + }); + + it('throws ForbiddenError for a non-assigned author without an admin override', async () => { + mockSuccessfulDecidePath(); + authorRepository.isAuthor.mockResolvedValue(true); + reviewerRepository.isReviewer.mockResolvedValue(false); + projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]); + await expect(service.decide(memberUser(), requestId, approveDto)).rejects.toThrow( ForbiddenError, ); @@ -244,11 +255,12 @@ 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. + // The missing note is a payload problem, and a non-reviewer 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); + reviewerRepository.isReviewer.mockResolvedValue(false); projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]); await expect( @@ -256,23 +268,33 @@ describe('WorkflowReviewRequestService.decide', () => { ).rejects.toThrow(ForbiddenError); }); - it('rejects a caller who became an author while waiting for the lock', async () => { + it('still allows an assigned reviewer 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 // added them to the author set before the critical section runs. authorRepository.isAuthor.mockResolvedValueOnce(false).mockResolvedValueOnce(true); projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]); - await expect(service.decide(memberUser(), requestId, approveDto)).rejects.toThrow( - ForbiddenError, - ); + const result = await service.decide(memberUser(), requestId, approveDto); - // The re-check must run against the lock's transaction, not a fresh read. + expect(result.decision).toBe('approved'); expect(authorRepository.isAuthor).toHaveBeenNthCalledWith( 2, expect.objectContaining({ workflowReviewRequestId: requestId }), ctx, ); + expect(requestRepository.saveRequest).toHaveBeenCalled(); + }); + + it('rejects a caller unassigned while waiting for the lock', async () => { + mockSuccessfulDecidePath(); + reviewerRepository.isReviewer.mockResolvedValueOnce(true).mockResolvedValueOnce(false); + authorRepository.isAuthor.mockResolvedValue(false); + projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue([]); + + await expect(service.decide(memberUser(), requestId, approveDto)).rejects.toThrow( + NotFoundError, + ); expect(requestRepository.saveRequest).not.toHaveBeenCalled(); }); @@ -309,6 +331,7 @@ describe('WorkflowReviewRequestService.decide', () => { it('throws ForbiddenError for an author who is only a project admin elsewhere', async () => { mockSuccessfulDecidePath(); authorRepository.isAuthor.mockResolvedValue(true); + reviewerRepository.isReviewer.mockResolvedValue(false); projectRelationRepository.getAccessibleProjectsByRoles.mockResolvedValue(['other-proj']); await expect(service.decide(memberUser(), requestId, approveDto)).rejects.toThrow( 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 e68fc21558f..6e548db8e81 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 @@ -1589,7 +1589,7 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', () }); test('returns 403 for the requesting author without admin override', async () => { - const { request } = await seedRequest(member); + const { request } = await seedRequest(member, {}, [owner.id]); await memberAgent .post(`/workflow-review-requests/${request.id}/decision`) @@ -1602,7 +1602,7 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', () }); }); - test('returns 403 for a user who became an author via update-version', async () => { + test('allows an assigned reviewer to decide after they update the review version', async () => { const { request, workflow } = await seedRequest(owner); await createWorkflowHistoryItem(workflow.id, { versionId: 'version-2' }); @@ -1618,7 +1618,7 @@ describe('POST /workflow-review-requests/:workflowReviewRequestId/decision', () await memberAgent .post(`/workflow-review-requests/${request.id}/decision`) .send({ decision: 'approved' }) - .expect(403); + .expect(200); }); test('allows the instance owner to decide their own review (admin override)', async () => { @@ -3358,7 +3358,7 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => { // `missing_reviewer_permission` cannot happen over HTTP. The eligibility service // unit tests cover that branch. - test('tells an assigned author why they cannot decide their own review', async () => { + test('lets an assigned author decide', async () => { const workflow = await createWorkflow({}, teamProject); const request = await seedRequest(workflow.id, null, member); await reviewerRepository.addReviewers( @@ -3368,6 +3368,16 @@ describe('GET /workflow-review-requests/:workflowReviewRequestId', () => { const response = await memberAgent.get(`/workflow-review-requests/${request.id}`).expect(200); + expect(response.body.data.viewerCanDecide).toBe(true); + expect(response.body.data.viewerDecisionIneligibilityReason).toBeNull(); + }); + + test('tells a non-assigned author why they cannot decide their own review', async () => { + const workflow = await createWorkflow({}, teamProject); + const request = await seedRequest(workflow.id, null, member); + + const response = await memberAgent.get(`/workflow-review-requests/${request.id}`).expect(200); + expect(response.body.data.viewerCanDecide).toBe(false); expect(response.body.data.viewerDecisionIneligibilityReason).toBe('author'); }); diff --git a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-eligibility.service.ts b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-eligibility.service.ts index 1e7a4f963fd..51556f05b2a 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/workflow-review-eligibility.service.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/workflow-review-eligibility.service.ts @@ -54,8 +54,9 @@ export class WorkflowReviewEligibilityService { /** * `canDecide` is an advisory read-time snapshot of whether the viewer could decide * the request, mirroring `decide()`'s authorization checks in order (read on the - * pinned workflow, then admin / author / assignee) so the surfaced reason matches - * the error the endpoint would return. The endpoint remains the source of truth + * pinned workflow, then admin / assignee / author) so the surfaced reason matches + * the error the endpoint would return. Assigned reviewers stay eligible even if + * they later submitted a version. The endpoint remains the source of truth * and re-checks under its lock. * * Deliberately viewer-scoped: `decide()`'s `assertRequestUpdatable` lifecycle @@ -102,6 +103,14 @@ export class WorkflowReviewEligibilityService { return { canDecide: true, decisionIneligibilityReason: null, canComment: true }; } + const isAssignedReviewer = await this.workflowReviewRequestReviewerRepository.isReviewer( + { workflowReviewRequestId: request.id, userId: user.id }, + {}, + ); + if (isAssignedReviewer) { + return { canDecide: true, decisionIneligibilityReason: null, canComment: true }; + } + if (isAuthor) { return { canDecide: false, @@ -110,18 +119,10 @@ export class WorkflowReviewEligibilityService { }; } - const isAssignedReviewer = await this.workflowReviewRequestReviewerRepository.isReviewer( - { workflowReviewRequestId: request.id, userId: user.id }, - {}, - ); - if (!isAssignedReviewer) { - return { - canDecide: false, - decisionIneligibilityReason: 'missing_reviewer_permission', - canComment: false, - }; - } - - return { canDecide: true, decisionIneligibilityReason: null, canComment: true }; + return { + canDecide: false, + decisionIneligibilityReason: 'missing_reviewer_permission', + canComment: false, + }; } } 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 7d467a9f1fb..133ad3adb70 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 @@ -714,7 +714,7 @@ export class WorkflowReviewRequestService { request.projectId, ); - // Fast path: reject a known author / non-assignee before queueing on the lock. + // Fast path: reject ineligible callers before queueing on the lock. const isAuthor = await this.workflowReviewRequestAuthorRepository.isAuthor( { workflowReviewRequestId, userId: user.id }, {}, @@ -754,10 +754,9 @@ export class WorkflowReviewRequestService { } this.assertRequestUpdatable(current); - // Re-check authorship here — a sync that won the lock first has - // added its syncer to the author set since the pre-lock check, and that - // syncer must not be able to decide. Assignment can also change; keep - // the same gate under the lock. + // Re-check authorship and assignment under the lock. A concurrent + // sync may add the caller as an author; assigned reviewers stay + // eligible. Assignment can also change. const isAuthorNow = await this.workflowReviewRequestAuthorRepository.isAuthor( { workflowReviewRequestId, userId: user.id }, ctx, @@ -938,10 +937,10 @@ export class WorkflowReviewRequestService { } /** - * Only assigned reviewers (or admins) may decide. Authors / version syncers - * are always blocked unless an admin override applies — being assigned does - * not lift that. Called before and again inside the decision lock, since the - * author set can change while the caller waits for the lock. The override is + * Assigned reviewers (or admins) may decide, including after they submit or + * sync a version. Non-reviewer authors stay blocked unless an admin override + * applies. Called before and again inside the decision lock, since the author + * and assignee sets can change while the caller waits. The override is * resolved once, pre-lock: the lock guards author/assignee rows, not role * membership — like every other authorization check in `decide`, roles are * evaluated up front. @@ -951,7 +950,7 @@ export class WorkflowReviewRequestService { isAssignedReviewer: boolean, hasAdminOverride: boolean, ): void { - if (hasAdminOverride) { + if (hasAdminOverride || isAssignedReviewer) { return; } @@ -959,9 +958,7 @@ export class WorkflowReviewRequestService { throw new ForbiddenError('Authors cannot decide on their own review request'); } - if (!isAssignedReviewer) { - throw new NotFoundError('Could not find workflow'); - } + throw new NotFoundError('Could not find workflow'); } /**