From b10a3ef22e4521aa207c45ad1573015f3eb8a741 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 11:57:11 +0200 Subject: [PATCH] fix(onboarding): leave a trail for the three settings columns the flow writes The setup flow writes a record's module map, its glucose unit and its unit preference. All three have a dedicated settings route that records an audit row, and the flow recorded none, so "why did my modules change" had no answer in the activity panel. Restarting the flow did write a row, which made the gap look like an oversight rather than a decision, because it was one. Same event names as the dedicated routes, since it is the same column and the panel should group them. The module row names the keys that actually moved and the domains kept for their content. --- docs/api/openapi.yaml | 6 +++-- src/app/api/onboarding/answers/route.ts | 28 +++++++++++++++++++ src/app/api/onboarding/complete/route.ts | 31 ++++++++++++++++++++-- src/lib/openapi/routes/onboarding.ts | 2 +- tests/integration/onboarding-needs.test.ts | 20 ++++++++++++++ 5 files changed, 82 insertions(+), 5 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 21e04da25..c0fdbaf7c 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -16389,8 +16389,10 @@ paths: The older sibling of `POST /api/onboarding/step`. It is unconditional in a way the step route is not: it stamps the completion whatever the stored step is, it re-stamps on every call rather than refusing a second one, and it - writes no audit row and enforces no rate limit of its own. It also does NOT seed the dashboard from the goal - selection — that only happens on the step route's completing call. + enforces no rate limit of its own. Its legacy half writes no audit row; the needs half writes one + (`user.modules.update`) when it derives, because that is the same column the dedicated modules route audits. It + also does NOT seed the dashboard from the goal selection — that only happens on the step route's completing + call. Calling this mid-wizard makes every remaining `POST /api/onboarding/step` answer 409 diff --git a/src/app/api/onboarding/answers/route.ts b/src/app/api/onboarding/answers/route.ts index fd101d76e..ac502bea3 100644 --- a/src/app/api/onboarding/answers/route.ts +++ b/src/app/api/onboarding/answers/route.ts @@ -27,9 +27,11 @@ import { apiHandler, requireAuth } from "@/lib/api-handler"; import { apiError, apiSuccess, + getClientIp, returnAllZodIssues, safeJson, } from "@/lib/api-response"; +import { auditLog } from "@/lib/auth/audit"; import { prisma, toJson } from "@/lib/db"; import { annotate } from "@/lib/logging/context"; import { applyOnboardingAnswer } from "@/lib/onboarding/needs-apply"; @@ -146,6 +148,32 @@ export const PATCH = apiHandler(async (request: NextRequest) => { unitPreference: unitData.unitPreference ?? user.unitPreference, }); + // Both columns have a dedicated settings route that writes an audit row, and + // the trail should not depend on which surface the person changed them from. + // Same event names, so the activity panel groups them. + if (unitData.glucoseUnit !== undefined) { + await auditLog("user.glucose-unit.update", { + userId: user.id, + ipAddress: getClientIp(request), + details: { + previous: user.glucoseUnit, + next: unitData.glucoseUnit, + source: "onboarding", + }, + }); + } + if (unitData.unitPreference !== undefined) { + await auditLog("user.unit-preference.update", { + userId: user.id, + ipAddress: getClientIp(request), + details: { + previous: user.unitPreference, + next: unitData.unitPreference, + source: "onboarding", + }, + }); + } + annotate({ action: { name: "onboarding.answer.save" }, meta: { diff --git a/src/app/api/onboarding/complete/route.ts b/src/app/api/onboarding/complete/route.ts index 50a4563c4..d4d378072 100644 --- a/src/app/api/onboarding/complete/route.ts +++ b/src/app/api/onboarding/complete/route.ts @@ -1,7 +1,13 @@ import { prisma, toJson } from "@/lib/db"; import { apiHandler, requireAuth } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; -import { apiSuccess, apiError, safeJson } from "@/lib/api-response"; +import { + apiSuccess, + apiError, + getClientIp, + safeJson, +} from "@/lib/api-response"; +import { auditLog } from "@/lib/auth/audit"; import { setOnboardingPendingCookie } from "@/lib/auth/session"; import { normalisePrefs } from "@/lib/modules/gate"; import { modulesHoldingRecordData } from "@/lib/modules/domain-data"; @@ -98,6 +104,7 @@ export const POST = apiHandler(async (request: NextRequest) => { const onboarding = await completeNeedsFlow( user.id, readHeldUnitPreferences(user), + request, ); return apiSuccess({ completed: true, ...(onboarding ? { onboarding } : {}) }); @@ -111,7 +118,11 @@ export const POST = apiHandler(async (request: NextRequest) => { * needs flow — a legacy wizard completion then answers exactly the body it * always did. */ -async function completeNeedsFlow(userId: string, held: HeldUnitPreferences) { +async function completeNeedsFlow( + userId: string, + held: HeldUnitPreferences, + request: NextRequest, +) { const record = await prisma.onboardingRecord.findUnique({ where: { userId }, select: ONBOARDING_RECORD_SELECT, @@ -195,6 +206,22 @@ async function completeNeedsFlow(userId: string, held: HeldUnitPreferences) { ...(defaults.cycleTracking ? { cycleTrackingEnabled: true } : {}), }); + // The same trail the dedicated modules route writes, under the same event + // name, because it is the same column and the activity panel is where + // somebody goes to ask why their modules changed. The flow was writing + // three audited settings columns and leaving no trail at all, which made + // the answer to that question "nothing did it". + const stored = normalisePrefs(row?.modulePreferencesJson); + const changed = [ + ...Object.keys(merged).filter((key) => merged[key] !== stored[key]), + ...(defaults.cycleTracking ? ["cycleTrackingEnabled"] : []), + ]; + await auditLog("user.modules.update", { + userId, + ipAddress: getClientIp(request), + details: { changed, keptForData: [...holdsData], source: "onboarding" }, + }); + keptForData = [...holdsData]; derived = true; } diff --git a/src/lib/openapi/routes/onboarding.ts b/src/lib/openapi/routes/onboarding.ts index 01da443e5..ff219c68d 100644 --- a/src/lib/openapi/routes/onboarding.ts +++ b/src/lib/openapi/routes/onboarding.ts @@ -376,7 +376,7 @@ export const onboardingPaths: NonNullable = { tags: ["Onboarding"], summary: "Stamp onboarding as complete, and derive the module map", description: - "Marks onboarding finished and saves whatever profile fields came with it, then clears the proxy-readable pending cookie so the next navigation stops redirecting to `/onboarding`.\n\nv1.39 — this is also the needs-based flow's confirm endpoint, and the two halves live side by side. For a record that FINISHED the questions — every one of them answered or deliberately passed — it derives that record's module map from the answers and applies it, ONCE: the derivation marker is what stops a second confirm re-applying the questionnaire over decisions taken in Settings since, and `POST /api/onboarding/restart` clears the marker when the person asks for the questions again. The merge is one-directional — a module the answers name is switched on, a module they do not name is switched off only where the record does not already carry an explicit on. The `cycle` key is not written here at all: it delegates to the cycle profile, which this route sets to `true` when the cycle area was chosen and never to `false`. A half-answered flow derives nothing and is not stamped as complete: the answers become a module map, and a flow somebody abandoned after the first screen would have every remaining question read as its conservative default. The same gate is what keeps the LEGACY wizard's completion out of the derivation, since that wizard never puts these questions.\n\nThe response then carries `onboarding` beside `completed`; a caller that never entered the needs flow gets the fixed acknowledgement it always did.\n\nThe older sibling of `POST /api/onboarding/step`. It is unconditional in a way the step route is not: it stamps the completion whatever the stored step is, it re-stamps on every call rather than refusing a second one, and it writes no audit row and enforces no rate limit of its own. It also does NOT seed the dashboard from the goal selection — that only happens on the step route's completing call.\n\nCalling this mid-wizard makes every remaining `POST /api/onboarding/step` answer 409 `onboarding.step.completed`, because that route's guarded update requires the completion stamp to still be null.\n\nCookie or wildcard Bearer; `userId` is never read from the body.", + "Marks onboarding finished and saves whatever profile fields came with it, then clears the proxy-readable pending cookie so the next navigation stops redirecting to `/onboarding`.\n\nv1.39 — this is also the needs-based flow's confirm endpoint, and the two halves live side by side. For a record that FINISHED the questions — every one of them answered or deliberately passed — it derives that record's module map from the answers and applies it, ONCE: the derivation marker is what stops a second confirm re-applying the questionnaire over decisions taken in Settings since, and `POST /api/onboarding/restart` clears the marker when the person asks for the questions again. The merge is one-directional — a module the answers name is switched on, a module they do not name is switched off only where the record does not already carry an explicit on. The `cycle` key is not written here at all: it delegates to the cycle profile, which this route sets to `true` when the cycle area was chosen and never to `false`. A half-answered flow derives nothing and is not stamped as complete: the answers become a module map, and a flow somebody abandoned after the first screen would have every remaining question read as its conservative default. The same gate is what keeps the LEGACY wizard's completion out of the derivation, since that wizard never puts these questions.\n\nThe response then carries `onboarding` beside `completed`; a caller that never entered the needs flow gets the fixed acknowledgement it always did.\n\nThe older sibling of `POST /api/onboarding/step`. It is unconditional in a way the step route is not: it stamps the completion whatever the stored step is, it re-stamps on every call rather than refusing a second one, and it enforces no rate limit of its own. Its legacy half writes no audit row; the needs half writes one (`user.modules.update`) when it derives, because that is the same column the dedicated modules route audits. It also does NOT seed the dashboard from the goal selection — that only happens on the step route's completing call.\n\nCalling this mid-wizard makes every remaining `POST /api/onboarding/step` answer 409 `onboarding.step.completed`, because that route's guarded update requires the completion stamp to still be null.\n\nCookie or wildcard Bearer; `userId` is never read from the body.", requestBody: { required: true, content: { "application/json": { schema: onboardingCompleteRequest } }, diff --git a/tests/integration/onboarding-needs.test.ts b/tests/integration/onboarding-needs.test.ts index 01b5ae15f..beeceb41a 100644 --- a/tests/integration/onboarding-needs.test.ts +++ b/tests/integration/onboarding-needs.test.ts @@ -229,6 +229,17 @@ describe("PATCH /api/onboarding/answers", () => { select: { glucoseUnit: true, unitPreference: true }, }); expect(row).toEqual({ glucoseUnit: "mmol/L", unitPreference: "imperial" }); + + // Same trail as the two dedicated settings routes, so it does not matter + // which surface the person changed the units from. + const actions = ( + await getPrismaClient().auditLog.findMany({ + where: { userId: user.id }, + select: { action: true }, + }) + ).map((entry) => entry.action); + expect(actions).toContain("user.glucose-unit.update"); + expect(actions).toContain("user.unit-preference.update"); }); it("does not re-ask the units the account already holds", async () => { @@ -304,6 +315,15 @@ describe("POST /api/onboarding/complete", () => { expect(statusOf(state, "confirm")).toBe("done"); expect(state.completedAt).not.toBeNull(); + // The activity panel has to be able to answer "why did my modules change". + const audit = await getPrismaClient().auditLog.findMany({ + where: { userId: user.id, action: "user.modules.update" }, + }); + expect(audit).toHaveLength(1); + expect( + (audit[0].details as { changed?: string[] } | null)?.changed, + ).toContain("medications"); + // Somebody changes their mind in Settings, then the confirm screen is // replayed. The second call must leave that decision alone: the latch is // what stops a questionnaire from re-applying itself over a later choice.