✨ Add Japanese Yen (JPY) currency support #23151
Conversation
This commit introduces support for the Japanese Yen (JPY) currency. The following changes were made: - Added JPY to the list of supported currencies in `apps/admin-x-settings/src/utils/currency.ts` and `ghost/admin/app/utils/currency.js`. - Updated `currencyToDecimal` and `currencyFromDecimal` functions to handle zero-decimal currencies correctly. - Updated `getNonDecimal` function to handle zero-decimal currencies correctly. - Updated `getSubscriptionData` function to use the updated `getNonDecimal` function. - Updated `PaymentsService` to handle zero-decimal currencies correctly when creating and retrieving prices from Stripe. - Added JPY to the milestones test data in `ghost/core/test/unit/server/services/milestones/ MilestonesService.test.js`. These changes ensure that the application correctly handles JPY and other zero-decimal currencies, improving accuracy and functionality.
WalkthroughThe changes implement support for zero-decimal currencies across multiple modules by introducing a constant array of zero-decimal currency codes and a helper function 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/admin-x-settings/src/utils/currency.ts (1)
219-242: 💡 Verification agent🧩 Analysis chain
Verify validation function compatibility with zero-decimal currencies
The
validateCurrencyAmountfunction multiplies minimum and maximum amounts by 100 during validation, which assumes currencies use minor units. While JPY has a specific minimum amount defined (100), you should verify this function works correctly for all zero-decimal currencies.
🏁 Script executed:
#!/bin/bash # Check if validateCurrencyAmount would work correctly for zero-decimal currencies # First, let's show the implementation of validateCurrencyAmount echo "Checking validateCurrencyAmount implementation..." cat "apps/admin-x-settings/src/utils/currency.ts" | grep -A 25 "export function validateCurrencyAmount" # Look for any usage of validateCurrencyAmount with zero-decimal currencies echo -e "\nChecking for usages of validateCurrencyAmount with zero-decimal currencies..." rg "validateCurrencyAmount.*JPY|validateCurrencyAmount.*currency.*JPY|validateCurrencyAmount.*zeroDecimal" -A 5 -B 5Length of output: 1252
🏁 Script executed:
#!/bin/bash # Inspect the implementation of minimumAmountForCurrency echo "=== minimumAmountForCurrency implementation ===" rg -n -C5 "function minimumAmountForCurrency" apps/admin-x-settings/src/utils/currency.ts # Search for currency metadata (decimals / zeroDecimal flags) echo -e "\n=== Currency metadata (decimals / zeroDecimal) ===" rg -n "decimals" -C3 apps/admin-x-settings/src/utils rg -n "zeroDecimal" -C3 . # Verify getSymbol currency lookup mapping as well echo -e "\n=== getSymbol implementation ===" rg -n -C5 "function getSymbol" apps/admin-x-settings/src/utils/currency.tsLength of output: 4729
Update validateCurrencyAmount to handle zero-decimal currencies correctly
The current implementation always multiplies the major-unit minimum/maximum by 100, which only works for currencies with two minor-unit decimals. Zero-decimal currencies (e.g. JPY, KRW, VND) use a factor of 1.
In apps/admin-x-settings/src/utils/currency.ts →
validateCurrencyAmount:• Import and use
isZeroDecimalCurrencyto derive the minor-unit factor:import {isZeroDecimalCurrency} from './currency'; const unit = isZeroDecimalCurrency(currency) ? 1 : 100;• Replace hard-coded
* 100with* unit:- if (cents !== 0 && cents < (minAmount * 100)) { - return `Non-zero amount must be at least ${symbol}${minAmount}.`; - } + if (cents !== 0 && cents < (minAmount * unit)) { + return `Non-zero amount must be at least ${symbol}${minAmount}.`; + } - if (maxAmount && cents !== 0 && cents > (maxAmount * 100)) { - return `Suggested amount cannot be more than ${symbol}${maxAmount}.`; - } + if (maxAmount && cents !== 0 && cents > (maxAmount * unit)) { + return `Suggested amount cannot be more than ${symbol}${maxAmount}.`; + }This ensures the correct threshold for both two-decimal and zero-decimal currencies.
🧹 Nitpick comments (3)
ghost/core/test/unit/server/services/milestones/MilestonesService.test.js (1)
82-97: Proper test case for JPY milestone creation, but fix coding style.The new test thoroughly verifies that JPY milestones are correctly created, which is crucial for ensuring the feature works as expected.
Fix the brace style to match project conventions:
async getARR() { return [{currency: 'jpy', arr: 750}]; }, - async hasImportedMembersInPeriod() { return false; }, - async getDefaultCurrency() { return 'jpy'; } + async hasImportedMembersInPeriod() { + return false; + }, + async getDefaultCurrency() { + return 'jpy'; + }🧰 Tools
🪛 ESLint
[error] 88-88: Statement inside of curly braces should be on next line.
(brace-style)
[error] 88-88: Closing curly brace should be on the same line as opening curly brace or on the line after the previous block.
(brace-style)
[error] 89-89: Statement inside of curly braces should be on next line.
(brace-style)
[error] 89-89: Closing curly brace should be on the same line as opening curly brace or on the line after the previous block.
(brace-style)
[error] 90-90: Statement inside of curly braces should be on next line.
(brace-style)
[error] 90-90: Closing curly brace should be on the same line as opening curly brace or on the line after the previous block.
(brace-style)
ghost/core/core/server/services/members/members-api/services/PaymentsService.js (2)
442-449: Correct handling of zero-decimal currencies in Stripe price comparison.This properly compares Stripe's unit_amount with the expected amount based on whether the currency is zero-decimal or not. For zero-decimal currencies like JPY, it correctly divides by 100 to account for how Ghost stores prices internally versus how Stripe expects them.
Add a clarifying comment about why we're dividing by 100 for zero-decimal currencies:
// For comparison with Stripe's unit_amount, we need to handle zero-decimal currencies // For zero-decimal currencies, Stripe's unit_amount will be the same as our tierAmount // For other currencies, Stripe's unit_amount will be 100x our tierAmount + // We divide by 100 for zero-decimal currencies because our amounts are stored in cents internally, + // but Stripe expects them in the base currency units (e.g., 1 yen instead of 100 cents) const expectedUnitAmount = isZeroDecimalCurrency(tier.currency) ? tierAmount / 100 : tierAmount;
484-486: Proper handling of zero-decimal currencies when creating Stripe prices.This correctly formats the amount for Stripe based on whether the currency is zero-decimal or not, ensuring that prices are created with the correct amounts in Stripe.
Similar to above, add a clarifying comment about the division by 100:
// For zero-decimal currencies like JPY, we don't need to multiply by 100 // For other currencies, Stripe expects the amount in cents (smallest currency unit) + // We divide by 100 for zero-decimal currencies because our amounts are stored in cents internally, + // but for currencies like JPY, Stripe expects amounts in whole yen, not "cents of yen" const amount = isZeroDecimalCurrency(tier.currency) ? priceAmount / 100 : priceAmount;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/admin-x-settings/src/utils/currency.ts(2 hunks)ghost/admin/app/utils/currency.js(2 hunks)ghost/admin/app/utils/subscription-data.js(1 hunks)ghost/core/core/server/services/members/members-api/services/PaymentsService.js(4 hunks)ghost/core/test/unit/server/services/milestones/MilestonesService.test.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
ghost/admin/app/utils/subscription-data.js (1)
ghost/admin/app/utils/currency.js (1)
getNonDecimal(138-143)
apps/admin-x-settings/src/utils/currency.ts (2)
ghost/admin/app/utils/currency.js (2)
zeroDecimalCurrencies(132-132)isZeroDecimalCurrency(134-136)ghost/core/core/server/services/members/members-api/services/PaymentsService.js (1)
zeroDecimalCurrencies(8-8)
ghost/admin/app/utils/currency.js (2)
ghost/core/core/server/services/members/members-api/services/PaymentsService.js (1)
zeroDecimalCurrencies(8-8)apps/admin-x-settings/src/utils/currency.ts (1)
isZeroDecimalCurrency(159-161)
🪛 ESLint
ghost/core/test/unit/server/services/milestones/MilestonesService.test.js
[error] 88-88: Statement inside of curly braces should be on next line.
(brace-style)
[error] 88-88: Closing curly brace should be on the same line as opening curly brace or on the line after the previous block.
(brace-style)
[error] 89-89: Statement inside of curly braces should be on next line.
(brace-style)
[error] 89-89: Closing curly brace should be on the same line as opening curly brace or on the line after the previous block.
(brace-style)
[error] 90-90: Statement inside of curly braces should be on next line.
(brace-style)
[error] 90-90: Closing curly brace should be on the same line as opening curly brace or on the line after the previous block.
(brace-style)
🔇 Additional comments (11)
ghost/admin/app/utils/subscription-data.js (1)
20-20: Updated to handle zero-decimal currencies correctly.The code now passes the currency code to
getNonDecimalwhich will correctly handle zero-decimal currencies like JPY by not dividing by 100 when appropriate.ghost/core/test/unit/server/services/milestones/MilestonesService.test.js (1)
37-41: Added JPY to supported milestones currencies.This correctly adds Japanese Yen to the supported milestone currencies with the same milestone values as other currencies.
ghost/admin/app/utils/currency.js (3)
56-56: Added JPY to supported currencies.Japanese Yen has been correctly added to the currencies list, which is required for proper dropdown selection and display throughout the application.
131-136: Added zero-decimal currencies support.This addition of the
zeroDecimalCurrenciesarray and theisZeroDecimalCurrencyhelper function properly identifies currencies that don't use minor units, following Stripe's conventions.
138-142: Updated getNonDecimal to handle zero-decimal currencies.The function now correctly checks if a currency is zero-decimal and returns the amount unchanged if so, otherwise dividing by 100 as before. This is essential for proper amount display, particularly for JPY.
ghost/core/core/server/services/members/members-api/services/PaymentsService.js (1)
7-17: Implemented zero-decimal currency support.The addition of the
zeroDecimalCurrenciesarray andisZeroDecimalCurrencyhelper function is well-documented and follows the same pattern as implemented in the admin utilities.apps/admin-x-settings/src/utils/currency.ts (5)
63-63: Excellent addition of JPY to currency optionsAdding Japanese Yen to the supported currencies list will enable appropriate representation in the UI. This aligns with the PR objective of adding JPY support to the platform.
156-157: Appropriate implementation of zero-decimal currencies listThe constant and comment correctly define currencies that don't use minor units (including JPY). This implementation matches the constants in other parts of the codebase like
ghost/admin/app/utils/currency.jsand ensures consistent handling across the application.
159-161: Well-implemented helper functionThe
isZeroDecimalCurrencyfunction correctly:
- Handles case-insensitivity with toUpperCase()
- Uses optional chaining to safely handle null/undefined
- Returns a boolean as expected
This implementation is consistent with similar helper functions in other parts of the codebase.
163-168: Proper handling of zero-decimal currencies in conversion functionThe
currencyToDecimalfunction has been appropriately updated to:
- Accept an optional currency parameter (maintaining backward compatibility)
- Skip division by 100 for zero-decimal currencies
- Maintain existing behavior for decimal currencies
This change is essential for correctly handling JPY and other zero-decimal currencies.
170-174: Correct implementation for inverse conversionThe
currencyFromDecimalfunction has been properly updated with the same pattern ascurrencyToDecimal, ensuring symmetrical conversion behavior for zero-decimal currencies. This ensures amounts are not incorrectly multiplied by 100 for currencies like JPY.
The changes improve the readability of the code by adding line breaks to the asynchronous functions within the queries object. This enhances maintainability and understanding of the test suite.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #23151 +/- ##
=======================================
Coverage 71.84% 71.84%
=======================================
Files 1435 1435
Lines 99367 99369 +2
Branches 12201 12201
=======================================
+ Hits 71388 71390 +2
Misses 26989 26989
Partials 990 990
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hey @mtane0412 ! (I'm not a Ghost Foundation employee, just an open-source contributor. ) I can't tell you anything for sure, but I don't think this could be merged without a whole bunch of new tests. (And indeed, you don't want it to get merged without new tests, because you don't want to break something.) For best success on getting the core team to evaluate and merge it, you should write a set of tests that demonstrate that it works. I'd take a look at the existing tests, and basically try to add one that tests JPY in the same way other currencies are being tested. Aside: How's it look in Portal? I'd be a little bit surprised if you didn't need some changes there, too... p.s. I started the test suite. Passing that will be a good start (but definitely not enough). I recommend getting your tests (old and new) all passing on your local install, then pushing changes here to confirm that they're good on CI also. You can @ me if you need me to get your tests started again (after confirming they work locally as much as possible) - tests here won't auto-run for you since you're new. |
This commit introduces support for the Japanese Yen (JPY) currency. The following changes were made:
apps/admin-x-settings/src/utils/currency.tsandghost/admin/app/utils/currency.js.currencyToDecimalandcurrencyFromDecimalfunctions to handle zero-decimal currencies correctly.getNonDecimalfunction to handle zero-decimal currencies correctly.getSubscriptionDatafunction to use the updatedgetNonDecimalfunction.PaymentsServiceto handle zero-decimal currencies correctly when creating and retrieving prices from Stripe.ghost/core/test/unit/server/services/milestones/ MilestonesService.test.js.These changes ensure that the application correctly handles JPY and other zero-decimal currencies, improving accuracy and functionality.
Got some code for us? Awesome 🎊!
Please take a minute to explain the change you're making:
Please check your PR against these items:
We appreciate your contribution! 🙏