Fixed discounted price formatting missing trailing zero#26774
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPrice formatting was centralized by adding 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/portal/src/components/pages/account-plan-page.js (1)
325-329: The fix correctly ensures two decimal places for fractional amounts.The conditional
toFixed(2)approach properly addresses the trailing zero issue. For $5.40, this produces "5.40" instead of "5.4".One optional simplification—the
Number()wrapper andMath.round()are redundant sincetoFixed(2)handles both conversion and rounding:♻️ Optional simplification
const originalAmount = price.amount / 100; -const originalPrice = formatNumber(originalAmount % 1 !== 0 ? Number(Math.round(originalAmount * 100) / 100).toFixed(2) : originalAmount); +const originalPrice = formatNumber(originalAmount % 1 !== 0 ? originalAmount.toFixed(2) : originalAmount); const currency = getCurrencySymbol(price.currency); const updatedAmount = getUpdatedOfferPrice({offer, price}); -const discountedPrice = formatNumber(updatedAmount % 1 !== 0 ? Number(Math.round(updatedAmount * 100) / 100).toFixed(2) : updatedAmount); +const discountedPrice = formatNumber(updatedAmount % 1 !== 0 ? updatedAmount.toFixed(2) : updatedAmount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/src/components/pages/account-plan-page.js` around lines 325 - 329, The conditional formatting for originalPrice and discountedPrice is fine but you can simplify by removing the redundant Number() and Math.round() wrappers; change the ternary branches to use originalAmount % 1 !== 0 ? originalAmount.toFixed(2) : originalAmount and updatedAmount % 1 !== 0 ? updatedAmount.toFixed(2) : updatedAmount respectively (keep formatNumber and the surrounding variables: originalAmount, updatedAmount, originalPrice, discountedPrice, getUpdatedOfferPrice).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/admin/app/components/gh-member-settings-form.hbs`:
- Around line 123-127: The template is passing already-dollar values
(nonDecimalAmount) into the gh-price-amount helper with cents=false which causes
incorrect division; update the three uses (sub.discountedPrice.nonDecimalAmount,
sub.originalPrice.nonDecimalAmount, sub.price.nonDecimalAmount) to provide
cent-based integers instead (e.g., sub.discountedPrice.amount,
sub.originalPrice.amount, sub.price.amount) or multiply the nonDecimalAmount by
100 before passing, so gh-price-amount receives cents units and renders
correctly.
---
Nitpick comments:
In `@apps/portal/src/components/pages/account-plan-page.js`:
- Around line 325-329: The conditional formatting for originalPrice and
discountedPrice is fine but you can simplify by removing the redundant Number()
and Math.round() wrappers; change the ternary branches to use originalAmount % 1
!== 0 ? originalAmount.toFixed(2) : originalAmount and updatedAmount % 1 !== 0 ?
updatedAmount.toFixed(2) : updatedAmount respectively (keep formatNumber and the
surrounding variables: originalAmount, updatedAmount, originalPrice,
discountedPrice, getUpdatedOfferPrice).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50167fba-a678-423a-9292-53fa2b731cf0
📒 Files selected for processing (2)
apps/portal/src/components/pages/account-plan-page.jsghost/admin/app/components/gh-member-settings-form.hbs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ghost/admin/app/components/gh-member-settings-form.hbs (1)
123-124:⚠️ Potential issue | 🔴 CriticalPass cent amounts here;
cents=falsemakes the discounted values wrong.
gh-price-amountstill divides by 100 whencents=false, so feeding itnonDecimalAmounthere turns values like5.40into0. Even swapping to.amountwithout removingcents=falsewould still round away the decimals, so this won't render the required trailing zeros. Use the cent-based fields and let the helper keep its default behavior.Suggested fix
- <span class="amount">{{gh-price-amount sub.discountedPrice.nonDecimalAmount cents=false}}</span> - <span class="gh-membertier-original-price">{{sub.originalPrice.currencySymbol}}{{gh-price-amount sub.originalPrice.nonDecimalAmount cents=false}}</span> + <span class="amount">{{gh-price-amount sub.discountedPrice.amount}}</span> + <span class="gh-membertier-original-price">{{sub.originalPrice.currencySymbol}}{{gh-price-amount sub.originalPrice.amount}}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/admin/app/components/gh-member-settings-form.hbs` around lines 123 - 124, The template is passing nonDecimalAmount and forcing cents=false which causes gh-price-amount to divide by 100 twice and lose decimals; change the usage in gh-member-settings-form.hbs to pass the cent-based fields (e.g., sub.discountedPrice.amount and sub.originalPrice.amount or the existing cent fields like sub.discountedPrice.cents) and remove the cents=false attribute so gh-price-amount uses its default cent-handling behavior; update the two spans that reference gh-price-amount (the one showing discountedPrice and the one showing originalPrice) to use the cent fields and no cents flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ghost/admin/app/components/gh-member-settings-form.hbs`:
- Around line 123-124: The template is passing nonDecimalAmount and forcing
cents=false which causes gh-price-amount to divide by 100 twice and lose
decimals; change the usage in gh-member-settings-form.hbs to pass the cent-based
fields (e.g., sub.discountedPrice.amount and sub.originalPrice.amount or the
existing cent fields like sub.discountedPrice.cents) and remove the cents=false
attribute so gh-price-amount uses its default cent-handling behavior; update the
two spans that reference gh-price-amount (the one showing discountedPrice and
the one showing originalPrice) to use the cent fields and no cents flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e73c585-8227-4487-b150-f3ab7a88d606
📒 Files selected for processing (1)
ghost/admin/app/components/gh-member-settings-form.hbs
🤖 Velo CI Failure AnalysisClassification: 🟠 SOFT FAIL
|
There was a problem hiding this comment.
@minimaluminium left a suggestion to extract the new formatting logic to a helper
Moreover, in Admin > Member page, I'm still seeing a single decimal place (missing trailing 0):
| const price = getPriceFromSubscription({subscription}); | ||
| const originalPrice = formatNumber(price.amount / 100); | ||
| const originalAmount = price.amount / 100; | ||
| const originalPrice = formatNumber(originalAmount % 1 !== 0 ? Number(Math.round(originalAmount * 100) / 100).toFixed(2) : originalAmount); |
There was a problem hiding this comment.
suggestion: shall we move that formatting logic into the original formatNumber() helper? Or if this helper is used to display non-prices, we could have a formatPrice() helper?
That way, we can also add unit tests for the helper, such as "formats decimal prices correctly"
There was a problem hiding this comment.
Additional review points from Codex, look valid:
[P2] The admin member detail page still leaves the original bug in place for non-discounted prices. The discounted branch now uses gh-price-amount, but the non-discounted branch still renders sub.price.nonDecimalAmount with format-number, so a plan like 590 cents will still show 5.9 instead of 5.90. See gh-member-settings-form.hbs (line 121) and subscription-data.js (line 17)
[P2] The portal fix preserves trailing zeros by converting decimals to strings before calling formatNumber, but that also disables locale number formatting for larger values. formatNumber just does amount.toLocaleString(), so "1234.50" stays "1234.50" instead of "1,234.50". See account-plan-page.js (line 324) and helpers.js (line 756)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/portal/test/utils/helpers.test.js (1)
300-303: Consider adding a non-numeric input case for completeness.
formatPriceguards against invalid numeric input; adding one assertion (e.g.'abc'orNaN) would lock that behavior down.Optional test addition
test('returns empty string for null/undefined input', () => { expect(formatPrice(null)).toBe(''); expect(formatPrice(undefined)).toBe(''); }); + + test('returns empty string for non-numeric input', () => { + expect(formatPrice('abc')).toBe(''); + expect(formatPrice(NaN)).toBe(''); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/test/utils/helpers.test.js` around lines 300 - 303, Extend the existing test for formatPrice (in the test named "returns empty string for null/undefined input") to also assert behavior for non-numeric input: call formatPrice with a non-number value such as 'abc' and/or NaN and expect an empty string; this ensures formatPrice('abc') and formatPrice(NaN) return '' like the null/undefined cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/portal/test/utils/helpers.test.js`:
- Around line 300-303: Extend the existing test for formatPrice (in the test
named "returns empty string for null/undefined input") to also assert behavior
for non-numeric input: call formatPrice with a non-number value such as 'abc'
and/or NaN and expect an empty string; this ensures formatPrice('abc') and
formatPrice(NaN) return '' like the null/undefined cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5076c802-4e6c-4ca9-b2f1-a43690c90f91
📒 Files selected for processing (6)
apps/portal/src/components/pages/account-plan-page.jsapps/portal/src/utils/helpers.jsapps/portal/test/unit/components/pages/account-plan-page.test.jsapps/portal/test/utils/helpers.test.jsghost/admin/app/components/gh-member-settings-form.hbsghost/admin/tests/acceptance/members/details-test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/portal/src/components/pages/account-plan-page.js
- ghost/admin/app/components/gh-member-settings-form.hbs
minimaluminium
force-pushed
the
offer-price-format-BER-3427
branch
from
c3c2bac to
7114fb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/portal/test/utils/helpers.test.js`:
- Around line 296-298: The test is fragile because formatPrice uses
toLocaleString(undefined, options) which depends on system locale; update the
implementation of formatPrice to pass an explicit locale (e.g., 'en-US') to
toLocaleString so it always formats as '1,234.50', and run/update the test in
apps/portal/test/utils/helpers.test.js if needed; locate the function named
formatPrice in helpers.js and replace toLocaleString(undefined, options) with
toLocaleString('en-US', options) (or accept an optional locale parameter and
default it to 'en-US') to ensure deterministic behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24cc4f7a-525e-4c14-8732-975a72eb7987
📒 Files selected for processing (4)
apps/portal/src/components/pages/account-plan-page.jsapps/portal/src/utils/helpers.jsapps/portal/test/unit/components/pages/account-plan-page.test.jsapps/portal/test/utils/helpers.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/portal/src/components/pages/account-plan-page.js
- apps/portal/src/utils/helpers.js
sagzy
left a comment
sagzy
left a comment
There was a problem hiding this comment.
Besides the comment above on forcing en-US locale, I think it'd be good to also apply the new helper for fixed amounts here: (
Ghost/apps/portal/src/utils/helpers.js
Line 825 in ce4d993
${getCurrencySymbol(offer.currency)}${formatPrice(offer.amount / 100)}
We don't use fixed-amount offers for retention, but good to fix for any future use of that helper
apps/portal/src/utils/helpers.js
Outdated
| ? undefined | ||
| : {minimumFractionDigits: 2, maximumFractionDigits: 2}; | ||
|
|
||
| return normalizedAmount.toLocaleString('en-US', options); |
There was a problem hiding this comment.
I'm unsure about forcing en-US formatting here instead of keeping the user locale.
This would mean that prices are printed with 1,234.50 format for all countries, whereas I believe currently we render:
- "1,234.50" for en-US (comma-separated thousands)
- but e.g. "1 234,50" for fr-FR (space-separated thousands)
sagzy
force-pushed
the
offer-price-format-BER-3427
branch
from
71fbee0 to
47c6f0c
Compare
gh-price-amount with cents=false was dividing the already-converted nonDecimalAmount by 100 again. Pass the raw cent amount instead so the helper formats correctly.
sagzy
force-pushed
the
offer-price-format-BER-3427
branch
from
47c6f0c to
0459d6a
Compare
3 participants
ref https://linear.app/ghost/issue/BER-3427/fix-price-formatting-issue
Non-whole discounted prices (e.g. $5.40) were displayed without trailing zeros (e.g. $5.4) in two places:
formatNumber()which calls.toLocaleString()without enforcing decimal places. Fixed by applying the same.toFixed(2)rounding that the signup offer page already uses inrenderRoundedPrice().{{format-number}}which doesn't enforce decimal places. Fixed by switching to{{gh-price-amount}} (with cents=false), which already handles this correctly via{minimumFractionDigits: 2}.