Repository navigation
Sales: per-farm discount ceiling, with Owner/Manager approval above it #727
Description
Activity
Mockup
Measured against its own precedent, this is bigger than it reads. Commit
0955095("feat: add configurable worker sale allocation (#619)") added one policy field toAccountand touched 58 files: 7 docs/spec, 16 src, ~14 test, 1 Playwright spec, 9 web. A percent instead of an enum dropsenums.tsbut adds numeric range validation. Expect ~25 files, 2-3 days.The guard nobody remembers.
BaseReferenceDataMigrationTests.cs:71-101builds twoHashSet<string>overAccount's mapped properties, asserts an exact partition, then hard-codes the counts:Assert.Equal(10, accountComparedProperties.Count); Assert.Equal(8, accountExcludedProperties.Count);
A new
Accountproperty fails this until it is placed in one set and the literal is bumped. Find it withgrep -rn "AssertExactMappedPropertyPartition" tests/, not by recall. (AccountIdConcurrencyTokenModelTestsandFlockScopeDiscoveryTestsuse>=floors and are additive-safe by construction; this one uses==, and that is why it bites.)UpdateFarmSettingsCommandis a whole-block positional replace includingVersion, so a new field is a required contract change onPUT /account/settings.Account.UpdateSettingsis positional too, which makesCurrencyLockRaceTests.cs:65,CurrencyLockSerializationTests.cs:60andSalesProductTests.cs:326compile errors.FarmSettingsTests.cs:55has aBody(…)helper with one optional param per setting — one line there covers ~15 call sites.The real risk is the enforcement, not the setting.
ConfirmSaleHandleris 265 lines of carefully ordered locking (AccountFOR SHARE→ SalesOrderFOR UPDATE→ fresh role read → FIFO lotsFOR UPDATE). The check must read the policy off the already-lockedaccountinstance, and must sit afterCheckCanConfirm()and after the role re-read, since the rule is role-conditional. It also needs a distinct error code and a 403-vs-422 decision, because the endpoint'sMapFailurebranches onAuth.Forbiddenversus everything else.Two unstated requirements.
GET /account/settingsisAdminOnly, butSalesPageneeds the ceiling to decide whether to warn — #612 solved the same problem forWorkerSaleAllocationPolicyby threading it through a different route; check how before designing. And this issue says Owner-only, whilenav.tsxgates/settingsonisAdmin= Owner or Manager. That is a new gate shape for that screen.A migration adding a column with a
defaultValuealso gets a dedicated migration test here —WorkerSaleAllocationPolicyMigrationTests.cs(78 lines, real Postgres, backfill + downgrade + re-upgrade) is the template.Correction: the body's "Owner-only" for the setting is wrong
Checked against source. A Manager can already reach Farm settings, in the nav and at the API:
AuthPolicies.AdminOnlyisRequireRole(Roles.Owner, Roles.Manager). The name is historic from F19: Admin-gate corrective/destructive actions — stepping stone to full RBAC #73 and does not mean Owner-only.OwnerOnlyis the separateRequireRole(Roles.Owner)policy, reserved for user management.GETandPUT /account/settingsare bothAdminOnly(AccountEndpoints.cs:26,31).nav.tsx:79gates/settingsonisAdmin, with a comment saying it deliberately mirrors the API gate.AuthContext.tsx:22definesisAdminas Owner or Manager.
This issue's body said Owner-only for the setting while its own title says Owner/Manager approval. The title is the right one.
Recommendation: two separate gates, and only one of them is strict
The ceiling value: Owner + Manager, the screen's existing gate.
AuthPoliciesdefines Manager as the "corrective actions, config, money" tier, and a Manager can already void a confirmed order, void a payment and change the farm currency. A discount ceiling is smaller than all three.The mechanical argument is the stronger one.
UpdateFarmSettingsCommandis a whole-block positional replace includingVersion, so the SPA sends every field on every save. A per-field Owner-only rule cannot be an endpoint policy; the handler would have to compare the incoming ceiling against the stored value and refuse when a Manager changed that one field. That is a new pattern on a screen with no mixed per-field permissions today.Confirming above the ceiling: this is where strictness belongs. It is a fresh role check inside
ConfirmSaleHandler, which already does a role re-read inside the transaction, so an Owner-only rule there costs nothing extra. If the farm owner wants a hard "only I can approve a big discount", that is the lever, not the settings field.Net: drop "Owner-only" from the setting, keep the approval tier as a deliberate choice between Owner and Owner+Manager, and state which in the acceptance criteria.
Decisions taken before implementation, 2026-09-11
Design record:
docs/plans/727-discount-ceiling/01-design.mdonfeat/727-discount-ceiling.
Synthesized from two independent design candidates; every fork below was decided
deliberately, and the ones the owner ruled on are marked.The four open questions in the body, answered
Question Decision Why Approval tier Owner + Manager (owner's call) Matches the title. A Manager can already void a confirmed order, void a payment and change the farm currency — a discount ceiling is smaller than all three. The correction comment above already withdrew "Owner-only" for the setting; this extends the same reading to the approval. Per line or per order Per line (owner's call) An order total is gameable by padding with at-list lines, which is the exact failure this epic exists to stop. Also what makes "the refusal names the offending line" meaningful. Approval flow Honest refusal, out of band (owner's call) The order stays Draft; the seller asks a Manager, who confirms it in the app. No queue, no request object, no new order state. #719 open question 3 answered: yes, "Sales cannot confirm, go find a manager" is enough. Settings gate AdminOnly(Owner + Manager)Per this issue's own correction comment. UpdateFarmSettingsCommandis a whole-block positional replace, so a per-field Owner-only rule would be a new mixed-permission pattern on a screen that has none.Amendment to acceptance criterion 5
A line with aNULLlist price cannot trip the ceiling (no list price, no measurable discount).This criterion is superseded. It was written 2026-09-08, one day before #720 shipped
and replaced the single NULL with a four-valueListPriceBasis.SalesOrder.cs:304-308
names #727 by number and says the opposite for one of those four values:for
ProductUnpricedandNotComparable, "no comparable list price" is a RECORDED FACT
and no discount is computable; forPreDatingit means "we do not know", and the line may
have been deeply discounted. Those two need opposite treatment.The replacement criterion, decided by the owner:
ListPriceBasisAt the ceiling Why RecordedMeasured The list price is a captured fact. ProductUnpricedNever violates Recorded fact: the product had no list price, so nothing was discounted from anything. NotComparableNever violates Recorded fact: currency or minor unit did not match. PreDatingRefused for a ceiling-bound actor We do not know. Fails closed. The hole this closes. A pre-#720 draft can still be re-priced to anything today. It
carries no list price, so it does not tripHasBelowListLine, so #721 never asks for a
discount reason — and if the ceiling skipped it too, a Sales or Worker user could give an
unlimited discount with nothing recorded anywhere.The cost, bounded. The
ListPriceBasisbackfill relabelled every line older than
2026-09-09 asPreDating, then DROPped the default, and nothing in the application ever
writes that value — so the population can never grow and shrinks as those drafts are
confirmed or voided. Confirmed pre-#720 orders are unaffected: they are notDraft, so
they cannot be re-confirmed at all. An Owner or Manager confirms such an order untouched.ProductUnpricedandNotComparableare not a hiding place either: minting or unpricing a
product isAdminOnly, i.e. exactly the tier that may exceed the ceiling anyway, so there
is no privilege to gain.Shape, decided
CheckCanConfirmholds rules about the ORDER; the ceiling is a rule about the ACTOR.
So the ceiling gets its own pureSalesOrder.CheckWithinCeiling(ceiling), called from
ConfirmSaleHandlerafter the fresh in-transaction role read (Configurable Worker sales allocation scope by flock assignment #612) and before the
EggLots lock. It is not folded intoCheckCanConfirm, whose check order is pinned
test-by-test and whichConfirm()re-runs at mutation time.- 422, not 403.
EggLot.AssignedFlocksInsufficientStockis a role-conditional refusal
on this same handler and it is already a 422. 403 here means "your role cannot confirm
sales orders at all"; Sales generically can.SaleEndpoints.csneeds no edit — the
new codes fall into the existingelse → 422. - Stored as
int? MaxDiscountBasisPoints, plain nullable, no backfill. Compared
cross-multiplied inInt128((list − unit) × 10 000 > bp × list), so nothing is ever
divided and the boundary cannot disagree with itself. Exactly on the boundary is
ALLOWED — "maximum 10%" means at most 10%. NULL is "no ceiling";0is a legal,
different setting meaning "no discount at all". - A Sales user sees the ceiling before confirming, via a derived per-caller
yourMaxDiscountPercenton the role-agnosticGET /account— the established
ShowFarmWideSaleAllocationNoticepattern. Withholding it buys nothing (the refusal
discloses the same number on the first attempt) and costs the blind retries this issue
exists to prevent. The SPA marks the breaching row live and disables Confirm, so the
discount-reason dialog never opens for an order that is about to be refused. - The refusal's numbers render client-side. A server-composed sentence would print
18.0%to a farm whose locale writes18,0. The catalog key is parameter-free in
en/es/tl; the Englishdetailcarries the numbers and names the line by egg grade for
curl, k6, seeders, logs and tests. - Settings write stays on the plain optimistic path. The
FOR UPDATEescalation exists
for read-then-decide fields (currency probes whether money already moved); a ceiling
decides nothing from elsewhere. The read side is already ordered by the Account
FOR SHAREthe confirm holds. This is asserted, not assumed: an integration test copied
fromSaleAllocationPolicyTests.ConfirmSale_ParksOnTheAccountLock_...with the ceiling in
place of the policy. If it goes red, that decision was wrong.
Deliberately out of scope
DiscountReasonCode.ManagerApprovedstays unenforced. A reason describes the sale;
the ceiling describes the actor. Coupling them would make Reports: discount totals per salesperson and per customer, in report and CSV export #725's per-reason totals a
function of who clicked confirm. Who approved is already recorded better —AuditWriter
requires an actor (fix(seed): seeded audit events carry "(unresolved)" as the actor, now visible on record History columns #500), so theSalesOrder.Confirmrow names them.- No
ceilingOverrideon the confirm audit payload. That is the fact Alerts: raise an alert-centre entry for a large discount at confirm #728 needs, and it
belongs to Alerts: raise an alert-centre entry for a large discount at confirm #728 alongside the alert it feeds. - No ceiling on the order total; no per-customer or per-product override.
- added 4 commits that reference this issue
on Sep 11, 2026
Part of #719. Blocked by #720 (snapshot) and #721 (reason).
What
A per-farm maximum discount percent. Below it: allowed, reason required, visible.
Above it: a Sales user cannot confirm — the order stays Draft until an
Owner/Manager approves it.
Why this one is different
Every other slice in #719 reports. This is the only slice that prevents.
Everything else makes a discount visible after it has been given; this is the
only one that stops an unauthorised discount from becoming a confirmed sale.
Scope
MaxDiscountPercentonAccount, besideWorkerSaleAllocationPolicy— theexisting precedent for a per-farm policy setting on that aggregate. Nullable /
unset means no ceiling, so existing farms are unaffected until an Owner sets one.
ConfirmSaleHandlerrefuses a confirm from a Sales-role user when any lineexceeds the ceiling; the order stays Draft.
shape —
AuthPolicies.AdminOnlygates void and payment-void(
src/Cluckwork.Api/Endpoints/Sales/PaymentEndpoints.cs:36) for the samereason.
to ask for rather than retrying blindly.
Decide before writing code
real in-app approval request/queue? EPIC: Discount visibility and control on sales #719 open question 3. Out-of-band is
cheaper and probably right for the first cut, but say so deliberately — a
half-built approval flow is worse than an honest refusal.
padding an order with at-list items.
Acceptance
Draft.
NULLlist price cannot trip the ceiling (no list price, nomeasurable discount).
Repo rules
tests that only reach the handler layer, and to
tools/simulation/k6/and thePlaywright specs. Read those callers.
specs/product/GLOSSARY.mdand the in-app glossary in the same PR.