diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md b/docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md index 265fa41..854f879 100644 --- a/docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md +++ b/docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md @@ -100,41 +100,41 @@ deployed first_, not _must ship together_. Every ticket tracing to a `BIO-` finding, plus every row on agent 07's authoritative 16-row "Compliance review required" list, carries it — regardless of priority. -| ID | Module | Category | Description | Baseline metric improved | Effort | Risk | Priority | CD batch # | Depends on | Compliance | Status | -| --------- | -------------------------------- | ------------- | ------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | ------ | -------- | -------- | ---------- | ---------- | ------------ | -------- | -| **RB-01** | backend/Program.cs + Data | security | Add an owner/capability check to `GET /uploads/{id}/content` and `/uploads/status`; 404 not 403 | §3c Data 75.5% branch vs 99.0% line (BL-005) | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** | -| **RB-02** | backend/Program.cs + Data | privacy | Stop concatenating the BSN into `AuthzAudit.Resource`; assert on **values** in the test | §3c Data 75.5% branch (BL-005) | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** | -| **RB-03** | backend/Contracts | privacy | `MaskTail(a.Owner, 3)` in `ToAdminSummaryDto` — both cross-owner lists inherit it | §3a bhp/behandeling 91.6%/81.5%; §7 Mapping row | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** | -| **RB-04** | backend/Data | privacy | Mask the BSN used as `AuditEntry.Actor` on document audit rows (ownership column untouched) | §3c Data 99.0% line / 75.5% branch | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** | -| **RB-05** | backend/Zgw | privacy | Drop the BSN-bearing query + body snippet from the `ZgwHttpClient` exception message | §3c Zgw 98.1%/85.5% (best backend branch) — a design gap, not a test gap | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** | -| **RB-06** | backend/Program.cs | security | Delete the dead `POST /registrations` (no FE caller) — or add the `ForeignIds` guard | BL-003 (48 mappings in 940 lines, file CC 78) | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** | -| **RB-07** | backend/Program.cs | audit | Audit the **allow** path in all five authz gates + the 3 brief transitions and the besluit | §3c Program.cs 84.8% branch; BL-003 | S–M | Med | **P1** | 2 | — | **SIGN-OFF** | **done** | -| **RB-08** | backend/Program.cs | security | Route `DELETE /admin/uploads/{id}` through `CasesAdmin`; delete the orphaned `IsAdmin` gate | BL-003; §7 CQRS-light wrappers row | S | Low | **P1** | 2 | RB-07 | **SIGN-OFF** | **done** | -| **RB-09** | backend/Domain + Program.cs | security | `IIdentityProvider` can express "no identity"; stub Development-only; fail fast in Production | §7 "Single-impl interface `IIdentityProvider`"; BL-006 | S | Med | **P1** | 2 | — | **SIGN-OFF** | **done** | -| **RB-10** | ssp/auth + bhp/auth + ssp/shell | testability | Extract `parseStoredSession` (×2 apps) + spec `redactProfile`; assert a stored BSN yields `''` | §3a auth 42.9%/46.2% (worst FE line, §8); file LH 2/LF 20, BRH 3/BRF 13 | S | Low | **P1** | 2 | — | **SIGN-OFF** | **done** | -| **RB-11** | ssp/brief + libs/shared/infra | security | Dev hatches out of prod on the 3 hand-written `fetch` paths; export their parse boundaries; fix the doc | §3b ssp/brief 42% reach (11/26, none `ui/`); §3a 68.8% branch | M | Med | **P1** | 2 | — | **SIGN-OFF** | **done** | -| **RB-12** | backend/tests (CI) | security gate | One test enumerating the route table; every route hits an authz wrapper or an explicit allow-list | BL-006 (zero backend architecture enforcement) | M | Low | **P1** | 3 | — | **SIGN-OFF** | **done** | -| **RB-13** | ssp/auth + bhp/auth | ADR execution | Land `Session → Principal`; `MedewerkerAdapter`; backoffice login stops being a DigiD/BSN form | BL-002 (211→151 dup after ADR-C-006; expected <40 after this) | M | Med | **P1** | 3 | RB-09 | **SIGN-OFF** | **done** | -| **RB-14** | repo (CI) | security gate | `dotnet list package --vulnerable --include-transitive` as a failing step | BL-006; §7 (the .NET tree is entirely unscanned today) | S | Low | P2 | 3 | — | **SIGN-OFF** | **done** | -| **RB-15** | backend/Program.cs | security | Wrap Swagger + the OpenAPI document in `if (app.Environment.IsDevelopment())` | BL-003; §3c Program.cs 97.4%/84.8% | S | Low | P2 | 3 | — | **SIGN-OFF** | **done** | -| **RB-16** | backend/Stamdata | input valid. | `DateOnly.TryParse` on `?peildatum=` → 400 instead of an unhandled 500 | §3c Stamdata 96.8% line / **71.7% branch** (BL-005) | S | Low | P2 | 3 | — | **SIGN-OFF** | **done** | -| **RB-17** | libs/shared/app + brief + beheer | CQRS-light | Split `runResult` (fold) from `runSubmit` (fold + idempotency mint); point the 5 reads at it | BL-007; §7 "read adapters 20 / mutations inline ~13" | S | Low | P2 | 3 | — | **SIGN-OFF** | **done** | -| **RB-18** | backend/Data | security | Key `IdempotencyStore` on `{SubjectId}:{idemKey}` | §7 stores "Not behind any port"; agent 02's Data note (no TTL, no reset) | S | Low | P2 | 3 | RB-17 | **SIGN-OFF** | **done** | -| **RB-19** | backend/Program.cs | structure | Reorder all 48 endpoints under read/write sub-banners; regroup admin-cases + org-template preview | BL-003 (940 lines, file CC 78 vs next-highest 27) | S | **High** | P2 | 4 | RB-12 | **SIGN-OFF** | **done** | -| **RB-20** | ssp/registratie | CQRS-light | `ApplicationsStore.cancel` / `AdminCasesStore.delete` through `runSubmit`; surface the error | BL-007; §7 "Command factories 3" | S | Low | P2 | 4 | — | **SIGN-OFF** | **done** | -| **RB-21** | ssp/registratie | CQRS-light | Extract the read half of `createDraftSync` into `application/find-concept.ts` | §4a `createDraftSync` 143 lines — longest fn in the repo; §9 (>40) | M | Med | P2 | 4 | — | — | **done** | -| **RB-22** | ssp/brief | CQRS-light | _(expand)_ `BriefStore.load()` tolerates a 404 by calling the existing `reset()` once | BL-003; §7 Backend CQRS-light row | S | Low | P2 | 4 | — | **SIGN-OFF** | **done** | -| **RB-23** | backend/Program.cs + Data | CQRS-light | _(contract)_ `GET /brief` 404s when absent; `GetOrCreate` → `Get` | BL-003; §7 Backend CQRS-light row | S | Med | P2 | 4 | RB-22 | **SIGN-OFF** | **done** | -| **RB-24** | libs/shared/upload | ADR conform. | Move `upload/` into `infrastructure`/`domain`/`application`; **delete** the depcruise carve-out | BL-010; §7 "+1 adapter outside `infrastructure/`", "8 of 9 machines in `domain/`"; §3b shared/domain 0% reach | M | Med | P2 | 5 | — | **SIGN-OFF** | **done** | -| **RB-25** | libs/shared/upload | testability | `UPLOAD_TRANSPORT` injection token (the `SESSION_PORT` shape) instead of `inject(KeepaliveTransport)` | §3a upload 52.0%/50.0%; §3b file unreached, non-`ui/` | S | Low | P2 | 5 | RB-24 | **SIGN-OFF** | **done** | -| **RB-26** | libs/shared/upload | testability | Move the accept/reject decision to `planFileSelection` in `upload.machine.ts` | §3a upload 52.0%/50.0%; §4a module max CC 27 | S | Low | P2 | 5 | RB-24 | **SIGN-OFF** | **done** | -| **RB-27** | libs/shared/upload | testability | Extract `uploadOutcome(status, responseText)` out of the XHR closure | file LH 5/64 (**7.8% line**), BRH 3/57 (**5.3% branch**) | S–M | Low | P2 | 5 | RB-25 | **SIGN-OFF** | open | -| **RB-28** | libs/beheer + ssp/brief | testability | `BLOB_PRESENTER` token; the 3 commands' success paths become assertable | §3a beheer/application **40.5% branch — worst FE**; brief.store BRH 32/64 | S–M | Low | P2 | 5 | — | **SIGN-OFF** | **done** | -| **RB-29** | backend/Domain | testability | Thread the existing `at` through `LetterHtml.ResolveAuto` instead of reading `UtcNow` | §3c Domain 82.0% branch; §4b `LetterHtml.cs` CC 21 | S | Low | P2 | 5 | — | — | **done** | -| **RB-30** | backend/Data + Domain | testability | Extract 5 brief guards into `Domain/Letters/BriefRules.cs`; add `tests/Domain/BriefRuleTests.cs` | §3c Data **75.5% branch** (BL-005); §4b `BriefStore.cs` CC 17, `ToDto` CC 16 | M | Med | P2 | 5 | — | **SIGN-OFF** | **done** | -| **RB-31** | 4 app contexts (specs only) | ADR conform. | Replace hand-rolled state literals with `given(reduce, initial)` replays in 4 machine specs | §7 Elm machines 9 (1 has a `*.testing.ts`); §3a herreg 67.8% / brief 68.8% branch | M | Low | P2 | 6 | — | — | open | -| **RB-32** | libs/shared/docs | ADR conform. | Add the missing `language-switcher` row to the CIBG gap register (9 markers vs 8 rows) | §2 libs/shared 86 files / 5 194 lines; §6 layout Ca 22 | S | Low | P3 | 6 | — | — | open | -| **RB-33** | libs/shared/testing | ADR conform. | Adopt `unwrapOk` at its one call site — **or delete it**; both satisfy ADR-0006 §3 | BL-004; §3a libs/shared/testing 3 files, 100% line | S | Low | P3 | 6 | — | — | open | +| ID | Module | Category | Description | Baseline metric improved | Effort | Risk | Priority | CD batch # | Depends on | Compliance | Status | +| --------- | -------------------------------- | ------------- | ------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | ------ | -------- | -------- | ---------- | ---------- | ------------ | --------------- | +| **RB-01** | backend/Program.cs + Data | security | Add an owner/capability check to `GET /uploads/{id}/content` and `/uploads/status`; 404 not 403 | §3c Data 75.5% branch vs 99.0% line (BL-005) | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** | +| **RB-02** | backend/Program.cs + Data | privacy | Stop concatenating the BSN into `AuthzAudit.Resource`; assert on **values** in the test | §3c Data 75.5% branch (BL-005) | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** | +| **RB-03** | backend/Contracts | privacy | `MaskTail(a.Owner, 3)` in `ToAdminSummaryDto` — both cross-owner lists inherit it | §3a bhp/behandeling 91.6%/81.5%; §7 Mapping row | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** | +| **RB-04** | backend/Data | privacy | Mask the BSN used as `AuditEntry.Actor` on document audit rows (ownership column untouched) | §3c Data 99.0% line / 75.5% branch | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** | +| **RB-05** | backend/Zgw | privacy | Drop the BSN-bearing query + body snippet from the `ZgwHttpClient` exception message | §3c Zgw 98.1%/85.5% (best backend branch) — a design gap, not a test gap | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** | +| **RB-06** | backend/Program.cs | security | Delete the dead `POST /registrations` (no FE caller) — or add the `ForeignIds` guard | BL-003 (48 mappings in 940 lines, file CC 78) | S | Low | **P1** | 1 | — | **SIGN-OFF** | **done** | +| **RB-07** | backend/Program.cs | audit | Audit the **allow** path in all five authz gates + the 3 brief transitions and the besluit | §3c Program.cs 84.8% branch; BL-003 | S–M | Med | **P1** | 2 | — | **SIGN-OFF** | **done** | +| **RB-08** | backend/Program.cs | security | Route `DELETE /admin/uploads/{id}` through `CasesAdmin`; delete the orphaned `IsAdmin` gate | BL-003; §7 CQRS-light wrappers row | S | Low | **P1** | 2 | RB-07 | **SIGN-OFF** | **done** | +| **RB-09** | backend/Domain + Program.cs | security | `IIdentityProvider` can express "no identity"; stub Development-only; fail fast in Production | §7 "Single-impl interface `IIdentityProvider`"; BL-006 | S | Med | **P1** | 2 | — | **SIGN-OFF** | **done** | +| **RB-10** | ssp/auth + bhp/auth + ssp/shell | testability | Extract `parseStoredSession` (×2 apps) + spec `redactProfile`; assert a stored BSN yields `''` | §3a auth 42.9%/46.2% (worst FE line, §8); file LH 2/LF 20, BRH 3/BRF 13 | S | Low | **P1** | 2 | — | **SIGN-OFF** | **done** | +| **RB-11** | ssp/brief + libs/shared/infra | security | Dev hatches out of prod on the 3 hand-written `fetch` paths; export their parse boundaries; fix the doc | §3b ssp/brief 42% reach (11/26, none `ui/`); §3a 68.8% branch | M | Med | **P1** | 2 | — | **SIGN-OFF** | **done** | +| **RB-12** | backend/tests (CI) | security gate | One test enumerating the route table; every route hits an authz wrapper or an explicit allow-list | BL-006 (zero backend architecture enforcement) | M | Low | **P1** | 3 | — | **SIGN-OFF** | **done** | +| **RB-13** | ssp/auth + bhp/auth | ADR execution | Land `Session → Principal`; `MedewerkerAdapter`; backoffice login stops being a DigiD/BSN form | BL-002 (211→151 dup after ADR-C-006; expected <40 after this) | M | Med | **P1** | 3 | RB-09 | **SIGN-OFF** | **done** | +| **RB-14** | repo (CI) | security gate | `dotnet list package --vulnerable --include-transitive` as a failing step | BL-006; §7 (the .NET tree is entirely unscanned today) | S | Low | P2 | 3 | — | **SIGN-OFF** | **done** | +| **RB-15** | backend/Program.cs | security | Wrap Swagger + the OpenAPI document in `if (app.Environment.IsDevelopment())` | BL-003; §3c Program.cs 97.4%/84.8% | S | Low | P2 | 3 | — | **SIGN-OFF** | **done** | +| **RB-16** | backend/Stamdata | input valid. | `DateOnly.TryParse` on `?peildatum=` → 400 instead of an unhandled 500 | §3c Stamdata 96.8% line / **71.7% branch** (BL-005) | S | Low | P2 | 3 | — | **SIGN-OFF** | **done** | +| **RB-17** | libs/shared/app + brief + beheer | CQRS-light | Split `runResult` (fold) from `runSubmit` (fold + idempotency mint); point the 5 reads at it | BL-007; §7 "read adapters 20 / mutations inline ~13" | S | Low | P2 | 3 | — | **SIGN-OFF** | **done** | +| **RB-18** | backend/Data | security | Key `IdempotencyStore` on `{SubjectId}:{idemKey}` | §7 stores "Not behind any port"; agent 02's Data note (no TTL, no reset) | S | Low | P2 | 3 | RB-17 | **SIGN-OFF** | **done** | +| **RB-19** | backend/Program.cs | structure | Reorder all 48 endpoints under read/write sub-banners; regroup admin-cases + org-template preview | BL-003 (940 lines, file CC 78 vs next-highest 27) | S | **High** | P2 | 4 | RB-12 | **SIGN-OFF** | **done** | +| **RB-20** | ssp/registratie | CQRS-light | `ApplicationsStore.cancel` / `AdminCasesStore.delete` through `runSubmit`; surface the error | BL-007; §7 "Command factories 3" | S | Low | P2 | 4 | — | **SIGN-OFF** | **done** | +| **RB-21** | ssp/registratie | CQRS-light | Extract the read half of `createDraftSync` into `application/find-concept.ts` | §4a `createDraftSync` 143 lines — longest fn in the repo; §9 (>40) | M | Med | P2 | 4 | — | — | **done** | +| **RB-22** | ssp/brief | CQRS-light | _(expand)_ `BriefStore.load()` tolerates a 404 by calling the existing `reset()` once | BL-003; §7 Backend CQRS-light row | S | Low | P2 | 4 | — | **SIGN-OFF** | **done** | +| **RB-23** | backend/Program.cs + Data | CQRS-light | _(contract)_ `GET /brief` 404s when absent; `GetOrCreate` → `Get` | BL-003; §7 Backend CQRS-light row | S | Med | P2 | 4 | RB-22 | **SIGN-OFF** | **done** | +| **RB-24** | libs/shared/upload | ADR conform. | Move `upload/` into `infrastructure`/`domain`/`application`; **delete** the depcruise carve-out | BL-010; §7 "+1 adapter outside `infrastructure/`", "8 of 9 machines in `domain/`"; §3b shared/domain 0% reach | M | Med | P2 | 5 | — | **SIGN-OFF** | **done** | +| **RB-25** | libs/shared/upload | testability | `UPLOAD_TRANSPORT` injection token (the `SESSION_PORT` shape) instead of `inject(KeepaliveTransport)` | §3a upload 52.0%/50.0%; §3b file unreached, non-`ui/` | S | Low | P2 | 5 | RB-24 | **SIGN-OFF** | **done** | +| **RB-26** | libs/shared/upload | testability | Move the accept/reject decision to `planFileSelection` in `upload.machine.ts` | §3a upload 52.0%/50.0%; §4a module max CC 27 | S | Low | P2 | 5 | RB-24 | **SIGN-OFF** | **done** | +| **RB-27** | libs/shared/upload | testability | Extract `uploadOutcome(status, responseText)` out of the XHR closure | file LH 5/64 (**7.8% line**), BRH 3/57 (**5.3% branch**) | S–M | Low | P2 | 5 | RB-25 | **SIGN-OFF** | **implemented** | +| **RB-28** | libs/beheer + ssp/brief | testability | `BLOB_PRESENTER` token; the 3 commands' success paths become assertable | §3a beheer/application **40.5% branch — worst FE**; brief.store BRH 32/64 | S–M | Low | P2 | 5 | — | **SIGN-OFF** | **done** | +| **RB-29** | backend/Domain | testability | Thread the existing `at` through `LetterHtml.ResolveAuto` instead of reading `UtcNow` | §3c Domain 82.0% branch; §4b `LetterHtml.cs` CC 21 | S | Low | P2 | 5 | — | — | **done** | +| **RB-30** | backend/Data + Domain | testability | Extract 5 brief guards into `Domain/Letters/BriefRules.cs`; add `tests/Domain/BriefRuleTests.cs` | §3c Data **75.5% branch** (BL-005); §4b `BriefStore.cs` CC 17, `ToDto` CC 16 | M | Med | P2 | 5 | — | **SIGN-OFF** | **done** | +| **RB-31** | 4 app contexts (specs only) | ADR conform. | Replace hand-rolled state literals with `given(reduce, initial)` replays in 4 machine specs | §7 Elm machines 9 (1 has a `*.testing.ts`); §3a herreg 67.8% / brief 68.8% branch | M | Low | P2 | 6 | — | — | open | +| **RB-32** | libs/shared/docs | ADR conform. | Add the missing `language-switcher` row to the CIBG gap register (9 markers vs 8 rows) | §2 libs/shared 86 files / 5 194 lines; §6 layout Ca 22 | S | Low | P3 | 6 | — | — | open | +| **RB-33** | libs/shared/testing | ADR conform. | Adopt `unwrapOk` at its one call site — **or delete it**; both satisfy ADR-0006 §3 | BL-004; §3a libs/shared/testing 3 files, 100% line | S | Low | P3 | 6 | — | — | open | --- diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-27.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-27.md new file mode 100644 index 0000000..596a495 --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-27.md @@ -0,0 +1,193 @@ +# RB-27 — `uploadOutcome` extracted from the XHR `load` closure + +Status: **implemented** · 2026-08-28 · Source finding: `02-testability.md` TE-005 · +`99-backlog.md` RB-27, "Merges" table row for RB-25/26/27 · Depends on +`implementation/rb-24.md` (the move that put this file at its current path) and +`implementation/rb-25.md` (handoff paragraph read before deciding the optional half) + +## What was wrong + +`libs/shared/src/infrastructure/upload.adapter.ts`'s `xhrUpload` constructs +`new XMLHttpRequest()` directly and attaches its `load` listener inline. The listener +body held the actual decisions: 2xx-vs-not, `JSON.parse` of the response body with a +fallback to a generic error, and (on a non-2xx status) ProblemDetails mapping via the +un-exported `parseError`. None of it is reachable without stubbing the XHR global, so +the interpretation logic had no spec. + +TE-005's baseline citation: **LH 5 / LF 64 (7.8% line), BRH 3 / BRF 57 (5.3% branch)**. +The file was counted "reached" in the module total only because another spec imports +it — essentially nothing in it executed. + +## What changed + +One function extracted from the `load` listener, in the same file: + +```ts +export function uploadOutcome( + status: number, + responseText: string, +): Result { + if (status < 200 || status >= 300) return err(parseError(responseText)); + try { + return ok({ documentId: JSON.parse(responseText).documentId }); + } catch { + return err(genericError()); + } +} +``` + +placed next to `genericError`/`parseError` (below the class, above the dev +`simulateUpload`). It contains exactly the 2xx-vs-not check, the `JSON.parse`-with- +fallback, and the ProblemDetails mapping — the three decisions TE-005 names. The `load` +listener is now a two-line dispatch: + +```ts +xhr.addEventListener('load', () => { + const outcome = uploadOutcome(xhr.status, xhr.responseText); + outcome.ok ? resolve(outcome.value) : reject(outcome.error); +}); +``` + +`Result`, `ok`, `err` are imported from `@shared/kernel/fp` (the repo's one `Result` +type, already used the same way by `libs/shared`'s other infrastructure adapters). +`parseError` and `genericError` are untouched — `uploadOutcome` calls them exactly as +the old listener body did, so their own behavior (ProblemDetails detail extraction, +generic fallback) is unchanged. + +## Abort-vs-error: left as a separate, smaller concern + +TE-005 names abort-vs-error disambiguation in the same sentence as the extraction +target, but its proposed signature — `uploadOutcome(status: number, responseText: +string)` — has no way to express "the request was aborted before any response +arrived." That is a real, structural mismatch, not an oversight to route around: + +- `uploadOutcome` runs inside the `load` listener, which fires only when the browser + received a complete HTTP response — it has a `status` and a `responseText` by + construction. +- The `abort` listener fires instead of `load` when `xhr.abort()` was called + client-side. There is no HTTP response at that point — no status, no body — so + folding it into `uploadOutcome`'s signature would mean inventing a fake status (e.g. + `0`) to stand for "not actually a response," which trades one implicit convention for + another and makes the pure function's contract lie about what it receives. + +The existing code already expresses this as the smallest form it can take: + +```ts +xhr.addEventListener('abort', () => (aborted ? reject(UPLOAD_ABORTED) : reject(genericError()))); +``` + +one ternary, deciding between two sentinels based on which native event fired and +whether `cancel()` was called first — not on response content. It is not a second +`uploadOutcome`-shaped decision hiding in a closure; it is a one-line dispatch already. +Extracting it into its own named function would add a call site and an import for a +single ternary with no reachable-only-via-DOM logic left inside it. Left in place, as +DoD point 2 allows. + +## Spec added, verified red + +`libs/shared/src/infrastructure/upload.adapter.spec.ts` (new file) — plain +`describe`/`it`, no `TestBed`, no DOM, no XHR stub, matching the DoD's explicit +"that is the entire point." Five cases: + +1. 2xx status with a valid JSON body → `{ ok: true, value: { documentId } }`. +2. 2xx status with an unparseable body → falls back to the generic `UPLOAD_FAILED` + text (the `JSON.parse`-with-fallback branch). +3. Non-2xx status with a ProblemDetails body → the `detail` field, via `parseError`. +4. Non-2xx status with a body that is not ProblemDetails-shaped → falls back to the + generic text. +5. The 200/300 boundary: 299 is success, 300 is not. + +`UPLOAD_FAILED`'s text is not exported (unchanged by this ticket), so the spec holds +its own copy of the Dutch string as a local constant with a comment pointing at the +source — the same trade every other spec makes when asserting against `$localize` +constants that never leave their module ($localize`strings are English-first prose +only where the source is`nl`, so this is the source text as written, not a stand-in). + +**Red-proof.** Edited `uploadOutcome`'s body down to a single line — +`return ok({ documentId: JSON.parse(responseText).documentId });`, dropping the +status check and the try/catch — with an `Edit` (not `git checkout`). Ran +`ng test shared`. Result: 4 of the 5 new specs failed: + +``` +SyntaxError: Unexpected token 'o', "not json" is not valid JSON + ❯ uploadOutcome libs/shared/src/infrastructure/upload.adapter.ts:168:32 + +AssertionError: expected { ok: true, value: { …(1) } } to deeply equal { ok: false, …(1) } +- Expected "error": "Document is al aan een aanvraag gekoppeld.", "ok": false, ++ Received "ok": true, "value": { "documentId": undefined }, + +SyntaxError: Unexpected token 'I', "Internal S"... is not valid JSON + +AssertionError: expected true to be false // Object.is equality +``` + +(only the plain 2xx-valid-JSON case still passed, as expected of a mutant that always +reports success). Re-applied the real body with a second `Edit`; `git diff` against +HEAD shows only the intended net change — the red edit left no trace. Re-ran: +163/163 green. + +## Coverage, `upload.adapter.ts` + +| Metric | Before (TE-005 baseline) | After | +| -------- | ------------------------ | ---------------------- | +| Lines | LH 5 / LF 64 (7.8%) | LH 12 / LF 65 (18.5%) | +| Branches | BRH 3 / BRF 57 (5.3%) | BRH 7 / BRF 59 (11.9%) | + +(`LF`/`BRF` grew by one line and two branches because `uploadOutcome` is new source; +`npm run test:coverage`'s shared run, `coverage/shared/lcov.info`, narrowed to this +file's `SF:` block.) The jump is real but modest in absolute percentage: `uploadOutcome` +itself is now fully exercised (`FNDA:6,uploadOutcome`, both branches of the status +check hit, both the try and the catch path hit), but the class methods +(`categoriesResource`, `status`, `deleteDocument`, `xhrUpload`'s own body, +`simulateUpload`) remain unreached — they need DI/XHR/timers to test and are +out of this ticket's scope, exactly as TE-005 scopes it ("extract the interpretation, +not the transport"). + +## Optional scenario-branch move: not taken + +TE-005 suggests, as an explicitly optional second half, moving the `currentScenario()` +branch from `xhrUpload` up into `KeepaliveTransport.send()` +(`libs/shared/src/application/upload-shell.service.ts`) so `xhrUpload` becomes +transport-only. RB-25's handoff confirms the seam is available (`KeepaliveTransport` +is still unexported, `send()` is still an unchanged one-liner) but not required. + +This ticket does not take that half, for a reason RB-25's handoff does not settle: +the ticket's own **Scope** section restricts this ticket to `upload.adapter.ts` and its +spec only ("RB-24, RB-25, RB-26, RB-28 have all already merged — nothing else in the +upload module is in flight, so you have the folder to yourself"). Moving the scenario +branch requires editing `upload-shell.service.ts` too — exporting `simulateUpload` (or +moving it) out of `upload.adapter.ts` and importing it into the application-layer +`send()` — which is a second file, outside the stated scope. Doing it anyway would also +widen this single-file ticket's diff for an explicitly optional half the ticket itself +says to skip when it "complicates the diff." The dev simulator's behavior is therefore +byte-for-byte unchanged: `xhrUpload` still checks `currentScenario()` first and still +delegates to the untouched `simulateUpload` for `upload-slow`/`upload-fail`, verified by +inspection (the only edit inside `xhrUpload` is the `load`-listener dispatch) and by the +full `shared` suite staying green, including `upload-shell.service.spec.ts`'s existing +scenario-adjacent assertions. + +## Verification + +- `npm run lint`: clean. +- `npm run dep:check`: unaffected — the only new import is `@shared/kernel/fp`, already + the repo's shared `Result` module, imported the same way by other `libs/shared` + infrastructure adapters (no new import direction). +- `npm test` / `ng test shared`: 163/163, across 26 spec files — 5 of those tests are + the new `upload.adapter.spec.ts`, the other 158 across 25 pre-existing files are + unchanged by this ticket. +- `npm run ci`: result and step count reported in the implementing agent's final answer. + +## Batch 5 close-out + +Batch 5 (RB-25 through RB-30) is now fully implemented. For `libs/shared/upload` +(moved to its layered home by RB-24) specifically: `upload.machine.ts` (domain) has its +own spec and `planFileSelection` extracted by RB-26; `upload-shell.service.ts` +(application) has a full spec covering `upload()`/`cancel()`/`delete()`/ +`pollReturning()` via the `UPLOAD_TRANSPORT` token RB-25 added; `upload-controller.ts` +(application) was already spec'd before this batch; `upload.adapter.ts` +(infrastructure) now has `uploadOutcome` as a pure, spec'd seam, though the class's +HTTP-bound methods (categories/status/delete/the XHR transport itself) remain +untested by design — XHR is the one boundary this batch deliberately does not +abstract, per TE-005's own instruction. End to end, every layer of the upload module +that can hold pure logic now does, and has a spec proving it; what is left uncovered is +exactly the DOM/network edge the module exists to wrap, not logic hiding behind it. diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index d80a96a..2d7c71d 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. 492 frontend behaviours across +**is** the suite, reshaped for a business reader. 497 frontend behaviours across 9 contexts; 261 backend behaviours across 42 test classes. @@ -929,6 +929,14 @@ classes. - failed then retried returns to queued - UploadRemoved drops the upload +#### uploadOutcome + +- resolves a 2xx response with a valid JSON body to the document id +- falls back to the generic error when a 2xx body is not valid JSON +- maps a non-2xx ProblemDetails body to its detail +- falls back to the generic error for a non-2xx body without a ProblemDetails detail +- treats status 200-299 as success and everything else as failure + #### withIdempotencyKey / currentIdempotencyKey - threads the key to every read made inside the wrapped fn diff --git a/libs/shared/src/infrastructure/upload.adapter.spec.ts b/libs/shared/src/infrastructure/upload.adapter.spec.ts new file mode 100644 index 0000000..ddd33ec --- /dev/null +++ b/libs/shared/src/infrastructure/upload.adapter.spec.ts @@ -0,0 +1,35 @@ +import { describe, it, expect } from 'vitest'; +import { uploadOutcome } from './upload.adapter'; + +/** Matches the un-exported UPLOAD_FAILED fallback text in upload.adapter.ts. */ +const UPLOAD_FAILED = 'Uploaden is niet gelukt. Probeer het opnieuw.'; + +describe('uploadOutcome', () => { + it('resolves a 2xx response with a valid JSON body to the document id', () => { + const outcome = uploadOutcome(200, JSON.stringify({ documentId: 'doc-1' })); + expect(outcome).toEqual({ ok: true, value: { documentId: 'doc-1' } }); + }); + + it('falls back to the generic error when a 2xx body is not valid JSON', () => { + const outcome = uploadOutcome(201, 'not json'); + expect(outcome).toEqual({ ok: false, error: UPLOAD_FAILED }); + }); + + it('maps a non-2xx ProblemDetails body to its detail', () => { + const outcome = uploadOutcome( + 409, + JSON.stringify({ detail: 'Document is al aan een aanvraag gekoppeld.', status: 409 }), + ); + expect(outcome).toEqual({ ok: false, error: 'Document is al aan een aanvraag gekoppeld.' }); + }); + + it('falls back to the generic error for a non-2xx body without a ProblemDetails detail', () => { + const outcome = uploadOutcome(500, 'Internal Server Error'); + expect(outcome).toEqual({ ok: false, error: UPLOAD_FAILED }); + }); + + it('treats status 200-299 as success and everything else as failure', () => { + expect(uploadOutcome(299, JSON.stringify({ documentId: 'd' })).ok).toBe(true); + expect(uploadOutcome(300, JSON.stringify({ detail: 'x' })).ok).toBe(false); + }); +}); diff --git a/libs/shared/src/infrastructure/upload.adapter.ts b/libs/shared/src/infrastructure/upload.adapter.ts index 0cb0e7e..38bc22a 100644 --- a/libs/shared/src/infrastructure/upload.adapter.ts +++ b/libs/shared/src/infrastructure/upload.adapter.ts @@ -9,6 +9,7 @@ import { currentScenario } from '@shared/infrastructure/scenario'; import { currentSubject } from '@shared/infrastructure/subject'; import { environment } from '@shared/environments/environment'; import { DocumentCategory } from '@shared/domain/upload.machine'; +import { Result, err, ok } from '@shared/kernel/fp'; /** Answer-derived query params that affect which categories the server presents. */ export interface CategoryParams { @@ -128,15 +129,8 @@ export class UploadAdapter { if (e.lengthComputable) onProgress(Math.round((e.loaded / e.total) * 100)); }); xhr.addEventListener('load', () => { - if (xhr.status >= 200 && xhr.status < 300) { - try { - resolve({ documentId: JSON.parse(xhr.responseText).documentId }); - } catch { - reject(genericError()); - } - } else { - reject(parseError(xhr.responseText)); - } + const outcome = uploadOutcome(xhr.status, xhr.responseText); + outcome.ok ? resolve(outcome.value) : reject(outcome.error); }); xhr.addEventListener('error', () => reject(genericError())); xhr.addEventListener('abort', () => @@ -160,6 +154,24 @@ export class UploadAdapter { const UPLOAD_FAILED = $localize`:@@upload.failed:Uploaden is niet gelukt. Probeer het opnieuw.`; const genericError = (): string => UPLOAD_FAILED; +/** + * Pure interpretation of one finished XHR `load` event: 2xx-vs-not, `JSON.parse` + * of the body with a fallback to a generic error, and (on a non-2xx status) + * ProblemDetails mapping via `parseError`. No DOM and no XHR — the listener that + * calls this only reads `xhr.status`/`xhr.responseText` and dispatches the result. + */ +export function uploadOutcome( + status: number, + responseText: string, +): Result { + if (status < 200 || status >= 300) return err(parseError(responseText)); + try { + return ok({ documentId: JSON.parse(responseText).documentId }); + } catch { + return err(genericError()); + } +} + /** * Demo-only (dev): the real XHR POST finishes instantly for metadata, so progress * and failure can't otherwise be shown. Drives the progress bar over ~2.5s, then