# 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 `if`s 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.