Merge RB-30 — extract BriefStore's guards into Domain/Letters/BriefRules.cs
TE-008: five guard decisions in BriefStore (Save, Submit, Send, the shared Approve/Reject review path) were pure functions of status tag, actor role and entity completeness, but each sat inside a lock-held, DB-opening method, so a spec could not exercise a decision without a booted host and a real SQLite file. BriefRules.cs holds the five pure statics; BriefStore keeps its lock, its Db.Create(), its static shape and every method signature. 29 new free-running unit assertions in BriefRuleTests.cs; the existing host-booting brief endpoint tests are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> # Conflicts: # docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md # libs/shared/docs/behaviour-spec.mdx
This commit is contained in:
@@ -68,10 +68,10 @@ public static class BriefStore
|
||||
using var db = Db.Create();
|
||||
var e = db.Briefs.FirstOrDefault(e => e.Owner == owner);
|
||||
if (e is null) return (Outcome.Conflict, null);
|
||||
if (!isDrafter) return (Outcome.Forbidden, null);
|
||||
if (e.Status.Tag is not ("draft" or "rejected")) return (Outcome.Conflict, null);
|
||||
var outcome = BriefRules.CanSave(e.Status, isDrafter);
|
||||
if (outcome != Outcome.Ok) return (outcome, null);
|
||||
e.Sections = sections.ToList();
|
||||
if (e.Status.Tag == "rejected") e.Status = new BriefStatusDto("draft");
|
||||
e.Status = BriefRules.StatusAfterSave(e.Status);
|
||||
db.SaveChanges();
|
||||
return (Outcome.Ok, e);
|
||||
}
|
||||
@@ -84,8 +84,8 @@ public static class BriefStore
|
||||
using var db = Db.Create();
|
||||
var e = db.Briefs.FirstOrDefault(e => e.Owner == owner);
|
||||
if (e is null) return (Outcome.Conflict, null);
|
||||
if (!isDrafter) return (Outcome.Forbidden, null);
|
||||
if (e.Status.Tag != "draft" || !RequiredFilled(e)) return (Outcome.Conflict, null);
|
||||
var outcome = BriefRules.CanSubmit(e.Status, isDrafter, BriefRules.RequiredFilled(e.Sections));
|
||||
if (outcome != Outcome.Ok) return (outcome, null);
|
||||
e.Status = new BriefStatusDto("submitted", SubmittedBy: e.DrafterId, SubmittedAt: at);
|
||||
db.SaveChanges();
|
||||
return (Outcome.Ok, e);
|
||||
@@ -107,7 +107,8 @@ public static class BriefStore
|
||||
using var db = Db.Create();
|
||||
var e = db.Briefs.FirstOrDefault(e => e.Owner == owner);
|
||||
if (e is null) return (Outcome.Conflict, null);
|
||||
if (e.Status.Tag != "approved") return (Outcome.Conflict, null);
|
||||
var outcome = BriefRules.CanSend(e.Status);
|
||||
if (outcome != Outcome.Ok) return (outcome, null);
|
||||
e.Status = new BriefStatusDto("sent", SentAt: at);
|
||||
// Pin the org-template version the letter was sent with (WP-23): from here on
|
||||
// its appearance is frozen — republishing the template touches unsent briefs only.
|
||||
@@ -150,7 +151,7 @@ public static class BriefStore
|
||||
// from the drafter (a drafter cannot approve their own letter). The SoD check is
|
||||
// Authz.CanActOn — the SAME check the screen DTO's decision flags use — checked
|
||||
// BEFORE the status guard so Forbidden vs Conflict ordering matches the old
|
||||
// inline check exactly.
|
||||
// inline check exactly (BriefRules.CanDecide preserves that order).
|
||||
private static (Outcome, BriefEntity?) Review(string owner, Principal principal, BriefAction action, Func<BriefStatusDto> next)
|
||||
{
|
||||
lock (_gate)
|
||||
@@ -158,15 +159,13 @@ public static class BriefStore
|
||||
using var db = Db.Create();
|
||||
var e = db.Briefs.FirstOrDefault(e => e.Owner == owner);
|
||||
if (e is null) return (Outcome.Conflict, null);
|
||||
if (!Authz.CanActOn(action, principal, e.DrafterId)) return (Outcome.Forbidden, null);
|
||||
if (e.Status.Tag != "submitted") return (Outcome.Conflict, null);
|
||||
var outcome = BriefRules.CanDecide(action, e.Status, principal, e.DrafterId);
|
||||
if (outcome != Outcome.Ok) return (outcome, null);
|
||||
e.Status = next();
|
||||
db.SaveChanges();
|
||||
return (Outcome.Ok, e);
|
||||
}
|
||||
}
|
||||
|
||||
private static bool RequiredFilled(BriefEntity e) => e.Sections.All(s => !s.Required || s.Blocks.Count > 0);
|
||||
}
|
||||
|
||||
/// <summary>Seeded template (sections + placeholder fields) and passage library.</summary>
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
using BigRegister.Api.Contracts;
|
||||
using BigRegister.Api.Data;
|
||||
using BigRegister.Domain.Authorization;
|
||||
|
||||
namespace BigRegister.Domain.Letters;
|
||||
|
||||
/// <summary>
|
||||
/// SERVER-OWNED brief state-transition and authorization rules (RB-30, TE-008). Each
|
||||
/// method is a pure decision over (status tag, actor role, entity completeness) —
|
||||
/// extracted out of <see cref="BriefStore"/>'s lock-held, DB-opening methods so the
|
||||
/// decision can be unit-tested without a booted host or a real SQLite file. Callers
|
||||
/// pass the values the rule needs, never the entity, so this stays pure.
|
||||
///
|
||||
/// Returns <see cref="BriefStore.Outcome"/> — that type already exists as the domain
|
||||
/// concept the whole brief flow reports through (`BriefResult` in Program.cs switches
|
||||
/// on it directly), so this reuses it rather than inventing a second result shape.
|
||||
/// </summary>
|
||||
public static class BriefRules
|
||||
{
|
||||
/// Save is drafter-only, and only while the letter is editable (draft/rejected).
|
||||
/// Order matches the store's original inline check: role before status, so a
|
||||
/// non-drafter always sees Forbidden even against a non-editable status.
|
||||
public static BriefStore.Outcome CanSave(BriefStatusDto status, bool isDrafter)
|
||||
{
|
||||
if (!isDrafter) return BriefStore.Outcome.Forbidden;
|
||||
if (status.Tag is not ("draft" or "rejected")) return BriefStore.Outcome.Conflict;
|
||||
return BriefStore.Outcome.Ok;
|
||||
}
|
||||
|
||||
/// A save on a rejected letter reopens it to draft (mirrors the FE reducer); a save
|
||||
/// on a draft leaves the status untouched.
|
||||
public static BriefStatusDto StatusAfterSave(BriefStatusDto status) =>
|
||||
status.Tag == "rejected" ? new BriefStatusDto("draft") : status;
|
||||
|
||||
/// Every required section needs at least one block before a letter is submittable.
|
||||
public static bool RequiredFilled(IReadOnlyList<LetterSectionDto> sections) =>
|
||||
sections.All(s => !s.Required || s.Blocks.Count > 0);
|
||||
|
||||
/// Submit is drafter-only, only from draft, and only once every required section
|
||||
/// is filled.
|
||||
public static BriefStore.Outcome CanSubmit(BriefStatusDto status, bool isDrafter, bool requiredFilled)
|
||||
{
|
||||
if (!isDrafter) return BriefStore.Outcome.Forbidden;
|
||||
if (status.Tag != "draft" || !requiredFilled) return BriefStore.Outcome.Conflict;
|
||||
return BriefStore.Outcome.Ok;
|
||||
}
|
||||
|
||||
/// Send only from approved — sending is a mechanical dispatch step, not role-gated
|
||||
/// (Authz.CanActOn already returns true unconditionally for BriefAction.Send).
|
||||
public static BriefStore.Outcome CanSend(BriefStatusDto status) =>
|
||||
status.Tag == "approved" ? BriefStore.Outcome.Ok : BriefStore.Outcome.Conflict;
|
||||
|
||||
/// Approve/Reject share this guard: the caller must be entitled to act on the letter
|
||||
/// (four-eyes/SoD, via the existing <see cref="Authz.CanActOn"/>), and the letter must
|
||||
/// be submitted. The entitlement check runs BEFORE the status check — Forbidden takes
|
||||
/// priority over Conflict, matching the store's original order exactly.
|
||||
public static BriefStore.Outcome CanDecide(BriefAction action, BriefStatusDto status, Principal principal, string drafterId)
|
||||
{
|
||||
if (!Authz.CanActOn(action, principal, drafterId)) return BriefStore.Outcome.Forbidden;
|
||||
if (status.Tag != "submitted") return BriefStore.Outcome.Conflict;
|
||||
return BriefStore.Outcome.Ok;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,147 @@
|
||||
using BigRegister.Api.Contracts;
|
||||
using BigRegister.Api.Data;
|
||||
using BigRegister.Domain.Authorization;
|
||||
using BigRegister.Domain.Letters;
|
||||
|
||||
namespace BigRegister.Tests.Domain;
|
||||
|
||||
public class BriefRuleTests
|
||||
{
|
||||
private static BriefStatusDto Status(string tag) => new(tag);
|
||||
|
||||
private static readonly Principal Drafter = new(PrincipalRole.Drafter);
|
||||
private static readonly Principal Approver = new(PrincipalRole.Approver);
|
||||
|
||||
// --- CanSave -----------------------------------------------------------------
|
||||
|
||||
[Theory]
|
||||
[InlineData("draft")]
|
||||
[InlineData("rejected")]
|
||||
public void A_drafter_may_save_a_draft_or_rejected_letter(string tag) =>
|
||||
Assert.Equal(BriefStore.Outcome.Ok, BriefRules.CanSave(Status(tag), isDrafter: true));
|
||||
|
||||
[Theory]
|
||||
[InlineData("submitted")]
|
||||
[InlineData("approved")]
|
||||
[InlineData("sent")]
|
||||
public void A_drafter_may_not_save_a_non_editable_letter(string tag) =>
|
||||
Assert.Equal(BriefStore.Outcome.Conflict, BriefRules.CanSave(Status(tag), isDrafter: true));
|
||||
|
||||
[Theory]
|
||||
[InlineData("draft")]
|
||||
[InlineData("submitted")]
|
||||
public void A_non_drafter_is_forbidden_to_save_regardless_of_status(string tag) =>
|
||||
// Role is checked before status: Forbidden wins even against an otherwise-open status.
|
||||
Assert.Equal(BriefStore.Outcome.Forbidden, BriefRules.CanSave(Status(tag), isDrafter: false));
|
||||
|
||||
// --- StatusAfterSave -----------------------------------------------------------
|
||||
|
||||
[Fact]
|
||||
public void Saving_a_rejected_letter_reopens_it_to_draft() =>
|
||||
Assert.Equal("draft", BriefRules.StatusAfterSave(Status("rejected")).Tag);
|
||||
|
||||
[Fact]
|
||||
public void Saving_a_draft_letter_leaves_its_status_unchanged() =>
|
||||
Assert.Equal("draft", BriefRules.StatusAfterSave(Status("draft")).Tag);
|
||||
|
||||
// --- RequiredFilled --------------------------------------------------------------
|
||||
|
||||
private static LetterSectionDto Section(string key, bool required, int blockCount) =>
|
||||
new(key, key, required, Enumerable.Range(0, blockCount)
|
||||
.Select(i => new LetterBlockDto("freeText", $"{key}-{i}", new RichTextBlockDto(Array.Empty<ParagraphDto>())))
|
||||
.ToList());
|
||||
|
||||
[Fact]
|
||||
public void No_required_sections_means_nothing_to_fill() =>
|
||||
Assert.True(BriefRules.RequiredFilled(Array.Empty<LetterSectionDto>()));
|
||||
|
||||
[Fact]
|
||||
public void An_optional_empty_section_does_not_block_submission() =>
|
||||
Assert.True(BriefRules.RequiredFilled(new[] { Section("slot", required: false, blockCount: 0) }));
|
||||
|
||||
[Fact]
|
||||
public void A_required_section_with_a_block_is_filled() =>
|
||||
Assert.True(BriefRules.RequiredFilled(new[] { Section("kern", required: true, blockCount: 1) }));
|
||||
|
||||
[Fact]
|
||||
public void A_required_section_with_no_blocks_is_not_filled() =>
|
||||
Assert.False(BriefRules.RequiredFilled(new[] { Section("kern", required: true, blockCount: 0) }));
|
||||
|
||||
[Fact]
|
||||
public void One_unfilled_required_section_blocks_submission_even_if_others_are_filled() =>
|
||||
Assert.False(BriefRules.RequiredFilled(new[]
|
||||
{
|
||||
Section("kern", required: true, blockCount: 1),
|
||||
Section("bijlage", required: true, blockCount: 0),
|
||||
}));
|
||||
|
||||
// --- CanSubmit -----------------------------------------------------------------
|
||||
|
||||
[Fact]
|
||||
public void A_drafter_may_submit_a_filled_draft() =>
|
||||
Assert.Equal(BriefStore.Outcome.Ok, BriefRules.CanSubmit(Status("draft"), isDrafter: true, requiredFilled: true));
|
||||
|
||||
[Fact]
|
||||
public void A_drafter_may_not_submit_an_unfilled_draft() =>
|
||||
Assert.Equal(BriefStore.Outcome.Conflict, BriefRules.CanSubmit(Status("draft"), isDrafter: true, requiredFilled: false));
|
||||
|
||||
[Fact]
|
||||
public void A_drafter_may_not_submit_a_letter_that_is_not_a_draft() =>
|
||||
Assert.Equal(BriefStore.Outcome.Conflict, BriefRules.CanSubmit(Status("submitted"), isDrafter: true, requiredFilled: true));
|
||||
|
||||
[Fact]
|
||||
public void A_non_drafter_is_forbidden_to_submit_even_a_filled_draft() =>
|
||||
// Role is checked before status/completeness: Forbidden wins over Conflict.
|
||||
Assert.Equal(BriefStore.Outcome.Forbidden, BriefRules.CanSubmit(Status("draft"), isDrafter: false, requiredFilled: true));
|
||||
|
||||
// --- CanSend ---------------------------------------------------------------------
|
||||
|
||||
[Fact]
|
||||
public void An_approved_letter_may_be_sent() =>
|
||||
Assert.Equal(BriefStore.Outcome.Ok, BriefRules.CanSend(Status("approved")));
|
||||
|
||||
[Theory]
|
||||
[InlineData("draft")]
|
||||
[InlineData("submitted")]
|
||||
[InlineData("rejected")]
|
||||
[InlineData("sent")]
|
||||
public void Only_an_approved_letter_may_be_sent(string tag) =>
|
||||
Assert.Equal(BriefStore.Outcome.Conflict, BriefRules.CanSend(Status(tag)));
|
||||
|
||||
// --- CanDecide (Approve/Reject shared guard) --------------------------------------
|
||||
|
||||
[Theory]
|
||||
[InlineData(BriefAction.Approve)]
|
||||
[InlineData(BriefAction.Reject)]
|
||||
public void An_approver_may_decide_a_submitted_letter_drafted_by_someone_else(BriefAction action) =>
|
||||
Assert.Equal(
|
||||
BriefStore.Outcome.Ok,
|
||||
BriefRules.CanDecide(action, Status("submitted"), Approver, drafterId: BriefStore.DrafterId));
|
||||
|
||||
[Fact]
|
||||
public void A_drafter_may_not_approve_or_reject() =>
|
||||
Assert.Equal(
|
||||
BriefStore.Outcome.Forbidden,
|
||||
BriefRules.CanDecide(BriefAction.Approve, Status("submitted"), Drafter, drafterId: BriefStore.DrafterId));
|
||||
|
||||
[Fact]
|
||||
public void An_approver_may_not_decide_a_letter_they_drafted_themselves() =>
|
||||
// Four-eyes / SoD: the acting approver id happens to equal the letter's drafterId.
|
||||
Assert.Equal(
|
||||
BriefStore.Outcome.Forbidden,
|
||||
BriefRules.CanDecide(BriefAction.Approve, Status("submitted"), Approver, drafterId: BriefStore.ApproverId));
|
||||
|
||||
[Fact]
|
||||
public void An_approver_may_not_decide_a_letter_that_is_not_submitted() =>
|
||||
Assert.Equal(
|
||||
BriefStore.Outcome.Conflict,
|
||||
BriefRules.CanDecide(BriefAction.Approve, Status("draft"), Approver, drafterId: BriefStore.DrafterId));
|
||||
|
||||
[Fact]
|
||||
public void Entitlement_is_checked_before_status_forbidden_wins_over_conflict() =>
|
||||
// Same actor as drafter AND a non-submitted status: still Forbidden, not Conflict —
|
||||
// matches the store's original check order (Authz.CanActOn before the status guard).
|
||||
Assert.Equal(
|
||||
BriefStore.Outcome.Forbidden,
|
||||
BriefRules.CanDecide(BriefAction.Approve, Status("draft"), Approver, drafterId: BriefStore.ApproverId));
|
||||
}
|
||||
@@ -131,7 +131,7 @@ Every ticket tracing to a `BIO-` finding, plus every row on agent 07's authorita
|
||||
| **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 | — | — | **done** |
|
||||
| **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-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** | **done** |
|
||||
| **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 |
|
||||
|
||||
@@ -0,0 +1,210 @@
|
||||
# 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.
|
||||
@@ -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; 240 backend behaviours across 41 test
|
||||
9 contexts; 261 backend behaviours across 42 test
|
||||
classes.
|
||||
|
||||
## Frontend (by context)
|
||||
@@ -1017,6 +1017,30 @@ classes.
|
||||
- Me returns no capabilities for drafter and the brief set for approver
|
||||
- Reset recreates a fresh draft with locked prefilled sections
|
||||
|
||||
### BriefRuleTests
|
||||
|
||||
- A drafter may save a draft or rejected letter
|
||||
- A drafter may not save a non editable letter
|
||||
- A non drafter is forbidden to save regardless of status
|
||||
- Saving a rejected letter reopens it to draft
|
||||
- Saving a draft letter leaves its status unchanged
|
||||
- No required sections means nothing to fill
|
||||
- An optional empty section does not block submission
|
||||
- A required section with a block is filled
|
||||
- A required section with no blocks is not filled
|
||||
- One unfilled required section blocks submission even if others are filled
|
||||
- A drafter may submit a filled draft
|
||||
- A drafter may not submit an unfilled draft
|
||||
- A drafter may not submit a letter that is not a draft
|
||||
- A non drafter is forbidden to submit even a filled draft
|
||||
- An approved letter may be sent
|
||||
- Only an approved letter may be sent
|
||||
- An approver may decide a submitted letter drafted by someone else
|
||||
- A drafter may not approve or reject
|
||||
- An approver may not decide a letter they drafted themselves
|
||||
- An approver may not decide a letter that is not submitted
|
||||
- Entitlement is checked before status forbidden wins over conflict
|
||||
|
||||
### DiplomaRuleTests
|
||||
|
||||
- Profession is derived from program
|
||||
|
||||
Reference in New Issue
Block a user