diff --git a/docs/project/backlog/README.md b/docs/project/backlog/README.md index 721fbac..e6eea0b 100644 --- a/docs/project/backlog/README.md +++ b/docs/project/backlog/README.md @@ -122,6 +122,10 @@ for its existing violations, so every WP ends green. | [WP-69](WP-69-intake-scholing-threshold-enforcement.md) | Enforce the scholing threshold server-side | 12 · DDD hardening | done | | [WP-70](WP-70-test-data-builders.md) | Test-data builders: illegal fixtures unrepresentable (ADR-0006) | 12 · DDD hardening | done | | [WP-71](WP-71-test-framework-coherence.md) | Test framework coherence: BDD/DDD alignment, close the escape hatches | 12 · DDD hardening | done | +| [WP-72](WP-72-delete-legacy-submit-endpoints.md) | Delete the dead legacy submit endpoints | 12 · DDD hardening | done | +| [WP-73](WP-73-domain-unions.md) | `RegistrationStatus` and `Aanvraag` as closed unions | 12 · DDD hardening | done | +| [WP-74](WP-74-e2e-isolation.md) | E2E isolation without a new backend endpoint | 12 · DDD hardening | done | +| [WP-75](WP-75-fe-be-seam-closure.md) | Close the remaining FE/BE seams | 12 · DDD hardening | done | Sequencing dependencies (stated in the WPs too): 01 before 10–15 (axe covers story churn); 03/04 before 05–09 (boundaries stop new violations during refactors); 06 before 07 (typed diff --git a/docs/project/backlog/WP-72-delete-legacy-submit-endpoints.md b/docs/project/backlog/WP-72-delete-legacy-submit-endpoints.md new file mode 100644 index 0000000..fd8c177 --- /dev/null +++ b/docs/project/backlog/WP-72-delete-legacy-submit-endpoints.md @@ -0,0 +1,56 @@ +# WP-72 — Delete the dead legacy submit endpoints + +Status: done (6bc00a9) +Phase: 12 — DDD hardening + +## Why + +`POST /api/v1/intakes` and `POST /api/v1/herregistraties` were dead from the UI — the wizard +submits through `POST /applications/{id}/submit`, and nothing in `apps/` or `libs/` called the +generated `intakes()`/`herregistraties()` client methods. They were also strictly **less +capable** than the endpoint that replaced them: they minted a bare reference and wrote no +`Aanvraag`, made no ZGW/OpenZaak call, and performed no document-ownership check. + +WP-69 hardened `/intakes` with a 400 last session. Deleting the surface is the stronger fix; +WP-69's `/applications/{id}/submit` enforcement — the path the wizard actually uses — is +untouched. + +## Decisions (pre-made) + +1. Delete both routes together. Their two `EndpointTests` are `[Theory]`s parameterised across + _both_ routes, so deleting one would leave an `InlineData` row 404-ing. +2. **Keep** the shared `Submit(...)` helper, `ReferentieResponse`, `SubmissionRules.NewReference` + and the whole `IdempotencyStore` path — `/registrations` and `/change-requests` still use + them, and `IdempotencyTests` covers the latter. +3. This WP owns the wire artifacts; no other track runs `gen:api`. + +## Acceptance criteria + +- [x] Both routes return **404** against a live backend (verified by curl, not by inference). +- [x] Zero references remain in `libs/shared/src/infrastructure/api-client.ts`. +- [x] `gen:api` diff is **pure deletion** — 124 lines out of `swagger.json`, 109 out of the API + client, zero additions. +- [x] Backend tests 245 → 240, exactly the 5 deleted cases (2 `[Theory]`s × 2 rows + 1 `[Fact]`). +- [x] `Submit(...)` and the idempotency path survive with their live callers intact. + +## Verification + +```bash +cd backend && dotnet test BigRegister.slnx --filter "Category!=Integration" +npm run ci +curl -X POST http://localhost:5000/api/v1/intakes -d '{"uren":500}' # 404 +``` + +## Notes + +Committed together with WP-73 (`6bc00a9`): both edit `Program.cs`, and splitting them would +have produced a commit that does not build. The two were run in separate execution waves to +avoid a concurrent `dotnet build` collision — but since neither committed independently, the +file-level entanglement remained at integration time. Worth remembering when planning future +parallel backend tracks: **separate waves do not produce separate commits.** + +## Follow-ups + +- `docs/reference/fp-tea-atomic-design.md:587` / `ARCHITECTURE.md:464` still teach a + `visibleSteps`-with-a-`'scholing'`-step intake the fixed-3-step wizard no longer matches + (inherited from WP-69). diff --git a/docs/project/backlog/WP-73-domain-unions.md b/docs/project/backlog/WP-73-domain-unions.md new file mode 100644 index 0000000..239e720 --- /dev/null +++ b/docs/project/backlog/WP-73-domain-unions.md @@ -0,0 +1,90 @@ +# WP-73 — `RegistrationStatus` and `Aanvraag` as closed unions + +Status: done (6bc00a9) +Phase: 12 — DDD hardening + +## Why + +Two backend domain types still allowed illegal states, against `CLAUDE.md`'s non-negotiable #3. + +`RegistrationStatus` was a flat record whose **own doc-comment** admitted only `Geregistreerd` +should carry a herregistratie deadline — and noted the frontend modelled it correctly as a +discriminated union while the backend did not. It also made `reden` nullable on all three +variants where the FE requires it on two. + +`Aanvraag` was a mutable EF class with 14 public setters. Its `StatusAt` carried **five +`Referentie!` null-forgiving derefs** plus a `SubmittedAt!.Value` — the compiler saying out loud +that "Submitted ⇒ Referentie != null" was convention, not type. WP-68 left it mutable +deliberately; WP-70/71 bought most of the safety with a test-only builder, which was itself a +hand-rolled prototype of the union this WP builds for real. + +## Decisions (pre-made) + +1. **Full union, not private setters.** The cheaper option (flip 14 setters to `private set`, + 3 files, no migration) was rejected in favour of the honest modelling. +2. `RegistrationStatus` → abstract record + three sealed variants behind a private base ctor. + Chosen over WP-68's static-factory shape (`AanvraagStatus`) because with only 4 read sites the + abstract record is affordable and makes **reading** safe too, not just construction. +3. `Aanvraag` → `Concept | Submitted | Decided` (with `Decided` further split into + `Goedgekeurd | Afgewezen | MeerInfoGevraagd`), the EF row demoted to `AanvraagEntity` behind + a two-way mapper. +4. The `(Owner, Type)` "at most one unsubmitted aanvraag" rule is an **aggregate-set** invariant — + it cannot live on the entity and stays procedural in `CreateConcept` under the lock. Stated in + code so nobody tries to move it. +5. No migration, no schema change, no wire change. + +## Acceptance criteria + +- [x] **Illegal construction is a compile error, proven not assumed.** Each was attempted, the + compiler error recorded, then reverted: + +| Attempted illegal state | Compiler error | +| -------------------------------------- | ------------------------------------------------------------------------------------------------- | +| `Decided` with no referentie | `CS9035: Required member 'Aanvraag.Decided.Referentie' must be set` | +| `Geschorst` with a HerregistratieDatum | `CS1739: The best overload for 'Geschorst' does not have a parameter named 'HerregistratieDatum'` | +| `Afwijzen` with no toelichting | `CS9035: Required member 'Aanvraag.Decided.Afgewezen.Toelichting' must be set` | + +- [x] All five `Referentie!` derefs and the `SubmittedAt!.Value` are **gone**, not suppressed. + `IZaakSource.CreateZaak` narrows to `Aanvraag.Submitted`, removing the same class of deref + in both `LocalZaakSource` and `OpenZaakZaakSource`. +- [x] `reden` is now required on `Geschorst`/`Doorgehaald`, matching the FE union. +- [x] `HerregistratieRule.IsStatusConsistent` deleted as dead code — the type now guarantees what + it checked, and its test **could no longer construct the illegal state it existed to + catch**. That failure to compile is the proof the refactor worked. +- [x] Backend 242 → 241, exactly that one deleted test. No other count change. +- [x] `RegistrationStatusDto` and the application DTOs byte-identical — confirmed by diffing a + live backend's `/swagger/v1/swagger.json` against the checked-in copy. No `gen:api`. + +## The `Draft` decision (made explicitly) + +`ApplicationStore`'s doc-comment claimed `Draft` was "Concept only" (`Draft != null ⇒ !Submitted`), +but `Submit` never cleared it — so the invariant was **violated in production**. Resolved in +favour of the code matching the comment: `Submitted`/`Decided` simply have no `Draft` property, +so submitting drops it. Verified nothing reads a submitted aanvraag's draft — `draft-sync.ts`'s +`applyResume` is the only consumer of `ApplicationDetailDto.Draft` and only ever resumes an +unsubmitted wizard. + +## Deviations + +- **`Aanvraag` (EF row) renamed to `AanvraagEntity`.** The domain union needed the bare name to + match `RegistrationStatus`/`AanvraagStatus` conventions; keeping both would make every file + importing both namespaces ambiguous (`CS0104`). The table name is unaffected — EF derives it + from the `Applications` `DbSet` property, not the CLR type. +- **Step invariant loosened** from `0 <= StepIndex < StepCount` to `<=`: `CreateConcept` produces + `(0, 0)` before the wizard's first draft sync, which the strict form would reject at creation. +- `AanvraagBuilder.Decided(...)` now delegates to the real union constructors, dropping its own + hand-rolled toelichting guard; a one-line wrapper keeps the `.Decided(...).Build()` chain + source-compatible for existing call sites. + +## Verification + +```bash +cd backend && dotnet format --verify-no-changes && dotnet test BigRegister.slnx --filter "Category!=Integration" +npm run ci +``` + +## Follow-ups + +- Making `Besluit` flow through the generated client as an enum rather than a `string` would + remove that FE/BE seam entirely rather than guarding it (WP-75 added the guard) — but it is a + wire change. diff --git a/docs/project/backlog/WP-74-e2e-isolation.md b/docs/project/backlog/WP-74-e2e-isolation.md new file mode 100644 index 0000000..dc50248 --- /dev/null +++ b/docs/project/backlog/WP-74-e2e-isolation.md @@ -0,0 +1,86 @@ +# WP-74 — E2E isolation without a new backend endpoint + +Status: done (42f7bd6) +Phase: 12 — DDD hardening + +## Why + +The three Playwright specs shared one mutable backend and admitted it in their own comments +("Restart the backend between CI runs — a second run would see a leftover Concept"). A crashed +mid-wizard run poisoned every subsequent run via `CreateConcept`'s 409, and both mutating specs +acted as the same identity (`DocumentStore.DemoOwner`), so any new state-touching spec would +collide immediately. + +## Decisions (pre-made) + +**WP-70 recorded the fix as a dev-only seed endpoint. That premise was wrong**, and exploration +established why: + +- The DB path already routes through `IConfiguration` (`Program.cs`, + `Db.ConnectionString = GetConnectionString("AppDb") ?? …`), so `ConnectionStrings__AppDb` as an + env var gives a throwaway DB with **zero backend change** — the same trick + `TestWebApplicationFactory` already uses per-test. +- `StubIdentityProvider` **already honours** an `X-Subject` header; the only gap was that no FE + interceptor sent one. +- The backend has **no `IsDevelopment()` gate anywhere** (grep: zero hits), so a seed endpoint + would have had to invent the codebase's first environment gate — a new security posture for no + gain. + +So: throwaway DB + a dev-only `X-Subject` interceptor. No new endpoint, no environment gate. + +## Acceptance criteria + +- [x] **`npm run e2e` passes twice back-to-back with no backend restart** — the actual acceptance + test, and the thing that failed before this WP. +- [x] **`X-Subject` observed on a real request** reaching the backend (`X-Subject: 111222333` on + `GET /api/v1/uploads/categories`), not merely wired. +- [x] Each new BSN elfproef-verified by script against the weights `[9,8,7,6,5,4,3,2,-1]`. +- [x] No backend change, no new endpoint, no `IsDevelopment()` gate. +- [x] Committed port config still defaults to 4200 (verification used an override). + +## Notes on the two caveats + +- **`reuseExistingServer` stays on.** Flipping it to `false` would hard-fail `npm run e2e` for + anyone already running the docker stack on 4200/5000 — a real local-workflow regression. The + consequence (the throwaway DB only applies when Playwright itself spawns the backend; always + true in CI) is documented in a comment on the `webServer` entry. +- **Unique DB filename per invocation**, with `global-setup.ts` sweeping only _prior_ runs' + leftovers. A fixed name unlinked mid-run is only safe if SQLite's pool never reopens by path + afterwards; under `fullyParallel` that risks silently recreating an empty, unmigrated DB. + +## Deviation: interceptors alone were not enough + +Two hand-written call sites bypass Angular's interceptor chain (as `CLAUDE.md` documents) and +needed `X-Subject` stamped explicitly: + +- `libs/shared/src/upload/upload.adapter.ts`'s raw XHR upload — without this every uploaded + document landed under `DemoOwner`, breaking submit for any other identity. +- `apps/ssp/.../letter-preview.adapter.ts`'s preview fetch (plus `cache: 'no-store'`, correct + regardless since the endpoint sends no `Cache-Control`). + +## Known gap (a real backend bug, not caused by this WP) + +Under any BSN other than `DemoOwner`, `GET /brief/preview` returns a **sent** letter still +carrying the draft watermark — while `curl` against the same backend at the same instant returns +the correct frozen archive. Client caching was ruled out (`no-store`, then cache-busting query +strings), the dev proxy was ruled out, and it reproduced across two BSNs and never for +`DemoOwner`. This points at a staleness/race in `BriefStore`'s SQLite read path. + +`brief-v2.spec.ts` therefore keeps the shared `zorgverlener` identity — it still gains +throwaway-DB repeatability, just not per-spec identity isolation. `actors.ts` reserves a +`briefOpsteller` actor for whoever fixes the backend. **Tracked as a follow-up below.** + +## Verification + +```bash +npm run e2e # twice consecutively, no backend restart +npm run lint && npm run typecheck && npm test && npm run build +``` + +Note: port 4200 was held by an unrelated container on the dev machine, so verification ran with +`E2E_BASE_URL` pointed at an alternate port. The committed default is unchanged. + +## Follow-ups + +- **`/brief/preview` staleness for non-`DemoOwner` identities** (above) — the blocker for giving + `brief-v2.spec.ts` its own identity. diff --git a/docs/project/backlog/WP-75-fe-be-seam-closure.md b/docs/project/backlog/WP-75-fe-be-seam-closure.md new file mode 100644 index 0000000..b84a378 --- /dev/null +++ b/docs/project/backlog/WP-75-fe-be-seam-closure.md @@ -0,0 +1,65 @@ +# WP-75 — Close the remaining FE/BE seams + +Status: done (6fa27d1) +Phase: 12 — DDD hardening + +## Why + +WP-71 added `scripts/check-seam.sh` guarding one literal pair (the scholing threshold) and +documented three further FE/BE duplications that nothing tested across the seam. This closes +them — two by deletion, one by a guard, one by an actual fix. + +## Decisions (pre-made) + +1. **Dead reference impls get deleted, and `CLAUDE.md` is amended.** This overturns the + documented policy that server-owned rules "stay in `domain/*.policy.ts` as reference impl + + unit test". That policy is precisely what kept dead code alive. Blast radius is small: + `registration.policy.ts` is the only `*.policy.ts` in the repo. +2. Guard the `Besluit` tag list by **extending** `check-seam.sh`, not adding a second script. +3. The phone seam gets a **contract test**, not a grep check — see below. + +## Acceptance criteria + +- [x] `isHerregistratieEligible` deleted (uncalled; dead by its own doc-comment) along with + `isStatusConsistent` (also uncalled — WP-71 had added a spec for it the session before). + The three live exports (`statusLabel`, `statusColor`, `herregistratieDeadline`) stay, and + `herregistratieDeadline` gained direct coverage it previously only had transitively. +- [x] `CLAUDE.md` amended: server-owned rules live **only** on the server; the FE may mirror a + server-supplied _value_ (a threshold, a bound) for instant feedback, but never + reimplements the _algorithm_. ADR-0001's matching claim aligned. +- [x] `check-seam.sh` guards the `Besluit` tag list, **proven to fail** when a fourth member is + added to the C# enum only, naming both files and both lists. Anchored on the full + declaration so it avoids the "greps all matches" trap WP-69 documented. +- [x] Phone contract test added and green; backend stripping fixed. + +## The phone divergence was real, not latent + +WP-71 recorded this as latent because the Angular app normalises before sending — true of _that_ +path. The contract test proved the two sides genuinely disagreed: the backend returned **422** +for `+31612345678` and `(06) 12345678`, both of which the FE's own `parseTelefoonnummer` +accepts. Any non-Angular client, crafted POST, or future FE change would have hit it. + +`SubmissionRules.RejectPhoneChange` now strips exactly what the FE strips (`[\s\-()]`, then a +leading `+31` → `0`) before applying the shared `^0\d{9}$`. The FE value object was not touched — +it is the more permissive and correct side. + +**Why a contract test rather than a grep check:** both sides carry the identical `^0\d{9}$` +literal, so a drift check would have compared them, found them equal, and reported all clear. +The divergence was in the _normalisation before_ the regex — invisible to text comparison. Worth +remembering when choosing between the two guard styles: grep checks catch drifting **constants**, +contract tests catch drifting **behaviour**. + +## Verification + +```bash +npm run check:seam # both checks OK +npm run ci +cd backend && dotnet test BigRegister.slnx --filter "Category!=Integration" +``` + +## Follow-ups + +- Making `Besluit` flow through the generated client as an enum rather than a `string` would + remove that seam entirely rather than guarding it — a wire change, so not done here. +- The herregistratie-eligibility seam is closed by deletion; if a FE mirror is ever reintroduced, + the disjoint-fixture problem returns and would need a contract test, not a grep check. diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index febc184..34118ac 100644 --- a/libs/shared/docs/behaviour-spec.mdx +++ b/libs/shared/docs/behaviour-spec.mdx @@ -20,8 +20,8 @@ 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. 404 frontend behaviours across -8 contexts; 219 backend behaviours across 35 test +**is** the suite, reshaped for a business reader. 406 frontend behaviours across +8 contexts; 217 backend behaviours across 36 test classes. ## Frontend (by context) @@ -541,11 +541,9 @@ classes. #### registration.policy -- only an active registration within the window is eligible -- struck-off / suspended registrations are never eligible +- statusLabel echoes the tag - statusColor is total over the union -- a well-formed status is always consistent -- a Geregistreerd status without its herregistratieDatum is inconsistent +- herregistratieDeadline is only set for an active registration #### submit @@ -768,6 +766,13 @@ classes. - keeps unrelated query params and the path/hash - is a no-op when neither param is present +#### subjectInterceptor + +- stamps X-Subject on an /api/v1/ request once ?subject= has been seen +- keeps stamping later requests on the same tab after the query param is gone (WP-33-style stickiness) +- leaves a non-API request untouched even when a subject is known +- sends no header at all when no subject has ever been seen + #### upload lifecycle messages - queued → progress → complete @@ -922,8 +927,6 @@ classes. - IntakePolicy returns scholing threshold - Registration with duo diploma succeeds - Registration with manual diploma is rejected with problem details -- Zero hours submission is rejected -- Worked hours submission succeeds - Change request with valid phone succeeds - Change request with bad phone is rejected - Health endpoint is ok @@ -952,7 +955,6 @@ classes. - Not eligible before window - Eligible on window boundary - Suspended is not eligible -- Status consistency invariant ### IdempotencyTests @@ -977,7 +979,6 @@ classes. - Punten without gevolgd is rejected - Herregistratie is unaffected by the intake only gate - Zero uren is still afgewezen not a 400 -- Legacy intakes endpoint enforces it too ### LetterHtmlTests @@ -1040,6 +1041,11 @@ classes. - Rejects a margin outside the allowed range - Accepts margins on the boundary +### PhoneFormatContractTests + +- A leading plus31 is accepted like the frontends normalised form +- Parentheses around the area code are accepted like the frontend + ### PreviewEndpointTests - Preview of an unsent brief renders live with a watermark