From 440f0cc7a0befed74acf6ce7b49357639d94464b Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Tue, 16 Jun 2026 16:32:06 -0400 Subject: [PATCH] fix(tasks): respect evidenceApprovalEnabled when gating task completion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tasks could become permanently un-completable. The updateTask gate that blocks "mark as done" when a task has an approverId never consulted the org's evidenceApprovalEnabled flag, while the UI hides the approver dropdown and submit-for-review flow whenever approval is disabled. A task carrying a stale approverId (e.g. approveTask leaves it set, then the recurring task-schedule cron resets the task off "done" without clearing it) was wedged: no UI path to clear the approver, yet the API refused a direct "done" — surfacing the misleading "Submit for review instead". - updateTask: load evidenceApprovalEnabled (folded into the existing task query), only enforce the in_review / approver locks when approval is enabled, and self-heal a stale approverId when approval is off (unless the caller explicitly sets one). - updateTasksStatus (bulk): same blind spot — the in_review / approverId where-clause constraints silently dropped tasks. Gate them on the flag. - Add tasks.service.spec.ts covering the regression, the preserved approval-enabled workflow, the symmetric in_review case, and edge cases. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/api/src/tasks/tasks.service.spec.ts | 220 +++++++++++++++++++++++ apps/api/src/tasks/tasks.service.ts | 81 ++++++--- 2 files changed, 276 insertions(+), 25 deletions(-) create mode 100644 apps/api/src/tasks/tasks.service.spec.ts diff --git a/apps/api/src/tasks/tasks.service.spec.ts b/apps/api/src/tasks/tasks.service.spec.ts new file mode 100644 index 0000000000..29c3b802a9 --- /dev/null +++ b/apps/api/src/tasks/tasks.service.spec.ts @@ -0,0 +1,220 @@ +// Mock the DB layer before importing the service. We spread the real Prisma +// client so generated enums (TaskStatus, TaskFrequency) keep their values. +jest.mock('@db', () => ({ + ...jest.requireActual('@prisma/client'), + db: { + task: { + findFirst: jest.fn(), + update: jest.fn(), + updateMany: jest.fn(), + }, + organization: { + findUnique: jest.fn(), + }, + member: { + findFirst: jest.fn(), + findUnique: jest.fn(), + }, + auditLog: { + create: jest.fn(), + }, + }, +})); + +// checkAutoCompletePhases reaches into the DB; stub it out for unit tests. +jest.mock('../frameworks/frameworks-timeline.helper', () => ({ + checkAutoCompletePhases: jest.fn().mockResolvedValue(undefined), +})); + +// The service's transitive imports pull in @trycompai/auth (ESM better-auth) +// which jest can't transform; stub it as the controller spec does. +jest.mock('@trycompai/auth', () => ({ + statement: { + task: ['create', 'read', 'update', 'delete'], + evidence: ['create', 'read', 'delete'], + }, + BUILT_IN_ROLE_PERMISSIONS: {}, +})); + +jest.mock('../auth/auth.server', () => ({ + auth: { api: { getSession: jest.fn() } }, +})); + +import { Test } from '@nestjs/testing'; +import { db, TaskStatus } from '@db'; +import { TaskNotifierService } from './task-notifier.service'; +import { TimelinesService } from '../timelines/timelines.service'; +import { TasksService } from './tasks.service'; + +const taskFindFirst = db.task.findFirst as jest.Mock; +const taskUpdate = db.task.update as jest.Mock; +const taskUpdateMany = db.task.updateMany as jest.Mock; +const orgFindUnique = db.organization.findUnique as jest.Mock; +const memberFindFirst = db.member.findFirst as jest.Mock; +const auditLogCreate = db.auditLog.create as jest.Mock; + +const ORG_ID = 'org_1'; +const TASK_ID = 'tsk_1'; +const USER_ID = 'usr_1'; + +// Builds the existingTask row updateTask() reads. evidenceApprovalEnabled is +// folded into the task's organization relation (a single query); pass null to +// simulate a missing organization row. +const existing = ( + approvalEnabled: boolean | null, + overrides: Record = {}, +) => ({ + id: TASK_ID, + title: 'Test task', + status: TaskStatus.todo, + assigneeId: null, + approverId: null, + frequency: null, + organization: + approvalEnabled === null + ? null + : { evidenceApprovalEnabled: approvalEnabled }, + ...overrides, +}); + +describe('TasksService approval gating', () => { + let service: TasksService; + + const notifier = { + notifyStatusChange: jest.fn().mockResolvedValue(undefined), + notifyAssigneeChange: jest.fn().mockResolvedValue(undefined), + notifyBulkStatusChange: jest.fn().mockResolvedValue(undefined), + }; + + beforeEach(async () => { + jest.clearAllMocks(); + const moduleRef = await Test.createTestingModule({ + providers: [ + TasksService, + { provide: TaskNotifierService, useValue: notifier }, + { provide: TimelinesService, useValue: {} }, + ], + }).compile(); + service = moduleRef.get(TasksService); + memberFindFirst.mockResolvedValue({ id: 'mem_actor', user: { role: 'owner' } }); + taskUpdate.mockResolvedValue({ id: TASK_ID }); + auditLogCreate.mockResolvedValue({}); + }); + + const lastUpdateData = () => taskUpdate.mock.calls[0][0].data; + + describe('updateTask', () => { + it('REGRESSION: approval OFF + stale approver allows todo→done and clears the approver', async () => { + taskFindFirst.mockResolvedValue(existing(false, { approverId: 'mem_appr' })); + + await service.updateTask(ORG_ID, TASK_ID, { status: TaskStatus.done }, USER_ID); + + expect(taskUpdate).toHaveBeenCalledTimes(1); + expect(lastUpdateData().status).toBe(TaskStatus.done); + expect(lastUpdateData().approverId).toBeNull(); + }); + + it('PRESERVE: approval ON + approver assigned blocks a direct todo→done', async () => { + taskFindFirst.mockResolvedValue(existing(true, { approverId: 'mem_appr' })); + + await expect( + service.updateTask(ORG_ID, TASK_ID, { status: TaskStatus.done }, USER_ID), + ).rejects.toThrow( + 'Cannot mark task as done directly when an approver is assigned. Submit for review instead.', + ); + expect(taskUpdate).not.toHaveBeenCalled(); + }); + + it('ALLOWED: approval ON + no approver allows todo→done', async () => { + taskFindFirst.mockResolvedValue(existing(true, { approverId: null })); + + await service.updateTask(ORG_ID, TASK_ID, { status: TaskStatus.done }, USER_ID); + + expect(taskUpdate).toHaveBeenCalledTimes(1); + expect(lastUpdateData().status).toBe(TaskStatus.done); + }); + + it('SYMMETRIC: approval OFF lets an in_review task move to done and clears the approver', async () => { + taskFindFirst.mockResolvedValue( + existing(false, { status: TaskStatus.in_review, approverId: 'mem_appr' }), + ); + + await service.updateTask(ORG_ID, TASK_ID, { status: TaskStatus.done }, USER_ID); + + expect(lastUpdateData().status).toBe(TaskStatus.done); + expect(lastUpdateData().approverId).toBeNull(); + }); + + it('IN_REVIEW LOCK: approval ON blocks moving an in_review task directly', async () => { + taskFindFirst.mockResolvedValue(existing(true, { status: TaskStatus.in_review })); + + await expect( + service.updateTask(ORG_ID, TASK_ID, { status: TaskStatus.todo }, USER_ID), + ).rejects.toThrow('Cannot change status directly while task is in review'); + expect(taskUpdate).not.toHaveBeenCalled(); + }); + + it('SELF-HEAL GUARD: an explicit approverId from the caller is not force-cleared', async () => { + taskFindFirst.mockResolvedValue(existing(false, { approverId: 'mem_old' })); + + await service.updateTask(ORG_ID, TASK_ID, { approverId: 'mem_new' }, USER_ID); + + expect(lastUpdateData().approverId).toBe('mem_new'); + }); + + it('NO-OP: approval OFF + no approver leaves approverId untouched on todo→done', async () => { + taskFindFirst.mockResolvedValue(existing(false, { approverId: null })); + + await service.updateTask(ORG_ID, TASK_ID, { status: TaskStatus.done }, USER_ID); + + expect(lastUpdateData().status).toBe(TaskStatus.done); + expect(lastUpdateData().approverId).toBeUndefined(); + }); + + it('DEFENSIVE: a missing organization row is treated as approval disabled', async () => { + taskFindFirst.mockResolvedValue(existing(null, { approverId: 'mem_appr' })); + + await service.updateTask(ORG_ID, TASK_ID, { status: TaskStatus.done }, USER_ID); + + expect(lastUpdateData().status).toBe(TaskStatus.done); + expect(lastUpdateData().approverId).toBeNull(); + }); + }); + + describe('updateTasksStatus (bulk)', () => { + it('approval OFF: bulk done does not exclude approver-assigned / in_review tasks', async () => { + orgFindUnique.mockResolvedValue({ evidenceApprovalEnabled: false }); + taskUpdateMany.mockResolvedValue({ count: 1 }); + + await service.updateTasksStatus( + ORG_ID, + [TASK_ID], + TaskStatus.done, + undefined, + USER_ID, + ); + + const where = taskUpdateMany.mock.calls[0][0].where; + expect(where).toEqual({ id: { in: [TASK_ID] }, organizationId: ORG_ID }); + expect(where.approverId).toBeUndefined(); + expect(where.status).toBeUndefined(); + }); + + it('approval ON: bulk done excludes approver-assigned and in_review tasks', async () => { + orgFindUnique.mockResolvedValue({ evidenceApprovalEnabled: true }); + taskUpdateMany.mockResolvedValue({ count: 1 }); + + await service.updateTasksStatus( + ORG_ID, + [TASK_ID], + TaskStatus.done, + undefined, + USER_ID, + ); + + const where = taskUpdateMany.mock.calls[0][0].where; + expect(where.approverId).toBeNull(); + expect(where.status).toEqual({ not: 'in_review' }); + }); + }); +}); diff --git a/apps/api/src/tasks/tasks.service.ts b/apps/api/src/tasks/tasks.service.ts index 8be182dcf0..f6cfa76ba1 100644 --- a/apps/api/src/tasks/tasks.service.ts +++ b/apps/api/src/tasks/tasks.service.ts @@ -375,17 +375,29 @@ export class TasksService { notRelevantJustification?: string, ): Promise<{ updatedCount: number }> { try { - // Enforce approval workflow: exclude tasks that can't be bulk-updated + // The approval-workflow constraints only apply when evidence approval is + // enabled for the org. With it disabled, applying them would silently drop + // tasks carrying a stale approverId / in_review status from the bulk + // update (and throw "No tasks were updated" if all targets are dropped). + const organization = await db.organization.findUnique({ + where: { id: organizationId }, + select: { evidenceApprovalEnabled: true }, + }); + const evidenceApprovalEnabled = + organization?.evidenceApprovalEnabled ?? false; + const where: Record = { id: { in: taskIds }, organizationId, - // Cannot change status of tasks currently in review - status: { not: 'in_review' as TaskStatus }, }; - // Cannot mark tasks as done if they have an approver assigned - if (status === TaskStatus.done) { - where.approverId = null; + if (evidenceApprovalEnabled) { + // Cannot change status of tasks currently in review + where.status = { not: 'in_review' as TaskStatus }; + // Cannot mark tasks as done if they have an approver assigned + if (status === TaskStatus.done) { + where.approverId = null; + } } const justificationData = @@ -586,6 +598,12 @@ export class TasksService { status: true, assigneeId: true, approverId: true, + // The submit-for-review → approve workflow only applies when the org + // has evidence approval enabled. When it's disabled, the approver + // dropdown and review flow are hidden in the UI, so a leftover + // approverId / in_review status must never block a direct status + // change (otherwise the task is wedged with no way to clear it). + organization: { select: { evidenceApprovalEnabled: true } }, }, }); @@ -593,6 +611,9 @@ export class TasksService { throw new BadRequestException('Task not found or access denied'); } + const evidenceApprovalEnabled = + existingTask.organization?.evidenceApprovalEnabled ?? false; + // Prepare update data - Prisma handles updatedAt automatically const dataToUpdate: { title?: string; @@ -614,25 +635,30 @@ export class TasksService { dataToUpdate.description = updateData.description; } if (updateData.status !== undefined) { - // Prevent bypassing the approval workflow via direct status change - if ( - existingTask.status === 'in_review' && - updateData.status !== 'in_review' - ) { - throw new BadRequestException( - 'Cannot change status directly while task is in review. Use the approve or reject actions instead.', - ); - } - // Prevent directly setting status to 'done' when an approver is assigned - // (must go through submitForReview → approveTask workflow) - if ( - updateData.status === 'done' && - existingTask.status !== 'done' && - existingTask.approverId - ) { - throw new BadRequestException( - 'Cannot mark task as done directly when an approver is assigned. Submit for review instead.', - ); + // Only enforce the approval-workflow locks when evidence approval is + // enabled for the org. With it disabled, these would wedge a task that + // carries a stale approverId / in_review status (see comment above). + if (evidenceApprovalEnabled) { + // Prevent bypassing the approval workflow via direct status change + if ( + existingTask.status === 'in_review' && + updateData.status !== 'in_review' + ) { + throw new BadRequestException( + 'Cannot change status directly while task is in review. Use the approve or reject actions instead.', + ); + } + // Prevent directly setting status to 'done' when an approver is assigned + // (must go through submitForReview → approveTask workflow) + if ( + updateData.status === 'done' && + existingTask.status !== 'done' && + existingTask.approverId + ) { + throw new BadRequestException( + 'Cannot mark task as done directly when an approver is assigned. Submit for review instead.', + ); + } } dataToUpdate.status = updateData.status; @@ -661,6 +687,11 @@ export class TasksService { if (updateData.approverId !== undefined) { dataToUpdate.approverId = updateData.approverId === null ? null : updateData.approverId; + } else if (!evidenceApprovalEnabled && existingTask.approverId) { + // Self-heal: with approval disabled, a leftover approverId is invisible + // in the UI and non-functional — clear it on any update so it can't + // silently re-block future status changes. + dataToUpdate.approverId = null; } if (updateData.frequency !== undefined) { dataToUpdate.frequency = updateData.frequency;