From 831940f1b9bde932047324424e62937cddce830c Mon Sep 17 00:00:00 2001 From: Edwin van den Houdt Date: Fri, 4 Sep 2026 22:00:04 +0200 Subject: [PATCH] refactor: extract toWizardErrors, adopted by all 3 wizards (RD-20) Each wizard flattened its per-field error record into the shell's WizardError[] summary with its own copy of the same loop. Extract one pure helper, wizard-errors.ts, next to naarStapLabel. Add a spec that covers a flat record, an empty record, skipped undefined/empty-string values, the idPrefix, and a skipped nested object. registratie-wizard.machine.ts changes Errors from an interface to a type alias, because only a type alias gets an implicit index signature and is assignable to the helper's Record parameter. The other two machines already declare their error maps as type aliases, so this also makes the three consistent. Co-Authored-By: Claude Sonnet 5 --- .../herregistratie-wizard.component.ts | 8 +- .../intake-wizard/intake-wizard.component.ts | 10 +- .../domain/registratie-wizard.machine.ts | 4 +- .../registratie-wizard.component.ts | 10 +- .../readable-codebase/RD-20-wizard-errors.md | 170 ++++++++++++++++++ docs/project/readable-codebase/README.md | 2 +- libs/shared/docs/behaviour-spec.mdx | 11 +- .../layout/wizard-shell/wizard-errors.spec.ts | 35 ++++ .../src/layout/wizard-shell/wizard-errors.ts | 13 ++ 9 files changed, 239 insertions(+), 24 deletions(-) create mode 100644 docs/project/readable-codebase/RD-20-wizard-errors.md create mode 100644 libs/shared/src/layout/wizard-shell/wizard-errors.spec.ts create mode 100644 libs/shared/src/layout/wizard-shell/wizard-errors.ts diff --git a/apps/ssp/src/app/herregistratie/ui/herregistratie-wizard/herregistratie-wizard.component.ts b/apps/ssp/src/app/herregistratie/ui/herregistratie-wizard/herregistratie-wizard.component.ts index f2bf768..45c0284 100644 --- a/apps/ssp/src/app/herregistratie/ui/herregistratie-wizard/herregistratie-wizard.component.ts +++ b/apps/ssp/src/app/herregistratie/ui/herregistratie-wizard/herregistratie-wizard.component.ts @@ -9,6 +9,7 @@ import { WizardPhase, naarStapLabel, } from '@shared/layout/wizard-shell/wizard-shell.component'; +import { toWizardErrors } from '@shared/layout/wizard-shell/wizard-errors'; import { ConfirmationComponent } from '@shared/ui/confirmation/confirmation.component'; import { createStore } from '@shared/application/store'; import { whenTag } from '@shared/kernel/fp'; @@ -251,12 +252,7 @@ export class HerregistratieWizardComponent { } }); /** Current step's field errors, flattened for the shell's error summary. */ - protected errorList = computed(() => { - const e = this.editing()?.errors ?? {}; - return (Object.keys(e) as (keyof typeof e)[]) - .filter((k) => e[k]) - .map((k) => ({ id: k, message: e[k]! })); - }); + protected errorList = computed(() => toWizardErrors(this.editing()?.errors ?? {})); constructor() { // An explicit seed (stories/tests) wins; otherwise resume the backend draft diff --git a/apps/ssp/src/app/herregistratie/ui/intake-wizard/intake-wizard.component.ts b/apps/ssp/src/app/herregistratie/ui/intake-wizard/intake-wizard.component.ts index 2ee3ff1..d6b87e9 100644 --- a/apps/ssp/src/app/herregistratie/ui/intake-wizard/intake-wizard.component.ts +++ b/apps/ssp/src/app/herregistratie/ui/intake-wizard/intake-wizard.component.ts @@ -15,6 +15,7 @@ import { WizardPhase, naarStapLabel, } from '@shared/layout/wizard-shell/wizard-shell.component'; +import { toWizardErrors } from '@shared/layout/wizard-shell/wizard-errors'; import { createStore } from '@shared/application/store'; import { whenTag } from '@shared/kernel/fp'; import { BigProfileStore } from '@registratie/application/big-profile.store'; @@ -360,12 +361,9 @@ export class IntakeWizardComponent { }); /** Current step's field errors, flattened for the shell's error summary. The field ids match the answer keys, so the summary anchors jump to the field. */ - protected errorList = computed(() => { - const e = this.answering()?.errors ?? {}; - return (Object.keys(e) as (keyof Answers)[]) - .filter((k) => e[k]) - .map((k) => ({ id: k, message: e[k]! })); - }); + protected errorList = computed(() => + toWizardErrors(this.answering()?.errors ?? {}), + ); protected err = (k: keyof Answers) => this.answering()?.errors[k] ?? ''; protected set = (key: keyof Answers, value: string) => diff --git a/apps/ssp/src/app/registratie/domain/registratie-wizard.machine.ts b/apps/ssp/src/app/registratie/domain/registratie-wizard.machine.ts index d5fd063..67abd47 100644 --- a/apps/ssp/src/app/registratie/domain/registratie-wizard.machine.ts +++ b/apps/ssp/src/app/registratie/domain/registratie-wizard.machine.ts @@ -66,7 +66,7 @@ export type DraftField = 'straat' | 'postcode' | 'woonplaats' | 'email'; /** Per-field error map. `antwoorden` holds per-policy-question errors, keyed by question id (a step can show several questions). */ -export interface Errors { +export type Errors = { straat?: string; postcode?: string; woonplaats?: string; @@ -75,7 +75,7 @@ export interface Errors { diploma?: string; documenten?: string; antwoorden?: Record; -} +}; export type RegistratieState = | { tag: 'Invullen'; draft: Draft; cursor: number; errors: Errors; upload: UploadState } diff --git a/apps/ssp/src/app/registratie/ui/registratie-wizard/registratie-wizard.component.ts b/apps/ssp/src/app/registratie/ui/registratie-wizard/registratie-wizard.component.ts index 50a1d4c..21e75b4 100644 --- a/apps/ssp/src/app/registratie/ui/registratie-wizard/registratie-wizard.component.ts +++ b/apps/ssp/src/app/registratie/ui/registratie-wizard/registratie-wizard.component.ts @@ -17,6 +17,7 @@ import { WizardPhase, naarStapLabel, } from '@shared/layout/wizard-shell/wizard-shell.component'; +import { toWizardErrors } from '@shared/layout/wizard-shell/wizard-errors'; import { ASYNC } from '@shared/ui/async/async.component'; import { AddressFieldsComponent } from '@registratie/ui/address-fields/address-fields.component'; import { createStore } from '@shared/application/store'; @@ -468,14 +469,7 @@ export class RegistratieWizardComponent { /** Current step's errors (incl. per-question), flattened for the error summary. */ protected errorList = computed(() => { const e = this.invullen()?.errors ?? {}; - const out: WizardError[] = []; - for (const [k, v] of Object.entries(e)) { - if (k !== 'antwoorden' && typeof v === 'string' && v) out.push({ id: k, message: v }); - } - for (const [qid, msg] of Object.entries(e.antwoorden ?? {})) { - if (msg) out.push({ id: 'vraag-' + qid, message: msg }); - } - return out; + return [...toWizardErrors(e), ...toWizardErrors(e.antwoorden ?? {}, 'vraag-')]; }); protected adresSamenvatting = computed(() => { const d = this.draft(); diff --git a/docs/project/readable-codebase/RD-20-wizard-errors.md b/docs/project/readable-codebase/RD-20-wizard-errors.md new file mode 100644 index 0000000..35bc178 --- /dev/null +++ b/docs/project/readable-codebase/RD-20-wizard-errors.md @@ -0,0 +1,170 @@ +# RD-20 — `wizard-errors.ts` + spec, adopted by all three wizards + +Status: done +Source: PLAN.md 3c + +## Why + +Each of the three wizards flattens its per-field error record into the shell's +`WizardError[]` summary, and each writes the flattening itself. Two of the three are the same +six lines with a different key type. The third does the same job plus a nested group. + +The loop is pure, it has no spec, and it lives in a component — so the one part of the wizard +that decides what the user sees in the error summary is the one part no test covers. + +Extract one pure helper beside `naarStapLabel`, which lives in that folder for exactly this +reason. + +## Read first + +- `libs/shared/src/layout/wizard-shell/wizard-shell.component.ts:19-27` — `naarStapLabel` and + the `WizardError` interface. The new file sits beside this one. +- The three call sites, in the order they get easier: + - `herregistratie-wizard.component.ts:253-259` + - `intake-wizard.component.ts:361-368` + - `registratie-wizard.component.ts:468-479` — the one with the nested group. +- `apps/ssp/src/app/registratie/domain/registratie-wizard.machine.ts:69` — `Errors`, declared + as an `interface`. Decision 3 is about this line. + +## Decisions (pre-made, don't relitigate) + +1. **One helper, two parameters, both used today:** + + ```ts + /** Flatten a machine's per-field error record into the shell's summary list. + Values that are not a non-empty string are skipped, so a nested group + (the registratie wizard's `antwoorden`) is appended by the caller with its + own `idPrefix` rather than special-cased here. */ + export function toWizardErrors(errors: Record, idPrefix = ''): WizardError[] { + const out: WizardError[] = []; + for (const [k, v] of Object.entries(errors)) { + if (typeof v === 'string' && v) out.push({ id: idPrefix + k, message: v }); + } + return out; + } + ``` + + `Record` plus the `typeof` guard narrows `v` to `string` with **no cast**, + which keeps the file inside the repo's `any`-free rule. Do not reach for a generic key type; + the three machines key their errors differently and the helper does not care. + +2. **New file `libs/shared/src/layout/wizard-shell/wizard-errors.ts`, plus its spec.** It takes + the `WizardError` type from the component file with a **type-only import**: + + ```ts + import type { WizardError } from './wizard-shell.component'; + ``` + + `import type` is erased at compile time, so the helper stays pure and its spec needs no + TestBed and pulls in no Angular at run time. Moving the `WizardError` interface into the new + file would be tidier on paper and would touch four more files for no behaviour; not worth it. + +3. **`registratie-wizard.machine.ts:69` changes from `interface Errors` to `type Errors`.** + This is required, not cosmetic. TypeScript gives an implicit index signature to a type alias + but **not** to an interface, so `Errors` as an interface is not assignable to + `Record` and the call site will not compile. The other two machines already + declare their error maps as type aliases (`Partial>`), so this also makes + the three consistent. Nothing `extends` or `implements` `Errors` — verified. + +4. **The three call sites become:** + + | Wizard | Body | + | -------------- | ------------------------------------------------------------------------- | + | herregistratie | `toWizardErrors(this.editing()?.errors ?? {})` | + | intake | `toWizardErrors(this.answering()?.errors ?? {})` | + | registratie | `[...toWizardErrors(e), ...toWizardErrors(e.antwoorden ?? {}, 'vraag-')]` | + + The registratie site keeps its `const e = this.invullen()?.errors ?? {};` line. The + `vraag-` prefix and the skipping of non-string values together replace its + `if (k !== 'antwoorden' && typeof v === 'string' && v)` filter. + +5. **Do not touch the three `shellStatus` switches.** The tags genuinely differ per machine and + an exhaustive switch is the house style. This ticket is about the error list only. + +6. **No story.** The helper is a pure function. Its spec is the test; `wizard-shell.stories.ts` + already renders the error summary. + +## Files + +- `libs/shared/src/layout/wizard-shell/wizard-errors.ts` (new) +- `libs/shared/src/layout/wizard-shell/wizard-errors.spec.ts` (new) +- `apps/ssp/src/app/herregistratie/ui/herregistratie-wizard/herregistratie-wizard.component.ts` +- `apps/ssp/src/app/herregistratie/ui/intake-wizard/intake-wizard.component.ts` +- `apps/ssp/src/app/registratie/ui/registratie-wizard/registratie-wizard.component.ts` +- `apps/ssp/src/app/registratie/domain/registratie-wizard.machine.ts` (decision 3, one line) +- `libs/shared/docs/behaviour-spec.mdx` (regenerated, never hand-edited) + +## Steps + +1. Write `wizard-errors.ts` per decisions 1 and 2. +2. Write `wizard-errors.spec.ts`. Cover: a flat record; an empty record; a record whose values + are `undefined` or `''` (both skipped); the `idPrefix`; and a record holding a nested object + value (skipped, which is what lets decision 4's registratie case work). +3. Apply decision 3 — one word, `interface` → `type`, and the `{` stays. +4. Convert the three call sites per decision 4. +5. Run `npm run gen:behaviour-spec` — the new spec titles otherwise fail the drift check. +6. `git add -A`, then run the acceptance commands. +7. Update this ticket's `Status:` to `done` and the README's RD-20 row to `done`. +8. Commit all of it together. + +## Acceptance criteria + +Measured against the tree before handover. + +```bash +git grep -c "export function toWizardErrors" -- libs/shared/src/layout/wizard-shell/wizard-errors.ts # MUST be 1 +git grep -l "toWizardErrors" -- apps libs | wc -l # is 0 -> MUST be 5 (helper, spec, 3 wizards) +``` + +The hand-rolled flattening is gone from all three: + +```bash +git grep -c "filter((k) => e\[k\])" -- apps | awk -F: '{s+=$NF} END {print s+0}' # is 2 -> MUST be 0 +git grep -c "k !== 'antwoorden'" -- apps | awk -F: '{s+=$NF} END {print s+0}' # is 1 -> MUST be 0 +``` + +Decision 3 landed: + +```bash +git grep -c "export interface Errors" -- apps/ssp/src/app/registratie/domain/registratie-wizard.machine.ts # is 1 -> MUST be 0 +git grep -c "export type Errors" -- apps/ssp/src/app/registratie/domain/registratie-wizard.machine.ts # MUST be 1 +``` + +```bash +npm run ci # exits 0 +``` + +## Verification + +`npm run ci` is enough. `--full` is **not** required: no story changes, and the new file is +under `libs/shared/src/layout/`, not `libs/shared/src/ui/`. Regenerating `behaviour-spec.mdx` +does not trigger `--full` on its own — RD-17 set that precedent. + +`npm run lint` inside the gate is what proves the three `/* eslint-disable max-lines */` +directives are still needed. `reportUnusedDisableDirectives` is `error`, so a directive that +stops being necessary fails the build. + +## Out of scope + +- Splitting any wizard into step components. RD-22 (intake) and RD-23 (registratie) own that, + and this ticket makes both smaller first. +- The `shellStatus` switches (decision 5). +- `change-request-form` and `besluit-form`. Neither builds a `WizardError[]`; they render field + errors directly. There is nothing to share. + +## Risks + +- **The `interface` → `type` change is load-bearing** (decision 3). Skipping it produces + "Index signature for type 'string' is missing in type 'Errors'" at the registratie call site, + and the tempting wrong fix is to widen the helper's parameter to `object`, which forces an + `any` and fails lint. +- **PLAN says this ticket deletes an `eslint-disable max-lines` from + `herregistratie-wizard.component.ts`. That is stale — there is no such directive.** Earlier + tickets already brought the file under the budget: it measures ~248 effective lines against a + limit of 250. The two disables that do exist (intake, registratie) stay; RD-22 and RD-23 + remove them. Both files are far above 250 (~362 and ~574 effective), so this ticket's saving + cannot make either directive unused. +- **`behaviour-spec.mdx` drift** from the new spec titles. Run `gen:behaviour-spec` in the same + commit, and never edit that file by hand. +- **Keep `import type`, not a value import** (decision 2). A value import of the component into + the helper drags Angular into a pure module and its spec. diff --git a/docs/project/readable-codebase/README.md b/docs/project/readable-codebase/README.md index 1d2137f..ef07a59 100644 --- a/docs/project/readable-codebase/README.md +++ b/docs/project/readable-codebase/README.md @@ -114,7 +114,7 @@ two. Note that RD-15 exists because 22 abandoned agent worktrees are still on di | RD-17 | `successOf`/`successOr` sweep — 10 sites, 8 files | 01 | | done | | RD-18 | Ticket-reference sweep, frontend — 181 refs, 100 files | 01 | yes | done | | RD-19 | Ticket-reference sweep, backend — 370 refs, 86 files | 01 | | done | -| RD-20 | `wizard-errors.ts` + spec, adopted by all 3 wizards | 02 | | todo | +| RD-20 | `wizard-errors.ts` + spec, adopted by all 3 wizards | 02 | | done | | RD-21 | `rich-text-dom.ts` helpers + spec cases | 02 | yes | todo | | RD-22 | `intake-wizard` to 3 step components | 08, 20 | yes | todo | | RD-23 | `registratie-wizard` to 3 steps + the upload-controller move | 08, 20 | yes | todo | diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index c733132..26cd4ee 100644 --- a/libs/shared/docs/behaviour-spec.mdx +++ b/libs/shared/docs/behaviour-spec.mdx @@ -20,7 +20,7 @@ tested where._ Every bullet below is a real test name from the suite — an `it()` title (frontend) or a test method name (backend), read as a sentence. Nothing here is hand-written prose: this page -**is** the suite, reshaped for a business reader. 532 frontend behaviours across +**is** the suite, reshaped for a business reader. 538 frontend behaviours across 9 contexts; 261 backend behaviours across 42 test classes. @@ -973,6 +973,15 @@ classes. - unwraps a Success value - is the fallback for every other state +#### toWizardErrors + +- flattens a flat record of field errors +- returns an empty list for an empty record +- skips a value that is undefined +- skips a value that is an empty string +- prefixes every id with idPrefix when given +- skips a value that is a nested object, so the caller can flatten it separately + #### upload lifecycle messages - queued → progress → complete diff --git a/libs/shared/src/layout/wizard-shell/wizard-errors.spec.ts b/libs/shared/src/layout/wizard-shell/wizard-errors.spec.ts new file mode 100644 index 0000000..26aaad1 --- /dev/null +++ b/libs/shared/src/layout/wizard-shell/wizard-errors.spec.ts @@ -0,0 +1,35 @@ +import { describe, it, expect } from 'vitest'; +import { toWizardErrors } from './wizard-errors'; + +describe('toWizardErrors', () => { + it('flattens a flat record of field errors', () => { + expect(toWizardErrors({ straat: 'Verplicht', postcode: 'Ongeldig' })).toEqual([ + { id: 'straat', message: 'Verplicht' }, + { id: 'postcode', message: 'Ongeldig' }, + ]); + }); + + it('returns an empty list for an empty record', () => { + expect(toWizardErrors({})).toEqual([]); + }); + + it('skips a value that is undefined', () => { + expect(toWizardErrors({ straat: undefined })).toEqual([]); + }); + + it('skips a value that is an empty string', () => { + expect(toWizardErrors({ straat: '' })).toEqual([]); + }); + + it('prefixes every id with idPrefix when given', () => { + expect(toWizardErrors({ q1: 'Verplicht' }, 'vraag-')).toEqual([ + { id: 'vraag-q1', message: 'Verplicht' }, + ]); + }); + + it('skips a value that is a nested object, so the caller can flatten it separately', () => { + expect(toWizardErrors({ straat: 'Verplicht', antwoorden: { q1: 'Verplicht' } })).toEqual([ + { id: 'straat', message: 'Verplicht' }, + ]); + }); +}); diff --git a/libs/shared/src/layout/wizard-shell/wizard-errors.ts b/libs/shared/src/layout/wizard-shell/wizard-errors.ts new file mode 100644 index 0000000..09e307e --- /dev/null +++ b/libs/shared/src/layout/wizard-shell/wizard-errors.ts @@ -0,0 +1,13 @@ +import type { WizardError } from './wizard-shell.component'; + +/** Flatten a machine's per-field error record into the shell's summary list. + Values that are not a non-empty string are skipped, so a nested group + (the registratie wizard's `antwoorden`) is appended by the caller with its + own `idPrefix` rather than special-cased here. */ +export function toWizardErrors(errors: Record, idPrefix = ''): WizardError[] { + const out: WizardError[] = []; + for (const [k, v] of Object.entries(errors)) { + if (typeof v === 'string' && v) out.push({ id: idPrefix + k, message: v }); + } + return out; +}