atomic-design-poc/docs/project/archive/refactor-backlog-setup/refactor-backlog/implementation/rb-30.md

RB-30 — extract BriefStore's guards into Domain/Letters/BriefRules.cs

Status: implemented · 2026-08-27 · Source finding: 02-testability.md TE-008 · 99-backlog.md RB-30

RB-30 moves the brief workflow's five guard decisions out of BriefStore (a lock-held, DB-opening static store) into a pure Domain/Letters/BriefRules.cs, and adds a free-running unit test file for them. This is a pure extraction: the store keeps its lock, its Db.Create(), its static shape, and every method's signature.

What was wrong

Five guard clusters in Data/BriefStore.cs are pure decisions over (status tag, actor role, entity completeness) — Save, Submit, Send, and the shared Approve/Reject review path each start with an if cascade that is a function of two enums and a bool. But every one of those ifs sat inside a method that had already done lock (_gate) { using var db = Db.Create(); ... }, so a spec could not exercise the decision without a booted host and a real SQLite file. Domain/Letters/ held only LetterHtml.cs and OrgTemplateRules.cs; there was no BriefRules class, even though Authz.CanActOn — a pure Domain/Authorization/ call one line away from three of the guards — already proved the pattern worked for this exact file.

What changed

File	Change
backend/src/BigRegister.Api/Domain/Letters/BriefRules.cs	New. Five pure statics: CanSave, StatusAfterSave, RequiredFilled + CanSubmit, CanSend, CanDecide. All take BriefStatusDto/bool/Principal/string, never BriefEntity — no persistence type reaches this file.
backend/src/BigRegister.Api/Data/BriefStore.cs	Save, Submit, Send, and the private Review (the Approve/Reject shared path) each replace their inline if cascade with one call into BriefRules, then branch only on the returned Outcome. The private RequiredFilled(BriefEntity e) helper is deleted — BriefRules.RequiredFilled(IReadOnlyList<LetterSectionDto>) replaces it. Lock, Db.Create(), method signatures, and the public Outcome enum are all unchanged.
backend/tests/BigRegister.Tests/Domain/BriefRuleTests.cs	New. 29 [Fact]/[Theory] assertions covering every branch of all five rules — see "Tests added" below.
docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md	RB-30's status cell: open → done.

The surface, as built — and where it differs from TE-008's proposal

TE-008 proposed:

CanSave(BriefStatusDto status, bool isDrafter) → Outcome
StatusAfterSave(BriefStatusDto) → BriefStatusDto
CanSubmit(status, isDrafter, bool requiredFilled) → Outcome
CanSend(status)
CanDecide(status, Principal, drafterId)

The ticket explicitly names this a proposal, not a specification. What was built matches it almost exactly, with two adjustments forced by the real code:

• Outcome is BriefStore.Outcome, not a new type. BriefStore already exposes a public enum Outcome { Ok, Forbidden, Conflict }, and Program.cs's BriefResult switches on it directly across every brief endpoint. TE-008 itself says: "if Outcome does not already exist as a domain concept, use whatever the sibling rule classes already return" — it does exist, so BriefRules returns it rather than inventing a second result shape. This does mean Domain/Letters/ references a type nested in Api.Data; the same cross-reference already exists in this file's neighbor, LetterHtml.cs (using BigRegister.Api.Data;, for BriefEntity), and in Authz.cs (for BriefStore's role-id constants) — both in the same single-assembly project, so this is a namespace convention, not an assembly boundary. Outcome itself is a plain three-value enum with no EF/ASP.NET attached, so this does not pull a persistence type into Domain/.
• CanDecide takes an explicit BriefAction action parameter, not just (status, Principal, drafterId). The real guard — BriefStore.Review — is one private method shared by both Approve and Reject, and it calls Authz.CanActOn(action, principal, drafterId), which needs to know which action is being attempted. BriefRules.CanDecide composes that existing pure Authz.CanActOn call with the status check, rather than re-implementing the SoD logic a second time — so the four-eyes rule still has exactly one source of truth.

The RequiredFilled predicate is a sixth pure static, not one of the five guards proper — TE-008 names it separately ("plus the RequiredFilled(e) predicate") and it is built the same way: RequiredFilled(IReadOnlyList<LetterSectionDto> sections) → bool, taking the section list rather than the entity.

Order and behaviour preserved

Every rule keeps the original check order, which matters because Outcome.Forbidden must outrank Outcome.Conflict (a non-drafter or non-entitled caller sees Forbidden even against an otherwise-invalid status):

• CanSave: !isDrafter (Forbidden) before the status-tag check (Conflict).
• CanSubmit: !isDrafter (Forbidden) before status.Tag != "draft" || !requiredFilled (Conflict).
• CanDecide: !Authz.CanActOn(...) (Forbidden) before status.Tag != "submitted" (Conflict) — the exact order the old inline check in Review used, per its own comment ("checked BEFORE the status guard").
• CanSave's entity-not-found branch (e is null → Conflict) stays inline in BriefStore — it is a persistence fact ("no row for this owner"), not one of the three business axes TE-008 names (status tag, actor role, entity completeness), so it was left where it was rather than forced into a rule that would then need to accept a nullable entity.

Tests added

backend/tests/BigRegister.Tests/Domain/BriefRuleTests.cs, 29 assertions, alongside the seven Domain test files that already existed:

• CanSave — drafter saves draft/rejected (Ok, [Theory]); drafter saves submitted/approved/sent (Conflict, [Theory]); non-drafter saves draft or submitted (Forbidden both times — proves role beats status).
• StatusAfterSave — rejected → draft; draft stays draft.
• RequiredFilled — no sections; an unfilled optional section; a filled required section; an unfilled required section; one filled + one unfilled required section (proves one bad section blocks the whole letter).
• CanSubmit — filled draft (Ok); unfilled draft (Conflict — the required-filled gate the ticket explicitly asked for); non-draft status (Conflict); non-drafter on a filled draft (Forbidden — role beats completeness).
• CanSend — approved (Ok); draft/submitted/rejected/sent (Conflict, [Theory]).
• CanDecide — approver decides a submitted letter drafted by someone else, for both Approve and Reject (Ok, [Theory]); a drafter attempting to decide (Forbidden — the non-drafter denial the ticket asked for); an approver whose acting id equals the drafter id, i.e. self-review (Forbidden — the four-eyes/SoD case); an approver deciding a non-submitted letter (Conflict); an approver who is also the drafter AND the status is non-submitted (Forbidden, not Conflict — proves the priority order survived the extraction).

Verified red without the fix

Inverted CanSubmit's completeness check (!requiredFilled → requiredFilled) with an Edit, ran BriefRuleTests alone:

[xUnit.net]     BigRegister.Tests.Domain.BriefRuleTests.A_drafter_may_not_submit_an_unfilled_draft [FAIL]
  Assert.Equal() Failure: Values differ
Expected: Conflict
Actual:   Ok
[xUnit.net]     BigRegister.Tests.Domain.BriefRuleTests.A_drafter_may_submit_a_filled_draft [FAIL]
  Assert.Equal() Failure: Values differ
Expected: Ok
Actual:   Conflict

Failed!  - Failed: 2, Passed: 27, Skipped: 0, Total: 29

Reverted with a second Edit (never git checkout — that would have discarded the whole file). Reran: 29/29 green.

Existing tests — unchanged

BriefEndpointTests.cs, PreviewEndpointTests.cs, and OrgTemplateEndpointTests.cs (the three host-booting suites that exercise the brief endpoints) needed no changes. Ran together: 32/32 passing, proving the extraction preserved every HTTP outcome (Save_is_drafter_only, Submit_blocks_on_empty_required_section, Submit_succeeds_when_required_sections_filled, Drafter_cannot_approve_own_letter_but_a_different_reviewer_can, Reject_returns_comments, Editing_a_rejected_letter_reopens_it_to_draft, Send_only_from_approved, and the rest, all unmodified).

The metric TE-008 cares about: host-booting brief-rule assertions

Before this ticket, the five guard decisions had zero free-running unit assertions. Every branch of every guard was reachable only through the seven host-booting endpoint test methods above (six of them containing an explicit Assert.Equal(HttpStatusCode.Forbidden/Conflict, ...), each paying a full TestWebApplicationFactory host boot plus a real SQLite round-trip, run serially process-wide because of [assembly: DisableTestParallelization]).

After this ticket:

• 0 → 29 free-running unit assertions covering these branches (BriefRuleTests.cs, dotnet test --filter FullyQualifiedName~BriefRuleTests completes in ~120 ms, no host, no SQLite file).
• 7 → 7 host-booting endpoint tests, unchanged. They stay — they are now the proof that BriefStore wires BriefRules's answer to the right HTTP status, not the only place the business decision itself is checked. That split (wiring proven at the integration layer, decision logic proven at the unit layer) is the seam TE-008 argued for.
• New branches this ticket made assertable that the endpoint suite never covered directly: the SoD self-review case (An_approver_may_not_decide_a_letter_they_drafted_themselves) and the Forbidden-beats-Conflict priority ordering for both CanSave/CanSubmit (role checked first) and CanDecide (entitlement checked first) — these existed as implicit behaviour in the original if cascades but had no assertion pinning them before RB-30.

What was not extracted

Nothing — all five guards named in TE-008, plus the RequiredFilled predicate, moved cleanly. None needed the DbContext: each was already a function of values already resident on the in-memory BriefEntity (its Status, Sections, DrafterId), never of a query against the database itself.

Scope respected

• Domain/Letters/LetterHtml.cs was not touched (a concurrent agent owns it).
• BriefEntity.ToDto() was not touched — its CC 16 is a separate, out-of-scope finding per the ticket.
• Data/Db.cs's static-store decision and TestWebApplicationFactory's serialized-test position were not challenged; the store's lock, Db.Create(), and public shape are byte-for-byte the same as before this ticket, other than the if cascades moving out.

Verification

• dotnet build: 0 warnings, 0 errors.
• dotnet test --filter FullyQualifiedName~BriefRuleTests: 29/29, ~120 ms.
• dotnet test --filter FullyQualifiedName~BriefEndpointTests|...PreviewEndpointTests|...OrgTemplateEndpointTests: 32/32, unchanged.
• Full backend suite: 291/292 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 ci (foreground, no background/Monitor): see the commit message / session report for the exit code and step count.

What this ticket did not touch

No frontend file was touched — the brief workflow's status machine is server- authoritative, and the FE's own pure reducer (mirroring these same transitions for UX) was already out of this ticket's scope. No file outside backend/Data/BriefStore.cs, backend/Domain/Letters/BriefRules.cs, backend/tests/BigRegister.Tests/Domain/BriefRuleTests.cs, and 99-backlog.md was changed.