From cc6e6ca221fb2d12ddc5ed1304b18bb37ba9ab8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 11:42:38 +0200 Subject: [PATCH] fix(onboarding): derive the module map only from a questionnaire that was finished The only gate on the derivation was Q1 being set, so one answer was enough to turn the whole questionnaire into a module map. Two ways that landed wrong and they compound. Somebody answers who the record is for, closes the tab, and later finishes the legacy four-step wizard, which posts to this same endpoint and knows nothing about these questions: the five unanswered ones each read as their conservative default, eleven surfaces are written off, and the once-only marker makes it unrepeatable. Or any client patches one answer and posts the confirm straight back. The gate now asks whether every question has an answer or a deliberate pass. A half-answered flow derives nothing, is not stamped complete, and keeps its confirm step pending, so finishing the questions later still does the work. The legacy arm is untouched: a caller with no setup row still answers the fixed acknowledgement it always did. --- docs/api/openapi.yaml | 22 +++++--- src/app/api/onboarding/complete/route.ts | 21 +++++++- src/lib/onboarding/__tests__/needs.test.ts | 60 ++++++++++++++++++++++ src/lib/onboarding/needs.ts | 31 +++++++++++ src/lib/openapi/routes/onboarding.ts | 2 +- tests/integration/onboarding-needs.test.ts | 41 ++++++++++++++- 6 files changed, 165 insertions(+), 12 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 597841360..21e04da25 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -16371,14 +16371,20 @@ paths: v1.39 — this is also the needs-based flow's confirm endpoint, and the two halves live side by side. For a record - that answered the questions (Q1 is set), 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`. The response then carries `onboarding` beside `completed`; a caller that never entered the needs - flow gets the fixed acknowledgement it always did. + 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. + + + The response then carries `onboarding` beside `completed`; a caller that never entered the needs flow gets the + fixed acknowledgement it always did. The older sibling of `POST /api/onboarding/step`. It is unconditional in a way the step route is not: it stamps diff --git a/src/app/api/onboarding/complete/route.ts b/src/app/api/onboarding/complete/route.ts index e18087449..d34e644f8 100644 --- a/src/app/api/onboarding/complete/route.ts +++ b/src/app/api/onboarding/complete/route.ts @@ -11,6 +11,7 @@ import { mergeDerivedModulePreferences, } from "@/lib/modules/registry"; import { + everyOnboardingQuestionSettled, readHeldUnitPreferences, resolveOnboardingSteps, type HeldUnitPreferences, @@ -32,8 +33,9 @@ import { onboardingCompleteSchema } from "@/lib/validations/onboarding"; * flow, and the two live side by side rather than one replacing the other. * The legacy half is unchanged: the optional profile fields, the completion * stamp on `User.onboardingCompletedAt`, and the cleared pending cookie. The - * needs half runs only for a record that actually answered the questions - * (`needs.recordTarget` is set) and is what turns the answers into a module + * needs half runs only for a record that actually FINISHED the questions — + * every one of them answered or deliberately passed — and is what turns them + * into a module * map — ONCE, guarded by `OnboardingRecord.modulesDerivedAt`, because a * re-derivation would re-apply the questionnaire over decisions taken in * Settings since. `POST /api/onboarding/restart` clears that marker when the @@ -129,6 +131,21 @@ async function completeNeedsFlow(userId: string, held: HeldUnitPreferences) { }); return toOnboardingStateDto(record, held); } + if (!everyOnboardingQuestionSettled(steps)) { + // The confirm screen is the only caller that may derive, and it is reached + // only once every question has an answer or a deliberate pass. Q1 alone is + // not enough: an abandoned flow would have every remaining question read + // as its conservative default, switch off the surfaces that default + // implies, and latch the result so nobody could re-derive it. This route + // is also the LEGACY wizard's endpoint, and that wizard knows nothing + // about these questions — so a person who answered Q1, left, and later + // finished the old wizard must not have a module map derived for them. + annotate({ + action: { name: "onboarding.needs.complete" }, + meta: { outcome: "incomplete" }, + }); + return toOnboardingStateDto(record, held); + } let derived = false; if (record.modulesDerivedAt === null) { diff --git a/src/lib/onboarding/__tests__/needs.test.ts b/src/lib/onboarding/__tests__/needs.test.ts index db09724f2..7af629791 100644 --- a/src/lib/onboarding/__tests__/needs.test.ts +++ b/src/lib/onboarding/__tests__/needs.test.ts @@ -16,6 +16,8 @@ import { applyOnboardingAnswer } from "../needs-apply"; import { accountHoldsBothUnits, defaultOnboardingSteps, + everyOnboardingQuestionSettled, + ONBOARDING_QUESTION_STEP_IDS, emptyOnboardingNeeds, isOnboardingSettled, ONBOARDING_STEP_IDS, @@ -354,3 +356,61 @@ describe("resolveOnboardingSteps", () => { ); }); }); + +describe("everyOnboardingQuestionSettled", () => { + function withStatuses( + status: "pending" | "done" | "skipped", + overrides: Partial> = {}, + ) { + return defaultOnboardingSteps().map((step) => ({ + ...step, + status: overrides[step.id] ?? status, + })); + } + + it("names the six questions and stops before the confirm screen", () => { + expect([...ONBOARDING_QUESTION_STEP_IDS]).toEqual([ + "who", + "areas", + "medication", + "sources", + "visit", + "units", + ]); + }); + + it("refuses a flow that only answered the first question", () => { + expect( + everyOnboardingQuestionSettled(withStatuses("pending", { who: "done" })), + ).toBe(false); + }); + + it("refuses a flow with any one question still waiting", () => { + for (const id of ONBOARDING_QUESTION_STEP_IDS) { + expect( + everyOnboardingQuestionSettled( + withStatuses("done", { [id]: "pending" }), + ), + `\`${id}\` still pending should not read as a finished questionnaire`, + ).toBe(false); + } + }); + + it("accepts a deliberate pass as an answer", () => { + expect( + everyOnboardingQuestionSettled(withStatuses("skipped", { who: "done" })), + ).toBe(true); + }); + + it("ignores what happens after the confirm screen", () => { + expect( + everyOnboardingQuestionSettled( + withStatuses("done", { + confirm: "pending", + "first-result": "pending", + done: "pending", + }), + ), + ).toBe(true); + }); +}); diff --git a/src/lib/onboarding/needs.ts b/src/lib/onboarding/needs.ts index be7606a9b..97a1282ef 100644 --- a/src/lib/onboarding/needs.ts +++ b/src/lib/onboarding/needs.ts @@ -296,6 +296,37 @@ export function parseOnboardingFirstResult( }; } +/** + * The steps that are QUESTIONS — everything the flow puts to the person before + * the confirm screen. + * + * Sliced out of the ordered list rather than written out again, so a question + * added to the flow joins this set on the day it lands and the completion gate + * below cannot fall behind the screens. + */ +export const ONBOARDING_QUESTION_STEP_IDS: readonly OnboardingStepId[] = + ONBOARDING_STEP_IDS.slice(0, ONBOARDING_STEP_IDS.indexOf("confirm")); + +/** + * Has the questionnaire been finished? + * + * Every question answered or deliberately passed — not "Q1 is set". The + * difference matters because the answers become a module map: deriving from a + * flow somebody abandoned after the first screen reads every unanswered + * question as its conservative default and switches off the surfaces that + * default implies, and the derivation latch then makes that unrepeatable. + * + * `who` cannot be passed (the schema gives it no skip arm), so "not pending" + * means "answered" for it and "answered or passed" for the rest. + */ +export function everyOnboardingQuestionSettled( + steps: readonly OnboardingStepState[], +): boolean { + return ONBOARDING_QUESTION_STEP_IDS.every( + (id) => steps.find((step) => step.id === id)?.status !== "pending", + ); +} + /** True once every step has been answered or deliberately passed. */ export function everyOnboardingStepSettled( steps: readonly OnboardingStepState[], diff --git a/src/lib/openapi/routes/onboarding.ts b/src/lib/openapi/routes/onboarding.ts index 2474b4814..01da443e5 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 answered the questions (Q1 is set), 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`. The 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 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.", 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 8d9771d55..e4277eacc 100644 --- a/tests/integration/onboarding-needs.test.ts +++ b/tests/integration/onboarding-needs.test.ts @@ -127,13 +127,18 @@ function statusOf(state: OnboardingStateDto, id: OnboardingStepId) { return state.steps.find((step) => step.id === id)?.status; } -/** The five answers a "blood pressure and medication" setup gives. */ +/** + * The answers a "blood pressure and medication" setup gives — all six + * questions, because the confirm screen is only reached once every one of them + * has an answer or a deliberate pass. + */ async function answerTheQuestions() { await patchAnswer({ step: "who", recordTarget: "me" }); await patchAnswer({ step: "areas", areas: ["blood-pressure", "labs"] }); await patchAnswer({ step: "medication", medication: "yes" }); await patchAnswer({ step: "sources", status: "skipped" }); await patchAnswer({ step: "visit", visit: "no" }); + await patchAnswer({ step: "units", status: "skipped" }); } async function modulePrefs(userId: string) { @@ -339,6 +344,40 @@ describe("POST /api/onboarding/complete", () => { expect(profile.cycleTrackingEnabled).toBe(true); }); + it("derives nothing from a questionnaire that was never finished", async () => { + const user = await makeUser("half"); + await signIn(user.id); + // Q1 and nothing else — the tab was closed, or the legacy four-step wizard + // posted here afterwards. Either way the remaining five questions have no + // answer, and reading them as their conservative defaults would switch off + // eleven surfaces and latch the result. + await patchAnswer({ step: "who", recordTarget: "me" }); + + const res = await postComplete(); + expect(res.status).toBe(200); + + const row = await getPrismaClient().user.findUniqueOrThrow({ + where: { id: user.id }, + select: { modulePreferencesJson: true }, + }); + expect(row.modulePreferencesJson).toBeNull(); + + const record = await getPrismaClient().onboardingRecord.findUniqueOrThrow({ + where: { userId: user.id }, + }); + expect(record.modulesDerivedAt).toBeNull(); + expect(record.completedAt).toBeNull(); + + const state = await readState(res); + expect(statusOf(state, "confirm")).toBe("pending"); + + // Finishing the questions later still derives, so the gate defers the + // work rather than losing it. + await answerTheQuestions(); + await postComplete(); + expect((await modulePrefs(user.id)).medications).toBe(true); + }); + it("leaves a record that never answered the questions exactly as it was", async () => { const user = await makeUser("legacy"); await signIn(user.id);