fix(milestones): reject front/tail key collisions; dedupe on collision; stamp installed/trained dates
Final whole-branch review FIX 1 + FIX 5. - setFrontTemplate / setModuleTail now reject a key that collides across the front/tail halves (checked against the default tail, every configured per-module tail, and the shared front respectively) instead of letting milestoneTemplate silently compose a duplicate-key list. - ensureProjectMilestones adds each inserted key to its own `existing` set inside the loop, so a template that still ends up with a duplicate key (a pre-existing bad setting) is deduped instead of hitting the UNIQUE (client_module_id, key) violation a second time. - GET /client-modules/:id/milestones now wraps ensureProjectMilestones + listProjectMilestones in try/catch -> 400, matching its POST sibling, instead of an unhandled async rejection that hung the request. - advanceStatusForStep now stamps client_module.installed_on / trained_on from the triggering milestone's own done_on the first time status advances into 'installed' / 'trained' (never overwriting an existing date), replacing the UI date-writers the Client Detail redesign removed without a replacement. Reflected in the same audit row as the status change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>main
Thomas Joise
1 day ago
parent
7494f53e2f
commit
e8e2a202c1
@ -0,0 +1,224 @@
|
||||
// apps/hq/test/onboarding-template-routes.test.ts — final whole-branch review FIX 4: the
|
||||
// owner onboarding-template editor surface (normalizeTemplateSteps / setFrontTemplate /
|
||||
// setModuleTail / getFrontTemplate / getModuleTail + the four routes) had no direct test
|
||||
// coverage. Also exercises the cross-half (front <-> tail) key collision rejection added by
|
||||
// FIX 1, at both the repo level and over HTTP.
|
||||
import express from 'express'
|
||||
import { describe, it, expect, afterAll } from 'vitest'
|
||||
import { openDb, type DB } from '../src/db'
|
||||
import { seedIfEmpty } from '../src/seed'
|
||||
import { apiRouter } from '../src/api'
|
||||
import { createStaff } from '../src/auth'
|
||||
import { createModule } from '../src/repos-modules'
|
||||
import {
|
||||
normalizeTemplateSteps, setFrontTemplate, setModuleTail, getFrontTemplate, getModuleTail,
|
||||
FRONT_FALLBACK, TAIL_FALLBACK,
|
||||
} from '../src/repos-milestones'
|
||||
|
||||
const KEY = '33'.repeat(32)
|
||||
|
||||
describe('normalizeTemplateSteps (pure validation)', () => {
|
||||
it('rejects a non-array or empty body', () => {
|
||||
expect(() => normalizeTemplateSteps(undefined)).toThrow(/at least one/i)
|
||||
expect(() => normalizeTemplateSteps('nope')).toThrow(/at least one/i)
|
||||
expect(() => normalizeTemplateSteps([])).toThrow(/at least one/i)
|
||||
})
|
||||
|
||||
it('rejects a blank label', () => {
|
||||
expect(() => normalizeTemplateSteps([{ label: ' ' }])).toThrow(/label/i)
|
||||
expect(() => normalizeTemplateSteps([{ key: 'a', label: 'A' }, { label: '' }])).toThrow(/label/i)
|
||||
})
|
||||
|
||||
it('rejects a duplicate explicit key', () => {
|
||||
expect(() => normalizeTemplateSteps([
|
||||
{ key: 'dup', label: 'First' }, { key: 'dup', label: 'Second' },
|
||||
])).toThrow(/Duplicate step key/)
|
||||
})
|
||||
|
||||
it('derives a key by slugifying the label when none is given, bumping on collision', () => {
|
||||
const out = normalizeTemplateSteps([
|
||||
{ label: 'Site Visit!' }, { label: 'site visit' }, { label: 'Site -- Visit' },
|
||||
])
|
||||
expect(out).toEqual([
|
||||
{ key: 'site_visit', label: 'Site Visit!' },
|
||||
{ key: 'site_visit_2', label: 'site visit' },
|
||||
{ key: 'site_visit_3', label: 'Site -- Visit' },
|
||||
])
|
||||
})
|
||||
|
||||
it('a key derived from a label collides with a later EXPLICIT key of the same value -> rejected', () => {
|
||||
expect(() => normalizeTemplateSteps([{ label: 'Go Live' }, { key: 'go_live', label: 'Go-live (explicit)' }]))
|
||||
.toThrow(/Duplicate step key/)
|
||||
})
|
||||
})
|
||||
|
||||
describe('setFrontTemplate / setModuleTail / getFrontTemplate / getModuleTail (repo level)', () => {
|
||||
it('getFrontTemplate / getModuleTail fall back to the code defaults on an empty DB', async () => {
|
||||
const db = openDb(':memory:')
|
||||
expect(await getFrontTemplate(db)).toEqual(FRONT_FALLBACK)
|
||||
expect(await getModuleTail(db, 'SMS')).toEqual(TAIL_FALLBACK)
|
||||
})
|
||||
|
||||
it('setFrontTemplate rejects a key colliding with the default tail', async () => {
|
||||
const db = openDb(':memory:')
|
||||
await expect(setFrontTemplate(db, 'u1', [{ key: 'installed', label: 'Site installed' }]))
|
||||
.rejects.toThrow(/used by both the front/i)
|
||||
})
|
||||
|
||||
it('setFrontTemplate rejects a key colliding with an already-configured per-module tail', async () => {
|
||||
const db = openDb(':memory:')
|
||||
await setModuleTail(db, 'u1', 'SMS', [{ key: 'kickoff', label: 'Kickoff call' }])
|
||||
await expect(setFrontTemplate(db, 'u1', [{ key: 'kickoff', label: 'Kickoff' }]))
|
||||
.rejects.toThrow(/SMS/)
|
||||
// The front setting was never written — still the fallback.
|
||||
expect(await getFrontTemplate(db)).toEqual(FRONT_FALLBACK)
|
||||
})
|
||||
|
||||
it('setModuleTail rejects a key colliding with the shared front', async () => {
|
||||
const db = openDb(':memory:')
|
||||
await expect(setModuleTail(db, 'u1', 'SMS', [{ key: 'enquiry', label: 'Enquiry (dup)' }]))
|
||||
.rejects.toThrow(/front/i)
|
||||
})
|
||||
|
||||
it('setModuleTail rejects a key colliding with a CUSTOM front (not just the code fallback)', async () => {
|
||||
const db = openDb(':memory:')
|
||||
await setFrontTemplate(db, 'u1', [{ key: 'lead_in', label: 'Lead in' }])
|
||||
await expect(setModuleTail(db, 'u1', 'SMS', [{ key: 'lead_in', label: 'Also lead in' }]))
|
||||
.rejects.toThrow(/front/i)
|
||||
})
|
||||
|
||||
it('a non-colliding edit round-trips through the getters and is audited', async () => {
|
||||
const db = openDb(':memory:')
|
||||
const front = [{ key: 'lead_in', label: 'Lead in' }]
|
||||
const tail = [{ key: 'kickoff', label: 'Kickoff call' }, { key: 'wrapup', label: 'Wrap up' }]
|
||||
await setFrontTemplate(db, 'u1', front)
|
||||
await setModuleTail(db, 'u1', 'SMS', tail)
|
||||
expect(await getFrontTemplate(db)).toEqual(front)
|
||||
expect(await getModuleTail(db, 'SMS')).toEqual(tail)
|
||||
// A different, unconfigured module still gets the code fallback tail.
|
||||
expect(await getModuleTail(db, 'RTGS')).toEqual(TAIL_FALLBACK)
|
||||
const audited = await db.all<{ n: number }>(
|
||||
`SELECT COUNT(*) AS n FROM audit_log WHERE entity='setting' AND entity_id LIKE 'project.milestone_template%'`,
|
||||
)
|
||||
expect((audited[0] as unknown as { n: number }).n).toBe(2)
|
||||
})
|
||||
})
|
||||
|
||||
describe('onboarding-template routes (owner-only editor)', () => {
|
||||
const servers: { close: () => void }[] = []
|
||||
afterAll(() => { for (const s of servers) s.close() })
|
||||
|
||||
async function httpWorld(): Promise<{ db: DB; base: string; ownerH: Record<string, string>; staffH: Record<string, string> }> {
|
||||
const db = openDb(':memory:')
|
||||
await seedIfEmpty(db)
|
||||
await createStaff(db, { email: 'owner2@t.in', displayName: 'Owner2', role: 'owner', password: 'owner-pass-1' })
|
||||
await createStaff(db, { email: 'staff@t.in', displayName: 'Staff', role: 'staff', password: 'staff-pass-1' })
|
||||
await createModule(db, 'u1', { code: 'SMS', name: 'Bulk SMS' })
|
||||
// seedIfEmpty pre-seeds a `project.milestone_template:SMS` setting (the shipped SMS
|
||||
// pipeline), so use an UNconfigured module for tests that assert the code TAIL_FALLBACK.
|
||||
await createModule(db, 'u1', { code: 'WIDGET', name: 'Unconfigured Widget' })
|
||||
const app = express(); app.use(express.json()); app.locals['db'] = db
|
||||
app.use('/api', apiRouter(db, { keyHex: KEY }))
|
||||
const server = app.listen(0); servers.push(server)
|
||||
const base = `http://localhost:${(server.address() as { port: number }).port}/api`
|
||||
const login = async (email: string, password: string): Promise<Record<string, string>> => {
|
||||
const token = ((await (await fetch(`${base}/auth/login`, {
|
||||
method: 'POST', headers: { 'content-type': 'application/json' },
|
||||
body: JSON.stringify({ email, password }),
|
||||
})).json()) as { token: string }).token
|
||||
return { 'content-type': 'application/json', authorization: `Bearer ${token}` }
|
||||
}
|
||||
return { db, base, ownerH: await login('owner2@t.in', 'owner-pass-1'), staffH: await login('staff@t.in', 'staff-pass-1') }
|
||||
}
|
||||
|
||||
it('GET /onboarding-template/front: owner only, serves the fallback by default', async () => {
|
||||
const { base, ownerH, staffH } = await httpWorld()
|
||||
const denied = await fetch(`${base}/onboarding-template/front`, { headers: staffH })
|
||||
expect(denied.status).toBe(403)
|
||||
const out = await (await fetch(`${base}/onboarding-template/front`, { headers: ownerH })).json() as
|
||||
{ ok: boolean; steps: { key: string; label: string }[] }
|
||||
expect(out.ok).toBe(true)
|
||||
expect(out.steps).toEqual(FRONT_FALLBACK)
|
||||
})
|
||||
|
||||
it('PUT /onboarding-template/front: owner only; rejects blank label / duplicate key; round-trips on GET', async () => {
|
||||
const { base, ownerH, staffH } = await httpWorld()
|
||||
const denied = await fetch(`${base}/onboarding-template/front`, {
|
||||
method: 'PUT', headers: staffH, body: JSON.stringify({ steps: [{ key: 'a', label: 'A' }] }),
|
||||
})
|
||||
expect(denied.status).toBe(403)
|
||||
|
||||
const blank = await fetch(`${base}/onboarding-template/front`, {
|
||||
method: 'PUT', headers: ownerH, body: JSON.stringify({ steps: [{ key: 'a', label: ' ' }] }),
|
||||
})
|
||||
expect(blank.status).toBe(400)
|
||||
|
||||
const dup = await fetch(`${base}/onboarding-template/front`, {
|
||||
method: 'PUT', headers: ownerH,
|
||||
body: JSON.stringify({ steps: [{ key: 'a', label: 'A' }, { key: 'a', label: 'A2' }] }),
|
||||
})
|
||||
expect(dup.status).toBe(400)
|
||||
|
||||
const newFront = [{ label: 'Lead In!' }, { key: 'confirmed', label: 'Confirmed' }]
|
||||
const put = await fetch(`${base}/onboarding-template/front`, {
|
||||
method: 'PUT', headers: ownerH, body: JSON.stringify({ steps: newFront }),
|
||||
})
|
||||
expect(put.status).toBe(200)
|
||||
const putBody = await put.json() as { steps: { key: string; label: string }[] }
|
||||
expect(putBody.steps[0]).toEqual({ key: 'lead_in', label: 'Lead In!' })
|
||||
|
||||
const reGet = await (await fetch(`${base}/onboarding-template/front`, { headers: ownerH })).json() as
|
||||
{ steps: { key: string; label: string }[] }
|
||||
expect(reGet.steps).toEqual(putBody.steps)
|
||||
})
|
||||
|
||||
it('GET/PUT /modules/:code/onboarding-template: 404 on an unknown module code', async () => {
|
||||
const { base, ownerH } = await httpWorld()
|
||||
const getMissing = await fetch(`${base}/modules/NOPE/onboarding-template`, { headers: ownerH })
|
||||
expect(getMissing.status).toBe(404)
|
||||
const putMissing = await fetch(`${base}/modules/NOPE/onboarding-template`, {
|
||||
method: 'PUT', headers: ownerH, body: JSON.stringify({ steps: [{ key: 'a', label: 'A' }] }),
|
||||
})
|
||||
expect(putMissing.status).toBe(404)
|
||||
})
|
||||
|
||||
it('GET/PUT /modules/:code/onboarding-template: owner only, round-trips a valid edit', async () => {
|
||||
const { base, ownerH, staffH } = await httpWorld()
|
||||
const denied = await fetch(`${base}/modules/WIDGET/onboarding-template`, { headers: staffH })
|
||||
expect(denied.status).toBe(403)
|
||||
|
||||
const before = await (await fetch(`${base}/modules/WIDGET/onboarding-template`, { headers: ownerH })).json() as
|
||||
{ steps: { key: string; label: string }[] }
|
||||
expect(before.steps).toEqual(TAIL_FALLBACK)
|
||||
|
||||
const newTail = [{ key: 'kickoff', label: 'Kickoff call' }, { key: 'wrapup', label: 'Wrap up' }]
|
||||
const put = await fetch(`${base}/modules/WIDGET/onboarding-template`, {
|
||||
method: 'PUT', headers: ownerH, body: JSON.stringify({ steps: newTail }),
|
||||
})
|
||||
expect(put.status).toBe(200)
|
||||
|
||||
const after = await (await fetch(`${base}/modules/WIDGET/onboarding-template`, { headers: ownerH })).json() as
|
||||
{ steps: { key: string; label: string }[] }
|
||||
expect(after.steps).toEqual(newTail)
|
||||
})
|
||||
|
||||
it('cross-half collision (FIX 1) is rejected with 400 over HTTP, both directions', async () => {
|
||||
const { base, ownerH } = await httpWorld()
|
||||
// Front step whose key collides with the (fallback) SMS tail's 'installed'... use the
|
||||
// module tail directly since SMS starts from TAIL_FALLBACK (has 'installed').
|
||||
const frontCollide = await fetch(`${base}/onboarding-template/front`, {
|
||||
method: 'PUT', headers: ownerH, body: JSON.stringify({ steps: [{ key: 'installed', label: 'Installed' }] }),
|
||||
})
|
||||
expect(frontCollide.status).toBe(400)
|
||||
const frontCollideBody = await frontCollide.json() as { ok: boolean; error: string }
|
||||
expect(frontCollideBody.error).toMatch(/used by both the front/i)
|
||||
|
||||
// Tail step whose key collides with the shared front's 'enquiry'.
|
||||
const tailCollide = await fetch(`${base}/modules/SMS/onboarding-template`, {
|
||||
method: 'PUT', headers: ownerH, body: JSON.stringify({ steps: [{ key: 'enquiry', label: 'Enquiry (dup)' }] }),
|
||||
})
|
||||
expect(tailCollide.status).toBe(400)
|
||||
const tailCollideBody = await tailCollide.json() as { ok: boolean; error: string }
|
||||
expect(tailCollideBody.error).toMatch(/front/i)
|
||||
})
|
||||
})
|
||||
Loading…
Reference in New Issue