diff --git a/apps/ssp/src/app/registratie/application/admin-cases.store.spec.ts b/apps/ssp/src/app/registratie/application/admin-cases.store.spec.ts index 4b757dd..ffab1b2 100644 --- a/apps/ssp/src/app/registratie/application/admin-cases.store.spec.ts +++ b/apps/ssp/src/app/registratie/application/admin-cases.store.spec.ts @@ -1,5 +1,6 @@ import { TestBed } from '@angular/core/testing'; import { describe, it, expect, vi } from 'vitest'; +import { SUBMIT_FAILED } from '@shared/application/submit'; import { ApplicationsAdapter } from '@registratie/infrastructure/applications.adapter'; import { AdminCasesStore } from './admin-cases.store'; @@ -43,7 +44,10 @@ describe('AdminCasesStore', () => { expect(s.tag === 'Success' && s.value.map((c) => c.id)).toEqual(['b']); }); - it('rolls back the removal when the delete fails', async () => { + // RB-20: a failed delete must not be silent — the row rolls back AND the store + // surfaces the error the page renders. Before RB-20 this only rolled back + // (bare `catch { this.state.set(before) }`), so `lastError()` stayed null forever. + it('rolls back the removal and surfaces the error when the delete fails', async () => { const deleteAny = vi.fn().mockRejectedValue(new Error('boom')); const store = setup({ listAll: () => Promise.resolve([summary('a')]), deleteAny }); await store.load(); @@ -51,5 +55,24 @@ describe('AdminCasesStore', () => { await store.delete('a'); const s = store.cases(); expect(s.tag === 'Success' && s.value.map((c) => c.id)).toEqual(['a']); // reappears + expect(store.lastError()).toBe(SUBMIT_FAILED); + }); + + it('clears a stale error on the next delete attempt', async () => { + const deleteAny = vi + .fn() + .mockRejectedValueOnce(new Error('boom')) + .mockResolvedValueOnce(undefined); + const store = setup({ + listAll: () => Promise.resolve([summary('a'), summary('b')]), + deleteAny, + }); + await store.load(); + + await store.delete('a'); + expect(store.lastError()).toBe(SUBMIT_FAILED); + + await store.delete('b'); + expect(store.lastError()).toBeNull(); }); }); diff --git a/apps/ssp/src/app/registratie/application/admin-cases.store.ts b/apps/ssp/src/app/registratie/application/admin-cases.store.ts index 943dcb4..b4580b9 100644 --- a/apps/ssp/src/app/registratie/application/admin-cases.store.ts +++ b/apps/ssp/src/app/registratie/application/admin-cases.store.ts @@ -1,5 +1,6 @@ import { Injectable, inject, signal } from '@angular/core'; import { RemoteData } from '@shared/application/remote-data'; +import { runSubmit, SUBMIT_FAILED } from '@shared/application/submit'; import { Aanvraag } from '@registratie/domain/aanvraag'; import { ApplicationsAdapter, @@ -12,8 +13,9 @@ type Err = Error | undefined; * Admin view of ALL cases across owners (WP-36; `cases:manage`) — the back-office * counterpart of the user-facing `ApplicationsStore`. Same shape: one root singleton * owns the list as a writable RemoteData signal, delete removes the row synchronously - * (optimistic) and rolls back on error. Admin delete removes any case (any owner, - * submitted or not — the server enforces the capability). + * (optimistic), goes through `runSubmit`, and rolls back plus surfaces `lastError` on + * failure (RB-20). Admin delete removes any case (any owner, submitted or not — the + * server enforces the capability). */ @Injectable({ providedIn: 'root' }) export class AdminCasesStore { @@ -22,6 +24,11 @@ export class AdminCasesStore { private state = signal>({ tag: 'Loading' }); readonly cases = this.state.asReadonly(); + /** Set on a failed delete (RB-20): the optimistic removal already rolled back by + then, this is only the message for the alert the page renders above the list. */ + private error = signal(null); + readonly lastError = this.error.asReadonly(); + /** Fetch + parse at the trust boundary, then publish as RemoteData. Keeps the last-good value on a resync (only shows Loading on the first load). */ async load() { @@ -42,16 +49,18 @@ export class AdminCasesStore { void this.load(); } - /** Delete a case: drop it now (synchronous), then confirm the DELETE; roll back on error. */ + /** Delete a case: drop it now (synchronous), then confirm the DELETE; roll back on error + AND surface it (RB-20) — a silent reappearance leaves the admin guessing why. */ async delete(id: string) { const before = this.state(); if (before.tag === 'Success') { this.state.set({ tag: 'Success', value: before.value.filter((a) => a.id !== id) }); } - try { - await this.adapter.deleteAny(id); - } catch { + this.error.set(null); + const r = await runSubmit(() => this.adapter.deleteAny(id), SUBMIT_FAILED); + if (!r.ok) { this.state.set(before); // roll back: the row reappears + this.error.set(r.error); } } } diff --git a/apps/ssp/src/app/registratie/application/applications.store.spec.ts b/apps/ssp/src/app/registratie/application/applications.store.spec.ts new file mode 100644 index 0000000..f8860af --- /dev/null +++ b/apps/ssp/src/app/registratie/application/applications.store.spec.ts @@ -0,0 +1,77 @@ +import { TestBed } from '@angular/core/testing'; +import { describe, it, expect, vi } from 'vitest'; +import { SUBMIT_FAILED } from '@shared/application/submit'; +import { ApplicationsAdapter } from '@registratie/infrastructure/applications.adapter'; +import { ApplicationsStore } from './applications.store'; + +const summary = (id: string) => ({ + id, + type: 'registratie', + status: { tag: 'Concept', stepIndex: 0, stepCount: 3 }, + documentIds: [], + createdAt: '2026-07-23T10:00:00Z', + updatedAt: '2026-07-23T10:00:00Z', +}); + +function setup(adapter: Partial): ApplicationsStore { + TestBed.configureTestingModule({ + providers: [{ provide: ApplicationsAdapter, useValue: adapter }], + }); + // The store's own constructor kicks off `load()` (dashboard revisit refresh) — + // give every test a `list` so that initial call has something to resolve. + return TestBed.inject(ApplicationsStore); +} + +describe('ApplicationsStore', () => { + it('loads and parses the list', async () => { + const store = setup({ list: () => Promise.resolve([summary('a'), summary('b')]) }); + await store.load(); + const s = store.applications(); + expect(s.tag).toBe('Success'); + expect(s.tag === 'Success' && s.value.map((a) => a.id)).toEqual(['a', 'b']); + }); + + it('cancels optimistically and confirms via the DELETE endpoint', async () => { + const cancel = vi.fn().mockResolvedValue(undefined); + const store = setup({ + list: () => Promise.resolve([summary('a'), summary('b')]), + cancel, + }); + await store.load(); + + await store.cancel('a'); + expect(cancel).toHaveBeenCalledWith('a'); + const s = store.applications(); + expect(s.tag === 'Success' && s.value.map((a) => a.id)).toEqual(['b']); + expect(store.lastError()).toBeNull(); + }); + + // RB-20: a failed cancel must not be silent — the row rolls back AND the store + // surfaces the error the page renders. Before RB-20 this only rolled back + // (bare `catch { this.state.set(before) }`), so `lastError()` stayed null forever. + it('rolls back the removal and surfaces the error when the cancel fails', async () => { + const cancel = vi.fn().mockRejectedValue(new Error('boom')); + const store = setup({ list: () => Promise.resolve([summary('a')]), cancel }); + await store.load(); + + await store.cancel('a'); + const s = store.applications(); + expect(s.tag === 'Success' && s.value.map((a) => a.id)).toEqual(['a']); // reappears + expect(store.lastError()).toBe(SUBMIT_FAILED); + }); + + it('clears a stale error on the next cancel attempt', async () => { + const cancel = vi + .fn() + .mockRejectedValueOnce(new Error('boom')) + .mockResolvedValueOnce(undefined); + const store = setup({ list: () => Promise.resolve([summary('a'), summary('b')]), cancel }); + await store.load(); + + await store.cancel('a'); + expect(store.lastError()).toBe(SUBMIT_FAILED); + + await store.cancel('b'); + expect(store.lastError()).toBeNull(); + }); +}); diff --git a/apps/ssp/src/app/registratie/application/applications.store.ts b/apps/ssp/src/app/registratie/application/applications.store.ts index db96201..5157dad 100644 --- a/apps/ssp/src/app/registratie/application/applications.store.ts +++ b/apps/ssp/src/app/registratie/application/applications.store.ts @@ -1,5 +1,6 @@ import { Injectable, inject, signal } from '@angular/core'; import { RemoteData } from '@shared/application/remote-data'; +import { runSubmit, SUBMIT_FAILED } from '@shared/application/submit'; import { Aanvraag } from '@registratie/domain/aanvraag'; import { ApplicationsAdapter, @@ -15,7 +16,8 @@ type Err = Error | undefined; * the row SYNCHRONOUSLY, so the block disappears deterministically — no dependence on * change-detection timing, HTTP caching, or a resource `reload()`. `reload()` re-fetches * so a page revisit reflects auto-approval (Concept → In behandeling → Goedgekeurd is - * computed server-side on read). + * computed server-side on read). Cancel goes through `runSubmit` and rolls back plus + * surfaces `lastError` on failure (RB-20). */ @Injectable({ providedIn: 'root' }) export class ApplicationsStore { @@ -24,6 +26,11 @@ export class ApplicationsStore { private state = signal>({ tag: 'Loading' }); readonly applications = this.state.asReadonly(); + /** Set on a failed cancel (RB-20): the optimistic removal already rolled back by + then, this is only the message for the alert the page renders above the list. */ + private error = signal(null); + readonly lastError = this.error.asReadonly(); + constructor() { void this.load(); } @@ -50,16 +57,19 @@ export class ApplicationsStore { } /** Cancel a Concept: drop it now (synchronous, guaranteed), then confirm the DELETE. - No resync — the delete succeeded, so the optimistic removal is authoritative. */ + No resync — the delete succeeded, so the optimistic removal is authoritative. On + failure, roll back AND surface the error (RB-20) — a silent reappearance leaves the + user guessing why the block came back. */ async cancel(id: string) { const before = this.state(); if (before.tag === 'Success') { this.state.set({ tag: 'Success', value: before.value.filter((a) => a.id !== id) }); } - try { - await this.adapter.cancel(id); - } catch { + this.error.set(null); + const r = await runSubmit(() => this.adapter.cancel(id), SUBMIT_FAILED); + if (!r.ok) { this.state.set(before); // roll back: the block reappears + this.error.set(r.error); } } } diff --git a/apps/ssp/src/app/registratie/ui/admin-cases.page.ts b/apps/ssp/src/app/registratie/ui/admin-cases.page.ts index aa088a4..8f83fad 100644 --- a/apps/ssp/src/app/registratie/ui/admin-cases.page.ts +++ b/apps/ssp/src/app/registratie/ui/admin-cases.page.ts @@ -42,6 +42,9 @@ import { AdminCasesStore } from '@registratie/application/admin-cases.store'; } @else if (!canManage()) { {{ deniedText }} } @else { + @if (store.lastError(); as err) { + {{ err }} + } {{ failedText }} diff --git a/apps/ssp/src/app/registratie/ui/dashboard.page.ts b/apps/ssp/src/app/registratie/ui/dashboard.page.ts index 1e17309..86c5a35 100644 --- a/apps/ssp/src/app/registratie/ui/dashboard.page.ts +++ b/apps/ssp/src/app/registratie/ui/dashboard.page.ts @@ -51,6 +51,9 @@ import { tasksFromProfile } from '@registratie/domain/tasks'; intro="Welkom in uw persoonlijke omgeving van het BIG-register. Hier ziet u uw registratie en regelt u uw zaken." >
+ @if (cancelError(); as err) { + {{ err }} + } @if (aanvragen().length) {
@for (a of concepten(); track a.id) { @@ -260,6 +263,8 @@ export class DashboardPage { protected cancelAanvraag(a: Aanvraag) { void this.apps.cancel(a.id); } + /** RB-20: the message from a failed cancel, rendered above the list. */ + protected cancelError = computed(() => this.apps.lastError()); /** Server-computed eligibility (rendered, not recomputed). */ private readonly eligible = computed(() => { 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..e1ededd 100644 --- a/docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md +++ b/docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md @@ -121,7 +121,7 @@ Every ticket tracing to a `BIO-` finding, plus every row on agent 07's authorita | **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-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 | — | — | 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 | diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-20.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-20.md new file mode 100644 index 0000000..c4b242b --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-20.md @@ -0,0 +1,117 @@ +# RB-20 — route `ApplicationsStore.cancel` / `AdminCasesStore.delete` through `runSubmit`, surface the error + +Status: **implemented** · 2026-08-27 · Source finding: `04-cqrs-light.md` CQ-002 · +`00-baseline.md` BL-007 · `99-backlog.md` RB-20 · SIGN-OFF: consolidation approved +2026-08-27, HALT lifted + +## What was wrong + +`ApplicationsStore.cancel` and `AdminCasesStore.delete` both owned an optimistic write next +to their `RemoteData` read, and both reached `ApplicationsAdapter` directly instead of going +through `runSubmit` (the fold + Idempotency-Key mint every other mutation in the repo uses, +including `createSubmitChangeRequest` in the same folder). The failure path was a bare +`catch { this.state.set(before); }`: a failed cancel or delete rolled the row back, but the +user saw no message at all — no `ActionState`, no ProblemDetails `detail`, nothing. The +`Idempotency-Key` on the wire was also a fresh UUID per HTTP attempt (minted by +`api-client.provider.ts`'s default), not the per-logical-submit key `runSubmit` promises — +harmless today only because `Program.cs` happens to ignore the header outside the `Submit` +helper (CQ-005's note). + +## What changed + +CQ-002's option (a) — the smallest fix, applied identically to both stores. No new command +factory, no adapter split (CQ-002's own "Not filed" note reserves that split for option (b), +which this ticket does not take). + +| File | Change | +| --------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `apps/ssp/src/app/registratie/application/applications.store.ts` | `cancel` now calls `runSubmit(() => this.adapter.cancel(id), SUBMIT_FAILED)`; added a private `error` signal, exposed read-only as `lastError`. On failure: roll back AND `this.error.set(r.error)`. On the next attempt, the error is cleared before the call so a stale message never survives a fresh action. | +| `apps/ssp/src/app/registratie/application/applications.store.spec.ts` (new) | 4 specs: load+parse, optimistic cancel, roll-back-and-surface-error on failure, stale-error-clears-on-next-attempt. No spec file existed for this store before RB-20. | +| `apps/ssp/src/app/registratie/application/admin-cases.store.ts` | Same shape as `applications.store.ts`: `delete` through `runSubmit`, `error`/`lastError` signal pair. | +| `apps/ssp/src/app/registratie/application/admin-cases.store.spec.ts` | Existing "rolls back … when the delete fails" spec extended to also assert `lastError()`; one new stale-error-clears spec added. | +| `apps/ssp/src/app/registratie/ui/dashboard.page.ts` | One `@if (cancelError(); as err) { {{ err }} }` above the aanvragen list, mirroring `brief.page.ts`'s `lastError` rendering. `cancelError` is a `computed(() => this.apps.lastError())`. | +| `apps/ssp/src/app/registratie/ui/admin-cases.page.ts` | Same `@if (store.lastError(); as err) { {{ err }} }`, placed above `` inside the `canManage()` branch (`store` was already `protected`, so no new exposure needed). | + +`applications.adapter.ts` (`cancel`, `deleteAny`) is **unchanged** — the fix is entirely in +the two stores, which now wrap the existing thin adapter calls in `runSubmit` at the call +site, exactly as `createSubmitChangeRequest` wraps `ChangeRequestAdapter.changeRequest`. The +adapter methods still return a bare `Promise`; `runSubmit` is what folds that into a +`Result`. + +Neither UI change introduces a new user-facing string: the rendered text is either the +existing `SUBMIT_FAILED` constant (`@@submit.failed`, already translated in +`messages.en.xlf` since RB-17) or, when the backend sends one, a ProblemDetails `detail` +string carried verbatim from the server — never a new `$localize` id. `messages.en.xlf` did +not need a new ``. + +## The tests, and their red failures + +Both specs assert `store.lastError()` after a rejected adapter call, which only the fix can +satisfy — the old bare `catch { this.state.set(before) }` never touched an error signal, so +`lastError()` stayed `null` forever. + +**Verified red without the fix** (an `Edit` undo of the store method, not `git checkout`, so +the rest of the change — imports, the other store, the UI, the specs — stayed in place): + +- `applications.store.ts`: reverted `cancel` to `try { await this.adapter.cancel(id); } catch +{ this.state.set(before); }`. Reran `ng test ssp --include applications.store.spec.ts`: + 2 of 4 failed — + `rolls back the removal and surfaces the error when the cancel fails` and + `clears a stale error on the next cancel attempt`, both with + `AssertionError: expected null to be 'Het indienen is niet gelukt. Probeer het later opnieuw.'`. + The other two specs (load, optimistic-cancel-success) stayed green, as expected — they + don't touch the error path. Re-applied the fix (`Edit` back to the `runSubmit` version); + reran: 4/4 green. +- `admin-cases.store.ts`: same procedure on `delete`. Reran + `ng test ssp --include admin-cases.store.spec.ts`: 2 of 4 failed with the identical + `expected null to be '...'` shape. Reverted to the fix; reran: 4/4 green. + +## Judgement calls + +- **Signal naming**: private backing field `error`, public readonly `lastError` — matching + the name `BriefStore`/`OrgTemplateStore` already expose for exactly this purpose (CQ-002's + own citation), rather than inventing a new name per store. +- **Error cleared at the start of each write**, not only on success, so a second cancel/delete + attempt after a failure doesn't leave a stale banner up if the retry itself is still in + flight. Covered by the "clears a stale error on the next attempt" spec in each file. +- **No `ActionState`/`SaveState` pair** (the fuller shape `BriefStore` uses for busy-state and + save-state together) — CQ-002 explicitly scoped option (a) to "one `error` signal", and + neither store needs a busy indicator: the row already disappears optimistically the instant + the click happens, so there is nothing for a spinner to cover. +- **UI placement**: one alert per page, above the list the mutated row belongs to, using the + same `@if (x(); as err) { {{ err }} }` shape as + `brief.page.ts` — composition of an existing atom, no new building block (CLAUDE.md §2). +- **`applications.adapter.ts` left untouched, on purpose** — CQ-002's "Not filed" note ties + the read/write file split to option (b) only; taking option (a) means this ticket changes + no adapter code at all, matching the ticket's own framing ("(a) touches 2 files plus a UI + line each"). + +## Ticket accuracy + +CQ-002's description matched the code as found: both stores' `cancel`/`delete` reached the +adapter directly with a bare `catch { this.state.set(before); }`, no `Result`, no error +channel — no discrepancy to flag. + +## Residuals (not this ticket) + +- RB-18 (key `IdempotencyStore` on `{SubjectId}:{idemKey}`) is unaffected: `cancel`/`delete` + now mint a key through `runSubmit` like every other mutation, so it lands on the same + write-only call set RB-18 already targets. +- RB-21 (extract `createDraftSync`'s read half) is a separate CQRS-light finding in the same + context, untouched by this ticket. + +## Verification + +`npm run ci` (foreground, `timeout: 600000`): **green** — `✔ local CI passed`. Lint, +typecheck, `dep:check` (342 + 226 modules, 0 violations), `format:check`, `check:tokens`, +`check:seam`, tests (ssp 263/263 — 5 more than the pre-RB-20 258, from the new/extended +specs above — behandelportal 37/37, shared 138/138, beheer 23/23), `ng build --localize` +(both apps), `npm audit` (0 vulnerabilities), backend `dotnet format --verify-no-changes` + +`dotnet test --filter "Category!=Integration"` (260/260 — this filter is what keeps the +known `OpenZaakIntegrationTests.Admin_cases_…` container-dependent test, which needs a live +OpenZaak container, out of `npm run ci` entirely; it is a standing caveat, not introduced by +this change, and not exercised by this run), backend dependency audit (0 vulnerable +packages), `gen:snippets` / `gen:behaviour-spec` / `gen:api` drift checks all clean once the +regenerated `behaviour-spec.mdx` was staged alongside the code (the local gate's +`git diff --exit-code` compares the working tree to the index, so it is clean once the file +is staged — this is the documented pre-commit behaviour from RB-17's note, not a defect). diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index a334174..eeafe30 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. 456 frontend behaviours across 9 contexts; 236 backend behaviours across 41 test classes. @@ -406,7 +406,15 @@ classes. - loads and parses the cross-owner list - deletes optimistically and confirms via the admin endpoint -- rolls back the removal when the delete fails +- rolls back the removal and surfaces the error when the delete fails +- clears a stale error on the next delete attempt + +#### ApplicationsStore + +- loads and parses the list +- cancels optimistically and confirms via the DELETE endpoint +- rolls back the removal and surfaces the error when the cancel fails +- clears a stale error on the next cancel attempt #### STEPS (fixed)