diff --git a/apps/ssp/src/app/brief/application/brief.store.spec.ts b/apps/ssp/src/app/brief/application/brief.store.spec.ts index 145994a..b96941a 100644 --- a/apps/ssp/src/app/brief/application/brief.store.spec.ts +++ b/apps/ssp/src/app/brief/application/brief.store.spec.ts @@ -3,7 +3,12 @@ import { describe, it, expect, vi, afterEach } from 'vitest'; import { Result } from '@shared/kernel/fp'; import { Brief, BriefDecisions, CaseContext, LetterBlock } from '@brief/domain/brief'; import { OrgTemplate } from '@brief/domain/org-template'; -import { BriefAdapter, BriefView } from '@brief/infrastructure/brief.adapter'; +import { + BRIEF_LOAD_FAILED, + BriefAdapter, + BriefLoadFailure, + BriefView, +} from '@brief/infrastructure/brief.adapter'; import { LetterPreviewAdapter, PREVIEW_FAILED } from '@brief/infrastructure/letter-preview.adapter'; import { RevealBigNummerAdapter } from '@brief/infrastructure/reveal-bignummer.adapter'; import { BriefStore } from './brief.store'; @@ -60,7 +65,8 @@ describe('BriefStore action state (Idle | Busy | Failed)', () => { brief: { ...brief, status: { tag: 'approved', approvedBy: 'u2', approvedAt: 't' } }, }; const store = setup({ - load: (): Promise> => Promise.resolve({ ok: true, value: view }), + load: (): Promise> => + Promise.resolve({ ok: true, value: view }), save: (): Promise> => Promise.resolve({ ok: true, value: view }), approve: (): Promise> => Promise.resolve({ ok: true, value: approved }), @@ -79,7 +85,8 @@ describe('BriefStore action state (Idle | Busy | Failed)', () => { brief: { ...brief, status: { tag: 'approved', approvedBy: 'u2', approvedAt: 't' } }, }; const store = setup({ - load: (): Promise> => Promise.resolve({ ok: true, value: view }), + load: (): Promise> => + Promise.resolve({ ok: true, value: view }), save: (): Promise> => Promise.resolve({ ok: true, value: view }), approve: (): Promise> => Promise.resolve({ ok: true, value: approved }), @@ -93,7 +100,8 @@ describe('BriefStore action state (Idle | Busy | Failed)', () => { it('goes Busy then Failed on a failing transition, surfacing the error', async () => { const store = setup({ - load: (): Promise> => Promise.resolve({ ok: true, value: view }), + load: (): Promise> => + Promise.resolve({ ok: true, value: view }), save: (): Promise> => Promise.resolve({ ok: true, value: view }), approve: (): Promise> => Promise.resolve({ ok: false, error: 'niet toegestaan' }), @@ -108,7 +116,8 @@ describe('BriefStore action state (Idle | Busy | Failed)', () => { it('a subsequent successful transition clears a prior Failed state', async () => { let approveResult: Result = { ok: false, error: 'eerste poging mislukt' }; const store = setup({ - load: (): Promise> => Promise.resolve({ ok: true, value: view }), + load: (): Promise> => + Promise.resolve({ ok: true, value: view }), save: (): Promise> => Promise.resolve({ ok: true, value: view }), approve: (): Promise> => Promise.resolve(approveResult), }); @@ -156,8 +165,10 @@ function loadedBrief(store: BriefStore): Brief { } async function loadedStore(over: Partial = {}): Promise { - const ok = (v: BriefView): Promise> => - Promise.resolve({ ok: true, value: v }); + // Untyped return (inferred as the narrow `{ ok: true; value }` literal) so this one + // helper satisfies both `load` (error channel `BriefLoadFailure`) and `save` (error + // channel `string`) — it only ever produces the `ok: true` branch. + const ok = (v: BriefView) => Promise.resolve({ ok: true, value: v } as const); const store = setup({ load: () => ok(filledView), save: () => ok(filledView), ...over }); await store.load(); return store; @@ -255,8 +266,7 @@ describe('BriefStore rejection diff', () => { ...filledBrief, status: { tag: 'rejected', rejectedBy: 'u2', rejectedAt: 't', comments: 'nee' }, }; - const ok = (v: BriefView): Promise> => - Promise.resolve({ ok: true, value: v }); + const ok = (v: BriefView) => Promise.resolve({ ok: true, value: v } as const); const store = setup({ load: () => ok({ ...filledView, brief: submitted }), save: () => ok(filledView), @@ -283,7 +293,8 @@ describe('BriefStore.previewLetter', () => { it('opens the composed letter in a new tab on success', async () => { const store = setup({ - load: (): Promise> => Promise.resolve({ ok: true, value: view }), + load: (): Promise> => + Promise.resolve({ ok: true, value: view }), }); await store.load(); const blob = new Blob([''], { type: 'text/html' }); @@ -301,7 +312,8 @@ describe('BriefStore.previewLetter', () => { it('surfaces the error without opening a tab on failure', async () => { const store = setup({ - load: (): Promise> => Promise.resolve({ ok: true, value: view }), + load: (): Promise> => + Promise.resolve({ ok: true, value: view }), }); await store.load(); const open = vi.spyOn(window, 'open').mockImplementation(() => null); @@ -377,3 +389,42 @@ describe('BriefStore.flushPending (CanDeactivate guard / beforeunload)', () => { expect(save).not.toHaveBeenCalled(); }); }); + +// --- RB-22 (CQ-007 expand half): a 404 from GET /brief tolerates by calling the +// existing reset() command, exactly once. Today's backend never 404s (RB-23 adds +// that); this fake adapter is what exercises the branch until then. --- + +describe('BriefStore.load — 404 tolerance (RB-22)', () => { + const notFound: Result = { ok: false, error: { tag: 'notFound' } }; + const resetOk: Result = { ok: true, value: view }; + + it('a 404 drives exactly one reset(), which populates the store', async () => { + // Given GET /brief 404s (no brief exists yet) and reset() succeeds. + const load = vi.fn(() => Promise.resolve(notFound)); + const reset = vi.fn(() => Promise.resolve(resetOk)); + const store = setup({ load, reset }); + + // When the store loads... + await store.load(); + + // Then reset() ran exactly once, and the store ends up loaded from its result. + expect(reset).toHaveBeenCalledTimes(1); + expect(store.model().tag).toBe('loaded'); + }); + + it('a second 404 does not drive a second reset()', async () => { + // Given every load() attempt 404s (e.g. the brief still fails to appear). + const load = vi.fn(() => Promise.resolve(notFound)); + const reset = vi.fn(() => Promise.resolve(resetOk)); + const store = setup({ load, reset }); + + // When the store loads twice... + await store.load(); + await store.load(); + + // Then reset() ran exactly once — the once-only bound holds across calls, not + // just within one — and the second 404 surfaces as an ordinary load failure. + expect(reset).toHaveBeenCalledTimes(1); + expect(store.model()).toEqual({ tag: 'failed', reason: BRIEF_LOAD_FAILED }); + }); +}); diff --git a/apps/ssp/src/app/brief/application/brief.store.ts b/apps/ssp/src/app/brief/application/brief.store.ts index 88c2a1c..6bb8073 100644 --- a/apps/ssp/src/app/brief/application/brief.store.ts +++ b/apps/ssp/src/app/brief/application/brief.store.ts @@ -16,7 +16,7 @@ import { import { BriefMsg, BriefState, initial, reduce } from '@brief/domain/brief.machine'; import { BlockDiffKind, changedBlocks, diffBlocks } from '@brief/domain/brief-diff'; import { OrgTemplate } from '@brief/domain/org-template'; -import { BriefAdapter, BriefView } from '@brief/infrastructure/brief.adapter'; +import { BRIEF_LOAD_FAILED, BriefAdapter, BriefView } from '@brief/infrastructure/brief.adapter'; import { LetterPreviewAdapter } from '@brief/infrastructure/letter-preview.adapter'; import { RevealBigNummerAdapter } from '@brief/infrastructure/reveal-bignummer.adapter'; import { uploadContentUrl } from '@shared/upload/upload.adapter'; @@ -119,13 +119,40 @@ export class BriefStore implements PendingSave { return !!b && canSubmit(b) && !hasBlockingErrors(this.diagnostics()); }); + /** True once a 404-triggered recovery has been attempted (RB-22, CQ-007's expand + half — see `recoverFromMissingBrief`). This is the structural once-only bound: + a repeated 404 falls straight to the `error` branch below and can never reach + `adapter.reset()` a second time, regardless of how many times `load()` runs. */ + private hasRecoveredFromMissingBrief = false; + async load() { const r = await this.adapter.load(); if (r.ok) { - this.orgTemplate.set(r.value.orgTemplate); - this.caseContext.set(r.value.caseContext); - this.history.clear(); - this.store.dispatch({ tag: 'BriefLoaded', ...r.value }); + this.applyLoadedView(r.value); + } else if (r.error.tag === 'notFound' && !this.hasRecoveredFromMissingBrief) { + this.hasRecoveredFromMissingBrief = true; + await this.recoverFromMissingBrief(); + } else { + const reason = r.error.tag === 'notFound' ? BRIEF_LOAD_FAILED : r.error.reason; + this.store.dispatch({ tag: 'BriefLoadFailed', reason }); + } + } + + private applyLoadedView(view: BriefView) { + this.orgTemplate.set(view.orgTemplate); + this.caseContext.set(view.caseContext); + this.history.clear(); + this.store.dispatch({ tag: 'BriefLoaded', ...view }); + } + + /** `GET /brief` 404'd — no brief exists yet for this owner. Recover by calling the + existing `reset()` command directly (the same POST `resetDemo()` uses) and + applying whatever it returns; this NEVER calls `load()` again, so a second 404 + (e.g. `reset()` itself failing) cannot loop back into this method. */ + private async recoverFromMissingBrief() { + const r = await this.adapter.reset(); + if (r.ok) { + this.applyLoadedView(r.value); } else { this.store.dispatch({ tag: 'BriefLoadFailed', reason: r.error }); } diff --git a/apps/ssp/src/app/brief/infrastructure/brief.adapter.ts b/apps/ssp/src/app/brief/infrastructure/brief.adapter.ts index ad3aec3..02d3fad 100644 --- a/apps/ssp/src/app/brief/infrastructure/brief.adapter.ts +++ b/apps/ssp/src/app/brief/infrastructure/brief.adapter.ts @@ -1,6 +1,7 @@ import { Injectable, inject } from '@angular/core'; import { Result, ok, err } from '@shared/kernel/fp'; -import { runResult, runSubmit } from '@shared/application/submit'; +import { runSubmit } from '@shared/application/submit'; +import { problemDetail } from '@shared/infrastructure/api-error'; import { ApiClient, BriefDecisionsDto, @@ -33,9 +34,13 @@ import { Mark, Paragraph, RichTextBlock, RichTextNode } from '@shared/kernel/ric * The only place brief HTTP lives (ADR-0001 anti-corruption boundary). The wire * uses FLAT unions (a `type`/`tag` string + nullable fields, the repo convention); * the `parse*` boundary narrows them into the domain's proper discriminated unions - * and rejects malformed shapes. `load` (the only read) folds through `runResult`; - * every mutation folds through `runSubmit` (ProblemDetails → error string, plus the - * Idempotency-Key mint), then parses the returned brief. + * and rejects malformed shapes. Every mutation folds through `runSubmit` + * (ProblemDetails → error string, plus the Idempotency-Key mint), then parses the + * returned brief. `load` (the only read) does its own try/catch instead of the + * shared `runResult` fold, because it needs one extra bit `runResult` throws away: + * whether the failure was an HTTP 404 (see `BriefLoadFailure` — RB-22, CQ-007's + * expand half). Today's backend never 404s `GET /brief` (RB-23 adds that), so the + * `notFound` branch is unreached until RB-23 ships; this adapter is ready in advance. */ export interface BriefView { @@ -46,16 +51,39 @@ export interface BriefView { readonly caseContext: CaseContext; } +/** + * Why `load()` did not return a brief. `notFound` is a bare HTTP 404 — kept + * distinct from every other failure so `BriefStore.load()` can tolerate it (call + * `reset()` instead of showing an error banner) without conflating it with a real + * failure. See the class docstring above. + */ +export type BriefLoadFailure = + { readonly tag: 'notFound' } | { readonly tag: 'error'; readonly reason: string }; + export const BRIEF_LOAD_FAILED = $localize`:@@brief.load.failed:De brief kon niet worden geladen.`; export const BRIEF_ACTION_FAILED = $localize`:@@brief.action.failed:De actie is niet gelukt. Probeer het later opnieuw.`; +/** True when the thrown value carries an HTTP 404 status — matches both the + generic `SwaggerException` (today's shape, since `GET /brief` declares no 404 + response yet) and a parsed `ProblemDetails` (RFC 7807 `status`, the shape once + RB-23 gives the endpoint a documented 404 response). */ +function isHttpNotFound(e: unknown): boolean { + return !!e && typeof e === 'object' && (e as { status?: unknown }).status === 404; +} + @Injectable({ providedIn: 'root' }) export class BriefAdapter { private client = inject(ApiClient); - async load(): Promise> { - const r = await runResult(() => this.client.briefGET(), BRIEF_LOAD_FAILED); - return r.ok ? parseBriefView(r.value) : r; + async load(): Promise> { + try { + const dto = await this.client.briefGET(); + const parsed = parseBriefView(dto); + return parsed.ok ? ok(parsed.value) : err({ tag: 'error', reason: parsed.error }); + } catch (e) { + if (isHttpNotFound(e)) return err({ tag: 'notFound' }); + return err({ tag: 'error', reason: problemDetail(e, BRIEF_LOAD_FAILED) }); + } } async save(sections: readonly LetterSection[]): Promise> { 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 fcd230b..e47ed4e 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** | open | -| **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** | open | -| **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** | open | -| **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 | — | — | open | -| **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** | open | -| **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** | open | -| **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** | open | -| **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** | open | -| **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** | open | -| **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** | open | -| **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 | — | — | open | -| **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** | open | -| **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** | open | +| **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** | open | +| **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** | open | +| **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 | — | — | open | +| **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** | **implemented** | +| **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** | open | +| **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** | open | +| **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** | open | +| **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** | open | +| **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** | open | +| **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 | — | — | open | +| **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** | open | +| **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-22.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-22.md new file mode 100644 index 0000000..bce59a1 --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-22.md @@ -0,0 +1,139 @@ +# RB-22 — `BriefStore.load()` tolerates a 404, calling `reset()` exactly once + +Status: **implemented** · 2026-08-27 · Source findings: `04-cqrs-light.md` CQ-007 · +`99-backlog.md` RB-22, "Tickets that were rejected and split" · `implementation/rb-17.md` +(the `runResult`/`runSubmit` seam this store already sits on) + +This is the **expand** half of an expand/contract pair. RB-23 (backend: `GET /brief` 404s +when absent; `BriefStore.GetOrCreate` splits into `Get` + `ResetAndCreate`) ships after this +ticket, in a later merge. Today's backend never 404s `GET /brief`, so this ticket's new +branch is dead code in the running app — provably backend-frontend-safe by construction. + +## What was wrong + +CQ-007 flags `GET /brief` (`Program.cs:603` → `BriefStore.GetOrCreate`, +`Data/BriefStore.cs:50`) as the one endpoint in the backend where a GET performs a +persisted write, breaking the read/write split every other endpoint respects. The fix is +split across both sides of the seam because the FE must be ready to receive a 404 before +the backend can safely start sending one. This ticket is the FE half: `BriefStore.load()` +(`apps/ssp/src/app/brief/application/brief.store.ts`) had no notion of "no brief exists +yet" — every adapter failure, 404 included, dispatched `BriefLoadFailed` and showed the +generic error banner. `BriefAdapter.load()` (`brief.adapter.ts`) also had no way to tell +the store a failure was specifically an HTTP 404: it folded every failure through the +shared `runResult` helper (RB-17), which keeps only a human-readable string and throws +away the HTTP status. + +The ticket read as filed against the current code: `BriefStore.load()` is exactly where +CQ-007 says it is, `BriefAdapter.load()` is exactly the read `runResult` call RB-17 pointed +at it, and nothing about either file was factually wrong. Nothing to flag here. + +## What changed + +| File | Change | +| -------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `apps/ssp/src/app/brief/infrastructure/brief.adapter.ts` | `load()`'s error channel becomes `BriefLoadFailure` (`{tag:'notFound'} \| {tag:'error', reason:string}`) instead of a plain `string`. `load()` no longer routes through the shared `runResult` — it does its own try/catch so it can read the thrown value's HTTP `status` before folding it away, via the new local `isHttpNotFound` predicate. Every other method (`save`/`submit`/`approve`/`reject`/`send`/`reset`) is untouched, still on `runSubmit`. | +| `apps/ssp/src/app/brief/application/brief.store.ts` | `load()` branches on `BriefLoadFailure`: `notFound` (and not already recovered) calls the existing `reset()` command directly and applies its result; every other failure (including a repeated `notFound`) dispatches `BriefLoadFailed` as before. Extracted `applyLoadedView` (the success-path body shared by `load()` and the new recovery path) and added `recoverFromMissingBrief`. | +| `apps/ssp/src/app/brief/application/brief.store.spec.ts` | New `describe('BriefStore.load — 404 tolerance (RB-22)')` with the two required cases. Six pre-existing `load:` fakes' explicit `Result` return-type annotations updated to `Result` (they only ever produce the `ok: true` branch, so this is a type-only change); two shared `ok(v)` test helpers that build fakes for both `load` and `save` had their return-type annotation dropped in favour of `as const` inference, since one helper now serves two different error-channel types. | +| `libs/shared/docs/behaviour-spec.mdx` | Regenerated (`npm run gen:behaviour-spec`) — picks up the two new `it()` titles. | + +No `backend/` file was touched — `Program.cs` and `Data/BriefStore.cs` are RB-23's, per the +ticket's explicit scope. + +## How the once-only bound is structural + +`BriefStore` gains one field: `private hasRecoveredFromMissingBrief = false`. `load()` +takes the recovery branch only when `r.error.tag === 'notFound' && !this.hasRecoveredFromMissingBrief`, +and the branch's first statement sets the flag before doing anything else. A second 404 — +whether from a second `load()` call, or in principle from `reset()` itself somehow also +404ing — falls through to the plain `BriefLoadFailed` branch instead, on every subsequent +call, for the life of the store instance. This is a field on the singleton store, not a +comment: nothing in the reachable call graph can flip it back to `false`. + +The loop CQ-007's proposed change warns about ("the reset's own load must not be able to +loop") is not merely bounded, it is **structurally absent**: `recoverFromMissingBrief` +calls `this.adapter.reset()` and applies its `BriefView` result directly (the same +`applyLoadedView` the success path uses) — it never calls `this.load()` again. There is no +recursive edge from the recovery path back into `load()` for the once-only flag to have to +stop; the flag exists only to stop a **second, separate** `load()` invocation (e.g. a +caller retrying navigation) from reaching `reset()` again. + +## Judgement calls + +- **`load()` no longer uses `runResult`, only for this one method.** `runResult` + (`libs/shared/src/application/submit.ts`, RB-17) intentionally keeps only a string — + every other read in the app is fine with that. This is the first read that needs one + more bit (the HTTP status) than `runResult` exposes, so `load()` does its own + try/catch instead, matching `runResult`'s shape (`problemDetail(e, fallback)` on the + non-404 path) but adding the 404 branch first. `libs/shared/src/application/submit.ts` + itself is untouched — changing a shared helper used by many call sites for one adapter's + need was out of scope and unjustified. +- **404 detection reads `(e as {status?:unknown}).status === 404`, not + `SwaggerException.isSwaggerException`.** The generated client throws a plain + `SwaggerException` for `GET /brief` today (no OpenAPI 404 response is declared for it + yet), but throws the parsed `ProblemDetails` object instead for an endpoint whose spec + **does** declare a 404 (both shapes carry a `status` field). Checking `status` alone, + not the `SwaggerException` type, means this predicate keeps working unchanged once + RB-23 regenerates the client with a documented 404 response for `briefGET()` — no + follow-up FE edit needed for detection to keep working. +- **`BriefLoadFailure` is a new exported type, not a sentinel string.** CLAUDE.md's + default reflex is a discriminated union over a second flag; a magic string + (`'__not_found__'`) compared by identity would have kept `load()`'s signature at + `Result` and touched fewer test lines, but it is exactly the kind + of stringly-typed control flow the union tool exists to avoid. The touched-test cost + was six type annotations plus two helper signatures, all in the one already-scoped + spec file — judged worth it for the correct shape. + `BriefLoadFailure` is exported. +- **On a repeated 404, the store shows `BRIEF_LOAD_FAILED`** (the same generic banner + text `load()` already used for every other failure), not a distinct "still missing" + message. No new user-facing copy was needed or added, so no new `$localize` id and no + `messages.en.xlf` change — confirmed by diffing for `$localize` occurrences: both + hits in the diff are unchanged context lines, not new additions. +- **`resetDemo()` (the "start over" button) was left untouched**, even though it + duplicates part of the same apply-a-fresh-view logic now factored into + `applyLoadedView`. It also manages `actionState`/`saveState`/`rejectionSnapshot` that + `recoverFromMissingBrief` correctly does not touch (an automatic recovery on first + load is not a user-initiated "start over" action), and refactoring it was not asked + for by this ticket. + +## Verification + +- **Verified red without the fix.** Temporarily replaced `load()`'s body (via `Edit`, + not `git checkout`) with the pre-fix shape — every failure, `notFound` included, + dispatches `BriefLoadFailed` straight away, no `reset()` call — and reran the spec + file. Both new tests failed: + `expected "vi.fn()" to be called 1 times, but got 0 times` on `reset`, for both "a 404 + drives exactly one reset()" and "a second 404 does not drive a second reset()"; the + other 18 tests in the file stayed green. Restored the real fix with a second `Edit` + and reran: all 20 tests in the file green, 30/30 across both touched spec files. +- `npm run ci` (foreground, no background/Monitor): **green**, exit 0 — lint, + typecheck, `dep:check` (341 + 226 modules, 0 violations), `format:check`, + `check:tokens`, `check:seam`, tests (ssp 260/260, behandelportal 37/37, shared + 138/138, beheer 23/23 — 458 total), `ng build --localize` (both apps), `npm audit` + (0 vulnerabilities), backend `dotnet test` (260/260 — the known + `OpenZaakIntegrationTests.Admin_cases_…` container-dependent failure did not + reproduce on this run, matching the standing caveat that it needs a live OpenZaak + container and is not this ticket's bug), backend dependency audit clean, `gen:snippets` + drift clean, `gen:behaviour-spec` drift clean once the regenerated file was staged (the + local gate diffs the working tree against the index, so it necessarily shows a diff + before the file is staged/committed — the same documented, expected behaviour RB-17 + recorded, not a defect). +- `npx prettier --check` on every touched file (including the reformatted + `99-backlog.md` table and the regenerated `behaviour-spec.mdx`): clean. + +## What RB-23 must do + +Once `GET /brief` in `Program.cs` returns a real 404 (no `ProblemDetails` body is +required — `BriefAdapter.load()`'s `isHttpNotFound` only reads the HTTP `status`, not the +response body), and `BriefStore.GetOrCreate` splits into `Get` (query) + the existing +`ResetAndCreate` (already there, already used by `POST /brief/reset`), this ticket's +`notFound` branch stops being dead code and starts running on first-ever page load for any +owner with no persisted brief. Run `npm run gen:api` as part of RB-23 so `briefGET()` +regenerates with a documented `status === 404` branch throwing the parsed `ProblemDetails` +(matching the shape most other endpoints already use) — the detection predicate here +already tolerates that shape and needs no FE follow-up change. Two things worth +re-verifying once RB-23 lands, not fixing preemptively here: first, that the resulting +double round-trip (404, then `reset()`) is an acceptable UX cost on a first visit, per +CQ-007's own framing of this as its least certain finding; second, that this store's +`hasRecoveredFromMissingBrief` field — private to one store instance, reset only by a full +reload — is still the right lifetime for the once-only guard once a real 404 can occur in +production traffic, not only in a test's fake adapter. diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index a334174..230d599 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. 451 frontend behaviours across +**is** the suite, reshaped for a business reader. 453 frontend behaviours across 9 contexts; 236 backend behaviours across 41 test classes. @@ -180,6 +180,11 @@ classes. - flushes a pending debounced edit immediately and clears the pending flag - is a no-op when no edit is pending +#### BriefStore.load — 404 tolerance (RB-22) + +- a 404 drives exactly one reset(), which populates the store +- a second 404 does not drive a second reset() + #### BriefStore.previewLetter - opens the composed letter in a new tab on success