fix(milestones): stamp installed_on/trained_on on every tick, keep links on a re-tick

Two bugs in the same tick path.

1. installed_on / trained_on were written only as a side effect of a status
   UPGRADE. The ~300 APEX-imported projects are already at 'live' -- the furthest
   status -- so ticking Installation or Training upgraded nothing and therefore
   recorded nothing, and the UI's own date writers were removed in the Client
   Detail redesign, leaving those columns unfillable and the Module Directory's
   "Installed" column blank forever. The date is now stamped whenever the step is
   ticked, independent of any status move: same transaction, same audit payload,
   still write-once (an existing date is never overwritten) and still no downgrade.
   A tick that changes neither status nor a date now writes nothing at all.

2. setMilestone wrote payment_id/document_id as `done ? linked : null`, so any tick
   that did not resend the ids cleared them. bulkSetMilestone routes through it and
   never passes ids (one key, many projects), so a single bulk re-tick silently cut
   every project's step loose from the payment or quotation it had been linked to.
   An already-done step now keeps the link it carries when nothing is supplied.
   An explicit empty id is still an unlink, and unticking still clears both.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

apps/hq/src/repos-milestones.ts

@@ -45,13 +45,25 @@ const STEP_STATUS: Record<string, string> = {
 }
 
 /**
- * Also replaces the removed UI date-writers (Client Detail redesign dropped `RowDate` /
+ * The two things ticking a milestone does to its parent `client_module`, applied in the
- * the summary-table date columns without a replacement): the first time status crosses
+ * caller's transaction and audited together:
- * into 'installed' / 'trained', stamp client_module.installed_on / trained_on from the
+ *
- * triggering milestone's own done_on — never overwriting a date already on file (e.g. one
+ *  1. **Status**, advanced to the step's target — never downgraded.
- * set by the APEX importer or a direct edit).
+ *  2. **installed_on / trained_on**, stamped from the triggering milestone's own done_on.
+ *     This also replaces the removed UI date-writers (the Client Detail redesign dropped
+ *     `RowDate` / the summary-table date columns without a replacement), so a tick is now the
+ *     ONLY way those columns get filled.
+ *
+ * The two are INDEPENDENT (bug fix): the date used to be stamped only as a side effect of a
+ * status upgrade, so on the ~300 APEX-imported projects — already at `live`, the furthest
+ * status — ticking Installation or Training upgraded nothing and therefore recorded nothing,
+ * leaving the Module Directory's "Installed" column blank forever with no way to fill it.
+ * Ticking the step now stamps the date whether or not the coarse status moves. Still
+ * write-once: a date already on file (from the APEX importer or an earlier tick) is never
+ * overwritten. If neither the status nor a date would change, nothing is written and no audit
+ * row is added — a re-tick stays a no-op.
  */
-async function advanceStatusForStep(
+async function applyStepEffects(
   db: DB, userId: string, clientModuleId: string, key: string, doneOn: string | null,
 ): Promise<void> {
   const target = STEP_STATUS[key]
@@ -59,11 +71,14 @@ async function advanceStatusForStep(
   const cm = await db.get<{ status: string; installed_on: string | null; trained_on: string | null }>(
     `SELECT status, installed_on, trained_on FROM client_module WHERE id=?`, clientModuleId)
   if (cm === undefined) return
-  if ((STATUS_RANK[cm.status] ?? 0) >= (STATUS_RANK[target] ?? 0)) return // never downgrade
+  const sets: string[] = []
-  const sets = ['status=?']
+  const args: unknown[] = []
-  const args: unknown[] = [target]
+  const before: Record<string, unknown> = {}
-  const before: Record<string, unknown> = { status: cm.status }
+  const after: Record<string, unknown> = {}
-  const after: Record<string, unknown> = { status: target }
+  if ((STATUS_RANK[cm.status] ?? 0) < (STATUS_RANK[target] ?? 0)) { // never downgrade
+    sets.push('status=?'); args.push(target)
+    before['status'] = cm.status; after['status'] = target
+  }
   if (target === 'installed' && cm.installed_on === null && doneOn !== null) {
     sets.push('installed_on=?'); args.push(doneOn)
     before['installedOn'] = null; after['installedOn'] = doneOn
@@ -72,6 +87,7 @@ async function advanceStatusForStep(
     sets.push('trained_on=?'); args.push(doneOn)
     before['trainedOn'] = null; after['trainedOn'] = doneOn
   }
+  if (sets.length === 0) return
   args.push(clientModuleId)
   await db.run(`UPDATE client_module SET ${sets.join(', ')} WHERE id=?`, ...args)
   await writeAudit(db, userId, 'update', 'client_module', clientModuleId, before, after)
@@ -266,6 +282,13 @@ export async function listProjectMilestones(db: DB, clientModuleId: string): Pro
  * and its `received_on` mirrors into `done_on` unless an explicit `doneOn` is also given.
  * A quotation-step tick (`quotation_sent`) can likewise pass `documentId` to link the
  * quotation / proforma that was sent — it must belong to the same client too. Audited.
+ *
+ * Link lifecycle (bug fix): re-ticking an ALREADY-DONE step without resending its ids keeps
+ * the links already on file. They used to be written as `done ? linked : null`, so any tick
+ * that didn't carry the ids wiped them — most visibly `bulkSetMilestone`, which never passes
+ * them, so one bulk re-tick silently cut every project's step loose from its payment /
+ * document. Passing an explicit empty string still clears a link (that is the "unlink" the
+ * picker sends), and unticking still clears both, as before.
  */
 export async function setMilestone(
   db: DB, userId: string, clientModuleId: string, key: string, done: boolean,
@@ -282,7 +305,9 @@ export async function setMilestone(
       throw new Error('doneOn must be YYYY-MM-DD')
     }
     let stamp = done ? (doneOn ?? new Date().toISOString().slice(0, 10)) : null
-    let linkedPayment: string | null = null
+    // Nothing supplied for an already-done step means "leave the link alone", not "clear it".
+    const wasDone = before.done === 1
+    let linkedPayment: string | null = paymentId === undefined && wasDone ? before.payment_id : null
     if (done && paymentId !== undefined && paymentId !== '') {
       const pay = await db.get<{ received_on: string }>(
         `SELECT received_on FROM payment WHERE id=? AND client_id=(SELECT client_id FROM client_module WHERE id=?)`,
@@ -292,7 +317,7 @@ export async function setMilestone(
       linkedPayment = paymentId
       if (doneOn === undefined) stamp = pay.received_on // link date wins unless caller forced one
     }
-    let linkedDocument: string | null = null
+    let linkedDocument: string | null = documentId === undefined && wasDone ? before.document_id : null
     if (done && documentId !== undefined && documentId !== '') {
       const doc = await db.get<{ id: string }>(
         `SELECT id FROM document WHERE id=? AND client_id=(SELECT client_id FROM client_module WHERE id=?)`,
@@ -301,14 +326,17 @@ export async function setMilestone(
       if (doc === undefined) throw new Error('Document not found for this client')
       linkedDocument = documentId
     }
+    // Unticking always clears both links, whatever was resolved above.
+    const finalPayment = done ? linkedPayment : null
+    const finalDocument = done ? linkedDocument : null
     await db.run(
       `UPDATE project_milestone SET done=?, done_on=?, payment_id=?, document_id=?
         WHERE client_module_id=? AND key=?`,
-      done ? 1 : 0, stamp, done ? linkedPayment : null, done ? linkedDocument : null, clientModuleId, key,
+      done ? 1 : 0, stamp, finalPayment, finalDocument, clientModuleId, key,
     )
     await writeAudit(db, userId, 'set_milestone', 'client_module', clientModuleId, undefined,
-      { key, done, doneOn: stamp, paymentId: linkedPayment, documentId: linkedDocument })
+      { key, done, doneOn: stamp, paymentId: finalPayment, documentId: finalDocument })
-    if (done) await advanceStatusForStep(db, userId, clientModuleId, key, stamp)
+    if (done) await applyStepEffects(db, userId, clientModuleId, key, stamp)
     return listProjectMilestones(db, clientModuleId)
   })
 }
@@ -339,9 +367,13 @@ export async function addProjectMilestone(
  * single summary audit row. Only touches projects that actually have the milestone.
  *
  * FIX F: routes each project through `setMilestone` — the SAME status-advance and
- * payment/document link-clearing logic a single tick uses — instead of a bare UPDATE.
+ * payment/document link logic a single tick uses — instead of a bare UPDATE.
  * Before this, a bulk `go_live` left `client_module.status` at 'quoted' (never advanced)
  * and a bulk untick left a stale `payment_id`/`document_id` link on the row.
+ *
+ * A bulk call never carries per-project ids, so it relies on `setMilestone` PRESERVING the
+ * links already on an already-done step (see there) — otherwise re-ticking a step in bulk
+ * would wipe every payment/document link that had been set one at a time.
  */
 export async function bulkSetMilestone(
   db: DB, userId: string, clientModuleIds: string[], key: string, done: boolean, doneOn?: string,

apps/hq/test/milestone-link-preservation.test.ts

@@ -0,0 +1,113 @@
+// apps/hq/test/milestone-link-preservation.test.ts — re-ticking an already-done step must not
+// wipe the payment / document link it already carries.
+//
+// The bug: setMilestone wrote `payment_id`/`document_id` as `done ? linked : null`, so any tick
+// that didn't resend the ids cleared them. bulkSetMilestone routes through setMilestone and
+// never passes ids at all (there is one key for many projects), so a single bulk re-tick — the
+// onboarding-cleanup tool — silently cut every project's step loose from the payment or
+// quotation it had been linked to one at a time. Unticking must still clear both.
+import { describe, it, expect } from 'vitest'
+import { openDb } from '../src/db'
+import { seedIfEmpty } from '../src/seed'
+import { createClient } from '../src/repos-clients'
+import { createModule, setPrice, assignModule } from '../src/repos-modules'
+import { createDraft } from '../src/repos-documents'
+import { recordPayment } from '../src/repos-payments'
+import { bulkSetMilestone, listProjectMilestones, setMilestone } from '../src/repos-milestones'
+
+async function world() {
+  const db = openDb(':memory:')
+  await seedIfEmpty(db)
+  const c = await createClient(db, 'u1', { name: 'Kothavara SCB', stateCode: '32' })
+  const m = await createModule(db, 'u1', { code: 'SMS', name: 'Bulk SMS' })
+  await setPrice(db, 'u1', { moduleId: m.id, kind: 'yearly', pricePaise: 10_000_00, effectiveFrom: '2020-01-01' })
+  const cm = await assignModule(db, 'u1', { clientId: c.id, moduleId: m.id, kind: 'yearly' })
+  const step = async (key: string) => (await listProjectMilestones(db, cm.id)).find((s) => s.key === key)!
+  return { db, clientId: c.id, moduleId: m.id, cmId: cm.id, step }
+}
+
+describe('bulk re-tick preserves the payment link (5a)', () => {
+  it('tick with a payment link → bulk re-tick keeps it → bulk untick clears it', async () => {
+    const { db, clientId, cmId, step } = await world()
+    const pay = await recordPayment(db, 'u1', {
+      clientId, receivedOn: '2026-05-01', mode: 'bank', amountPaise: 1000_00,
+    })
+    await setMilestone(db, 'u1', cmId, 'payment_received', true, undefined, pay.payment.id)
+    expect((await step('payment_received')).paymentId).toBe(pay.payment.id)
+
+    // The bulk tool carries no ids — the stored link must survive.
+    const res = await bulkSetMilestone(db, 'u1', [cmId], 'payment_received', true, '2026-06-01')
+    expect(res.updated).toBe(1)
+    const after = await step('payment_received')
+    expect(after.done).toBe(true)
+    expect(after.paymentId).toBe(pay.payment.id)
+
+    // Unticking still cuts the link loose.
+    await bulkSetMilestone(db, 'u1', [cmId], 'payment_received', false)
+    const cleared = await step('payment_received')
+    expect(cleared.done).toBe(false)
+    expect(cleared.doneOn).toBeNull()
+    expect(cleared.paymentId).toBeNull()
+  })
+
+  it('a single re-tick that sends no ids keeps the link too', async () => {
+    const { db, clientId, cmId, step } = await world()
+    const pay = await recordPayment(db, 'u1', {
+      clientId, receivedOn: '2026-05-01', mode: 'bank', amountPaise: 1000_00,
+    })
+    await setMilestone(db, 'u1', cmId, 'payment_received', true, undefined, pay.payment.id)
+    await setMilestone(db, 'u1', cmId, 'payment_received', true, '2026-06-01')
+    const s = await step('payment_received')
+    expect(s.doneOn).toBe('2026-06-01') // the date the caller did send still moves
+    expect(s.paymentId).toBe(pay.payment.id)
+  })
+
+  it('an explicit empty id is still an unlink, not a preserve', async () => {
+    const { db, clientId, cmId, step } = await world()
+    const pay = await recordPayment(db, 'u1', {
+      clientId, receivedOn: '2026-05-01', mode: 'bank', amountPaise: 1000_00,
+    })
+    await setMilestone(db, 'u1', cmId, 'payment_received', true, undefined, pay.payment.id)
+    await setMilestone(db, 'u1', cmId, 'payment_received', true, undefined, '')
+    expect((await step('payment_received')).paymentId).toBeNull()
+  })
+
+  it('the preserved link is what the audit row reports', async () => {
+    const { db, clientId, cmId } = await world()
+    const pay = await recordPayment(db, 'u1', {
+      clientId, receivedOn: '2026-05-01', mode: 'bank', amountPaise: 1000_00,
+    })
+    await setMilestone(db, 'u1', cmId, 'payment_received', true, undefined, pay.payment.id)
+    await bulkSetMilestone(db, 'u1', [cmId], 'payment_received', true, '2026-06-01')
+    const audits = await db.all<{ after_json: string | null }>(
+      `SELECT after_json FROM audit_log WHERE action='set_milestone' ORDER BY id`,
+    )
+    expect(JSON.parse(audits.at(-1)!.after_json!)).toMatchObject({ paymentId: pay.payment.id })
+  })
+})
+
+describe('bulk re-tick preserves the document link (5a)', () => {
+  it('tick with a quotation link → bulk re-tick keeps it → untick clears it', async () => {
+    const { db, clientId, moduleId, cmId, step } = await world()
+    const doc = await createDraft(db, 'u1', {
+      docType: 'QUOTATION', clientId, lines: [{ moduleId, qty: 1, kind: 'yearly' }],
+    })
+    await setMilestone(db, 'u1', cmId, 'quotation_sent', true, '2026-05-01', undefined, doc.id)
+    expect((await step('quotation_sent')).documentId).toBe(doc.id)
+
+    await bulkSetMilestone(db, 'u1', [cmId], 'quotation_sent', true, '2026-06-01')
+    expect((await step('quotation_sent')).documentId).toBe(doc.id)
+
+    await bulkSetMilestone(db, 'u1', [cmId], 'quotation_sent', false)
+    expect((await step('quotation_sent')).documentId).toBeNull()
+  })
+
+  it('a step that was never done still starts with no link (nothing to preserve)', async () => {
+    const { db, cmId, step } = await world()
+    await bulkSetMilestone(db, 'u1', [cmId], 'quotation_sent', true, '2026-06-01')
+    const s = await step('quotation_sent')
+    expect(s.done).toBe(true)
+    expect(s.documentId).toBeNull()
+    expect(s.paymentId).toBeNull()
+  })
+})

apps/hq/test/milestone-status.test.ts

@@ -110,6 +110,63 @@ describe('milestone-driven installed_on / trained_on stamping (FIX 5)', () => {
   })
 })
 
+// The date-stamp used to ride on the status UPGRADE, so a project already at the furthest
+// status could never record one. That is the state of all ~300 APEX-imported projects: they
+// come in at 'live', ticking Installation/Training upgrades nothing, and — with the UI's own
+// date writers removed — installed_on/trained_on became permanently unfillable, leaving the
+// Module Directory's "Installed" column blank forever. Ticking now stamps regardless.
+describe('date stamping is independent of the status upgrade (APEX-imported projects at live)', () => {
+  it("ticking 'installation' on a project already at live records installed_on and leaves the status alone", async () => {
+    const { db, cmId } = await world()
+    await updateClientModule(db, 'u1', cmId, { status: 'live' })
+    await setMilestone(db, 'u1', cmId, 'installation', true, '2026-03-25')
+    const cm = await getClientModule(db, cmId)
+    expect(cm!.installedOn).toBe('2026-03-25')
+    expect(cm!.status).toBe('live') // unchanged — no downgrade, no spurious upgrade
+  })
+
+  it("ticking 'training' on a project already at live records trained_on and leaves the status alone", async () => {
+    const { db, cmId } = await world()
+    await updateClientModule(db, 'u1', cmId, { status: 'live' })
+    await setMilestone(db, 'u1', cmId, 'training', true, '2026-05-02')
+    const cm = await getClientModule(db, cmId)
+    expect(cm!.trainedOn).toBe('2026-05-02')
+    expect(cm!.status).toBe('live')
+  })
+
+  it('still never overwrites a date already on file, even with no status move', async () => {
+    const { db, cmId } = await world()
+    await updateClientModule(db, 'u1', cmId, { status: 'live', installedOn: '2025-01-01' })
+    await setMilestone(db, 'u1', cmId, 'installation', true, '2026-03-25')
+    expect((await getClientModule(db, cmId))!.installedOn).toBe('2025-01-01')
+  })
+
+  it('the audit payload records the date-only change (no status key, since status did not move)', async () => {
+    const { db, cmId } = await world()
+    await updateClientModule(db, 'u1', cmId, { status: 'live' })
+    await setMilestone(db, 'u1', cmId, 'installation', true, '2026-03-25')
+    const audits = await db.all<{ after_json: string | null }>(
+      `SELECT after_json FROM audit_log WHERE entity='client_module' AND entity_id=? AND action='update' ORDER BY id`,
+      cmId,
+    )
+    // The last 'update' row is the tick's own (the earlier one is the status seed above).
+    const after = JSON.parse(audits.at(-1)!.after_json!) as Record<string, unknown>
+    expect(after['installedOn']).toBe('2026-03-25')
+    expect(after['status']).toBeUndefined()
+  })
+
+  it('a re-tick that changes nothing writes no client_module audit row at all', async () => {
+    const { db, cmId } = await world()
+    await updateClientModule(db, 'u1', cmId, { status: 'live', installedOn: '2025-01-01' })
+    const countBefore = (await db.all(
+      `SELECT id FROM audit_log WHERE entity='client_module' AND entity_id=? AND action='update'`, cmId)).length
+    await setMilestone(db, 'u1', cmId, 'installation', true, '2026-03-25')
+    const countAfter = (await db.all(
+      `SELECT id FROM audit_log WHERE entity='client_module' AND entity_id=? AND action='update'`, cmId)).length
+    expect(countAfter).toBe(countBefore)
+  })
+})
+
 // Pre-ship audit FIX E: STEP_STATUS only had keys from the NEW template vocabulary
 // (installation/training/go_live/quotation_approved), but TAIL_FALLBACK — used by any module
 // with no seeded/owner-configured tail (e.g. WHATSAPP, ATM, AMC) — still uses the legacy