Skip to content

Commit d2e2caf

Browse files
yinlianghuiclaude
andauthored
fix(fields): formatCurrency keeps both cents digits on a fractional amount (#4332) (#4362)
The symbol branch passed `minimumFractionDigits: 0` against a wholeness-switched `maximumFractionDigits`, handing Intl the RANGE [0, 2]. Intl emits the shortest representation in range, so a genuine cents value of `.50` printed as `.5`: `$1,234.5`, `$19.9`, `$0.5` — money on a record page and in grid cells reading as a data error rather than a formatting one, and the opposite of what the function's own doc comment promises. Both bounds now take the same wholeness-switched width. That also makes the function's three branches agree: the no-currency branch already routed through `formatNumber` (both bounds = the width it is given) and the bad-currency fallback already used `toFixed`, so both already rendered `1,234.50` for the same amount. Only the symbol branch disagreed. The whole-number half of the switch is unchanged — `$1,234` still drops its `.00`, as the doc comment promises just as explicitly and as #4033 pinned through the renderer. Flips the control #4033 deliberately left pinned on the wrong value with a comment pointing at this card, and adds branch-level pins for the fix and for every branch it must not disturb. Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 405e808 commit d2e2caf

4 files changed

Lines changed: 207 additions & 14 deletions

File tree

‎.changeset/curvy-donkeys-shave.md‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
'@object-ui/fields': patch
3+
---
4+
5+
fix(fields): `formatCurrency` keeps both cents digits on a fractional amount
6+
7+
The symbol branch passed `minimumFractionDigits: 0` against a
8+
wholeness-switched `maximumFractionDigits`, which handed `Intl` the range
9+
`[0, 2]` — and `Intl` emits the shortest representation in range, so a real
10+
cents value of `.50` was printed as `.5`. Any price ending in a zero cent digit
11+
rendered one digit short: `$1,234.50` as `$1,234.5`, `$19.90` as `$19.9`,
12+
`$0.50` as `$0.5` — money on a record page and in grid cells reading as a data
13+
error rather than a formatting one.
14+
15+
Both bounds now take the same wholeness-switched width, so the function
16+
delivers the contract its own doc comment states: a fractional amount shows
17+
exactly two digits, a whole amount still drops `.00` (`$1,234`). The
18+
no-currency branch and the bad-currency fallback already behaved this way; only
19+
the symbol branch disagreed.
20+
21+
Reaches every consumer of the shared helper: `CurrencyCellRenderer`,
22+
`ObjectGrid`, the dashboard `recordFields` and `ObjectGantt`.

‎packages/fields/src/__tests__/NumberCellRenderer.grouping.test.tsx‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -122,14 +122,15 @@ describe('CurrencyCellRenderer — CONTROL, unaffected by the ordinal default',
122122

123123
it('keeps grouping and the fractional part when the amount is not whole', () => {
124124
renderCurrency(1234.5, { currency: 'USD' }, 'en-US');
125-
// Pins CURRENT behaviour, which is `$1,234.5` — NOT the `$1,234.50` that
126-
// `formatCurrency`'s own doc comment promises ("Salesforce convention:
127-
// `$1,234.50` keeps cents"). The construction passes
128-
// `minimumFractionDigits: 0`, so a trailing zero is dropped from a real
129-
// cents value. Pre-existing and out of this card's scope (it is neither a
130-
// locale nor a grouping defect) — filed separately; this control exists to
131-
// prove #4033 does not disturb it.
132-
expect(screen.getByText('$1,234.5')).toBeInTheDocument();
125+
// This control pinned the CURRENT behaviour when #4033 landed — `$1,234.5`,
126+
// NOT the `$1,234.50` that `formatCurrency`'s own doc comment promises
127+
// ("Salesforce convention: `$1,234.50` keeps cents"). That was a real
128+
// defect, deliberately filed rather than folded in (objectui#4332) and
129+
// pinned here so its fix would flip a watched test instead of silently
130+
// changing an unwatched surface. #4332 is now fixed, so the expectation
131+
// moves to the promised form; grouping — this control's actual subject —
132+
// is unchanged, which is still what #4033 needs it to prove.
133+
expect(screen.getByText('$1,234.50')).toBeInTheDocument();
133134
});
134135

135136
it('follows the active locale for currency too', () => {
Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* objectui#4332 — `formatCurrency` dropped a real cents digit.
11+
*
12+
* The symbol branch passed `minimumFractionDigits: 0` against a
13+
* wholeness-switched `maximumFractionDigits`, handing Intl the RANGE `[0, 2]`.
14+
* Intl then emits the shortest representation in range, so a genuine cents
15+
* value of `.50` printed as `.5`: `$1,234.5`, `$19.9`, `$0.5` — money on a
16+
* record page reading as a data error rather than a formatting one.
17+
*
18+
* The fix is one width for both bounds (`isWhole ? 0 : 2`), which also makes
19+
* the function's three branches agree: the no-currency branch already routed
20+
* through `formatNumber` (both bounds = the width), and the bad-currency
21+
* fallback already used `toFixed`. Only the symbol branch disagreed.
22+
*
23+
* What is pinned here, and why each half matters:
24+
*
25+
* 1. **Fractional amounts keep both cents digits** — the defect.
26+
* 2. **Whole amounts still drop `.00`** — the OTHER half of the wholeness
27+
* switch, which the doc comment promises just as explicitly ("Salesforce
28+
* convention: `$1,234.50` keeps cents; `$1,234` does not") and which
29+
* objectui#4033 independently pinned through the renderer. Widening the
30+
* minimum to a constant 2 would have "fixed" this card by breaking that
31+
* one, so it is a control, not a decoration.
32+
* 3. **The branches that were already correct are unchanged** — no-currency,
33+
* bad-currency fallback, and `formatCompactCurrency` (a different function
34+
* with its own `maximumFractionDigits: 1` compact policy).
35+
*/
36+
37+
import { describe, it, expect } from 'vitest';
38+
import React from 'react';
39+
import { render, screen } from '@testing-library/react';
40+
import '@testing-library/jest-dom';
41+
import { LocalizationProvider } from '@object-ui/i18n';
42+
import {
43+
formatCurrency,
44+
formatCompactCurrency,
45+
formatNumber,
46+
CurrencyCellRenderer,
47+
} from '../index';
48+
49+
describe('formatCurrency — a fractional amount keeps both cents digits (objectui#4332)', () => {
50+
it("renders the card's fixture as the doc comment promises", () => {
51+
expect(formatCurrency(1234.5, 'USD', 'en-US')).toBe('$1,234.50');
52+
});
53+
54+
it.each([
55+
[19.9, '$19.90'],
56+
[0.5, '$0.50'],
57+
[1234.5, '$1,234.50'],
58+
])('an amount ending in a zero cent digit keeps it: %s', (value, expected) => {
59+
// The issue's impact list — every price whose cents end in 0 was truncated
60+
// by one digit (`$19.9`, `$0.5`).
61+
expect(formatCurrency(value as number, 'USD', 'en-US')).toBe(expected as string);
62+
});
63+
64+
it('keeps the sign in front of a truncated-then-restored amount', () => {
65+
expect(formatCurrency(-1234.5, 'USD', 'en-US')).toBe('-$1,234.50');
66+
});
67+
68+
it('a fractional amount that ROUNDS to zero cents still shows them', () => {
69+
// Sub-cent precision: `1234.001` is not whole, so it keeps cents and
70+
// rounds into them. Before the fix the `[0, 2]` range rounded to `.00` and
71+
// then trimmed it away, printing a bare `$1,234` for a value that is NOT
72+
// the whole number that spelling claims. Deliberate: wholeness — not the
73+
// rounded output — decides whether cents are shown.
74+
expect(formatCurrency(1234.001, 'USD', 'en-US')).toBe('$1,234.00');
75+
});
76+
77+
it('still caps at two digits (the maximum is unchanged)', () => {
78+
expect(formatCurrency(1234.567, 'USD', 'en-US')).toBe('$1,234.57');
79+
expect(formatCurrency(1234.56, 'USD', 'en-US')).toBe('$1,234.56');
80+
});
81+
82+
it('follows the active locale while doing it (objectui#4033 is not disturbed)', () => {
83+
// de-DE: dot grouping, comma decimal mark, TRAILING symbol separated by
84+
// U+00A0. Matched loosely on the separator (written as an escape, never a
85+
// raw byte) so an ICU spacing change cannot fail the digits this pins.
86+
expect(formatCurrency(1234.5, 'EUR', 'de-DE')).toMatch(/^1\.234,50\s*€$/);
87+
});
88+
});
89+
90+
describe('formatCurrency — CONTROL: the whole-number half of the switch is untouched', () => {
91+
it.each([
92+
[1234, '$1,234'],
93+
[5000000, '$5,000,000'],
94+
[0, '$0'],
95+
])('a whole amount still drops `.00`: %s', (value, expected) => {
96+
// The doc comment promises this as explicitly as it promises the cents,
97+
// and objectui#4033 pinned it through `CurrencyCellRenderer`. A constant
98+
// `minimumFractionDigits: 2` would turn all three of these into `.00`.
99+
expect(formatCurrency(value as number, 'USD', 'en-US')).toBe(expected as string);
100+
});
101+
102+
it('a non-finite amount is unaffected (it is not "whole")', () => {
103+
expect(formatCurrency(Number.NaN, 'USD', 'en-US')).toBe('$NaN');
104+
});
105+
});
106+
107+
describe('formatCurrency — CONTROL: the branches that were already correct', () => {
108+
it('the no-currency branch is unchanged — it never had the defect', () => {
109+
// It routes through `formatNumber`, which sets BOTH bounds to the width it
110+
// is given, so it already rendered `1,234.50` while the symbol branch
111+
// rendered `$1,234.5`. That disagreement is what the fix removes.
112+
expect(formatCurrency(1234.5, undefined, 'en-US')).toBe('1,234.50');
113+
expect(formatCurrency(1234.5, undefined, 'en-US')).toBe(formatNumber(1234.5, 2, 'en-US'));
114+
// A whole amount with no currency is still money and keeps its separators
115+
// (objectui#4033: `formatNumber` passes no `scale`, so the ordinal
116+
// no-grouping policy cannot fire here).
117+
expect(formatCurrency(5000000, undefined, 'en-US')).toBe('5,000,000');
118+
});
119+
120+
it('the bad-currency fallback is unchanged — it always used toFixed', () => {
121+
// `Intl` throws `RangeError: Invalid currency code` for a non-ISO-4217
122+
// code, and `formatDisplayNumber`'s locale retry re-throws it rather than
123+
// swallowing it, so this lands in `formatCurrency`'s own catch.
124+
expect(formatCurrency(1234.5, 'NOT_A_CODE', 'en-US')).toBe('NOT_A_CODE 1234.50');
125+
});
126+
127+
it('formatCompactCurrency is a different function and keeps its own policy', () => {
128+
// Compact notation caps at ONE fraction digit and strips a trailing `.0`;
129+
// it never had the `[0, max]` range, and this card does not touch it.
130+
expect(formatCompactCurrency(1234.5, 'USD', 'en-US')).toBe('$1.2K');
131+
expect(formatCompactCurrency(150000, 'USD', 'en-US')).toBe('$150K');
132+
});
133+
});
134+
135+
describe('CurrencyCellRenderer — the user-visible surface (objectui#4332)', () => {
136+
const renderCurrency = (value: unknown, field: Record<string, unknown>, locale?: string) =>
137+
render(
138+
<LocalizationProvider value={{ locale }}>
139+
<CurrencyCellRenderer value={value as any} field={{ type: 'currency', ...field } as any} />
140+
</LocalizationProvider>,
141+
);
142+
143+
it('a grid cell shows the cents digit the amount actually has', () => {
144+
renderCurrency(1234.5, { currency: 'USD' }, 'en-US');
145+
expect(screen.getByText('$1,234.50')).toBeInTheDocument();
146+
});
147+
148+
it('CONTROL: a declared `scale` is inert on the currency cell path', () => {
149+
// Measured, not assumed: `CurrencyCellRenderer` reads `currency` and never
150+
// `scale`, and `formatCurrency` takes no scale argument — on this path
151+
// `scale` is neither a display width nor (money always groups) a grouping
152+
// input. So a currency field declaring `scale: 0` renders exactly like one
153+
// declaring nothing, before and after this card. Pinned so that a future
154+
// change making `scale` load-bearing here has to say so out loud.
155+
renderCurrency(1234.5, { currency: 'USD', scale: 0 }, 'en-US');
156+
expect(screen.getByText('$1,234.50')).toBeInTheDocument();
157+
});
158+
});

‎packages/fields/src/index.tsx‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -358,26 +358,38 @@ export function coerceToSafeValue(value: unknown): string | number | boolean | n
358358
* RMB amount showing as dollars?" bug reports.
359359
*
360360
* Trailing `.00` is dropped when the value is a whole number — Salesforce
361-
* convention: `$1,234.50` keeps cents; `$1,234` does not.
361+
* convention: `$1,234.50` keeps cents; `$1,234` does not. Wholeness picks ONE
362+
* fraction-digit width, never a range: a whole amount shows 0 digits, and a
363+
* fractional amount shows exactly 2 — so a real cents value of `.50` renders
364+
* `.50`, not `.5`.
362365
*/
363366
import { resolveFieldCurrency } from './currency';
364367
export { resolveFieldCurrency };
365368

366369
export function formatCurrency(value: number, currency?: string, locale?: string): string {
367370
const isWhole = Number.isFinite(value) && value === Math.trunc(value);
368-
const maxFrac = isWhole ? 0 : 2;
371+
// ONE width for both bounds, not a range (objectui#4332). The symbol branch
372+
// used to pass `minimumFractionDigits: 0` against a wholeness-switched
373+
// maximum, which handed Intl the range [0, 2] — and Intl then emits the
374+
// SHORTEST representation in range, so a genuine cents value of `.50` was
375+
// printed as `.5`: `$1,234.5` instead of the `$1,234.50` promised above.
376+
// It was the only branch that did: the no-currency branch reaches
377+
// `formatNumber`, which sets both bounds to the width it is given, and the
378+
// bad-currency fallback below has always used `toFixed`. Both already
379+
// rendered `1,234.50` for the same amount.
380+
const fracDigits = isWhole ? 0 : 2;
369381
if (!currency) {
370-
return formatNumber(value, maxFrac, locale);
382+
return formatNumber(value, fracDigits, locale);
371383
}
372384
try {
373385
return formatDisplayNumber(value, {
374386
locale,
375387
currency,
376-
minimumFractionDigits: 0,
377-
maximumFractionDigits: maxFrac,
388+
minimumFractionDigits: fracDigits,
389+
maximumFractionDigits: fracDigits,
378390
});
379391
} catch {
380-
return `${currency} ${value.toFixed(maxFrac)}`;
392+
return `${currency} ${value.toFixed(fracDigits)}`;
381393
}
382394
}
383395

0 commit comments

Comments
 (0)