diff --git a/backend/src/BigRegister.Api/Data/BriefStore.cs b/backend/src/BigRegister.Api/Data/BriefStore.cs index f880d98..170b3fd 100644 --- a/backend/src/BigRegister.Api/Data/BriefStore.cs +++ b/backend/src/BigRegister.Api/Data/BriefStore.cs @@ -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 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); } /// Seeded template (sections + placeholder fields) and passage library. diff --git a/backend/src/BigRegister.Api/Domain/Letters/BriefRules.cs b/backend/src/BigRegister.Api/Domain/Letters/BriefRules.cs new file mode 100644 index 0000000..a5bea7c --- /dev/null +++ b/backend/src/BigRegister.Api/Domain/Letters/BriefRules.cs @@ -0,0 +1,63 @@ +using BigRegister.Api.Contracts; +using BigRegister.Api.Data; +using BigRegister.Domain.Authorization; + +namespace BigRegister.Domain.Letters; + +/// +/// 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 '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 — 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. +/// +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 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 ), 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; + } +} diff --git a/backend/tests/BigRegister.Tests/Domain/BriefRuleTests.cs b/backend/tests/BigRegister.Tests/Domain/BriefRuleTests.cs new file mode 100644 index 0000000..de01174 --- /dev/null +++ b/backend/tests/BigRegister.Tests/Domain/BriefRuleTests.cs @@ -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()))) + .ToList()); + + [Fact] + public void No_required_sections_means_nothing_to_fill() => + Assert.True(BriefRules.RequiredFilled(Array.Empty())); + + [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)); +} 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 fe54364..a95d9fa 100644 --- a/docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md +++ b/docs/project/refactor-backlog-setup/refactor-backlog/99-backlog.md @@ -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 | — | — | 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-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 | diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-30.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-30.md new file mode 100644 index 0000000..207d4f3 --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-30.md @@ -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)` 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 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. diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index 581ab7d..ff45045 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; 238 backend behaviours across 41 test +9 contexts; 259 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