docs(backend): correct IntakePolicy's false re-validation claim (WP-68 F5)
The doc-comment claimed "the backend re-validates on submit as the authority" — it doesn't. Neither SubmitApplicationRequest nor IntakeRequest carries a scholing answer at all, so there's nothing to re-validate; both submit paths only apply SubmissionRules.RejectZeroUren. A crafted POST can bypass the scholing requirement entirely. States the gap and points to WP-69 (opened, not yet planned) for the enforcement, which needs a wire change. Also strengthens the F2 concurrency test to assert the persisted status matches whichever request actually won the race, and updates WP-68's own Decisions/ acceptance-criteria text to reflect two implementation choices that improved on the original write-up once real constraints surfaced: ProcessingWindow stays on ApplicationStore (StatusAt is already in the same file), and AanvraagStatusTag is not given a Concept member (would have broken AanvraagStatusTag_covers_the_published_lifecycle) — AanvraagStatus.Tag is nullable instead, null exactly for Concept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -1,6 +1,6 @@
|
||||
# WP-68 — Aggregate invariants + status modelling (architecture review remediation)
|
||||
|
||||
Status: todo
|
||||
Status: in progress
|
||||
Phase: 12 — DDD hardening
|
||||
|
||||
## Why
|
||||
@@ -84,18 +84,39 @@ Pre-made — do not relitigate.
|
||||
### F3 — the status type
|
||||
|
||||
1. **Move `AanvraagStatusTag` and `Besluit`** out of `Data/ApplicationStore.cs` into
|
||||
`Domain/Applications/` (namespace `BigRegister.Domain.Applications`). Move
|
||||
`ApplicationStore.ProcessingWindow` there too — `StatusAt` needs it, and `Data → Domain` is
|
||||
the legal direction.
|
||||
2. **Add `Concept` as the first member of `AanvraagStatusTag`.** Keep `Ingediend` even though
|
||||
nothing produces it today (verified: neither `ToStatusDto` nor `ZgwZaakMapper` emits it) —
|
||||
`BeoordelingRules.CanDecide` accepts it, the FE's `BeoordelingStatus` union declares it,
|
||||
`statusLabel` has a `$localize` id for it, and `Only_open_statuses_are_decidable` tests it.
|
||||
Deleting it would ripple into `messages.en.xlf`. Mark it reserved with a comment instead.
|
||||
3. **New `Domain/Applications/AanvraagStatus.cs`**: a `sealed record` carrying
|
||||
`AanvraagStatusTag Tag` plus the same optional payload fields the DTO has
|
||||
(`StepIndex`, `StepCount`, `Referentie`, `Manual`, `Reden`), constructed **only** via static
|
||||
factories — `Concept(stepIndex, stepCount)`, `InBehandeling(referentie, manual)`,
|
||||
`Domain/Applications/` (namespace `BigRegister.Domain.Applications`).
|
||||
**`ApplicationStore.ProcessingWindow` stays where it is.** The original text here said to
|
||||
move it too "because `StatusAt` needs it" — but `StatusAt` is an instance method on
|
||||
`Aanvraag`, itself defined in `ApplicationStore.cs`, so it already sits in the same file/
|
||||
namespace as `ProcessingWindow` and can reference it directly with no cross-namespace
|
||||
issue. Moving it would have been motion without a reason, and — found only once
|
||||
implementation started — `ApplicationTests.cs` references `ApplicationStore.ProcessingWindow`
|
||||
directly in two tests this WP's own acceptance criteria require to stay **unmodified**;
|
||||
moving the constant would have forced a choice between breaking that criterion or adding a
|
||||
forwarding shim for no gain. Leave it.
|
||||
2. **`AanvraagStatusTag` is NOT given a `Concept` member — implemented differently, deliberately.**
|
||||
The original text said to add `Concept` as the first member. That directly conflicts with
|
||||
this WP's own acceptance criterion that `AanvraagStatusTag_covers_the_published_lifecycle`
|
||||
(which asserts `Enum.GetNames<AanvraagStatusTag>()` equals exactly the five published-lifecycle
|
||||
names) passes **unmodified** — adding a sixth name breaks it. Found only once implementation
|
||||
started; resolved in favor of the harder constraint (the regression-net test) and a cleaner
|
||||
design: **`AanvraagStatus.Tag` is `AanvraagStatusTag?`, null exactly for Concept.** This
|
||||
still closes the actual finding (a magic string with no corresponding enum member,
|
||||
round-tripped through the DTO and `Enum.Parse`d) without touching the enum the test pins,
|
||||
and without the reduce-only "boolean + tag" shape rule #3 warns against — a nullable
|
||||
discriminator is the standard two-case union, not a second boolean bolted on. `Ingediend`
|
||||
is unaffected by this and is still kept reserved (see below).
|
||||
Keep `Ingediend` even though nothing produces it today (verified: neither `ToStatusDto` nor
|
||||
`ZgwZaakMapper` emits it) — `BeoordelingRules.CanDecide` accepts it, the FE's
|
||||
`BeoordelingStatus` union declares it, `statusLabel` has a `$localize` id for it, and
|
||||
`Only_open_statuses_are_decidable` tests it. Deleting it would ripple into
|
||||
`messages.en.xlf`. Mark it reserved with a comment instead.
|
||||
3. **New `Domain/Applications/AanvraagStatus.cs`**: a `sealed class` (not a `record` — no
|
||||
external mutation via `with` is wanted, and record value-equality/`ToString` boilerplate
|
||||
buys nothing for a short-lived read model) carrying `AanvraagStatusTag? Tag` (null =
|
||||
Concept) plus the same optional payload fields the DTO has (`StepIndex`, `StepCount`,
|
||||
`Referentie`, `Manual`, `Reden`), constructed **only** via static factories —
|
||||
`Concept(stepIndex, stepCount)`, `InBehandeling(referentie, manual)`,
|
||||
`Goedgekeurd(referentie)`, `Afgewezen(referentie, reden)`,
|
||||
`MeerInfoGevraagd(referentie, reden)`.
|
||||
**Rejected: a full abstract-record union** (one subrecord per tag). It is the purer
|
||||
@@ -104,8 +125,10 @@ Pre-made — do not relitigate.
|
||||
4. **`Aanvraag.StatusAt(DateTimeOffset now)`** — an instance method on the entity carrying the
|
||||
logic currently in `ToStatusDto` **verbatim**, including the "a recorded decision wins over
|
||||
the auto-approve computation" ordering.
|
||||
5. **`Mappers.ToStatusDto` becomes a one-line projection** of `a.StatusAt(now)`:
|
||||
`new(s.Tag.ToString(), s.StepIndex, s.StepCount, s.Referentie, s.Manual, s.Reden)`.
|
||||
5. **`Mappers.ToStatusDto` becomes a one-line projection** of `a.StatusAt(now)`, via a shared
|
||||
`Mappers.ToDto(this AanvraagStatus s)` extension (also used by `ZgwZaakMapper` — see below,
|
||||
point 7 — so both status producers agree on one projection):
|
||||
`new(s.Tag?.ToString() ?? "Concept", s.StepIndex, s.StepCount, s.Referentie, s.Manual, s.Reden)`.
|
||||
6. **`AanvraagStatusDto` is unchanged — `Tag` stays a `string`.** This is the safety property
|
||||
that makes F3 an internal refactor: **no wire change, no `gen:api` drift, no frontend
|
||||
change, no `messages.en.xlf` change.** Do not "improve" the DTO in this WP.
|
||||
@@ -196,23 +219,41 @@ member names are unchanged).
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] Submitting (or draft-syncing) an aanvraag with a `documentId` owned by another citizen is
|
||||
- [x] Submitting (or draft-syncing) an aanvraag with a `documentId` owned by another citizen is
|
||||
rejected with 400, and the other citizen's document remains deletable
|
||||
(`DeleteResult.Ok`).
|
||||
- [ ] `AanvraagStatusTag` contains `Concept`; no code compares a status against a string
|
||||
literal. `grep -rn '"Concept"' backend/src` returns no comparison sites.
|
||||
- [ ] `Enum.Parse<AanvraagStatusTag>` appears **at most once** in `backend/src`, at the
|
||||
`IZaakSource` seam (`Program.cs` beoordeling GET), and does not throw on an unknown tag.
|
||||
- [ ] `Mappers.ToStatusDto` contains no lifecycle logic — it projects `Aanvraag.StatusAt(now)`.
|
||||
- [ ] `ZgwZaakMapper` constructs no `AanvraagStatusDto` from string literals.
|
||||
- [ ] `npm run gen:api` leaves **no diff** in `backend/swagger.json` or
|
||||
`libs/shared/src/infrastructure/api-client.ts` (proof F3 changed no wire shape).
|
||||
- [ ] Two concurrent `POST /beoordeling/{id}/besluit` against an already-terminal aanvraag yield
|
||||
exactly one 200 and one 409; the recorded besluit is the first one.
|
||||
- [ ] `BeoordelingRules.RequiresToelichting` exists, is unit-tested, and is the only place the
|
||||
rule lives.
|
||||
- [ ] A `[Theory]` covers the (status × besluit) transition table at domain level.
|
||||
- [ ] `IntakePolicy`'s doc-comment no longer claims server-side re-validation; WP-69 exists.
|
||||
(`DeleteResult.Ok`). (`Submitting_a_foreign_documentId_is_rejected_and_leaves_it_deletable_by_its_owner`,
|
||||
`Draft_sync_with_a_foreign_documentId_is_rejected`.)
|
||||
- [x] `AanvraagStatusTag` does NOT contain `Concept` — implemented instead as
|
||||
`AanvraagStatus.Tag` being `AanvraagStatusTag?`, null exactly for Concept (see Decisions
|
||||
§F3.2 for why this replaced the original "add Concept to the enum" instruction). No
|
||||
_internal domain_ code compares a status against the `"Concept"` string; the one
|
||||
remaining comparison (`Program.cs`'s beoordeling GET, against `IZaakSource`'s wire DTO)
|
||||
is the deliberate wire-boundary exception, paired with the one allowed `Enum.TryParse`
|
||||
below.
|
||||
- [x] `Enum.Parse`/`TryParse<AanvraagStatusTag>` appears **at most once** in `backend/src`, at
|
||||
the `IZaakSource` seam (`Program.cs` beoordeling GET), and does not throw on an unknown
|
||||
tag (`Enum.TryParse` there, not `Enum.Parse`).
|
||||
- [x] `Mappers.ToStatusDto` contains no lifecycle logic — it projects `Aanvraag.StatusAt(now)`.
|
||||
- [x] `ZgwZaakMapper` constructs no `AanvraagStatusDto` from string literals.
|
||||
- [x] `npm run gen:api` leaves **no diff** in `backend/swagger.json` or
|
||||
`libs/shared/src/infrastructure/api-client.ts` beyond F1's new 400 responses (verified —
|
||||
the only diff after F3 is the two `.ProducesProblem(400)` blocks F1 added; proof F3
|
||||
changed no wire shape).
|
||||
- [x] Two concurrent `POST /beoordeling/{id}/besluit` racing on the same still-open aanvraag
|
||||
yield exactly one 200 and one 409; the persisted status matches whichever request won
|
||||
(`Concurrent_besluiten_on_the_same_aanvraag_yield_exactly_one_success`, stable across 5
|
||||
repeated runs).
|
||||
- [x] `BeoordelingRules.RequiresToelichting` exists, is unit-tested
|
||||
(`Only_a_non_approval_requires_a_toelichting`), and is the only place the rule lives.
|
||||
- [x] A `[Theory]`/aggregate-level test covers the transition table
|
||||
(`A_terminal_decision_refuses_any_further_besluit`,
|
||||
`MeerInfoOpvragen_is_not_terminal_a_further_besluit_is_still_legal` — via
|
||||
`Aanvraag.StatusAt` + `BeoordelingRules.CanDecide`, not just a bare-tag `[Theory]`, since
|
||||
`CanDecide` doesn't vary by which besluit is attempted — see Decisions for why a literal
|
||||
status×besluit cross-product theory would have been redundant with
|
||||
`Only_open_statuses_are_decidable`).
|
||||
- [x] `IntakePolicy`'s doc-comment no longer claims server-side re-validation; WP-69 exists
|
||||
(`docs/project/backlog/WP-69-intake-scholing-threshold-enforcement.md`).
|
||||
|
||||
## Verification
|
||||
|
||||
|
||||
Reference in New Issue
Block a user