Wired up gift subscription checkout end-to-end#27080
Open
Conversation
Contributor
|
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:
WalkthroughAdds end-to-end gift purchase support. Frontend: new 🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
ghost/core/test/unit/server/services/members/members-api/services/payments-service.test.js (1)
431-553: Consider adding one edge-case assertion forcustomerEmailfallback behavior.A test where
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/members/members-api/services/payments-service.test.js` around lines 431 - 553, Add a test for PaymentsService.getGiftPaymentLink that verifies the customerEmail fallback when the explicit email argument is omitted: instantiate PaymentsService with a createGiftCheckoutSession stub (as in other tests), provide member: {get: sinon.stub().returns('member@example.com')} with isAuthenticated: false and no email field, call getGiftPaymentLink, then assert that the stubbed createGiftCheckoutSession was called with args.customer === null and args.customerEmail === 'member@example.com' (use the same createGiftCheckoutSessionStub.firstCall.firstArg pattern).ghost/core/test/unit/server/services/stripe/stripe-api.test.js (1)
659-803: Add one gift-session test forcustomer_updatewith automatic tax enabled.This suite already covers most gift payload behavior; adding an assertion for the
customer_update: {address: 'auto'}branch would close the remaining gap forcreateGiftCheckoutSession.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/stripe/stripe-api.test.js` around lines 659 - 803, Add a new unit test in the createGiftCheckoutSession suite that enables automatic tax (stub/mock the labs/feature flag used for automatic tax), calls api.createGiftCheckoutSession with typical gift params, then inspect mockStripe.checkout.sessions.create.firstCall.firstArg and assert that customer_update exists and has address === 'auto' (i.e. args.customer_update.address === 'auto'); reference the createGiftCheckoutSession API and mockStripe.checkout.sessions.create to locate where to add the assertion.ghost/core/core/server/services/stripe/stripe-api.js (1)
704-705: Validate unsupported cadence values before building the product name.At Line 704, any cadence other than
'month'is treated as'1 year'. That can silently generate incorrect checkout labels if upstream validation ever regresses. Prefer explicit handling for'month' | 'year'and throw otherwise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/stripe/stripe-api.js` around lines 704 - 705, The cadenceLabel assignment silently treats any non-'month' cadence as '1 year'; update the code that computes cadenceLabel (and any product name construction that uses it) to explicitly accept only 'month' or 'year' — e.g., check the cadence variable and set cadenceLabel for 'month' -> '1 month' and 'year' -> '1 year', and throw a clear Error (including the invalid cadence value) for any other input so invalid cadence values fail fast instead of generating incorrect labels.
🤖 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/src/components/pages/gift-page.js`:
- Around line 182-183: The CTA is still enabled when the selected cadence isn't
available on a tier, causing failed checkouts; update the isDisabled logic used
where isPurchasing/isCookiesDisabled are checked to also validate the active
cadence exists on the tier (e.g., if cadence === 'monthly' require
tier.monthlyPrice, if 'yearly' require tier.yearlyPrice). Locate the usage
around isPurchasing and isDisabled (and the handlePurchase call) and
short-circuit the CTA/click handler when the cadence is unsupported so
GiftProductCardPrice and the button remain disabled for mixed-cadence tiers.
In `@apps/portal/src/utils/api.js`:
- Around line 542-552: The current block calls await response.json() unguarded —
move JSON parsing behind a try/catch: attempt to parse with await
response.json() into responseJson, but if parsing throws, read fallbackText via
await response.text() and set responseJson = { fallbackText }; then keep the
existing if (!response.ok) branch but use responseJson?.errors?.[0] when
present, otherwise throw a new Error that includes the fallbackText (or the raw
text/response.status) so non-JSON/HTML error responses are surfaced; update the
throw logic around responseJson, response.ok and error to use these guarded
values.
In
`@ghost/core/core/server/services/members/members-api/controllers/router-controller.js`:
- Around line 743-748: The current call to this._createGiftCheckoutSession
overwrites the caller-provided cancelUrl by unconditionally setting cancelUrl:
this._urlUtils.getSiteUrl(); change it so the cancelUrl is preserved when
provided (e.g. use options.cancelUrl || data.cancelUrl ||
this._urlUtils.getSiteUrl()) so callers' req.body.cancelUrl is used for aborted
gift checkouts; keep successUrl as-is but only default cancelUrl to getSiteUrl()
when no caller value exists.
In
`@ghost/core/core/server/services/members/members-api/services/payments-service.js`:
- Around line 192-198: The Stripe session payload sets purchaser_email using
email || (member && member.get('email')) || '' but customerEmail only uses the
raw email; update the customerEmail expression in the session creation (near
successUrlObj, cancelUrl, customer) to use the same fallback as purchaser_email
(i.e. email || (member && member.get('email')) or null when absent) so Stripe
receives the prefilled email when member.get('email') exists.
---
Nitpick comments:
In `@ghost/core/core/server/services/stripe/stripe-api.js`:
- Around line 704-705: The cadenceLabel assignment silently treats any
non-'month' cadence as '1 year'; update the code that computes cadenceLabel (and
any product name construction that uses it) to explicitly accept only 'month' or
'year' — e.g., check the cadence variable and set cadenceLabel for 'month' -> '1
month' and 'year' -> '1 year', and throw a clear Error (including the invalid
cadence value) for any other input so invalid cadence values fail fast instead
of generating incorrect labels.
In
`@ghost/core/test/unit/server/services/members/members-api/services/payments-service.test.js`:
- Around line 431-553: Add a test for PaymentsService.getGiftPaymentLink that
verifies the customerEmail fallback when the explicit email argument is omitted:
instantiate PaymentsService with a createGiftCheckoutSession stub (as in other
tests), provide member: {get: sinon.stub().returns('member@example.com')} with
isAuthenticated: false and no email field, call getGiftPaymentLink, then assert
that the stubbed createGiftCheckoutSession was called with args.customer ===
null and args.customerEmail === 'member@example.com' (use the same
createGiftCheckoutSessionStub.firstCall.firstArg pattern).
In `@ghost/core/test/unit/server/services/stripe/stripe-api.test.js`:
- Around line 659-803: Add a new unit test in the createGiftCheckoutSession
suite that enables automatic tax (stub/mock the labs/feature flag used for
automatic tax), calls api.createGiftCheckoutSession with typical gift params,
then inspect mockStripe.checkout.sessions.create.firstCall.firstArg and assert
that customer_update exists and has address === 'auto' (i.e.
args.customer_update.address === 'auto'); reference the
createGiftCheckoutSession API and mockStripe.checkout.sessions.create to locate
where to add the assertion.
🪄 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: 0ffba2c2-b672-4b2a-92c8-4d8230445bc7
📒 Files selected for processing (10)
apps/portal/src/actions.jsapps/portal/src/components/pages/gift-page.jsapps/portal/src/utils/api.jsapps/portal/test/actions.test.tsghost/core/core/server/services/members/members-api/controllers/router-controller.jsghost/core/core/server/services/members/members-api/services/payments-service.jsghost/core/core/server/services/stripe/stripe-api.jsghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.jsghost/core/test/unit/server/services/members/members-api/services/payments-service.test.jsghost/core/test/unit/server/services/stripe/stripe-api.test.js
ghost/core/core/server/services/members/members-api/controllers/router-controller.js
Show resolved
Hide resolved
Comment on lines
+192
to
+198
| purchaser_email: email || (member && member.get('email')) || '' | ||
| }, | ||
| successUrl: successUrlObj.toString(), | ||
| cancelUrl, | ||
| customer, | ||
| customerEmail: !customer && email ? email : null | ||
| }; |
Contributor
There was a problem hiding this comment.
Align customerEmail fallback with purchaser_email fallback.
At Line 197, customerEmail only uses explicit email. If email is missing but member.get('email') exists, Stripe won’t get a prefilled email even though metadata has one.
Proposed fix
- customerEmail: !customer && email ? email : null
+ customerEmail: !customer ? (email || (member && member.get('email')) || null) : null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| purchaser_email: email || (member && member.get('email')) || '' | |
| }, | |
| successUrl: successUrlObj.toString(), | |
| cancelUrl, | |
| customer, | |
| customerEmail: !customer && email ? email : null | |
| }; | |
| purchaser_email: email || (member && member.get('email')) || '' | |
| }, | |
| successUrl: successUrlObj.toString(), | |
| cancelUrl, | |
| customer, | |
| customerEmail: !customer ? (email || (member && member.get('email')) || null) : null | |
| }; |
🤖 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 192 - 198, The Stripe session payload sets purchaser_email using
email || (member && member.get('email')) || '' but customerEmail only uses the
raw email; update the customerEmail expression in the session creation (near
successUrlObj, cancelUrl, customer) to use the same fallback as purchaser_email
(i.e. email || (member && member.get('email')) or null when absent) so Stripe
receives the prefilled email when member.get('email') exists.
mike182uk
force-pushed
the
BER-3484-stripe-checkout
branch
from
907f650 to
7e462c1
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/stripe/stripe-api.test.js (1)
659-803: Good test coverage; consider adding automatic tax test case.The test suite thoroughly covers the core functionality: session mode, price_data structure, cadence labels, metadata passthrough, and customer/email handling.
One gap: the implementation includes
customer_updatelogic when bothcustomerandenableAutomaticTaxare truthy (lines 729-731 in stripe-api.js), but this behavior isn't tested here. The file already has a pattern for this in the "createCheckoutSetupSession automatic tax flag" suite.💡 Optional: Add automatic tax test case
+ it('adds customer_update when customer and automatic tax are enabled', async function () { + api.configure({ + checkoutSessionSuccessUrl: '/success', + checkoutSessionCancelUrl: '/cancel', + checkoutSetupSessionSuccessUrl: '/setup-success', + checkoutSetupSessionCancelUrl: '/setup-cancel', + secretKey: '', + enableAutomaticTax: true + }); + + await api.createGiftCheckoutSession({ + amount: 5000, + currency: 'usd', + tierName: 'Pro', + cadence: 'year', + successUrl: '/gift-success', + cancelUrl: '/gift-cancel', + metadata: {}, + customer: {id: mockCustomerId} + }); + + const args = mockStripe.checkout.sessions.create.firstCall.firstArg; + + assert.deepEqual(args.customer_update, {address: 'auto'}); + }); + it('does not include invoice_creation or custom_fields', async function () {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/stripe/stripe-api.test.js` around lines 659 - 803, The tests for createGiftCheckoutSession miss the case where both a customer is provided and automatic taxes are enabled, so add a test that stubs/mock Labs to enable the automatic tax flag and calls createGiftCheckoutSession with a customer (use mockCustomerId), then assert that the Stripe session create args include the expected customer_update property (and still set args.customer to the provided id and not customer_email); reference createGiftCheckoutSession and the customer_update behavior checked by enableAutomaticTax to locate where to add the test.
🤖 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/test/unit/server/services/stripe/stripe-api.test.js`:
- Around line 659-803: The tests for createGiftCheckoutSession miss the case
where both a customer is provided and automatic taxes are enabled, so add a test
that stubs/mock Labs to enable the automatic tax flag and calls
createGiftCheckoutSession with a customer (use mockCustomerId), then assert that
the Stripe session create args include the expected customer_update property
(and still set args.customer to the provided id and not customer_email);
reference createGiftCheckoutSession and the customer_update behavior checked by
enableAutomaticTax to locate where to add the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97d0f71a-1853-42a2-8c87-f8c01dad0464
📒 Files selected for processing (11)
apps/portal/package.jsonapps/portal/src/actions.jsapps/portal/src/components/pages/gift-page.jsapps/portal/src/utils/api.jsapps/portal/test/actions.test.tsghost/core/core/server/services/members/members-api/controllers/router-controller.jsghost/core/core/server/services/members/members-api/services/payments-service.jsghost/core/core/server/services/stripe/stripe-api.jsghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.jsghost/core/test/unit/server/services/members/members-api/services/payments-service.test.jsghost/core/test/unit/server/services/stripe/stripe-api.test.js
✅ Files skipped from review due to trivial changes (4)
- apps/portal/package.json
- apps/portal/test/actions.test.ts
- ghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.js
- ghost/core/core/server/services/members/members-api/controllers/router-controller.js
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/portal/src/actions.js
- ghost/core/core/server/services/members/members-api/services/payments-service.js
- apps/portal/src/utils/api.js
- apps/portal/src/components/pages/gift-page.js
- ghost/core/test/unit/server/services/members/members-api/services/payments-service.test.js
mike182uk
force-pushed
the
BER-3484-stripe-checkout
branch
2 times, most recently
from
086701b to
57e1d3d
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/portal/src/components/pages/gift-page.js (1)
182-183:⚠️ Potential issue | 🟠 MajorDisable purchase for tiers that don't support the active cadence.
GiftProductCardPricecan render no price for mixed-cadence tiers, butisDisabledandhandlePurchase()still let users submit that unavailable interval. That leaves a clickable CTA that can only end in a rejected checkout.Suggested fix
+ const hasPriceForInterval = product => ( + activeInterval === 'month' ? Boolean(product.monthlyPrice) : Boolean(product.yearlyPrice) + ); + const handlePurchase = (e, product) => { e.preventDefault(); + + if (!hasPriceForInterval(product)) { + return; + } const errors = ValidateInputForm({fields: emailField}); @@ <GiftProductCard key={product.id} brandColor={brandColor} product={product} selectedInterval={activeInterval} - isDisabled={isDisabled} + isDisabled={isDisabled || !hasPriceForInterval(product)} isPurchasing={isPurchasing && selectedProduct === product.id} onPurchase={handlePurchase} />Also applies to: 197-214, 253-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/src/components/pages/gift-page.js` around lines 182 - 183, The CTA is still enabled for tiers that don't support the currently selected cadence; update the disable logic and purchase handler to check cadence support so users cannot start checkout for an unavailable interval. Modify isDisabled (where isPurchasing and isCookiesDisabled are computed) to also call the tier/cadence support checker used by GiftProductCardPrice (e.g., add a check like doesTierSupportCadence(tier, activeCadence) or reuse the same helper) and set isDisabled true when unsupported, and in handlePurchase (the purchase submission function) early-return if the cadence is unsupported to guard against direct calls; ensure you reference the same tier/cadence identifiers used by GiftProductCardPrice so the UI and logic are consistent.apps/portal/src/utils/api.js (1)
542-558:⚠️ Potential issue | 🟡 MinorGuard JSON parsing before the fallback error handling.
response.json()can throw on empty or HTML responses, which skips the intended!response.okbranch and bubbles aSyntaxErrorinstead. Parse defensively so gift checkout failures still return a controlled error.Suggested fix
- const responseJson = await response.json(); + const responseText = await response.text(); + let responseJson = {}; + try { + responseJson = responseText ? JSON.parse(responseText) : {}; + } catch (e) { + responseJson = {}; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/portal/src/utils/api.js` around lines 542 - 558, The code currently calls response.json() directly which can throw on empty or non-JSON responses and bypass the intended !response.ok error handling; update the logic around response.json() (the responseJson variable) to parse defensively — e.g., check response.headers.get('content-type') for application/json before parsing or wrap response.json() in try/catch and fall back to an empty object (or populate responseJson.errors if available) so the subsequent !response.ok branch and final controlled Error('Failed to process gift checkout, please try again.') execute instead of a raw SyntaxError; ensure the existing checks for responseJson?.errors?.[0] and responseJson.url and the call to window.location.assign(responseJson.url) continue to work with the guarded parse.
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.js (1)
738-749: Strengthen the gift-checkout assertions in these two tests.The disabled-lab case currently passes on any
BadRequestError, and the paid-member case only proves the spy ran. Please also pin the expected lab-flag failure and the forwarded authenticated context so an unrelated rejection or a silent fallback to guest checkout doesn't still pass.Also applies to: 780-801
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.js` around lines 738 - 749, The test accepts any BadRequestError on lab-flag disabled; tighten it by asserting the labs flag checked and the error identifies the giftSubscriptions flag: verify labsService.isSet was called with 'giftSubscriptions' and assert the thrown error.message (or error.context) contains 'giftSubscriptions' or a specific tag indicating the lab is disabled when invoking createCheckoutSession on the controller returned by createGiftController; likewise, in the paid-member test (the other test at ~780-801) assert the authenticated context is forwarded by checking the controller.createCheckoutSession path receives/uses the expected auth user (e.g., the ctx.user or session identifier) and that the rejection is specifically about members already being paid rather than just that a spy ran, so replace the generic spy assertion with an assertion on the error type/message that matches the "paid member" rejection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/portal/src/components/pages/gift-page.js`:
- Around line 182-183: The CTA is still enabled for tiers that don't support the
currently selected cadence; update the disable logic and purchase handler to
check cadence support so users cannot start checkout for an unavailable
interval. Modify isDisabled (where isPurchasing and isCookiesDisabled are
computed) to also call the tier/cadence support checker used by
GiftProductCardPrice (e.g., add a check like doesTierSupportCadence(tier,
activeCadence) or reuse the same helper) and set isDisabled true when
unsupported, and in handlePurchase (the purchase submission function)
early-return if the cadence is unsupported to guard against direct calls; ensure
you reference the same tier/cadence identifiers used by GiftProductCardPrice so
the UI and logic are consistent.
In `@apps/portal/src/utils/api.js`:
- Around line 542-558: The code currently calls response.json() directly which
can throw on empty or non-JSON responses and bypass the intended !response.ok
error handling; update the logic around response.json() (the responseJson
variable) to parse defensively — e.g., check
response.headers.get('content-type') for application/json before parsing or wrap
response.json() in try/catch and fall back to an empty object (or populate
responseJson.errors if available) so the subsequent !response.ok branch and
final controlled Error('Failed to process gift checkout, please try again.')
execute instead of a raw SyntaxError; ensure the existing checks for
responseJson?.errors?.[0] and responseJson.url and the call to
window.location.assign(responseJson.url) continue to work with the guarded
parse.
---
Nitpick comments:
In
`@ghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.js`:
- Around line 738-749: The test accepts any BadRequestError on lab-flag
disabled; tighten it by asserting the labs flag checked and the error identifies
the giftSubscriptions flag: verify labsService.isSet was called with
'giftSubscriptions' and assert the thrown error.message (or error.context)
contains 'giftSubscriptions' or a specific tag indicating the lab is disabled
when invoking createCheckoutSession on the controller returned by
createGiftController; likewise, in the paid-member test (the other test at
~780-801) assert the authenticated context is forwarded by checking the
controller.createCheckoutSession path receives/uses the expected auth user
(e.g., the ctx.user or session identifier) and that the rejection is
specifically about members already being paid rather than just that a spy ran,
so replace the generic spy assertion with an assertion on the error type/message
that matches the "paid member" rejection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f4936cb-d684-491d-a164-1f852b318e12
📒 Files selected for processing (11)
apps/portal/package.jsonapps/portal/src/actions.jsapps/portal/src/components/pages/gift-page.jsapps/portal/src/utils/api.jsapps/portal/test/actions.test.tsghost/core/core/server/services/members/members-api/controllers/router-controller.jsghost/core/core/server/services/members/members-api/services/payments-service.jsghost/core/core/server/services/stripe/stripe-api.jsghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.jsghost/core/test/unit/server/services/members/members-api/services/payments-service.test.jsghost/core/test/unit/server/services/stripe/stripe-api.test.js
✅ Files skipped from review due to trivial changes (3)
- apps/portal/package.json
- apps/portal/test/actions.test.ts
- ghost/core/test/unit/server/services/stripe/stripe-api.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/services/members/members-api/controllers/router-controller.js
mike182uk
force-pushed
the
BER-3484-stripe-checkout
branch
from
57e1d3d to
76f0188
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ghost/core/core/server/services/members/members-api/services/payments-service.js (1)
187-199:⚠️ Potential issue | 🟡 MinorAlign email fallback for Stripe prefill and metadata.
When
member.get('email')exists, bothmetadata.purchaser_emailandcustomerEmailcurrently miss that fallback. This can reduce checkout prefill reliability and metadata completeness.Proposed fix
metadata: { ...metadata, ghost_gift: 'true', gift_token: token, tier_id: tier.id.toHexString(), cadence, - purchaser_email: email + purchaser_email: email || (member && member.get('email')) || '' }, successUrl: successUrlObj.toString(), cancelUrl, customer, - customerEmail: !customer ? email : null + customerEmail: !customer ? (email || (member && member.get('email')) || null) : null };🤖 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 187 - 199, The purchaser_email metadata and customerEmail field should fall back to member.get('email') when the local email variable is empty; update the object construction that sets metadata.purchaser_email and customerEmail to use (email || member.get('email')) so metadata.purchaser_email, customerEmail and the Stripe prefill are consistent (refer to metadata.purchaser_email, customerEmail, tier.id.toHexString(), token, cadence and member.get in payments-service.js).
🧹 Nitpick comments (1)
ghost/core/core/server/services/members/members-api/services/payments-service.js (1)
165-166: Update return type annotation to match actual return value.
getGiftPaymentLinkreturnssession.url(a string), but JSDoc declaresPromise<URL>. This can mislead callers and type tooling.Proposed fix
- * `@returns` {Promise<URL>} + * `@returns` {Promise<string>}Also applies to: 203-203
🤖 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 165 - 166, The JSDoc for getGiftPaymentLink incorrectly declares the return as Promise<URL> while the function returns session.url (a string); update the JSDoc return annotation for getGiftPaymentLink (and the other similar annotation in the same file) from Promise<URL> to Promise<string> so types reflect the actual return value and toolchains won't be misled.
🤖 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/core/core/server/services/members/members-api/services/payments-service.js`:
- Around line 187-199: The purchaser_email metadata and customerEmail field
should fall back to member.get('email') when the local email variable is empty;
update the object construction that sets metadata.purchaser_email and
customerEmail to use (email || member.get('email')) so metadata.purchaser_email,
customerEmail and the Stripe prefill are consistent (refer to
metadata.purchaser_email, customerEmail, tier.id.toHexString(), token, cadence
and member.get in payments-service.js).
---
Nitpick comments:
In
`@ghost/core/core/server/services/members/members-api/services/payments-service.js`:
- Around line 165-166: The JSDoc for getGiftPaymentLink incorrectly declares the
return as Promise<URL> while the function returns session.url (a string); update
the JSDoc return annotation for getGiftPaymentLink (and the other similar
annotation in the same file) from Promise<URL> to Promise<string> so types
reflect the actual return value and toolchains won't be misled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77015d5f-c1c8-48b4-8bfd-1220ff21a0bc
📒 Files selected for processing (11)
apps/portal/package.jsonapps/portal/src/actions.jsapps/portal/src/components/pages/gift-page.jsapps/portal/src/utils/api.jsapps/portal/test/actions.test.tsghost/core/core/server/services/members/members-api/controllers/router-controller.jsghost/core/core/server/services/members/members-api/services/payments-service.jsghost/core/core/server/services/stripe/stripe-api.jsghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.jsghost/core/test/unit/server/services/members/members-api/services/payments-service.test.jsghost/core/test/unit/server/services/stripe/stripe-api.test.js
✅ Files skipped from review due to trivial changes (2)
- apps/portal/package.json
- apps/portal/test/actions.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/portal/src/actions.js
- ghost/core/test/unit/server/services/members/members-api/controllers/router-controller.test.js
- ghost/core/test/unit/server/services/stripe/stripe-api.test.js
- ghost/core/test/unit/server/services/members/members-api/services/payments-service.test.js
- ghost/core/core/server/services/members/members-api/controllers/router-controller.js
- ghost/core/core/server/services/stripe/stripe-api.js
ref https://linear.app/ghost/issue/BER-3484 Wired up the gift subscription checkout end-to-end
mike182uk
force-pushed
the
BER-3484-stripe-checkout
branch
from
76f0188 to
e2ad21b
Compare
sagzy
approved these changes
Apr 6, 2026
| async createGiftCheckoutSession({amount, currency, tierName, cadence, metadata, successUrl, cancelUrl, customer, customerEmail}) { | ||
| await this._rateLimitBucket.throttle(); | ||
|
|
||
| const cadenceLabel = cadence === 'month' ? '1 month' : '1 year'; |
Contributor
There was a problem hiding this comment.
Should we pass cadence and duration to this method, so that extending to other durations such as 3 mo / 6 mo just works?
Member
Author
There was a problem hiding this comment.
Yeh this is probably a good shout, just means having to pass duration through the call chain, but no biggie
|
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
Wired up the gift subscription checkout end-to-end