From a10ecc309a4e2283c5e9d36555dfe1673fd142f3 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 28 Apr 2026 14:33:50 -0400 Subject: [PATCH 1/2] fix: fix-sync-requirement-map-drift [dev] [Marfuen] mariano/fix-sync-requirement-map-drift --- .../framework-sync-apply.spec.ts | 193 +++++++++++++++++- .../framework-sync-apply.ts | 173 +++++++++++----- 2 files changed, 316 insertions(+), 50 deletions(-) diff --git a/apps/api/src/frameworks/framework-versioning/framework-sync-apply.spec.ts b/apps/api/src/frameworks/framework-versioning/framework-sync-apply.spec.ts index 2e46f09a1..a2889fc5e 100644 --- a/apps/api/src/frameworks/framework-versioning/framework-sync-apply.spec.ts +++ b/apps/api/src/frameworks/framework-versioning/framework-sync-apply.spec.ts @@ -40,11 +40,12 @@ function mockTx() { task: { create: jest.fn().mockImplementation(({ data }) => Promise.resolve({ ...data, id: `tsk_new_${Math.random()}` })), update: jest.fn(), findMany: jest.fn().mockResolvedValue([]) }, policy: { create: jest.fn().mockImplementation(({ data }) => Promise.resolve({ ...data, id: `pol_new_${Math.random()}` })), update: jest.fn(), findMany: jest.fn().mockResolvedValue([]) }, policyVersion: { create: jest.fn().mockResolvedValue({ id: 'pv_new', version: 2 }), findFirst: jest.fn().mockResolvedValue({ version: 1 }) }, - requirementMap: { create: jest.fn().mockImplementation(({ data }) => Promise.resolve({ ...data, id: `rm_new_${Math.random()}` })), updateMany: jest.fn(), findMany: jest.fn().mockResolvedValue([]) }, + requirementMap: { create: jest.fn().mockImplementation(({ data }) => Promise.resolve({ ...data, id: `rm_new_${Math.random()}` })), update: jest.fn(), updateMany: jest.fn(), findMany: jest.fn().mockResolvedValue([]) }, frameworkInstance: { findMany: jest.fn().mockResolvedValue([]), update: jest.fn() }, frameworkSyncOperation: { create: jest.fn().mockResolvedValue({ id: 'fso_new' }) }, controlDocumentType: { findUnique: jest.fn().mockResolvedValue(null), create: jest.fn().mockResolvedValue({ id: 'cdt_new' }), delete: jest.fn() }, $executeRaw: jest.fn().mockResolvedValue(0), + $queryRaw: jest.fn().mockResolvedValue([]), } as any; } @@ -299,6 +300,196 @@ describe('applySync', () => { })); }); + // Drift regression: the diff alone misses edges the (backfilled) v1 + // manifest already claimed but the customer's actual rows never had. + // Sync must reconcile against the to-manifest, not just the diff. + it('creates missing RequirementMap edge when v1 and v2 both claim it but customer has no row (drift)', async () => { + const tx = mockTx(); + tx.control.findMany.mockResolvedValue([ + { id: 'ctl_1', controlTemplateId: 'ct_1', organizationId: 'org_1', name: 'C', description: 'D', archivedAt: null }, + ]); + // Customer has no RequirementMap row for (ctl_1, rq_1) — drift between + // backfilled v1.0.0 manifest and the customer's actual onboarding state. + tx.requirementMap.findMany.mockResolvedValue([]); + + const sameControl = { id: 'ct_1', name: 'C', description: 'D', requirementIds: ['rq_1'], policyIds: [], taskIds: [] }; + await applySync(tx, { + instance: baseInstance as any, + currentVersion: { + id: 'fvr_v1', + frameworkId: 'frk_soc2', + manifest: manifest({ + requirements: [{ id: 'rq_1', identifier: 'CC1', name: 'X', description: null }], + controls: [sameControl], + }), + } as any, + targetVersion: { + id: 'fvr_v2', + frameworkId: 'frk_soc2', + manifest: manifest({ + requirements: [{ id: 'rq_1', identifier: 'CC1', name: 'X', description: null }], + controls: [sameControl], + }), + } as any, + memberId: 'mem_1', + }); + + expect(tx.requirementMap.create).toHaveBeenCalledWith(expect.objectContaining({ + data: expect.objectContaining({ controlId: 'ctl_1', requirementId: 'rq_1', frameworkInstanceId: 'frm_1' }), + })); + }); + + it('unarchives an existing archived RequirementMap row instead of creating a duplicate', async () => { + const tx = mockTx(); + tx.control.findMany.mockResolvedValue([ + { id: 'ctl_1', controlTemplateId: 'ct_1', organizationId: 'org_1', name: 'C', description: 'D', archivedAt: null }, + ]); + const archivedAt = new Date('2026-01-01'); + tx.requirementMap.findMany.mockResolvedValue([ + { id: 'rm_archived', controlId: 'ctl_1', requirementId: 'rq_1', frameworkInstanceId: 'frm_1', archivedAt }, + ]); + + const sameControl = { id: 'ct_1', name: 'C', description: 'D', requirementIds: ['rq_1'], policyIds: [], taskIds: [] }; + await applySync(tx, { + instance: baseInstance as any, + currentVersion: { + id: 'fvr_v1', + frameworkId: 'frk_soc2', + manifest: manifest({ + requirements: [{ id: 'rq_1', identifier: 'CC1', name: 'X', description: null }], + controls: [sameControl], + }), + } as any, + targetVersion: { + id: 'fvr_v2', + frameworkId: 'frk_soc2', + manifest: manifest({ + requirements: [{ id: 'rq_1', identifier: 'CC1', name: 'X', description: null }], + controls: [sameControl], + }), + } as any, + memberId: 'mem_1', + }); + + expect(tx.requirementMap.update).toHaveBeenCalledWith(expect.objectContaining({ + where: { id: 'rm_archived' }, + data: { archivedAt: null }, + })); + expect(tx.requirementMap.create).not.toHaveBeenCalled(); + }); + + it('inserts missing _ControlToPolicy edge when v1 and v2 both claim it but customer has no row (drift)', async () => { + const tx = mockTx(); + tx.control.findMany.mockResolvedValue([ + { id: 'ctl_1', controlTemplateId: 'ct_1', organizationId: 'org_1', name: 'C', description: 'D', archivedAt: null }, + ]); + tx.policy.findMany.mockResolvedValue([ + { id: 'pol_1', policyTemplateId: 'pt_1', organizationId: 'org_1', name: 'P', description: 'd', content: [], frequency: null, department: null, status: 'draft', archivedAt: null }, + ]); + // Customer's _ControlToPolicy table has no edge for (ctl_1, pol_1). + tx.$queryRaw.mockResolvedValue([]); + + const sameControl = { id: 'ct_1', name: 'C', description: 'D', requirementIds: [], policyIds: ['pt_1'], taskIds: [] }; + const samePolicy = { id: 'pt_1', name: 'P', description: 'd', content: [], frequency: null, department: null }; + await applySync(tx, { + instance: baseInstance as any, + currentVersion: { + id: 'fvr_v1', + frameworkId: 'frk_soc2', + manifest: manifest({ controls: [sameControl], policies: [samePolicy] }), + } as any, + targetVersion: { + id: 'fvr_v2', + frameworkId: 'frk_soc2', + manifest: manifest({ controls: [sameControl], policies: [samePolicy] }), + } as any, + memberId: 'mem_1', + }); + + // The reconcile pass should have run an INSERT INTO _ControlToPolicy with + // the missing pair. We can't easily inspect the SQL strings on Prisma's + // tagged template through jest, so just assert $executeRaw was invoked. + const calls = tx.$executeRaw.mock.calls.map((c: unknown[]) => String(c[0]?.[0] ?? '')); + const cpInsertCalled = calls.some((s: string) => s.includes('INSERT INTO "_ControlToPolicy"')); + expect(cpInsertCalled).toBe(true); + }); + + it('creates missing ControlDocumentType row when v1 and v2 both claim it but customer has no row (drift)', async () => { + const tx = mockTx(); + tx.control.findMany.mockResolvedValue([ + { id: 'ctl_1', controlTemplateId: 'ct_1', organizationId: 'org_1', name: 'C', description: 'D', archivedAt: null }, + ]); + tx.controlDocumentType.findUnique.mockResolvedValue(null); + + const sameControl = { + id: 'ct_1', + name: 'C', + description: 'D', + requirementIds: [], + policyIds: [], + taskIds: [], + documentTypes: ['infrastructure_inventory'], + }; + await applySync(tx, { + instance: baseInstance as any, + currentVersion: { + id: 'fvr_v1', + frameworkId: 'frk_soc2', + manifest: manifest({ controls: [sameControl] }), + } as any, + targetVersion: { + id: 'fvr_v2', + frameworkId: 'frk_soc2', + manifest: manifest({ controls: [sameControl] }), + } as any, + memberId: 'mem_1', + }); + + expect(tx.controlDocumentType.create).toHaveBeenCalledWith(expect.objectContaining({ + data: expect.objectContaining({ controlId: 'ctl_1', formType: 'infrastructure_inventory' }), + })); + }); + + it('does NOT re-insert a _ControlToPolicy edge that already exists in the customer DB (no double-undo)', async () => { + const tx = mockTx(); + tx.control.findMany.mockResolvedValue([ + { id: 'ctl_1', controlTemplateId: 'ct_1', organizationId: 'org_1', name: 'C', description: 'D', archivedAt: null }, + ]); + tx.policy.findMany.mockResolvedValue([ + { id: 'pol_1', policyTemplateId: 'pt_1', organizationId: 'org_1', name: 'P', description: 'd', content: [], frequency: null, department: null, status: 'draft', archivedAt: null }, + ]); + // Edge already exists (e.g., another framework's onboarding created it). + tx.$queryRaw.mockResolvedValue([{ A: 'ctl_1', B: 'pol_1' }]); + + const sameControl = { id: 'ct_1', name: 'C', description: 'D', requirementIds: [], policyIds: ['pt_1'], taskIds: [] }; + const samePolicy = { id: 'pt_1', name: 'P', description: 'd', content: [], frequency: null, department: null }; + await applySync(tx, { + instance: baseInstance as any, + currentVersion: { + id: 'fvr_v1', + frameworkId: 'frk_soc2', + manifest: manifest({ controls: [sameControl], policies: [samePolicy] }), + } as any, + targetVersion: { + id: 'fvr_v2', + frameworkId: 'frk_soc2', + manifest: manifest({ controls: [sameControl], policies: [samePolicy] }), + } as any, + memberId: 'mem_1', + }); + + // Sync must not have written to _ControlToPolicy (nothing to add) and the + // sync operation's undo payload must NOT claim it connected this edge — + // otherwise rollback would delete a pre-existing edge another framework + // still wants. + const cpInserts = tx.$executeRaw.mock.calls.map((c: unknown[]) => String(c[0]?.[0] ?? '')) + .filter((s: string) => s.includes('INSERT INTO "_ControlToPolicy"')); + expect(cpInserts).toHaveLength(0); + + const undoPayload = tx.frameworkSyncOperation.create.mock.calls[0][0].data.undoPayload; + expect(undoPayload.controlPolicyLinks.connected).toEqual([]); + }); + it('writes a sync operation row with undoPayload and summary', async () => { const tx = mockTx(); await applySync(tx, { diff --git a/apps/api/src/frameworks/framework-versioning/framework-sync-apply.ts b/apps/api/src/frameworks/framework-versioning/framework-sync-apply.ts index 5effc7a9a..652843a27 100644 --- a/apps/api/src/frameworks/framework-versioning/framework-sync-apply.ts +++ b/apps/api/src/frameworks/framework-versioning/framework-sync-apply.ts @@ -216,34 +216,74 @@ export async function applySync( } // --- RequirementMap edges --- + // Reconcile every edge declared by the to-manifest with the customer's + // RequirementMap rows, instead of only applying the v1→v2 diff. Diff alone + // misses edges the backfilled v1.0.0 manifest claimed exist but the + // customer's actual rows never had — drift introduced when CX added a + // control↔requirement link between the customer's onboarding and the + // backfill. The to-manifest is authoritative for what a v2-pinned customer + // should see, regardless of what v1 said. + // + // Query both active and archived rows because the unique constraint + // (controlId, frameworkInstanceId, requirementId) ignores archivedAt — a + // plain `create` over an archived row would violate it. If a row exists + // archived, unarchive it instead. const existingEdges = await tx.requirementMap.findMany({ - where: { frameworkInstanceId: ctx.instance.id, archivedAt: null }, - select: { id: true, controlId: true, requirementId: true }, + where: { frameworkInstanceId: ctx.instance.id }, + select: { id: true, controlId: true, requirementId: true, archivedAt: true }, }); const keyOf = (controlId: string, requirementId: string) => `${controlId}::${requirementId}`; const existingByKey = new Map( existingEdges.filter((e) => e.requirementId).map((e) => [keyOf(e.controlId, e.requirementId!), e]), ); - for (const edge of diff.requirementMapEdges.added) { - const ctlInst = ctlByTemplate.get(edge.controlTemplateId); + const toReqIds = new Set(to.requirements.map((r) => r.id)); + for (const c of to.controls) { + const ctlInst = ctlByTemplate.get(c.id); if (!ctlInst) continue; - if (existingByKey.has(keyOf(ctlInst.id, edge.requirementTemplateId))) continue; - const created = await tx.requirementMap.create({ - data: { - frameworkInstanceId: ctx.instance.id, + for (const rid of c.requirementIds) { + // Strip cross-framework refs the same way the diff sanitizer does. + if (!toReqIds.has(rid)) continue; + const key = keyOf(ctlInst.id, rid); + const existing = existingByKey.get(key); + if (existing) { + if (existing.archivedAt === null) continue; + // Unarchive — the row exists from a prior sync that archived it, + // and to-manifest wants it back. + const prevArchivedAt = existing.archivedAt; + await tx.requirementMap.update({ + where: { id: existing.id }, + data: { archivedAt: null }, + }); + existing.archivedAt = null; + undo.requirementMaps.archived.push({ id: existing.id, prevArchivedAt }); + summary.requirementMapsAdded += 1; + continue; + } + const created = await tx.requirementMap.create({ + data: { + frameworkInstanceId: ctx.instance.id, + controlId: ctlInst.id, + requirementId: rid, + }, + }); + existingByKey.set(key, { + id: created.id, controlId: ctlInst.id, - requirementId: edge.requirementTemplateId, - }, - }); - undo.requirementMaps.created.push(created.id); - summary.requirementMapsAdded += 1; + requirementId: rid, + archivedAt: null, + }); + undo.requirementMaps.created.push(created.id); + summary.requirementMapsAdded += 1; + } } + + // Archive edges still claimed by v1 but absent from v2. for (const edge of diff.requirementMapEdges.removed) { const ctlInst = ctlByTemplate.get(edge.controlTemplateId); if (!ctlInst) continue; const existing = existingByKey.get(keyOf(ctlInst.id, edge.requirementTemplateId)); - if (!existing) continue; + if (!existing || existing.archivedAt !== null) continue; await tx.requirementMap.updateMany({ where: { id: existing.id, archivedAt: null }, data: { archivedAt: new Date() }, @@ -253,18 +293,35 @@ export async function applySync( } // --- Control<->Policy / Control<->Task relations (Prisma implicit M:N) --- - // Use raw SQL on the junction tables — Prisma 7's implicit-M:N `disconnect` - // is strict and throws P2025 if the edge isn't there, which breaks sync in - // the (rare) case where manifest/instance state disagrees about whether an - // edge exists (e.g., a manual edit or a prior partial sync). Raw INSERT … - // ON CONFLICT / DELETE … WHERE IN is naturally idempotent. + // Same drift story as RequirementMap: reconcile against to-manifest, not + // just diff. These tables are org-scoped (shared across frameworks), so we + // only ADD missing edges — removing extras would clobber edges another + // framework still wants. Pre-check existence so undo records only edges + // this sync actually created (otherwise rollback would delete a pre- + // existing edge that a different framework's onboarding had created). + const ctlInstIds = Array.from(ctlByTemplate.values()).map((c) => c.id); + + const existingCp = + ctlInstIds.length > 0 + ? await tx.$queryRaw>` + SELECT "A", "B" FROM "_ControlToPolicy" + WHERE "A" IN (${Prisma.join(ctlInstIds)}) + ` + : []; + const existingCpKey = new Set(existingCp.map((r) => `${r.A}::${r.B}`)); + const cpAdded: Array<{ controlId: string; policyId: string }> = []; - for (const edge of diff.controlPolicyEdges.added) { - const ctlInst = ctlByTemplate.get(edge.controlTemplateId); - const polInst = polByTemplate.get(edge.policyTemplateId); - if (!ctlInst || !polInst) continue; - cpAdded.push({ controlId: ctlInst.id, policyId: polInst.id }); - undo.controlPolicyLinks.connected.push({ controlId: ctlInst.id, otherId: polInst.id }); + for (const c of to.controls) { + const ctlInst = ctlByTemplate.get(c.id); + if (!ctlInst) continue; + for (const pid of c.policyIds) { + const polInst = polByTemplate.get(pid); + if (!polInst) continue; + if (existingCpKey.has(`${ctlInst.id}::${polInst.id}`)) continue; + cpAdded.push({ controlId: ctlInst.id, policyId: polInst.id }); + undo.controlPolicyLinks.connected.push({ controlId: ctlInst.id, otherId: polInst.id }); + existingCpKey.add(`${ctlInst.id}::${polInst.id}`); + } } if (cpAdded.length > 0) { const rows = Prisma.join( @@ -273,6 +330,7 @@ export async function applySync( await tx.$executeRaw`INSERT INTO "_ControlToPolicy" ("A", "B") VALUES ${rows} ON CONFLICT ("A", "B") DO NOTHING`; } + // Diff-based removal: only edges v1 claimed and v2 dropped. const cpRemoved: Array<{ controlId: string; policyId: string }> = []; for (const edge of diff.controlPolicyEdges.removed) { const ctlInst = ctlByTemplate.get(edge.controlTemplateId); @@ -288,13 +346,27 @@ export async function applySync( await tx.$executeRaw`DELETE FROM "_ControlToPolicy" WHERE ("A", "B") IN (${pairs})`; } + const existingCt = + ctlInstIds.length > 0 + ? await tx.$queryRaw>` + SELECT "A", "B" FROM "_ControlToTask" + WHERE "A" IN (${Prisma.join(ctlInstIds)}) + ` + : []; + const existingCtKey = new Set(existingCt.map((r) => `${r.A}::${r.B}`)); + const ctAdded: Array<{ controlId: string; taskId: string }> = []; - for (const edge of diff.controlTaskEdges.added) { - const ctlInst = ctlByTemplate.get(edge.controlTemplateId); - const tInst = taskByTemplate.get(edge.taskTemplateId); - if (!ctlInst || !tInst) continue; - ctAdded.push({ controlId: ctlInst.id, taskId: tInst.id }); - undo.controlTaskLinks.connected.push({ controlId: ctlInst.id, otherId: tInst.id }); + for (const c of to.controls) { + const ctlInst = ctlByTemplate.get(c.id); + if (!ctlInst) continue; + for (const tid of c.taskIds) { + const tInst = taskByTemplate.get(tid); + if (!tInst) continue; + if (existingCtKey.has(`${ctlInst.id}::${tInst.id}`)) continue; + ctAdded.push({ controlId: ctlInst.id, taskId: tInst.id }); + undo.controlTaskLinks.connected.push({ controlId: ctlInst.id, otherId: tInst.id }); + existingCtKey.add(`${ctlInst.id}::${tInst.id}`); + } } if (ctAdded.length > 0) { const rows = Prisma.join( @@ -319,25 +391,28 @@ export async function applySync( } // --- Control <-> DocumentType (explicit junction table ControlDocumentType) --- - // formType is an enum; uniqueness is on (controlId, formType). We treat adds - // as real row creates and removals as hard-deletes because this table has no - // archivedAt column and there's no need to preserve archived edges (the - // formType is just metadata describing what evidence the control accepts). - for (const edge of diff.controlDocumentTypeEdges.added) { - const ctlInst = ctlByTemplate.get(edge.controlTemplateId); + // formType is an enum; uniqueness is on (controlId, formType). Same drift + // story as the other edge types — reconcile against to-manifest, not just + // diff.added, so customers missing rows the v1 manifest already claimed + // get them on sync. Removals stay diff-based (this table has no archivedAt + // and the row is shared across frameworks, so we should only hard-delete + // when the to-manifest explicitly drops it). + for (const c of to.controls) { + const ctlInst = ctlByTemplate.get(c.id); if (!ctlInst) continue; - const formType = normalizeFormType(edge.formType); - // Idempotent create: skip if already present (shared-entity case). - const existing = await tx.controlDocumentType.findUnique({ - where: { controlId_formType: { controlId: ctlInst.id, formType: formType as never } }, - select: { id: true }, - }); - if (existing) continue; - const created = await tx.controlDocumentType.create({ - data: { controlId: ctlInst.id, formType: formType as never }, - }); - undo.controlDocumentTypes.created.push(created.id); - summary.controlDocumentTypesAdded += 1; + for (const rawFormType of c.documentTypes ?? []) { + const formType = normalizeFormType(rawFormType); + const existing = await tx.controlDocumentType.findUnique({ + where: { controlId_formType: { controlId: ctlInst.id, formType: formType as never } }, + select: { id: true }, + }); + if (existing) continue; + const created = await tx.controlDocumentType.create({ + data: { controlId: ctlInst.id, formType: formType as never }, + }); + undo.controlDocumentTypes.created.push(created.id); + summary.controlDocumentTypesAdded += 1; + } } for (const edge of diff.controlDocumentTypeEdges.removed) { const ctlInst = ctlByTemplate.get(edge.controlTemplateId); From b58c4d3f70ec82e56da178dac4c3562226cfed6a Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 28 Apr 2026 14:36:36 -0400 Subject: [PATCH 2/2] fix(framework-editor): don't auto-link new task/policy templates to all framework controls (#2686) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When CX created a new task or policy template from a framework's editor view, the create endpoint queried every control template attached to that framework and connected the new primitive to all of them. CX rarely wants the new template on every control — it forced manual cleanup after every create. Make all primitives consistent: a fresh create produces an unlinked row regardless of which framework page the user happened to be on. Linking to specific controls happens explicitly via the dedicated link endpoints (POST /:id/control-templates/:ctId), which already existed. - task-template.service.ts: drop frameworkId param + the findMany + connect logic - policy-template.service.ts: same - task-template.controller.ts, policy-template.controller.ts: drop the @Query('frameworkId') from the POST handler - frontend (useTaskChangeTracking, CreatePolicyDialog): stop appending ?frameworkId=... to the POST URL Control template and requirement creates were already correct (no auto-linking) — this brings task and policy templates in line. New regression tests on TaskTemplateService and PolicyTemplateService mirror the existing CS-271 guard on ControlTemplateService: even if a stray frameworkId is passed, the service must not query for or attach controls. Verified red without the fix, green with it. Co-authored-by: Mariano Co-authored-by: Claude Opus 4.7 (1M context) --- .../policy-template.controller.ts | 7 +- .../policy-template.service.spec.ts | 81 ++++++++++++++++++ .../policy-template.service.ts | 18 ++-- .../task-template/task-template.controller.ts | 7 +- .../task-template.service.spec.ts | 84 +++++++++++++++++++ .../task-template/task-template.service.ts | 18 ++-- .../(pages)/policies/PoliciesClientPage.tsx | 1 - .../components/CreatePolicyDialog.tsx | 10 ++- .../tasks/hooks/useTaskChangeTracking.ts | 7 +- 9 files changed, 190 insertions(+), 43 deletions(-) create mode 100644 apps/api/src/framework-editor/policy-template/policy-template.service.spec.ts create mode 100644 apps/api/src/framework-editor/task-template/task-template.service.spec.ts diff --git a/apps/api/src/framework-editor/policy-template/policy-template.controller.ts b/apps/api/src/framework-editor/policy-template/policy-template.controller.ts index 5213ecd5b..7f53df998 100644 --- a/apps/api/src/framework-editor/policy-template/policy-template.controller.ts +++ b/apps/api/src/framework-editor/policy-template/policy-template.controller.ts @@ -45,11 +45,8 @@ export class PolicyTemplateController { @Post() @ApiOperation({ summary: 'Create a policy template' }) @UsePipes(new ValidationPipe({ whitelist: true, transform: true })) - async create( - @Body() dto: CreatePolicyTemplateDto, - @Query('frameworkId') frameworkId?: string, - ) { - return this.service.create(dto, frameworkId); + async create(@Body() dto: CreatePolicyTemplateDto) { + return this.service.create(dto); } @Patch(':id') diff --git a/apps/api/src/framework-editor/policy-template/policy-template.service.spec.ts b/apps/api/src/framework-editor/policy-template/policy-template.service.spec.ts new file mode 100644 index 000000000..54f6948de --- /dev/null +++ b/apps/api/src/framework-editor/policy-template/policy-template.service.spec.ts @@ -0,0 +1,81 @@ +jest.mock('@db', () => ({ + db: { + frameworkEditorPolicyTemplate: { + create: jest.fn(), + findUnique: jest.fn(), + findMany: jest.fn(), + update: jest.fn(), + delete: jest.fn(), + }, + frameworkEditorControlTemplate: { + findMany: jest.fn(), + }, + }, + Prisma: { PrismaClientKnownRequestError: class {} }, +})); + +import { db } from '@db'; +import { PolicyTemplateService } from './policy-template.service'; + +const mockDb = db as jest.Mocked; + +describe('PolicyTemplateService', () => { + let service: PolicyTemplateService; + + beforeEach(() => { + service = new PolicyTemplateService(); + jest.clearAllMocks(); + (mockDb.frameworkEditorPolicyTemplate.create as jest.Mock).mockResolvedValue({ + id: 'frk_pt_new', + name: 'New Policy', + }); + }); + + describe('create', () => { + const baseDto = { + name: 'New Policy', + description: 'desc', + frequency: 'monthly', + department: 'none', + } as never; + + // Regression test: previously, passing `frameworkId` caused the create + // path to query every control template in the framework and auto-connect + // the new policy to all of them. CX rarely wants that — the new policy + // should start unlinked and be attached to specific controls explicitly + // via the dedicated link endpoints. The `frameworkId` parameter is gone, + // but a legacy caller passing one anyway must still produce an unlinked + // row. + it('never queries or auto-links framework controls on create, even when a stray frameworkId is passed', async () => { + (mockDb.frameworkEditorControlTemplate.findMany as jest.Mock).mockResolvedValue([ + { id: 'frk_ct_1' }, + { id: 'frk_ct_2' }, + ]); + + // Bypass TypeScript so we can simulate a stray legacy caller still + // passing frameworkId — the service must ignore it. + await (service.create as (dto: unknown, frameworkId?: string) => Promise)( + baseDto, + 'frk_soc2', + ); + + expect(mockDb.frameworkEditorControlTemplate.findMany).not.toHaveBeenCalled(); + const createArgs = (mockDb.frameworkEditorPolicyTemplate.create as jest.Mock).mock + .calls[0][0]; + expect(createArgs.data).not.toHaveProperty('controlTemplates'); + }); + + it('persists name, description, frequency, department and an empty content blob', async () => { + await service.create(baseDto); + const createArgs = (mockDb.frameworkEditorPolicyTemplate.create as jest.Mock).mock + .calls[0][0]; + expect(createArgs.data).toMatchObject({ + name: 'New Policy', + description: 'desc', + frequency: 'monthly', + department: 'none', + content: {}, + }); + }); + }); +}); diff --git a/apps/api/src/framework-editor/policy-template/policy-template.service.ts b/apps/api/src/framework-editor/policy-template/policy-template.service.ts index 082ea290c..d97b332eb 100644 --- a/apps/api/src/framework-editor/policy-template/policy-template.service.ts +++ b/apps/api/src/framework-editor/policy-template/policy-template.service.ts @@ -48,16 +48,11 @@ export class PolicyTemplateService { return pt; } - async create(dto: CreatePolicyTemplateDto, frameworkId?: string) { - const controlIds = frameworkId - ? await db.frameworkEditorControlTemplate - .findMany({ - where: { requirements: { some: { frameworkId } } }, - select: { id: true }, - }) - .then((cts) => cts.map((ct) => ({ id: ct.id }))) - : []; - + // New primitives are created unlinked. CX explicitly attaches them to + // controls via the dedicated link endpoints — auto-linking to every + // control in a framework was wrong (CX rarely wants the new policy on + // every control) and forced manual cleanup after each create. + async create(dto: CreatePolicyTemplateDto) { const pt = await db.frameworkEditorPolicyTemplate.create({ data: { name: dto.name, @@ -65,9 +60,6 @@ export class PolicyTemplateService { frequency: dto.frequency, department: dto.department, content: {}, - ...(controlIds.length > 0 && { - controlTemplates: { connect: controlIds }, - }), }, }); this.logger.log(`Created policy template: ${pt.name} (${pt.id})`); diff --git a/apps/api/src/framework-editor/task-template/task-template.controller.ts b/apps/api/src/framework-editor/task-template/task-template.controller.ts index b261546df..f0cd7a717 100644 --- a/apps/api/src/framework-editor/task-template/task-template.controller.ts +++ b/apps/api/src/framework-editor/task-template/task-template.controller.ts @@ -47,11 +47,8 @@ export class TaskTemplateController { transform: true, }), ) - async createTaskTemplate( - @Body() dto: CreateTaskTemplateDto, - @Query('frameworkId') frameworkId?: string, - ) { - return this.taskTemplateService.create(dto, frameworkId); + async createTaskTemplate(@Body() dto: CreateTaskTemplateDto) { + return this.taskTemplateService.create(dto); } @Get() diff --git a/apps/api/src/framework-editor/task-template/task-template.service.spec.ts b/apps/api/src/framework-editor/task-template/task-template.service.spec.ts new file mode 100644 index 000000000..1c895e1a7 --- /dev/null +++ b/apps/api/src/framework-editor/task-template/task-template.service.spec.ts @@ -0,0 +1,84 @@ +jest.mock('@db', () => ({ + db: { + frameworkEditorTaskTemplate: { + create: jest.fn(), + findUnique: jest.fn(), + findMany: jest.fn(), + update: jest.fn(), + delete: jest.fn(), + }, + frameworkEditorControlTemplate: { + findMany: jest.fn(), + }, + }, + Frequency: { monthly: 'monthly', yearly: 'yearly', daily: 'daily', weekly: 'weekly' }, + Departments: { none: 'none', admin: 'admin', it: 'it' }, +})); + +import { db } from '@db'; +import { TaskTemplateService } from './task-template.service'; + +const mockDb = db as jest.Mocked; + +describe('TaskTemplateService', () => { + let service: TaskTemplateService; + + beforeEach(() => { + service = new TaskTemplateService(); + jest.clearAllMocks(); + (mockDb.frameworkEditorTaskTemplate.create as jest.Mock).mockResolvedValue({ + id: 'frk_tt_new', + name: 'New Task', + }); + }); + + describe('create', () => { + const baseDto = { + name: 'New Task', + description: 'desc', + }; + + // Regression test: previously, passing `frameworkId` caused the create + // path to query every control template in the framework and auto-connect + // the new task to all of them. CX rarely wants that — the new task should + // start unlinked and be attached to specific controls explicitly via the + // dedicated link endpoints. The `frameworkId` parameter is gone, but a + // legacy caller passing one anyway must still produce an unlinked row. + it('never queries or auto-links framework controls on create, even when a stray frameworkId is passed', async () => { + (mockDb.frameworkEditorControlTemplate.findMany as jest.Mock).mockResolvedValue([ + { id: 'frk_ct_1' }, + { id: 'frk_ct_2' }, + ]); + + // Bypass TypeScript so we can simulate a stray legacy caller still + // passing frameworkId — the service must ignore it. + await (service.create as (dto: unknown, frameworkId?: string) => Promise)( + baseDto, + 'frk_soc2', + ); + + expect(mockDb.frameworkEditorControlTemplate.findMany).not.toHaveBeenCalled(); + const createArgs = (mockDb.frameworkEditorTaskTemplate.create as jest.Mock).mock + .calls[0][0]; + expect(createArgs.data).not.toHaveProperty('controlTemplates'); + }); + + it('persists name and description', async () => { + await service.create(baseDto); + const createArgs = (mockDb.frameworkEditorTaskTemplate.create as jest.Mock).mock + .calls[0][0]; + expect(createArgs.data).toMatchObject({ + name: 'New Task', + description: 'desc', + }); + }); + + it('applies default frequency and department when not supplied', async () => { + await service.create({ name: 'X' } as never); + const createArgs = (mockDb.frameworkEditorTaskTemplate.create as jest.Mock).mock + .calls[0][0]; + expect(createArgs.data.frequency).toBe('monthly'); + expect(createArgs.data.department).toBe('none'); + }); + }); +}); diff --git a/apps/api/src/framework-editor/task-template/task-template.service.ts b/apps/api/src/framework-editor/task-template/task-template.service.ts index bf18256e8..217b86580 100644 --- a/apps/api/src/framework-editor/task-template/task-template.service.ts +++ b/apps/api/src/framework-editor/task-template/task-template.service.ts @@ -7,25 +7,17 @@ import { UpdateTaskTemplateDto } from './dto/update-task-template.dto'; export class TaskTemplateService { private readonly logger = new Logger(TaskTemplateService.name); - async create(dto: CreateTaskTemplateDto, frameworkId?: string) { - const controlIds = frameworkId - ? await db.frameworkEditorControlTemplate - .findMany({ - where: { requirements: { some: { frameworkId } } }, - select: { id: true }, - }) - .then((cts) => cts.map((ct) => ({ id: ct.id }))) - : []; - + // New primitives are created unlinked. CX explicitly attaches them to + // controls via the dedicated link endpoints — auto-linking to every + // control in a framework was wrong (CX rarely wants the new task on + // every control) and forced manual cleanup after each create. + async create(dto: CreateTaskTemplateDto) { const taskTemplate = await db.frameworkEditorTaskTemplate.create({ data: { name: dto.name, description: dto.description ?? '', frequency: dto.frequency ?? Frequency.monthly, department: dto.department ?? Departments.none, - ...(controlIds.length > 0 && { - controlTemplates: { connect: controlIds }, - }), }, }); diff --git a/apps/framework-editor/app/(pages)/policies/PoliciesClientPage.tsx b/apps/framework-editor/app/(pages)/policies/PoliciesClientPage.tsx index 9ef1196bb..b037a2325 100644 --- a/apps/framework-editor/app/(pages)/policies/PoliciesClientPage.tsx +++ b/apps/framework-editor/app/(pages)/policies/PoliciesClientPage.tsx @@ -64,7 +64,6 @@ export function PoliciesClientPage({ initialPolicies, emptyMessage, frameworkId {frameworkId && ( void; - frameworkId?: string; } -export function CreatePolicyDialog({ isOpen, onOpenChange, frameworkId }: CreatePolicyDialogProps) { +export function CreatePolicyDialog({ isOpen, onOpenChange }: CreatePolicyDialogProps) { const [isPending, startTransition] = useTransition(); const router = useRouter(); @@ -56,8 +55,11 @@ export function CreatePolicyDialog({ isOpen, onOpenChange, frameworkId }: Create const onSubmit = (values: CreatePolicySchemaType) => { startTransition(async () => { try { - const queryParam = frameworkId ? `?frameworkId=${frameworkId}` : ''; - const result = await apiClient<{ id: string }>(`/policy-template${queryParam}`, { + // Don't pass frameworkId — new policy templates are created unlinked. + // CX explicitly attaches them to specific controls afterward via the + // dedicated link endpoints. Auto-linking to every control in the + // framework forced manual cleanup after every create. + const result = await apiClient<{ id: string }>(`/policy-template`, { method: 'POST', body: JSON.stringify({ name: values.name, diff --git a/apps/framework-editor/app/(pages)/tasks/hooks/useTaskChangeTracking.ts b/apps/framework-editor/app/(pages)/tasks/hooks/useTaskChangeTracking.ts index c480f2504..a262516c1 100644 --- a/apps/framework-editor/app/(pages)/tasks/hooks/useTaskChangeTracking.ts +++ b/apps/framework-editor/app/(pages)/tasks/hooks/useTaskChangeTracking.ts @@ -155,8 +155,11 @@ export const useTaskChangeTracking = (initialData: TasksPageGridData[], framewor } try { - const queryParam = frameworkId ? `?frameworkId=${frameworkId}` : ''; - const newTask = await apiClient<{ id: string }>(`/task-template${queryParam}`, { + // Don't pass frameworkId — new task templates are created unlinked. + // CX explicitly attaches them to specific controls afterward via the + // dedicated link endpoints. Auto-linking to every control in the + // framework forced manual cleanup after every create. + const newTask = await apiClient<{ id: string }>(`/task-template`, { method: 'POST', body: JSON.stringify({ name: row.name,