feat(sales): require a discount reason when confirming a below-list order by mforce · Pull Request #756 · mforce/cluckwork

mforce · 2026-09-11T00:12:22Z

Closes #721

Epic #719, slice 5. Confirming a sales order with any line priced below its
snapshotted list price now requires a reason, chosen from a closed picklist with
an optional note that becomes required for Other. The reason is stored on the
order and surfaced on the order panel, in the Orders list and in the
sales-orders CSV export.

Design record: docs/plans/721-discount-reason/01-design.md.

What decides "discounted"

SalesOrder.HasBelowListLine — a line counts only when a comparable list price
exists and the sale price is strictly under it. That answers two of the epic's
open questions: a NULL list price is not a discount (nothing to compare
against), and a price above list is not one either. Same two branches the
SPA's lineDiscount already uses, so the screen and the server cannot disagree.

The reason is per order, not per line (epic open question 2). Per line
multiplies clicks on the app's most routine write and buys precision no
consumer — #725, #726, #728 — asks for.

Two corrections the work made to its own design

The optional body does not bind the way the design claimed. The design cited
DisableUser as proof that a bodyless POST still reaches an endpoint with a
nullable body parameter. It proves the opposite:
DisableUserTests.Disable_WithNoContentTypeAtAll_Is404_NotUnsupportedMediaType
pins that exact 404. A typed body parameter attaches application/json Accepts
metadata, the consumes matcher turns it into a route constraint, and a POST with
no Content-Type — which is what every existing caller sends — stops matching
and falls through to Program.cs's /api/{**rest} catch-all. Four tests went
red the moment the parameter landed. The fix is one line of route metadata,
.Accepts<ConfirmSaleRequest>(isOptional: true, "application/json", "*/*"),
which changes no caller; Confirm_WithNoContentTypeAtAll_StillConfirms pins it.

The rules belong in CheckCanConfirm, not in Confirm. The design put them
in Confirm, which meant a missing reason was refused only after
ConfirmSaleHandler had planned and applied a whole FIFO allocation — and the
handler's "contradicted its own CheckCanConfirm" throw then turned that 422 into
a 500. Both corrections are written back into the design record.

#394 — the callers CI does not cover

Read from source against the seeded catalog, not inferred from a green run.

tools/simulation/ui/specs/worker-sale-allocation.spec.ts broke. It prices
a line at 0.01 against Sim Large Eggs at 45, so after this change the
first click opens the reason dialog, the spec's wait for
sales:confirmOrderTitle never resolves, no /confirm POST is sent, and its
waitForResponse can never settle. Fixed here by driving the new dialog while
keeping 0.01 and every Configurable Worker sales allocation scope by flock assignment #612 assertion, so it now covers both paths.
DemoDataSeeder and SimulationDataSeeder are unaffected: both add lines with
a null unit price, so the handler prices them at list. The simulation seeder
does seed below-list lines via EnsureExtraLineAsync, but only onto drafts
that are never confirmed — a near miss worth knowing about before moving them.
tools/simulation/k6/ never confirms a sale at all (bundles.js:391-393).
tools/simulation/ui/specs/sales.spec.ts survives by five minor units — it
sells at 0.50 against a 45 list — and now says so in a comment.

Verification

Driver-verified (run and read by the reviewing agent, on the final source
state, not quoted from the implementer):

Passed!  - Failed:     0, Passed:   428, Skipped:     0, Total:   428 - Cluckwork.Domain.Tests.dll
Passed!  - Failed:     0, Passed:    10, Skipped:     0, Total:    10 - Cluckwork.AppHost.Tests.dll
Passed!  - Failed:     0, Passed:   281, Skipped:     0, Total:   281 - Cluckwork.Application.Tests.dll
Passed!  - Failed:     0, Passed:  1743, Skipped:     0, Total:  1743 - Cluckwork.Api.IntegrationTests.dll
 Test Files  122 passed (122)
      Tests  2810 passed (2810)

dotnet build Cluckwork.sln is 0 warnings / 0 errors, tsc -b --noEmit is
clean, and tools/schema-docs/generate.sh --check reports docs/schema/ is up to date (#417).

One mutation planted and observed by the driver, rather than trusting the
implementer's report: flipping HasBelowListLine's strict < to <= turns four
domain tests red; restored, re-run, 428 pass and the file diffs clean.

Implementer-attested, not driver-verified: 39 further mutations across the
domain rules, the API surface, the SPA and the new guards, all reported red with
none surviving. git grep MUTANT returns only the repo's own pre-existing
Playwright harness.

CI-verified (corrects an earlier claim in this body): the
Playwright smoke over the simulation fixture job passed in 4m8s against a real
seeded stack, so both specs were exercised — including
worker-sale-allocation.spec.ts, the one this change broke and repairs here. An
earlier revision said neither suite had been run by anyone; that was written
before the job reported and was wrong.

Still owed:

Screenshots. This slice adds a dialog, so it owes a before/after capture
under the rule broadened in docs(agents): drop the commit and push gate, and require screenshots on UI changes #757. Blocked on a local dev admin credential, not
on the work.

Reviewer status. CodeRabbit did not review this PR. Its check reports pass,
but the detail reads Review skipped: manual review required for this OSS repository — the green badge here means no review ran, not that nothing was
found. Recorded so the silence is not read as a clean pass.

Every acceptance criterion is met

The reason survives to order detail, the Orders list, the sales-orders
CSV export and, as of c364f93, history — the SalesOrder.Confirm audit
row carries discountReasonCode and discountReasonNote, and the Audit page's
Details column renders them.

An earlier revision of this body argued for leaving history out, on the grounds
that a copy in the audit row would be a second source of the same truth. The
owner reversed it, and the reversal is right: the reason is write-once and the
audit row commits in the same transaction as the columns, so the two cannot
disagree. The duplication risk that argument guarded against does not exist. The
design record's non-goal is struck through with that reasoning rather than
quietly rewritten.

Two behaviours worth knowing. An order confirmed at list writes no payload at
all rather than a payload of nulls. And there is no backfill: an order
confirmed before this shipped shows an em dash in Details even where its columns
carry a reason, which is the honest rendering of "recorded somewhere this view
cannot reach".

…rm route The route now takes a nullable ConfirmSaleRequest carrying the discount reason fields #721 will populate. A typed body parameter attaches application/json Accepts metadata, which the consumes matcher turns into a route constraint, so every caller that POSTs /confirm with no Content-Type at all started 404-ing through Program.cs's /api/{**rest} catch-all. Declaring */* alongside application/json keeps those callers matching, and the new Confirm_WithNoContentTypeAtAll_StillConfirms pins it.

SalesOrder gains DiscountReasonCode and DiscountReasonNote, both nullable and both written only by Confirm. HasBelowListLine compares the sale price strictly against the line's snapshotted list price, so a NULL list price and an above-list price are both left alone. The whole rule lives on the aggregate because the seeders reach Confirm through the handler and never see a validator. One migration, columns nullable with no backfill: NULL on a confirmed order means the reason was not recorded, never that no discount was given. The code is stored by name so reordering the enum cannot relabel history. Schema docs regenerated.

…e export The confirm command gains the two optional fields, ConfirmSaleValidator turns a malformed code or an overlong note into a 400 before any lock is taken, and the handler refuses an unknown code before opening the transaction — the seeders build the command directly and never pass a validator. One parser, DiscountReason.TryParseCode, backs both, so the boundary and the handler cannot disagree about what a legal code is; it rejects a numeric string, an undefined number and a comma-separated combination that Enum.TryParse alone would accept. The discount rules moved into CheckCanConfirm beside NotDraft and NoItems, so a missing reason is refused before the FIFO plan runs rather than after it, and SalesOrderResponse and the sales-orders export both carry the stored reason.

…rder The confirm button opens a picklist instead of the plain yes/no when any line is priced below list, decided by the same lineDiscount predicate the row markers already use rather than a second one that could drift from it. The reason and its note ride on the confirm, and the stored reason then shows on the confirmed order beside the give-away it explains and in the Orders list. useConfirm gains a third shape rather than the screen gaining a second dialog idiom: askChoice keeps the inline refusal askReason already has, so a missing note never costs the user the option they had chosen. confirm and askReason are untouched and their tests pass as written. Strings in en, es and tl, and the codes render through i18n/enums.ts.

…spec worker-sale-allocation.spec.ts sells at 0.01 against a seeded list price of 45, so the confirm button now opens the reason picklist and its waitForResponse on POST /confirm could never settle. Nothing in CI runs that spec, so it would have hung rather than failed. The deep discount stays; the spec picks a reason. sales.spec.ts sells at 0.50 against the same 45 and is unaffected, which is now stated where the price is defined. SimulationDataSeeder's below-list line sits on a draft that is never confirmed, pinned the same way.

… glossary Same PR as the behaviour, per the owner directive. Each locale names the control with its own label — Discount reason, Motivo del descuento, Dahilan ng diskwento — so no locale disagrees with itself about what the field is called (#688).

A fieldset maps to role="group", which does not support aria-invalid, so the attribute was accessibility that was not there. aria-describedby is global and already carries the error; the focus return to the first radio is what actually takes the user to it.

…se the review gaps The confirm dialog now matches the mockup on #721: it keeps its title and its FIFO prose and gains the order's discount figure and the below-list lines it comes from, both read from the same orderDiscount/lineDiscount the screen behind it renders, so the dialog can never quote a different number. From the adversarial pass over the diff: - DiscountReasonCode was mirrored into enums.ts by hand with nothing coupling them, so a member added on either side would silently never reach the other. DiscountReasonVocabularyTests pins both directions, the way AuditVocabularyCoverageTests already pins the two audit vocabularies, and the Other literal that drives the inline note rule is pinned with it. - ConfirmSaleValidator had no test at its own layer; the integration tests can only see the status code, not which layer refused. - The picklist test compared the rendered radios to the constant the page maps over, so a rename moved both sides together. It asserts literal labels now. - askChoice had no supersede and no unmount test, the one arm of the widened settle union the design named as this slice's risk. - The two discount refusals a seller can actually reach are races the design predicted, so they get es and tl strings rather than server English. - The design record now states the two rules the implementation had to amend: the checks live in CheckCanConfirm so a missing reason is refused before the FIFO plan, and rule 5 also refuses a note with no code beside it.

coderabbitai · 2026-09-11T00:12:40Z

Important

🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 4737711b-482f-4d6a-8a30-8648c653d624

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

…on UI changes (#757) Two rule changes to `AGENTS.md`, both owner directives, 2026-09-11. ## 1. Removes the commit/push gate Drops `AGENTS.md:210`, "Only commit/push when the human asks". Agents may now commit, push a branch and open a PR without asking. The merge stays the owner's, and `main` is protected, so nothing reaches `main` unreviewed either way. **Why it goes rather than gets narrowed.** The rule carried no `→` decision-record link, so by this file's own preamble it was "a plain convention that has not yet cost anything" rather than an earned rule — no archaeology to follow before changing it. It was also ambiguous about where the gate actually sat: an agent working in an isolated worktree has to decide whether *every commit* waits or only the push, and the two readings differ a lot in practice. That ambiguity cost a round of clarification mid-way through #721. **What it changes for an agent.** Everything up to and including opening the PR becomes autonomous. Worth being explicit about the consequence, since it is the one thing the rule was implicitly buying: a PR opened without asking starts CI and spends a round of the review bot's included budget. That is the trade being accepted, not an oversight. No other file restates the gate — checked `CONTRIBUTING.md`, `docs/` and `.github/`. ## 2. Broadens the screenshot rule The #662 rule already required a 1:1 before/after comparison, but only for **a PR whose *purpose* is visual**. That trigger is too narrow: a PR whose purpose is a domain rule can still add a dialog, a field or a label, and those are exactly the changes a reader cannot check from the diff. It now reads: **any PR that changes what a user sees attaches screenshots**, before and after wherever a before exists, same viewport and same scenario in both, after-only where the UI is net-new. Two additions to the mechanics beside the existing ones (capture at 1:1, rebuild the stack at the head under review): - **Attach with `gh pr comment --attach`, never by committing images to a branch.** A branch is deletable, so routine cleanup silently breaks the images on a merged PR while the prose around them still claims they exist. - A **pointer from the Git / PR workflow section**, which is what people actually read while opening a PR. The mechanics stay stated once, under Operations and callers, so the two cannot drift. This PR changes no UI, so rule 2 does not apply to it. ## Related #756 is the slice that surfaced both: its own ambiguity about the commit gate, and a confirm dialog that the old screenshot trigger would not have required. --------- Co-authored-by: mforce <mforce@users.noreply.github.com>

mforce · 2026-09-11T00:24:43Z

Screenshots (#662)

Captured at 1:1 (1280×900, deviceScaleFactor: 1), same viewport and same
scenario in both, from a stack rebuilt at each side: origin/main for the
before, this PR's head 4728a62 for the after. Same draft order throughout —
SO-EC83FF53, 120 Large Eggs at $0.35 against a $0.45 list (below), plus 60
Medium Eggs at $0.38 against a $0.38 list (at list).

Before — `origin/main`

The order already marks the discount — struck list price, Below list chip,
tinted row, $12.00 · 22.2% in the Discount cell, and 15.6% · $12.00 in the
Orders list. All of that is #720/#723/#724. The dialog asks for nothing, so
the order confirms with no record of why it was discounted.

After — this PR

The FIFO prose is unchanged. Added above the picklist: what this order gives
away ($12.00 below list price, 15.6% of the order, across 1 of 2 lines) and
the line responsible. The person justifying the discount sees the size of it
first.

The reason is required, not advisory

Confirming with nothing selected keeps the dialog open and errors inline —
the same behaviour askReason already had for Void, so a missing answer never
costs what was already typed.

It survives the round trip

Discount reason: Long-standing customer on the order panel, and under the
discount badge in the Orders list.

The Orders list, across every state the Discount column has

The reason sits under the badge, on its own line. Worth reading the other rows
too, because they are the states this column has to tell apart: an order that is
partly unmeasurable (part of this order has no list price), one with
nothing to measure (Unknown, a pre-#720 order), and one with no lines at
all (an em dash). None of them reads as a measured zero, which is the failure
#719 exists to stop.

One deliberate detail: the reason renders outside the branch that computes
the badge (SalesPage.tsx:1669-1676). The badge is what a live measurement says
about the lines right now; the reason is a stored fact about the confirmation. So
an order whose lines later stop measuring as discounted still shows the reason it
was confirmed under.

An at-list order still asks for nothing

Not pictured because there is nothing to picture: with no below-list line the
dialog is byte-for-byte the "before" image above. That is acceptance criterion 2.

History — the confirm audit row (added in `c364f93`)

Four cases in one view, top to bottom:

22798d1a — confirmed at list, so no reason was asked for and none is
written. Em dash, not a broken or half-empty cell.
8bd1a707 — confirmed below list, reason and note both rendered:
Discount reason: Damaged stock (Hairline cracks found during grading).
ec83ff53 — this order has a discount reason on its columns, but its
confirm row predates the payload, so it shows an em dash. That is the
no-backfill behaviour stated honestly rather than papered over.
6679ca10 — a pre-Sales: require a discount reason when confirming a below-list order #721 confirm. Same em dash.

Row 3 is the one worth looking at: it is the difference between "we never
recorded this" and "we recorded it somewhere this view cannot reach", and the
screen does not pretend otherwise.

One thing to look at

In the after-dialog shot the focused Volume radio draws a square focus ring,
which at a glance reads as a checkbox next to four circles. It is a real focus
indicator and removing it would be worse, but if the square is not wanted it is
a CSS change, not a behaviour one.

History and the #745 Details column read audit EVENTS, never the aggregate, so the SalesOrders columns alone left #721's "survives to history" acceptance unmet. Not a second source of truth: the reason is write-once, set only by Confirm and never edited afterwards, and the audit row commits in the same transaction as the columns, so the two cannot disagree. Omitted entirely, never written as nulls, when the order gave nothing away. The AuditPage Details branch sits ABOVE the productName/currency guard on purpose: a confirm payload carries neither field, so a branch placed below that guard renders nothing at all — proved by moving it and watching the render test go red, then reverting. Updates the #721 design doc's non-goal: the owner reversed the earlier "no audit payload" decision.

…vent rows Each row now collapses to When (UTC)/Who/Action by default, with a per-row disclosure toggle (accessible name "Details", aria-expanded) revealing Entity and Details in place — a genuine expandable-row interaction rather than a cosmetic relabel, kept on real <table>/role="row" semantics (not a free-floating Accordion list) specifically to preserve the screen's dense existing invariant coverage (#493's stale-scope handling, #666's date-window validation, #679's filter/clear discipline) rather than rewriting it around a new locator strategy. Filters, actor, action, entity scope, UTC meaning (the "When (UTC)" header is unchanged), Details rendering (#745/#756/#758/#720's price-summary logic) and pagination are all untouched; still no mutation controls — the only buttons a read-only row carries are its own disclosure toggles, proven by a rewritten test that checks every button is a collapsed "Details" toggle, not just that none exist. Rows default COLLAPSED (not expanded) so the visual result actually matches #833's Concept C reference renders rather than looking unchanged — worth the larger test diff: ~20 existing assertions that read Entity or Details now call a new expandRow() helper first (mirroring AccountPage.test.tsx's expandChangePassword() pattern), and two columnheader-based tests were rewritten since Entity/Details are no longer table columns. New tests cover the toggle itself: collapsed by default, expands/collapses on click (mutation-checked), and each row toggles independently (mutation-checked by making the toggle a shared boolean and watching it fail before reverting) — plus that expanding a row never re-queries listAuditEvents.

mforce added 8 commits September 10, 2026 22:22

mforce mentioned this pull request Sep 11, 2026

Sales: require a discount reason when confirming a below-list order #721

Closed

mforce mentioned this pull request Sep 11, 2026

docs(agents): drop the commit and push gate, and require screenshots on UI changes #757

Merged

mforce merged commit c7864be into main Sep 11, 2026
11 checks passed

mforce deleted the feat/721-discount-reason branch September 11, 2026 01:37

This was referenced Sep 11, 2026

EPIC: Discount visibility and control on sales #719

Closed

web: long tl/es strings overflow fixed-width controls on narrow screens #740

Closed

This was referenced Oct 10, 2026

docs(plans): mark each planning record with its shipped status #1198

Merged

refactor(web): split the row renderer, detail panel and filter bar out of AuditPage #1242

Merged

Navigation Menu

Repository navigation

feat(sales): require a discount reason when confirming a below-list order - #756

feat(sales): require a discount reason when confirming a below-list order#756
mforce merged 9 commits into
mainfrom
feat/721-discount-reason

mforce commented Sep 11, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Sep 11, 2026 •

edited

Loading

Uh oh!

mforce commented Sep 11, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mforce commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What decides "discounted"

Two corrections the work made to its own design

#394 — the callers CI does not cover

Verification

Every acceptance criterion is met

Uh oh!

coderabbitai Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mforce commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots (#662)

Before — origin/main

After — this PR

The reason is required, not advisory

It survives the round trip

The Orders list, across every state the Discount column has

An at-list order still asks for nothing

History — the confirm audit row (added in c364f93)

One thing to look at

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mforce commented Sep 11, 2026 •

edited

Loading

coderabbitai Bot commented Sep 11, 2026 •

edited

Loading

mforce commented Sep 11, 2026 •

edited

Loading

Before — `origin/main`

History — the confirm audit row (added in `c364f93`)