Added purchase confirmation email to gift subscription flow#27205
Merged
Added purchase confirmation email to gift subscription flow#27205
Conversation
Contributor
WalkthroughAdds a new Handlebars HTML template and a plaintext TypeScript renderer for gift purchase confirmation emails. Introduces Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/core/server/services/gifts/gift-service.ts (1)
93-116: Best-effort email delivery correctly implemented.The try/catch ensures email failures don't break the purchase flow, matching the PR's stated design. However, note that
NotFoundError(tier not found) andInternalServerError(missing expiration) thrown within this block are also caught and logged silently rather than propagated.If these errors indicate data integrity issues that should be investigated, consider logging them at a different level (e.g.,
logging.warnfor tier not found vslogging.errorfor unexpected failures) to distinguish between expected edge cases and actual email delivery failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/gifts/gift-service.ts` around lines 93 - 116, The current try/catch around reading the tier and sending the email swallows validation errors; move or narrow the try block so only the call to `#giftEmailService.sendPurchaseConfirmation` is inside it (or, alternatively, in the catch inspect the error type) so that errors.NotFoundError thrown by `#tiersService.api.read` and errors.InternalServerError for missing expiresAt are not silently logged at error level—either rethrow those validation/data errors or log them with logging.warn while keeping unexpected email failures logged with logging.error; update references in the handler for `#tiersService.api.read`, errors.NotFoundError, errors.InternalServerError, `#giftEmailService.sendPurchaseConfirmation` and logging.error/warn accordingly.ghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.hbs (1)
68-68: Consider validatingaccentColorformat.The
{{accentColor}}is injected directly into astyleattribute. While this value comes from admin-controlled settings (relatively safe), consider validating it's a valid hex color (e.g.,#ff5500) in the service layer before rendering to prevent potential CSS injection if the settings cache ever receives unexpected input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.hbs` at line 68, Validate the accentColor value before it is passed to the gift-purchase-confirmation.hbs template: in the service that builds the template context (the function that prepares email data / render context for the gift purchase confirmation), assert that accentColor matches a safe hex color regex (e.g. ^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$) and if not, replace it with a known-safe default (e.g. theme accent or a hardcoded `#000000`) so the template's usage of {{accentColor}} cannot inject arbitrary CSS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@ghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.hbs`:
- Line 68: Validate the accentColor value before it is passed to the
gift-purchase-confirmation.hbs template: in the service that builds the template
context (the function that prepares email data / render context for the gift
purchase confirmation), assert that accentColor matches a safe hex color regex
(e.g. ^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$) and if not, replace it with a
known-safe default (e.g. theme accent or a hardcoded `#000000`) so the template's
usage of {{accentColor}} cannot inject arbitrary CSS.
In `@ghost/core/core/server/services/gifts/gift-service.ts`:
- Around line 93-116: The current try/catch around reading the tier and sending
the email swallows validation errors; move or narrow the try block so only the
call to `#giftEmailService.sendPurchaseConfirmation` is inside it (or,
alternatively, in the catch inspect the error type) so that errors.NotFoundError
thrown by `#tiersService.api.read` and errors.InternalServerError for missing
expiresAt are not silently logged at error level—either rethrow those
validation/data errors or log them with logging.warn while keeping unexpected
email failures logged with logging.error; update references in the handler for
`#tiersService.api.read`, errors.NotFoundError, errors.InternalServerError,
`#giftEmailService.sendPurchaseConfirmation` and logging.error/warn accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 080a2f55-dfe7-4b66-84bf-cfe073cb9145
📒 Files selected for processing (8)
ghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.hbsghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.tsghost/core/core/server/services/gifts/gift-email-renderer.tsghost/core/core/server/services/gifts/gift-email-service.tsghost/core/core/server/services/gifts/gift-service-wrapper.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/test/unit/server/services/gifts/gift-email-service.test.jsghost/core/test/unit/server/services/gifts/gift-service.test.ts
mike182uk
force-pushed
the
BER-3484-send-buyer-email
branch
from
761c07e to
48dbaf2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ghost/core/core/server/services/members/members-api/services/payments-service.js (1)
189-197: Dual-writepurchaser_emailfor one rollout window.If checkout creation and webhook handling can land on mixed-version workers during a rolling deploy, writing only the new key will break older readers. Keeping both metadata fields temporarily makes the rename safe to ship.
💡 Proposed fix
cadence, duration: String(duration), - buyer_email: email + buyer_email: email, + purchaser_email: email },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/members/members-api/services/payments-service.js` around lines 189 - 197, The metadata object currently writes buyer_email but to support a mixed-version rollout you must dual-write the old key as well: when building the metadata in payments-service (the object containing ghost_gift, gift_token, tier_id (tier.id.toHexString()), cadence, duration, buyer_email), also include purchaser_email set to the same email value (and keep buyer_email unchanged) so both keys are present for the rollout window; ensure you use the same token and email variables and leave other metadata fields (ghost_gift, gift_token, tier_id, cadence, duration) untouched.
🤖 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/core/core/server/services/gifts/gift-email-service.ts`:
- Line 57: The code currently assumes 2 decimal places by dividing amount by 100
and `#formatAmount` hardcodes two fraction digits; change this to derive the
currency's minor-unit exponent dynamically and format using that exponent. At
the call site (the formattedAmount assignment) stop hardcoding division by 100
and pass the raw minor-unit amount, then update `#formatAmount` to determine the
exponent for the given currency (e.g., with
Intl.NumberFormat(currency).resolvedOptions().maximumFractionDigits or a
currency-to-exponent map) compute majorAmount = amount / (10 ** exponent) and
use Intl.NumberFormat with minimumFractionDigits/maximumFractionDigits set to
that exponent to format the value; reference the formattedAmount assignment and
the private method `#formatAmount` for where to apply these changes.
In
`@ghost/core/core/server/services/stripe/services/webhook/checkout-session-event-service.js`:
- Around line 67-70: The call to this.deps.giftService.recordPurchase uses
session.metadata?.buyer_email but misses the legacy key purchaser_email, so
older checkout sessions end up with no buyer email; update the payload passed to
recordPurchase to prefer session.metadata?.buyer_email and fall back to
session.metadata?.purchaser_email (or null if neither exists), keeping
stripeCustomerId as before; locate the call to
this.deps.giftService.recordPurchase in checkout-session-event-service.js and
change the buyerEmail field to use the buyer_email || purchaser_email fallback.
---
Nitpick comments:
In
`@ghost/core/core/server/services/members/members-api/services/payments-service.js`:
- Around line 189-197: The metadata object currently writes buyer_email but to
support a mixed-version rollout you must dual-write the old key as well: when
building the metadata in payments-service (the object containing ghost_gift,
gift_token, tier_id (tier.id.toHexString()), cadence, duration, buyer_email),
also include purchaser_email set to the same email value (and keep buyer_email
unchanged) so both keys are present for the rollout window; ensure you use the
same token and email variables and leave other metadata fields (ghost_gift,
gift_token, tier_id, cadence, duration) untouched.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e569b520-a6bf-48b9-bce9-7eae81083177
📒 Files selected for processing (13)
ghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.hbsghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.tsghost/core/core/server/services/gifts/gift-email-renderer.tsghost/core/core/server/services/gifts/gift-email-service.tsghost/core/core/server/services/gifts/gift-service-wrapper.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/members/members-api/services/payments-service.jsghost/core/core/server/services/stripe/services/webhook/checkout-session-event-service.jsghost/core/test/unit/server/services/gifts/gift-email-service.test.jsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/members/members-api/services/payments-service.test.jsghost/core/test/unit/server/services/stripe/services/webhooks/checkout-session-event-service.test.jsghost/core/test/unit/server/services/stripe/stripe-api.test.js
✅ Files skipped from review due to trivial changes (2)
- ghost/core/test/unit/server/services/stripe/services/webhooks/checkout-session-event-service.test.js
- ghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.hbs
🚧 Files skipped from review as they are similar to previous changes (4)
- ghost/core/core/server/services/gifts/gift-service-wrapper.js
- ghost/core/core/server/services/gifts/gift-email-renderer.ts
- ghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.ts
- ghost/core/test/unit/server/services/gifts/gift-email-service.test.js
| } | ||
|
|
||
| async sendPurchaseConfirmation({buyerEmail, amount, currency, token, tierName, cadence, duration, expiresAt}: PurchaseConfirmationData): Promise<void> { | ||
| const formattedAmount = this.#formatAmount({currency, amount: amount / 100}); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node <<'NODE'
for (const [currency, amount] of [['USD', 5000], ['JPY', 5000], ['KWD', 5000]]) {
const digits = new Intl.NumberFormat('en', {style: 'currency', currency}).resolvedOptions().maximumFractionDigits;
const current = new Intl.NumberFormat('en', {
style: 'currency',
currency,
currencyDisplay: 'symbol',
maximumFractionDigits: 2,
minimumFractionDigits: 2
}).format(amount / 100);
const expected = new Intl.NumberFormat('en', {
style: 'currency',
currency,
currencyDisplay: 'symbol'
}).format(amount / (10 ** digits));
console.log({currency, digits, current, expected});
}
NODERepository: TryGhost/Ghost
Length of output: 281
🏁 Script executed:
#!/bin/bash
# Check if the file exists and read the relevant lines
if [ -f "ghost/core/core/server/services/gifts/gift-email-service.ts" ]; then
echo "=== File found, checking lines 50-70 ===" && sed -n '50,70p' "ghost/core/core/server/services/gifts/gift-email-service.ts"
echo -e "\n=== Checking lines 100-120 ===" && sed -n '100,120p' "ghost/core/core/server/services/gifts/gift-email-service.ts"
else
echo "File not found at expected path"
# Try to find it
find . -name "*gift-email-service.ts" -type f
fiRepository: TryGhost/Ghost
Length of output: 1558
Fix currency formatting to use the actual minor-unit exponent instead of hardcoding 2 decimals.
Line 57 and the #formatAmount method (lines 103–114) assume all currencies have two fractional digits. This breaks formatting for currencies with zero (JPY) or three (KWD) decimal places, causing confirmation emails to display incorrect amounts.
Proposed fix
- const formattedAmount = this.#formatAmount({currency, amount: amount / 100});
+ const fractionDigits = this.#getCurrencyFractionDigits(currency);
+ const formattedAmount = this.#formatAmount({currency, amount: amount / (10 ** fractionDigits)});
@@
+ `#getCurrencyFractionDigits`(currency: string): number {
+ return new Intl.NumberFormat('en', {
+ style: 'currency',
+ currency
+ }).resolvedOptions().maximumFractionDigits;
+ }
+
`#formatAmount`({amount = 0, currency}: {amount?: number; currency?: string}): string {
if (!currency) {
return Intl.NumberFormat('en', {maximumFractionDigits: 2}).format(amount);
}
return Intl.NumberFormat('en', {
style: 'currency',
currency,
- currencyDisplay: 'symbol',
- maximumFractionDigits: 2,
- minimumFractionDigits: 2
+ currencyDisplay: 'symbol'
}).format(amount);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/core/server/services/gifts/gift-email-service.ts` at line 57, The
code currently assumes 2 decimal places by dividing amount by 100 and
`#formatAmount` hardcodes two fraction digits; change this to derive the
currency's minor-unit exponent dynamically and format using that exponent. At
the call site (the formattedAmount assignment) stop hardcoding division by 100
and pass the raw minor-unit amount, then update `#formatAmount` to determine the
exponent for the given currency (e.g., with
Intl.NumberFormat(currency).resolvedOptions().maximumFractionDigits or a
currency-to-exponent map) compute majorAmount = amount / (10 ** exponent) and
use Intl.NumberFormat with minimumFractionDigits/maximumFractionDigits set to
that exponent to format the value; reference the formattedAmount assignment and
the private method `#formatAmount` for where to apply these changes.
ghost/core/core/server/services/stripe/services/webhook/checkout-session-event-service.js
Show resolved
Hide resolved
mike182uk
force-pushed
the
BER-3484-send-buyer-email
branch
from
48dbaf2 to
42f83bf
Compare
ref https://linear.app/ghost/issue/BER-3484 The buyer now receives a transactional email with the gift redemption link, tier name, amount paid, cadence, and expiration date after a successful gift checkout. Follows the `comments-service` pattern with a dedicated `GiftEmailService` + `GiftEmailRenderer` and co-located `Handlebars` templates. Email delivery is best-effort (failures are logged but don't break the webhook handler)
mike182uk
force-pushed
the
BER-3484-send-buyer-email
branch
from
42f83bf to
34f35d1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ghost/core/core/server/services/gifts/gift-email-service.ts (1)
57-57:⚠️ Potential issue | 🟠 MajorUse currency-specific minor units instead of hardcoded 2-decimal formatting.
Line 57 and Line 107-113 currently assume all currencies are 2-decimal, which misformats buyer totals for currencies like JPY (0) and KWD (3).
Proposed fix
- const formattedAmount = this.formatAmount({currency, amount: amount / 100}); + const formattedAmount = this.formatAmount({currency, amount}); + private getCurrencyFractionDigits(currency: string): number { + return Intl.NumberFormat('en', { + style: 'currency', + currency: currency.toUpperCase() + }).resolvedOptions().maximumFractionDigits; + } + private formatAmount({amount = 0, currency}: {amount?: number; currency?: string}): string { if (!currency) { - return Intl.NumberFormat('en', {maximumFractionDigits: 2}).format(amount); + return Intl.NumberFormat('en', {maximumFractionDigits: 2}).format(amount / 100); } + const normalizedCurrency = currency.toUpperCase(); + const fractionDigits = this.getCurrencyFractionDigits(normalizedCurrency); + return Intl.NumberFormat('en', { style: 'currency', - currency, + currency: normalizedCurrency, currencyDisplay: 'symbol', - maximumFractionDigits: 2, - minimumFractionDigits: 2 - }).format(amount); + maximumFractionDigits: fractionDigits, + minimumFractionDigits: fractionDigits + }).format(amount / (10 ** fractionDigits)); }Also applies to: 102-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/gifts/gift-email-service.ts` at line 57, The code is dividing amounts by a hardcoded 100 (assuming 2 decimal minor units) when formatting (see the use of this.formatAmount({currency, amount: amount / 100}) and the block around lines 102-113); replace this with currency-specific minor-unit handling: determine the currency's minor unit/precision (e.g., 0 for JPY, 3 for KWD) and divide by 10**precision (or convert using that precision) before calling this.formatAmount, updating all occurrences (the single-line conversion and the buyer-total logic in the block that formats amounts) to use the currency precision lookup instead of hardcoded 100. Ensure you reference the currency symbol/ISO code passed in (the existing currency variable) and a single helper or map (e.g., getCurrencyDecimals or a CURRENCY_DECIMALS map) so all formatting is correct for non-2-decimal currencies.
🧹 Nitpick comments (1)
ghost/core/core/server/services/gifts/gift-service.ts (1)
16-27: Consider extracting a shared purchase-confirmation payload type.Line 16-27 duplicates a cross-service contract that already exists in the email service module. Pulling this into a shared type will reduce drift risk between caller and callee.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/gifts/gift-service.ts` around lines 16 - 27, The GiftEmailService.sendPurchaseConfirmation payload duplicates an existing contract in the email service; extract the shared payload into a common type (e.g., PurchaseConfirmationPayload) in the shared/email-types module and replace the inline object type in GiftEmailService.sendPurchaseConfirmation with that type; update the gift-service.ts interface signature to use PurchaseConfirmationPayload and update any callers/imports to import the shared type instead of redefining the shape.
🤖 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/core/test/unit/server/services/gifts/gift-email-service.test.js`:
- Around line 74-78: Add regression tests that assert non-2-decimal currencies
are formatted correctly: create two new it() cases in gift-email-service.test.js
that call service.sendPurchaseConfirmation with currency: 'jpy' (e.g., amount:
1500) and currency: 'kwd' (e.g., amount: 1500 or another value showing 3
decimals) and use sinon.assert.calledWith(mailer.send, sinon.match.has('html',
sinon.match(...))) to check the HTML contains the correctly formatted strings
for JPY (no decimals) and KWD (three decimals). Target the same helpers used in
the existing tests (service.sendPurchaseConfirmation, formatAmount and
mailer.send) so future changes to formatAmount that hardcode 2 decimals will be
caught.
---
Duplicate comments:
In `@ghost/core/core/server/services/gifts/gift-email-service.ts`:
- Line 57: The code is dividing amounts by a hardcoded 100 (assuming 2 decimal
minor units) when formatting (see the use of this.formatAmount({currency,
amount: amount / 100}) and the block around lines 102-113); replace this with
currency-specific minor-unit handling: determine the currency's minor
unit/precision (e.g., 0 for JPY, 3 for KWD) and divide by 10**precision (or
convert using that precision) before calling this.formatAmount, updating all
occurrences (the single-line conversion and the buyer-total logic in the block
that formats amounts) to use the currency precision lookup instead of hardcoded
100. Ensure you reference the currency symbol/ISO code passed in (the existing
currency variable) and a single helper or map (e.g., getCurrencyDecimals or a
CURRENCY_DECIMALS map) so all formatting is correct for non-2-decimal
currencies.
---
Nitpick comments:
In `@ghost/core/core/server/services/gifts/gift-service.ts`:
- Around line 16-27: The GiftEmailService.sendPurchaseConfirmation payload
duplicates an existing contract in the email service; extract the shared payload
into a common type (e.g., PurchaseConfirmationPayload) in the shared/email-types
module and replace the inline object type in
GiftEmailService.sendPurchaseConfirmation with that type; update the
gift-service.ts interface signature to use PurchaseConfirmationPayload and
update any callers/imports to import the shared type instead of redefining the
shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5f35c7a-e03a-450b-be95-ccbbcd8d1e82
📒 Files selected for processing (13)
ghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.hbsghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.tsghost/core/core/server/services/gifts/gift-email-renderer.tsghost/core/core/server/services/gifts/gift-email-service.tsghost/core/core/server/services/gifts/gift-service-wrapper.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/members/members-api/services/payments-service.jsghost/core/core/server/services/stripe/services/webhook/checkout-session-event-service.jsghost/core/test/unit/server/services/gifts/gift-email-service.test.jsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/members/members-api/services/payments-service.test.jsghost/core/test/unit/server/services/stripe/services/webhooks/checkout-session-event-service.test.jsghost/core/test/unit/server/services/stripe/stripe-api.test.js
✅ Files skipped from review due to trivial changes (4)
- ghost/core/core/server/services/members/members-api/services/payments-service.js
- ghost/core/test/unit/server/services/stripe/services/webhooks/checkout-session-event-service.test.js
- ghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.hbs
- ghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/core/server/services/stripe/services/webhook/checkout-session-event-service.js
- ghost/core/core/server/services/gifts/gift-service-wrapper.js
- ghost/core/core/server/services/gifts/gift-email-renderer.ts
Comment on lines
+74
to
+78
| it('includes formatted amount in HTML', async function () { | ||
| await service.sendPurchaseConfirmation({...defaultData, amount: 1500, currency: 'eur'}); | ||
|
|
||
| sinon.assert.calledWith(mailer.send, sinon.match.has('html', sinon.match('€15.00'))); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n ghost/core/test/unit/server/services/gifts/gift-email-service.test.js | sed -n '60,100p'Repository: TryGhost/Ghost
Length of output: 2147
🏁 Script executed:
fd gift-email-service.js --type f | head -5Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
rg -A 10 -B 5 "sendPurchaseConfirmation" ghost/core/test/unit/server/services/gifts/gift-email-service.test.js | head -50Repository: TryGhost/Ghost
Length of output: 2154
🏁 Script executed:
# Search for the actual service file
fd -E "*.test.*" "gift-email-service" --type fRepository: TryGhost/Ghost
Length of output: 117
🏁 Script executed:
cat -n ghost/core/core/server/services/gifts/gift-email-service.tsRepository: TryGhost/Ghost
Length of output: 4505
🏁 Script executed:
# Also check if there are any other currency-related tests or fixtures
rg -B 3 -A 3 "currency" ghost/core/test/unit/server/services/gifts/gift-email-service.test.jsRepository: TryGhost/Ghost
Length of output: 524
🏁 Script executed:
# Check defaultData to understand current test coverage
rg -B 5 "defaultData\s*=" ghost/core/test/unit/server/services/gifts/gift-email-service.test.js | head -30Repository: TryGhost/Ghost
Length of output: 172
🏁 Script executed:
# Check the full test file to see all tests
wc -l ghost/core/test/unit/server/services/gifts/gift-email-service.test.jsRepository: TryGhost/Ghost
Length of output: 131
🏁 Script executed:
# Check if there are any tests with JPY, KWD, or other non-2-decimal currencies
rg "jpy|kwd|bhd|tnd" ghost/core/test/unit/server/services/gifts/gift-email-service.test.js -iRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Verify the exact behavior of Intl.NumberFormat with hardcoded fractionDigits
python3 << 'EOF'
# Simulate what Intl.NumberFormat does with the hardcoded parameters
currencies_test = {
'USD': 5000,
'EUR': 1500,
'JPY': 150000,
'KWD': 1500,
'BHD': 1500,
}
# Show what the current implementation would produce
print("Current implementation (hardcoded minimumFractionDigits: 2, maximumFractionDigits: 2):")
print("All currencies forced to 2 decimal places\n")
# Native fraction digits per currency
native_fractions = {
'USD': 2,
'EUR': 2,
'JPY': 0,
'KWD': 3,
'BHD': 3,
}
print("Native fraction digits by currency:")
for currency, fractions in native_fractions.items():
print(f" {currency}: {fractions} decimal places")
EOFRepository: TryGhost/Ghost
Length of output: 343
🏁 Script executed:
# Check if there are any TODO or FIXME comments related to currency formatting
rg "TODO|FIXME" ghost/core/core/server/services/gifts/gift-email-service.tsRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Get the full test file to see all tests
cat -n ghost/core/test/unit/server/services/gifts/gift-email-service.test.js | tail -40Repository: TryGhost/Ghost
Length of output: 1899
🏁 Script executed:
# Check if there are any other gift-email related files that might test currency
fd gift-email ghost/core/test -type fRepository: TryGhost/Ghost
Length of output: 228
🏁 Script executed:
# Find all gift-email related files in test directory
find ghost/core/test -name "*gift-email*" -type fRepository: TryGhost/Ghost
Length of output: 127
🏁 Script executed:
# Check the implementation again - looking at lines 102-114 for formatAmount
cat -n ghost/core/core/server/services/gifts/gift-email-service.ts | sed -n '102,114p'Repository: TryGhost/Ghost
Length of output: 609
Add regression tests for non-2-decimal currencies (e.g., JPY with 0 decimals, KWD with 3 decimals).
Lines 74-78 and 86-91 only verify USD/EUR formatting. The formatAmount() implementation hardcodes minimumFractionDigits: 2 and maximumFractionDigits: 2, which forces all currencies to render with 2 decimals. Add tests for JPY and KWD to ensure formatting regressions are caught if the implementation is updated to respect native currency precision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/test/unit/server/services/gifts/gift-email-service.test.js` around
lines 74 - 78, Add regression tests that assert non-2-decimal currencies are
formatted correctly: create two new it() cases in gift-email-service.test.js
that call service.sendPurchaseConfirmation with currency: 'jpy' (e.g., amount:
1500) and currency: 'kwd' (e.g., amount: 1500 or another value showing 3
decimals) and use sinon.assert.calledWith(mailer.send, sinon.match.has('html',
sinon.match(...))) to check the HTML contains the correctly formatted strings
for JPY (no decimals) and KWD (three decimals). Target the same helpers used in
the existing tests (service.sendPurchaseConfirmation, formatAmount and
mailer.send) so future changes to formatAmount that hardcode 2 decimals will be
caught.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ghost/core/core/server/services/gifts/gift-email-renderer.ts (1)
16-21: Avoid duplicate template compilation on concurrent first renders.
renderPurchaseConfirmationcan runreadFile + compilemultiple times if several requests hit beforepurchaseConfirmationTemplateis set. Memoizing the initialization promise prevents duplicate work.Proposed refactor
export class GiftEmailRenderer { private readonly handlebars: typeof Handlebars; private purchaseConfirmationTemplate: HandlebarsTemplateDelegate | null = null; + private purchaseConfirmationTemplatePromise: Promise<HandlebarsTemplateDelegate> | null = null; constructor() { this.handlebars = Handlebars.create(); } async renderPurchaseConfirmation(data: GiftPurchaseConfirmationData): Promise<{html: string; text: string}> { - if (!this.purchaseConfirmationTemplate) { - const source = await fs.readFile(path.join(__dirname, './email-templates/gift-purchase-confirmation.hbs'), 'utf8'); - - this.purchaseConfirmationTemplate = this.handlebars.compile(source); - } + if (!this.purchaseConfirmationTemplatePromise) { + this.purchaseConfirmationTemplatePromise = fs + .readFile(path.join(__dirname, './email-templates/gift-purchase-confirmation.hbs'), 'utf8') + .then(source => this.handlebars.compile(source)); + } + this.purchaseConfirmationTemplate = this.purchaseConfirmationTemplate ?? await this.purchaseConfirmationTemplatePromise; return { html: this.purchaseConfirmationTemplate(data), text: renderPurchaseConfirmationText(data) }; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/gifts/gift-email-renderer.ts` around lines 16 - 21, The renderPurchaseConfirmation method currently races on first use by performing fs.readFile + this.handlebars.compile directly when purchaseConfirmationTemplate is falsy; fix it by introducing and using a memoized initialization promise (e.g., purchaseConfirmationTemplateInit or purchaseConfirmationTemplatePromise) alongside purchaseConfirmationTemplate so the first concurrent caller sets the promise to the read+compile task and others await it, then assign the compiled template to this.purchaseConfirmationTemplate once the promise resolves; update renderPurchaseConfirmation to await that promise when present to avoid duplicate compilation.ghost/core/test/unit/server/services/gifts/gift-email-service.test.js (1)
84-94: Also assert plaintext output for non-default formatting cases.These tests currently validate only
html. Addingtextassertions keeps coverage aligned with the dual-format contract.Suggested test additions
it('formats non-USD currency correctly', async function () { await service.sendPurchaseConfirmation({...defaultData, amount: 1500, currency: 'eur'}); sinon.assert.calledWith(mailer.send, sinon.match.has('html', sinon.match('€15.00'))); + sinon.assert.calledWith(mailer.send, sinon.match.has('text', sinon.match('€15.00'))); }); it('formats month cadence correctly', async function () { await service.sendPurchaseConfirmation({...defaultData, cadence: 'month'}); sinon.assert.calledWith(mailer.send, sinon.match.has('html', sinon.match('1 month'))); + sinon.assert.calledWith(mailer.send, sinon.match.has('text', sinon.match('1 month'))); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/gifts/gift-email-service.test.js` around lines 84 - 94, Update the two unit tests that assert only HTML output to also assert plaintext output exists and is formatted correctly: after calling service.sendPurchaseConfirmation (used in both tests) add sinon.assert.calledWith(mailer.send, sinon.match.has('text', sinon.match('€15.00'))) for the non-USD currency test and sinon.assert.calledWith(mailer.send, sinon.match.has('text', sinon.match('1 month'))) for the month cadence test, referencing the same defaultData and mailer.send used in the current assertions so both html and text formats are validated.ghost/core/core/server/services/gifts/gift-service.ts (1)
109-132: Consider a retry path for buyer confirmation failures.Current best-effort logging is fine for webhook continuity, but transient tier/email failures mean some buyers will never receive confirmation. Queueing a retryable background job (idempotent by gift token/session) would improve delivery reliability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/gifts/gift-service.ts` around lines 109 - 132, The sendPurchaseConfirmation call in GiftService (gift-service.ts) currently swallows errors and only logs them, so transient failures can lose buyer confirmations; change this to enqueue a retryable background job instead of calling `#giftEmailService.sendPurchaseConfirmation`() directly: create/dispatch a job (e.g., "send-gift-confirmation") via the app's background job/queue service from the same code path, pass all necessary payload (buyerEmail, amount, currency, token, tierName, cadence, duration, expiresAt), and make the job handler idempotent keyed by gift token/session to avoid duplicate emails; keep the try/catch but on failure of enqueue log and surface errors appropriately so webhook continuity is preserved while email delivery is retried.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/server/services/gifts/gift-email-renderer.ts`:
- Around line 16-21: The renderPurchaseConfirmation method currently races on
first use by performing fs.readFile + this.handlebars.compile directly when
purchaseConfirmationTemplate is falsy; fix it by introducing and using a
memoized initialization promise (e.g., purchaseConfirmationTemplateInit or
purchaseConfirmationTemplatePromise) alongside purchaseConfirmationTemplate so
the first concurrent caller sets the promise to the read+compile task and others
await it, then assign the compiled template to this.purchaseConfirmationTemplate
once the promise resolves; update renderPurchaseConfirmation to await that
promise when present to avoid duplicate compilation.
In `@ghost/core/core/server/services/gifts/gift-service.ts`:
- Around line 109-132: The sendPurchaseConfirmation call in GiftService
(gift-service.ts) currently swallows errors and only logs them, so transient
failures can lose buyer confirmations; change this to enqueue a retryable
background job instead of calling `#giftEmailService.sendPurchaseConfirmation`()
directly: create/dispatch a job (e.g., "send-gift-confirmation") via the app's
background job/queue service from the same code path, pass all necessary payload
(buyerEmail, amount, currency, token, tierName, cadence, duration, expiresAt),
and make the job handler idempotent keyed by gift token/session to avoid
duplicate emails; keep the try/catch but on failure of enqueue log and surface
errors appropriately so webhook continuity is preserved while email delivery is
retried.
In `@ghost/core/test/unit/server/services/gifts/gift-email-service.test.js`:
- Around line 84-94: Update the two unit tests that assert only HTML output to
also assert plaintext output exists and is formatted correctly: after calling
service.sendPurchaseConfirmation (used in both tests) add
sinon.assert.calledWith(mailer.send, sinon.match.has('text',
sinon.match('€15.00'))) for the non-USD currency test and
sinon.assert.calledWith(mailer.send, sinon.match.has('text', sinon.match('1
month'))) for the month cadence test, referencing the same defaultData and
mailer.send used in the current assertions so both html and text formats are
validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dde7476d-efac-4746-a145-8c5ebcfd8fa6
📒 Files selected for processing (13)
ghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.hbsghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.tsghost/core/core/server/services/gifts/gift-email-renderer.tsghost/core/core/server/services/gifts/gift-email-service.tsghost/core/core/server/services/gifts/gift-service-wrapper.jsghost/core/core/server/services/gifts/gift-service.tsghost/core/core/server/services/members/members-api/services/payments-service.jsghost/core/core/server/services/stripe/services/webhook/checkout-session-event-service.jsghost/core/test/unit/server/services/gifts/gift-email-service.test.jsghost/core/test/unit/server/services/gifts/gift-service.test.tsghost/core/test/unit/server/services/members/members-api/services/payments-service.test.jsghost/core/test/unit/server/services/stripe/services/webhooks/checkout-session-event-service.test.jsghost/core/test/unit/server/services/stripe/stripe-api.test.js
✅ Files skipped from review due to trivial changes (3)
- ghost/core/test/unit/server/services/stripe/services/webhooks/checkout-session-event-service.test.js
- ghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.ts
- ghost/core/core/server/services/gifts/email-templates/gift-purchase-confirmation.hbs
🚧 Files skipped from review as they are similar to previous changes (5)
- ghost/core/core/server/services/members/members-api/services/payments-service.js
- ghost/core/test/unit/server/services/members/members-api/services/payments-service.test.js
- ghost/core/test/unit/server/services/stripe/stripe-api.test.js
- ghost/core/core/server/services/gifts/gift-service-wrapper.js
- ghost/core/core/server/services/gifts/gift-email-service.ts
sagzy
approved these changes
Apr 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref https://linear.app/ghost/issue/BER-3484
The buyer now receives a transactional email with the gift redemption link, tier name, amount paid, cadence, and expiration date after a successful gift checkout. Follows the
comments-servicepattern with a dedicatedGiftEmailService+GiftEmailRendererand co-locatedHandlebarstemplates. Email delivery is best-effort (failures are logged but don't break the webhook handler)