diff --git a/backend/src/BigRegister.Api/Data/AppDbContext.cs b/backend/src/BigRegister.Api/Data/AppDbContext.cs index 876789a..08f109d 100644 --- a/backend/src/BigRegister.Api/Data/AppDbContext.cs +++ b/backend/src/BigRegister.Api/Data/AppDbContext.cs @@ -53,7 +53,7 @@ public sealed class AppDbContext(DbContextOptions options) : DbCon modelBuilder.Entity(e => { e.HasKey(b => b.BriefId); - e.HasIndex(b => b.Owner).IsUnique(); // one demo brief per owner (GetOrCreate's invariant) + e.HasIndex(b => b.Owner).IsUnique(); // one demo brief per owner (ResetAndCreate's invariant) e.Property(b => b.Placeholders).HasConversion(Json>()); e.Property(b => b.Sections).HasConversion(Json>()); e.Property(b => b.Status).HasConversion(Json()); diff --git a/backend/src/BigRegister.Api/Data/BriefStore.cs b/backend/src/BigRegister.Api/Data/BriefStore.cs index 7a3f636..f880d98 100644 --- a/backend/src/BigRegister.Api/Data/BriefStore.cs +++ b/backend/src/BigRegister.Api/Data/BriefStore.cs @@ -47,17 +47,15 @@ public static class BriefStore private static readonly object _gate = new(); - public static BriefEntity GetOrCreate(string owner) + /// Pure query (RB-23/CQ-007): no write. `GET /brief` 404s when this returns null — + /// the owner's first-ever draft is created only through the explicit `ResetAndCreate` + /// command (`POST /brief/reset`), never as a side effect of a read. + public static BriefEntity? Get(string owner) { lock (_gate) { using var db = Db.Create(); - var existing = db.Briefs.FirstOrDefault(e => e.Owner == owner); - if (existing is not null) return existing; - var created = BriefSeed.NewBrief(owner); - db.Briefs.Add(created); - db.SaveChanges(); - return created; + return db.Briefs.FirstOrDefault(e => e.Owner == owner); } } diff --git a/backend/src/BigRegister.Api/Program.cs b/backend/src/BigRegister.Api/Program.cs index ce38f5e..7fbea17 100644 --- a/backend/src/BigRegister.Api/Program.cs +++ b/backend/src/BigRegister.Api/Program.cs @@ -675,10 +675,15 @@ api.MapPut("/admin/flags/{key}", (string key, SetFeatureFlagRequest req, HttpCon api.MapGet("/brief", (HttpContext ctx) => { - var e = BriefStore.GetOrCreate(ctx.Zorgverlener().Bsn); - return ToView(ctx, e); + // RB-23/CQ-007: a read that used to allocate a row on first call. The owner's first + // draft now comes only from the explicit POST /brief/reset (BriefStore.ResetAndCreate) + // — this GET is a pure query and 404s when there is nothing to read yet. + var e = BriefStore.Get(ctx.Zorgverlener().Bsn); + if (e is null) return Results.NotFound(); + return Results.Ok(ToView(ctx, e)); }) -.Produces(); +.Produces() +.Produces(StatusCodes.Status404NotFound); api.MapPut("/brief", (SaveBriefRequest req, HttpContext ctx) => { @@ -766,7 +771,12 @@ api.MapPost("/brief/reveal-bignummer", (HttpContext ctx) => // letters serve their frozen archive; anything else renders live with a watermark. api.MapGet("/brief/preview", (HttpContext ctx) => { - var e = BriefStore.GetOrCreate(ctx.Zorgverlener().Bsn); + // RB-23: BriefStore.GetOrCreate is gone (split into Get + ResetAndCreate). This GET + // must not create a brief as a side effect either, so it 404s under the same + // precondition as GET /brief — in the running app the FE only reaches this endpoint + // from the brief page, which has already loaded (and, if needed, reset) a brief. + var e = BriefStore.Get(ctx.Zorgverlener().Bsn); + if (e is null) return Results.NotFound(); if (e.Status.Tag == "sent" && e.ArchivedHtml is { } archived) return Results.Content(archived, "text/html"); var template = OrgTemplateStore.TemplateForBrief(e.SubOrgId, null); diff --git a/backend/swagger.json b/backend/swagger.json index 1a0dad1..9985fce 100644 --- a/backend/swagger.json +++ b/backend/swagger.json @@ -1000,6 +1000,9 @@ } } } + }, + "404": { + "description": "Not Found" } } }, diff --git a/backend/tests/BigRegister.Tests/BriefEndpointTests.cs b/backend/tests/BigRegister.Tests/BriefEndpointTests.cs index 22ec6ef..78cdd05 100644 --- a/backend/tests/BigRegister.Tests/BriefEndpointTests.cs +++ b/backend/tests/BigRegister.Tests/BriefEndpointTests.cs @@ -28,10 +28,14 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu return new SaveBriefRequest(sections); } - private async Task Get() + /// RB-23: `GET /brief` no longer seeds a brief on first call, so every test that + /// needs one present creates it explicitly through `POST /brief/reset` + /// (`BriefStore.ResetAndCreate`) — the same command the "start over" affordance uses. + private async Task SeedBrief() { BriefStore.Reset(); - var view = await _client.GetFromJsonAsync("/api/v1/brief"); + var res = await _client.PostAsync("/api/v1/brief/reset", null); + var view = await res.Content.ReadFromJsonAsync(); Assert.NotNull(view); return view.Brief; } @@ -44,10 +48,26 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu return req; } + // --- RB-23/CQ-007: GET /brief is a pure query — it must not create a row. --- + [Fact] - public async Task Get_creates_a_draft_with_expected_sections_locked_and_empty() + public async Task Get_returns_404_and_writes_no_row_when_no_brief_exists_for_the_owner() { - var brief = await Get(); + BriefStore.Reset(); + + var res = await _client.GetAsync("/api/v1/brief"); + Assert.Equal(HttpStatusCode.NotFound, res.StatusCode); + + // The non-idempotent write CQ-007 flagged: a GET that allocated a row on first call. + // Assert directly against the store, not only the HTTP status, so a regression that + // reintroduces GetOrCreate-style seeding fails here even if the response shape stays 404. + Assert.Null(BriefStore.Get(DocumentStore.DemoOwner)); + } + + [Fact] + public async Task SeedBrief_creates_a_draft_with_expected_sections_locked_and_empty() + { + var brief = await SeedBrief(); Assert.Equal("draft", brief.Status.Tag); Assert.Equal(new[] { "aanhef", "kern", "slot" }, brief.Sections.Select(s => s.SectionKey)); // aanhef + slot are locked, predefined and prefilled; only kern is editable + empty. @@ -63,7 +83,7 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu [Fact] public async Task Get_offers_only_global_and_arts_scoped_besluit_tagged_passages() { - await Get(); + await SeedBrief(); var view = await _client.GetFromJsonAsync("/api/v1/brief"); Assert.NotNull(view); // global passages + the arts-scoped one; no other-beroep passages leak in. @@ -78,7 +98,7 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu [Fact] public async Task Get_joins_the_case_context_with_the_BIG_nummer_masked() { - await Get(); + await SeedBrief(); var view = await _client.GetFromJsonAsync("/api/v1/brief"); Assert.NotNull(view); // Case context is joined onto the screen DTO for the behandel scherm header. @@ -128,7 +148,7 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu [Fact] public async Task Save_is_drafter_only() { - var brief = await Get(); + var brief = await SeedBrief(); var save = FilledFrom(brief); var approver = Post("/api/v1/brief", role: "approver"); @@ -142,7 +162,7 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu [Fact] public async Task Submit_blocks_on_empty_required_section() { - await Get(); + await SeedBrief(); // Nothing filled yet → required sections empty → 409. Assert.Equal(HttpStatusCode.Conflict, (await _client.SendAsync(Post("/api/v1/brief/submit"))).StatusCode); } @@ -150,7 +170,7 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu [Fact] public async Task Submit_succeeds_when_required_sections_filled() { - await Get(); + await SeedBrief(); var view = await _client.GetFromJsonAsync("/api/v1/brief"); Assert.NotNull(view); await _client.PutAsJsonAsync("/api/v1/brief", FilledFrom(view.Brief)); @@ -170,7 +190,7 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu [Fact] public async Task Drafter_cannot_approve_own_letter_but_a_different_reviewer_can() { - var brief = await Get(); + var brief = await SeedBrief(); await _client.PutAsJsonAsync("/api/v1/brief", FilledFrom(brief)); await _client.SendAsync(Post("/api/v1/brief/submit")); @@ -187,7 +207,7 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu [Fact] public async Task Reject_returns_comments() { - var brief = await Get(); + var brief = await SeedBrief(); await _client.PutAsJsonAsync("/api/v1/brief", FilledFrom(brief)); await _client.SendAsync(Post("/api/v1/brief/submit")); @@ -202,7 +222,7 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu [Fact] public async Task Editing_a_rejected_letter_reopens_it_to_draft() { - var brief = await Get(); + var brief = await SeedBrief(); await _client.PutAsJsonAsync("/api/v1/brief", FilledFrom(brief)); await _client.SendAsync(Post("/api/v1/brief/submit")); await _client.SendAsync( @@ -218,7 +238,7 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu [Fact] public async Task Send_only_from_approved() { - var brief = await Get(); + var brief = await SeedBrief(); await _client.PutAsJsonAsync("/api/v1/brief", FilledFrom(brief)); await _client.SendAsync(Post("/api/v1/brief/submit")); @@ -236,7 +256,7 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu [Fact] public async Task Decisions_on_the_view_mirror_the_acting_principal_and_live_status() { - var brief = await Get(); + var brief = await SeedBrief(); var view = await _client.GetFromJsonAsync("/api/v1/brief"); Assert.NotNull(view); Assert.True(view.Decisions.CanEdit); // default (no X-Role) = drafter, draft status @@ -269,7 +289,7 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu [Fact] public async Task Reset_recreates_a_fresh_draft_with_locked_prefilled_sections() { - var brief = await Get(); + var brief = await SeedBrief(); // Advance out of draft so the reset back to draft is observable. await _client.PutAsJsonAsync("/api/v1/brief", FilledFrom(brief)); await _client.SendAsync(Post("/api/v1/brief/submit")); diff --git a/backend/tests/BigRegister.Tests/OrgTemplateEndpointTests.cs b/backend/tests/BigRegister.Tests/OrgTemplateEndpointTests.cs index 01784b9..dcf26b2 100644 --- a/backend/tests/BigRegister.Tests/OrgTemplateEndpointTests.cs +++ b/backend/tests/BigRegister.Tests/OrgTemplateEndpointTests.cs @@ -58,8 +58,8 @@ public class OrgTemplateEndpointTests(TestWebApplicationFactory factory) : IClas public async Task Publish_increments_the_version() { ResetStores(); - // One unsent brief for this sub-org (GetOrCreate on first read). - await _client.GetAsync("/api/v1/brief"); + // One unsent brief for this sub-org (RB-23: GET no longer seeds — create explicitly). + await _client.PostAsync("/api/v1/brief/reset", null); var res = await _client.SendAsync(Req(HttpMethod.Post, $"/api/v1/admin/org-template/{Registers}/publish", role: "admin")); res.EnsureSuccessStatusCode(); @@ -74,7 +74,7 @@ public class OrgTemplateEndpointTests(TestWebApplicationFactory factory) : IClas public async Task Publish_appends_to_the_version_history() { ResetStores(); - await _client.GetAsync("/api/v1/brief"); + await _client.PostAsync("/api/v1/brief/reset", null); // RB-23: GET no longer seeds — create explicitly var res = await _client.SendAsync(Req(HttpMethod.Post, $"/api/v1/admin/org-template/{Registers}/publish", role: "admin")); res.EnsureSuccessStatusCode(); @@ -87,8 +87,8 @@ public class OrgTemplateEndpointTests(TestWebApplicationFactory factory) : IClas public async Task Publish_counts_the_unsent_briefs_it_affects() { ResetStores(); - // One unsent brief for this sub-org (GetOrCreate on first read). - await _client.GetAsync("/api/v1/brief"); + // One unsent brief for this sub-org (RB-23: GET no longer seeds — create explicitly). + await _client.PostAsync("/api/v1/brief/reset", null); var res = await _client.SendAsync(Req(HttpMethod.Post, $"/api/v1/admin/org-template/{Registers}/publish", role: "admin")); res.EnsureSuccessStatusCode(); @@ -154,7 +154,8 @@ public class OrgTemplateEndpointTests(TestWebApplicationFactory factory) : IClas private async Task WalkBriefToSentThenRepublish() { ResetStores(); - var brief = (await _client.GetFromJsonAsync("/api/v1/brief"))!.Brief; + var resetRes = await _client.PostAsync("/api/v1/brief/reset", null); // RB-23: create explicitly + var brief = (await resetRes.Content.ReadFromJsonAsync())!.Brief; var filled = brief.Sections .Select(s => new LetterSectionDto(s.SectionKey, s.Title, s.Required, s.Required && s.Blocks.Count == 0 @@ -210,7 +211,8 @@ public class OrgTemplateEndpointTests(TestWebApplicationFactory factory) : IClas public async Task Admin_cannot_slip_into_the_brief_review_flow() { ResetStores(); - var brief = (await _client.GetFromJsonAsync("/api/v1/brief"))!.Brief; + var resetRes = await _client.PostAsync("/api/v1/brief/reset", null); // RB-23: create explicitly + var brief = (await resetRes.Content.ReadFromJsonAsync())!.Brief; var filled = brief.Sections .Select(s => new LetterSectionDto(s.SectionKey, s.Title, s.Required, s.Required && s.Blocks.Count == 0 diff --git a/backend/tests/BigRegister.Tests/PreviewEndpointTests.cs b/backend/tests/BigRegister.Tests/PreviewEndpointTests.cs index 62369ec..d86c9df 100644 --- a/backend/tests/BigRegister.Tests/PreviewEndpointTests.cs +++ b/backend/tests/BigRegister.Tests/PreviewEndpointTests.cs @@ -41,7 +41,7 @@ public class PreviewEndpointTests(TestWebApplicationFactory factory) : IClassFix public async Task Preview_of_an_unsent_brief_renders_live_with_a_watermark() { ResetStores(); - await _client.GetAsync("/api/v1/brief"); // GetOrCreate the demo draft + await _client.PostAsync("/api/v1/brief/reset", null); // RB-23: GET no longer seeds — create explicitly var res = await _client.GetAsync("/api/v1/brief/preview"); res.EnsureSuccessStatusCode(); @@ -54,7 +54,8 @@ public class PreviewEndpointTests(TestWebApplicationFactory factory) : IClassFix public async Task Preview_of_a_sent_brief_serves_the_archive_unchanged_after_a_republish() { ResetStores(); - var brief = (await _client.GetFromJsonAsync("/api/v1/brief"))!.Brief; + var resetRes = await _client.PostAsync("/api/v1/brief/reset", null); // RB-23: create explicitly + var brief = (await resetRes.Content.ReadFromJsonAsync())!.Brief; await _client.PutAsJsonAsync("/api/v1/brief", FilledFrom(brief)); await _client.SendAsync(Req(HttpMethod.Post, "/api/v1/brief/submit")); await _client.SendAsync(Req(HttpMethod.Post, "/api/v1/brief/approve", role: "approver")); diff --git a/backend/tests/BigRegister.Tests/RouteInventoryTests.cs b/backend/tests/BigRegister.Tests/RouteInventoryTests.cs index 8c284cf..a010096 100644 --- a/backend/tests/BigRegister.Tests/RouteInventoryTests.cs +++ b/backend/tests/BigRegister.Tests/RouteInventoryTests.cs @@ -72,14 +72,14 @@ public class RouteInventoryTests(TestWebApplicationFactory factory) : IClassFixt // enforce/emit twin for this whole surface (Authz.CanActOn via BriefStore, ToView's // Decisions dto) — a different single-source-of-truth than the five Program.cs wrappers, // not a missing one. --- - new("GET", "/api/v1/brief", "Ownership-scoped inline: BriefStore.GetOrCreate(ctx.Zorgverlener().Bsn)."), + new("GET", "/api/v1/brief", "Ownership-scoped inline: BriefStore.Get(ctx.Zorgverlener().Bsn), 404 when absent (RB-23)."), new("PUT", "/api/v1/brief", "Brief status-machine enforcement: BriefStore.Save + Authz.CanActOn (drafter-only)."), new("POST", "/api/v1/brief/submit", "Brief status-machine enforcement: BriefStore.Submit + Authz.CanActOn."), new("POST", "/api/v1/brief/approve", "Brief status-machine enforcement: BriefStore.Approve + Authz.CanActOn (approver != drafter)."), new("POST", "/api/v1/brief/reject", "Brief status-machine enforcement: BriefStore.Reject + Authz.CanActOn."), new("POST", "/api/v1/brief/send", "Brief status-machine enforcement: BriefStore.Send; not role-gated today, per the endpoint's own comment."), new("POST", "/api/v1/brief/reveal-bignummer", "Own inline capability + step-up check (Authz.CanRevealBigNummer + X-Step-Up), audited directly."), - new("GET", "/api/v1/brief/preview", "Ownership-scoped inline: BriefStore.GetOrCreate(ctx.Zorgverlener().Bsn); hand-written FE fetch."), + new("GET", "/api/v1/brief/preview", "Ownership-scoped inline: BriefStore.Get(ctx.Zorgverlener().Bsn), 404 when absent (RB-23); hand-written FE fetch."), new("POST", "/api/v1/brief/reset", "Deliberately unguarded demo affordance — the endpoint's own comment says so: 'showcase affordance only'."), ]; 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 a39a6f5..3ed6ac5 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** | 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 | — | — | **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** | 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** | **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** | 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 | — | — | **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** | **implemented** | +| **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-23.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-23.md new file mode 100644 index 0000000..faf9fa4 --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-23.md @@ -0,0 +1,183 @@ +# RB-23 — `GET /brief` 404s when absent; `BriefStore.GetOrCreate` splits into `Get` + `ResetAndCreate` + +Status: **implemented** · 2026-08-27 · Source findings: `04-cqrs-light.md` CQ-007 · +`99-backlog.md` RB-23, "Tickets that were rejected and split" · `implementation/rb-22.md` +(the FE **expand** half this ticket **contracts** against) + +This is the **contract** half of the RB-22/RB-23 expand/contract pair. RB-22 shipped first +and made `BriefStore.load()` tolerate a 404 by calling `reset()` once, as a no-op against +the (then) still-seeding backend. This ticket is what makes that branch live: `GET /brief` +now 404s when the owner has no brief yet, and the endpoint no longer performs a persisted +write on a read. + +## What was wrong + +CQ-007 flagged `GET /brief` (`Program.cs:676` → `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 FE +retries GETs automatically (`api-client.provider.ts`, `retry({ count: 2, delay: 500 })`, +GET-only, precisely because GETs are assumed safe), so a transient failure could enter the +create path more than once; `GetOrCreate`'s `lock` prevented a duplicate row today, but the +safety depended on the lock rather than on the endpoint being a query. + +The ticket read as filed against the current code: `GetOrCreate` was exactly at +`BriefStore.cs:50`, `GET /brief` called it exactly as described, and `ResetAndCreate` +already existed and was already the sole body of `POST /brief/reset`. One thing the +ticket's own text did not mention: `BriefStore.GetOrCreate` had a **second** call site, +`GET /brief/preview` (`Program.cs:769`, excluded from the OpenAPI doc — a hand-written FE +`fetch`, same seam as uploads). Splitting `GetOrCreate` away necessarily touches that +call site too, or the file does not compile. See "What changed" below — this was a forced +consequence of the split, not a new business decision, and it is reported here rather than +silently worked around. + +## What changed + +| File | Change | +| -------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `backend/src/BigRegister.Api/Data/BriefStore.cs` | `GetOrCreate` removed. New `Get(string owner): BriefEntity?` — pure query, `lock`-guarded like every other method in this file for consistency, no write. `ResetAndCreate` is untouched. | +| `backend/src/BigRegister.Api/Program.cs` | `GET /brief`: calls `BriefStore.Get`; returns `Results.NotFound()` when null, `Results.Ok(ToView(ctx, e))` otherwise; declares `.Produces(StatusCodes.Status404NotFound)` (the same bare-404 pattern already used at 17 other call sites in this file). `GET /brief/preview`: same `Get` + 404 treatment — forced by the split (see above), not a scope decision made independently. | +| `backend/src/BigRegister.Api/Data/AppDbContext.cs` | One comment updated (`GetOrCreate's invariant` → `ResetAndCreate's invariant`) — the unique index on `Owner` it annotates is unchanged. | +| `backend/tests/BigRegister.Tests/BriefEndpointTests.cs` | New `Get_returns_404_and_writes_no_row_when_no_brief_exists_for_the_owner` (the DoD-required test). The `Get()` seeding helper, used by nearly every other test in the file, renamed to `SeedBrief()` and changed to create the brief explicitly via `POST /brief/reset` instead of relying on `GET /brief`'s old side effect. One test renamed (`Get_creates_a_draft_with_expected_sections_locked_and_empty` → `SeedBrief_creates_a_draft_with_expected_sections_locked_and_empty`) — it asserts on the shape of a freshly created brief, which is now `SeedBrief()`'s job, not `GET`'s. | +| `backend/tests/BigRegister.Tests/PreviewEndpointTests.cs` | Two tests explicitly create the brief (`POST /brief/reset`) before hitting `/brief/preview`, instead of relying on the old `GET /brief` implicit create. | +| `backend/tests/BigRegister.Tests/OrgTemplateEndpointTests.cs` | Five call sites (three bare seeding `GetAsync` calls, two `GetFromJsonAsync` calls used as seeding) changed to an explicit `POST /brief/reset` first. One call site (`Sent_brief_keeps_its_pinned_template_after_a_republish`, reading a brief already created and sent by the shared `WalkBriefToSentThenRepublish` helper) needed no change — a brief already exists by the time it runs. | +| `backend/tests/BigRegister.Tests/RouteInventoryTests.cs` | Two `AllowList` reason strings updated (`GetOrCreate` → `Get`, 404 noted) — documentation text only, not itself a check the test enforces beyond "some reason is on record". | +| `e2e/brief-v2.spec.ts` | One header comment updated to name the current methods and to state explicitly that this spec's own first click ("Opnieuw beginnen (demo)") is fixture setup, not a workaround for the new 404 — see "e2e and seeding paths" below. | +| `libs/shared/src/infrastructure/api-client.ts` | Regenerated (`npm run gen:api`). `briefGET()` gains a `status === 404` branch. See "The generated client" below for the shape it actually took. | +| `docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md` | RB-23's status cell: `open` → `implemented`. | + +No `apps/ssp/src/app/brief/**` file was touched — RB-22's `BriefStore.load()` recovery and +`BriefAdapter.load()`'s `BriefLoadFailure`/`isHttpNotFound` are unchanged, per this +ticket's explicit scope. + +## The generated client + +RB-22's handoff note predicted `briefGET()` would regenerate "throwing the parsed +`ProblemDetails` (matching the shape most other endpoints already use)". That did not +happen, and the actual result is still correct. `Results.NotFound()` (this ticket's +implementation, and the pattern used at every one of the 17 other bare-404 call sites in +`Program.cs` — none of them use `ProducesProblem`/a typed body) declares a 404 with **no** +response body schema. With nothing to parse into, NSwag emits a generic branch that throws +a plain `SwaggerException` carrying `status: 404` — the same shape `briefGET()` already +threw before this ticket, for the same reason (no declared 404 body). `BriefAdapter.load()`'s +`isHttpNotFound` predicate (`(e as {status?:unknown}).status === 404`) already tolerates +both a `SwaggerException` and a parsed `ProblemDetails`, by design, precisely so this +detail would not matter — RB-22's own comment says as much. No FE follow-up was needed, and +none was made. + +## Judgement calls + +- **`GET /brief/preview` also moved off `GetOrCreate`, to `Get` + 404.** Not mentioned in + the ticket text, but unavoidable: `GetOrCreate` no longer exists once split, and this + was its only other caller. The alternative — leaving a private, undocumented + `GetOrCreate`-shaped helper only for this one endpoint — would have reintroduced + exactly the GET-writes-on-read pattern CQ-007 is about, in the one place nobody would + think to look for it. Returning 404 there too keeps both `/brief` GETs behaving the + same way. In the running app this is unreachable in practice: the preview button + only renders inside the brief page's `@if (loaded(); as s)` block + (`apps/ssp/src/app/brief/ui/brief.page.ts`), which by construction only shows once + `BriefStore.load()` has already succeeded — including via RB-22's 404-recovery branch. + So a brief always exists by the time a real user can trigger `/brief/preview`; the 404 + path there is a defensive consequence of the type split, not a new user-facing + behaviour anyone will hit. +- **`BriefStore.Get` keeps the `lock (_gate)` wrap**, even though a plain SQLite read + does not strictly need the same mutual exclusion a write does. Every other method in + this file, including the pre-existing `ApplicationStore.Get`-style query in the + sibling store, locks unconditionally — matching that convention was judged more + valuable than a lock-free read this ticket did not need to justify removing. +- **Existing test changes create the brief via `POST /brief/reset`, not a new + `BriefStore.Get`/`ResetAndCreate` direct call from the test.** Going through the HTTP + endpoint (as the old `Get()` helper always did) keeps the tests exercising the real + request pipeline (identity resolution, `ToView` mapping) rather than reaching around + it — the same reasoning that already justified an `IClassFixture` + HTTP-level test suite in the first place. + +## e2e and seeding paths + +- **`e2e/brief-v2.spec.ts`** is the only e2e spec that reaches `/brief`. It already opens + `/brief?role=drafter` and immediately clicks "Opnieuw beginnen (demo)" (`POST +/brief/reset`) before asserting anything — a deliberate fixture reset, not a + workaround. With this ticket live, the page's first `GET /brief` on the fresh + per-run database (WP-74) now 404s; RB-22's `BriefStore.load()` recovers from that by + calling `reset()` once, so the page still renders correctly, and the spec's own + explicit reset click still runs on top of that (harmless — resetting an + already-fresh brief). No behavioural change to the spec was needed; one comment was + updated to say this explicitly rather than leave it to be re-derived. +- **Storybook**: no `brief.page.stories.ts` exists, and none of the eleven `brief/ui/**` + component stories call `HttpClient`/`fetch`/`ApiClient` — every story supplies data + through component `input()`s, per the house convention (design-system/component + stories are not live-network integration tests). Nothing in Storybook depended on + `GET /brief`'s old seeding behaviour. + +## The double round-trip — verdict + +CQ-007 named this its least certain point: a first-ever visit to `/brief` now costs a 404 +followed by a `reset()` call, instead of one request that both creates and returns the +brief. **Shipped as-is; the cost is acceptable.** Three reasons: + +1. **It happens once per browser tab, ever, for one demo entity.** `BriefStore`'s + `hasRecoveredFromMissingBrief` flag (RB-22) makes the 404 unreachable again for the + life of the store instance; a real deployment has one brief per zorgverlener, created + the first time that person ever opens the page. This is not a cost paid on every + page load, or even every session — a page reload still 404s once if the flag reset + with the page, but the underlying row is already there by then, so the _second_ call + in the pair — `reset()` — is now hitting an existing row rather than truly + first-creating one, and returns just as fast as `Get` would have. +2. **An extra round-trip is not an extra spinner.** `BriefStore.load()`'s failure + handling for `notFound` calls `reset()` and applies the result through the same + `applyLoadedView` the success path uses — there is no intermediate "not found" UI + state rendered to the user between the two calls; the page shows its loading state + once, for the combined duration of both requests. +3. **The alternative was rejected, not merely deprioritized.** CQ-007's own + documentation-only alternative — leave `GetOrCreate` in place, just write down that + the GET seeds on first call — was rejected outright by agent 07 in `99-backlog.md`: + "a non-idempotent GET must be visible in the code, not only in a ticket." Given that, + the only way to remove the mixing is some version of this two-call shape; a + single-call alternative would mean either GET creates (the defect) or `POST +/brief/reset` runs unconditionally on load (destructive — it deletes an existing + brief, unacceptable for anyone with real content already saved). + +## The once-only guard's lifetime — re-verified + +RB-22 flagged this as worth re-checking once a real 404 could occur in production +traffic, not only in a test's fake adapter. Having now made the 404 real: `hasRecoveredFromMissingBrief` +is a private field on `BriefStore`, which is `providedIn: 'root'` — one instance per +browser tab (per CLAUDE.md's "shared cross-page state = one root singleton" convention), +reset only by a full page reload. That lifetime is still correct for what the flag +guards: it exists to stop a _second, separate_ `load()` call in the same tab session from +re-triggering `reset()` (e.g. a caller retrying navigation after the first recovery +already ran) — not to remember "this owner has a brief" across reloads or across owners, +which is the server's job (`BriefStore.Get` returning non-null). A page reload correctly +starts the guard over: the first `load()` after a reload will find the now-existing row +via a plain `GET` (no 404, no `reset()` call at all), so the flag never actually gets +exercised a second time in the reload case either. No FE change was needed or made. + +## Verification + +- **Verified red without the fix.** Temporarily (via `Edit`, never `git checkout`) + restored `BriefStore.GetOrCreate` alongside the new `Get`, and pointed `GET /brief` in + `Program.cs` back at `GetOrCreate`. Ran the new test alone: + ``` + BigRegister.Tests.BriefEndpointTests.Get_returns_404_and_writes_no_row_when_no_brief_exists_for_the_owner [FAIL] + Assert.Equal() Failure: Values differ + Expected: NotFound + Actual: OK + ``` + Restored the real fix with a second `Edit` (removed the temporary `GetOrCreate`, + pointed `GET /brief` back at `Get` + 404) and reran: green. +- Full backend suite after the fix: **262/262 passing**, plus the one known, + pre-existing, container-dependent failure + (`OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT`, + "Connection refused (localhost:8000)") — not this ticket's bug, does not run under + `npm run ci`, reproduces on a clean tree with no OpenZaak container running. +- `npm run gen:api`: the client changed (`libs/shared/src/infrastructure/api-client.ts`, + `briefGET()` gains a `status === 404` branch — 4 lines). Regenerated and committed; + see "The generated client" above for why the shape differs from RB-22's prediction and + why that difference is harmless. +- `npm run ci` (foreground, no background/Monitor): see result below. + +## What this ticket did not touch + +`apps/ssp/src/app/brief/application/brief.store.ts`, `brief.store.spec.ts`, and +`apps/ssp/src/app/brief/infrastructure/brief.adapter.ts` are unchanged — RB-22's FE logic +was already correct and already tested against exactly this contract, per this ticket's +explicit scope. diff --git a/e2e/brief-v2.spec.ts b/e2e/brief-v2.spec.ts index 25c8742..4e696a7 100644 --- a/e2e/brief-v2.spec.ts +++ b/e2e/brief-v2.spec.ts @@ -7,9 +7,14 @@ import { Actors, loginAs } from './support/actors'; // Preview assertions are content-type/body-level (text/html + watermark marker), not // pixel, per WP-28's decision. // -// This test mutates real state (a letter, keyed per-owner by `BriefStore.GetOrCreate`), -// and WP-74 gives it a fresh throwaway backend DB every `npm run e2e` run, so a -// leftover/in-progress letter from a PREVIOUS RUN is never an issue any more. It +// This test mutates real state (a letter, keyed per-owner by `BriefStore.Get`/ +// `ResetAndCreate` — RB-23 split the old `GetOrCreate`), and WP-74 gives it a fresh +// throwaway backend DB every `npm run e2e` run, so a leftover/in-progress letter from +// a PREVIOUS RUN is never an issue any more. `GET /brief` 404s on that fresh DB until +// the "Opnieuw beginnen (demo)" click below creates the first row — RB-22's +// `BriefStore.load()` already tolerates that 404 by calling `reset()` once, so the +// page renders correctly either way; the explicit click is this test's own fixture +// setup, not a workaround for the 404. It // deliberately still logs in as the shared `Actors.zorgverlener` rather than its own // BSN, though: giving it a distinct BSN (as `smoke.spec.ts` does) hit a real, // reproducible bug in this repo's own e2e run — `GET /brief/preview`'s sent-letter diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index a463750..581ab7d 100644 --- a/libs/shared/docs/behaviour-spec.mdx +++ b/libs/shared/docs/behaviour-spec.mdx @@ -21,7 +21,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. 467 frontend behaviours across -9 contexts; 237 backend behaviours across 41 test +9 contexts; 238 backend behaviours across 41 test classes. ## Frontend (by context) @@ -999,7 +999,8 @@ classes. ### BriefEndpointTests -- Get creates a draft with expected sections locked and empty +- Get returns 404 and writes no row when no brief exists for the owner +- SeedBrief creates a draft with expected sections locked and empty - Get offers only global and arts scoped besluit tagged passages - Get joins the case context with the BIG nummer masked - Reveal returns the unmasked BIG nummer for the drafter with step up diff --git a/libs/shared/src/infrastructure/api-client.ts b/libs/shared/src/infrastructure/api-client.ts index 04e7639..8a30596 100644 --- a/libs/shared/src/infrastructure/api-client.ts +++ b/libs/shared/src/infrastructure/api-client.ts @@ -1350,6 +1350,10 @@ export class ApiClient { result200 = _responseText === "" ? null : JSON.parse(_responseText, this.jsonParseReviver) as BriefViewDto; return result200; }); + } else if (status === 404) { + return response.text().then((_responseText) => { + return throwException("Not Found", status, _responseText, _headers); + }); } else if (status !== 200 && status !== 204) { return response.text().then((_responseText) => { return throwException("An unexpected server error occurred.", status, _responseText, _headers);