From 3b376dbbccc4fcbe49873290649998bfb65d48c1 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Tue, 21 Jul 2026 21:55:41 -0400 Subject: [PATCH 1/2] fix(isms): harden internal-audit validation and edge cases from deploy review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - reject a planned end date before the start date, client-side (zod cross-field rule with inline error) and server-side against the effective merged schedule on update - linked findings inherit the control row's reference as their clause label when none is given, and deleting a control row labels its label-less findings first — no finding is ever left unlabelled - add the optional Notes field to the add-control-row form - render the control-reference validation error in the row editor - preserve paragraph breaks when displaying the programme - don't count a whitespace-only signatory as signed - correct the F-NN reference doc comment (max+1 over surviving rows) --- .../isms/isms-audit-control.service.spec.ts | 19 +++++++ .../src/isms/isms-audit-control.service.ts | 8 ++- .../isms/isms-audit-finding.service.spec.ts | 40 +++++++++++++++ .../src/isms/isms-audit-finding.service.ts | 43 +++++++++++----- apps/api/src/isms/isms-audit.service.spec.ts | 49 +++++++++++++++++++ apps/api/src/isms/isms-audit.service.ts | 40 +++++++++++++-- .../isms/components/AuditControlRow.tsx | 21 +++++--- .../isms/components/AuditControlsTable.tsx | 15 ++++++ .../documents/isms/components/AuditFields.tsx | 19 ++++--- .../isms/components/AuditSignoffCard.tsx | 4 +- .../isms/components/ProgrammeCard.tsx | 6 ++- .../isms/components/audit-schema.test.ts | 49 +++++++++++++++++++ .../documents/isms/components/audit-schema.ts | 35 +++++++++---- 13 files changed, 307 insertions(+), 41 deletions(-) create mode 100644 apps/app/src/app/(app)/[orgId]/documents/isms/components/audit-schema.test.ts diff --git a/apps/api/src/isms/isms-audit-control.service.spec.ts b/apps/api/src/isms/isms-audit-control.service.spec.ts index bab3d64d9e..3ebd845f4a 100644 --- a/apps/api/src/isms/isms-audit-control.service.spec.ts +++ b/apps/api/src/isms/isms-audit-control.service.spec.ts @@ -15,6 +15,7 @@ jest.mock('@db', () => { update: jest.fn(), delete: jest.fn(), }, + ismsAuditFinding: { updateMany: jest.fn() }, $executeRaw: jest.fn(), $transaction: jest.fn((cb: (tx: unknown) => unknown) => cb(db)), }; @@ -133,6 +134,7 @@ describe('IsmsAuditControlService', () => { id: 'ac_1', documentId: 'doc_1', controlKey: 'clause_4_1_context', + controlRef: 'Clause 4.1 Context', }); (mockDb.ismsAuditControl.delete as jest.Mock).mockResolvedValue({}); @@ -146,5 +148,22 @@ describe('IsmsAuditControlService', () => { where: { id: 'ac_1' }, }); }); + + it('labels linked label-less findings with the row reference before deleting', async () => { + (mockDb.ismsAuditControl.findFirst as jest.Mock).mockResolvedValue({ + id: 'ac_1', + documentId: 'doc_1', + controlKey: null, + controlRef: 'A.8.13 Backup', + }); + (mockDb.ismsAuditControl.delete as jest.Mock).mockResolvedValue({}); + + await service.remove({ controlId: 'ac_1', organizationId: 'org_1' }); + + expect(mockDb.ismsAuditFinding.updateMany).toHaveBeenCalledWith({ + where: { controlId: 'ac_1', clauseOrControl: null }, + data: { clauseOrControl: 'A.8.13 Backup' }, + }); + }); }); }); diff --git a/apps/api/src/isms/isms-audit-control.service.ts b/apps/api/src/isms/isms-audit-control.service.ts index 3270bb05c5..70ac8645a7 100644 --- a/apps/api/src/isms/isms-audit-control.service.ts +++ b/apps/api/src/isms/isms-audit-control.service.ts @@ -99,7 +99,13 @@ export class IsmsAuditControlService { await lockDocument(tx, control.documentId); await invalidateApprovalIfNeeded({ tx, documentId: control.documentId }); // Findings linked to this row survive: the FK is SET NULL and each - // finding keeps its own clauseOrControl text as the frozen label. + // finding keeps its clauseOrControl text as the frozen label. A linked + // finding created without one (possible via the API) inherits the row's + // reference now, so no finding is left unlabelled by the deletion. + await tx.ismsAuditFinding.updateMany({ + where: { controlId, clauseOrControl: null }, + data: { clauseOrControl: control.controlRef }, + }); await tx.ismsAuditControl.delete({ where: { id: controlId } }); }); return { success: true }; diff --git a/apps/api/src/isms/isms-audit-finding.service.spec.ts b/apps/api/src/isms/isms-audit-finding.service.spec.ts index 980d1c1755..0cc22f7a6e 100644 --- a/apps/api/src/isms/isms-audit-finding.service.spec.ts +++ b/apps/api/src/isms/isms-audit-finding.service.spec.ts @@ -98,10 +98,50 @@ describe('IsmsAuditFindingService', () => { ).rejects.toThrow(NotFoundException); expect(mockDb.ismsAuditControl.findFirst).toHaveBeenCalledWith({ where: { id: 'ac_other', auditId: 'aud_1' }, + select: { id: true, controlRef: true }, }); expect(mockDb.ismsAuditFinding.create).not.toHaveBeenCalled(); }); + it('inherits the clause label from the linked control when none is given', async () => { + (mockDb.ismsAuditControl.findFirst as jest.Mock).mockResolvedValue({ + id: 'ac_1', + controlRef: 'Clause 9.1 Monitoring', + }); + + await service.create({ + ...args, + dto: { ...args.dto, controlId: 'ac_1' }, + }); + + expect(mockDb.ismsAuditFinding.create).toHaveBeenCalledWith({ + data: expect.objectContaining({ + controlId: 'ac_1', + clauseOrControl: 'Clause 9.1 Monitoring', + }), + }); + }); + + it('keeps an explicitly provided clause label over the linked control', async () => { + (mockDb.ismsAuditControl.findFirst as jest.Mock).mockResolvedValue({ + id: 'ac_1', + controlRef: 'Clause 9.1 Monitoring', + }); + + await service.create({ + ...args, + dto: { + ...args.dto, + controlId: 'ac_1', + clauseOrControl: 'Clause 9.1(b)', + }, + }); + + expect(mockDb.ismsAuditFinding.create).toHaveBeenCalledWith({ + data: expect.objectContaining({ clauseOrControl: 'Clause 9.1(b)' }), + }); + }); + it('rejects an owner that is not an active org member', async () => { (mockDb.member.findFirst as jest.Mock).mockResolvedValue(null); diff --git a/apps/api/src/isms/isms-audit-finding.service.ts b/apps/api/src/isms/isms-audit-finding.service.ts index ae1f028ecf..d6ebf7bff7 100644 --- a/apps/api/src/isms/isms-audit-finding.service.ts +++ b/apps/api/src/isms/isms-audit-finding.service.ts @@ -31,7 +31,7 @@ export class IsmsAuditFindingService { documentId, organizationId, }); - const controlId = await this.resolveControl({ + const control = await this.resolveControl({ controlId: dto.controlId, auditId: audit.id, }); @@ -54,8 +54,12 @@ export class IsmsAuditFindingService { documentId: audit.documentId, reference: await this.nextReference({ tx, auditId: audit.id }), type: dto.type, - controlId: controlId ?? null, - clauseOrControl: dto.clauseOrControl?.trim() || null, + controlId: control?.id ?? null, + // A linked finding without its own clause text inherits the row's + // reference, so unlinking (or deleting the row) never leaves the + // finding unlabelled in the generated document. + clauseOrControl: + dto.clauseOrControl?.trim() || control?.controlRef || null, description: dto.description, ownerMemberId: ownerMemberId ?? null, dueDate: dueDate ?? null, @@ -77,10 +81,17 @@ export class IsmsAuditFindingService { dto: UpdateAuditFindingInput; }) { const finding = await this.requireFinding({ findingId, organizationId }); - const controlId = await this.resolveControl({ + const control = await this.resolveControl({ controlId: dto.controlId, auditId: finding.auditId, }); + // Same inheritance on link change: when this update links a control and + // the finding's effective clause text is empty, derive it from the row. + const requestedClause = + dto.clauseOrControl === undefined + ? finding.clauseOrControl + : dto.clauseOrControl?.trim() || null; + const derivedClause = requestedClause ?? control?.controlRef ?? null; const ownerMemberId = await this.resolveMember({ memberId: dto.ownerMemberId, organizationId, @@ -95,11 +106,11 @@ export class IsmsAuditFindingService { where: { id: findingId }, data: { type: dto.type ?? undefined, - controlId: dto.controlId === undefined ? undefined : controlId, + controlId: dto.controlId === undefined ? undefined : (control?.id ?? null), clauseOrControl: - dto.clauseOrControl === undefined + dto.clauseOrControl === undefined && control === undefined ? undefined - : dto.clauseOrControl?.trim() || null, + : derivedClause, description: dto.description ?? undefined, ownerMemberId: dto.ownerMemberId === undefined ? undefined : ownerMemberId, @@ -131,7 +142,12 @@ export class IsmsAuditFindingService { return { success: true }; } - /** Next "F-NN" for the audit. Max + 1, so a deleted reference never recurs. */ + /** + * Next "F-NN" for the audit: max + 1 over the surviving rows, so deleting an + * older finding never reissues its number (deleting the newest frees it — + * acceptable: findings are draft rows until published, and every published + * version freezes its own findings in the version contentSnapshot). + */ private async nextReference({ tx, auditId, @@ -150,24 +166,29 @@ export class IsmsAuditFindingService { return `F-${String(maxSequence + 1).padStart(2, '0')}`; } - /** The linked Controls Tested row must belong to the SAME audit. */ + /** + * The linked Controls Tested row must belong to the SAME audit. Returns the + * row (id + reference) so callers can inherit the clause label; undefined = + * link untouched, null = explicitly unlinked. + */ private async resolveControl({ controlId, auditId, }: { controlId: string | null | undefined; auditId: string; - }): Promise { + }): Promise<{ id: string; controlRef: string } | null | undefined> { if (controlId === undefined) return undefined; const trimmed = controlId?.trim(); if (!trimmed) return null; const control = await db.ismsAuditControl.findFirst({ where: { id: trimmed, auditId }, + select: { id: true, controlRef: true }, }); if (!control) { throw new NotFoundException('Audit control row not found in this audit'); } - return trimmed; + return control; } private async resolveMember({ diff --git a/apps/api/src/isms/isms-audit.service.spec.ts b/apps/api/src/isms/isms-audit.service.spec.ts index 10dbb611a3..2cf1a2c001 100644 --- a/apps/api/src/isms/isms-audit.service.spec.ts +++ b/apps/api/src/isms/isms-audit.service.spec.ts @@ -137,6 +137,19 @@ describe('IsmsAuditService', () => { ).rejects.toThrow(BadRequestException); expect(mockDb.ismsAudit.create).not.toHaveBeenCalled(); }); + + it('rejects a planned end date before the start date', async () => { + (mockDb.ismsDocument.findFirst as jest.Mock).mockResolvedValue({ + id: 'doc_1', + }); + await expect( + service.create({ + ...args, + dto: { plannedStartDate: '2026-05-20', plannedEndDate: '2026-05-15' }, + }), + ).rejects.toThrow(BadRequestException); + expect(mockDb.ismsAudit.create).not.toHaveBeenCalled(); + }); }); describe('update', () => { @@ -144,10 +157,46 @@ describe('IsmsAuditService', () => { (mockDb.ismsAudit.findFirst as jest.Mock).mockResolvedValue({ id: 'aud_1', documentId: 'doc_1', + plannedStartDate: null, + plannedEndDate: null, }); (mockDb.ismsAudit.update as jest.Mock).mockResolvedValue({}); }); + it('rejects an end date before the STORED start date (merged schedule)', async () => { + (mockDb.ismsAudit.findFirst as jest.Mock).mockResolvedValue({ + id: 'aud_1', + documentId: 'doc_1', + plannedStartDate: new Date('2026-05-15T00:00:00.000Z'), + plannedEndDate: null, + }); + + await expect( + service.update({ + auditId: 'aud_1', + organizationId: 'org_1', + dto: { plannedEndDate: '2026-05-10' }, + }), + ).rejects.toThrow(BadRequestException); + expect(mockDb.ismsAudit.update).not.toHaveBeenCalled(); + }); + + it('allows fixing an inverted schedule by moving both dates together', async () => { + (mockDb.ismsAudit.findFirst as jest.Mock).mockResolvedValue({ + id: 'aud_1', + documentId: 'doc_1', + plannedStartDate: new Date('2026-05-15T00:00:00.000Z'), + plannedEndDate: null, + }); + + await service.update({ + auditId: 'aud_1', + organizationId: 'org_1', + dto: { plannedStartDate: '2026-06-01', plannedEndDate: '2026-06-05' }, + }); + expect(mockDb.ismsAudit.update).toHaveBeenCalled(); + }); + it('updates status and conclusion, leaving omitted fields untouched', async () => { await service.update({ auditId: 'aud_1', diff --git a/apps/api/src/isms/isms-audit.service.ts b/apps/api/src/isms/isms-audit.service.ts index 2bb105f26d..8421f1c41c 100644 --- a/apps/api/src/isms/isms-audit.service.ts +++ b/apps/api/src/isms/isms-audit.service.ts @@ -1,4 +1,8 @@ -import { Injectable, NotFoundException } from '@nestjs/common'; +import { + BadRequestException, + Injectable, + NotFoundException, +} from '@nestjs/common'; import { db } from '@db'; import type { Prisma } from '@db'; import { seedAuditControlsIfMissing } from './documents/internal-audit'; @@ -35,6 +39,10 @@ export class IsmsAuditService { await this.requireDocument({ documentId, organizationId }); const plannedStartDate = parseOptionalDate(dto.plannedStartDate); const plannedEndDate = parseOptionalDate(dto.plannedEndDate); + this.assertDateOrder({ + start: plannedStartDate ?? null, + end: plannedEndDate ?? null, + }); return db.$transaction(async (tx) => { // The lock also serializes reference generation, so concurrent creates @@ -71,6 +79,17 @@ export class IsmsAuditService { dto: UpdateAuditInput; }) { const audit = await this.requireAudit({ auditId, organizationId }); + const plannedStartDate = parseOptionalDate(dto.plannedStartDate); + const plannedEndDate = parseOptionalDate(dto.plannedEndDate); + // Validate the EFFECTIVE schedule: a field omitted from this update keeps + // its stored value, so the check merges dto values over the existing row. + this.assertDateOrder({ + start: + plannedStartDate === undefined + ? audit.plannedStartDate + : plannedStartDate, + end: plannedEndDate === undefined ? audit.plannedEndDate : plannedEndDate, + }); return db.$transaction(async (tx) => { // Same per-document lock submit/approve take, so an edit can't race the @@ -86,8 +105,8 @@ export class IsmsAuditService { dto.auditorName === undefined ? undefined : dto.auditorName?.trim() || null, - plannedStartDate: parseOptionalDate(dto.plannedStartDate), - plannedEndDate: parseOptionalDate(dto.plannedEndDate), + plannedStartDate, + plannedEndDate, status: dto.status ?? undefined, conclusionVerdict: dto.conclusionVerdict === undefined @@ -134,6 +153,21 @@ export class IsmsAuditService { return value?.trim() || null; } + /** A planned schedule must not end before it starts (client gate mirrored). */ + private assertDateOrder({ + start, + end, + }: { + start: Date | null; + end: Date | null; + }): void { + if (start && end && end.getTime() < start.getTime()) { + throw new BadRequestException( + 'Planned end date must be on or after the planned start date', + ); + } + } + /** * Next "IA-YYYY-NN" for the document: the year of creation plus a two-digit * per-document, per-year sequence. Max + 1 over the surviving rows, so diff --git a/apps/app/src/app/(app)/[orgId]/documents/isms/components/AuditControlRow.tsx b/apps/app/src/app/(app)/[orgId]/documents/isms/components/AuditControlRow.tsx index b01dcafbce..61fe6b9182 100644 --- a/apps/app/src/app/(app)/[orgId]/documents/isms/components/AuditControlRow.tsx +++ b/apps/app/src/app/(app)/[orgId]/documents/isms/components/AuditControlRow.tsx @@ -11,6 +11,8 @@ import { AlertDialogHeader, AlertDialogTitle, Button, + Field, + FieldError, Input, Select, SelectContent, @@ -136,13 +138,18 @@ export function AuditControlRow({ return ( - ( - - )} - /> + + ( + <> + + {fieldState.error?.message} + + )} + /> + + + ( +