Repository navigation
Sales: record list/old/new price in the order-line audit payload #722
Description
Amendment (2026-09-10): this issue is backend-only and shipped in #742. The payload it records was invisible on screen until #747/#748 (snapshot the product name) and #745/#749 (render it as a Details column). See this comment for what landed where and what this issue never claimed. Body below left as written.
Part of #719. Blocked by #720 (needs the list-price snapshot).
What
Put the actual prices into the sales-line audit events.
AddOrderItemHandler.cs:106 and UpdateOrderItemHandler.cs:30 already write
SalesOrder.AddItem / SalesOrder.UpdateItem audit events — but with no
payload. The trail today shows someone touched a line, never what price
they set. That is why the discounts that triggered #719 are not reconstructible
from stored data.
Scope
Carry list price, previous unit price and new unit price in the audit payload for
both actions. AuditWriter already resolves the actor (#500) and AuditEvents
already has a durable global ordering key (#508), so this one change makes "who
discounted what, in what order" answerable from data already being written.
Guard to read before writing the call site
AuditVocabularyCoverageTests accepts only a literal AuditActions.X — or a
ternary of two such references — as the action argument of an
IAuditWriter.WriteAsync call, and fails closed on everything else. The obvious
tidy-up here (forwarding both calls through one shared private helper) goes red
on a test you would never have opened. Keep the two call sites literal.
Acceptance
- Adding a line records list price and unit price.
- Editing a line records the price it changed from and to.
- Events are attributable to the acting user and orderable across a same-second
burst (fix(api): order same-instant audit events by a durable monotonic key, not a random Guid #508'sSequencetiebreak). AuditVocabularyCoverageTestsstays green.
Note
Audit rows are not a reporting surface — the report in #719 reads the order
lines, not this. This slice exists so a specific past incident can be
investigated after the fact, which is the thing that was impossible in the case
that opened #719.
Activity
mforce commentedon Sep 8, 2026
Mockup
This slice is much smaller than the issue implies, and it should ship first.
IAuditWriter.WriteAsyncalready has the parameter:AuditWriterserialisesdetailsintoAuditEvent.DetailsJson, and ~30 call sites already pass it —UpdateFarmSettingsHandler.cs:148passesdetails: new { before, after = Snapshot(account) }, which is this slice's exact shape. No signature change, no new column, no migration.DetailsJsonalready reaches the API response (AuditEndpoints.cs:45), the CSV export (ExportQueries.cs:259) and the SPA (AuditPage.tsx:409renders it as the row'stitle). So no i18n, no glossary, no catalog parity. The house test idiom is oneAssert.Contains("\"amountMinorUnits\":500", row.DetailsJson)inAuditTests.cs.The guard named in this issue does not apply the way I wrote it:
AuditVocabularyCoverageTestssplits the arguments and checks onlyargs[0]andargs[1]. Adetails:argument is invisible to it. The rule that still holds is the other half — reuse the existingAuditActionsconstants; minting a new one dragsweb/src/i18n/enums.tsand three locales.~2 src files, 1 test file, hours. Note the caveat drawn on the artboard: today the SPA shows
detailsJsonas a raw JSON tooltip with no per-key renderer, so the readable table in the mockup needs a small follow-up. And this slice need not wait for #720 if it logs old/new unit price only —UpdateOrderItemHandlerholds only the order, not the product, so the list price there arrives with #720.test(sales): make the order-line audit payload fields distinguishable (…
docs(agents): a PR closes its issue from the body, not the title (#722)
docs(agents): a PR closes its issue from the body, not the title (#744)
mforce commentedon Sep 10, 2026
Shipped, and what was deliberately left out
Merged as
97c866f(PR #742). All four acceptance criteria delivered:AddItem_RecordsListPriceBasisByNameUpdateItem_RecordsThePriceItChangedFromAndToSequence) — this slice does not regress itAuditVocabularyCoverageTestsstays greenThe Add payload carries
salesOrderItemId,productId,quantity,unitPriceMinorUnits,listUnitPriceMinorUnits,listPriceBasis,currencyCode,currencyMinorUnit. The Update payload adds abefore/afterpair.One thing this issue's body got wrong, worth recording: it says the Add path cannot know the line id. It can, but only after the save that assigns it — so the handler's tail now runs inside
ExecuteInTransactionAsync, saving to get the id, then writing the audit row, then committing together. Mutation row M3 proves it: move the audit write back above the save and the payload carriesGuid.Empty. That mattered because the owner supplied a fact the issue did not have — duplicate products on one order are the common case here, so identifying a line by product would not have identified it.Deliberately NOT in this slice, now filed
detailsJsonas a rawtitletooltip (AuditPage.tsx:409), so the readable table in this issue's mockup does not exist yet. Scoped out by this issue's own comment; the work is mostly i18n, since rendering the keys as labels makes them translatable strings in three locales.listPriceBasis; found while designing it.AddOrderItemHandlersaves inside its transaction delegate with noDiscardChanges-style cleanup, unlikeUpdateFarmSettingsHandler. Unreachable today; a trap for a futurereturn falsebelow the inner save.Not reopening this issue — its stated acceptance is met, and the repo's rule is that unimplemented criteria become follow-up issues rather than bullets left open. The three above are that. Filed late and only after the owner asked, which is the miss: the design recorded #745 and #746 as "file at Phase 13b" and close-out ran without them.
mforce commentedon Sep 10, 2026
One more, found after the fact by opening this issue's mockup rather than reading the comment about it: #747 — the payloads carry
productIdbut no product name, while the artboard's Details column renders "Medium Eggs". A renderer resolving the id live gets today's name, so a renamed product would re-render history under a name the seller never saw — the same hazard #720's list-price snapshot exists to prevent, one field over. The house idiom already snapshots names in three sibling catalog handlers.My design diffed this slice's block against
CreateProductHandlerfield by field and matched its transaction shape while missing that it snapshots a name. Five review seats did not catch it either, because every one of them was briefed on code, tests or repo rules — none looked at the rendered intent.fix(audit): store catalog enums by name and guard the add-item transa…
mforce commentedon Sep 10, 2026
Amendment — where this actually landed, and what it did not cover
Written after the fact so the next reader of this issue is not misled by its closure. The body above is left exactly as written; this comment is the correction.
What shipped, and where
#742shipped the whole of this issue's Scope and met all four Acceptance bullets:AddOrderItemHandlerUpdateOrderItemHandlerAuditVocabularyCoverageTestsstayed green, with both call sites kept literal per the guard warning abovePinned by
tests/Cluckwork.Api.IntegrationTests/SalesOrderAuditPayloadTests.cs.One design fact worth carrying forward, because it is not obvious from the scope: the
AddItempayload names the line, not just the order.SalesOrderItem.Idis assigned by EF duringSaveChanges, so the audit write had to move insideExecuteInTransactionAsyncafter an interleaved save. Without that, an order carrying the same product on several lines — the normal case here — has no attributable discount, becauseproductId+quantitydoes not identify a line.What this issue did NOT cover, and where that went
This issue is backend-only, deliberately. Its acceptance list contains no UI bullet, and its own closing Note says audit rows are not a reporting surface. That is accurate as written — but it means closing it left the payload invisible on every screen: the SPA's audit table rendered a
Reasoncolumn that was an em dash on every sales-line row, so nothing a user could open showed the prices this issue had just started recording.Two follow-ups close that, and neither is a criterion this issue failed — both are scope it never claimed:
#748, merged) — the payload storedproductIdand no name. The audit screen's own mockup renders product names, and resolving an id live would re-render history under today's name, so a renamed product would show the seller a name they never saw. The name and unit are now snapshotted at write time, the same rule Sales: snapshot the list price on the order line (discount foundation) #720 applies to the list price.#749) — the SPA now renders the payload as a readable Details column:Large Eggs$0.40→$0.38(list $0.45), with an explicit no-list-price wording rather than a zero.The honest note
The gap between "this issue is complete" and "a person can see a discount" was one full slice wide, and it was the owner who caught it, not the delivery process — the process had already closed this issue and moved on. If you are writing a follow-up to this one, the transferable lesson is that a payload-only slice is not done in any sense a user can observe until something renders it, and that is worth stating in the issue rather than discovering after it closes.
docs(plans): commit the #722 and #745 design records
docs(plans): commit the #722 and #745 design records (#754)
chore(main): release 0.1.0 (#542)