From a9ef9e1286b4111dcdea0a53b249a10fd423872c Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 22:00:50 -0700 Subject: [PATCH 01/17] feat(eggs): bind Daily Entry condition counters to stable grade identities (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 1 of the plan, domain half. Carries the plan and design docs from the closed #397 so the reviewed rules land with the code that implements them. EggGrade gains DailyEntryKind (Manual/Cracked/Dirty) — an IDENTITY for which counter feeds a grade, deliberately not a name match: a farm can rename "Cracked" to "Segunda" through the ordinary grade endpoint, and a name-matching resolver would then detach the farm's own counter from its own grade. Immutable after creation, for a stronger reason than GradeType's: an official entry snapshots the id its counter resolved to, so re-pointing a kind afterwards would restate what a past day produced. Update takes no kind parameter, which is the enforcement. DailyEntry gains CrackedGradeId/DirtyGradeId, written once at the first official transition and never re-resolved. Null means "resolved to nothing" — a durable record that the condition was a loss — not "not yet known". The domain does not resolve them (it has no catalog); the handler passes what it resolved, having required both Active and IsSaleable. That both-flags rule is pinned at its source by Deactivate_leaves_saleability_alone: EggGrade.Deactivate() flips Active and leaves IsSaleable set, so inactive-but-saleable is an ordinary reachable state. A saleability-only resolver would mint lots under a grade the farm has retired from capture — the defect #397's review round 3 caught in prose, now caught by a test. Mutation-checked, baseline green (303/303, solution builds 0 warnings): dropping the Submit assignment turns Submit_records_the_resolved_quality_grades red; making ManagerAdjust clear the snapshot turns Adjust_keeps_the_original_snapshot_and_never_re_resolves red. Still to come in Task 1: EF configuration, the hand-carried InitialCreate columns/partial index/seed defaults, and the two export projections. --- ...y-entry-reconciliation-and-quality-eggs.md | 88 ++++++++++ ...-reconciliation-and-quality-eggs-design.md | 156 ++++++++++++++++++ src/Cluckwork.Domain/Eggs/DailyEntry.cs | 25 ++- src/Cluckwork.Domain/Eggs/EggGrade.cs | 31 +++- .../Eggs/DailyEntryTests.cs | 66 ++++++++ .../Eggs/EggGradeTests.cs | 67 ++++++++ 6 files changed, 430 insertions(+), 3 deletions(-) create mode 100644 docs/superpowers/plans/2026-08-02-daily-entry-reconciliation-and-quality-eggs.md create mode 100644 docs/superpowers/specs/2026-08-02-daily-entry-reconciliation-and-quality-eggs-design.md create mode 100644 tests/Cluckwork.Domain.Tests/Eggs/EggGradeTests.cs diff --git a/docs/superpowers/plans/2026-08-02-daily-entry-reconciliation-and-quality-eggs.md b/docs/superpowers/plans/2026-08-02-daily-entry-reconciliation-and-quality-eggs.md new file mode 100644 index 000000000..bc15bc410 --- /dev/null +++ b/docs/superpowers/plans/2026-08-02-daily-entry-reconciliation-and-quality-eggs.md @@ -0,0 +1,88 @@ +# Daily Entry Reconciliation and Saleable Quality Eggs Implementation Plan + +Tracks #394 and #396. Implementation must follow the non-CI writer rule in +PR #395. + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Require official Daily Entry reconciliation while allowing independently saleable Cracked and Dirty stock with farm-defined product prices. + +**Architecture:** Store a stable Daily Entry kind on egg grades and snapshot the resolved Cracked/Dirty grade ids on the official Daily Entry. Manual grades always reconcile against the non-condition remainder; submission and adjustment add or reconcile all snapshot-backed lots atomically. Product prices remain product-owned and are never derived from another grade. + +**Tech Stack:** .NET 10/C#, EF Core/Postgres migrations, React 19/Vite/TypeScript, FluentValidation, Vitest, xUnit/Testcontainers, Playwright simulation harness. + +## Global Constraints + +- Preserve `DailyEntry.Version++` on every mutation and add parallel-race coverage for new aggregate mutation paths. +- Drafts accept incomplete manual grade lines; submit and adjustment require exact reconciliation. +- `Discarded` stays non-saleable; Cracked and Dirty are saleable only through their snapshotted configured grade. +- **Add no second migration.** `Persistence/Migrations/` holds exactly one, `InitialCreate` (#245), and this application has never been deployed — a virgin database is the only starting state anywhere. Hand-carry the columns, the partial unique index, and the seed changes into `InitialCreate` itself, and hand-edit rather than regenerate: it carries four un-regenerable `lower("Name")` expression indexes and 21 rows of `WHERE NOT EXISTS`-guarded reference data that `dotnet ef migrations add` would silently drop. There is correspondingly **no existing farm whose saleability could be modified**, so the "leave existing farms untouched" half of this constraint describes data that cannot exist — dropping it avoids shipping the dead-backfill pattern the #245 squash was performed to remove. +- Update every non-CI writer and fixture per PR #395, including simulation manifest verification. +- Update `HelpPage.tsx` and `specs/product/GLOSSARY.md` with the behavior change. + +--- + +### Task 1: Persist quality-condition identity and official-entry snapshots + +**Files:** +- Modify: `src/Cluckwork.Domain/Eggs/EggGrade.cs`, `src/Cluckwork.Domain/Eggs/DailyEntry.cs` +- Modify: EF configurations and the hand-maintained `InitialCreate` (hand-edited, never regenerated) — **no second migration**, see Global Constraints +- Modify: base-reference seed SQL and migration security/base-data tests +- Modify: `src/Cluckwork.Infrastructure/Repositories/ExportQueries.cs` — add the two quality snapshot ids to the `daily-entries` projection and `DailyEntryKind` to the `egg-grades` projection, so a full account export can still reconstruct which grade a condition counter represented +- Test: Egg-grade and DailyEntry domain tests; migration tests; `ExportTests.cs` regression coverage for both changed projections + +- [ ] Write failing tests for immutable `DailyEntryKind`, one Cracked/Dirty grade per farm, null/non-null quality snapshots on official entries, and an account export missing both additions. +- [ ] Run the focused tests and verify the model cannot yet distinguish a renamed Cracked/Dirty grade from any other quality grade, and that a full account export cannot yet reconstruct which grade a condition counter represented. +- [ ] Add the enum, partial uniqueness constraint, and official-entry snapshots, carried into `InitialCreate` by hand — columns, index, and the seed defaults together. Confirm afterwards that the four `lower("Name")` expression indexes and all 21 guarded reference-data rows survive, and that `MigrationSecurityReviewTests` and `BaseReferenceDataMigrationTests` still pass; treat a failure there as having destroyed something, not as a test to adjust. +- [ ] Add the quality snapshot ids to the `daily-entries` export projection and `DailyEntryKind` to the `egg-grades` export projection. +- [ ] Verify new-base defaults are saleable, existing official records remain non-reclassified, migration security tests still pass, and both export regression tests are green. + +### Task 2: Enforce exact reconciliation and atomically produce quality lots + +**Files:** +- Modify: `DailyEntry`, `RecordDailyEntryHandler`, `SubmitDailyEntryHandler`, `AdjustDailyEntryHandler`, grade repositories/commands as required +- Test: Domain, record, submit, adjustment, stock, report, and concurrency integration tests + +- [ ] Write failing domain/API tests for ungraded and partially graded official entries, zero remainder, each saleable quality counter, each non-saleable quality counter, an **inactive-but-saleable** condition grade (must snapshot null — `EggGrade.Deactivate()` leaves `IsSaleable` set, so this is the case a saleability-only rule wrongly accepts), a zero-count day for an otherwise-saleable quality counter (snapshot recorded, no lot — `EggLot.Create` rejects zero), and an adjustment that preserves its original snapshot. +- [ ] Write failing API tests submitting a **condition-kind grade id as a manual line** — on both record and adjust — and assert it is refused. Excluding condition grades from the manual pane is a UI affordance, not the enforcement: `RecordDailyEntryHandler` gates manual-line eligibility on `IsSaleable` **alone**, with no `GradeType`/kind restriction, and this feature makes Cracked and Dirty saleable by default. So a direct or stale API client can name a Cracked id as a manual line, the exact-total check passes (it is only a sum), and submission then creates **both** that manual lot and the counter-backed quality lot for the same grade — double-counting the day's stock and breaking the one-lot-per-grade assumption reconciliation depends on. The rejection belongs at the handler/aggregate boundary where the UI cannot be bypassed. +- [ ] Run them red; assert status/version/lots remain unchanged on refusal. +- [ ] Implement `manualGrades == total - cracked - dirty - discarded` at the aggregate boundary, reject any manual line naming a condition-kind grade, resolve snapshots only on first official submission and only for a grade that is both `Active` and `IsSaleable`, and create/reconcile quality lots — skipping lot creation for a zero counter while still recording its snapshot — in the existing transactions. +- [ ] Update report and stock projections to count only snapshot-backed quality stock as marketable; verify no historical row changes its meaning. +- [ ] Run targeted integration suites plus parallel submit/adjust tests. + +### Task 3: Extend the API contract and align Daily Entry and adjustment UI + +**Files:** +- Modify: `src/Cluckwork.Api/Endpoints/DailyEntries/DailyEntryEndpoints.cs` — `DailyEntryResponse` gains the two quality snapshot ids +- Modify: `src/Cluckwork.Api/Endpoints/EggGrades/EggGradeEndpoints.cs` — `EggGradeResponse` gains `DailyEntryKind` +- Modify: `web/src/api/cluckwork.ts` — mirror both DTO changes in the `DailyEntry` and `EggGrade` TypeScript contracts +- Modify: `web/src/routes/DailyEntryPage.tsx`, `web/src/routes/HistoryPage.tsx`, shared dialog/styles if needed +- Modify: `web/src/i18n/en.ts`, `web/src/i18n/es.ts`, `web/src/i18n/tl.ts` +- Test: API integration coverage in `SubmitDailyEntryTests.cs`/`DailyEntryAdjustTests.cs` and `EggGradeManagementTests.cs` for the two new response fields; `DailyEntryPage.test.tsx`, `HistoryPage.test.tsx`, catalog parity/typecheck + +- [ ] Write failing API integration tests asserting `DailyEntryResponse` carries both quality snapshot ids and `EggGradeResponse` carries `DailyEntryKind`, plus failing UI tests that exclude condition grades from manual entry, show quality counts in the marketable summary, disable official saves for an under/over grade total, and render the adjustment dialog as the two-pane Daily Entry layout. +- [ ] Run focused API and Vitest tests red. +- [ ] Add both response fields and their TypeScript contract mirrors, then implement shared calculations and the adjustment layout without changing draft-save behavior, conflict rebind, reason requirement, or accessible live feedback. The History UI must derive its reconciliation feedback from the entry's own snapshots, never the current egg-grade catalog, so a later saleability change cannot retroactively rewrite how a past entry reads. +- [ ] Run API integration tests, page tests, i18n catalog parity, and typecheck. + +### Task 4: Preserve sales configuration and update non-CI writers + +**Files:** +- Modify: product/grade UI guidance and relevant API tests +- Modify: `DemoDataSeeder`, `SimulationDataSeeder`, `tools/simulation/k6/bundles.js`, Manager/Worker Playwright specs, simulation manifest/verification scripts +- Test: seeder/manifest tests and simulation-harness self-check + +- [ ] Write failing coverage proving a Cracked/Dirty product maps only to its saleable quality lot and can retain an independently configured default price. +- [ ] Update all seed and browser writers to satisfy exact reconciliation and include quality lots where intended. +- [ ] Recompute the simulation fixture manifest and add/update self-checks so a stale writer or fixture fails closed. +- [ ] Execute targeted seed, k6 bundle, Playwright, and harness verification commands documented by PR #395. + +### Task 5: Document the operating rule + +**Files:** +- Modify: `web/src/routes/HelpPage.tsx`, i18n help catalogs, `specs/product/GLOSSARY.md`, `specs/product/specs.md` +- Test: Help-page tests and catalog parity + +- [ ] Explain that drafts are flexible but official saves reconcile exactly, and that Cracked/Dirty quality stock is farm-configurable and separately priced through products. +- [ ] Document the no-retroactive-reclassification rule. +- [ ] Run Help, i18n, backend, and SPA verification suites before review. diff --git a/docs/superpowers/specs/2026-08-02-daily-entry-reconciliation-and-quality-eggs-design.md b/docs/superpowers/specs/2026-08-02-daily-entry-reconciliation-and-quality-eggs-design.md new file mode 100644 index 000000000..642d12d15 --- /dev/null +++ b/docs/superpowers/specs/2026-08-02-daily-entry-reconciliation-and-quality-eggs-design.md @@ -0,0 +1,156 @@ +# Daily Entry Reconciliation and Saleable Quality Eggs + +Tracks #394 and #396. Implementation must follow the non-CI writer rule in +PR #395. + +## Goal + +Prevent an official Daily Entry from losing production, while allowing a farm to +sell Cracked and Dirty eggs as separate, optionally discounted stock. + +## Decisions + +- Draft Daily Entries remain editable with incomplete grading. +- Submit and manager adjustment require exact reconciliation; no API client can + bypass it. +- Cracked and Dirty are saleable by default on a new installation and + independently configurable per farm. +- Discarded remains non-saleable. +- A farm prices Cracked and Dirty through normal, separate Products. There is + no global discount percentage or automatic price calculation. +- A configuration change affects future submissions only. Historical entries + keep their recorded disposition and are never backfilled into stock. + +## Data model + +`EggGrade` gains a stable `DailyEntryKind` (`Manual`, `Cracked`, or `Dirty`). +At most one grade of each quality kind exists per farm; a partial unique index +enforces that rule. The kind is immutable, so renaming a grade never changes +its meaning. The default Cracked and Dirty grades receive their matching kinds +and are saleable in fresh base data. + +`DailyEntry` snapshots `CrackedQualityGradeId` and `DirtyQualityGradeId` when +it first becomes official. Resolution requires the condition grade to be +**both `Active` and `IsSaleable`**; anything else snapshots null. Both +conditions are load-bearing, and saleability alone is not sufficient: +`EggGrade.Deactivate()` flips `Active` and deliberately leaves `IsSaleable` +untouched, so an inactive-but-saleable grade is an ordinary reachable state +whenever an owner deactivates a condition grade without first clearing its +saleability. Resolving on saleability alone would then snapshot a grade the +egg-grade endpoints have removed from capture and mint new lots under it. +A null snapshot therefore records "this condition was not stock that day", +whether because it was non-saleable or because it was deactivated. + +Cover the inactive-but-saleable submission explicitly — it is the case a +saleability-only rule passes and this rule refuses. + +The snapshot—not the current grade setting—governs later reporting, adjustment, +and lot reconciliation. Existing official entries initialise both snapshots as +null. + +Both snapshot ids and each grade's `DailyEntryKind` are part of the public API +contract (`DailyEntryResponse`, `EggGradeResponse`) and their TypeScript +mirrors, not domain-internal state: the History UI must derive its +reconciliation feedback from an entry's own snapshot, never from the egg-grade +catalog's current state, or a later saleability change would retroactively +rewrite how a past entry reads. + +## Write path + +The manual Grading pane excludes the two configured quality-condition grades, +and the **record and adjust handlers refuse a manual line naming either of +them**. The pane is an affordance; the handler check is the guarantee, and +both are required. Manual-line eligibility is gated on `IsSaleable` alone with +no kind restriction, and this feature makes Cracked and Dirty saleable by +default — so with UI-only exclusion a direct or stale API client could name a +Cracked id as a manual line, pass the exact-total check (it is only a sum), +and produce **two lots for one grade**: the manual line's and the +counter-backed one. That double-counts the day's stock and breaks the +one-lot-per-grade assumption reconciliation depends on. + +Its target is always `total - cracked - dirty - discarded`. On submit, the +domain verifies that exact total, then creates one lot per manual grade line +plus one lot for each quality snapshot whose counter is positive that day. A +non-null snapshot is always recorded, independent of its counter — later +adjustments and historical reporting need to know which grade a condition +represented even on a zero-count day — but `EggLot.Create` rejects a zero or +negative quantity, so a zero counter must never reach lot creation. A +zero-cracked day and a zero-dirty day are each an explicit, tested case: the +snapshot is recorded and only that condition's lot is skipped. On adjustment, +the same invariant applies and all linked lots, including quality lots, +reconcile in the existing transaction and retain sold-floor protection. + +This preserves one accounting identity: every collected egg is discarded, a +non-saleable condition loss, or represented by exactly one stock lot. + +## UI and pricing + +Daily Entry and the History adjustment dialog share the same counts/grading +hierarchy and live reconciliation feedback. The adjustment dialog has no draft +state, so its save control is disabled until the exact target and reason are +present. The Products screen continues to map one product to one saleable +grade. Products mapped to Cracked and Dirty set their own optional default +price; an operator may price them lower, equal, or leave a price unset. + +## Compatibility and operations + +Both the schema change and the seed change are carried into the hand-maintained +`InitialCreate` migration itself. There is **no follow-on migration** — see the +next paragraph for why one would touch zero rows by construction, and AGENTS.md +(#245) for why adding a dead backfill is the pattern that squash was performed +to eliminate. Cracked and Dirty are therefore saleable in base data on every +database, because a virgin database is the only database. No historical lots +are invented. + +No migration or grandfather path is needed for a pre-existing incomplete +official entry or a pre-existing manual Cracked/Dirty grade line. This +application has never been deployed, and #245 squashed schema history to one +`InitialCreate` migration, so a virgin database is the only starting state — +there is no production data to grandfather. Two narrower checks confirm +nothing else stands in for it: + +- **A pre-existing incomplete official entry, adjusted after this feature + lands.** The exact-reconciliation gate on Submit/`ManagerAdjust` + (`manualGrades == total - cracked - dirty - discarded`) is #394's rule, + shared with PR #400 — whichever of the two lands first establishes it, and + nothing here changes what it requires. What this design adds on top is + quality-lot creation/reconciliation, gated on a non-null quality snapshot. + Every entry that predates this feature, complete or incomplete, snapshots + null for both conditions (see Data model), so that addition is a no-op on + their adjustment path: it neither blocks nor unblocks anything the shared + #394 rule didn't already decide. Whether an entry submitted under the old + lenient rule — including the explicitly supported no-grade case + `SubmitDailyEntryTests.Submit_WithoutGrades_Succeeds_NoLots` — can later be + adjusted is #394's own compatibility question, unaffected by whether this + feature has landed. +- **A pre-existing manual grade line against the Cracked or Dirty grade.** + Cracked and Dirty already exist in `InitialCreate`'s base reference data + today, as ordinary `GradeType = 'Quality'` grades seeded `IsSaleable = + FALSE`. `RecordDailyEntryHandler` gates a manual grade line on `IsSaleable` + alone, with no `GradeType` restriction, so the capability to grade into them + manually has existed since #245 — a farm need only mark one saleable through + the existing egg-grade endpoint. Nothing exercises that capability today: + `DemoDataSeeder` and `SimulationDataSeeder` are the only writers that submit + entries outside a human operator, and both draw manual grade lines only from + `Large`/`Medium`/`Small` by name — Cracked and Dirty are never candidates, + both because neither seeder selects them and because they seed + non-saleable. There is no deployed farm to have done it by hand either. The + one place it could theoretically exist is a developer's own local dev + database, hand-poked through the SPA and never reset — and that is already + covered by the standing remedy AGENTS.md prescribes for this exact class of + dev-database/migration mismatch: drop and recreate (`docker compose -f + deploy/docker-compose.dev.yml down -v && docker compose -f + deploy/docker-compose.dev.yml up -d`), the same fix already required at the + #245 squash boundary. This feature does not need its own migration or + preservation logic for that case. + +Because this changes the Daily Entry write contract, implementation must update +and verify every non-CI writer and fixture named in PR #395: demo/simulation +seeders, k6 bundles, and the Playwright Manager and Worker flows. The +simulation manifest must change when fixture totals or lots change. + +## Out of scope + +- Food-safety eligibility rules and jurisdiction-specific restrictions. +- Automatic discount formulas, price levels, or multi-grade products. +- A later regrade/repack workflow. diff --git a/src/Cluckwork.Domain/Eggs/DailyEntry.cs b/src/Cluckwork.Domain/Eggs/DailyEntry.cs index c722b61d0..ac513e44e 100644 --- a/src/Cluckwork.Domain/Eggs/DailyEntry.cs +++ b/src/Cluckwork.Domain/Eggs/DailyEntry.cs @@ -23,6 +23,21 @@ public sealed class DailyEntry : AggregateRoot // Sellable production by grade (Phase 1: eggs are graded at collection). public IReadOnlyList Grades => _grades.AsReadOnly(); + // #396 — which grade this day's Cracked and Dirty counters resolved to, or + // null when that condition was a loss. Written ONCE, at the first official + // transition, and never re-resolved: the entry is the historical record of + // what the farm's configuration meant ON THAT DAY. Re-resolving on a later + // adjustment would let a saleability or Active change silently restate a + // past day — banking eggs recorded as a loss, or dropping stock the farm + // already holds and may have sold. + // + // The domain does not resolve these itself (it has no grade catalog); the + // handler passes what it resolved, having required both Active and + // IsSaleable. Null therefore means "resolved to nothing", a durable fact, + // not "not yet known". + public Guid? CrackedGradeId { get; private set; } + public Guid? DirtyGradeId { get; private set; } + // Audit trail until the audit-log slice lands (#69): the last adjust's // reason + the values it replaced (JSON snapshot), the void reason, and // when the auto-lock job locked the entry. @@ -81,7 +96,11 @@ public Result RecordProduction( return Result.Success(); } - public Result Submit() + // #396 — crackedGradeId/dirtyGradeId are the grades the CALLER resolved for + // this day's condition counters, each already required to be both Active + // and IsSaleable, or null when that condition is a loss. They are snapshot + // here and never resolved again (see the fields). + public Result Submit(Guid? crackedGradeId = null, Guid? dirtyGradeId = null) { if (Status != DailyEntryStatus.Draft) return Result.Failure(Error.Domain( @@ -98,6 +117,10 @@ public Result Submit() requireExactReconciliation: true); if (gradeResult.IsFailure) return gradeResult; + // The single resolution point for the life of the entry. + CrackedGradeId = crackedGradeId; + DirtyGradeId = dirtyGradeId; + Status = DailyEntryStatus.Submitted; // Version is a concurrency token, not auto-incremented by EF: without this // bump, two racing submits both match WHERE Version = N and both succeed — diff --git a/src/Cluckwork.Domain/Eggs/EggGrade.cs b/src/Cluckwork.Domain/Eggs/EggGrade.cs index e2ec62e8e..f3544bde2 100644 --- a/src/Cluckwork.Domain/Eggs/EggGrade.cs +++ b/src/Cluckwork.Domain/Eggs/EggGrade.cs @@ -15,13 +15,23 @@ public sealed class EggGrade : AggregateRoot public int SortOrder { get; private set; } public bool IsSaleable { get; private set; } public bool Active { get; private set; } + + // #396 — which Daily Entry counter, if any, feeds this grade. The binding + // is an IDENTITY, deliberately not a name match: a farm may rename + // "Cracked" to "Segunda" through the ordinary grade endpoint, and a + // name-matching resolver would then quietly detach the farm's own counter + // from the farm's own grade. At most one Cracked and one Dirty per farm, + // enforced by a partial unique index (see InitialCreate). + public DailyEntryKind DailyEntryKind { get; private set; } + public int Version { get; private set; } private EggGrade() { } public static EggGrade Create( Guid id, Guid accountId, Guid farmId, string name, - EggGradeType gradeType, int sortOrder, bool isSaleable) + EggGradeType gradeType, int sortOrder, bool isSaleable, + DailyEntryKind dailyEntryKind = DailyEntryKind.Manual) { if (string.IsNullOrWhiteSpace(name)) throw new ArgumentException("Grade name is required.", nameof(name)); @@ -35,13 +45,18 @@ public static EggGrade Create( GradeType = gradeType, SortOrder = sortOrder, IsSaleable = isSaleable, + DailyEntryKind = dailyEntryKind, Active = true }; } // GradeType stays immutable after creation — history recorded under a bucket // keeps the axis it was captured with; relabeling the axis would silently - // reinterpret past entries. + // reinterpret past entries. DailyEntryKind is immutable for a STRONGER + // reason (#396): an official entry snapshots the grade id its Cracked or + // Dirty counter resolved to, so re-pointing a kind afterwards would change + // what a past day's counter is understood to have produced. Neither is a + // parameter here, which is the enforcement — there is no path to change one. public Result Update(string name, int sortOrder, bool isSaleable) { if (string.IsNullOrWhiteSpace(name)) @@ -77,3 +92,15 @@ public Result Activate() } public enum EggGradeType { Size, Quality, Custom } + +// #396 — which Daily Entry input feeds a grade. `Manual` is every ordinary +// grade, entered by hand in the Grading pane. `Cracked` and `Dirty` are the two +// grades fed by the entry's own condition counters instead, and are excluded +// from manual grading precisely so a condition egg cannot also be counted as a +// manual line (which would produce two lots for one grade on one day). +// +// Deliberately separate from EggGradeType: that says what KIND of bucket this +// is (a size, a quality, a custom one) and is the farm's own taxonomy, while +// this says WHERE THE NUMBER COMES FROM and is the app's wiring. A farm can +// have many Quality grades; only one of them can be the Cracked counter's. +public enum DailyEntryKind { Manual, Cracked, Dirty } diff --git a/tests/Cluckwork.Domain.Tests/Eggs/DailyEntryTests.cs b/tests/Cluckwork.Domain.Tests/Eggs/DailyEntryTests.cs index 311857e55..857873321 100644 --- a/tests/Cluckwork.Domain.Tests/Eggs/DailyEntryTests.cs +++ b/tests/Cluckwork.Domain.Tests/Eggs/DailyEntryTests.cs @@ -11,6 +11,72 @@ private static DailyEntry MakeDraft() => private static readonly Guid GradeLarge = Guid.NewGuid(); private static readonly Guid GradeMedium = Guid.NewGuid(); + // ---- #396 quality-condition snapshots ------------------------------- + // + // An official entry records WHICH grade its Cracked and Dirty counters + // resolved to, so a later catalog change cannot reinterpret a past day. + // The domain does not resolve them — it has no catalog — so the caller + // passes the ids it resolved (both Active and IsSaleable, or null). + + private static readonly Guid GradeCracked = Guid.NewGuid(); + private static readonly Guid GradeDirty = Guid.NewGuid(); + + [Fact] + public void Draft_has_no_quality_snapshots() + { + var entry = MakeDraft(); + entry.RecordProduction(100, 10, 5, 0, 0, [new GradeQuantity(GradeLarge, 85)]); + + Assert.Null(entry.CrackedGradeId); + Assert.Null(entry.DirtyGradeId); + } + + [Fact] + public void Submit_records_the_resolved_quality_grades() + { + var entry = MakeDraft(); + entry.RecordProduction(100, 10, 5, 0, 0, [new GradeQuantity(GradeLarge, 85)]); + + Assert.True(entry.Submit(GradeCracked, GradeDirty).IsSuccess); + + Assert.Equal(GradeCracked, entry.CrackedGradeId); + Assert.Equal(GradeDirty, entry.DirtyGradeId); + } + + [Fact] + public void Submit_records_null_for_a_condition_that_is_a_loss() + { + // Non-saleable, or inactive: either way the caller resolved nothing, + // and null is the durable record that this day's cracked eggs were a + // loss — not an absence of information to be re-derived later. + var entry = MakeDraft(); + entry.RecordProduction(100, 10, 5, 0, 0, [new GradeQuantity(GradeLarge, 85)]); + + entry.Submit(crackedGradeId: null, dirtyGradeId: GradeDirty); + + Assert.Null(entry.CrackedGradeId); + Assert.Equal(GradeDirty, entry.DirtyGradeId); + } + + [Fact] + public void Adjust_keeps_the_original_snapshot_and_never_re_resolves() + { + // The load-bearing one. Submission is the ONLY resolution point: if an + // adjustment re-resolved, a farm that turned Cracked non-saleable (or + // deactivated it) between submit and correction would see a corrected + // day silently stop counting eggs it had already banked as stock — or + // the reverse, minting lots for a day that was recorded as a loss. + var entry = MakeDraft(); + entry.RecordProduction(100, 10, 5, 0, 0, [new GradeQuantity(GradeLarge, 85)]); + entry.Submit(GradeCracked, GradeDirty); + + var result = entry.ManagerAdjust(120, 10, 5, 0, 0, "recount", [new GradeQuantity(GradeLarge, 105)]); + + Assert.True(result.IsSuccess); + Assert.Equal(GradeCracked, entry.CrackedGradeId); + Assert.Equal(GradeDirty, entry.DirtyGradeId); + } + [Fact] public void RecordProduction_OnDraft_Succeeds() { diff --git a/tests/Cluckwork.Domain.Tests/Eggs/EggGradeTests.cs b/tests/Cluckwork.Domain.Tests/Eggs/EggGradeTests.cs new file mode 100644 index 000000000..1802720de --- /dev/null +++ b/tests/Cluckwork.Domain.Tests/Eggs/EggGradeTests.cs @@ -0,0 +1,67 @@ +namespace Cluckwork.Domain.Tests.Eggs; + +using Cluckwork.Domain.Eggs; + +// #396 — a Daily Entry's Cracked and Dirty counters feed a specific grade, and +// which grade that is must survive the farm renaming it. `DailyEntryKind` is +// that stable binding: an identity the code can resolve, as opposed to matching +// on the mutable `Name` (a farm renaming "Cracked" to "Segunda" would otherwise +// silently detach its own counter from its own grade). +public sealed class EggGradeTests +{ + private static EggGrade Make( + string name = "Large", + EggGradeType type = EggGradeType.Size, + bool saleable = true, + DailyEntryKind kind = DailyEntryKind.Manual) => + EggGrade.Create( + Guid.NewGuid(), Guid.NewGuid(), Guid.NewGuid(), name, + type, sortOrder: 0, isSaleable: saleable, dailyEntryKind: kind); + + [Fact] + public void Create_defaults_to_the_manual_kind() + { + // Every ordinary grade is graded by hand in the Grading pane. Only the + // two condition grades are fed by a counter, so Manual is the default + // and a condition kind is always a deliberate act. + Assert.Equal(DailyEntryKind.Manual, Make().DailyEntryKind); + } + + [Theory] + [InlineData(DailyEntryKind.Cracked)] + [InlineData(DailyEntryKind.Dirty)] + public void Create_carries_a_condition_kind(DailyEntryKind kind) => + Assert.Equal(kind, Make("Cracked", EggGradeType.Quality, kind: kind).DailyEntryKind); + + [Fact] + public void Update_cannot_change_the_kind() + { + // Same reasoning as GradeType's immutability, and stronger: an entry + // that already snapshotted this grade as its Cracked identity would be + // reinterpreted by a later kind change. Update takes no kind parameter + // at all — this pins that the signature cannot quietly grow one. + var grade = Make("Cracked", EggGradeType.Quality, kind: DailyEntryKind.Cracked); + + var result = grade.Update("Segunda", sortOrder: 3, isSaleable: false); + + Assert.True(result.IsSuccess); + Assert.Equal("Segunda", grade.Name); + Assert.Equal(DailyEntryKind.Cracked, grade.DailyEntryKind); + } + + [Fact] + public void Deactivate_leaves_saleability_alone() + { + // The trap #397's review round 3 caught, pinned at the source: the two + // flags move independently, so "inactive but still saleable" is an + // ordinary reachable state. Every consumer resolving a condition grade + // must therefore require BOTH, and this test is what makes that + // requirement visible rather than folklore. + var grade = Make("Cracked", EggGradeType.Quality, kind: DailyEntryKind.Cracked); + + Assert.True(grade.Deactivate().IsSuccess); + + Assert.False(grade.Active); + Assert.True(grade.IsSaleable); + } +} From 455b48f9526c33c884f57d85c9a8b3f7f3a7164f Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 22:24:02 -0700 Subject: [PATCH 02/17] feat(eggs): persist condition-grade bindings and entry snapshots (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 1, persistence half. Hand-carried into InitialCreate per #245 — no second migration. The delta was generated with `ef migrations add` against a scratch, read, folded in, and the scratch deleted; both model snapshots were rebuilt so the model and the single migration still agree. Schema: EggGrades."DailyEntryKind" (varchar(16), default 'Manual', stored as a string like GradeType so an enum reorder cannot repoint existing rows), and DailyEntries."CrackedGradeId"/"DirtyGradeId" (nullable uuid). The snapshots take no FK deliberately — they must survive their grade being deleted, exactly as they must survive it being renamed. Index: IX_EggGrades_AccountId_FarmId_DailyEntryKind, unique and PARTIAL (WHERE "DailyEntryKind" <> 'Manual'). One Cracked and one Dirty per farm, since the entry has exactly one counter for each and a second claimant would make resolution silently pick one. The filter is what leaves the many ordinary Manual grades unconstrained, which is also why the column default must stay 'Manual'. Seed: Cracked and Dirty are now saleable by default and carry their kinds; Discarded stays a loss on 'Manual' — binding it would make discarded eggs resolvable to stock. Statement and row counts are unchanged (12 INSERTs, 21 rows), the grades guard stays whole-set and blind to "Name", so MigrationSecurityReviewTests holds. The existing base-data test would have passed with every new flag wrong — it counts ten grades and checks the name index. Two tests added, both asserting through observable behaviour rather than names: - the two condition grades are saleable and bound, Discarded is neither, and the other eight are Manual - a second Cracked is refused by the partial index WHILE an eleventh Manual grade is still accepted — the half a filterless index would break Mutation-checked, baseline green (13/13 migration, 303/303 domain, 0 warnings): seeding Cracked non-saleable turns exactly one test red; dropping the index filter turns the whole class red (a filterless unique index cannot even seed eight Manual grades). --- .../Configurations/DailyEntryConfiguration.cs | 20 ++++++ .../20260801190854_InitialCreate.Designer.cs | 15 ++++ .../20260801190854_InitialCreate.cs | 69 ++++++++++++++---- .../Migrations/AppDbContextModelSnapshot.cs | 15 ++++ .../BaseReferenceDataMigrationTests.cs | 72 +++++++++++++++++++ 5 files changed, 178 insertions(+), 13 deletions(-) diff --git a/src/Cluckwork.Infrastructure/Persistence/Configurations/DailyEntryConfiguration.cs b/src/Cluckwork.Infrastructure/Persistence/Configurations/DailyEntryConfiguration.cs index 4da1e6bb4..9c38220fd 100644 --- a/src/Cluckwork.Infrastructure/Persistence/Configurations/DailyEntryConfiguration.cs +++ b/src/Cluckwork.Infrastructure/Persistence/Configurations/DailyEntryConfiguration.cs @@ -86,6 +86,26 @@ public void Configure(EntityTypeBuilder builder) .HasMaxLength(16) .IsRequired(); + // #396 — stored as a string like GradeType, for the same reason: the + // column stays readable in a psql session and an enum reorder cannot + // silently repoint existing rows to a different kind. + builder.Property(g => g.DailyEntryKind) + .HasConversion() + .HasMaxLength(16) + .IsRequired(); + + // At most ONE Cracked and ONE Dirty grade per farm — the Daily Entry has + // exactly one counter for each, so a second claimant makes "which grade + // does this counter feed" ambiguous, and resolution would silently pick + // one. Filtered so the many ordinary Manual grades are unconstrained. + // + // Scoped per FARM rather than per account: grade rows are farm-owned + // (the case-insensitive name index is per farm too), so two farms in one + // account each get their own Cracked. + builder.HasIndex(g => new { g.AccountId, g.FarmId, g.DailyEntryKind }) + .IsUnique() + .HasFilter("\"DailyEntryKind\" <> 'Manual'"); + builder.Property(g => g.Version).IsConcurrencyToken(); // Name uniqueness is case-insensitive per farm — enforced by a raw diff --git a/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.Designer.cs b/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.Designer.cs index 796be0365..61d75c3a0 100644 --- a/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.Designer.cs +++ b/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.Designer.cs @@ -358,12 +358,18 @@ protected override void BuildTargetModel(ModelBuilder modelBuilder) b.Property("CrackedEggs") .HasColumnType("integer"); + b.Property("CrackedGradeId") + .HasColumnType("uuid"); + b.Property("Date") .HasColumnType("date"); b.Property("DirtyEggs") .HasColumnType("integer"); + b.Property("DirtyGradeId") + .HasColumnType("uuid"); + b.Property("DiscardedEggs") .HasColumnType("integer"); @@ -450,6 +456,11 @@ protected override void BuildTargetModel(ModelBuilder modelBuilder) b.Property("Active") .HasColumnType("boolean"); + b.Property("DailyEntryKind") + .IsRequired() + .HasMaxLength(16) + .HasColumnType("character varying(16)"); + b.Property("FarmId") .HasColumnType("uuid"); @@ -475,6 +486,10 @@ protected override void BuildTargetModel(ModelBuilder modelBuilder) b.HasKey("Id"); + b.HasIndex("AccountId", "FarmId", "DailyEntryKind") + .IsUnique() + .HasFilter("\"DailyEntryKind\" <> 'Manual'"); + b.ToTable("EggGrades"); }); diff --git a/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.cs b/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.cs index 5c2f5b91c..bba994fad 100644 --- a/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.cs +++ b/src/Cluckwork.Infrastructure/Persistence/Migrations/20260801190854_InitialCreate.cs @@ -194,6 +194,15 @@ protected override void Up(MigrationBuilder migrationBuilder) DirtyEggs = table.Column(type: "integer", nullable: false), DiscardedEggs = table.Column(type: "integer", nullable: false), MortalityCount = table.Column(type: "integer", nullable: false), + // #396 — the grade each condition counter resolved to when + // this entry became official, or NULL when that condition was + // a loss. NULL is a RECORDED OUTCOME, not "unknown": a draft + // has not resolved yet, and an official entry that reads NULL + // is stating that those eggs were never stock. Deliberately + // no FK to EggGrades — the snapshot must survive its grade + // being deleted, exactly as it must survive a rename. + CrackedGradeId = table.Column(type: "uuid", nullable: true), + DirtyGradeId = table.Column(type: "uuid", nullable: true), AdjustReason = table.Column(type: "character varying(500)", maxLength: 500, nullable: true), AdjustedFromJson = table.Column(type: "text", nullable: true), VoidReason = table.Column(type: "character varying(500)", maxLength: 500, nullable: true), @@ -236,6 +245,16 @@ protected override void Up(MigrationBuilder migrationBuilder) SortOrder = table.Column(type: "integer", nullable: false), IsSaleable = table.Column(type: "boolean", nullable: false), Active = table.Column(type: "boolean", nullable: false), + // #396 — which Daily Entry counter feeds this grade. Defaults + // to 'Manual' so every ordinary grade, and every row an older + // writer inserts without naming a kind, is a hand-graded one: + // a condition binding is only ever created deliberately. The + // partial unique index below is what keeps a farm from having + // two Cracked grades; the default must therefore stay + // 'Manual', since that value is the one the filter excludes. + DailyEntryKind = table.Column( + type: "character varying(16)", maxLength: 16, nullable: false, + defaultValue: "Manual"), Version = table.Column(type: "integer", nullable: false), AccountId = table.Column(type: "uuid", nullable: false) }, @@ -1311,6 +1330,23 @@ protected override void Up(MigrationBuilder migrationBuilder) table: "WaterUsages", columns: new[] { "FlockId", "Date" }); + // #396 — one Cracked and one Dirty grade per farm. The Daily Entry + // has exactly one counter for each, so a second claimant would make + // "which grade does this counter feed" ambiguous and resolution + // would silently pick one. Partial, so the many ordinary 'Manual' + // grades stay unconstrained — which is also why 'Manual' must remain + // the column default (see the EggGrades table above). + // + // EF CAN model this one (unlike the four lower("Name") expression + // indexes below), so it lives above the hand-carried line and is + // reproduced by a regenerate. + migrationBuilder.CreateIndex( + name: "IX_EggGrades_AccountId_FarmId_DailyEntryKind", + table: "EggGrades", + columns: new[] { "AccountId", "FarmId", "DailyEntryKind" }, + unique: true, + filter: "\"DailyEntryKind\" <> 'Manual'"); + // --------------------------------------------------------------- // Carried forward by hand from the squashed history (#245). // Everything above this line is regenerated from the EF model; @@ -1423,21 +1459,28 @@ CREATE UNIQUE INDEX "IX_Products_AccountId_LowerName" // not reference "Name", so either all ten insert or none do). --- migrationBuilder.Sql( """ - INSERT INTO "EggGrades" ("Id", "AccountId", "FarmId", "Name", "GradeType", "SortOrder", "IsSaleable", "Active", "Version") + INSERT INTO "EggGrades" ("Id", "AccountId", "FarmId", "Name", "GradeType", "SortOrder", "IsSaleable", "DailyEntryKind", "Active", "Version") SELECT v.id::uuid, '0000000a-0000-0000-0000-000000000001', '0000000f-0000-0000-0000-000000000001', - v.name, v.grade_type, v.sort_order, v.is_saleable, TRUE, 0 + v.name, v.grade_type, v.sort_order, v.is_saleable, v.daily_entry_kind, TRUE, 0 FROM (VALUES - ('0000000e-0000-0000-0000-000000000001', 'Small', 'Size', 0, TRUE), - ('0000000e-0000-0000-0000-000000000002', 'Medium', 'Size', 1, TRUE), - ('0000000e-0000-0000-0000-000000000003', 'Large', 'Size', 2, TRUE), - ('0000000e-0000-0000-0000-000000000004', 'Jumbo', 'Size', 3, TRUE), - ('0000000e-0000-0000-0000-000000000005', 'Seconds', 'Quality', 4, TRUE), - ('0000000e-0000-0000-0000-000000000006', 'Cracked', 'Quality', 5, FALSE), - ('0000000e-0000-0000-0000-000000000007', 'Dirty', 'Quality', 6, FALSE), - ('0000000e-0000-0000-0000-000000000008', 'Soft Shell', 'Quality', 7, FALSE), - ('0000000e-0000-0000-0000-000000000009', 'Discarded', 'Custom', 8, FALSE), - ('0000000e-0000-0000-0000-000000000010', 'Internal Use', 'Custom', 9, FALSE) - ) AS v(id, name, grade_type, sort_order, is_saleable) + ('0000000e-0000-0000-0000-000000000001', 'Small', 'Size', 0, TRUE, 'Manual'), + ('0000000e-0000-0000-0000-000000000002', 'Medium', 'Size', 1, TRUE, 'Manual'), + ('0000000e-0000-0000-0000-000000000003', 'Large', 'Size', 2, TRUE, 'Manual'), + ('0000000e-0000-0000-0000-000000000004', 'Jumbo', 'Size', 3, TRUE, 'Manual'), + ('0000000e-0000-0000-0000-000000000005', 'Seconds', 'Quality', 4, TRUE, 'Manual'), + -- #396 — Cracked and Dirty are SALEABLE by default and are + -- the two counter-fed grades. Saleable is the product + -- decision (a farm that does not sell them turns either off + -- in Settings); the kind is the wiring, and is what makes + -- the binding survive a rename. Discarded stays a loss and + -- keeps 'Manual' precisely because no counter resolves to a + -- grade for it — discarded eggs never become stock. + ('0000000e-0000-0000-0000-000000000006', 'Cracked', 'Quality', 5, TRUE, 'Cracked'), + ('0000000e-0000-0000-0000-000000000007', 'Dirty', 'Quality', 6, TRUE, 'Dirty'), + ('0000000e-0000-0000-0000-000000000008', 'Soft Shell', 'Quality', 7, FALSE, 'Manual'), + ('0000000e-0000-0000-0000-000000000009', 'Discarded', 'Custom', 8, FALSE, 'Manual'), + ('0000000e-0000-0000-0000-000000000010', 'Internal Use', 'Custom', 9, FALSE, 'Manual') + ) AS v(id, name, grade_type, sort_order, is_saleable, daily_entry_kind) WHERE NOT EXISTS ( SELECT 1 FROM "EggGrades" WHERE "AccountId" = '0000000a-0000-0000-0000-000000000001' diff --git a/src/Cluckwork.Infrastructure/Persistence/Migrations/AppDbContextModelSnapshot.cs b/src/Cluckwork.Infrastructure/Persistence/Migrations/AppDbContextModelSnapshot.cs index 0a860c8d2..1b7049adb 100644 --- a/src/Cluckwork.Infrastructure/Persistence/Migrations/AppDbContextModelSnapshot.cs +++ b/src/Cluckwork.Infrastructure/Persistence/Migrations/AppDbContextModelSnapshot.cs @@ -355,12 +355,18 @@ protected override void BuildModel(ModelBuilder modelBuilder) b.Property("CrackedEggs") .HasColumnType("integer"); + b.Property("CrackedGradeId") + .HasColumnType("uuid"); + b.Property("Date") .HasColumnType("date"); b.Property("DirtyEggs") .HasColumnType("integer"); + b.Property("DirtyGradeId") + .HasColumnType("uuid"); + b.Property("DiscardedEggs") .HasColumnType("integer"); @@ -447,6 +453,11 @@ protected override void BuildModel(ModelBuilder modelBuilder) b.Property("Active") .HasColumnType("boolean"); + b.Property("DailyEntryKind") + .IsRequired() + .HasMaxLength(16) + .HasColumnType("character varying(16)"); + b.Property("FarmId") .HasColumnType("uuid"); @@ -472,6 +483,10 @@ protected override void BuildModel(ModelBuilder modelBuilder) b.HasKey("Id"); + b.HasIndex("AccountId", "FarmId", "DailyEntryKind") + .IsUnique() + .HasFilter("\"DailyEntryKind\" <> 'Manual'"); + b.ToTable("EggGrades"); }); diff --git a/tests/Cluckwork.Api.IntegrationTests/BaseReferenceDataMigrationTests.cs b/tests/Cluckwork.Api.IntegrationTests/BaseReferenceDataMigrationTests.cs index a715ae925..1dd956f70 100644 --- a/tests/Cluckwork.Api.IntegrationTests/BaseReferenceDataMigrationTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/BaseReferenceDataMigrationTests.cs @@ -1,6 +1,7 @@ namespace Cluckwork.Api.IntegrationTests; using Cluckwork.Domain.Accounts; +using Cluckwork.Domain.Eggs; using Cluckwork.Infrastructure.Persistence; using Cluckwork.Infrastructure.Persistence.Interceptors; using Cluckwork.Infrastructure.Providers; @@ -101,4 +102,75 @@ public async Task MigratingAVirginDatabase_CreatesTheCaseInsensitiveUniqueIndexe Assert.NotNull(duplicate); Assert.Contains("IX_EggGrades_AccountId_FarmId_LowerName", duplicate.ToString(), StringComparison.Ordinal); } + + // #396 — the product decision ("Cracked and Dirty are saleable on a fresh + // install, and each is bound to its counter") lives entirely in the seed + // SQL, where nothing else would notice it being reverted: the sibling test + // above counts ten grades and would still pass with every flag wrong. + [Fact] + public async Task MigratingAVirginDatabase_MakesTheTwoConditionGradesSaleableAndBound() + { + await using var postgres = new PostgreSqlBuilder(PostgresImage).Build(); + await postgres.StartAsync(); + await using var db = BuildContext(postgres.GetConnectionString()); + + await db.Database.MigrateAsync(); + + var grades = await db.EggGrades.IgnoreQueryFilters() + .Where(g => g.AccountId == SeedDefaults.AccountId && g.FarmId == SeedDefaults.FarmId) + .ToListAsync(); + + var cracked = Assert.Single(grades, g => g.DailyEntryKind == DailyEntryKind.Cracked); + var dirty = Assert.Single(grades, g => g.DailyEntryKind == DailyEntryKind.Dirty); + + Assert.Equal("Cracked", cracked.Name); + Assert.Equal("Dirty", dirty.Name); + Assert.True(cracked.IsSaleable, "Cracked must be saleable on a fresh install (#396)."); + Assert.True(dirty.IsSaleable, "Dirty must be saleable on a fresh install (#396)."); + + // Discarded is always a loss, so it must NOT be bound to a counter — + // binding it would make discarded eggs resolvable to stock. + Assert.Equal( + DailyEntryKind.Manual, + Assert.Single(grades, g => g.Name == "Discarded").DailyEntryKind); + Assert.False(Assert.Single(grades, g => g.Name == "Discarded").IsSaleable); + + // Every other seeded grade is hand-graded. + Assert.Equal(8, grades.Count(g => g.DailyEntryKind == DailyEntryKind.Manual)); + } + + // The partial unique index, proved the same way as the expression indexes + // above — by the behaviour it exists to produce, not by its name. A name + // check passes against an index created without its WHERE clause, which + // would instead forbid a farm from having more than one Manual grade. + [Fact] + public async Task MigratingAVirginDatabase_AllowsManyManualGradesButOnlyOneOfEachCondition() + { + await using var postgres = new PostgreSqlBuilder(PostgresImage).Build(); + await postgres.StartAsync(); + await using var db = BuildContext(postgres.GetConnectionString()); + + await db.Database.MigrateAsync(); + + // A second Cracked for the same farm is refused. + var secondCracked = await Record.ExceptionAsync(() => db.Database.ExecuteSqlAsync($""" + INSERT INTO "EggGrades" ("Id", "AccountId", "FarmId", "Name", "GradeType", "SortOrder", "IsSaleable", "DailyEntryKind", "Active", "Version") + VALUES ({Guid.NewGuid()}, {SeedDefaults.AccountId}, {SeedDefaults.FarmId}, 'Cracked 2', 'Quality', 98, TRUE, 'Cracked', TRUE, 0) + """)); + + Assert.NotNull(secondCracked); + Assert.Contains( + "IX_EggGrades_AccountId_FarmId_DailyEntryKind", + secondCracked.ToString(), + StringComparison.Ordinal); + + // ...but an eleventh Manual grade is fine. This is the half a filterless + // index would break, and it is why the column default must stay 'Manual'. + var anotherManual = await Record.ExceptionAsync(() => db.Database.ExecuteSqlAsync($""" + INSERT INTO "EggGrades" ("Id", "AccountId", "FarmId", "Name", "GradeType", "SortOrder", "IsSaleable", "DailyEntryKind", "Active", "Version") + VALUES ({Guid.NewGuid()}, {SeedDefaults.AccountId}, {SeedDefaults.FarmId}, 'Peewee', 'Size', 97, TRUE, 'Manual', TRUE, 0) + """)); + + Assert.Null(anotherManual); + } } From 9b116162e690cdec73a08099e57e6ca250a87d66 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 22:26:03 -0700 Subject: [PATCH 03/17] feat(export): carry the condition snapshots and grade binding into exports (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes Task 1. An export is the farm's own copy of its records, and without these columns it states that a day had N cracked eggs while omitting whether they became saleable stock or a loss — which is NOT recoverable afterwards, since the snapshot exists precisely because the grade catalog may have changed since. daily-entries gains crackedGradeId/dirtyGradeId next to the counters they explain; egg-grades gains dailyEntryKind, the only field naming which counter a grade serves and the only one that survives a rename. ExportTests previously asserted dataset names and content types only, so both projections could have been left out silently. The new test checks the header AND a value — a header-only assertion passes against a column wired to the wrong property. Mutation-checked, baseline green (6/6 export, 13/13 migration, 0 warnings): dropping dailyEntryKind from the projection turns exactly that test red. --- .../Repositories/ExportQueries.cs | 17 +++++++-- .../ExportTests.cs | 35 +++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/Cluckwork.Infrastructure/Repositories/ExportQueries.cs b/src/Cluckwork.Infrastructure/Repositories/ExportQueries.cs index 848ef96db..8326541ed 100644 --- a/src/Cluckwork.Infrastructure/Repositories/ExportQueries.cs +++ b/src/Cluckwork.Infrastructure/Repositories/ExportQueries.cs @@ -120,11 +120,19 @@ private sealed class AsyncDisposableAction(Func action) : IAsyncDispo "daily-entries" => Rows(activeDb.DailyEntries.AsNoTracking() .OrderBy(x => x.Date).ThenBy(x => x.Id), + // #396 — the two snapshot ids ride next to the counters they + // explain. Without them an export records that a day had 40 + // cracked eggs but not whether those became saleable stock or a + // loss, and that is no longer derivable from the current + // catalog: the whole point of the snapshot is that the catalog + // may have changed since. ["id", "farmId", "houseId", "flockId", "date", "status", "totalEggs", "crackedEggs", "dirtyEggs", "discardedEggs", "mortalityCount", + "crackedGradeId", "dirtyGradeId", "adjustReason", "adjustedFromJson", "voidReason", "lockedAtUtc", "version"], x => [x.Id, x.FarmId, x.HouseId, x.FlockId, x.Date, x.Status, x.TotalEggs, x.CrackedEggs, x.DirtyEggs, x.DiscardedEggs, x.MortalityCount, + x.CrackedGradeId, x.DirtyGradeId, x.AdjustReason, x.AdjustedFromJson, x.VoidReason, x.LockedAtUtc, x.Version]), "daily-entry-grades" => Rows(activeDb.DailyEntryGrades.AsNoTracking() @@ -134,8 +142,13 @@ private sealed class AsyncDisposableAction(Func action) : IAsyncDispo "egg-grades" => Rows(activeDb.EggGrades.AsNoTracking() .OrderBy(x => x.SortOrder).ThenBy(x => x.Id), - ["id", "farmId", "name", "gradeType", "sortOrder", "isSaleable", "active", "version"], - x => [x.Id, x.FarmId, x.Name, x.GradeType, x.SortOrder, x.IsSaleable, x.Active, x.Version]), + // #396 — dailyEntryKind is what makes the snapshot ids above + // interpretable: it is the only field saying WHICH counter a + // grade serves, and unlike the name it cannot be edited. + ["id", "farmId", "name", "gradeType", "sortOrder", "isSaleable", + "dailyEntryKind", "active", "version"], + x => [x.Id, x.FarmId, x.Name, x.GradeType, x.SortOrder, x.IsSaleable, + x.DailyEntryKind, x.Active, x.Version]), "egg-lots" => Rows(activeDb.EggLots.AsNoTracking() .OrderBy(x => x.ProductionDate).ThenBy(x => x.Id), diff --git a/tests/Cluckwork.Api.IntegrationTests/ExportTests.cs b/tests/Cluckwork.Api.IntegrationTests/ExportTests.cs index c36ac594d..ed052d8f2 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ExportTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ExportTests.cs @@ -34,6 +34,41 @@ private sealed record Created(Guid Id); return (client, accountId, farmId, flockId); } + // #396 — an export is the farm's own copy of its records, so it must stay + // self-describing. Without these two columns it says a day had N cracked + // eggs but not whether they became stock or a loss, and that is NOT + // recoverable from the grade catalog afterwards — the snapshot exists + // precisely because the catalog can change. dailyEntryKind is the other + // half: the only field naming which counter a grade serves, and the only + // one that survives a rename. + [Fact] + public async Task Export_CarriesTheConditionSnapshotsAndTheGradeBinding() + { + var (client, accountId, farmId, _) = await SetupAsync(); + + await factory.WithTenantScopeAsync(accountId, async db => + { + db.EggGrades.Add(Domain.Eggs.EggGrade.Create( + Guid.NewGuid(), accountId, farmId, "Cracked", + Domain.Eggs.EggGradeType.Quality, 50, isSaleable: true, + dailyEntryKind: Domain.Eggs.DailyEntryKind.Cracked)); + await db.SaveChangesAsync(); + }); + + var gradesCsv = await (await client.GetAsync("/api/v1/export/egg-grades")) + .Content.ReadAsStringAsync(); + var entriesHeader = (await (await client.GetAsync("/api/v1/export/daily-entries")) + .Content.ReadAsStringAsync()).Split('\n')[0]; + + // Header, then the value — a header-only check passes against a column + // wired to the wrong property. + Assert.Contains("dailyEntryKind", gradesCsv.Split('\n')[0], StringComparison.Ordinal); + Assert.Contains("Cracked", gradesCsv, StringComparison.Ordinal); + + Assert.Contains("crackedGradeId", entriesHeader, StringComparison.Ordinal); + Assert.Contains("dirtyGradeId", entriesHeader, StringComparison.Ordinal); + } + [Fact] public async Task Export_IsAdminOnly_UnknownDatasetIs404() { From 87ec4c7ef5f1a1e59f38ac9bdd5dbfef1614cbf2 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 22:37:59 -0700 Subject: [PATCH 04/17] feat(daily-entry): refuse a condition grade named as a manual line (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 2, first slice — the hole this feature opens, closed before the code that opens it. Making Cracked and Dirty saleable makes them pass the manual-line eligibility check, which asks "active and saleable" with no kind restriction. So a direct or stale API client could name a Cracked id as a graded line; #394's exact-total check does not catch it, because that check is only a sum and the sum still reconciles. Submission would then create the manual lot AND the counter-backed quality lot for the same grade — two lots for one grade on one day, double-counting stock and breaking the one-lot-per-grade assumption lot reconciliation depends on. Enforced in the handlers, not a validator (#395/#394): the demo and simulation seeders construct commands and call HandleAsync directly, skipping validators entirely, so a validator-only rule would let a fixture build exactly this state. ConditionGradeGuard is deliberately NOT folded into either handler's existing allow-set. The two ask different questions, and adjust's set additionally unions the lines already on the entry, so a guard sharing it could be talked past. Tests assert the durable state, not the status code: a refused record leaves no entry at all, and a refused adjust leaves status, version and lot count byte-identical — adjust is the path that has already generated lots, so a partial failure there is worse than on record. Mutation-checked with a rebuild after each restore (a --no-build confirmation re-runs the mutant and can report a false green): removing the guard from record turns both record tests red, removing it from adjust turns the adjust test red. Baseline green — 34/34 daily-entry integration, 114/114 application, 0 warnings. One comment corrected rather than left overstated: the guard reads the whole catalog, and I had written that as though it closed a reachable gap. It does not today — an inactive grade is already refused upstream, so narrowing it would change the error code, not the outcome. The comment now says that plainly and says why the shape is still worth keeping. --- .../AdjustDailyEntryHandler.cs | 9 + .../DailyEntries/ConditionGradeGuard.cs | 61 +++++++ .../RecordDailyEntryHandler.cs | 9 + .../QualityEggTests.cs | 154 ++++++++++++++++++ 4 files changed, 233 insertions(+) create mode 100644 src/Cluckwork.Application/Features/DailyEntries/ConditionGradeGuard.cs create mode 100644 tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs diff --git a/src/Cluckwork.Application/Features/DailyEntries/AdjustDailyEntry/AdjustDailyEntryHandler.cs b/src/Cluckwork.Application/Features/DailyEntries/AdjustDailyEntry/AdjustDailyEntryHandler.cs index 577d598ca..26950d0e0 100644 --- a/src/Cluckwork.Application/Features/DailyEntries/AdjustDailyEntry/AdjustDailyEntryHandler.cs +++ b/src/Cluckwork.Application/Features/DailyEntries/AdjustDailyEntry/AdjustDailyEntryHandler.cs @@ -74,6 +74,15 @@ public async Task> HandleAsync( "DailyEntry.UnknownGrade", "One or more egg grades do not exist, are inactive, or are not saleable.")) .LogFailure(logger, "AdjustDailyEntry"); + + // #396 — deliberately NOT folded into `allowed` above: that set + // unions the lines already on the entry, so a condition grade could + // be talked past it. ConditionGradeGuard asks the catalog directly. + var conditionGrade = await ConditionGradeGuard.CheckAsync( + eggGrades, command.Grades.Select(g => g.EggGradeId), ct); + if (conditionGrade is not null) + return Result.Failure(conditionGrade) + .LogFailure(logger, "AdjustDailyEntry"); } var previousMortality = entry.MortalityCount; diff --git a/src/Cluckwork.Application/Features/DailyEntries/ConditionGradeGuard.cs b/src/Cluckwork.Application/Features/DailyEntries/ConditionGradeGuard.cs new file mode 100644 index 000000000..4e2acca1f --- /dev/null +++ b/src/Cluckwork.Application/Features/DailyEntries/ConditionGradeGuard.cs @@ -0,0 +1,61 @@ +namespace Cluckwork.Application.Features.DailyEntries; + +using Cluckwork.Application.Features.EggGrades; +using Cluckwork.Domain.Common; +using Cluckwork.Domain.Eggs; + +// #396 — a condition grade is fed by its Daily Entry COUNTER and never by a +// manual grade line. +// +// Hiding those grades from the Grading pane is a UI affordance, not the +// enforcement. The eligibility check a manual line passes is "active and +// saleable" with NO kind restriction, and this feature makes Cracked and Dirty +// saleable — so a direct or stale API client can name a Cracked id as a manual +// line. The exact-total check (#394) does not catch it, because that check is +// only a sum and the sum still reconciles. Submission would then create the +// manual lot AND the counter-backed quality lot for the same grade: two lots for +// one grade on one day, double-counting the day's stock and breaking the +// one-lot-per-grade assumption lot reconciliation depends on. +// +// Lives in the handlers rather than in a validator on purpose (#394): the demo +// and simulation seeders construct commands and call HandleAsync directly, +// skipping validators entirely, so a validator-only rule would let a fixture +// build exactly the state this refuses. +internal static class ConditionGradeGuard +{ + internal const string ErrorCode = "DailyEntry.ConditionGradeNotManual"; + + // Reads the WHOLE catalog (ListAllAsync) rather than the active+saleable + // slice the callers already fetch, because the question here is "is this id + // bound to a counter", which does not depend on the grade's current flags. + // + // Stated at the strength the evidence supports: this is DEFENSIVE, not a + // fix for a reachable bug. Today an inactive grade is already refused + // upstream by the UnknownGrade check, so narrowing this to ListActiveAsync + // would change the error code a caller sees, not whether the write is + // refused — no test distinguishes them and none is claimed to. It is written + // this way so the guard keeps answering the same question if either + // upstream check is later relaxed, which is exactly the kind of coupling + // that turns a refusal into an acceptance without anything going red. + internal static async Task CheckAsync( + IEggGradeRepository eggGrades, + IEnumerable lineGradeIds, + CancellationToken ct) + { + var ids = lineGradeIds as IReadOnlyCollection ?? lineGradeIds.ToList(); + if (ids.Count == 0) return null; + + var conditionGradeIds = (await eggGrades.ListAllAsync(ct)) + .Where(g => g.DailyEntryKind != DailyEntryKind.Manual) + .Select(g => g.Id) + .ToHashSet(); + + return ids.Any(conditionGradeIds.Contains) + ? Error.Validation( + ErrorCode, + "Cracked and Dirty eggs are captured by their own counters, not as graded lines. " + + "Remove the condition grade from the grading breakdown and use the Cracked or " + + "Dirty count instead.") + : null; + } +} diff --git a/src/Cluckwork.Application/Features/DailyEntries/RecordDailyEntry/RecordDailyEntryHandler.cs b/src/Cluckwork.Application/Features/DailyEntries/RecordDailyEntry/RecordDailyEntryHandler.cs index 9effa001e..2914d3843 100644 --- a/src/Cluckwork.Application/Features/DailyEntries/RecordDailyEntry/RecordDailyEntryHandler.cs +++ b/src/Cluckwork.Application/Features/DailyEntries/RecordDailyEntry/RecordDailyEntryHandler.cs @@ -64,6 +64,15 @@ public async Task> HandleAsync( "DailyEntry.UnknownGrade", "One or more egg grades do not exist, are inactive, or are not saleable.")) .LogFailure(logger, "RecordDailyEntry"); + + // #396 — and it must not be a COUNTER-fed grade. This check is + // separate from `known` above because the two ask different + // questions: `known` is "may this grade receive production at all", + // which Cracked and Dirty now pass. See ConditionGradeGuard. + var conditionGrade = await ConditionGradeGuard.CheckAsync( + eggGrades, command.Grades.Select(g => g.EggGradeId), ct); + if (conditionGrade is not null) + return Result.Failure(conditionGrade).LogFailure(logger, "RecordDailyEntry"); } var existing = await repository.FindByNaturalKeyAsync( diff --git a/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs b/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs new file mode 100644 index 000000000..8ad2f974e --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs @@ -0,0 +1,154 @@ +namespace Cluckwork.Api.IntegrationTests; + +using System.Net; +using Cluckwork.Api.IntegrationTests.Infrastructure; +using Cluckwork.Domain.Eggs; +using Microsoft.EntityFrameworkCore; + +// #396 — Cracked and Dirty become saleable stock through their own counters. +// +// The rule these tests exist to hold: a condition grade is fed by its COUNTER +// and never by a manual grade line. Excluding it from the Grading pane is a UI +// affordance; the enforcement has to be server-side, because the eligibility +// check a manual line passes is "active and saleable" with no kind restriction — +// and this feature makes Cracked and Dirty saleable. So a direct or stale API +// client can name a Cracked id as a manual line, the exact-total check passes +// (it is only a sum), and submission then creates BOTH that manual lot and the +// counter-backed one: two lots for one grade on one day, double-counting the +// day's stock and breaking the one-lot-per-grade assumption reconciliation +// depends on. +[Collection(IntegrationCollection.Name)] +public sealed class QualityEggTests(CluckworkWebApplicationFactory factory) +{ + private sealed record RecordedDto(Guid Id); + + private static object Body(Guid farmId, Guid flockId, object[] grades, int total = 618) => new + { + farmId, + houseId = Guid.NewGuid(), + flockId, + date = DateOnly.FromDateTime(DateTime.UtcNow.Date), + totalEggs = total, + crackedEggs = 10, + dirtyEggs = 5, + discardedEggs = 3, + mortalityCount = 0, + grades + }; + + private async Task<(HttpClient Client, Guid AccountId, Guid FarmId, Guid FlockId, + Dictionary Manual, Guid CrackedId, Guid DirtyId)> SetupAsync( + bool crackedSaleable = true, bool crackedActive = true) + { + var email = $"u-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var farmId = Guid.NewGuid(); + var manual = await factory.SeedEggGradesAsync(accountId, farmId, "Large"); + var flockId = await factory.SeedFlockAsync(accountId, farmId); + + var crackedId = Guid.NewGuid(); + var dirtyId = Guid.NewGuid(); + await factory.WithTenantScopeAsync(accountId, async db => + { + var cracked = EggGrade.Create( + crackedId, accountId, farmId, "Cracked", EggGradeType.Quality, + sortOrder: 50, isSaleable: crackedSaleable, + dailyEntryKind: DailyEntryKind.Cracked); + if (!crackedActive) cracked.Deactivate(); + db.EggGrades.Add(cracked); + + db.EggGrades.Add(EggGrade.Create( + dirtyId, accountId, farmId, "Dirty", EggGradeType.Quality, + sortOrder: 51, isSaleable: true, + dailyEntryKind: DailyEntryKind.Dirty)); + await db.SaveChangesAsync(); + }); + + var client = factory.CreateAuthedClient(await factory.LoginForAccessTokenAsync(email)); + return (client, accountId, farmId, flockId, manual, crackedId, dirtyId); + } + + [Fact] + public async Task Recording_a_manual_line_naming_a_condition_grade_is_refused() + { + var (client, _, farmId, flockId, _, crackedId, _) = await SetupAsync(); + + // 600 named against the CRACKED grade as a manual line. The sum still + // reconciles (600 + 10 + 5 + 3 = 618), which is exactly why the total + // check cannot catch this. + var response = await client.PostWithKeyAsync( + "/api/v1/daily-entries", Guid.NewGuid().ToString(), + Body(farmId, flockId, [new { eggGradeId = crackedId, quantity = 600 }])); + + Assert.Equal(HttpStatusCode.UnprocessableEntity, response.StatusCode); + Assert.Contains("ConditionGradeNotManual", await response.Content.ReadAsStringAsync(), StringComparison.Ordinal); + } + + [Fact] + public async Task A_refused_condition_line_writes_nothing() + { + var (client, accountId, farmId, flockId, _, crackedId, _) = await SetupAsync(); + + await client.PostWithKeyAsync( + "/api/v1/daily-entries", Guid.NewGuid().ToString(), + Body(farmId, flockId, [new { eggGradeId = crackedId, quantity = 600 }])); + + // Not "the response was 422" — the durable state. A refusal that still + // created a draft would leave the day half-recorded under a grade the + // farm cannot see in its Grading pane. + await factory.WithTenantScopeAsync(accountId, async db => + Assert.Empty(await db.DailyEntries.IgnoreQueryFilters() + .Where(e => e.FlockId == flockId).ToListAsync())); + } + + [Fact] + public async Task Adjusting_onto_a_condition_grade_is_refused_and_leaves_the_entry_intact() + { + var (client, accountId, farmId, flockId, manual, crackedId, _) = await SetupAsync(); + + var entryId = (await (await client.PostWithKeyAsync( + "/api/v1/daily-entries", Guid.NewGuid().ToString(), + Body(farmId, flockId, [new { eggGradeId = manual["Large"], quantity = 600 }]))) + .Content.ReadFromJsonAsync())!.Id; + + await client.PostWithKeyAsync($"/api/v1/daily-entries/{entryId}/submit", Guid.NewGuid().ToString()); + + var before = await StateOf(accountId, entryId); + + var response = await client.PostWithKeyAsync( + $"/api/v1/daily-entries/{entryId}/adjust", Guid.NewGuid().ToString(), + new + { + // Adjust carries the entry's optimistic version. Sent correctly + // so the refusal under test is the condition-grade guard and not + // a 409 the request would have earned anyway. + version = before.Version, + totalEggs = 618, + crackedEggs = 10, + dirtyEggs = 5, + discardedEggs = 3, + mortalityCount = 0, + reason = "recount", + grades = new[] { new { eggGradeId = crackedId, quantity = 600 } } + }); + + Assert.Equal(HttpStatusCode.UnprocessableEntity, response.StatusCode); + + // Adjust is the path that already generated lots, so a partial failure + // here is worse than on record: the entry must keep its status, its + // version, and its lot set exactly. + Assert.Equal(before, await StateOf(accountId, entryId)); + } + + private async Task<(string Status, int Version, int Lots)> StateOf(Guid accountId, Guid entryId) + { + (string, int, int) state = default; + await factory.WithTenantScopeAsync(accountId, async db => + { + var entry = await db.DailyEntries.IgnoreQueryFilters().SingleAsync(e => e.Id == entryId); + var lots = await db.EggLots.IgnoreQueryFilters().CountAsync(l => l.DailyEntryId == entryId); + state = (entry.Status.ToString(), entry.Version, lots); + }); + return state; + } +} From 37ed412bf1105539343734574dac051e65d2ccd0 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 22:41:27 -0700 Subject: [PATCH 05/17] feat(daily-entry): resolve condition grades at submit and mint their lots (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes Task 2. Submission is the one point at which the Cracked and Dirty counters resolve to a grade, and the resolution requires BOTH Active and IsSaleable. Neither flag implies the other. EggGrade.Deactivate() leaves IsSaleable set, so "inactive but saleable" is an ordinary reachable state — an owner retiring a condition grade without first clearing its saleability — and resolving on saleability alone would mint stock under a grade the farm has already removed from capture. That was #397's review round 3 finding, stated there as prose; it is now pinned by a test that goes red when the Active half is dropped. Unresolved is recorded as NULL, which is a durable outcome ("these eggs were a loss") rather than a gap to be filled in later. The lot loop reads the entry's own SNAPSHOT rather than the ids just resolved, so this loop and every later reader agree by construction and an adjustment — which never re-resolves — takes the identical path. A zero counter still snapshots but mints no lot: EggLot.Create rejects a zero quantity, and "the farm was selling cracked eggs that day" remains a fact worth recording. Snapshot and lot are separate decisions, and that test is what proves they are. Worth recording about the tests themselves: the two "resolves to nothing" cases passed VACUOUSLY before this commit, because nothing resolved at all. They only became meaningful once resolution existed, which is why each was mutation-checked against the specific rule it covers rather than trusted for being green. Mutation-checked, rebuilding after every restore, baseline green (44/44 integration, 303/303 domain, 114/114 application, 0 warnings): - resolving from the full catalog instead of the active slice → the inactive-but-saleable test alone goes red - dropping the IsSaleable requirement → the non-saleable test alone goes red - dropping the zero-counter skip → the zero-counter test alone goes red --- .../SubmitDailyEntryHandler.cs | 52 +++++++- .../QualityEggTests.cs | 118 ++++++++++++++++++ 2 files changed, 169 insertions(+), 1 deletion(-) diff --git a/src/Cluckwork.Application/Features/DailyEntries/SubmitDailyEntry/SubmitDailyEntryHandler.cs b/src/Cluckwork.Application/Features/DailyEntries/SubmitDailyEntry/SubmitDailyEntryHandler.cs index 9269f1146..02ffa3a7a 100644 --- a/src/Cluckwork.Application/Features/DailyEntries/SubmitDailyEntry/SubmitDailyEntryHandler.cs +++ b/src/Cluckwork.Application/Features/DailyEntries/SubmitDailyEntry/SubmitDailyEntryHandler.cs @@ -1,6 +1,7 @@ namespace Cluckwork.Application.Features.DailyEntries.SubmitDailyEntry; using Cluckwork.Application.Common; +using Cluckwork.Application.Features.EggGrades; using Cluckwork.Application.Features.EggLots; using Cluckwork.Application.Features.Eggs; using Cluckwork.Application.Features.Flocks; @@ -18,6 +19,7 @@ namespace Cluckwork.Application.Features.DailyEntries.SubmitDailyEntry; // are never generated twice. public sealed class SubmitDailyEntryHandler( IDailyEntryRepository entries, + IEggGradeRepository eggGrades, IEggLotRepository eggLots, IBirdMovementRepository birdMovements, IEggInventoryMovementRepository eggMovements, @@ -51,7 +53,19 @@ public async Task> HandleAsync( if (scope.IsFailure) return Result.Failure(scope.Error).LogFailure(logger, "SubmitDailyEntry"); - var submit = entry.Submit(); + // #396 — submission is the ONE point at which the Cracked and Dirty + // counters resolve to a grade. Both flags are required and neither + // implies the other: EggGrade.Deactivate() leaves IsSaleable set, so + // "inactive but saleable" is an ordinary reachable state, and resolving + // on saleability alone would mint stock under a grade the farm has + // already retired from capture. Unresolved (null) is a durable record + // that those eggs were a loss, not a gap to be filled in later — see + // DailyEntry.CrackedGradeId. + var farmGrades = await eggGrades.ListActiveAsync(entry.FarmId, ct); + var crackedGradeId = ResolveCondition(farmGrades, DailyEntryKind.Cracked); + var dirtyGradeId = ResolveCondition(farmGrades, DailyEntryKind.Dirty); + + var submit = entry.Submit(crackedGradeId, dirtyGradeId); if (submit.IsFailure) return Result.Failure(submit.Error).LogFailure(logger, "SubmitDailyEntry"); @@ -73,6 +87,35 @@ await eggMovements.AddAsync(EggInventoryMovement.Create( line.Quantity, nameof(DailyEntry), entry.Id, clock.UtcNow), ct); } + // #396 — the counter-backed lots, in the same transaction as the manual + // ones. Read from the entry's own SNAPSHOT rather than from the ids + // resolved above, so this loop and every later reader agree by + // construction and an adjustment (which never re-resolves) takes the + // identical path. + // + // A zero counter is skipped but stays snapshotted: EggLot.Create rejects + // a zero quantity, and "the farm was selling cracked eggs that day" is + // still a fact worth recording. Snapshot and lot are separate decisions. + foreach (var (gradeId, quantity) in new[] + { + (entry.CrackedGradeId, entry.CrackedEggs), + (entry.DirtyGradeId, entry.DirtyEggs), + }) + { + if (gradeId is null || quantity <= 0) continue; + + var lot = EggLot.Create( + Guid.NewGuid(), accountId, entry.FlockId, + entry.Date, gradeId.Value, quantity, + dailyEntryId: entry.Id); + await eggLots.AddAsync(lot, ct); + lotIds.Add(lot.Id); + + await eggMovements.AddAsync(EggInventoryMovement.Create( + Guid.NewGuid(), accountId, lot.Id, EggMovementType.Production, + quantity, nameof(DailyEntry), entry.Id, clock.UtcNow), ct); + } + // The day's mortality becomes a ledger row so the flock's current count // reflects it. Zero-mortality days write nothing. if (entry.MortalityCount > 0) @@ -94,6 +137,13 @@ await birdMovements.AddAsync(BirdMovement.Create( entry.Id, entry.FlockId, entry.Date, lotIds.Count, entry.MortalityCount); return Result.Success(new SubmitDailyEntryResponse(entry.Id, entry.Status.ToString(), lotIds)); } + + // BOTH flags, never one. ListActiveAsync has already excluded inactive + // grades; IsSaleable is checked here, and the pair is what the rule + // requires. Kept as a named method so the requirement reads as one thing + // rather than as an incidental filter clause. + private static Guid? ResolveCondition(IReadOnlyList activeFarmGrades, DailyEntryKind kind) => + activeFarmGrades.SingleOrDefault(g => g.DailyEntryKind == kind && g.IsSaleable)?.Id; } public sealed record SubmitDailyEntryResponse(Guid Id, string Status, IReadOnlyList EggLotIds); diff --git a/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs b/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs index 8ad2f974e..a214b9715 100644 --- a/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs @@ -140,6 +140,124 @@ public async Task Adjusting_onto_a_condition_grade_is_refused_and_leaves_the_ent Assert.Equal(before, await StateOf(accountId, entryId)); } + // ---- resolution and lot creation ----------------------------------- + + private async Task SubmitAsync(HttpClient client, Guid farmId, Guid flockId, Guid manualGradeId) + { + var entryId = (await (await client.PostWithKeyAsync( + "/api/v1/daily-entries", Guid.NewGuid().ToString(), + Body(farmId, flockId, [new { eggGradeId = manualGradeId, quantity = 600 }]))) + .Content.ReadFromJsonAsync())!.Id; + + var submit = await client.PostWithKeyAsync( + $"/api/v1/daily-entries/{entryId}/submit", Guid.NewGuid().ToString()); + Assert.Equal(HttpStatusCode.OK, submit.StatusCode); + return entryId; + } + + [Fact] + public async Task Submitting_turns_a_saleable_condition_counter_into_its_own_lot() + { + var (client, accountId, farmId, flockId, manual, crackedId, dirtyId) = await SetupAsync(); + + var entryId = await SubmitAsync(client, farmId, flockId, manual["Large"]); + + await factory.WithTenantScopeAsync(accountId, async db => + { + var entry = await db.DailyEntries.IgnoreQueryFilters().SingleAsync(e => e.Id == entryId); + Assert.Equal(crackedId, entry.CrackedGradeId); + Assert.Equal(dirtyId, entry.DirtyGradeId); + + var lots = await db.EggLots.IgnoreQueryFilters() + .Where(l => l.DailyEntryId == entryId).ToListAsync(); + + // 600 manual + 10 cracked + 5 dirty. Discarded is never stock. + Assert.Equal(3, lots.Count); + Assert.Equal(10, Assert.Single(lots, l => l.EggGradeId == crackedId).QuantityProduced); + Assert.Equal(5, Assert.Single(lots, l => l.EggGradeId == dirtyId).QuantityProduced); + }); + } + + [Fact] + public async Task A_non_saleable_condition_stays_a_loss() + { + var (client, accountId, farmId, flockId, manual, crackedId, _) = + await SetupAsync(crackedSaleable: false); + + var entryId = await SubmitAsync(client, farmId, flockId, manual["Large"]); + + await factory.WithTenantScopeAsync(accountId, async db => + { + var entry = await db.DailyEntries.IgnoreQueryFilters().SingleAsync(e => e.Id == entryId); + Assert.Null(entry.CrackedGradeId); + Assert.Empty(await db.EggLots.IgnoreQueryFilters() + .Where(l => l.DailyEntryId == entryId && l.EggGradeId == crackedId).ToListAsync()); + }); + } + + // The case a saleability-only rule gets wrong. EggGrade.Deactivate() leaves + // IsSaleable set, so "inactive but saleable" is an ordinary reachable state + // — reached by an owner retiring a condition grade without first clearing + // its saleability. Resolving on IsSaleable alone would mint stock under a + // grade the farm has already removed from capture. + [Fact] + public async Task An_inactive_but_saleable_condition_grade_resolves_to_nothing() + { + var (client, accountId, farmId, flockId, manual, crackedId, _) = + await SetupAsync(crackedSaleable: true, crackedActive: false); + + var entryId = await SubmitAsync(client, farmId, flockId, manual["Large"]); + + await factory.WithTenantScopeAsync(accountId, async db => + { + var grade = await db.EggGrades.IgnoreQueryFilters().SingleAsync(g => g.Id == crackedId); + Assert.False(grade.Active); + Assert.True(grade.IsSaleable, "the fixture must keep the trap: inactive yet still saleable"); + + var entry = await db.DailyEntries.IgnoreQueryFilters().SingleAsync(e => e.Id == entryId); + Assert.Null(entry.CrackedGradeId); + Assert.Empty(await db.EggLots.IgnoreQueryFilters() + .Where(l => l.DailyEntryId == entryId && l.EggGradeId == crackedId).ToListAsync()); + }); + } + + // A zero counter still RESOLVES — the snapshot records that the farm was + // selling cracked eggs that day — but mints no lot, because EggLot.Create + // rejects a zero quantity. Snapshot and lot are separate decisions and this + // is the case that proves it. + [Fact] + public async Task A_zero_counter_is_snapshotted_but_creates_no_lot() + { + var (client, accountId, farmId, flockId, manual, crackedId, _) = await SetupAsync(); + + var entryId = (await (await client.PostWithKeyAsync( + "/api/v1/daily-entries", Guid.NewGuid().ToString(), + new + { + farmId, + houseId = Guid.NewGuid(), + flockId, + date = DateOnly.FromDateTime(DateTime.UtcNow.Date), + totalEggs = 608, + crackedEggs = 0, + dirtyEggs = 5, + discardedEggs = 3, + mortalityCount = 0, + grades = new[] { new { eggGradeId = manual["Large"], quantity = 600 } } + })).Content.ReadFromJsonAsync())!.Id; + + Assert.Equal(HttpStatusCode.OK, (await client.PostWithKeyAsync( + $"/api/v1/daily-entries/{entryId}/submit", Guid.NewGuid().ToString())).StatusCode); + + await factory.WithTenantScopeAsync(accountId, async db => + { + var entry = await db.DailyEntries.IgnoreQueryFilters().SingleAsync(e => e.Id == entryId); + Assert.Equal(crackedId, entry.CrackedGradeId); + Assert.Empty(await db.EggLots.IgnoreQueryFilters() + .Where(l => l.DailyEntryId == entryId && l.EggGradeId == crackedId).ToListAsync()); + }); + } + private async Task<(string Status, int Version, int Lots)> StateOf(Guid accountId, Guid entryId) { (string, int, int) state = default; From 79163c5cda180e2cb84a6649149a261ebcc59731 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 23:00:54 -0700 Subject: [PATCH 06/17] fix(daily-entry): reconcile the quality lots on adjust, not just the manual ones (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A defect introduced by the previous commit in this branch, found by writing the adjustment tests rather than by reasoning about the change. The reconciler drives EVERY lot linked to the entry toward a target built from entry.Grades. Condition lots are deliberately absent from entry.Grades (a condition grade is never a manual line), so they had no target — and a lot with no target does not get skipped, it reads as target ZERO. Any adjustment to an entry carrying quality stock would therefore have emptied that stock silently, including adjustments that had nothing to do with the condition counters. Targets now include both snapshots, read from the ENTRY rather than re-resolved from the catalog: a farm that turns Cracked non-saleable after submitting must still be able to correct that day, and the day keeps the meaning it was recorded with. The existing new-lot loop then covers the other direction — a counter that goes from zero to positive gains a lot dated and linked like the rest. Mutation results, reported in full because one is not a clean pass: - removing the condition targets → the grow test and the manual-lot-untouched test go red - removing the zero-quantity skip in the new-lot loop → SURVIVED on the first attempt. Every existing test submitted a positive counter, so the zero-with-no-lot path was never exercised and the guard was an untested claim. Adjusting_a_day_that_never_had_cracked_eggs_still_succeeds now covers it — an ordinary good day has a snapshot and no lot, and EggLot.Create rejects a zero quantity, so without the skip the adjustment throws. Re-run: killed. Also worth recording: Adjusting_a_condition_counter_to_zero_empties_its_lot passed BEFORE the fix, because the bug and the correct behaviour agree at zero. It was green for the wrong reason and only became meaningful afterwards. Baseline green: 66/66 across the daily-entry, quality-egg and race suites, 0 warnings. Version handling is unchanged — no new aggregate mutation path — so the existing parallel submit/adjust race coverage still applies and passes. --- .../AdjustDailyEntryHandler.cs | 24 ++++ .../QualityEggTests.cs | 134 ++++++++++++++++++ 2 files changed, 158 insertions(+) diff --git a/src/Cluckwork.Application/Features/DailyEntries/AdjustDailyEntry/AdjustDailyEntryHandler.cs b/src/Cluckwork.Application/Features/DailyEntries/AdjustDailyEntry/AdjustDailyEntryHandler.cs index 26950d0e0..dc83b15a9 100644 --- a/src/Cluckwork.Application/Features/DailyEntries/AdjustDailyEntry/AdjustDailyEntryHandler.cs +++ b/src/Cluckwork.Application/Features/DailyEntries/AdjustDailyEntry/AdjustDailyEntryHandler.cs @@ -128,6 +128,23 @@ await unitOfWork.ExecuteInTransactionAsync(async transactionCt => // untouchable wherever they sit) and the first lot in canonical // order carries the remainder. var targets = entry.Grades.ToDictionary(l => l.EggGradeId, l => l.Quantity); + + // #396 — the counter-backed lots are targets too. They are NOT in + // entry.Grades (a condition grade is never a manual line — see + // ConditionGradeGuard), so omitting them here does not merely skip + // them: the loop below drives EVERY lot linked to this entry toward + // its target, and a lot with no target reads as target ZERO. An + // adjustment would therefore silently destroy stock the farm holds, + // on an entry the adjustment did not even intend to change that way. + // + // Read from the entry's SNAPSHOT, never re-resolved from the + // catalog: a farm that turned Cracked non-saleable after submitting + // must still be able to correct that day's count, and the day keeps + // the meaning it was recorded with. + if (entry.CrackedGradeId is { } crackedGradeId) + targets[crackedGradeId] = entry.CrackedEggs; + if (entry.DirtyGradeId is { } dirtyGradeId) + targets[dirtyGradeId] = entry.DirtyEggs; foreach (var gradeLots in lockedLots.GroupBy(l => l.EggGradeId)) { var target = targets.GetValueOrDefault(gradeLots.Key, 0); @@ -176,6 +193,13 @@ await eggMovements.AddAsync(EggInventoryMovement.Create( foreach (var (gradeId, quantity) in targets) { if (lockedLots.Any(l => l.EggGradeId == gradeId)) continue; + + // #396 — a condition target can legitimately be ZERO (a day + // with no cracked eggs), and EggLot.Create rejects a zero + // quantity. Nothing to create: no lot, and nothing to + // reconcile away either, since there is no lot to shrink. + if (quantity <= 0) continue; + var newLot = EggLot.Create( Guid.NewGuid(), accountId, entry.FlockId, entry.Date, gradeId, quantity, dailyEntryId: entry.Id); diff --git a/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs b/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs index a214b9715..e2a9bd4d8 100644 --- a/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs @@ -258,6 +258,140 @@ await factory.WithTenantScopeAsync(accountId, async db => }); } + // ---- adjustment reconciles the quality lots too --------------------- + // + // The manual lines and the condition counters produce the same KIND of + // thing — a lot linked to this entry — so an adjustment has to reconcile + // both. Getting this wrong is not a missing feature but active damage: the + // reconciler drives every linked lot toward a target, and a condition lot + // absent from the target set is driven to ZERO, silently destroying stock + // the farm holds. + + private async Task CrackedLotQuantity(Guid accountId, Guid entryId, Guid crackedId) + { + var quantity = -1; + await factory.WithTenantScopeAsync(accountId, async db => + { + var lot = await db.EggLots.IgnoreQueryFilters() + .SingleOrDefaultAsync(l => l.DailyEntryId == entryId && l.EggGradeId == crackedId); + quantity = lot?.QuantityProduced ?? 0; + }); + return quantity; + } + + private Task AdjustAsync( + HttpClient client, Guid entryId, int version, int cracked, int total) => + client.PostWithKeyAsync( + $"/api/v1/daily-entries/{entryId}/adjust", Guid.NewGuid().ToString(), + new + { + version, + totalEggs = total, + crackedEggs = cracked, + dirtyEggs = 5, + discardedEggs = 3, + mortalityCount = 0, + reason = "recount", + grades = new[] { new { eggGradeId = ManualGradeId, quantity = 600 } } + }); + + private Guid ManualGradeId; + + [Fact] + public async Task Adjusting_a_condition_counter_upward_grows_its_lot() + { + var (client, accountId, farmId, flockId, manual, crackedId, _) = await SetupAsync(); + ManualGradeId = manual["Large"]; + + var entryId = await SubmitAsync(client, farmId, flockId, ManualGradeId); + Assert.Equal(10, await CrackedLotQuantity(accountId, entryId, crackedId)); + + var version = (await StateOf(accountId, entryId)).Version; + var response = await AdjustAsync(client, entryId, version, cracked: 20, total: 628); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(20, await CrackedLotQuantity(accountId, entryId, crackedId)); + } + + [Fact] + public async Task Adjusting_a_condition_counter_to_zero_empties_its_lot() + { + var (client, accountId, farmId, flockId, manual, crackedId, _) = await SetupAsync(); + ManualGradeId = manual["Large"]; + + var entryId = await SubmitAsync(client, farmId, flockId, ManualGradeId); + var version = (await StateOf(accountId, entryId)).Version; + + var response = await AdjustAsync(client, entryId, version, cracked: 0, total: 608); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(0, await CrackedLotQuantity(accountId, entryId, crackedId)); + } + + [Fact] + public async Task Adjusting_leaves_the_manual_lot_alone_while_the_counter_changes() + { + // The other half: reconciling the condition lots must not disturb the + // manual ones. A fix that simply overwrote the target set would pass + // the two tests above and empty every manual lot. + var (client, accountId, farmId, flockId, manual, crackedId, _) = await SetupAsync(); + ManualGradeId = manual["Large"]; + + var entryId = await SubmitAsync(client, farmId, flockId, ManualGradeId); + var version = (await StateOf(accountId, entryId)).Version; + + Assert.Equal(HttpStatusCode.OK, + (await AdjustAsync(client, entryId, version, cracked: 20, total: 628)).StatusCode); + + await factory.WithTenantScopeAsync(accountId, async db => + { + var manualLot = await db.EggLots.IgnoreQueryFilters() + .SingleAsync(l => l.DailyEntryId == entryId && l.EggGradeId == ManualGradeId); + Assert.Equal(600, manualLot.QuantityProduced); + }); + Assert.Equal(20, await CrackedLotQuantity(accountId, entryId, crackedId)); + } + + // A day with no cracked eggs is an ordinary good day, and it has a snapshot + // but NO lot. Adjusting such an entry puts a target of zero in front of the + // new-lot loop with no lot to match it — and EggLot.Create rejects a zero + // quantity, so without the skip the adjustment throws instead of applying. + // + // Written because the mutation check caught the guard surviving: every other + // test here submits a positive counter, so the zero-with-no-lot path was + // never exercised and the guard was, at that point, an untested claim. + [Fact] + public async Task Adjusting_a_day_that_never_had_cracked_eggs_still_succeeds() + { + var (client, accountId, farmId, flockId, manual, crackedId, _) = await SetupAsync(); + ManualGradeId = manual["Large"]; + + var entryId = (await (await client.PostWithKeyAsync( + "/api/v1/daily-entries", Guid.NewGuid().ToString(), + new + { + farmId, + houseId = Guid.NewGuid(), + flockId, + date = DateOnly.FromDateTime(DateTime.UtcNow.Date), + totalEggs = 608, + crackedEggs = 0, + dirtyEggs = 5, + discardedEggs = 3, + mortalityCount = 0, + grades = new[] { new { eggGradeId = ManualGradeId, quantity = 600 } } + })).Content.ReadFromJsonAsync())!.Id; + + Assert.Equal(HttpStatusCode.OK, (await client.PostWithKeyAsync( + $"/api/v1/daily-entries/{entryId}/submit", Guid.NewGuid().ToString())).StatusCode); + + var version = (await StateOf(accountId, entryId)).Version; + var response = await AdjustAsync(client, entryId, version, cracked: 0, total: 608); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(0, await CrackedLotQuantity(accountId, entryId, crackedId)); + } + private async Task<(string Status, int Version, int Lots)> StateOf(Guid accountId, Guid entryId) { (string, int, int) state = default; From 388abd0109668122cf1ba12a4882e91de1931f59 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 23:35:18 -0700 Subject: [PATCH 07/17] feat(reports): report eggs that became stock without being hand-graded (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Owner decision: keep Sellable at its existing meaning and show the condition figure BESIDE it, rather than folding the two together. For a 1000-egg day with 900 hand-graded, 60 cracked, 30 dirty and 10 discarded, the report showed "Sellable: 900" while the farm could actually sell 990. The new FromCounts column carries the 90 — the eggs that became stock without passing through the Grading pane. Deliberately a separate figure, not a widened Sellable. The two answer different questions ("how many did we grade" vs "how many can we sell"), were only ever equal because conditions used to be losses, and Sellable is also the target Daily Entry counts down to and that #394 requires the grade lines to reconcile against exactly — moving it would have silently changed what the capture screen validates. Only SNAPSHOT-BACKED conditions count. A null snapshot records that those eggs were a loss that day, so summing the raw counters would invent stock the farm never had — most visibly for entries recorded before this feature, whose conditions were never saleable at all. The test fixture makes Cracked saleable and Dirty NOT, so the naive rule reports 9 where the answer is 6. Mutation-checked, baseline green (7/7 reports, 0 warnings): summing the raw counters turns exactly that test red. Still to come: the Reports and History screens need the column rendered, and the label wants deciding — "From counts" is used here rather than "Ungraded", because those eggs do carry a grade (Cracked/Dirty) and appear in the report's own grade breakdown, so "ungraded" would contradict the table beside it. --- .../Features/Reports/IReportQueries.cs | 15 ++++- .../Repositories/ReportQueries.cs | 11 +++- .../ReportsTests.cs | 66 ++++++++++++++++++- 3 files changed, 86 insertions(+), 6 deletions(-) diff --git a/src/Cluckwork.Application/Features/Reports/IReportQueries.cs b/src/Cluckwork.Application/Features/Reports/IReportQueries.cs index 9d5e4c082..6400ad342 100644 --- a/src/Cluckwork.Application/Features/Reports/IReportQueries.cs +++ b/src/Cluckwork.Application/Features/Reports/IReportQueries.cs @@ -14,15 +14,26 @@ public interface IReportQueries Task GetProfitAsync(DateOnly from, DateOnly to, CancellationToken ct = default); } +// #396 — `Sellable` is the HAND-GRADED remainder (total − cracked − dirty − +// discarded): the figure Daily Entry's grading counts down to, and what #394 +// requires the grade lines to reconcile against exactly. +// +// `FromCounts` is the eggs that became stock WITHOUT being hand-graded — the +// Cracked and Dirty counters, but only where the entry resolved that condition +// to a grade (see DailyEntry.CrackedGradeId). Deliberately a SEPARATE figure +// rather than folded into Sellable: the two answer different questions ("how +// many did we grade" vs "how many can we sell"), they were only ever equal +// because conditions used to be losses, and merging them would silently move +// the number the capture screen is validated against. public sealed record ProductionDay( DateOnly Date, int TotalEggs, int Cracked, int Dirty, int Discarded, - int Sellable, int Deaths, long HenDays, decimal? HenDayPct); + int Sellable, int FromCounts, int Deaths, long HenDays, decimal? HenDayPct); public sealed record GradeTotal(Guid EggGradeId, string Name, int Quantity); public sealed record ProductionReport( IReadOnlyList Days, - int TotalEggs, int TotalSellable, int TotalDeaths, + int TotalEggs, int TotalSellable, int TotalFromCounts, int TotalDeaths, long TotalHenDays, decimal? PeriodHenDayPct, IReadOnlyList GradeTotals); diff --git a/src/Cluckwork.Infrastructure/Repositories/ReportQueries.cs b/src/Cluckwork.Infrastructure/Repositories/ReportQueries.cs index 20e9b7f8c..5fe82f141 100644 --- a/src/Cluckwork.Infrastructure/Repositories/ReportQueries.cs +++ b/src/Cluckwork.Infrastructure/Repositories/ReportQueries.cs @@ -25,6 +25,14 @@ public async Task GetProductionAsync( Cracked = g.Sum(e => e.CrackedEggs), Dirty = g.Sum(e => e.DirtyEggs), Discarded = g.Sum(e => e.DiscardedEggs), + // #396 — only the conditions this entry actually RESOLVED to a + // grade. A null snapshot means those eggs were a loss on that + // day, and summing the raw counters instead would invent stock + // the farm never had — most visibly for entries recorded before + // this feature, whose conditions were never saleable at all. + FromCounts = g.Sum(e => + (e.CrackedGradeId != null ? e.CrackedEggs : 0) + + (e.DirtyGradeId != null ? e.DirtyEggs : 0)), Deaths = g.Sum(e => e.MortalityCount), }) .ToDictionaryAsync(x => x.Date, ct); @@ -119,7 +127,7 @@ public async Task GetProductionAsync( var sellable = total - (row?.Cracked ?? 0) - (row?.Dirty ?? 0) - (row?.Discarded ?? 0); days.Add(new ProductionDay( d, total, row?.Cracked ?? 0, row?.Dirty ?? 0, row?.Discarded ?? 0, - sellable, row?.Deaths ?? 0, henDays, + sellable, row?.FromCounts ?? 0, row?.Deaths ?? 0, henDays, henDays > 0 ? Math.Round(total * 100m / henDays, 1) : null)); if (d == DateOnly.MaxValue) break; // AddDays would overflow } @@ -130,6 +138,7 @@ public async Task GetProductionAsync( days, totalEggs, days.Sum(x => x.Sellable), + days.Sum(x => x.FromCounts), days.Sum(x => x.Deaths), totalHenDays, totalHenDays > 0 ? Math.Round(totalEggs * 100m / totalHenDays, 1) : null, diff --git a/tests/Cluckwork.Api.IntegrationTests/ReportsTests.cs b/tests/Cluckwork.Api.IntegrationTests/ReportsTests.cs index 97769a70d..15ade3ad6 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ReportsTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ReportsTests.cs @@ -12,11 +12,11 @@ public sealed class ReportsTests(CluckworkWebApplicationFactory factory) private sealed record Created(Guid Id); private sealed record DayRow( DateOnly Date, int TotalEggs, int Cracked, int Dirty, int Discarded, - int Sellable, int Deaths, long HenDays, decimal? HenDayPct); + int Sellable, int FromCounts, int Deaths, long HenDays, decimal? HenDayPct); private sealed record GradeRow(Guid EggGradeId, string Name, int Quantity); private sealed record ProductionDto( - List Days, int TotalEggs, int TotalSellable, int TotalDeaths, - long TotalHenDays, decimal? PeriodHenDayPct, List GradeTotals); + List Days, int TotalEggs, int TotalSellable, int TotalFromCounts, + int TotalDeaths, long TotalHenDays, decimal? PeriodHenDayPct, List GradeTotals); private sealed record SalesDto( int ConfirmedCount, long RevenueMinorUnits, long PaidMinorUnits, long OutstandingMinorUnits, int VoidedCount, string CurrencyCode, int CurrencyMinorUnit); @@ -26,6 +26,66 @@ private sealed record ProfitDto( private static readonly DateOnly Today = DateOnly.FromDateTime(DateTime.UtcNow.Date); + // #396 — Cracked and Dirty can now become stock, so the report has to say + // how many eggs the day produced WITHOUT being hand-graded. Sellable keeps + // its meaning (the hand-graded remainder, and the target Daily Entry counts + // down to); FromCounts is the separate figure, so the two are never + // conflated and the period totals stay addable. + // + // Only SNAPSHOT-BACKED conditions count. This fixture makes Cracked saleable + // and Dirty NOT, so a rule that simply added both counters would report 9 + // where the farm can only sell 6 — inventing stock that was recorded as a + // loss. + [Fact] + public async Task Production_CountsOnlySnapshotBackedConditionsAsFromCounts() + { + var email = $"u-{Guid.NewGuid():N}@test.local"; + var accountId = await factory.SeedAccountWithUserAsync(email); + var farmId = Guid.NewGuid(); + var grades = await factory.SeedEggGradesAsync(accountId, farmId, "Large"); + var flockId = await factory.SeedFlockAsync(accountId, farmId); + + await factory.WithTenantScopeAsync(accountId, async db => + { + db.EggGrades.Add(Domain.Eggs.EggGrade.Create( + Guid.NewGuid(), accountId, farmId, "Cracked", Domain.Eggs.EggGradeType.Quality, + 60, isSaleable: true, dailyEntryKind: Domain.Eggs.DailyEntryKind.Cracked)); + db.EggGrades.Add(Domain.Eggs.EggGrade.Create( + Guid.NewGuid(), accountId, farmId, "Dirty", Domain.Eggs.EggGradeType.Quality, + 61, isSaleable: false, dailyEntryKind: Domain.Eggs.DailyEntryKind.Dirty)); + await db.SaveChangesAsync(); + }); + + var client = factory.CreateAuthedClient(await factory.LoginForAccessTokenAsync(email)); + + // 100 = 90 hand-graded + 6 cracked + 3 dirty + 1 discarded. + var entryId = (await (await client.PostWithKeyAsync( + "/api/v1/daily-entries", Guid.NewGuid().ToString(), new + { + farmId, + houseId = Guid.NewGuid(), + flockId, + date = Today, + totalEggs = 100, + crackedEggs = 6, + dirtyEggs = 3, + discardedEggs = 1, + mortalityCount = 0, + grades = new[] { new { eggGradeId = grades["Large"], quantity = 90 } } + })).Content.ReadFromJsonAsync())!.Id; + await client.PostWithKeyAsync($"/api/v1/daily-entries/{entryId}/submit", Guid.NewGuid().ToString()); + + var report = await client.GetFromJsonAsync( + $"/api/v1/reports/production?from={Today:yyyy-MM-dd}&to={Today:yyyy-MM-dd}"); + + var day = Assert.Single(report!.Days); + Assert.Equal(100, day.TotalEggs); + Assert.Equal(90, day.Sellable); // unchanged: the hand-graded remainder + Assert.Equal(6, day.FromCounts); // cracked only — dirty stayed a loss + Assert.Equal(90, report.TotalSellable); + Assert.Equal(6, report.TotalFromCounts); + } + [Fact] public async Task Production_HenDays_OfficialEntriesOnly() { From b6ecdc79d56e59c711ee1ff140a7d069b3f90bb1 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 23:49:36 -0700 Subject: [PATCH 08/17] feat(web): show condition stock on Reports and History (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Owner-named column: "Condition" — the term specs.md and the glossary already use for Cracked and Dirty. Deliberately not "Ungraded" (those eggs do carry a grade, shown in the same screen's by-grade breakdown), not "Seconds" (a real seeded grade name, so the column and the breakdown would disagree), and not "Bad eggs" (in food that reads as spoiled, and these are being sold — the app already has Discarded for eggs thrown away). Reports gains the column beside Sellable, never folded into it: Sellable stays the hand-graded remainder and the figure Daily Entry counts down to. History gains the same column per entry. History reads each entry's own SNAPSHOT, never the current grade catalog. A farm that switches Cracked off today must still see past days as they were recorded, and one that switches it on must not see past losses become stock. A draft shows an em dash rather than 0 — nothing has resolved yet, and 0 would assert "these were a loss", a different fact that is not yet true. API: DailyEntryResponse gains crackedGradeId/dirtyGradeId (null on a draft and on a resolved-to-loss entry alike; readers distinguish by Status). The ADJUST REQUEST deliberately does not carry them — an adjustment never re-resolves — and a test now pins that, after a broad fixture edit of mine wrongly added them to that assertion and the suite caught it. Help page and GLOSSARY.md updated in the same change, per the standing rule. The glossary gains "Condition grade" (the kind binding, the snapshot, the both- flags rule, and why a condition grade can never be a manual line) and "Condition" as the column. All three catalogs carry the strings; es/tl are machine-drafted pending native review (#182). Assertions are cell-indexed, not text-matched: "0" and the em dash both occur in sibling cells, so a text query passes against the wrong column. That is not hypothetical — the first draft of both tests did exactly that and had to be tightened. Mutation-checked, baseline green (1300/1300 vitest, typecheck clean, 0 warnings): summing the raw counters in History turns 2 tests red; dropping the draft em dash turns 1 red; wiring the Reports column to sellable turns 3 red. --- specs/product/GLOSSARY.md | 26 ++++++++++ .../DailyEntries/DailyEntryEndpoints.cs | 9 ++++ web/src/api/cluckwork.ts | 20 ++++++++ web/src/i18n/en.ts | 26 ++++++++-- web/src/i18n/es.ts | 19 +++++-- web/src/i18n/tl.ts | 18 ++++++- web/src/routes/DailyEntryPage.test.tsx | 8 +-- web/src/routes/Dashboard.test.tsx | 3 +- web/src/routes/HelpPage.tsx | 3 ++ web/src/routes/HistoryPage.test.tsx | 50 ++++++++++++++++++- web/src/routes/HistoryPage.tsx | 22 +++++++- web/src/routes/ReportsPage.test.tsx | 36 +++++++++++-- web/src/routes/ReportsPage.tsx | 6 +++ 13 files changed, 228 insertions(+), 18 deletions(-) diff --git a/specs/product/GLOSSARY.md b/specs/product/GLOSSARY.md index 84922e1d4..83c846f31 100644 --- a/specs/product/GLOSSARY.md +++ b/specs/product/GLOSSARY.md @@ -94,6 +94,32 @@ remainder controls; each screen still renders its own form and owns its own state and submission. So the two can drift, and keeping them saying the same thing is a review obligation, not something the code enforces. +**Condition grade (#396)** — the Cracked and Dirty grades, fed by the Daily +Entry's own **counters** rather than by a hand-typed grade line. A farm may sell +them as separate stock (both are saleable on a new installation) or keep them as +losses by switching either off; **Discarded** is always a loss and is never a +condition grade. + +Two things make the binding safe to rename around. The grade carries a fixed +**kind** (`Cracked` / `Dirty`) rather than being matched on its name, so renaming +"Cracked" to "Segunda" does not detach the counter from the grade. And the entry +**snapshots** which grade each counter resolved to at the moment it became +official — so turning a grade off later never rewrites a past day, and turning +one on never invents stock for a day recorded as a loss. Resolution requires the +grade to be **both active and saleable**; anything else records the condition as +a loss. + +A condition grade can never be entered as a manual grade line — the server +refuses it, not just the screen. Its counter already produces a lot, so a manual +line naming it would produce a second lot for the same grade on the same day. + +**Condition (Reports / History column, #396)** — the eggs a day produced that +became stock **without being hand-graded**: the condition counters above, counted +only where that entry resolved them to a grade. Deliberately shown beside +**Sellable** rather than added into it — Sellable is the hand-graded remainder +and the figure grading counts down to, so the two answer different questions and +are added together to get everything the day produced that can be sold. + **Left to grade (#134)** — grading counts **down** to zero rather than reporting *graded n of m*. The figure beside the grades is how many sellable eggs are still unaccounted for; it turns green when the day adds up exactly and red when diff --git a/src/Cluckwork.Api/Endpoints/DailyEntries/DailyEntryEndpoints.cs b/src/Cluckwork.Api/Endpoints/DailyEntries/DailyEntryEndpoints.cs index 5b27f1cc8..815f7f6c9 100644 --- a/src/Cluckwork.Api/Endpoints/DailyEntries/DailyEntryEndpoints.cs +++ b/src/Cluckwork.Api/Endpoints/DailyEntries/DailyEntryEndpoints.cs @@ -84,6 +84,11 @@ private static async Task ListDailyEntries( private static DailyEntryResponse ToResponse(Cluckwork.Domain.Eggs.DailyEntry e) => new( e.Id, e.FarmId, e.HouseId, e.FlockId, e.Date, e.Status.ToString(), e.TotalEggs, e.CrackedEggs, e.DirtyEggs, e.DiscardedEggs, e.MortalityCount, + // #396 — which grade each condition counter resolved to when this entry + // became official, or null when that condition was a loss. The SPA needs + // it to say whether a day's cracked eggs became stock; it cannot re-derive + // that from the current catalog, which is the whole point of the snapshot. + e.CrackedGradeId, e.DirtyGradeId, e.Grades.Select(g => new GradeLineResponse(g.EggGradeId, g.Quantity)).ToList(), e.Version, e.AdjustReason, e.VoidReason, e.LockedAtUtc, // The audit snapshot is stored as JSON; embed it as an object, not a string. @@ -219,6 +224,10 @@ public sealed record GradeQuantityRequest(Guid EggGradeId, int Quantity); public sealed record DailyEntryResponse( Guid Id, Guid FarmId, Guid HouseId, Guid FlockId, DateOnly Date, string Status, int TotalEggs, int CrackedEggs, int DirtyEggs, int DiscardedEggs, int MortalityCount, + // #396 — null on a draft (nothing has resolved yet) AND on an official entry + // whose condition was a loss. A reader distinguishes the two by Status, not + // by these fields. + Guid? CrackedGradeId, Guid? DirtyGradeId, IReadOnlyList Grades, int Version, string? AdjustReason, string? VoidReason, DateTimeOffset? LockedAtUtc, JsonElement? AdjustedFrom); diff --git a/web/src/api/cluckwork.ts b/web/src/api/cluckwork.ts index 0ad5770dd..d270a629a 100644 --- a/web/src/api/cluckwork.ts +++ b/web/src/api/cluckwork.ts @@ -51,6 +51,17 @@ export interface DailyEntry { dirtyEggs: number; discardedEggs: number; mortalityCount: number; + /** + * #396 — which grade each condition counter resolved to when this entry + * became official; null when that condition was a loss (and on any draft, + * which has not resolved yet — tell the two apart by `status`). + * + * The only way to know whether a past day's cracked eggs became stock: the + * current grade catalog cannot answer it, because the farm may have changed + * a grade's saleability since. + */ + crackedGradeId: string | null; + dirtyGradeId: string | null; grades: GradeLine[]; version: number; adjustReason: string | null; @@ -729,7 +740,15 @@ export interface ProductionDay { cracked: number; dirty: number; discarded: number; + /** Hand-graded remainder: total − cracked − dirty − discarded (#394). */ sellable: number; + /** + * #396 — eggs that became stock WITHOUT being hand-graded: the cracked and + * dirty counters, but only where that entry resolved the condition to a + * grade. Separate from `sellable` on purpose; the two answer different + * questions and were only ever equal while conditions were always losses. + */ + fromCounts: number; deaths: number; henDays: number; henDayPct: number | null; @@ -739,6 +758,7 @@ export interface ProductionReport { days: ProductionDay[]; totalEggs: number; totalSellable: number; + totalFromCounts: number; totalDeaths: number; totalHenDays: number; periodHenDayPct: number | null; diff --git a/web/src/i18n/en.ts b/web/src/i18n/en.ts index 2c9250613..b73030a90 100644 --- a/web/src/i18n/en.ts +++ b/web/src/i18n/en.ts @@ -1498,6 +1498,10 @@ export const en = { statusHeader: "Status", totalHeader: "Total", lossesHeader: "Losses (cr/di/ds)", + // #396 — Losses counts cracked/dirty/discarded whatever became of them; + // this counts only the ones that became stock, per the entry's snapshot. + // Em dash on a draft: nothing has resolved yet, and 0 would claim "loss". + conditionHeader: "Condition", mortalityHeader: "Mortality", gradedHeader: "Graded", // Lowercase link-styled row actions, same treatment as @@ -1544,6 +1548,12 @@ export const en = { eggsHeader: "Eggs", lossesHeader: "Losses (cr/di/ds)", sellableHeader: "Sellable", + // #396 — cracked/dirty eggs that became stock instead of being written off. + // "Condition" is the term the specs and glossary already use for those two + // grades; deliberately NOT "ungraded" (they do carry a grade, shown in the + // By-grade breakdown on this same screen) and NOT "seconds" (a real seeded + // grade name, which would make the column and the breakdown disagree). + conditionHeader: "Condition", deathsHeader: "Deaths", henDaysHeader: "Hen-days", henDayPctHeader: "Hen-day %", @@ -2169,9 +2179,19 @@ export const en = { // Reports reportsHeading: "Reports", reportsProduction: - "Production (everyone): pick a date range — per-day eggs, losses, sellable, deaths, " - + "and hen-day % (eggs collected ÷ birds alive that day × 100), with period totals and " - + "a by-grade breakdown. Draft and voided entries don't count.", + "Production (everyone): pick a date range — per-day eggs, losses, sellable, " + + "condition, deaths, and hen-day % (eggs collected ÷ birds alive that day × 100), " + + "with period totals and a by-grade breakdown. Draft and voided entries don't count.", + // #396 — the new column needs explaining because two of its neighbours look + // like they should already cover it: Losses shows the same cracked/dirty + // numbers (as counts, whether sold or not), and Sellable is the graded + // figure that deliberately excludes them. + reportsCondition: + "Condition: cracked and dirty eggs that became stock instead of being written off. " + + "They are counted in Egg counts, never graded by hand, so they are not part of " + + "Sellable — add the two together for everything the day produced that you can " + + "sell. A day shows 0 here if you have those grades switched off in Settings, and days recorded " + + "before you switched them on keep showing 0 — turning a grade on never rewrites a past day.", reportsMoney: "Money (admin): sales summary for the range's orders (revenue / paid / outstanding), " + "expenses by category, and basic profit — confirmed revenue minus recorded expenses, " diff --git a/web/src/i18n/es.ts b/web/src/i18n/es.ts index f97ebdedb..0e6c076fe 100644 --- a/web/src/i18n/es.ts +++ b/web/src/i18n/es.ts @@ -1223,6 +1223,8 @@ export const es = { // Spanish words (ag/su/de) — confirm this reads clearly as a // table-header abbreviation. lossesHeader: "Pérdidas (ag/su/de)", + // #396 — machine-drafted, pending native review (#182). + conditionHeader: "Con defecto", mortalityHeader: "Mortalidad", gradedHeader: "Clasificado", editButton: "editar", @@ -1254,6 +1256,9 @@ export const es = { eggsHeader: "Huevos", lossesHeader: "Pérdidas (ag/su/de)", sellableHeader: "Vendible", + // #396 — huevos rajados/sucios que pasaron a inventario en vez de perderse. + // Machine-drafted, pending native review (#182). + conditionHeader: "Con defecto", deathsHeader: "Muertes", henDaysHeader: "Días-gallina", henDayPctHeader: "% días-gallina", @@ -1839,9 +1844,17 @@ export const es = { reportsHeading: "Informes", reportsProduction: "Producción (todos): elija un rango de fechas — huevos por día, pérdidas, " - + "vendibles, muertes, y % de puesta diaria (huevos recolectados ÷ aves vivas ese " - + "día × 100), con totales del período y un desglose por grado. Las entradas en borrador y anuladas " - + "no cuentan.", + + "vendibles, con defecto, muertes, y % de puesta diaria (huevos recolectados ÷ " + + "aves vivas ese día × 100), con totales del período y un desglose por grado. Las entradas en " + + "borrador y anuladas no cuentan.", + // #396 — machine-drafted, pending native review (#182). + reportsCondition: + "Con defecto: huevos rajados y sucios que pasaron a inventario en vez de " + + "registrarse como pérdida. Se cuentan en Conteo de huevos, nunca se clasifican a " + + "mano, así que no forman parte de Vendible — sume ambos para obtener todo lo que " + + "el día produjo y usted puede vender. El día muestra 0 si esos grados están desactivados en " + + "Ajustes, y los días registrados antes de activarlos siguen mostrando 0: activar un grado nunca " + + "reescribe un día pasado.", reportsMoney: "Dinero (admin): resumen de ventas de los pedidos del rango (ingresos / pagado / " + "pendiente), gastos por categoría, y ganancia básica — ingresos confirmados menos " diff --git a/web/src/i18n/tl.ts b/web/src/i18n/tl.ts index 629a0f348..b5977ba1a 100644 --- a/web/src/i18n/tl.ts +++ b/web/src/i18n/tl.ts @@ -1260,6 +1260,8 @@ export const tl = { // Filipino words (basag/marumi/tapon → b/m/t) — confirm this reads // clearly as a table-header abbreviation. lossesHeader: "Nasira (b/m/t)", + // #396 — machine-drafted, pending native review (#182). + conditionHeader: "May depekto", mortalityHeader: "Mortalidad", gradedHeader: "Na-grade", editButton: "i-edit", @@ -1296,6 +1298,9 @@ export const tl = { eggsHeader: "Itlog", lossesHeader: "Nasira (b/m/t)", sellableHeader: "Maibebenta", + // #396 — basag/marumi na itlog na naging stock sa halip na malugi. + // Machine-drafted, pending native review (#182). + conditionHeader: "May depekto", deathsHeader: "Namatay", henDaysHeader: "Hen-days", henDayPctHeader: "Hen-day %", @@ -1910,8 +1915,17 @@ export const tl = { reportsHeading: "Mga Report", reportsProduction: "Production (lahat): pumili ng date range — itlog kada araw, nawala, naibibenta, " - + "namatay, at hen-day % (itlog na nakolekta ÷ ibong buhay sa araw na iyon × 100), " - + "may kasamang period total at breakdown kada grado. Hindi binibilang ang draft at na-void na entry.", + + "may depekto, namatay, at hen-day % (itlog na nakolekta ÷ ibong buhay sa araw na " + + "iyon × 100), may kasamang period total at breakdown kada grado. Hindi binibilang ang draft at " + + "na-void na entry.", + // #396 — machine-drafted, pending native review (#182). + reportsCondition: + "May depekto: basag at maruming itlog na naging stock sa halip na ituring na " + + "lugi. Binibilang ang mga ito sa Bilang ng itlog, hindi kailanman gine-grade sa " + + "kamay, kaya wala ang mga ito sa Maibebenta — pagsamahin ang dalawa para makuha " + + "ang lahat ng naibebenta sa araw na iyon. Magpapakita ng 0 kung naka-off ang mga gradong iyon sa " + + "Settings, at ang mga araw na naitala bago mo ito i-on ay mananatiling 0 — ang pag-on ng grado ay " + + "hindi kailanman binabago ang nakaraang araw.", reportsMoney: "Pera (admin): buod ng benta para sa mga order ng range (kita / nabayaran / " + "natitira), gastos kada kategorya, at basic profit — nakumpirmang kita bawas ang " diff --git a/web/src/routes/DailyEntryPage.test.tsx b/web/src/routes/DailyEntryPage.test.tsx index 478f3be56..d8ec306ac 100644 --- a/web/src/routes/DailyEntryPage.test.tsx +++ b/web/src/routes/DailyEntryPage.test.tsx @@ -194,7 +194,7 @@ describe("DailyEntryPage prefill gating", () => { it("locks the form when the day already has a submitted entry", async () => { const existing: DailyEntry = { id: "de1", farmId: "farm1", houseId: "h1", flockId: "f1", date: todayIso(), status: "Submitted", - totalEggs: 100, crackedEggs: 2, dirtyEggs: 3, discardedEggs: 5, mortalityCount: 1, + totalEggs: 100, crackedEggs: 2, dirtyEggs: 3, discardedEggs: 5, mortalityCount: 1, crackedGradeId: null, dirtyGradeId: null, grades: [{ eggGradeId: "gr1", quantity: 60 }, { eggGradeId: "gr2", quantity: 25 }], version: 1, adjustReason: null, voidReason: null, lockedAtUtc: "2026-07-20T10:00:00Z", adjustedFrom: null, }; @@ -217,7 +217,7 @@ describe("DailyEntryPage prefill gating", () => { it("shows the harmonized 'adjusted' label, not the raw status, for a manager-adjusted day", async () => { const existing: DailyEntry = { id: "de1", farmId: "farm1", houseId: "h1", flockId: "f1", date: todayIso(), status: "ManagerAdjusted", - totalEggs: 100, crackedEggs: 2, dirtyEggs: 3, discardedEggs: 5, mortalityCount: 1, + totalEggs: 100, crackedEggs: 2, dirtyEggs: 3, discardedEggs: 5, mortalityCount: 1, crackedGradeId: null, dirtyGradeId: null, grades: [{ eggGradeId: "gr1", quantity: 60 }, { eggGradeId: "gr2", quantity: 25 }], version: 1, adjustReason: null, voidReason: null, lockedAtUtc: "2026-07-20T10:00:00Z", adjustedFrom: null, }; @@ -398,7 +398,7 @@ describe("DailyEntryPage structure", () => { describe("DailyEntryPage draft badge", () => { const draftFor = (date: string) => ({ id: "de1", farmId: "farm1", houseId: "h1", flockId: "f1", date, status: "Draft", - totalEggs: 40, crackedEggs: 1, dirtyEggs: 0, discardedEggs: 0, mortalityCount: 0, + totalEggs: 40, crackedEggs: 1, dirtyEggs: 0, discardedEggs: 0, mortalityCount: 0, crackedGradeId: null, dirtyGradeId: null, grades: [], version: 1, adjustReason: null, voidReason: null, lockedAtUtc: null, adjustedFrom: null, } as DailyEntry); @@ -832,7 +832,7 @@ describe("DailyEntryPage i18n wiring (#182, Task 11)", () => { it("interpolates the enum-labelled status into the entry-locked banner from the catalog", async () => { const existing: DailyEntry = { id: "de1", farmId: "farm1", houseId: "h1", flockId: "f1", date: todayIso(), status: "Submitted", - totalEggs: 100, crackedEggs: 2, dirtyEggs: 3, discardedEggs: 5, mortalityCount: 1, + totalEggs: 100, crackedEggs: 2, dirtyEggs: 3, discardedEggs: 5, mortalityCount: 1, crackedGradeId: null, dirtyGradeId: null, grades: [], version: 1, adjustReason: null, voidReason: null, lockedAtUtc: "2026-07-20T10:00:00Z", adjustedFrom: null, }; mockListDailyEntries.mockResolvedValue([existing]); diff --git a/web/src/routes/Dashboard.test.tsx b/web/src/routes/Dashboard.test.tsx index afdafbea2..ab87a5df9 100644 --- a/web/src/routes/Dashboard.test.tsx +++ b/web/src/routes/Dashboard.test.tsx @@ -33,7 +33,8 @@ const flock = (id: string, status: string): Flock => ({ }); const entry = (flockId: string, status: string, totalEggs: number): DailyEntry => ({ id: `de-${flockId}`, farmId: "f", houseId: "h", flockId, date: "2026-07-21", status, - totalEggs, crackedEggs: 0, dirtyEggs: 0, discardedEggs: 0, mortalityCount: 0, grades: [], + totalEggs, crackedEggs: 0, dirtyEggs: 0, discardedEggs: 0, mortalityCount: 0, + crackedGradeId: null, dirtyGradeId: null, grades: [], version: 1, adjustReason: null, voidReason: null, lockedAtUtc: null, adjustedFrom: null, }); const STOCK: StockRow[] = [ diff --git a/web/src/routes/HelpPage.tsx b/web/src/routes/HelpPage.tsx index 08cef7e07..b3f3798f1 100644 --- a/web/src/routes/HelpPage.tsx +++ b/web/src/routes/HelpPage.tsx @@ -318,6 +318,9 @@ export function HelpPage() {
  • }} />
    • +
  • + }} /> +
  • }} />
    • diff --git a/web/src/routes/HistoryPage.test.tsx b/web/src/routes/HistoryPage.test.tsx index 81975e608..ceee83a6b 100644 --- a/web/src/routes/HistoryPage.test.tsx +++ b/web/src/routes/HistoryPage.test.tsx @@ -38,7 +38,7 @@ const GRADE_B: EggGrade = { id: "gr2", farmId: "farm1", name: "Grade B", gradeTy // sellable = 100 − 2 − 3 − 5 = 90; two graded lines summing to 60 (within). const SUBMITTED: DailyEntry = { id: "de1", farmId: "farm1", houseId: "h1", flockId: "f1", date: "2026-07-19", status: "Submitted", - totalEggs: 100, crackedEggs: 2, dirtyEggs: 3, discardedEggs: 5, mortalityCount: 1, + totalEggs: 100, crackedEggs: 2, dirtyEggs: 3, discardedEggs: 5, mortalityCount: 1, crackedGradeId: null, dirtyGradeId: null, grades: [{ eggGradeId: "gr1", quantity: 40 }, { eggGradeId: "gr2", quantity: 20 }], version: 1, adjustReason: null, voidReason: null, lockedAtUtc: null, adjustedFrom: null, }; @@ -68,6 +68,51 @@ async function openAdjustPanel() { fireEvent.click(await screen.findByRole("button", { name: "adjust" })); } +// #396 — the Condition column answers "how many of this day's cracked/dirty +// eggs became stock", read from the ENTRY's own snapshot. It must never be +// re-derived from the current grade catalog: a farm that switches Cracked off +// today would otherwise see past days lose stock they already sold. +// Columns: date, flock, status, total, losses, CONDITION, mortality, graded, +// actions. Indexed rather than matched by text because "0" and the em dash both +// occur in sibling cells, so a text query can pass against the wrong column. +const conditionCell = (row: HTMLElement) => within(row).getAllByRole("cell")[5]; + +describe("HistoryPage condition column", () => { + it("counts only the conditions this entry resolved to a grade", async () => { + // cracked 2 resolved (a grade id), dirty 3 did NOT (null) — so 2, not 5. + // Every number here is distinct, so a column wired to the wrong field, or + // one that sums the raw counters, produces a different value. + mockListDailyEntries.mockResolvedValue([ + { ...SUBMITTED, crackedGradeId: "gr1", dirtyGradeId: null }, + ]); + renderWithProviders(, { token: ADMIN }); + + const row = await screen.findByRole("row", { name: /2026-07-19/ }); + within(row).getByText("2/3/5"); // Losses still shows all three counters + // By CELL, not by text: "2" also appears inside the losses cell, so a text + // match would pass against a column that renders the wrong number. + expect(conditionCell(row)).toHaveTextContent("2"); + }); + + it("shows an em dash for a draft rather than 0", async () => { + // A draft has resolved nothing yet. 0 would state "these were a loss", + // which is a different fact and not yet true. + mockListDailyEntries.mockResolvedValue([DRAFT]); + renderWithProviders(, { token: ADMIN }); + + const row = await screen.findByRole("row", { name: /2026-07-18/ }); + expect(conditionCell(row)).toHaveTextContent("—"); + }); + + it("shows 0 for an official entry whose conditions were losses", async () => { + mockListDailyEntries.mockResolvedValue([SUBMITTED]); // both snapshots null + renderWithProviders(, { token: ADMIN }); + + const row = await screen.findByRole("row", { name: /2026-07-19/ }); + expect(conditionCell(row)).toHaveTextContent("0"); + }); +}); + describe("HistoryPage dialog dismissal", () => { it("closes the adjust dialog on Cancel without writing", async () => { mockListDailyEntries.mockResolvedValue([SUBMITTED]); @@ -172,6 +217,9 @@ describe("HistoryPage adjust — reconciliation guard", () => { const [id, body] = mockAdjustDailyEntry.mock.calls[0]; expect(id).toBe("de1"); expect(body).toMatchObject({ + // No crackedGradeId/dirtyGradeId here on purpose: an adjustment never + // re-resolves the condition grades, so the request does not carry them + // (#396). They live on the RESPONSE only. version: 1, totalEggs: 100, crackedEggs: 2, dirtyEggs: 3, discardedEggs: 5, mortalityCount: 1, reason: "recount", grades: [{ eggGradeId: "gr1", quantity: 45 }, { eggGradeId: "gr2", quantity: 45 }], }); diff --git a/web/src/routes/HistoryPage.tsx b/web/src/routes/HistoryPage.tsx index 3bc8bde80..725e70745 100644 --- a/web/src/routes/HistoryPage.tsx +++ b/web/src/routes/HistoryPage.tsx @@ -127,6 +127,20 @@ export function HistoryPage() { const correctable = (e: DailyEntry) => e.status === "Submitted" || e.status === "Locked" || e.status === "ManagerAdjusted"; + // #396 — how many of this day's cracked/dirty eggs became stock. Read from + // the ENTRY's snapshot, never from the current grade catalog: a farm that + // switched Cracked off after recording a day must still see that day as it + // was recorded, and one that switched it on must not see past losses + // retroactively turn into stock. + // + // A draft has resolved nothing yet, so it shows an em dash rather than 0 — + // "not decided" and "decided to be a loss" are different facts, and 0 would + // state the second while the first is true. + const conditionStock = (e: DailyEntry) => { + if (e.status === "Draft") return "—"; + return (e.crackedGradeId ? e.crackedEggs : 0) + (e.dirtyGradeId ? e.dirtyEggs : 0); + }; + function startAdjust(e: DailyEntry) { setAdjusting(e); setTotal(e.totalEggs); @@ -513,7 +527,12 @@ export function HistoryPage() { {t("dateHeader")}{t("flockHeader")}{t("statusHeader")}{t("totalHeader")} - {t("lossesHeader")}{t("mortalityHeader")}{t("gradedHeader")} + {t("lossesHeader")} + {/* #396 — Losses shows the cracked/dirty/discarded COUNTS + whatever became of them; this shows how many of those + actually became stock, per the entry's own snapshot. */} + {t("conditionHeader")} + {t("mortalityHeader")}{t("gradedHeader")} @@ -525,6 +544,7 @@ export function HistoryPage() { {statusCell(e)} {e.totalEggs} {e.crackedEggs}/{e.dirtyEggs}/{e.discardedEggs} + {conditionStock(e)} {e.mortalityCount} {e.grades.length === 0 diff --git a/web/src/routes/ReportsPage.test.tsx b/web/src/routes/ReportsPage.test.tsx index c54664804..9fbe741b8 100644 --- a/web/src/routes/ReportsPage.test.tsx +++ b/web/src/routes/ReportsPage.test.tsx @@ -36,10 +36,10 @@ const mockGetProfitReport = vi.mocked(getProfitReport); // of the `?? "—"` fallback (DATA, left raw per the namespace header comment). const PRODUCTION: ProductionReport = { days: [ - { date: "2026-07-19", totalEggs: 100, cracked: 2, dirty: 3, discarded: 5, sellable: 90, deaths: 1, henDays: 98, henDayPct: 91.8 }, - { date: "2026-07-18", totalEggs: 95, cracked: 1, dirty: 1, discarded: 2, sellable: 91, deaths: 0, henDays: 98, henDayPct: null }, + { date: "2026-07-19", totalEggs: 100, cracked: 2, dirty: 3, discarded: 5, sellable: 90, fromCounts: 6, deaths: 1, henDays: 98, henDayPct: 91.8 }, + { date: "2026-07-18", totalEggs: 95, cracked: 1, dirty: 1, discarded: 2, sellable: 91, fromCounts: 0, deaths: 0, henDays: 98, henDayPct: null }, ], - totalEggs: 195, totalSellable: 181, totalDeaths: 1, totalHenDays: 196, periodHenDayPct: 92.3, + totalEggs: 195, totalSellable: 181, totalFromCounts: 6, totalDeaths: 1, totalHenDays: 196, periodHenDayPct: 92.3, gradeTotals: [ { eggGradeId: "gr1", name: "Grade A", quantity: 60 }, { eggGradeId: "gr2", name: "Grade B", quantity: 30 }, @@ -104,6 +104,36 @@ describe("ReportsPage production section (renders for every role)", () => { expect(screen.getByText("By grade: Grade A 60, Grade B 30")).toBeInTheDocument(); }); + // #396 — Condition sits BESIDE Sellable, never folded into it. The fixture + // gives day 1 sellable 90 and fromCounts 6, two values that cannot be + // confused for one another, so a column wired to the wrong field fails. + it("renders the Condition column beside Sellable, and keeps Sellable unchanged", async () => { + renderWithProviders(, { token: NON_ADMIN }); + + const row1 = await screen.findByRole("row", { name: /2026-07-19/ }); + within(row1).getByText("90"); // sellable — still the hand-graded remainder + within(row1).getByText("6"); // condition — what the counters contributed + + const periodRow = screen.getByRole("row", { name: /Period/ }); + within(periodRow).getByText("181"); // totalSellable + within(periodRow).getByText("6"); // totalFromCounts + + // The header must resolve through the catalog, not be hardcoded English. + expect(screen.getByRole("columnheader", { name: "Condition" })).toBeInTheDocument(); + }); + + it("shows 0 in the Condition column for a day whose conditions were losses", async () => { + renderWithProviders(, { token: NON_ADMIN }); + + // Day 2 has cracked 1 / dirty 1 but fromCounts 0 — recorded as losses. The + // column must NOT fall back to the raw counters, which is the mistake that + // would invent stock the farm never had. + // Columns: date, eggs, losses, sellable, CONDITION, deaths, henDays, pct. + // Indexed: day 2 has a 0 in deaths too, so a text match proves nothing. + const row2 = await screen.findByRole("row", { name: /2026-07-18/ }); + expect(within(row2).getAllByRole("cell")[4]).toHaveTextContent("0"); + }); + it("falls back to the em dash for a null periodHenDayPct in the Period row too, not just per-day", async () => { mockGetProductionReport.mockResolvedValue({ ...PRODUCTION, periodHenDayPct: null }); renderWithProviders(, { token: NON_ADMIN }); diff --git a/web/src/routes/ReportsPage.tsx b/web/src/routes/ReportsPage.tsx index bc953e6b6..5bfb6225f 100644 --- a/web/src/routes/ReportsPage.tsx +++ b/web/src/routes/ReportsPage.tsx @@ -110,6 +110,10 @@ export function ReportsPage() { {t("dateHeader")}{t("eggsHeader")}{t("lossesHeader")}{t("sellableHeader")} + {/* #396 — beside Sellable, not folded into it: Sellable is the + hand-graded remainder, Condition is what the cracked/dirty + counters contributed as stock. */} + {t("conditionHeader")} {t("deathsHeader")}{t("henDaysHeader")}{t("henDayPctHeader")} @@ -120,6 +124,7 @@ export function ReportsPage() { {d.totalEggs} {d.cracked}/{d.dirty}/{d.discarded} {d.sellable} + {d.fromCounts} {d.deaths} {d.henDays} {d.henDayPct ?? "—"} @@ -132,6 +137,7 @@ export function ReportsPage() { {production.totalEggs} {production.totalSellable} + {production.totalFromCounts} {production.totalDeaths} {production.totalHenDays} {production.periodHenDayPct ?? "—"} From 1b307ad9c3e1b86218d5d3615459f0c0eb22342e Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sun, 2 Aug 2026 23:56:23 -0700 Subject: [PATCH 09/17] feat(web): keep counter-fed grades out of the grading panes (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes Task 3. EggGradeResponse gains dailyEntryKind ("Manual" | "Cracked" | "Dirty"); the SPA cannot infer it from name (renameable) or gradeType (a farm can have many Quality grades, only one of which is the Cracked counter's). Both grading panes filtered on saleability alone, and this feature makes Cracked and Dirty saleable — so both would have offered them as grade fields. Daily Entry's catalog filter and History's panelGrades now also require the Manual kind. History excludes it from the CATALOG half only: an existing line stays correctable whatever it names. This is an affordance, not the enforcement. The server refuses a manual line naming a condition grade regardless (ConditionGradeGuard, 7370dce); what this removes is a control whose only possible outcome is a rejection. Both tests had to be tightened after failing for the right reason. "Cracked" is a deliberately ambiguous label on these screens — Egg counts has a Cracked COUNTER, which must stay — so an unscoped query matches the counter and passes whatever the grading pane does. The Daily Entry test now scopes to the Grading section and additionally asserts the counter is still present in Egg counts; the History test asserts the dialog shows exactly ONE "Cracked" control, the counter, so a grade field would make two. Mutation-checked, baseline green (1302/1302 vitest, 16/16 egg-grade + quality integration, typecheck clean, 0 warnings): dropping the kind check from either filter turns exactly its own test red. --- .../Endpoints/EggGrades/EggGradeEndpoints.cs | 12 +++++- web/src/api/cluckwork.ts | 9 +++++ web/src/routes/DailyEntryPage.test.tsx | 39 ++++++++++++++++++- web/src/routes/DailyEntryPage.tsx | 8 +++- web/src/routes/GradesPage.test.tsx | 4 +- web/src/routes/HistoryPage.test.tsx | 30 +++++++++++++- web/src/routes/HistoryPage.tsx | 10 ++++- web/src/routes/ProductsPage.test.tsx | 4 +- web/src/routes/SalesPage.test.tsx | 2 +- 9 files changed, 105 insertions(+), 13 deletions(-) diff --git a/src/Cluckwork.Api/Endpoints/EggGrades/EggGradeEndpoints.cs b/src/Cluckwork.Api/Endpoints/EggGrades/EggGradeEndpoints.cs index 644c8987d..3165d021b 100644 --- a/src/Cluckwork.Api/Endpoints/EggGrades/EggGradeEndpoints.cs +++ b/src/Cluckwork.Api/Endpoints/EggGrades/EggGradeEndpoints.cs @@ -127,13 +127,21 @@ private static IResult MapFailure(Cluckwork.Domain.Common.Error error) } private static EggGradeResponse ToResponse(EggGrade g) => - new(g.Id, g.FarmId, g.Name, g.GradeType.ToString(), g.SortOrder, g.IsSaleable, g.Active); + new(g.Id, g.FarmId, g.Name, g.GradeType.ToString(), g.SortOrder, g.IsSaleable, + g.DailyEntryKind.ToString(), g.Active); } // FarmId included from day one — grades are farm-scoped (spec §9.1) and a // multi-farm client needs to know which farm each bucket belongs to. public sealed record EggGradeResponse( - Guid Id, Guid FarmId, string Name, string GradeType, int SortOrder, bool IsSaleable, bool Active); + Guid Id, Guid FarmId, string Name, string GradeType, int SortOrder, bool IsSaleable, + // #396 — "Manual", "Cracked" or "Dirty". The client cannot infer this from + // Name (the farm may rename a grade) or from GradeType (a farm can have many + // Quality grades, only one of which is the Cracked counter's). It is what + // lets the Grading pane leave the two counter-fed grades out; the server + // refuses them regardless, so this is the affordance, not the enforcement. + string DailyEntryKind, + bool Active); public sealed record CreateEggGradeRequest( string Name, string GradeType, int SortOrder, bool IsSaleable); diff --git a/web/src/api/cluckwork.ts b/web/src/api/cluckwork.ts index d270a629a..30316389a 100644 --- a/web/src/api/cluckwork.ts +++ b/web/src/api/cluckwork.ts @@ -8,6 +8,15 @@ export interface EggGrade { gradeType: string; sortOrder: number; isSaleable: boolean; + /** + * #396 — "Manual" | "Cracked" | "Dirty". Which Daily Entry input feeds this + * grade. Not inferable from `name` (renameable) or `gradeType` (a farm can + * have many Quality grades, only one of which is the Cracked counter's). + * + * Used to keep the two counter-fed grades out of the Grading pane. That is an + * affordance only — the server refuses a manual line naming one regardless. + */ + dailyEntryKind: string; active: boolean; } diff --git a/web/src/routes/DailyEntryPage.test.tsx b/web/src/routes/DailyEntryPage.test.tsx index d8ec306ac..140fc8e9f 100644 --- a/web/src/routes/DailyEntryPage.test.tsx +++ b/web/src/routes/DailyEntryPage.test.tsx @@ -32,9 +32,15 @@ const FLOCK: Flock = { placementDate: "2026-01-01", initialCount: 100, currentBirds: 98, status: "Active", }; const GRADES: EggGrade[] = [ - { id: "gr1", farmId: "farm1", name: "Grade A", gradeType: "Size", sortOrder: 1, isSaleable: true, active: true }, - { id: "gr2", farmId: "farm1", name: "Grade B", gradeType: "Size", sortOrder: 2, isSaleable: true, active: true }, + { id: "gr1", farmId: "farm1", name: "Grade A", gradeType: "Size", sortOrder: 1, isSaleable: true, dailyEntryKind: "Manual", active: true }, + { id: "gr2", farmId: "farm1", name: "Grade B", gradeType: "Size", sortOrder: 2, isSaleable: true, dailyEntryKind: "Manual", active: true }, ]; +// #396 — saleable AND active, so it passes every pre-existing filter. The only +// thing that keeps it out of the Grading pane is its kind. +const CRACKED: EggGrade = { + id: "gr-cracked", farmId: "farm1", name: "Cracked", gradeType: "Quality", + sortOrder: 3, isSaleable: true, dailyEntryKind: "Cracked", active: true, +}; beforeEach(() => { vi.clearAllMocks(); @@ -73,6 +79,35 @@ async function renderReady() { expect(mockListDailyEntries).toHaveBeenCalled(); // prefill really ran } +// #396 — Cracked and Dirty are fed by their own counters in Egg counts, so they +// must never appear as grade fields in the Grading pane. Their counter already +// produces a lot; a manual line naming one would produce a SECOND lot for the +// same grade on the same day. The server refuses it outright, so this is the +// affordance — the screen not offering a control whose only outcome is a +// rejection. +describe("DailyEntryPage grading pane excludes counter-fed grades", () => { + it("offers no grade field for a saleable, active condition grade", async () => { + mockListEggGrades.mockResolvedValue([...GRADES, CRACKED]); + await renderReady(); + + // Scoped to the GRADING pane. "Cracked" is a deliberately ambiguous label + // on this screen: Egg counts has a Cracked *counter*, which must stay — an + // unscoped query matches that and passes whatever the Grading pane does. + // (The first version of this test did exactly that and had to be tightened.) + const grading = screen.getByRole("heading", { name: /Grading/ }) + .closest("section") as HTMLElement; + + expect(within(grading).getByLabelText("Grade A")).toBeInTheDocument(); + expect(within(grading).getByLabelText("Grade B")).toBeInTheDocument(); + expect(within(grading).queryByLabelText("Cracked")).not.toBeInTheDocument(); + + // ...while the counter it IS fed by stays where it belongs. + const counts = screen.getByRole("heading", { name: /Egg counts/ }) + .closest("section") as HTMLElement; + expect(within(counts).getByLabelText("Cracked")).toBeInTheDocument(); + }); +}); + describe("DailyEntryPage accuracy gating", () => { // #394: grading must reconcile EXACTLY to sellable before submit — "within // sellable" (the old gate) is no longer enough. This used to be the case diff --git a/web/src/routes/DailyEntryPage.tsx b/web/src/routes/DailyEntryPage.tsx index f616c2e39..e1ae450c3 100644 --- a/web/src/routes/DailyEntryPage.tsx +++ b/web/src/routes/DailyEntryPage.tsx @@ -94,7 +94,13 @@ export function DailyEntryPage() { .then(([all, g]) => { const f = capturable(all); setFlocks(f); - setGrades(g.filter((x) => x.isSaleable)); + // #396 — saleable AND hand-graded. Cracked and Dirty are saleable now, + // so filtering on isSaleable alone would put them in the Grading pane; + // their counters already produce a lot, and a manual line naming one + // would produce a second lot for the same grade on the same day. The + // server refuses that outright (ConditionGradeGuard) — this keeps the + // screen from offering a control whose only outcome is a rejection. + setGrades(g.filter((x) => x.isSaleable && x.dailyEntryKind === "Manual")); // Deep link from History's Draft "edit" (#85): ?flockId=…&date=…. // The pair is applied atomically — applying only the date against a // fallback flock would open a DIFFERENT flock's day under the linked diff --git a/web/src/routes/GradesPage.test.tsx b/web/src/routes/GradesPage.test.tsx index 2f2cc74b0..bd85578c9 100644 --- a/web/src/routes/GradesPage.test.tsx +++ b/web/src/routes/GradesPage.test.tsx @@ -24,8 +24,8 @@ const mockUpdate = vi.mocked(updateEggGrade); const mockDeactivate = vi.mocked(deactivateEggGrade); const mockActivate = vi.mocked(activateEggGrade); -const GRADE_A: EggGrade = { id: "g1", farmId: "f", name: "Grade A", gradeType: "Size", sortOrder: 1, isSaleable: true, active: true }; -const GRADE_OLD: EggGrade = { id: "g2", farmId: "f", name: "Legacy", gradeType: "Quality", sortOrder: 2, isSaleable: false, active: false }; +const GRADE_A: EggGrade = { id: "g1", farmId: "f", name: "Grade A", gradeType: "Size", sortOrder: 1, isSaleable: true, dailyEntryKind: "Manual", active: true }; +const GRADE_OLD: EggGrade = { id: "g2", farmId: "f", name: "Legacy", gradeType: "Quality", sortOrder: 2, isSaleable: false, dailyEntryKind: "Manual", active: false }; const ADMIN = { sub: "u1", role: "Admin" }; const WORKER = { sub: "u1" }; diff --git a/web/src/routes/HistoryPage.test.tsx b/web/src/routes/HistoryPage.test.tsx index ceee83a6b..ee48e3b84 100644 --- a/web/src/routes/HistoryPage.test.tsx +++ b/web/src/routes/HistoryPage.test.tsx @@ -32,8 +32,8 @@ const FLOCK: Flock = { placementDate: "2026-01-01", initialCount: 100, currentBirds: 98, status: "Active", }; const ARCHIVED_FLOCK: Flock = { ...FLOCK, id: "f2", name: "Old Coop", status: "Archived" }; -const GRADE_A: EggGrade = { id: "gr1", farmId: "farm1", name: "Grade A", gradeType: "Size", sortOrder: 1, isSaleable: true, active: true }; -const GRADE_B: EggGrade = { id: "gr2", farmId: "farm1", name: "Grade B", gradeType: "Size", sortOrder: 2, isSaleable: true, active: true }; +const GRADE_A: EggGrade = { id: "gr1", farmId: "farm1", name: "Grade A", gradeType: "Size", sortOrder: 1, isSaleable: true, dailyEntryKind: "Manual", active: true }; +const GRADE_B: EggGrade = { id: "gr2", farmId: "farm1", name: "Grade B", gradeType: "Size", sortOrder: 2, isSaleable: true, dailyEntryKind: "Manual", active: true }; // sellable = 100 − 2 − 3 − 5 = 90; two graded lines summing to 60 (within). const SUBMITTED: DailyEntry = { @@ -113,6 +113,32 @@ describe("HistoryPage condition column", () => { }); }); +// #396 — the adjust dialog is the Daily entry form, so the same rule holds: +// a counter-fed grade is never offered for adding. Excluded from the CATALOG +// half only — an existing line stays correctable whatever it names. +describe("HistoryPage adjust panel excludes counter-fed grades", () => { + const CRACKED: EggGrade = { + id: "gr-cracked", farmId: "farm1", name: "Cracked", gradeType: "Quality", + sortOrder: 3, isSaleable: true, dailyEntryKind: "Cracked", active: true, + }; + + it("offers no grade field for a saleable, active condition grade", async () => { + mockListEggGrades.mockResolvedValue([GRADE_A, GRADE_B, CRACKED]); + mockListDailyEntries.mockResolvedValue([SUBMITTED]); + await openAdjustPanel(); + + const dialog = screen.getByRole("dialog"); + expect(within(dialog).getByLabelText("Grade A")).toBeInTheDocument(); + // Scoped to the dialog: the row behind it also renders the word, and the + // dialog's own Egg-counts half has a Cracked COUNTER that must stay. + expect(within(dialog).queryByLabelText("Cracked")).toBeInTheDocument(); + // ...so assert on the grade field specifically: the counter is the only + // "Cracked" control the dialog may show, and it is a count input, not a + // grade line. A grade field would make TWO. + expect(within(dialog).getAllByLabelText("Cracked")).toHaveLength(1); + }); +}); + describe("HistoryPage dialog dismissal", () => { it("closes the adjust dialog on Cancel without writing", async () => { mockListDailyEntries.mockResolvedValue([SUBMITTED]); diff --git a/web/src/routes/HistoryPage.tsx b/web/src/routes/HistoryPage.tsx index 725e70745..d57e51d49 100644 --- a/web/src/routes/HistoryPage.tsx +++ b/web/src/routes/HistoryPage.tsx @@ -157,9 +157,17 @@ export function HistoryPage() { // The entry's own lines (possibly deactivated grades — still correctable) // plus the active saleable catalog for adding a missed grade. + // + // #396 — a counter-fed grade is never offered for adding. It is excluded from + // the CATALOG half only, not from the entry's own lines: an existing line must + // stay correctable whatever it names, and a condition grade cannot legitimately + // be on an entry anyway (ConditionGradeGuard refuses it server-side). Adding + // one here would ask the server for a second lot on a grade its counter + // already produced, and be rejected. function panelGrades(e: DailyEntry): EggGrade[] { const onEntry = new Set(e.grades.map((g) => g.eggGradeId)); - return grades.filter((g) => onEntry.has(g.id) || (g.active && g.isSaleable)); + return grades.filter((g) => + onEntry.has(g.id) || (g.active && g.isSaleable && g.dailyEntryKind === "Manual")); } // #394: an adjustment has no draft state of its own — it replaces the diff --git a/web/src/routes/ProductsPage.test.tsx b/web/src/routes/ProductsPage.test.tsx index 44c1e9658..76b063636 100644 --- a/web/src/routes/ProductsPage.test.tsx +++ b/web/src/routes/ProductsPage.test.tsx @@ -43,8 +43,8 @@ const mockDeactivate = vi.mocked(deactivateProduct); const mockActivate = vi.mocked(activateProduct); const mockUpdateConversion = vi.mocked(updateEggUnitConversion); -const GRADE_A: EggGrade = { id: "g1", farmId: "f", name: "Grade A", gradeType: "Size", sortOrder: 1, isSaleable: true, active: true }; -const GRADE_B: EggGrade = { id: "g2", farmId: "f", name: "Grade B", gradeType: "Size", sortOrder: 2, isSaleable: true, active: true }; +const GRADE_A: EggGrade = { id: "g1", farmId: "f", name: "Grade A", gradeType: "Size", sortOrder: 1, isSaleable: true, dailyEntryKind: "Manual", active: true }; +const GRADE_B: EggGrade = { id: "g2", farmId: "f", name: "Grade B", gradeType: "Size", sortOrder: 2, isSaleable: true, dailyEntryKind: "Manual", active: true }; // KWD (3 decimals) throughout so a hard-coded 2-decimal money path fails: 500 // minor units renders "0.500 KWD" and typed "0.5" parses to 500 (not 50). diff --git a/web/src/routes/SalesPage.test.tsx b/web/src/routes/SalesPage.test.tsx index 10ae2c01f..957ee4541 100644 --- a/web/src/routes/SalesPage.test.tsx +++ b/web/src/routes/SalesPage.test.tsx @@ -52,7 +52,7 @@ const mockRecordPayment = vi.mocked(recordPayment); const CUSTOMER: Customer = { id: "c1", name: "Acme Eggs", phone: "555", email: null, address: null, note: null }; // Only gr1 is saleable → the picker offers PRODUCT_A only; gr2/PRODUCT_B exists // solely to resolve the second line's display name (allProducts). -const GRADE: EggGrade = { id: "gr1", farmId: "farm1", name: "Grade A", gradeType: "Size", sortOrder: 1, isSaleable: true, active: true }; +const GRADE: EggGrade = { id: "gr1", farmId: "farm1", name: "Grade A", gradeType: "Size", sortOrder: 1, isSaleable: true, dailyEntryKind: "Manual", active: true }; const PRODUCT_A: Product = { id: "p1", name: "Grade A Dozen", productType: "Egg", defaultUnit: "Dozen", defaultPriceMinorUnits: 300, currencyCode: "USD", currencyMinorUnit: 2, From f8a3b409ba97e4ce6af4f652369b80831f8f8f1a Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 3 Aug 2026 00:17:43 -0700 Subject: [PATCH 10/17] feat(seed): mint and sell condition lots in the demo and simulation fixtures (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes out #396's non-CI-caller obligation (AGENTS.md #395/#394): a change to what submit writes has to be carried into every automated caller in the same PR, and coverage over those callers is not uniform. What actually broke, and what did not — established by reading each caller's payload rather than by running it, since k6's `dailyEntryScreen` counts a tolerated 422 as a passing check and would have stayed green either way: * `SimulationDataSeeder` — BROKEN, and it would have failed closed. Its manifest asserts every count exactly, and `EggLots` was derived from `GradesPerDailyEntry` alone. Submit now also mints a lot per resolved condition counter, and this seeder's deterministic math keeps both counters strictly positive on every entry (`cracked = 4 + d % 3`, `dirty = 2 + d % 2`), so every submitted entry gained exactly two lots. `ValidateCounts` would have reported `Failed` on a correct seed. Added `ConditionLotsPerDailyEntry` beside the existing constant rather than merging the two into a `5`: they answer different questions (grade LINES vs counter-backed lots) and drift for different reasons. * Its grade lines name Large/Medium/Small explicitly, so `ConditionGradeGuard` never fires — checked, not assumed. * `DemoDataSeeder` — sound as written, same explicit grade lines. * `tools/simulation/k6/bundles.js` — unaffected. It posts no `grades` array at all and zeroes both counters, and it records without ever submitting, so neither the guard nor condition resolution is reachable. * Playwright — unaffected. `worker.spec.ts`'s `GRADES` list is exactly the five Manual saleable grades. `getByLabel("Cracked")` stays unambiguous for a reason worth recording: Cracked was previously non-saleable and so absent from the grading pane; it is now saleable but excluded by kind, so the counter remains the only match. `manager.spec.ts` submits with cracked=1 and now silently exercises condition-lot minting, then adjusts and voids it — all three paths hold. Both fixtures also gained a product mapped to a condition grade. Without one the feature reads as stock appearing that nothing can ever sell — ~180 lots of dead inventory in the simulation fixture — and #406 (stock reconciliation) does not exist yet. Priced below Small, which is the point of the feature. Also pins a guarantee that was implicit and unproven: voiding empties the condition lots too. `VoidDailyEntryHandler` reverses whatever `GetByDailyEntryLockedAsync` returns — grade-agnostic, so condition lots are covered by construction rather than by a second code path. Narrowing that query to the entry's grade lines (the exact shape this PR already fixed in `AdjustDailyEntryHandler`) would leave a voided day carrying live, sellable cracked and dirty stock with no entry behind it. Mutation-checked: baseline 12/12 green, the narrowed-query mutant leaves the cracked lot at 10 and fails the test, restored and rebuilt back to 12/12. Verified: 1253/1253 dotnet (114 application + 303 domain + 836 integration), 1340/1340 vitest across 69 files, typecheck clean, 0 build warnings. --- .../Persistence/DemoDataSeeder.cs | 7 +++ .../Persistence/SimulationDataSeeder.cs | 31 +++++++++-- .../QualityEggTests.cs | 53 +++++++++++++++++++ .../SimulationSeederTests.cs | 29 ++++++++-- 4 files changed, 112 insertions(+), 8 deletions(-) diff --git a/src/Cluckwork.Infrastructure/Persistence/DemoDataSeeder.cs b/src/Cluckwork.Infrastructure/Persistence/DemoDataSeeder.cs index 4c12ca40a..689c7ca38 100644 --- a/src/Cluckwork.Infrastructure/Persistence/DemoDataSeeder.cs +++ b/src/Cluckwork.Infrastructure/Persistence/DemoDataSeeder.cs @@ -249,10 +249,17 @@ private async Task SeedDemoAsync(Guid accountId, CancellationToken ct) // --- Products (#99): sales sell products, not raw grades. Prices are // per individual egg (unit Egg → factor 1), preserving the old demo math. + // The catalog is deliberately partial — Small has no product either, so + // the Stock screen shows a realistic mix of sellable and unlisted grades. var largeEggs = Require(await createProduct.HandleAsync(new CreateProductCommand( "Large Eggs", "Egg", "Egg", 45, grades["Large"], null), accountId, ct)); var mediumEggs = Require(await createProduct.HandleAsync(new CreateProductCommand( "Medium Eggs", "Egg", "Egg", 38, grades["Medium"], null), accountId, ct)); + // #396: the cracked counter now mints its own lot at submit, so the demo + // carries a discounted product for it — otherwise the feature reads as + // stock appearing that nothing can ever sell. Priced below Small. + Require(await createProduct.HandleAsync(new CreateProductCommand( + "Cracked Eggs", "Egg", "Egg", 18, grades["Cracked"], "Sold at a discount"), accountId, ct)); // --- Orders: one confirmed (exercises FIFO allocation), one open draft. var confirmed = Require(await createOrder.HandleAsync(new CreateSalesOrderCommand( diff --git a/src/Cluckwork.Infrastructure/Persistence/SimulationDataSeeder.cs b/src/Cluckwork.Infrastructure/Persistence/SimulationDataSeeder.cs index 3d68fc788..855599b98 100644 --- a/src/Cluckwork.Infrastructure/Persistence/SimulationDataSeeder.cs +++ b/src/Cluckwork.Infrastructure/Persistence/SimulationDataSeeder.cs @@ -66,8 +66,9 @@ namespace Cluckwork.Infrastructure.Persistence; // paths without any change — it's one entry per (flock, day), so the // production report and the daily-entries/daily-entry-grades/egg-lots export // datasets scale directly with HistoryDays (2 flocks × 90 days = 180 entries, -// ~500+ grade/lot rows once SubmitDailyEntry mints up to 3 egg lots per -// entry). Sales and expenses were the thin spot: Task 3b/3c's lifecycle +// ~500+ grade/lot rows once SubmitDailyEntry mints 3 graded + 2 condition +// (#396) egg lots per entry). Sales and expenses were the thin spot: Task +// 3b/3c's lifecycle // fixtures (draft/confirmed/partially-paid orders, the 4 expenses) all sat // inside the most recent ~week of history, so a sales/expense/profit report // or an export request against an OLDER slice of the window saw nothing. @@ -597,11 +598,21 @@ private async Task SeedFlockHistoryAsync( // One product per saleable grade, sold in individual eggs (factor 1) — // keeps order-item quantities directly comparable to lot sizes, which is // what makes the FIFO-contention shape below easy to reason about. + // + // #396: Cracked/Dirty are here for a different reason than the graded + // three. Submit now mints a lot for each condition counter, so without a + // product mapped to those grades the fixture accumulates stock that can + // never leave — ~180 lots of dead inventory the sales screens can see and + // nothing can sell. Priced below Small, which is the point of the feature + // (a cracked egg is saleable, at a discount). Nothing here orders them; + // the FIFO-contention shape above stays a Large/Medium story. private static readonly (string GradeName, string ProductName, long PriceMinorUnits)[] CatalogWanted = [ ("Large", "Sim Large Eggs", 45), ("Medium", "Sim Medium Eggs", 38), ("Small", "Sim Small Eggs", 30), + ("Cracked", "Sim Cracked Eggs", 18), + ("Dirty", "Sim Dirty Eggs", 22), ]; private static readonly (string Name, string Phone, string Note)[] CustomersWanted = @@ -1126,6 +1137,20 @@ private static void Require(Result result, string what) // not merely "at least one" (#279 Fix 2). private const int GradesPerDailyEntry = 3; + // #396: submit ALSO mints a lot for each condition counter that resolved + // to a condition grade (DailyEntryKind.Cracked/Dirty) and carried a + // positive count. Kept separate from GradesPerDailyEntry rather than + // folded into it: these are NOT grade lines — no GradeQuantityDto names + // them, and ConditionGradeGuard actively refuses one that does — so the + // two constants answer different questions and drift for different + // reasons. Both counters are structurally positive here (`cracked = 4 + + // d % 3` >= 4, `dirty = 2 + d % 2` >= 2, for every day and baseline), and + // the base seed ships Cracked/Dirty active + saleable, so both resolve on + // every submitted entry — exactly 2, never "up to 2". If a future fixture + // lets either counter reach 0, SubmitDailyEntryHandler skips that lot and + // this must become a per-entry sum instead of a constant. + private const int ConditionLotsPerDailyEntry = 2; + // Mirrors SeedSalesAsync's two EnsureDraftOrderAsync calls (customer1/2, // never confirmed) and its three lifecycle EnsureConfirmedOrderAsync // calls (customer1/2 unpaid + customer3 partially paid) — the recurring @@ -1330,7 +1355,7 @@ private static SimulationExpectedCounts ComputeExpectedCounts(SimulationOptions DraftEntries: draftEntries, SubmittedEntries: submittedEntries, LockedEntries: lockedEntries, - EggLots: GradesPerDailyEntry * submittedOrLockedEntries, + EggLots: (GradesPerDailyEntry + ConditionLotsPerDailyEntry) * submittedOrLockedEntries, SalesOrdersTotal: DraftOrdersCount + salesOrdersConfirmed, SalesOrdersDraft: DraftOrdersCount, SalesOrdersConfirmed: salesOrdersConfirmed, diff --git a/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs b/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs index e2a9bd4d8..a9e3d6f59 100644 --- a/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/QualityEggTests.cs @@ -178,6 +178,59 @@ await factory.WithTenantScopeAsync(accountId, async db => }); } + // Voiding is the destructive path, and the one where "condition lots are + // just lots" has to actually hold. VoidDailyEntryHandler reverses whatever + // GetByDailyEntryLockedAsync returns — every lot linked to the entry, + // grade-agnostic — so the condition lots are covered by construction rather + // than by a second code path. That is worth pinning precisely BECAUSE it is + // implicit: a future narrowing of that query to the entry's grade LINES + // (the shape AdjustDailyEntryHandler originally had, and the bug this PR + // fixed there) would leave a voided day carrying live, sellable cracked and + // dirty stock with no entry standing behind it — phantom inventory, and + // silent, since nothing else looks at a voided entry again. + [Fact] + public async Task Voiding_empties_the_condition_lots_too() + { + var (client, accountId, farmId, flockId, manual, crackedId, dirtyId) = await SetupAsync(); + var manualGradeId = manual["Large"]; + + var entryId = await SubmitAsync(client, farmId, flockId, manualGradeId); + var version = (await StateOf(accountId, entryId)).Version; + + var response = await client.PostWithKeyAsync( + $"/api/v1/daily-entries/{entryId}/void", Guid.NewGuid().ToString(), + new { version, reason = "recount void" }); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + await factory.WithTenantScopeAsync(accountId, async db => + { + var lots = await db.EggLots.IgnoreQueryFilters() + .Where(l => l.DailyEntryId == entryId).ToListAsync(); + + // All three lots survive as rows (#60 — nothing is deleted, only + // emptied), and every one of them is now at zero. Asserting the + // manual lot alongside the two condition ones is what makes this a + // test of the WHOLE reversal rather than of the condition half: + // a change that emptied only the condition lots would be just as + // wrong, and would pass an assertion that named them alone. + Assert.Equal(3, lots.Count); + Assert.Equal(0, Assert.Single(lots, l => l.EggGradeId == manualGradeId).QuantityProduced); + Assert.Equal(0, Assert.Single(lots, l => l.EggGradeId == crackedId).QuantityProduced); + Assert.Equal(0, Assert.Single(lots, l => l.EggGradeId == dirtyId).QuantityProduced); + + // The eggs left on the ledger, not merely off the lot (#101). One + // Void movement per lot that had stock, so a reversal that zeroed a + // quantity without writing the movement is caught here rather than + // showing up later as stock that cannot be reconciled. + var lotIds = lots.Select(l => l.Id).ToList(); + var voided = await db.EggInventoryMovements.IgnoreQueryFilters() + .Where(m => m.MovementType == EggMovementType.Void && lotIds.Contains(m.EggLotId)) + .ToListAsync(); + Assert.Equal(3, voided.Count); + Assert.Equal(-615, voided.Sum(m => m.QuantityDelta)); // 600 + 10 + 5 + }); + } + [Fact] public async Task A_non_saleable_condition_stays_a_loss() { diff --git a/tests/Cluckwork.Api.IntegrationTests/SimulationSeederTests.cs b/tests/Cluckwork.Api.IntegrationTests/SimulationSeederTests.cs index a85143dd9..87b31bac5 100644 --- a/tests/Cluckwork.Api.IntegrationTests/SimulationSeederTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/SimulationSeederTests.cs @@ -362,7 +362,14 @@ public async Task SimulationSeed_SeedsProductCatalogAndCustomers() var products = await db.Products.IgnoreQueryFilters() .Where(p => p.AccountId == SeedDefaults.AccountId) .ToListAsync(); - foreach (var name in new[] { "Sim Large Eggs", "Sim Medium Eggs", "Sim Small Eggs" }) + // #396: Cracked/Dirty are saleable grades that submit now mints lots + // for, so the catalog covers them too — without a product those lots + // are stock nothing can sell. + foreach (var name in new[] + { + "Sim Large Eggs", "Sim Medium Eggs", "Sim Small Eggs", + "Sim Cracked Eggs", "Sim Dirty Eggs", + }) Assert.Contains(products, p => p.Name == name); var customers = await db.Customers.IgnoreQueryFilters() @@ -471,12 +478,13 @@ public async Task SimulationSeed_Sales_IsIdempotent_NoDuplicatesAfterTwoHostStar .ToListAsync(); // A second full seed pass converges rather than doubling — same - // three products, three customers, six orders (2 draft + 2 + // five products (#396 added the two condition grades), three + // customers, six orders (2 draft + 2 // confirmed-unpaid + 1 confirmed-partially-paid + 1 recurring // confirmed, #243 Task 3d's RecurringStartDay/RecurringCadenceDays // drip — exactly one point lands inside a 12-day HistoryDays window) // as a single pass. - Assert.Equal(3, products.Count); + Assert.Equal(5, products.Count); Assert.Equal(3, customers.Count); Assert.Equal(6, orders.Count); Assert.DoesNotContain(products.GroupBy(p => p.Name), g => g.Count() > 1); @@ -813,6 +821,13 @@ private static async Task ReadManifestAsync(string path) private const int ExpectedDraftWindowDays = 2; private const int ExpectedGradesPerDailyEntry = 3; + // #396: submit also mints a lot for each condition counter that resolved to + // a condition grade. Kept as its own constant, mirroring the seeder's + // ConditionLotsPerDailyEntry — the two answer different questions (grade + // LINES vs counter-backed lots) and a single merged "5" would hide which + // half moved if either changes. + private const int ExpectedConditionLotsPerDailyEntry = 2; + [Fact] public async Task SimulationSeed_EmitsACompleteManifestWithValidatedCountsAndLifecycleStates() { @@ -846,9 +861,13 @@ public async Task SimulationSeed_EmitsACompleteManifestWithValidatedCountsAndLif // #279 review Fix 2: one lot per grade (Large/Medium/Small) for every // entry that reached Submitted — Draft entries (the most recent // ExpectedDraftWindowDays per flock) never call SubmitDailyEntryHandler, - // so they mint no lots. + // so they mint no lots. #396 adds the two condition lots (Cracked and + // Dirty), which the seeder's own deterministic counter math keeps + // strictly positive on every entry, so they are minted on exactly the + // same population. Assert.Equal( - ExpectedGradesPerDailyEntry * 2 * (SimulationSeedFactory.HistoryDays - ExpectedDraftWindowDays), + (ExpectedGradesPerDailyEntry + ExpectedConditionLotsPerDailyEntry) + * 2 * (SimulationSeedFactory.HistoryDays - ExpectedDraftWindowDays), counts.EggLots); Assert.Equal(2 + ExpectedConfirmedOrders, counts.SalesOrdersTotal); Assert.Equal(1, counts.Payments); From 082aa15d184a381f1c2d35320f01d05e835f27f0 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 3 Aug 2026 00:33:54 -0700 Subject: [PATCH 11/17] fix(reports): include condition stock in the By-grade breakdown (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of #407, P1 — a real defect and mine. `GetProductionAsync` returns both `TotalFromCounts` (eggs that became stock without being hand-graded) and `GradeTotals` (the By-grade breakdown), and the breakdown was built from `DailyEntryGrades` alone. Condition-backed production creates an `EggLot` but never a `DailyEntryGrade` row — and never can, because `ConditionGradeGuard` refuses a manual line naming a condition grade precisely so one day cannot produce two lots for one grade. So the same response reported N cracked eggs as produced stock while the By-grade table omitted every one of them: `sum(GradeTotals)` equalled `TotalSellable` when the stock that exists is `TotalSellable + TotalFromCounts`. Folds the snapshot-backed counters in under the grade each entry actually resolved to, keyed on the SNAPSHOT rather than the live catalog, so turning a grade off later still cannot rewrite a past period. Two queries rather than one: the counters sit in different columns under different snapshot keys, which a single GROUP BY cannot express without a UNION EF would not translate. Gated on `counter > 0`, mirroring SubmitDailyEntryHandler's own lot condition — a snapshot is recorded even on a zero-cracked day, and counting those would put a grade in the breakdown the farm holds no stock under, reading as an empty grade rather than as absence. The merge accumulates rather than assigns, so a farm that pointed both kinds at one catalog row gets correct arithmetic instead of a silently dropped counter. The assertion that states the guarantee is the arithmetic a reader of the screen actually does: `sum(GradeTotals) == TotalSellable + TotalFromCounts`. The existing fixture already had the ideal shape (Cracked saleable, Dirty not), so the test also pins the other half — Dirty must stay absent, since folding the counters in must not resurrect conditions the entry recorded as losses. Mutation-checked, and the first attempt was a false pass worth recording: an `if (false)` mutant failed to COMPILE under warnings-as-errors (CS0162), so `dotnet test --no-build` ran the stale binary and reported 7/7 green while testing nothing. Redone with a mutant that compiles: baseline 7/7 green, the merge removed fails the test, restored and rebuilt back to 7/7. No SPA change needed — ReportsPage maps whatever `gradeTotals` the API returns. Also folds in the #396 specification section from the closed #397, which was the only place it existed; specs.md on main had no section for this behaviour at all. Corrected one sentence that had gone stale against the single-migration rule: it described migrating "existing official entries" with null snapshots, but a virgin database is the only starting state, so there is no such population. Restated as a property of the columns (never backfilled, null is the durable "was a loss that day" answer) rather than as a migration step. Verified: 1253/1253 dotnet, 1340/1340 vitest across 69 files, 0 build warnings. --- specs/product/specs.md | 70 +++++++++++++++++++ .../Repositories/ReportQueries.cs | 52 +++++++++++++- .../ReportsTests.cs | 21 ++++++ 3 files changed, 140 insertions(+), 3 deletions(-) diff --git a/specs/product/specs.md b/specs/product/specs.md index 17b495895..cf2a0d0cd 100644 --- a/specs/product/specs.md +++ b/specs/product/specs.md @@ -901,6 +901,7 @@ egg_grades - farm_id - name - grade_type: size / quality / custom +- daily_entry_kind: manual / cracked / dirty - sort_order - is_saleable - is_default @@ -922,6 +923,26 @@ Internal Use Seconds ``` +The default Cracked and Dirty quality grades are active and saleable on a new +installation. A farm may independently mark either grade non-saleable. + +`daily_entry_kind` is immutable and makes the one grade fed by a Daily Entry +condition counter explicit. It is not inferred from a mutable grade name: + +```text +manual = entered in the Grading pane +cracked = populated from the Cracked counter +dirty = populated from the Dirty counter +``` + +There is at most one Cracked and one Dirty kind per farm. The manual Grading +pane excludes both condition grades, and the record and adjust handlers reject +a manual line naming either — so a condition egg cannot be entered a second +time as a manual grade, whether through the app or a direct API call. The +server check is what makes that true: manual lines are otherwise accepted on +saleability alone, and a condition grade named manually would produce a second +lot for the same grade alongside its counter-backed one. + ## 9.2 Daily production by grade ```text @@ -939,6 +960,44 @@ daily_egg_grade_entries - updated_at ``` +Daily Entry retains the three observed condition counters (`cracked`, `dirty`, +and `discarded`). Cracked and Dirty are saleable only when the configured +condition grade is **both active and saleable**. Both flags are required and +neither implies the other: deactivating a grade leaves its saleable flag +untouched, so an inactive-but-saleable grade is an ordinary reachable state, +and resolving on saleability alone would mint stock under a grade the farm has +already retired from capture. On the first official save, the entry snapshots +the resolved Cracked and Dirty grade id (or null when that condition is a loss, +whether because the grade is non-saleable or because it is inactive). The +snapshot controls future reporting and lot reconciliation, so a later catalog +change never reclassifies historical production. + +The manual grade lines must reconcile exactly when an entry becomes official: + +```text +manual grade total = total eggs − cracked − dirty − discarded +``` + +Drafts may remain incomplete. Submitting or manager-adjusting must satisfy the +equation. The submitted entry creates one lot per manual grade, plus one lot +for each non-null condition snapshot whose counter is positive that day — a +snapshot is always recorded once its condition grade is both active and +saleable, but a zero-cracked or zero-dirty day creates no lot for that +condition, since a lot cannot hold a zero quantity. Discarded eggs are always a +loss. Total marketable production is the manual grade total plus any Cracked +and Dirty counts represented by a non-null snapshot. + +The snapshot columns are nullable and are never backfilled: the system does not +create lots for, or retroactively convert, production recorded before its +condition grade resolved. Any entry that became official without a resolved +condition keeps a null snapshot permanently, and reads treat that null as the +durable answer "this condition was a loss that day" rather than as missing data. + +(Stated as a property of the columns rather than as a migration step. Under the +single-migration rule a virgin database is the only starting state, so there is +no population of pre-existing entries for a backfill to visit — and none should +ever be added.) + ## 9.3 Egg lots ```text @@ -1055,6 +1114,11 @@ Phase 1 assumes eggs are **graded at collection**: the daily grade entry (§9.2) the final grade of each egg. The lot's grade is fixed at production. This matches farms that candle/sort during collection and keeps the walking skeleton simple. +Cracked and Dirty are final grades at collection too. Their counts are entered +in Egg counts rather than the manual Grading pane, then materialize under the +entry's snapshotted condition grade only when that grade is both active and +saleable (§9.2). + Phase 1 does **not** model a separate post-collection grading or wash/pack shift, nor moving eggs between grades after the lot exists. @@ -1134,6 +1198,12 @@ Example: | Large Eggs | egg | Large | | Medium Eggs | egg | Medium | | Cracked Eggs | egg | Cracked | +| Dirty Eggs | egg | Dirty | + +Each product owns its own optional default price. Cracked and Dirty products +can be priced lower, but Cluckwork does not derive or enforce a discount from +another product: each farm sets the price appropriate to its market. A sale +continues to snapshot the actual selected product price. ## 10.3 Customers diff --git a/src/Cluckwork.Infrastructure/Repositories/ReportQueries.cs b/src/Cluckwork.Infrastructure/Repositories/ReportQueries.cs index 5fe82f141..20b8033f1 100644 --- a/src/Cluckwork.Infrastructure/Repositories/ReportQueries.cs +++ b/src/Cluckwork.Infrastructure/Repositories/ReportQueries.cs @@ -45,8 +45,53 @@ public async Task GetProductionAsync( .GroupBy(g => g.EggGradeId) .Select(g => new { EggGradeId = g.Key, Quantity = g.Sum(x => x.Quantity) }) .ToListAsync(ct); + + // #396 — condition-backed production has NO DailyEntryGrade row and + // never can: ConditionGradeGuard refuses a manual line naming a + // condition grade precisely so one day cannot produce two lots for one + // grade. So a breakdown built from DailyEntryGrades alone omits every + // cracked and dirty egg that this same response counts as produced + // stock in FromCounts/TotalFromCounts — the "By grade" table would sum + // to the hand-graded remainder while the header reported more. Fold the + // snapshot-backed counters in under the grade each entry actually + // resolved to, keyed on the SNAPSHOT (never the live grade catalog), so + // the breakdown reconciles with the stock that exists. + // + // Two queries rather than one: the counters live in different columns + // under different snapshot keys, so a single GROUP BY cannot express it + // without a UNION EF would not translate. + // + // `Counter > 0` mirrors SubmitDailyEntryHandler's own lot condition — + // a snapshot is recorded even on a zero-cracked day, and counting those + // would put a grade in the breakdown the farm holds no stock under, + // which reads as an empty grade rather than as absence. + var crackedTotals = await db.DailyEntries + .Where(e => e.Date >= from && e.Date <= to && OfficialStatuses.Contains(e.Status) + && e.CrackedGradeId != null && e.CrackedEggs > 0) + .GroupBy(e => e.CrackedGradeId!.Value) + .Select(g => new { EggGradeId = g.Key, Quantity = g.Sum(e => e.CrackedEggs) }) + .ToListAsync(ct); + + var dirtyTotals = await db.DailyEntries + .Where(e => e.Date >= from && e.Date <= to && OfficialStatuses.Contains(e.Status) + && e.DirtyGradeId != null && e.DirtyEggs > 0) + .GroupBy(e => e.DirtyGradeId!.Value) + .Select(g => new { EggGradeId = g.Key, Quantity = g.Sum(e => e.DirtyEggs) }) + .ToListAsync(ct); + + // Accumulate rather than assign. A condition grade cannot collide with + // a DailyEntryGrade row today (the guard above), but Cracked and Dirty + // could in principle be bound to the SAME grade id by a farm that + // pointed both kinds at one catalog row — accumulating keeps that + // arithmetic right instead of silently dropping one counter. + var combined = new Dictionary(); + foreach (var t in gradeTotals) + combined[t.EggGradeId] = combined.GetValueOrDefault(t.EggGradeId) + t.Quantity; + foreach (var t in crackedTotals.Concat(dirtyTotals)) + combined[t.EggGradeId] = combined.GetValueOrDefault(t.EggGradeId) + t.Quantity; + var gradeNames = await db.EggGrades - .Where(g => gradeTotals.Select(t => t.EggGradeId).Contains(g.Id)) + .Where(g => combined.Keys.Contains(g.Id)) .ToDictionaryAsync(g => g.Id, g => g.Name, ct); // Hen-days (spec §19.3): the farm's bird count on each day, summed over @@ -142,13 +187,14 @@ public async Task GetProductionAsync( days.Sum(x => x.Deaths), totalHenDays, totalHenDays > 0 ? Math.Round(totalEggs * 100m / totalHenDays, 1) : null, - gradeTotals + combined .Select(t => new GradeTotal( - t.EggGradeId, gradeNames.GetValueOrDefault(t.EggGradeId, "?"), t.Quantity)) + t.Key, gradeNames.GetValueOrDefault(t.Key, "?"), t.Value)) .OrderByDescending(t => t.Quantity) .ToList()); } + public async Task GetSalesAsync( DateOnly from, DateOnly to, CancellationToken ct = default) { diff --git a/tests/Cluckwork.Api.IntegrationTests/ReportsTests.cs b/tests/Cluckwork.Api.IntegrationTests/ReportsTests.cs index 15ade3ad6..2c2544919 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ReportsTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ReportsTests.cs @@ -84,6 +84,27 @@ await factory.WithTenantScopeAsync(accountId, async db => Assert.Equal(6, day.FromCounts); // cracked only — dirty stayed a loss Assert.Equal(90, report.TotalSellable); Assert.Equal(6, report.TotalFromCounts); + + // #396 (codex review of #407): the "By grade" breakdown has to account + // for the condition stock this same response just reported as produced. + // Condition production creates an EggLot but NEVER a DailyEntryGrade row + // — ConditionGradeGuard refuses a manual line naming a condition grade, + // so one can never exist — and a breakdown built from DailyEntryGrades + // alone therefore omitted every cracked egg while the header counted it. + Assert.Equal(90, Assert.Single(report.GradeTotals, g => g.Name == "Large").Quantity); + Assert.Equal(6, Assert.Single(report.GradeTotals, g => g.Name == "Cracked").Quantity); + + // Dirty was non-saleable, so it resolved to nothing and stayed a loss. + // Its absence is the other half of the guarantee: folding the counters + // in must not resurrect the ones the entry recorded as losses. + Assert.DoesNotContain(report.GradeTotals, g => g.Name == "Dirty"); + + // The whole point, stated as the arithmetic a reader of the screen does: + // the breakdown sums to the hand-graded remainder PLUS the condition + // stock, not to the remainder alone. + Assert.Equal( + report.TotalSellable + report.TotalFromCounts, + report.GradeTotals.Sum(g => g.Quantity)); } [Fact] From 6c84403996071a70027004e244be597becd474b9 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 3 Aug 2026 01:11:54 -0700 Subject: [PATCH 12/17] docs(migrations): make #407 the cutover to one migration per change (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Until now a schema change was hand-folded into `InitialCreate` (#245). That was only safe while the application had never been deployed: no database had applied that file yet, so rewriting it was free. This PR is the last change permitted to do it — from the commit that merges it, a database exists which has already applied `InitialCreate`, EF will never re-run it, and a hand-folded column is a column that silently does not exist. That failure mode is why this is worth a rule rather than a note: it surfaces as broken behaviour, not as a migration error. #399's amendment did exactly this to the dev database and presented as a failing login. The load-bearing part is NOT the prose. `MigrationSecurityReviewTests` asserted `Assert.Single(AllMigrations())`, so the first post-cutover PR to add a migration would have gone red no matter what the docs said. Documenting the cutover without moving that fence would have been a no-op. Replaced it with a guard that still has teeth, rather than deleting it: `InitialCreate` must stay FIRST and keep its recorded id (`20260801190854_InitialCreate`). Regenerating it mints a new timestamp, which reorders it behind the migrations that depend on it and desynchronises `__EFMigrationsHistory` on every deployed database — now a red test instead of a failed deploy. Ordering is asserted on the migration ID, never on list position: `AllMigrations()` is built from `Assembly.GetTypes()`, whose order is explicitly unspecified, so a positional check would pass or fail on reflection ordering. Relaxing that one test is safe because every other assertion in the file already fans out across the whole migration history (`AllInsertDataOperations` / `AllSqlOperations`) — the credential and reference-data guarantees extend to post-cutover migrations for free. They were written that way from the start. Mutation-checked: pointing the pinned id at a different timestamp fails the test; restored and rebuilt back to green. Docs updated in the three places the rule is actually stated or implied: - AGENTS.md's migration bullet — the canonical rule. - README gains a "Changing the database schema" section, which did not exist: the rule lived only in AGENTS.md, so a human following the README had nothing telling them to add a migration. Includes the #318 fail-closed design-time connection and its loopback opt-out, since `ef migrations add` needs both. - Both note that a dev database created BEFORE this merges predates the folded-in columns and must be recreated. `deploy/README.md`'s #364 line is deliberately untouched — it describes what was true of that release at the time, not a live rule. Verified: 1253/1253 dotnet, 0 build warnings. --- AGENTS.md | 8 ++- README.md | 32 ++++++++++ .../MigrationSecurityReviewTests.cs | 63 +++++++++++++++---- 3 files changed, 89 insertions(+), 14 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 511f2f62e..37735146e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,7 +43,13 @@ dotnet test Cluckwork.sln # 688 tests as of 2026-07; integratio - **Auth:** asymmetric JWT + rotating refresh tokens. PEM keys come from config with escaped `\n`; normalize via `PemKey.Normalize` before `ImportFromPem`. Integration tests generate an ephemeral RSA pair at test-process startup (`TestJwtKeys` in `CluckworkWebApplicationFactory.cs`) — no real key material is ever committed to source (`deploy/.env.example` carries PEM armor with a `replace-me` body, not a usable key). - **Credential epoch revocation (#364):** every access and refresh token is bound to the user's `CredentialEpoch`; all password-reset paths bump it so already-issued credentials fail on their next use. Epoch **`0` is permanently retired**: users start at `1`, legacy refresh rows start at `0`, and a missing or malformed `credential_epoch` claim is always a mismatch — never default the parsed claim to the current database epoch or otherwise treat it as an exemption. `CredentialEpochMiddleware` deliberately performs a fresh database read for every authenticated request; the round trip is the fail-closed revocation guarantee, so do not cache it. Keep the middleware after `TenantResolutionMiddleware` (the lookup is tenant-scoped) and before `MustChangePasswordMiddleware`; `/api/v1/auth/logout` is deliberately exempt so a superseded credential can still terminate its own session. Login and step-up snapshot the epoch + security stamp that the password actually verified **before** clearing the failed-access count, then refuse issuance if a concurrency-loss reload reveals different credentials — never let an old password adopt a concurrent reset. Their disabled-account branches still pay exactly one real password-hash cost, but must not mutate failed-access/lockout state; `AccountLockout` applies the same boundary after a concurrency reload, stopping rather than charging a stale failure to a newly disabled or reset credential. The SPA serializes refresh and self-password-change through the same per-tab queue + cross-tab Web Lock because both replace the shared HttpOnly cookie — a late old-epoch refresh must never overwrite the fresh pair that keeps the self-changing device signed in. This epoch is distinct from #283's first-login password-change flag, #265's operator-only break-glass temporary password, and #308's single-use browser step-up grant: the mechanisms have different audiences and lifetimes and none substitutes for another. - **Migrations + base provisioning (#283):** EF migrations auto-apply on startup (`Database:MigrateOnStartup`, default true — but **Production sets it `false`**; the `migrate` command runs DDL out-of-band, see the #263 bullet below). The default account, the four assignable roles (`Roles.Assignable`), the default egg grades, and the default packed-unit conversions are **static reference data baked into the migrations themselves** — deterministic and multi-instance-safe by construction, **no runtime seeder, no `Seed:*` config**. They ship as **hand-written `migrationBuilder.Sql` with `WHERE NOT EXISTS` guards, deliberately NOT EF's `HasData`/`InsertData`** — `HasData` keys on the PRIMARY KEY, which assumes a virgin schema, but every install that ever booted pre-#283 already has these rows under *different* ids (the old `DatabaseSeeder` minted roles/grades/conversions with `Guid.NewGuid()`), so `InsertData` either collides on `PK_Accounts` or silently violates the real natural-key unique indexes. The #245 squash retires *that* particular reason (a virgin database is now the only starting state), but **do not "regenerate" these into `HasData`**: `HasData` rows become part of the **model**, and every one of these tables is user-mutable through the app (Settings renames the account, `PUT /api/v1/egg-grades/{id}` renames a grade, `EggUnitConversion.Update` retunes `EggsPerUnit`), so a later model-diff would emit `UpdateData`/`DeleteData` that silently reverts the farm's own edits. Raw SQL seeds once and then leaves the rows alone forever. The guard's scope differs per batch, and the rule is *whether the user can change the natural key or remove a set member*: the **egg grades** batch is gated on the account's grade catalog being **entirely empty** (whole-set), because grades are user-managed — `PUT /api/v1/egg-grades/{id}` renames one, and a per-name guard cannot see a `Small` the farm renamed to `Pullet`, so it would resurrect an active, saleable duplicate (the old seeder skipped the whole set for exactly this reason); **roles, unit conversions, and the default account** stay **per-key**, because their keys are not user-mutable (no role CRUD exists at all; `EggUnitConversion.Update` cannot change `UnitCode` and there is no create/delete; the account's key is a fixed id), and per-key additionally lets a later-added role or unit code back-fill into an older install. `MigrationSecurityReviewTests` pins the SQL shape (12 INSERT statements, 21 rows, every one `WHERE NOT EXISTS`-guarded, the grades guard blind to `"Name"`) and `BaseReferenceDataMigrationTests` pins the result against a real virgin Postgres. `MigrationSecurityReviewTests` also asserts no migration ever inserts a credential-shaped row (`AspNetUsers`, `PasswordHash`, `SecurityStamp`, …) — the schema itself is never a place a credential can hide. The first admin is a **separate, deliberately NOT migration-baked** concern — see the #283 bullet below. -- **One migration, squashed (#245):** `Persistence/Migrations/` holds exactly **one** migration, `InitialCreate`, generated from the current model on 2026-08-01. It replaced the 34 that had accumulated since 2026-06-27 (12 of them carrying raw SQL written for mid-history data states — backfills that touch 0 rows on an empty database but still execute). Done once, before the first production deploy, precisely because no `__EFMigrationsHistory` anywhere needed baselining; **it cannot be repeated after go-live**. Any database still carrying the old history (a dev DB from `deploy/docker-compose.dev.yml`, an old demo/sim instance) can no longer migrate forward — **drop and recreate**: `docker compose -f deploy/docker-compose.dev.yml down -v && docker compose -f deploy/docker-compose.dev.yml up -d`. Two things in that migration are **hand-carried and un-regenerable** — a plain `dotnet ef migrations add` drops both on the floor, so never regenerate `InitialCreate` without re-adding them: (1) the four **`lower("Name")` expression unique indexes** (`IX_EggGrades_AccountId_FarmId_LowerName`, `IX_ExpenseCategories_NameCi`, `UX_InventoryItems_Account_Farm_LowerName`, `IX_Products_AccountId_LowerName`) — EF cannot model a functional index, and losing them loses four uniqueness constraints; (2) the **static base reference data** raw SQL (see the bullet above). `MigrationSecurityReviewTests` and `BaseReferenceDataMigrationTests` fail if either goes missing. +- **Migrations: `InitialCreate`, then one per change — the #407 cutover.** `Persistence/Migrations/` began as exactly **one** migration, `InitialCreate`, generated from the current model on 2026-08-01. It replaced the 34 that had accumulated since 2026-06-27 (12 of them carrying raw SQL written for mid-history data states — backfills that touch 0 rows on an empty database but still execute). Done once, before the first production deploy, precisely because no `__EFMigrationsHistory` anywhere needed baselining. + + **That squash is over, and so is the rule that came with it.** While the app had never been deployed, a schema change was *hand-folded into `InitialCreate`* — legitimate only because no database had applied that file yet. **PR #407 is the last change permitted to do that.** From the commit that merges it, **every schema change gets its own migration** (`dotnet ef migrations add `); `InitialCreate` is frozen. EF never re-runs an already-applied migration, so a hand-folded column is a column that **silently does not exist** on any database that already booted — and it fails as broken behaviour, not as a migration error. That is not hypothetical: it is exactly what #399's amendment did to the dev database, surfacing as a broken login. `MigrationSecurityReviewTests` enforces the new shape — `InitialCreate` must stay **first** and keep its recorded id `20260801190854_InitialCreate` — instead of the old `Assert.Single` fence. + + **Never regenerate `InitialCreate`.** Two things in it are **hand-carried and un-regenerable** — a plain `dotnet ef migrations add` drops both on the floor: (1) the four **`lower("Name")` expression unique indexes** (`IX_EggGrades_AccountId_FarmId_LowerName`, `IX_ExpenseCategories_NameCi`, `UX_InventoryItems_Account_Farm_LowerName`, `IX_Products_AccountId_LowerName`) — EF cannot model a functional index, and losing them loses four uniqueness constraints; (2) the **static base reference data** raw SQL (see the bullet above). Regenerating also mints a **new timestamp**, which reorders it behind the migrations that depend on it and desynchronises `__EFMigrationsHistory` on every deployed database. `MigrationSecurityReviewTests` and `BaseReferenceDataMigrationTests` fail if any of that goes missing. + + A database predating the squash (an old dev DB from `deploy/docker-compose.dev.yml`, an old demo/sim instance) still cannot migrate forward — **drop and recreate**: `docker compose -f deploy/docker-compose.dev.yml down -v && docker compose -f deploy/docker-compose.dev.yml up -d`. Same for any dev database created *before* #407 merged, since the columns it folded in will otherwise be absent. - **Seed command (#280, #284):** demo sample data is NOT boot-seeded — there is no `Seed__Demo` flag anymore. Run it explicitly, against a database that has **already been booted once** so the base seed above exists (default account, `Admin` role, default egg grades — the demo seed needs all three and fails fast with a clear message if they're missing): `ASPNETCORE_ENVIRONMENT=Development dotnet Cluckwork.Api.dll seed --profile demo` (an unset `ASPNETCORE_ENVIRONMENT` defaults to Production, which the guard below blocks — always set it for a local/dev/CI run). The command migrates the schema, runs `DemoDataSeeder`, then exits (Kestrel never starts); it is **authoritative** (seeds regardless of any config) and **fail-loud** (a real exit code: `0` only when data was actually seeded or was already present, non-zero with an stderr message otherwise — never a silent no-op). `DemoDataSeeder` is registered only when `!IsProduction()` (defense-in-depth): running the command against a Production-env process fails with a clear message instead of writing fake data. Intended flow: the serving process for a real farm stays Production; a non-Production process (dev box, CI, or a sim/load-test harness) runs `seed --profile demo` against its own already-base-seeded database. The **`simulation` profile** (#279) sits in the same `switch` with the same authoritative + fail-loud + Production-guarded contract (`SimulationDataSeeder`); it additionally records its date anchor + a completion marker in a durable `SimulationSeedState` row (anchor written before the first fixture mutation; completion stamped only after the manifest succeeds). A clean re-run — even one straddling a UTC-midnight rollover — reuses that anchor and converges to `AlreadySeeded` with an unchanged manifest fingerprint instead of writing a shifted duplicate set; `AlreadySeeded` is reported only when the counts are unchanged, so a `SimulationOptions` change that adds fixtures re-runs as `Seeded`. The durable anchor can't be poisoned by a mid-seed crash or by foreign daily entries a load test writes into the account (a data-derived anchor could); but a polluted account still fails the exact count validation **closed** (`Failed`) rather than certifying a fixture that no longer matches the definition. - **Break-glass recovery (#265):** `recover-admin` is a second one-off CLI verb on the same binary, same run-then-exit shape as `seed` — but deliberately **NOT** environment-gated (it must work against a real Production database): `dotnet Cluckwork.Api.dll recover-admin --email [--account ] [--reason ]`. For a locked-out account (the sole-Owner-lost-password case, no email/SMTP reset path) it, in one transaction, resets to a **freshly generated** temporary password (never one passed on the command line), rotates the security stamp, revokes every refresh token, and writes a conspicuous `User.BreakGlassReset` audit row carrying `--reason`. The temp password is printed to **stdout only** (never the logger/OTLP) and exit `0`; failures go to stderr with exit `1` and change nothing. `AdminRecoveryService` orchestrates; `IdentityProvider.BreakGlassResetAsync` shares the reset/revoke core with `SetUserPasswordAsync`. Full procedure + verification drill: `docs/runbooks/break-glass-account-recovery.md`. - **First-run admin provisioning (#283):** `bootstrap-admin` is a fourth one-off CLI verb, same run-then-exit shape as `seed`/`migrate`, always available in Production (same posture as `recover-admin`): `dotnet Cluckwork.Api.dll bootstrap-admin --email `. It migrates the schema (idempotent, like the other verbs), then — **only if the default account has no Owner yet** — creates one with a **freshly generated** password and `ApplicationUser.MustChangePassword = true`, printed to **stdout only** (never the logger/OTLP, identical rule to `recover-admin`). A re-run against an already-provisioned account is a silent no-op: no second Owner, no password reprinted. `FirstRunAdminService` orchestrates (mirrors `AdminRecoveryService`'s CLI-wrapper-calls-a-service shape); `IIdentityProvider.CreateUserAsync` gained an optional `mustChangePassword` parameter (default `false` — an ordinary Users-page-created user is never gated) that this is the only caller to pass `true`. While the flag is set: the JWT carries a `must_change_password` claim (`JwtTokenService`), `MustChangePasswordMiddleware` refuses **every** endpoint except `auth/change-password` and `auth/logout` with 403 (placed before `UseAuthorization`, so it applies uniformly regardless of an endpoint's `AuthPolicies` tier, and before the idempotency middleware, so a blocked write burns no key), and the SPA's `ProtectedRoute` renders a **"Set your password"** screen (`SetPasswordPage`, reusing `/auth/change-password` — the operator already knows the printed password as their "current" one) instead of the app shell, on every route. Any successful password reset (self-service change, an Owner's `SetUserPasswordAsync`, or break-glass) clears the flag — a single invariant in `IdentityProvider`, not re-derived per path. **Deliberately a separate credential type** from `recover-admin`'s temp password and from #308's browser step-up re-confirmation grant — different audience (pre-auth shell access vs. an authenticated Owner), different lifetime, never conflated; do not treat any of #283/#265/#308 as covering another. diff --git a/README.md b/README.md index a01617f55..bca3ca303 100644 --- a/README.md +++ b/README.md @@ -109,6 +109,38 @@ carries the pre-squash migration history: the 34 migrations were replaced by a single `InitialCreate`, which EF then sees as pending and tries to apply over tables that already exist. Such a database cannot migrate forward — recreate it. +### Changing the database schema + +**Add a migration. Never edit `InitialCreate`.** + +```bash +# Against the local dev Postgres. The connection is fail-closed (#318): there is +# no default, and every target is held to the same TLS floor as a Production +# boot — the loopback opt-out below is what permits plaintext, and only for +# localhost/127.0.0.1/::1. +CLUCKWORK_MIGRATIONS_CONNECTION='Host=localhost;Port=5432;Database=cluckwork;Username=…;Password=…' \ +CLUCKWORK_MIGRATIONS_ALLOW_INSECURE_LOOPBACK=true \ + dotnet ef migrations add \ + -p src/Cluckwork.Infrastructure -s src/Cluckwork.Api +``` + +Before PR #407 the repo held exactly one migration and schema changes were +hand-folded into it. That was only safe while the app had never been deployed — +no database had applied the file yet, so rewriting it was free. **#407 was the +cutover.** Now that a database exists which has already applied `InitialCreate`, +EF will never re-run it, so a hand-folded column is a column that silently does +not exist. It surfaces as broken behaviour (a failing login, a missing field), +not as a migration error — which is precisely what makes it worth a rule. + +`InitialCreate` is also **not regenerable**: it carries four `lower("Name")` +expression indexes EF cannot model and 21 rows of guarded reference-data SQL, +and re-adding it would mint a new timestamp that desynchronises +`__EFMigrationsHistory` everywhere. `MigrationSecurityReviewTests` fails if it +stops being the first migration or loses its recorded id. + +A dev database created **before #407 merged** predates those folded-in columns — +wipe and recreate it as above. + ### Backup & restore (self-hosted) Two complementary layers (spec §17.5): diff --git a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs index c6af20487..8e41a52d4 100644 --- a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs @@ -70,23 +70,60 @@ private static bool LooksHashShaped(string value) => // shape a migrationBuilder.Sql(...) credential would actually take. private static readonly Regex SqlStringLiteral = new(@"'([^']*)'", RegexOptions.Compiled); - // #245's whole contract is "exactly one migration, InitialCreate" — the - // application has never been deployed, so a virgin database is the only - // starting state that exists anywhere, and there is therefore never a - // reason to add a second migration file instead of hand-folding a change - // into InitialCreate (see that migration's own header comment, most - // recently its #364 addendum). This is the automated fence: a PR that - // adds e.g. "AddFoo.cs" beside InitialCreate.cs now fails here instead of - // only getting flagged in review — which is exactly what happened to the - // migration this test itself replaces (PR #399 shipped a second - // migration; this guard did not yet exist to catch it). + // #407 CUTOVER — this fence used to assert `Assert.Single(AllMigrations())`. + // + // #245's contract was "exactly one migration, InitialCreate", valid only + // while the application had never been deployed: a virgin database was the + // only starting state anywhere, so hand-folding a schema change into + // InitialCreate rewrote a file no deployed database had applied yet. PR + // #407 is the LAST change permitted to do that. From the commit that + // merges it, a real database exists that has already applied + // InitialCreate, and EF will never re-run it — so a hand-folded column is + // a column that silently does not exist in production. (That failure mode + // is not hypothetical: it is exactly what #399's amendment did to the dev + // database, surfacing as a broken login rather than as a migration error.) + // + // What replaces it is NOT "no fence". Two things must still hold, and both + // are cheap to break by accident: + // + // 1. InitialCreate stays FIRST and keeps its recorded id. Regenerating + // it (a fresh `dotnet ef migrations add InitialCreate`) mints a NEW + // timestamp, which reorders it behind migrations that depend on it + // and makes __EFMigrationsHistory disagree with the assembly on every + // deployed database. Pinning the id is what makes that a red test + // rather than a failed deploy. + // 2. Ordering is asserted on the MIGRATION ID, never on list position. + // AllMigrations() is built from Assembly.GetTypes(), whose order is + // explicitly unspecified — a positional check would pass or fail on + // reflection ordering, which is the definition of a flaky guard. + // + // Everything else in this file deliberately scans EVERY migration + // (AllInsertDataOperations/AllSqlOperations already fan out across the + // whole history), so the credential and reference-data guarantees below + // extend to post-cutover migrations for free — they were written that way + // from the start, and that is what makes relaxing this one test safe. + private const string InitialCreateMigrationId = "20260801190854_InitialCreate"; + [Fact] - public void ExactlyOneMigrationExists_AndItIsInitialCreate() + public void InitialCreate_IsStillTheFirstMigration_AndKeepsItsRecordedId() { - var migration = Assert.Single(AllMigrations()); - Assert.Equal("InitialCreate", migration.GetType().Name); + var ordered = AllMigrations() + .Select(m => (Migration: m, Id: MigrationIdOf(m))) + .OrderBy(x => x.Id, StringComparer.Ordinal) + .ToList(); + + Assert.NotEmpty(ordered); + Assert.Equal("InitialCreate", ordered[0].Migration.GetType().Name); + Assert.Equal(InitialCreateMigrationId, ordered[0].Id); } + private static string MigrationIdOf(Migration migration) => + migration.GetType() + .GetCustomAttributes(typeof(MigrationAttribute), inherit: false) + .Cast() + .Single() + .Id; + [Fact] public void NoMigration_EverInsertsIntoTheUsersTable() { From 1c81df1e6f41fe73f384cb01e283bdab68110b72 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 3 Aug 2026 01:23:27 -0700 Subject: [PATCH 13/17] test(migrations): freeze InitialCreate's operations, not just its identity (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of #407, P1 — correct, and it found the hole in my own fence. The cutover commit pinned InitialCreate's class name and migration id. That proves the migration is still FIRST, but says nothing about what it CONTAINS — and the regression the cutover exists to prevent is precisely an edit to its contents. A later PR folding a column into InitialCreate.cs changes neither the class name nor the MigrationAttribute, and the other tests in this file only inspect credential-shaped inserts, the reference-data SQL, and the four expression indexes. An ordinary AddColumn/CreateTable edit was invisible to all of them, so the guard passed on exactly the change it was written to stop. Fixed by fingerprinting the operations: a pinned count plus a digest over a structural description of every UpOperation. Any added, removed, or retyped column/table/index/raw-SQL statement fails the test with a message naming the fix (add a migration), rather than inviting a re-baseline. Two deliberate choices, both of which cost something if taken the other way: * The description is STRUCTURAL, not a serialization of EF's operation objects. A serialization would break on an EF version bump that touched an internal property nobody edited — training people to re-baseline the constant on sight, which is how a fence like this dies quietly. Describe() reads only the properties that define what the migration does. * The count is pinned ALONGSIDE the digest rather than instead of it. Describe() collapses unusual operation types to their type name, so two different DropColumns would describe identically; the count catches an added or removed operation even when its description is coarse. That limit is stated in the test rather than papered over. Mutation-checked with the real regression rather than a proxy: folding `MutantFoldedColumn` into an existing CreateTable in InitialCreate fails the test; restored and rebuilt back to green. Worth recording what that run showed — the operation COUNT stayed at 114 and only the digest moved, because the column went inside an existing CreateTable. A count-only guard would have passed. That is the finding's point, demonstrated rather than asserted. AGENTS.md's migration bullet updated to describe both halves of the fence and why identity alone was insufficient. Verified: 1254/1254 dotnet (114 application + 303 domain + 837 integration), 0 build warnings. --- AGENTS.md | 2 +- .../MigrationSecurityReviewTests.cs | 85 +++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 37735146e..479f30c75 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,7 +45,7 @@ dotnet test Cluckwork.sln # 688 tests as of 2026-07; integratio - **Migrations + base provisioning (#283):** EF migrations auto-apply on startup (`Database:MigrateOnStartup`, default true — but **Production sets it `false`**; the `migrate` command runs DDL out-of-band, see the #263 bullet below). The default account, the four assignable roles (`Roles.Assignable`), the default egg grades, and the default packed-unit conversions are **static reference data baked into the migrations themselves** — deterministic and multi-instance-safe by construction, **no runtime seeder, no `Seed:*` config**. They ship as **hand-written `migrationBuilder.Sql` with `WHERE NOT EXISTS` guards, deliberately NOT EF's `HasData`/`InsertData`** — `HasData` keys on the PRIMARY KEY, which assumes a virgin schema, but every install that ever booted pre-#283 already has these rows under *different* ids (the old `DatabaseSeeder` minted roles/grades/conversions with `Guid.NewGuid()`), so `InsertData` either collides on `PK_Accounts` or silently violates the real natural-key unique indexes. The #245 squash retires *that* particular reason (a virgin database is now the only starting state), but **do not "regenerate" these into `HasData`**: `HasData` rows become part of the **model**, and every one of these tables is user-mutable through the app (Settings renames the account, `PUT /api/v1/egg-grades/{id}` renames a grade, `EggUnitConversion.Update` retunes `EggsPerUnit`), so a later model-diff would emit `UpdateData`/`DeleteData` that silently reverts the farm's own edits. Raw SQL seeds once and then leaves the rows alone forever. The guard's scope differs per batch, and the rule is *whether the user can change the natural key or remove a set member*: the **egg grades** batch is gated on the account's grade catalog being **entirely empty** (whole-set), because grades are user-managed — `PUT /api/v1/egg-grades/{id}` renames one, and a per-name guard cannot see a `Small` the farm renamed to `Pullet`, so it would resurrect an active, saleable duplicate (the old seeder skipped the whole set for exactly this reason); **roles, unit conversions, and the default account** stay **per-key**, because their keys are not user-mutable (no role CRUD exists at all; `EggUnitConversion.Update` cannot change `UnitCode` and there is no create/delete; the account's key is a fixed id), and per-key additionally lets a later-added role or unit code back-fill into an older install. `MigrationSecurityReviewTests` pins the SQL shape (12 INSERT statements, 21 rows, every one `WHERE NOT EXISTS`-guarded, the grades guard blind to `"Name"`) and `BaseReferenceDataMigrationTests` pins the result against a real virgin Postgres. `MigrationSecurityReviewTests` also asserts no migration ever inserts a credential-shaped row (`AspNetUsers`, `PasswordHash`, `SecurityStamp`, …) — the schema itself is never a place a credential can hide. The first admin is a **separate, deliberately NOT migration-baked** concern — see the #283 bullet below. - **Migrations: `InitialCreate`, then one per change — the #407 cutover.** `Persistence/Migrations/` began as exactly **one** migration, `InitialCreate`, generated from the current model on 2026-08-01. It replaced the 34 that had accumulated since 2026-06-27 (12 of them carrying raw SQL written for mid-history data states — backfills that touch 0 rows on an empty database but still execute). Done once, before the first production deploy, precisely because no `__EFMigrationsHistory` anywhere needed baselining. - **That squash is over, and so is the rule that came with it.** While the app had never been deployed, a schema change was *hand-folded into `InitialCreate`* — legitimate only because no database had applied that file yet. **PR #407 is the last change permitted to do that.** From the commit that merges it, **every schema change gets its own migration** (`dotnet ef migrations add `); `InitialCreate` is frozen. EF never re-runs an already-applied migration, so a hand-folded column is a column that **silently does not exist** on any database that already booted — and it fails as broken behaviour, not as a migration error. That is not hypothetical: it is exactly what #399's amendment did to the dev database, surfacing as a broken login. `MigrationSecurityReviewTests` enforces the new shape — `InitialCreate` must stay **first** and keep its recorded id `20260801190854_InitialCreate` — instead of the old `Assert.Single` fence. + **That squash is over, and so is the rule that came with it.** While the app had never been deployed, a schema change was *hand-folded into `InitialCreate`* — legitimate only because no database had applied that file yet. **PR #407 is the last change permitted to do that.** From the commit that merges it, **every schema change gets its own migration** (`dotnet ef migrations add `); `InitialCreate` is frozen. EF never re-runs an already-applied migration, so a hand-folded column is a column that **silently does not exist** on any database that already booted — and it fails as broken behaviour, not as a migration error. That is not hypothetical: it is exactly what #399's amendment did to the dev database, surfacing as a broken login. `MigrationSecurityReviewTests` enforces the new shape instead of the old `Assert.Single` fence, in **two** parts, and the second is the load-bearing one: (1) `InitialCreate` must stay **first** and keep its recorded id `20260801190854_InitialCreate`; (2) its **operations are frozen** by a pinned count + digest over a structural description of every `UpOperation`. Identity alone is not enough and that gap is the whole regression — editing `InitialCreate.cs` to add a column changes neither the class name nor the `MigrationAttribute`, and the other tests in that file only inspect credential-shaped inserts, the reference-data SQL, and the four expression indexes, so an ordinary `AddColumn`/`CreateTable` edit was invisible to all of them (#407 codex review). Note the count alone would not have caught it either: a column folded into an existing `CreateTable` leaves the operation count unchanged and moves only the digest. The digest is deliberately a **structural** description, not a serialization of EF's operation objects — the latter would break on an EF version bump nobody asked for, training people to re-baseline the constant on sight, which is how a fence like this dies. **Never regenerate `InitialCreate`.** Two things in it are **hand-carried and un-regenerable** — a plain `dotnet ef migrations add` drops both on the floor: (1) the four **`lower("Name")` expression unique indexes** (`IX_EggGrades_AccountId_FarmId_LowerName`, `IX_ExpenseCategories_NameCi`, `UX_InventoryItems_Account_Farm_LowerName`, `IX_Products_AccountId_LowerName`) — EF cannot model a functional index, and losing them loses four uniqueness constraints; (2) the **static base reference data** raw SQL (see the bullet above). Regenerating also mints a **new timestamp**, which reorders it behind the migrations that depend on it and desynchronises `__EFMigrationsHistory` on every deployed database. `MigrationSecurityReviewTests` and `BaseReferenceDataMigrationTests` fail if any of that goes missing. diff --git a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs index 8e41a52d4..bba5afa56 100644 --- a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs @@ -124,6 +124,91 @@ private static string MigrationIdOf(Migration migration) => .Single() .Id; + // #407 codex review (P1) — pinning InitialCreate's NAME and ID is not the + // same as freezing it, and the gap is exactly the regression the cutover + // exists to prevent. A later PR that edits InitialCreate.cs to add a column + // (instead of adding a second migration) changes neither the class name nor + // the MigrationAttribute, so the identity test above stays green; the other + // tests in this file only look at credential-shaped inserts, the + // reference-data SQL, and the four expression indexes. An ordinary + // AddColumn/CreateTable edit was invisible to all of them. A database that + // already recorded this ID skips the edit, and the app then runs against a + // schema element that does not exist. + // + // So the operations themselves are fingerprinted. Any added, removed, or + // retyped column/table/index/raw-SQL statement changes the digest below and + // fails here, naming the fix. + // + // Deliberately a STRUCTURAL description, not a serialization of EF's + // operation objects: the latter would break on an EF version bump that + // touched an internal property nobody edited, training people to re-baseline + // the constant on sight — which is how a fence like this dies. Describe() + // reads only the properties that define what the migration DOES. + // + // Its `_` fallback collapses unusual operation types to their type name, so + // two different DropColumns would describe identically. That is covered by + // pinning the COUNT alongside the digest: an added or removed operation + // moves the count even when its description is coarse. Stated plainly + // because it is a real limit, not a claim of totality. + // + // Legitimately changing InitialCreate should now be impossible. If this + // fails, the fix is almost always "add a migration instead" — re-baselining + // the constant is the wrong move unless the change genuinely predates any + // deployed database. + private const int InitialCreateOperationCount = 114; + private const string InitialCreateOperationDigest = + "511b60a66c85566967c54556a58fead9b2798728a1d4f42164db9607d83c11c1"; + + [Fact] + public void InitialCreate_OperationsAreFrozen() + { + var initialCreate = AllMigrations() + .Single(m => m.GetType().Name == "InitialCreate"); + + var descriptions = initialCreate.UpOperations.Select(Describe).ToList(); + var digest = Sha256Hex(string.Join("\n", descriptions)); + + Assert.True( + InitialCreateOperationCount == descriptions.Count + && string.Equals(InitialCreateOperationDigest, digest, StringComparison.Ordinal), + $""" + InitialCreate's operations changed (count {descriptions.Count}, digest {digest}). + + Since PR #407 this migration is FROZEN: a database that already applied + it will never re-run it, so a column folded in here silently does not + exist in production and surfaces as broken behaviour, not as an error. + + Add a new migration instead: + dotnet ef migrations add -p src/Cluckwork.Infrastructure -s src/Cluckwork.Api + + Only re-baseline the two constants above if this change genuinely + predates every database that has applied {InitialCreateMigrationId}. + """); + } + + // Only the properties that define what an operation DOES — see the note on + // InitialCreateOperationDigest for why this is not a serialization. + private static string Describe(MigrationOperation operation) => operation switch + { + CreateTableOperation t => + $"CreateTable({t.Name}:{string.Join('|', t.Columns.Select(c => $"{c.Name}:{c.ColumnType}:{c.IsNullable}"))})", + AddColumnOperation c => $"AddColumn({c.Table}.{c.Name}:{c.ColumnType}:{c.IsNullable})", + DropColumnOperation c => $"DropColumn({c.Table}.{c.Name})", + CreateIndexOperation i => + $"CreateIndex({i.Table}.{i.Name}:{string.Join('|', i.Columns)}:unique={i.IsUnique}:filter={i.Filter})", + AddForeignKeyOperation f => $"AddForeignKey({f.Table}.{f.Name}->{f.PrincipalTable})", + AddPrimaryKeyOperation p => $"AddPrimaryKey({p.Table}.{p.Name})", + AddUniqueConstraintOperation u => $"AddUniqueConstraint({u.Table}.{u.Name})", + AlterColumnOperation a => $"AlterColumn({a.Table}.{a.Name}:{a.ColumnType}:{a.IsNullable})", + SqlOperation s => $"Sql({Sha256Hex(s.Sql)})", + _ => operation.GetType().Name, + }; + + private static string Sha256Hex(string value) => + Convert.ToHexString( + System.Security.Cryptography.SHA256.HashData(System.Text.Encoding.UTF8.GetBytes(value))) + .ToLowerInvariant(); + [Fact] public void NoMigration_EverInsertsIntoTheUsersTable() { From 4889567bbc4334ab5a641d319ad38a4cf7a45ecd Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 3 Aug 2026 01:31:10 -0700 Subject: [PATCH 14/17] test(migrations): fingerprint every operation property, not a hand-picked list (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of #407 round 2, P1 — right again, and the third miss of the same kind, which is the actual lesson. The previous digest described operations with a hand-written switch: for a column, name + type + nullability. It omitted the column's DEFAULT, and a CreateTable's nested primary key, foreign keys, unique constraints and delete behaviours. So the schema could change materially with the operation count and every emitted description identical, leaving the freeze test green. Codex's example is the one that stings: changing AspNetUsers.CredentialEpoch from `defaultValue: 1` to `0` — a value #364 declares PERMANENTLY RETIRED, the whole point of that design — was invisible. Enumerating harder is the same bet that had already lost twice, so this changes the approach instead. Describe() now walks every public readable property of an operation, recursing into nested column definitions, constraints, foreign keys, annotations and sequences. A schema-defining property is included because it EXISTS, not because someone remembered it; adding one to an operation type cannot silently escape the digest. Details that are load-bearing rather than incidental: * Properties are ordered by NAME, never reflection order. Type.GetProperties() is explicitly unspecified — the same hazard the ordering test above already avoids with GetTypes(). A digest built on reflection order would churn between runs, and a flaky fence is worse than none. * Collection ORDER is preserved, not sorted. For a composite key or an index's columns the order is itself schema, so sorting would hide a reordering. * Annotations are described, not skipped — Npgsql's ValueGenerationStrategy rides there, so skipping them would drop identity/serial changes. * A property that throws is recorded as having thrown rather than falling back to null, which would be a silent hole. * Determinism verified empirically, not assumed: three consecutive runs produce byte-identical digests. The EF-version-churn concern that motivated the original hand-written approach still applies and is accepted deliberately. This reads EF's PUBLIC operation model — the documented shape of a migration — not internals, and a genuine change there is worth a human look. The failure message says plainly that re-baselining is almost never the right response. Mutation-checked against BOTH cases codex named, not a proxy for them: A. CredentialEpoch defaultValue 1 -> 0 -> RED (was green before) B. a foreign key's onDelete Cascade -> Restrict -> RED (was green before) Both restored and rebuilt back to green, with the migration file confirmed byte-identical to HEAD afterwards. Verified: 1254/1254 dotnet (114 application + 303 domain + 837 integration), 0 build warnings. --- AGENTS.md | 2 +- .../MigrationSecurityReviewTests.cs | 106 +++++++++++++++--- 2 files changed, 90 insertions(+), 18 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 479f30c75..2e39eb79c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,7 +45,7 @@ dotnet test Cluckwork.sln # 688 tests as of 2026-07; integratio - **Migrations + base provisioning (#283):** EF migrations auto-apply on startup (`Database:MigrateOnStartup`, default true — but **Production sets it `false`**; the `migrate` command runs DDL out-of-band, see the #263 bullet below). The default account, the four assignable roles (`Roles.Assignable`), the default egg grades, and the default packed-unit conversions are **static reference data baked into the migrations themselves** — deterministic and multi-instance-safe by construction, **no runtime seeder, no `Seed:*` config**. They ship as **hand-written `migrationBuilder.Sql` with `WHERE NOT EXISTS` guards, deliberately NOT EF's `HasData`/`InsertData`** — `HasData` keys on the PRIMARY KEY, which assumes a virgin schema, but every install that ever booted pre-#283 already has these rows under *different* ids (the old `DatabaseSeeder` minted roles/grades/conversions with `Guid.NewGuid()`), so `InsertData` either collides on `PK_Accounts` or silently violates the real natural-key unique indexes. The #245 squash retires *that* particular reason (a virgin database is now the only starting state), but **do not "regenerate" these into `HasData`**: `HasData` rows become part of the **model**, and every one of these tables is user-mutable through the app (Settings renames the account, `PUT /api/v1/egg-grades/{id}` renames a grade, `EggUnitConversion.Update` retunes `EggsPerUnit`), so a later model-diff would emit `UpdateData`/`DeleteData` that silently reverts the farm's own edits. Raw SQL seeds once and then leaves the rows alone forever. The guard's scope differs per batch, and the rule is *whether the user can change the natural key or remove a set member*: the **egg grades** batch is gated on the account's grade catalog being **entirely empty** (whole-set), because grades are user-managed — `PUT /api/v1/egg-grades/{id}` renames one, and a per-name guard cannot see a `Small` the farm renamed to `Pullet`, so it would resurrect an active, saleable duplicate (the old seeder skipped the whole set for exactly this reason); **roles, unit conversions, and the default account** stay **per-key**, because their keys are not user-mutable (no role CRUD exists at all; `EggUnitConversion.Update` cannot change `UnitCode` and there is no create/delete; the account's key is a fixed id), and per-key additionally lets a later-added role or unit code back-fill into an older install. `MigrationSecurityReviewTests` pins the SQL shape (12 INSERT statements, 21 rows, every one `WHERE NOT EXISTS`-guarded, the grades guard blind to `"Name"`) and `BaseReferenceDataMigrationTests` pins the result against a real virgin Postgres. `MigrationSecurityReviewTests` also asserts no migration ever inserts a credential-shaped row (`AspNetUsers`, `PasswordHash`, `SecurityStamp`, …) — the schema itself is never a place a credential can hide. The first admin is a **separate, deliberately NOT migration-baked** concern — see the #283 bullet below. - **Migrations: `InitialCreate`, then one per change — the #407 cutover.** `Persistence/Migrations/` began as exactly **one** migration, `InitialCreate`, generated from the current model on 2026-08-01. It replaced the 34 that had accumulated since 2026-06-27 (12 of them carrying raw SQL written for mid-history data states — backfills that touch 0 rows on an empty database but still execute). Done once, before the first production deploy, precisely because no `__EFMigrationsHistory` anywhere needed baselining. - **That squash is over, and so is the rule that came with it.** While the app had never been deployed, a schema change was *hand-folded into `InitialCreate`* — legitimate only because no database had applied that file yet. **PR #407 is the last change permitted to do that.** From the commit that merges it, **every schema change gets its own migration** (`dotnet ef migrations add `); `InitialCreate` is frozen. EF never re-runs an already-applied migration, so a hand-folded column is a column that **silently does not exist** on any database that already booted — and it fails as broken behaviour, not as a migration error. That is not hypothetical: it is exactly what #399's amendment did to the dev database, surfacing as a broken login. `MigrationSecurityReviewTests` enforces the new shape instead of the old `Assert.Single` fence, in **two** parts, and the second is the load-bearing one: (1) `InitialCreate` must stay **first** and keep its recorded id `20260801190854_InitialCreate`; (2) its **operations are frozen** by a pinned count + digest over a structural description of every `UpOperation`. Identity alone is not enough and that gap is the whole regression — editing `InitialCreate.cs` to add a column changes neither the class name nor the `MigrationAttribute`, and the other tests in that file only inspect credential-shaped inserts, the reference-data SQL, and the four expression indexes, so an ordinary `AddColumn`/`CreateTable` edit was invisible to all of them (#407 codex review). Note the count alone would not have caught it either: a column folded into an existing `CreateTable` leaves the operation count unchanged and moves only the digest. The digest is deliberately a **structural** description, not a serialization of EF's operation objects — the latter would break on an EF version bump nobody asked for, training people to re-baseline the constant on sight, which is how a fence like this dies. + **That squash is over, and so is the rule that came with it.** While the app had never been deployed, a schema change was *hand-folded into `InitialCreate`* — legitimate only because no database had applied that file yet. **PR #407 is the last change permitted to do that.** From the commit that merges it, **every schema change gets its own migration** (`dotnet ef migrations add `); `InitialCreate` is frozen. EF never re-runs an already-applied migration, so a hand-folded column is a column that **silently does not exist** on any database that already booted — and it fails as broken behaviour, not as a migration error. That is not hypothetical: it is exactly what #399's amendment did to the dev database, surfacing as a broken login. `MigrationSecurityReviewTests` enforces the new shape instead of the old `Assert.Single` fence, in **two** parts, and the second is the load-bearing one: (1) `InitialCreate` must stay **first** and keep its recorded id `20260801190854_InitialCreate`; (2) its **operations are frozen** by a pinned count + digest over a structural description of every `UpOperation`. Identity alone is not enough and that gap is the whole regression — editing `InitialCreate.cs` to add a column changes neither the class name nor the `MigrationAttribute`, and the other tests in that file only inspect credential-shaped inserts, the reference-data SQL, and the four expression indexes, so an ordinary `AddColumn`/`CreateTable` edit was invisible to all of them (#407 codex review). Note the count alone would not have caught it either: a column folded into an existing `CreateTable` leaves the operation count unchanged and moves only the digest. The digest walks **every public readable property** of each operation, recursing into nested column definitions, constraints, foreign keys and annotations, ordered by property **name** (never reflection order — `GetProperties()` is unspecified, same hazard as `GetTypes()`). It is emphatically **not** a hand-written list of "the properties that matter": that was the first attempt and it missed a column's `defaultValue` and a `CreateTable`'s nested keys, so changing `AspNetUsers.CredentialEpoch` from `defaultValue: 1` to `0` — a value #364 says is **permanently retired** — left the count and the whole digest unchanged (#407 codex review round 2). A property is now included because it *exists*, not because someone remembered it. Sequence order inside a collection is preserved rather than sorted, since a composite key's column order is itself schema. **Never regenerate `InitialCreate`.** Two things in it are **hand-carried and un-regenerable** — a plain `dotnet ef migrations add` drops both on the floor: (1) the four **`lower("Name")` expression unique indexes** (`IX_EggGrades_AccountId_FarmId_LowerName`, `IX_ExpenseCategories_NameCi`, `UX_InventoryItems_Account_Farm_LowerName`, `IX_Products_AccountId_LowerName`) — EF cannot model a functional index, and losing them loses four uniqueness constraints; (2) the **static base reference data** raw SQL (see the bullet above). Regenerating also mints a **new timestamp**, which reorders it behind the migrations that depend on it and desynchronises `__EFMigrationsHistory` on every deployed database. `MigrationSecurityReviewTests` and `BaseReferenceDataMigrationTests` fail if any of that goes missing. diff --git a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs index bba5afa56..8461a898a 100644 --- a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs @@ -1,7 +1,11 @@ namespace Cluckwork.Api.IntegrationTests; +using System.Collections; +using System.Globalization; +using System.Reflection; using System.Text.RegularExpressions; using Cluckwork.Infrastructure.Persistence; +using Microsoft.EntityFrameworkCore.Infrastructure; using Microsoft.EntityFrameworkCore.Migrations; using Microsoft.EntityFrameworkCore.Migrations.Operations; @@ -157,7 +161,7 @@ private static string MigrationIdOf(Migration migration) => // deployed database. private const int InitialCreateOperationCount = 114; private const string InitialCreateOperationDigest = - "511b60a66c85566967c54556a58fead9b2798728a1d4f42164db9607d83c11c1"; + "543baa99852c47a2e9ea857c2c0591d58002eea508025012d8667f7dfa7e0f61"; [Fact] public void InitialCreate_OperationsAreFrozen() @@ -186,23 +190,91 @@ predates every database that has applied {InitialCreateMigrationId}. """); } - // Only the properties that define what an operation DOES — see the note on - // InitialCreateOperationDigest for why this is not a serialization. - private static string Describe(MigrationOperation operation) => operation switch + // #407 codex review round 2 (P1) — the hand-written switch this replaces + // listed properties per operation type, and that approach missed something + // on every pass. It captured a column's name/type/nullability but NOT its + // default, nor a CreateTable's nested primary key, foreign keys, unique + // constraints, or a foreign key's OnDelete. So changing + // `AspNetUsers.CredentialEpoch` from `defaultValue: 1` to `0` — a value + // #364 says is PERMANENTLY RETIRED — left the count and every emitted + // description identical, and the freeze test green. + // + // Enumerating harder is the same bet that already lost twice. This walks + // every public readable property instead, recursing into nested operations, + // column definitions, annotations and sequences, so a schema-defining + // property is included because it EXISTS, not because someone remembered + // it. Adding a property to an operation type cannot silently escape the + // digest. + // + // Determinism matters as much as completeness: properties are ordered by + // NAME, never by reflection order, for the same reason the ordering test + // above sorts on the migration id — Type.GetProperties() is explicitly + // unspecified, and a digest built on it would churn between runs. + // + // The EF-version-churn concern that motivated the original structural + // choice still applies and is accepted deliberately: this reads EF's + // PUBLIC operation model (the documented shape of a migration), not + // internals, and a genuine change there is worth a human look. The failure + // message says re-baselining is almost never the right response. + private static string Describe(MigrationOperation operation) => + $"{operation.GetType().Name}({DescribeValue(operation, depth: 0)})"; + + private const int MaxDescribeDepth = 6; + + private static string DescribeValue(object? value, int depth) { - CreateTableOperation t => - $"CreateTable({t.Name}:{string.Join('|', t.Columns.Select(c => $"{c.Name}:{c.ColumnType}:{c.IsNullable}"))})", - AddColumnOperation c => $"AddColumn({c.Table}.{c.Name}:{c.ColumnType}:{c.IsNullable})", - DropColumnOperation c => $"DropColumn({c.Table}.{c.Name})", - CreateIndexOperation i => - $"CreateIndex({i.Table}.{i.Name}:{string.Join('|', i.Columns)}:unique={i.IsUnique}:filter={i.Filter})", - AddForeignKeyOperation f => $"AddForeignKey({f.Table}.{f.Name}->{f.PrincipalTable})", - AddPrimaryKeyOperation p => $"AddPrimaryKey({p.Table}.{p.Name})", - AddUniqueConstraintOperation u => $"AddUniqueConstraint({u.Table}.{u.Name})", - AlterColumnOperation a => $"AlterColumn({a.Table}.{a.Name}:{a.ColumnType}:{a.IsNullable})", - SqlOperation s => $"Sql({Sha256Hex(s.Sql)})", - _ => operation.GetType().Name, - }; + switch (value) + { + case null: + return "null"; + case string s: + return $"'{s}'"; + case bool or char or decimal or double or float or int or long or short + or uint or ulong or ushort or byte or sbyte or Guid or DateTime or DateTimeOffset: + return Convert.ToString(value, CultureInfo.InvariantCulture) ?? "?"; + case Enum e: + return $"{e.GetType().Name}.{e}"; + } + + if (depth >= MaxDescribeDepth) return $""; + + // Annotations carry schema meaning (Npgsql's ValueGenerationStrategy, + // for one), so they are described rather than skipped — sorted by name, + // because IAnnotatable does not promise an order. + if (value is IAnnotation annotation) + return $"@{annotation.Name}={DescribeValue(annotation.Value, depth + 1)}"; + + if (value is IEnumerable enumerable) + { + var items = enumerable.Cast().Select(v => DescribeValue(v, depth + 1)); + // NOT sorted: for columns, key columns and operation lists the ORDER + // is itself schema (a composite key's column order changes the + // index). Sorting here would hide a reordering. + return $"[{string.Join(',', items)}]"; + } + + var members = value.GetType() + .GetProperties(BindingFlags.Public | BindingFlags.Instance) + .Where(p => p.CanRead && p.GetIndexParameters().Length == 0) + .OrderBy(p => p.Name, StringComparer.Ordinal) + .Select(p => $"{p.Name}={DescribeValue(SafeGet(p, value), depth + 1)}"); + + return string.Join(';', members); + } + + private static object? SafeGet(PropertyInfo property, object target) + { + try + { + return property.GetValue(target); + } + catch (Exception ex) + { + // A property that throws must not be silently equivalent to null — + // that would be a hole in the digest. Record the failure itself. + return $""; + } + } private static string Sha256Hex(string value) => Convert.ToHexString( From 6b3ee097355a57b66ef9a5aff7a5ecb0ac99d7eb Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 3 Aug 2026 01:40:25 -0700 Subject: [PATCH 15/17] test(migrations): reach annotations via GetAnnotations(), which a property walk cannot (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of #407 round 3, P1 — correct, and the worst of the four, because the previous commit did not merely miss something: it shipped a comment claiming coverage the code did not have. EF exposes an operation's annotations through GetAnnotations(), a METHOD. The reflection walk enumerates public non-indexed PROPERTIES, so it never reached them — and the IAnnotation branch written to handle them was unreachable dead code sitting under a comment that said annotations "are described rather than skipped". That sentence was false when written. Proven rather than argued, and the cleanest before/after available: with the previous commit in place, flipping InitialCreate's Npgsql:ValueGenerationStrategy from IdentityByDefaultColumn to IdentityAlwaysColumn — a material identity-generation change — left the freeze test GREEN. The SAME mutation against this commit fails it. Only the fingerprint changed between those two runs; the mutation and the migration were identical. Annotations are now enumerated explicitly from IReadOnlyAnnotatable and emitted FIRST, so every annotatable in the graph contributes — the operation itself and each AddColumnOperation nested inside a CreateTable. Sorted by name, because IReadOnlyAnnotatable promises no order and an unsorted digest would churn between runs; determinism re-verified across three consecutive runs. The pattern across these four rounds is the finding worth keeping, and AGENTS.md now records it: each version of this fence shipped a comment asserting coverage it did not have, and each was caught by review rather than by the test suite. The standard for changing this fingerprint is a mutation that goes RED, never an argument that it should. Verified: 1254/1254 dotnet (114 application + 303 domain + 837 integration), 0 build warnings. Migration file confirmed byte-identical to HEAD after the mutation runs. --- AGENTS.md | 2 +- .../MigrationSecurityReviewTests.cs | 28 +++++++++++++++---- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2e39eb79c..137af95f4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,7 +45,7 @@ dotnet test Cluckwork.sln # 688 tests as of 2026-07; integratio - **Migrations + base provisioning (#283):** EF migrations auto-apply on startup (`Database:MigrateOnStartup`, default true — but **Production sets it `false`**; the `migrate` command runs DDL out-of-band, see the #263 bullet below). The default account, the four assignable roles (`Roles.Assignable`), the default egg grades, and the default packed-unit conversions are **static reference data baked into the migrations themselves** — deterministic and multi-instance-safe by construction, **no runtime seeder, no `Seed:*` config**. They ship as **hand-written `migrationBuilder.Sql` with `WHERE NOT EXISTS` guards, deliberately NOT EF's `HasData`/`InsertData`** — `HasData` keys on the PRIMARY KEY, which assumes a virgin schema, but every install that ever booted pre-#283 already has these rows under *different* ids (the old `DatabaseSeeder` minted roles/grades/conversions with `Guid.NewGuid()`), so `InsertData` either collides on `PK_Accounts` or silently violates the real natural-key unique indexes. The #245 squash retires *that* particular reason (a virgin database is now the only starting state), but **do not "regenerate" these into `HasData`**: `HasData` rows become part of the **model**, and every one of these tables is user-mutable through the app (Settings renames the account, `PUT /api/v1/egg-grades/{id}` renames a grade, `EggUnitConversion.Update` retunes `EggsPerUnit`), so a later model-diff would emit `UpdateData`/`DeleteData` that silently reverts the farm's own edits. Raw SQL seeds once and then leaves the rows alone forever. The guard's scope differs per batch, and the rule is *whether the user can change the natural key or remove a set member*: the **egg grades** batch is gated on the account's grade catalog being **entirely empty** (whole-set), because grades are user-managed — `PUT /api/v1/egg-grades/{id}` renames one, and a per-name guard cannot see a `Small` the farm renamed to `Pullet`, so it would resurrect an active, saleable duplicate (the old seeder skipped the whole set for exactly this reason); **roles, unit conversions, and the default account** stay **per-key**, because their keys are not user-mutable (no role CRUD exists at all; `EggUnitConversion.Update` cannot change `UnitCode` and there is no create/delete; the account's key is a fixed id), and per-key additionally lets a later-added role or unit code back-fill into an older install. `MigrationSecurityReviewTests` pins the SQL shape (12 INSERT statements, 21 rows, every one `WHERE NOT EXISTS`-guarded, the grades guard blind to `"Name"`) and `BaseReferenceDataMigrationTests` pins the result against a real virgin Postgres. `MigrationSecurityReviewTests` also asserts no migration ever inserts a credential-shaped row (`AspNetUsers`, `PasswordHash`, `SecurityStamp`, …) — the schema itself is never a place a credential can hide. The first admin is a **separate, deliberately NOT migration-baked** concern — see the #283 bullet below. - **Migrations: `InitialCreate`, then one per change — the #407 cutover.** `Persistence/Migrations/` began as exactly **one** migration, `InitialCreate`, generated from the current model on 2026-08-01. It replaced the 34 that had accumulated since 2026-06-27 (12 of them carrying raw SQL written for mid-history data states — backfills that touch 0 rows on an empty database but still execute). Done once, before the first production deploy, precisely because no `__EFMigrationsHistory` anywhere needed baselining. - **That squash is over, and so is the rule that came with it.** While the app had never been deployed, a schema change was *hand-folded into `InitialCreate`* — legitimate only because no database had applied that file yet. **PR #407 is the last change permitted to do that.** From the commit that merges it, **every schema change gets its own migration** (`dotnet ef migrations add `); `InitialCreate` is frozen. EF never re-runs an already-applied migration, so a hand-folded column is a column that **silently does not exist** on any database that already booted — and it fails as broken behaviour, not as a migration error. That is not hypothetical: it is exactly what #399's amendment did to the dev database, surfacing as a broken login. `MigrationSecurityReviewTests` enforces the new shape instead of the old `Assert.Single` fence, in **two** parts, and the second is the load-bearing one: (1) `InitialCreate` must stay **first** and keep its recorded id `20260801190854_InitialCreate`; (2) its **operations are frozen** by a pinned count + digest over a structural description of every `UpOperation`. Identity alone is not enough and that gap is the whole regression — editing `InitialCreate.cs` to add a column changes neither the class name nor the `MigrationAttribute`, and the other tests in that file only inspect credential-shaped inserts, the reference-data SQL, and the four expression indexes, so an ordinary `AddColumn`/`CreateTable` edit was invisible to all of them (#407 codex review). Note the count alone would not have caught it either: a column folded into an existing `CreateTable` leaves the operation count unchanged and moves only the digest. The digest walks **every public readable property** of each operation, recursing into nested column definitions, constraints, foreign keys and annotations, ordered by property **name** (never reflection order — `GetProperties()` is unspecified, same hazard as `GetTypes()`). It is emphatically **not** a hand-written list of "the properties that matter": that was the first attempt and it missed a column's `defaultValue` and a `CreateTable`'s nested keys, so changing `AspNetUsers.CredentialEpoch` from `defaultValue: 1` to `0` — a value #364 says is **permanently retired** — left the count and the whole digest unchanged (#407 codex review round 2). A property is now included because it *exists*, not because someone remembered it. Sequence order inside a collection is preserved rather than sorted, since a composite key's column order is itself schema. + **That squash is over, and so is the rule that came with it.** While the app had never been deployed, a schema change was *hand-folded into `InitialCreate`* — legitimate only because no database had applied that file yet. **PR #407 is the last change permitted to do that.** From the commit that merges it, **every schema change gets its own migration** (`dotnet ef migrations add `); `InitialCreate` is frozen. EF never re-runs an already-applied migration, so a hand-folded column is a column that **silently does not exist** on any database that already booted — and it fails as broken behaviour, not as a migration error. That is not hypothetical: it is exactly what #399's amendment did to the dev database, surfacing as a broken login. `MigrationSecurityReviewTests` enforces the new shape instead of the old `Assert.Single` fence, in **two** parts, and the second is the load-bearing one: (1) `InitialCreate` must stay **first** and keep its recorded id `20260801190854_InitialCreate`; (2) its **operations are frozen** by a pinned count + digest over a structural description of every `UpOperation`. Identity alone is not enough and that gap is the whole regression — editing `InitialCreate.cs` to add a column changes neither the class name nor the `MigrationAttribute`, and the other tests in that file only inspect credential-shaped inserts, the reference-data SQL, and the four expression indexes, so an ordinary `AddColumn`/`CreateTable` edit was invisible to all of them (#407 codex review). Note the count alone would not have caught it either: a column folded into an existing `CreateTable` leaves the operation count unchanged and moves only the digest. The digest walks **every public readable property** of each operation, recursing into nested column definitions, constraints, foreign keys and annotations, ordered by property **name** (never reflection order — `GetProperties()` is unspecified, same hazard as `GetTypes()`). It is emphatically **not** a hand-written list of "the properties that matter": that was the first attempt and it missed a column's `defaultValue` and a `CreateTable`'s nested keys, so changing `AspNetUsers.CredentialEpoch` from `defaultValue: 1` to `0` — a value #364 says is **permanently retired** — left the count and the whole digest unchanged (#407 codex review round 2). A property is now included because it *exists*, not because someone remembered it. Sequence order inside a collection is preserved rather than sorted, since a composite key's column order is itself schema. **Annotations are enumerated explicitly via `GetAnnotations()`**, because EF exposes them through a *method*, not a property — a property walk never reaches them, and the version that merely claimed to cover them let `Npgsql:ValueGenerationStrategy` flip from `IdentityByDefaultColumn` to `IdentityAlwaysColumn` with the test still green (#407 codex review round 3). Sorted by name there, since `IReadOnlyAnnotatable` promises no order. Successive rounds of this fence each shipped a comment asserting coverage the code did not have; **the standard for changing it is a mutation that goes red, not an argument that it should**. **Never regenerate `InitialCreate`.** Two things in it are **hand-carried and un-regenerable** — a plain `dotnet ef migrations add` drops both on the floor: (1) the four **`lower("Name")` expression unique indexes** (`IX_EggGrades_AccountId_FarmId_LowerName`, `IX_ExpenseCategories_NameCi`, `UX_InventoryItems_Account_Farm_LowerName`, `IX_Products_AccountId_LowerName`) — EF cannot model a functional index, and losing them loses four uniqueness constraints; (2) the **static base reference data** raw SQL (see the bullet above). Regenerating also mints a **new timestamp**, which reorders it behind the migrations that depend on it and desynchronises `__EFMigrationsHistory` on every deployed database. `MigrationSecurityReviewTests` and `BaseReferenceDataMigrationTests` fail if any of that goes missing. diff --git a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs index 8461a898a..c00504b47 100644 --- a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs @@ -161,7 +161,7 @@ private static string MigrationIdOf(Migration migration) => // deployed database. private const int InitialCreateOperationCount = 114; private const string InitialCreateOperationDigest = - "543baa99852c47a2e9ea857c2c0591d58002eea508025012d8667f7dfa7e0f61"; + "320493300753dc57278ec990ff678e8fd7fed1a82ac438ea8563acda65756efc"; [Fact] public void InitialCreate_OperationsAreFrozen() @@ -238,9 +238,6 @@ private static string DescribeValue(object? value, int depth) if (depth >= MaxDescribeDepth) return $""; - // Annotations carry schema meaning (Npgsql's ValueGenerationStrategy, - // for one), so they are described rather than skipped — sorted by name, - // because IAnnotatable does not promise an order. if (value is IAnnotation annotation) return $"@{annotation.Name}={DescribeValue(annotation.Value, depth + 1)}"; @@ -253,13 +250,34 @@ private static string DescribeValue(object? value, int depth) return $"[{string.Join(',', items)}]"; } + // #407 codex review round 3 (P1) — annotations are reached through + // GetAnnotations(), a METHOD, so a walk over public non-indexed + // PROPERTIES never sees them. The previous version had an IAnnotation + // branch and a comment claiming annotations were covered; the branch was + // unreachable and the comment was simply false. Proven, not argued: + // with that version in place, flipping InitialCreate's + // Npgsql:ValueGenerationStrategy from IdentityByDefaultColumn to + // IdentityAlwaysColumn — a material identity-generation change — left + // the freeze test GREEN. + // + // Enumerated explicitly and FIRST, so every annotatable in the graph + // (the operation itself, and each AddColumnOperation nested in a + // CreateTable) contributes. Sorted by name: IReadOnlyAnnotatable + // promises no order, and an unsorted digest would churn between runs. + var annotations = value is IReadOnlyAnnotatable annotatable + ? annotatable.GetAnnotations() + .OrderBy(a => a.Name, StringComparer.Ordinal) + .Select(a => $"@{a.Name}={DescribeValue(a.Value, depth + 1)}") + .ToList() + : []; + var members = value.GetType() .GetProperties(BindingFlags.Public | BindingFlags.Instance) .Where(p => p.CanRead && p.GetIndexParameters().Length == 0) .OrderBy(p => p.Name, StringComparer.Ordinal) .Select(p => $"{p.Name}={DescribeValue(SafeGet(p, value), depth + 1)}"); - return string.Join(';', members); + return string.Join(';', annotations.Concat(members)); } private static object? SafeGet(PropertyInfo property, object target) From 0367bf7be87695fe8101d97f09b441933624df3d Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 3 Aug 2026 01:51:53 -0700 Subject: [PATCH 16/17] fix(test): stop the migration digest depending on the machine it runs on (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI failure on 6b3ee09, and it was mine — not a flake. CI: e4d0e30a8d59a2c7c75730d1f3203f0256424a662d20df1c90be22b7609f3c39 local: 320493300753dc57278ec990ff678e8fd7fed1a82ac438ea8563acda65756efc count: 114 on both ColumnOperation.ClrType is a System.Type, and the reflection walk recursed into it — out through Assembly.Location / Module.FullyQualifiedName / CodeBase, which are ABSOLUTE PATHS to System.Private.CoreLib.dll. The digest therefore encoded where the .NET runtime happens to live: a /nix/store path here, something else on the runner. Same migration, different answer. It also made the described text ~168 MB, because Type -> Assembly -> Module cycles back into the reflection graph until MaxDescribeDepth cuts it off. The frozen test took 2s; it now takes 17ms. Fixed by treating a System.Type as a LEAF, described by FullName. That is the schema-relevant part — a column's CLR type matters, the file its metadata lives in does not. The process failure is worth more than the bug. I verified determinism by running the test three times ON ONE MACHINE and reported it as verified. Same-run determinism is not portability, and the check I ran could not have detected this defect in principle. CI caught what my verification was structurally incapable of catching. So the missing check is now a test rather than a habit: InitialCreate_Description_ContainsNoEnvironmentSpecificData asserts the described text names no assembly file and contains neither AppContext .BaseDirectory nor the runtime directory, plus a size ceiling as a cheap canary for the same recursion blowing up again. A future property that leaks environment fails there by name instead of failing as a mystery digest mismatch in CI. Re-verified after the fix, not assumed to still hold: * annotation mutant (ValueGenerationStrategy IdentityByDefault -> IdentityAlways) -> RED * default-value mutant (CredentialEpoch defaultValue 1 -> 0) -> RED * migration file byte-identical to HEAD afterwards * 3 consecutive local runs green (necessary, and — as established above — not sufficient on its own; the environment test is what covers the rest) AGENTS.md records both the Type-leaf rule and the lesson that repeated runs on one machine prove nothing about portability. Verified: 1255/1255 dotnet (114 application + 303 domain + 838 integration), 0 build warnings. --- AGENTS.md | 2 +- .../MigrationSecurityReviewTests.cs | 57 ++++++++++++++++++- 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 137af95f4..0f8020fba 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,7 +45,7 @@ dotnet test Cluckwork.sln # 688 tests as of 2026-07; integratio - **Migrations + base provisioning (#283):** EF migrations auto-apply on startup (`Database:MigrateOnStartup`, default true — but **Production sets it `false`**; the `migrate` command runs DDL out-of-band, see the #263 bullet below). The default account, the four assignable roles (`Roles.Assignable`), the default egg grades, and the default packed-unit conversions are **static reference data baked into the migrations themselves** — deterministic and multi-instance-safe by construction, **no runtime seeder, no `Seed:*` config**. They ship as **hand-written `migrationBuilder.Sql` with `WHERE NOT EXISTS` guards, deliberately NOT EF's `HasData`/`InsertData`** — `HasData` keys on the PRIMARY KEY, which assumes a virgin schema, but every install that ever booted pre-#283 already has these rows under *different* ids (the old `DatabaseSeeder` minted roles/grades/conversions with `Guid.NewGuid()`), so `InsertData` either collides on `PK_Accounts` or silently violates the real natural-key unique indexes. The #245 squash retires *that* particular reason (a virgin database is now the only starting state), but **do not "regenerate" these into `HasData`**: `HasData` rows become part of the **model**, and every one of these tables is user-mutable through the app (Settings renames the account, `PUT /api/v1/egg-grades/{id}` renames a grade, `EggUnitConversion.Update` retunes `EggsPerUnit`), so a later model-diff would emit `UpdateData`/`DeleteData` that silently reverts the farm's own edits. Raw SQL seeds once and then leaves the rows alone forever. The guard's scope differs per batch, and the rule is *whether the user can change the natural key or remove a set member*: the **egg grades** batch is gated on the account's grade catalog being **entirely empty** (whole-set), because grades are user-managed — `PUT /api/v1/egg-grades/{id}` renames one, and a per-name guard cannot see a `Small` the farm renamed to `Pullet`, so it would resurrect an active, saleable duplicate (the old seeder skipped the whole set for exactly this reason); **roles, unit conversions, and the default account** stay **per-key**, because their keys are not user-mutable (no role CRUD exists at all; `EggUnitConversion.Update` cannot change `UnitCode` and there is no create/delete; the account's key is a fixed id), and per-key additionally lets a later-added role or unit code back-fill into an older install. `MigrationSecurityReviewTests` pins the SQL shape (12 INSERT statements, 21 rows, every one `WHERE NOT EXISTS`-guarded, the grades guard blind to `"Name"`) and `BaseReferenceDataMigrationTests` pins the result against a real virgin Postgres. `MigrationSecurityReviewTests` also asserts no migration ever inserts a credential-shaped row (`AspNetUsers`, `PasswordHash`, `SecurityStamp`, …) — the schema itself is never a place a credential can hide. The first admin is a **separate, deliberately NOT migration-baked** concern — see the #283 bullet below. - **Migrations: `InitialCreate`, then one per change — the #407 cutover.** `Persistence/Migrations/` began as exactly **one** migration, `InitialCreate`, generated from the current model on 2026-08-01. It replaced the 34 that had accumulated since 2026-06-27 (12 of them carrying raw SQL written for mid-history data states — backfills that touch 0 rows on an empty database but still execute). Done once, before the first production deploy, precisely because no `__EFMigrationsHistory` anywhere needed baselining. - **That squash is over, and so is the rule that came with it.** While the app had never been deployed, a schema change was *hand-folded into `InitialCreate`* — legitimate only because no database had applied that file yet. **PR #407 is the last change permitted to do that.** From the commit that merges it, **every schema change gets its own migration** (`dotnet ef migrations add `); `InitialCreate` is frozen. EF never re-runs an already-applied migration, so a hand-folded column is a column that **silently does not exist** on any database that already booted — and it fails as broken behaviour, not as a migration error. That is not hypothetical: it is exactly what #399's amendment did to the dev database, surfacing as a broken login. `MigrationSecurityReviewTests` enforces the new shape instead of the old `Assert.Single` fence, in **two** parts, and the second is the load-bearing one: (1) `InitialCreate` must stay **first** and keep its recorded id `20260801190854_InitialCreate`; (2) its **operations are frozen** by a pinned count + digest over a structural description of every `UpOperation`. Identity alone is not enough and that gap is the whole regression — editing `InitialCreate.cs` to add a column changes neither the class name nor the `MigrationAttribute`, and the other tests in that file only inspect credential-shaped inserts, the reference-data SQL, and the four expression indexes, so an ordinary `AddColumn`/`CreateTable` edit was invisible to all of them (#407 codex review). Note the count alone would not have caught it either: a column folded into an existing `CreateTable` leaves the operation count unchanged and moves only the digest. The digest walks **every public readable property** of each operation, recursing into nested column definitions, constraints, foreign keys and annotations, ordered by property **name** (never reflection order — `GetProperties()` is unspecified, same hazard as `GetTypes()`). It is emphatically **not** a hand-written list of "the properties that matter": that was the first attempt and it missed a column's `defaultValue` and a `CreateTable`'s nested keys, so changing `AspNetUsers.CredentialEpoch` from `defaultValue: 1` to `0` — a value #364 says is **permanently retired** — left the count and the whole digest unchanged (#407 codex review round 2). A property is now included because it *exists*, not because someone remembered it. Sequence order inside a collection is preserved rather than sorted, since a composite key's column order is itself schema. **Annotations are enumerated explicitly via `GetAnnotations()`**, because EF exposes them through a *method*, not a property — a property walk never reaches them, and the version that merely claimed to cover them let `Npgsql:ValueGenerationStrategy` flip from `IdentityByDefaultColumn` to `IdentityAlwaysColumn` with the test still green (#407 codex review round 3). Sorted by name there, since `IReadOnlyAnnotatable` promises no order. Successive rounds of this fence each shipped a comment asserting coverage the code did not have; **the standard for changing it is a mutation that goes red, not an argument that it should**. + **That squash is over, and so is the rule that came with it.** While the app had never been deployed, a schema change was *hand-folded into `InitialCreate`* — legitimate only because no database had applied that file yet. **PR #407 is the last change permitted to do that.** From the commit that merges it, **every schema change gets its own migration** (`dotnet ef migrations add `); `InitialCreate` is frozen. EF never re-runs an already-applied migration, so a hand-folded column is a column that **silently does not exist** on any database that already booted — and it fails as broken behaviour, not as a migration error. That is not hypothetical: it is exactly what #399's amendment did to the dev database, surfacing as a broken login. `MigrationSecurityReviewTests` enforces the new shape instead of the old `Assert.Single` fence, in **two** parts, and the second is the load-bearing one: (1) `InitialCreate` must stay **first** and keep its recorded id `20260801190854_InitialCreate`; (2) its **operations are frozen** by a pinned count + digest over a structural description of every `UpOperation`. Identity alone is not enough and that gap is the whole regression — editing `InitialCreate.cs` to add a column changes neither the class name nor the `MigrationAttribute`, and the other tests in that file only inspect credential-shaped inserts, the reference-data SQL, and the four expression indexes, so an ordinary `AddColumn`/`CreateTable` edit was invisible to all of them (#407 codex review). Note the count alone would not have caught it either: a column folded into an existing `CreateTable` leaves the operation count unchanged and moves only the digest. The digest walks **every public readable property** of each operation, recursing into nested column definitions, constraints, foreign keys and annotations, ordered by property **name** (never reflection order — `GetProperties()` is unspecified, same hazard as `GetTypes()`). It is emphatically **not** a hand-written list of "the properties that matter": that was the first attempt and it missed a column's `defaultValue` and a `CreateTable`'s nested keys, so changing `AspNetUsers.CredentialEpoch` from `defaultValue: 1` to `0` — a value #364 says is **permanently retired** — left the count and the whole digest unchanged (#407 codex review round 2). A property is now included because it *exists*, not because someone remembered it. Sequence order inside a collection is preserved rather than sorted, since a composite key's column order is itself schema. **Annotations are enumerated explicitly via `GetAnnotations()`**, because EF exposes them through a *method*, not a property — a property walk never reaches them, and the version that merely claimed to cover them let `Npgsql:ValueGenerationStrategy` flip from `IdentityByDefaultColumn` to `IdentityAlwaysColumn` with the test still green (#407 codex review round 3). Sorted by name there, since `IReadOnlyAnnotatable` promises no order. A `System.Type` is a **leaf** (described by `FullName` only): recursing into one reaches `Assembly.Location`, so the digest encoded an absolute path to `System.Private.CoreLib.dll` and differed between a dev box and the CI runner — green locally, red in CI, which is the worst shape for a fence because the tempting "fix" is to re-baseline until CI agrees, after which it guards nothing. `InitialCreate_Description_ContainsNoEnvironmentSpecificData` now asserts the described text names no assembly file and contains neither the runtime nor the test-binary directory. Successive rounds of this fence each shipped a comment asserting coverage the code did not have; **the standard for changing it is a mutation that goes red, not an argument that it should** — and for anything digest-shaped, **running it repeatedly on one machine proves nothing about portability**, which is exactly how the path leak reached CI. **Never regenerate `InitialCreate`.** Two things in it are **hand-carried and un-regenerable** — a plain `dotnet ef migrations add` drops both on the floor: (1) the four **`lower("Name")` expression unique indexes** (`IX_EggGrades_AccountId_FarmId_LowerName`, `IX_ExpenseCategories_NameCi`, `UX_InventoryItems_Account_Farm_LowerName`, `IX_Products_AccountId_LowerName`) — EF cannot model a functional index, and losing them loses four uniqueness constraints; (2) the **static base reference data** raw SQL (see the bullet above). Regenerating also mints a **new timestamp**, which reorders it behind the migrations that depend on it and desynchronises `__EFMigrationsHistory` on every deployed database. `MigrationSecurityReviewTests` and `BaseReferenceDataMigrationTests` fail if any of that goes missing. diff --git a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs index c00504b47..ee40f0429 100644 --- a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs @@ -161,7 +161,44 @@ private static string MigrationIdOf(Migration migration) => // deployed database. private const int InitialCreateOperationCount = 114; private const string InitialCreateOperationDigest = - "320493300753dc57278ec990ff678e8fd7fed1a82ac438ea8563acda65756efc"; + "240b373e763e15b0c73e400bc8ef69b1d80b4548efb9f29439938facd2ae305a"; + + // #407 CI failure — the digest above is only meaningful if it depends on the + // MIGRATION and nothing else. The first version depended on the machine: it + // recursed into ColumnOperation.ClrType (a System.Type) and out through + // Assembly.Location, embedding an absolute path to System.Private.CoreLib.dll. + // It passed here and failed in CI, which is the worst possible shape for a + // fence — the pinned constant becomes a local artefact and the natural + // "fix" is to re-baseline it until CI agrees, at which point it guards + // nothing. + // + // Running it three times on ONE machine is what I checked, and it proved + // exactly nothing about this: same-run determinism is not portability. This + // test is the check that was missing — it asserts the described text carries + // no environment at all, so the failure cannot come back silently the next + // time the walk reaches a new property. + [Fact] + public void InitialCreate_Description_ContainsNoEnvironmentSpecificData() + { + var initialCreate = AllMigrations().Single(m => m.GetType().Name == "InitialCreate"); + var text = string.Join("\n", initialCreate.UpOperations.Select(Describe)); + + // A schema description has no business naming an assembly file, and no + // business containing the runtime's or the test binary's own directory. + Assert.DoesNotContain(".dll", text, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain(AppContext.BaseDirectory, text, StringComparison.Ordinal); + Assert.DoesNotContain( + Path.GetDirectoryName(typeof(object).Assembly.Location) ?? "", + text, + StringComparison.Ordinal); + + // Size is the cheap canary for the same defect: the machine-dependent + // version ballooned to ~168 MB by cycling Type -> Assembly -> Module -> + // Type. A real description of this migration is a few hundred KB at most. + Assert.True(text.Length < 2_000_000, + $"The operation description is {text.Length:N0} chars — the walk is almost certainly " + + "recursing into reflection metadata rather than schema. See the Type leaf case in DescribeValue."); + } [Fact] public void InitialCreate_OperationsAreFrozen() @@ -234,6 +271,24 @@ private static string DescribeValue(object? value, int depth) return Convert.ToString(value, CultureInfo.InvariantCulture) ?? "?"; case Enum e: return $"{e.GetType().Name}.{e}"; + + // A System.Type is a LEAF here, described by its full name only. + // + // #407 CI failure — without this the walk recurses into Type's own + // reflection surface (ColumnOperation.ClrType is a Type) and reaches + // Assembly.Location / Module.FullyQualifiedName / CodeBase, which are + // ABSOLUTE PATHS to System.Private.CoreLib.dll. The digest then + // encodes where the .NET runtime happens to live, so it differed + // between this dev box (a /nix/store path) and the CI runner and the + // test failed there while passing locally. It also made the described + // text ~168 MB, since Type -> Assembly -> Module -> Type cycles back + // through the whole reflection graph until MaxDescribeDepth cuts it. + // + // FullName is the schema-relevant part and nothing else about a Type + // is: a column's CLR type matters, the file its metadata lives in + // does not. + case Type type: + return $"Type({type.FullName})"; } if (depth >= MaxDescribeDepth) return $""; From a0726fc4eb68bd8bb83d809687807f1d47602534 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 3 Aug 2026 02:02:16 -0700 Subject: [PATCH 17/17] docs(test): record why the Type leaf uses FullName, not AssemblyQualifiedName (#396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment only — no behaviour change, digest unchanged, 11/11 green. Pre-empts the obvious review suggestion, which would be a regression rather than a fix: AssemblyQualifiedName carries the assembly VERSION, so a routine dependency bump would move this digest with no schema change — the same false-red churn the Assembly.Location leak caused, arriving by a tidier route. Checked rather than asserted: every ClrType InitialCreate actually uses is a BCL primitive (Guid, string, int, long, decimal, bool, DateTime, DateTimeOffset, DateOnly), all from one assembly, so the FullName collision this trades away cannot occur here. --- .../MigrationSecurityReviewTests.cs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs index ee40f0429..c128bf5f5 100644 --- a/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/MigrationSecurityReviewTests.cs @@ -287,6 +287,17 @@ private static string DescribeValue(object? value, int depth) // FullName is the schema-relevant part and nothing else about a Type // is: a column's CLR type matters, the file its metadata lives in // does not. + // + // NOT AssemblyQualifiedName, and that is deliberate rather than + // lazy: it carries the assembly VERSION, so a routine dependency + // bump would change this digest without any schema changing — the + // same false-red churn the path leak caused, arriving by a tidier + // route. FullName can in principle collide between same-named types + // in different assemblies; every ClrType this migration actually + // uses is a BCL primitive (Guid, string, int, long, decimal, bool, + // DateTime, DateTimeOffset, DateOnly), so there is nothing to + // collide with, and a column typed as two different same-named + // types would be a far larger problem than this digest. case Type type: return $"Type({type.FullName})"; }