Skip to content

fix(catalog): refuse stale product and packed-unit edits, and order daily-entry grade lines - #1256

Merged
mforce merged 12 commits into
mainfrom
fix/catalog-version-grade-order
Oct 12, 2026
Merged

mforce merged 12 commits into
mainfrom
fix/catalog-version-grade-order

Conversation

@mforce

@mforce mforce commented Oct 12, 2026 •

Copy link
Copy Markdown
Owner

Two data-correctness fixes found during the #1215 split work. No issue tracks them, so this body records them.

1. Product and packed-unit edits overwrote each other

PUT /products/{id} and PUT /egg-unit-conversions/{id} sent no base version. When two admins edited the same product price or eggs-per-unit, the later save silently replaced the earlier one. Eggs-per-unit drives stock math.

  • Both requests and commands now carry version, following the UpdateCustomer pattern. An omitted version is a 400 (*.Version.Required), never a silent 0.
  • The handlers compare it and return Product.VersionMismatch / EggUnitConversion.VersionMismatch as 409. The EF Version token still returns 409 when two requests overlap. Both aggregates already bumped Version.
  • The Products screen sends the loaded version. On a version 409, it reloads the lists, rebinds the dialog to the other admin's values, uses a new idempotency key, and shows a message. This is the Expenses/History rebind pattern. Other 409s (duplicate name, the unique-index Data conflict, an idempotency key still in flight) show their own error and keep the typed form and the key.
  • Activate/deactivate are unchanged. They state the target state, so a stale request cannot lose an update, and a repeated toggle already returns 409.

2. Daily-entry grade lines came back in physical row order

DailyEntryRepository included Grades with no ORDER BY. The History "Graded" column therefore changed order between databases. The three filtered reads (GetByIdAsync, GetReadOnlyAsync, ListAsync) now order lines by the grade's SortOrder, then Name, then id. This is the order EggGradeRepository lists grades in. The grade lookup also matches the line's AccountId.

The two filter-free write lookups (GetByIdForFlockScopedWriteAsync, FindByNaturalKeyForFlockScopedWriteAsync) keep main's plain Include. Their callers return an id, a Guid or Id/Status/EggLotIds, never grade lines. With a plain Include, they do not read EggGrades without the tenant filter, and BypassAllowList.cs is unchanged from main. No client code re-sorts grade lines; History renders the array as received.

Evidence

Race tests (CatalogTests):

  • Two concurrent updates from the same version produce exactly one 204 and one 409. The stored row is entirely the winner's payload. One test each for products and conversions.
  • Each type has a stale-version test (409, row unchanged) and a missing-version test (400).

Mutations, each red then restored:

Mutation Result
Remove the product handler's version check Product_StaleBaseVersion_Is409_AndChangesNothing fails
Remove the conversion handler's version check Conversion_StaleBaseVersion_Is409_AndChangesNothing fails
Remove Product.Update's Version++ the product race test fails because both requests succeed
Make the SPA rebind on every 409 the three non-version-409 SPA cases fail
Never rebind in the SPA both rebind tests fail
Plain Include in the filtered reads Reads_ReturnGradeLinesInCatalogOrder_NotPhysicalOrder fails
Route either write lookup through WithGrades TenantBypassRealTreeTests fails (unexcused bypass)

Grade order test: the test rewrites two grade rows so they sit on disk in reverse catalog order, and asserts that order with ORDER BY ctid. It then checks the order from both GET /daily-entries/{id} and the list.

SQL check: a temporary ToQueryString dump, not committed, confirms the following. Every grade subquery in the three filtered reads carries AccountId. Neither write lookup references EggGrades.

Callers outside CI (#394)

  • tools/simulation/k6/bundles.js only reads GET /products?includeInactive=true.
  • tools/simulation/ui/specs/phone.spec.ts only navigates to /products.
  • Seeders, CLI verbs and MCP tools never call either update.
  • No checked-in docs or specs describe either request body.

The in-repo callers now send version: the SPA, and CatalogTests, SalesProductTests, ProductAuditPayloadTests and CurrencyLockRaceTests.

Other changes

  • ProductsPage.tsx grew past the 500-line file limit, so the Packed units table and inspector moved unchanged into PackedUnitsPanel.tsx.
  • The precache ceiling rises from 1,661 to 1,662 KiB; the merged branch measures 1,661.68 KiB locally. The two new messages add about 1 KiB.

Review

Two local review rounds. Round 1 found two problems, both fixed in new commits:

  • The rebind caught every non-duplicate-name 409 (be5d1e8c).
  • The ordering predicates in the filter-free write lookups sat outside the bypass guard's member hash (5c754b02).

Round 2 was clean.

Tests run locally

  • dotnet build Cluckwork.slnx: clean.
  • Application tests, filtered to the catalog validators, TenantBypass, Architecture and validator error codes: 502 passed.
  • Integration tests under Docker: CatalogTests, DailyEntryGradeTests, SalesProductTests, ProductAuditPayloadTests and CurrencyLockRaceTests (61 passed). Earlier runs also covered SubmitDailyEntry, DailyEntryAdjust, FlockScope, TenantIsolation and Mcp.
  • Web: typecheck, lint, ProductsPage.test.tsx, src/api and the i18n suites (551 passed), npm run build, npm run verify:sw. The version fields live in web/src/api/cluckwork/catalog.ts since refactor(web): split the API wrappers out of cluckwork.ts by area #1264 split the API client.

Docs-impact: none — two error messages (en/es/tl) for a save conflict; no new concept, control or label, so the glossary and Help page are unchanged.

Screenshots follow in a comment.

Product and egg-unit-conversion updates sent no base Version, so two admins
editing the same price or eggs-per-unit silently overwrote each other. Both
commands now carry the loaded Version (required, like UpdateCustomer), the
handlers compare it and return a VersionMismatch 409, and the SPA sends it and
rebinds the dialog to the winner's values on a conflict.
The version-conflict rebind took ProductsPage past the 500-line file limit;
the Packed units table and inspector move out unchanged.
Every DailyEntryRepository read included Grades without an ORDER BY, so lines
came back in physical row order, which differs between databases and reordered
History's Graded column. One ordered Include now sorts them by the grade's
SortOrder, then Name, then id, as EggGradeRepository lists grades.
The rebind treated every 409 except a duplicate name as a stale version, so
the unique-index race and an in-flight idempotency key also discarded the
typed form, showed the wrong message and dropped a key that had to be kept.
Only VersionMismatch and the EF race's Concurrency conflict rebind now.
… ordering

The two IgnoreQueryFilters write lookups ordered their lines through the
shared WithGrades helper, so its EggGrades subqueries ran without the tenant
filter and outside the bypass guard's member hash. Neither caller shows its
lines to anyone, so both return to main's plain Include and main's allow-list
rows. WithGrades now also matches the grade's AccountId to the line's.
…rade-order

# Conflicts:
#	web/scripts/verify-sw.mjs
@mforce mforce added this to the Platform hardening milestone Oct 12, 2026
@mforce

mforce commented Oct 12, 2026 •

Copy link
Copy Markdown
Owner Author

Screenshots of a save conflict on the Products screen. The capture opens the edit dialog and types a change. Another session then saves the same row through the API at its current version, and the dialog's Save is clicked. The farm is readme-farm, captured at 1:1, light theme.

Captured at 669bc580 (after) and at origin/main b02648c1 (before), each from its own rebuilt stack. The head is now a6056b10, which adds only the merge of #1254 (the API client split); the Products screen is unchanged.

  • Before: the dialog closes with no message. The row holds the dialog's values, so the other session's price or eggs-per-unit change is gone.
  • After: the dialog stays open, refilled with the other session's values, and shows the rebind message. The next Save sends the new version.

No page errors were recorded during the runs.

Before, product, 1280 light

After, product, 1280 light

Before, product, 390 light

After, product, 390 light

Before, packed unit, 1280 light

After, packed unit, 1280 light

Before, packed unit, 390 light

After, packed unit, 390 light

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant