diff --git a/apps/api/src/isms/documents/management-review-defaults.ts b/apps/api/src/isms/documents/management-review-defaults.ts index 1c7d546d83..ad6ad30f45 100644 --- a/apps/api/src/isms/documents/management-review-defaults.ts +++ b/apps/api/src/isms/documents/management-review-defaults.ts @@ -74,8 +74,9 @@ export const SEED_REVIEW_INPUT_DEFINITIONS: SeedReviewInputDefinition[] = [ inputKey: 'c_interested_party_changes', inputRef: '(c) Interested-party changes', whatItCovers: 'Changes in needs and expectations of interested parties.', + // Canonical document titles so the agenda reference is followable. whereToFind: - 'Comp AI > ISMS > Documents > Interested Parties Register & Requirements', + 'Comp AI > ISMS > Documents > Interested Parties Register & Interested Parties Requirements', }, { inputKey: 'd1_nonconformity_trends', @@ -100,7 +101,9 @@ export const SEED_REVIEW_INPUT_DEFINITIONS: SeedReviewInputDefinition[] = [ inputRef: '(d.4) Objectives fulfilment', whatItCovers: 'Extent to which information security objectives have been met.', - whereToFind: 'Comp AI > ISMS > Documents > Objectives Plan', + // Canonical document title so the agenda reference is followable. + whereToFind: + 'Comp AI > ISMS > Documents > Information Security Objectives and Plan', }, { inputKey: 'e_interested_party_feedback', diff --git a/apps/api/src/isms/documents/management-review.spec.ts b/apps/api/src/isms/documents/management-review.spec.ts index 59ef084277..09ac5342ce 100644 --- a/apps/api/src/isms/documents/management-review.spec.ts +++ b/apps/api/src/isms/documents/management-review.spec.ts @@ -61,6 +61,13 @@ describe('parseReviewAttendees / isReviewSigned', () => { expect(parseReviewAttendees(null)).toEqual([]); expect(parseReviewAttendees('not-an-array')).toEqual([]); expect(parseReviewAttendees([{ memberId: 'mem_1' }])).toEqual([]); + // Legacy rows may still carry duplicates — normalized on read. + expect( + parseReviewAttendees([ + { memberId: 'mem_1', name: 'Jane' }, + { memberId: 'mem_1', name: 'Jane (dup)' }, + ]), + ).toEqual([{ memberId: 'mem_1', name: 'Jane' }]); }); it('dedupes attendees by member, first occurrence winning', () => { diff --git a/apps/api/src/isms/documents/management-review.ts b/apps/api/src/isms/documents/management-review.ts index 46d4cd26f9..047eefc61c 100644 --- a/apps/api/src/isms/documents/management-review.ts +++ b/apps/api/src/isms/documents/management-review.ts @@ -44,10 +44,16 @@ export type ReviewAttendee = z.infer; export const reviewAttendeesSchema = z.array(reviewAttendeeSchema); -/** Parse a stored attendees JSON value defensively (invalid → empty list). */ +/** + * Parse a stored attendees JSON value defensively (invalid → empty list, + * all-or-nothing like the write schema) and normalize away duplicate members + * a legacy row may still carry. The client mirror (parseAttendees in + * management-review-constants.ts) applies the same rules so the Submit UI + * and the server gate always agree. + */ export function parseReviewAttendees(value: unknown): ReviewAttendee[] { const parsed = reviewAttendeesSchema.safeParse(value); - return parsed.success ? parsed.data : []; + return parsed.success ? dedupeReviewAttendees(parsed.data) : []; } /** A review is signed once the chair's name and date are both captured. */ diff --git a/apps/api/src/isms/isms.service.spec.ts b/apps/api/src/isms/isms.service.spec.ts index d3c5bb690e..35acc92c8b 100644 --- a/apps/api/src/isms/isms.service.spec.ts +++ b/apps/api/src/isms/isms.service.spec.ts @@ -130,7 +130,16 @@ describe('IsmsService ensureSetup', () => { (mockDb.ismsDocument.findMany as jest.Mock) .mockResolvedValueOnce([]) // existing-types probe .mockResolvedValueOnce([{ id: 'doc_ia', type: 'internal_audit' }]) // created lookup - .mockResolvedValueOnce([]); // final list + .mockResolvedValueOnce([ + { + id: 'doc_ia', + type: 'internal_audit', + status: 'draft', + requirementId: null, + currentVersionId: null, + draftNarrative: null, + }, + ]); // final list — the seed now works off this, not the created lookup await service.ensureSetup(dto); @@ -156,6 +165,70 @@ describe('IsmsService ensureSetup', () => { }); }); + it('heals a PRE-EXISTING Management Review doc whose procedure is still empty', async () => { + ( + mockDb.frameworkEditorFramework.findUnique as jest.Mock + ).mockResolvedValue({ id: 'fw_1', requirements: [] }); + mockTemplates.mockResolvedValue([]); + (mockDb.organization.findUnique as jest.Mock).mockResolvedValue({ + name: 'Acme Corp', + }); + (mockDb.ismsDocument.findMany as jest.Mock) + // Every type already exists, so provisioning creates nothing and the + // old created-scoped seed would never have run. + .mockResolvedValueOnce([ + { type: 'context_of_organization' }, + { type: 'interested_parties_register' }, + { type: 'interested_parties_requirements' }, + { type: 'isms_scope' }, + { type: 'leadership_commitment' }, + { type: 'roles_and_responsibilities' }, + { type: 'objectives_plan' }, + { type: 'monitoring' }, + { type: 'internal_audit' }, + { type: 'management_review' }, + ]) + .mockResolvedValueOnce([ + { + id: 'doc_mr', + type: 'management_review', + status: 'draft', + requirementId: null, + currentVersionId: null, + draftNarrative: {}, // blank — e.g. a crash between provision and seed + }, + { + id: 'doc_ia', + type: 'internal_audit', + status: 'draft', + requirementId: null, + currentVersionId: null, + draftNarrative: { programme: 'Customer-edited programme.' }, + }, + ]); // final list + + await service.ensureSetup(dto); + + // Only the blank document is healed; the populated one is untouched. + expect(mockDb.ismsDocument.updateMany).toHaveBeenCalledTimes(1); + expect(mockDb.ismsDocument.updateMany).toHaveBeenCalledWith({ + where: { + id: 'doc_mr', + OR: [ + { draftNarrative: { equals: Prisma.AnyNull } }, + { draftNarrative: { equals: {} } }, + ], + }, + data: { + draftNarrative: { + procedure: expect.stringContaining( + 'Acme Corp holds a management review', + ), + }, + }, + }); + }); + it('reports overdueMetricCount on the monitoring document row (CS-723)', async () => { const { addPeriods, periodStartFor } = jest.requireActual< typeof import('./utils/metric-periods') diff --git a/apps/api/src/isms/isms.service.ts b/apps/api/src/isms/isms.service.ts index 9394d76703..25cd90a1c0 100644 --- a/apps/api/src/isms/isms.service.ts +++ b/apps/api/src/isms/isms.service.ts @@ -5,6 +5,7 @@ import { NotFoundException, } from '@nestjs/common'; import { db, Prisma } from '@db'; +import type { IsmsDocumentType } from '@db'; import { SubmitIsmsForApprovalDto } from './dto/submit-isms-for-approval.dto'; import { deriveControlLinks, resolveDocumentPlans } from './utils/ensure-setup-plan'; import { collectPlatformData } from './documents/data-source'; @@ -79,6 +80,15 @@ export class IsmsService { where: { organizationId, frameworkId }, }); + // Heal empty Programme (9.2) / Procedure (9.3) paragraphs on EVERY setup, + // not just for documents created this call: a document provisioned before + // its narrative seed shipped (or a crash between provision and seed) would + // otherwise stay blank forever. Guarded per doc on the narrative still + // being empty, so a populated draft is never overwritten. + if (canWrite) { + await this.seedNarrativeDefaultsIfEmpty({ documents, organizationId }); + } + const monitoringDoc = documents.find((doc) => doc.type === 'monitoring'); const overdueMetricCount = monitoringDoc ? await this.countOverdueMetrics(monitoringDoc.id) @@ -223,53 +233,65 @@ export class IsmsService { ); } - // Same first-load guarantee for Internal Audit (9.2) and Management Review - // (9.3): the Programme / Procedure paragraph opens with its default text. - // Each write is conditional on the narrative still being NULL (its - // creation state), so it is atomic: under concurrent setup calls — where - // the "created" lookup can also match a row the other call just created — - // an early customer edit can never be clobbered (the seed simply matches - // zero rows). - const internalAuditDoc = created.find( - (doc) => doc.type === 'internal_audit', - ); - const managementReviewDoc = created.find( - (doc) => doc.type === 'management_review', + // The Programme (9.2) / Procedure (9.3) narrative seed happens in + // ensureSetup (seedNarrativeDefaultsIfEmpty), AFTER the document list is + // fetched — so it covers both the documents this call just created and + // any pre-existing document whose narrative is still empty. + } + + /** + * First-load guarantee for Internal Audit (9.2) and Management Review (9.3): + * the Programme / Procedure paragraph opens with its default text. Each + * write is conditional on the narrative still being empty — NULL (the + * creation state) or {}, generateNarrative's definition — so it is atomic: + * under concurrent setup calls, an early customer edit can never be + * clobbered (the seed simply matches zero rows). + */ + private async seedNarrativeDefaultsIfEmpty({ + documents, + organizationId, + }: { + documents: Array<{ + id: string; + type: IsmsDocumentType; + draftNarrative: Prisma.JsonValue; + }>; + organizationId: string; + }) { + const isEmptyNarrative = (narrative: Prisma.JsonValue): boolean => + narrative == null || + (typeof narrative === 'object' && + !Array.isArray(narrative) && + Object.keys(narrative).length === 0); + + const targets = documents.filter( + (doc) => + (doc.type === 'internal_audit' || doc.type === 'management_review') && + isEmptyNarrative(doc.draftNarrative), ); - if (internalAuditDoc || managementReviewDoc) { - const organization = await db.organization.findUnique({ - where: { id: organizationId }, - select: { name: true }, + if (targets.length === 0) return; + + const organization = await db.organization.findUnique({ + where: { id: organizationId }, + select: { name: true }, + }); + const organizationName = organization?.name ?? 'The organization'; + const whileNarrativeEmpty = { + OR: [ + { draftNarrative: { equals: Prisma.AnyNull } }, + { draftNarrative: { equals: {} } }, + ], + }; + for (const doc of targets) { + await db.ismsDocument.updateMany({ + where: { id: doc.id, ...whileNarrativeEmpty }, + data: { + draftNarrative: + doc.type === 'internal_audit' + ? { programme: defaultProgrammeText(organizationName) } + : { procedure: defaultProcedureText(organizationName) }, + }, }); - const organizationName = organization?.name ?? 'The organization'; - // "Empty" matches generateNarrative's definition: NULL (the creation - // state) or an empty object — never a populated draft. - const whileNarrativeEmpty = { - OR: [ - { draftNarrative: { equals: Prisma.AnyNull } }, - { draftNarrative: { equals: {} } }, - ], - }; - if (internalAuditDoc) { - await db.ismsDocument.updateMany({ - where: { id: internalAuditDoc.id, ...whileNarrativeEmpty }, - data: { - draftNarrative: { - programme: defaultProgrammeText(organizationName), - }, - }, - }); - } - if (managementReviewDoc) { - await db.ismsDocument.updateMany({ - where: { id: managementReviewDoc.id, ...whileNarrativeEmpty }, - data: { - draftNarrative: { - procedure: defaultProcedureText(organizationName), - }, - }, - }); - } } } @@ -315,7 +337,9 @@ export class IsmsService { }, }, reviews: { - orderBy: { position: 'asc' }, + // Same deterministic ordering as EXPORT_DOCUMENT_INCLUDE: the client + // computes the carried-forward lists from this order. + orderBy: [{ position: 'asc' }, { createdAt: 'asc' }, { id: 'asc' }], include: { inputs: { orderBy: { position: 'asc' } }, actions: { orderBy: { position: 'asc' } }, diff --git a/apps/api/src/isms/utils/export-payload.ts b/apps/api/src/isms/utils/export-payload.ts index 9857ad35ea..f5753b1194 100644 --- a/apps/api/src/isms/utils/export-payload.ts +++ b/apps/api/src/isms/utils/export-payload.ts @@ -81,7 +81,10 @@ export const EXPORT_DOCUMENT_INCLUDE = { }, }, reviews: { - orderBy: { position: 'asc' }, + // Deterministic tie-breakers: review order drives which actions carry + // forward into which review's input (a), so two reviews sharing a + // position must order the same on every read. + orderBy: [{ position: 'asc' }, { createdAt: 'asc' }, { id: 'asc' }], include: { inputs: { orderBy: { position: 'asc' } }, actions: { orderBy: { position: 'asc' } }, diff --git a/apps/api/src/isms/utils/review-participants.ts b/apps/api/src/isms/utils/review-participants.ts index 8506741862..d1bcffbe1a 100644 --- a/apps/api/src/isms/utils/review-participants.ts +++ b/apps/api/src/isms/utils/review-participants.ts @@ -40,7 +40,9 @@ export async function resolveReviewParticipantDefaults({ select: { roleKey: true, assignments: { - orderBy: { position: 'asc' }, + // Deterministic tie-breaker: two assignments sharing a position must + // resolve the same default chair/SPO on every read. + orderBy: [{ position: 'asc' }, { id: 'asc' }], select: { memberId: true }, }, }, diff --git a/apps/app/src/app/(app)/[orgId]/documents/isms/components/CarriedForwardActions.tsx b/apps/app/src/app/(app)/[orgId]/documents/isms/components/CarriedForwardActions.tsx index 0f426913a5..6af4e28457 100644 --- a/apps/app/src/app/(app)/[orgId]/documents/isms/components/CarriedForwardActions.tsx +++ b/apps/app/src/app/(app)/[orgId]/documents/isms/components/CarriedForwardActions.tsx @@ -73,8 +73,12 @@ export function CarriedForwardActions({ {action.description} + {/* An empty roster means the people request was unavailable + to this user, not that the owner left — stay neutral. */} {action.ownerMemberId - ? (memberNameById[action.ownerMemberId] ?? 'Former member') + ? memberOptions.length === 0 + ? 'Unknown member' + : (memberNameById[action.ownerMemberId] ?? 'Former member') : '—'} {action.dueDate?.slice(0, 10) ?? '—'} diff --git a/apps/app/src/app/(app)/[orgId]/documents/isms/components/ReviewActionRow.tsx b/apps/app/src/app/(app)/[orgId]/documents/isms/components/ReviewActionRow.tsx index 8f3acff2f2..0920f507a9 100644 --- a/apps/app/src/app/(app)/[orgId]/documents/isms/components/ReviewActionRow.tsx +++ b/apps/app/src/app/(app)/[orgId]/documents/isms/components/ReviewActionRow.tsx @@ -244,8 +244,12 @@ export function ReviewActionRow({ {action.description} + {/* An empty roster means the people request was unavailable to this + user, not that the owner left — stay neutral. */} {action.ownerMemberId - ? (memberNameById[action.ownerMemberId] ?? 'Former member') + ? memberOptions.length === 0 + ? 'Unknown member' + : (memberNameById[action.ownerMemberId] ?? 'Former member') : '—'} {action.dueDate?.slice(0, 10) ?? '—'} diff --git a/apps/app/src/app/(app)/[orgId]/documents/isms/components/ReviewFields.tsx b/apps/app/src/app/(app)/[orgId]/documents/isms/components/ReviewFields.tsx index 684085b843..9b6002ba62 100644 --- a/apps/app/src/app/(app)/[orgId]/documents/isms/components/ReviewFields.tsx +++ b/apps/app/src/app/(app)/[orgId]/documents/isms/components/ReviewFields.tsx @@ -23,6 +23,7 @@ import type { ReviewDetailsFormValues } from './management-review-schema'; import { IsmsFieldLabel } from './shared'; const NO_VERDICT = 'no-verdict'; +const NO_CHAIR = 'no-chair'; interface ReviewFieldsProps { control: Control; @@ -62,11 +63,18 @@ function ChairSelect({ ); } return ( - + onChange(!next || next === NO_CHAIR ? '' : next) + } + > + {/* Explicit clear: a stale chair must be removable, not only replaceable. */} + No chair yet {options.map((option) => ( {option} diff --git a/apps/app/src/app/(app)/[orgId]/documents/isms/components/ReviewOutputsSection.tsx b/apps/app/src/app/(app)/[orgId]/documents/isms/components/ReviewOutputsSection.tsx index fb69437065..1c5513addf 100644 --- a/apps/app/src/app/(app)/[orgId]/documents/isms/components/ReviewOutputsSection.tsx +++ b/apps/app/src/app/(app)/[orgId]/documents/isms/components/ReviewOutputsSection.tsx @@ -59,6 +59,13 @@ export function ReviewOutputsSection({ if (!isEditing) reset(toFormValues(review)); }, [review, isEditing, reset]); + // If the review becomes signed (or the caller loses edit rights) while the + // outputs editor is open, close it — otherwise the textareas would linger + // with stale local edits and no Save action. + useEffect(() => { + if (!canEdit && isEditing) setIsEditing(false); + }, [canEdit, isEditing]); + const handleSave = handleSubmit(async (values) => { try { await onSave(values); @@ -117,7 +124,7 @@ export function ReviewOutputsSection({ changed. Actions arising are recorded in the table below. - {isEditing ? ( + {isEditing && canEdit ? ( { }); describe('parseAttendees / isReviewSigned / parseProcedure', () => { - it('parses valid attendees and drops malformed entries', () => { + it('is all-or-nothing on malformed entries, mirroring the server parse', () => { + // One malformed entry invalidates the WHOLE list — exactly like the API's + // parseReviewAttendees — so the Submit UI can never look ready while the + // server gate counts zero attendees. expect( parseAttendees([ { memberId: 'm1', name: 'Jane' }, { memberId: 'm2' }, - 'garbage', - { memberId: 'm3', name: ' ' }, - // Empty memberId must not count — mirrors the server schema, so the - // Submit UI can't look ready while the server rejects the list. - { memberId: '', name: 'Ghost' }, ]), - ).toEqual([{ memberId: 'm1', name: 'Jane' }]); + ).toEqual([]); + expect(parseAttendees([{ memberId: '', name: 'Ghost' }])).toEqual([]); + expect(parseAttendees([{ memberId: 'm3', name: ' ' }])).toEqual([]); expect(parseAttendees(null)).toEqual([]); expect(parseAttendees('nope')).toEqual([]); }); + it('parses valid attendees, trims names, and dedupes by member', () => { + expect( + parseAttendees([ + // Trimmed like the server's zod `.trim()` transform, so the UI and + // the generated minutes show the same name. + { memberId: 'm1', name: ' Jane ' }, + { memberId: 'm2', name: 'Ada' }, + { memberId: 'm1', name: 'Jane (dup)' }, + ]), + ).toEqual([ + { memberId: 'm1', name: 'Jane' }, + { memberId: 'm2', name: 'Ada' }, + ]); + }); + it('requires both chair name and date for signed', () => { expect(isReviewSigned(makeReview())).toBe(true); expect(isReviewSigned(makeReview({ signoffChairName: ' ' }))).toBe(false); diff --git a/apps/app/src/app/(app)/[orgId]/documents/isms/components/management-review-constants.ts b/apps/app/src/app/(app)/[orgId]/documents/isms/components/management-review-constants.ts index 81d635de00..528977012f 100644 --- a/apps/app/src/app/(app)/[orgId]/documents/isms/components/management-review-constants.ts +++ b/apps/app/src/app/(app)/[orgId]/documents/isms/components/management-review-constants.ts @@ -75,21 +75,36 @@ export function fullActionReference( return `${reviewReference}-${actionReference}`; } -/** Parse a review's attendees defensively (stale/invalid JSON → empty list). - * Mirrors the server's reviewAttendeeSchema: memberId AND name non-empty. */ -export function parseAttendees(value: unknown): IsmsReviewAttendee[] { - if (!Array.isArray(value)) return []; - return value.filter( - (entry): entry is IsmsReviewAttendee => - !!entry && - typeof entry === 'object' && - typeof (entry as { memberId?: unknown }).memberId === 'string' && - (entry as { memberId: string }).memberId.length > 0 && - typeof (entry as { name?: unknown }).name === 'string' && - (entry as { name: string }).name.trim().length > 0, +function isValidAttendee(entry: unknown): entry is IsmsReviewAttendee { + return ( + !!entry && + typeof entry === 'object' && + typeof (entry as { memberId?: unknown }).memberId === 'string' && + (entry as { memberId: string }).memberId.length > 0 && + typeof (entry as { name?: unknown }).name === 'string' && + (entry as { name: string }).name.trim().length > 0 ); } +/** + * Parse a review's attendees defensively — the exact mirror of the server's + * parseReviewAttendees (documents/management-review.ts): all-or-nothing (one + * malformed entry invalidates the list, like the write schema), names trimmed + * (zod's `.trim()` transform does this server-side), and deduped by member, + * so the UI, the Submit gate, and the generated minutes always agree. + */ +export function parseAttendees(value: unknown): IsmsReviewAttendee[] { + if (!Array.isArray(value) || !value.every(isValidAttendee)) return []; + const seen = new Set(); + return value + .map((attendee) => ({ ...attendee, name: attendee.name.trim() })) + .filter((attendee) => { + if (seen.has(attendee.memberId)) return false; + seen.add(attendee.memberId); + return true; + }); +} + /** A review is signed (and locked) once the chair's name AND date are set. */ export function isReviewSigned( review: Pick,