fix(dashboard): #327 removed the default currency, twelve call sites still invent one - #329
Conversation
…all sites still invent one TelivityAI#327 deleted DEFAULT_CURRENCY from money.ts so an absent currency renders plainly instead of as dollars. The formatter is now honest, but twelve call sites in the same app still substitute 'USD' when a record has no code, and seven of those substitutions are sent to the API and written to a ledger. The sharpest ones are guards that CANNOT FIRE: const currencyCode = folio?.currencyCode ?? 'USD'; // Folios.tsx:500 ... requireCurrency(currencyCode); // :514, :535, :554 requireCurrency can never throw there, because the value it checks was made non-null one line above. Three mutations read as guarded and are not, which is worse than no guard: a reviewer sees the call and stops looking. HouseAccounts.tsx:324 has the identical shape in front of its charge and payment mutations. WRITES THAT SENT AN INVENTED CURRENCY Folios.tsx:252 split folio create -- no guard of any kind FrontDesk.tsx:489 walk-in reservation FrontDesk.tsx:542 reservation split Accounting.tsx:285 A/R payment (the case the requireCurrency docblock names: a ledger in USD against a yen property) Reservations.tsx:96 CSV import, per row ReservationPartyPanel currencyCode PROP DEFAULTED to 'USD', so the caller's own ?? 'USD' was the second of two fallbacks HouseAccounts.tsx:68 house-account create was unguarded while createProduct twelve lines below was guarded While wiring the panel prop I found a third instance of the same bug: GuestDetailsModal already receives currencyCode: string | null and renders money with it, but never passed it to ReservationPartyPanel -- so a split started from the front-desk guest modal took the panel's 'USD' default. Now passed through. WHAT EACH CHANGE DOES Writes: drop the ?? 'USD' and guard with requireCurrency, so a missing code fails loudly at the call site instead of quietly becoming dollars. The two defanged derivations become ?? null. Both values are also handed to formatMoney, which TelivityAI#327 already taught to render a null currency plainly, so display stays correct and the guards can now actually fire. CSV import throws naming the row number rather than importing it as USD. A missing column is not a dollar booking, it is a row we cannot price. RatePlans.tsx:544 displayed 'USD' for a plan carrying no currency -- a wrong fact on screen. Shows an em dash, matching every other absent field beside it. NOT INCLUDED, deliberately: Settings.tsx:76/106, which seeds its form state to 'USD'. That is the screen whose job is to SET the property currency, so a default there is a different question -- though note :106 means loading a property with no currency and saving any other field on the form stamps it USD. Happy to follow up if you want that changed too. No behaviour change for any property that has a currency set, which is all of them created through the UI. The change is entirely in what happens when one is missing: it now stops instead of guessing.
telivity-otaip
left a comment
There was a problem hiding this comment.
Review
Direction: yes. Real follow-up to #327 — the places that still invented USD on write (folios split, walk-in, reservation split, A/R payment, CSV import, house-account create, party-panel default) were defanging requireCurrency. Guarding those mutations so a missing code fails loudly is the right product behavior for non-USD properties.
Also good: threading currencyCode from GuestDetailsModal into ReservationPartyPanel (that was a second silent USD path), RatePlans em-dash for absent currency, CSV import throwing with a row number.
Deliberate leave-out of Settings.tsx form seed — agreed for this PR; noted follow-up is fair (property.currencyCode ?? … ?? 'USD' on load can stamp USD on save).
Blocker
All five CI jobs fail on one type error in ReservationPartyPanel.tsx:
requireCurrency(currencyCode) where prop is string | null | undefined.
Fix: requireCurrency(currencyCode ?? null).
After that lands and CI is green, this is merge-ready from my side. Happy to push the one-liner onto the fork if you want.
| const splitMutation = useMutation({ | ||
| mutationFn: () => { | ||
| requirePropertyId(propertyId); | ||
| requireCurrency(currencyCode); |
There was a problem hiding this comment.
CI blocker (TS2345): currencyCode is typed string | null | undefined (optional prop), but requireCurrency only accepts string | null.
Same pattern as the Folios/HouseAccounts fixes in this PR:
requireCurrency(currencyCode ?? null);That one-liner should green the typecheck/build/smoke jobs — they all fail on this same error today.
Co-authored-by: Charles Pizzato <311327716+modernitconsultants@users.noreply.github.com>
|
Hey @modernitconsultants — sorry about the earlier AI review noise on this. That was us being too pedantic on a clear, useful fix. Your change is a good add and we want it in. We pushed a one-line type coercion ( Thank you for running HAIP in production. We would genuinely love to hear your impressions — what works, what gets in the way, what you wish existed. Also: if you need help with fiscalization, or Japanese-market translations / localization, say the word — happy to prioritize that with you. |
telivity-otaip
left a comment
There was a problem hiding this comment.
LGTM — good follow-up to #327. Tiny type fix applied so checks can pass; merging once CI is green.
The problem
#327 removed
DEFAULT_CURRENCYfrommoney.ts, so an absent currency now renders plainly instead of as dollars. The formatter is honest — but twelve call sites in the same app still substitute'USD'when a record has no code, and seven of those are sent to the API and written to a ledger.The sharpest case: guards that cannot fire
requireCurrencycan never throw there — the value it checks was made non-null one line above. Three mutations read as guarded and are not, which is worse than no guard at all: a reviewer sees the call and stops looking.HouseAccounts.tsx:324has the identical shape in front of its charge and payment mutations.Writes that sent an invented currency
Folios.tsx:252FrontDesk.tsx:489FrontDesk.tsx:542Accounting.tsx:285requireCurrencydocblock namesReservations.tsx:96ReservationPartyPanelcurrencyCodeprop defaulted to'USD', so the caller's own?? 'USD'was the second of two fallbacksHouseAccounts.tsx:68createProducttwelve lines below is guardedWhile wiring the panel prop I found a third instance:
GuestDetailsModalalready receivescurrencyCode: string | nulland renders money with it, but never passed it toReservationPartyPanel— so a split started from the front-desk guest modal took the panel's'USD'default. Now passed through.What each change does
?? 'USD', guard withrequireCurrency. A missing code fails loudly at the call site instead of quietly becoming dollars.?? null. Both values also go toformatMoney, which fix(dashboard): there is no default currency #327 already taught to render a null currency plainly — so display stays correct and the guards can now actually fire.RatePlans.tsx:544displayed'USD'for a plan carrying no currency — a wrong fact on screen. Now an em dash, matching every other absent field beside it.Not included, deliberately
Settings.tsx:76/:106seed form state to'USD'. That's the screen whose job is to set the property currency, so a default there is a different question — though note:106means loading a property with no currency and saving any other field stamps it USD. Happy to follow up if you'd like that changed too.Risk
No behaviour change for any property that has a currency set, which is every property created through the UI. The change is entirely in what happens when one is missing: it stops instead of guessing.
Context, since it's relevant to why we keep finding these: we run HAIP for a 9-room lodge trading in JPY, so a fabricated
USDisn't hypothetical here — it's a wrong number on a real ledger. Same motivation as #301 and #327.