✨ Added tax ID collection to Stripe checkout when automatic tax is enabled#24682
Conversation
|
Caution Review failedThe head commit changed during the review from 7a07475d133d87acdd0a34666d399b8cca2e53dd to 7985934. WalkthroughAdded a private helper _applyAutomaticTaxSessionOptions in StripeAPI.js to centralize automatic-tax handling. It enables tax_id_collection when automatic tax is on and, if a customer is present, sets customer_update to include address: 'auto' and name: 'auto'. createCheckoutSession and createDonationCheckoutSession were updated to call this helper instead of inlining the logic. Unit tests in StripeAPI.test.js were expanded/renamed to assert customer_update now includes name and address when applicable and that tax_id_collection is present when enableAutomaticTax is true and absent when false across checkout and donation flows. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
🧹 Nitpick comments (3)
ghost/core/core/server/services/stripe/StripeAPI.js (1)
579-586: Consider extracting an “automatic tax options” helper to reduce duplicationThe same conditional for customer_update and tax_id_collection appears in both flows. A tiny helper would DRY this up and centralize future tweaks.
Apply within these ranges:
- if (customerId && this._config.enableAutomaticTax) { - stripeSessionOptions.customer_update = {address: 'auto', name: 'auto'}; - } - - // Enable tax ID collection when automatic tax is enabled - if (this._config.enableAutomaticTax) { - stripeSessionOptions.tax_id_collection = {enabled: true}; - } + this._applyAutomaticTaxSessionOptions(stripeSessionOptions, {hasCustomer: Boolean(customerId)});- if (customer && this._config.enableAutomaticTax) { - stripeSessionOptions.customer_update = {address: 'auto', name: 'auto'}; - } - - // Enable tax ID collection when automatic tax is enabled - if (this._config.enableAutomaticTax) { - stripeSessionOptions.tax_id_collection = {enabled: true}; - } + this._applyAutomaticTaxSessionOptions(stripeSessionOptions, {hasCustomer: Boolean(customer)});Add the helper once inside the class (outside the selected ranges):
/** * Apply Automatic Tax related session options consistently. * @param {object} stripeSessionOptions * @param {{hasCustomer: boolean}} ctx */ _applyAutomaticTaxSessionOptions(stripeSessionOptions, {hasCustomer}) { if (!this._config.enableAutomaticTax) { return; } stripeSessionOptions.tax_id_collection = {enabled: true}; if (hasCustomer) { stripeSessionOptions.customer_update = {address: 'auto', name: 'auto'}; } }Also applies to: 660-667
ghost/core/test/unit/server/services/stripe/StripeAPI.test.js (2)
642-663: Add donation test for customer_update to match production behaviorProduction sets customer_update = {address: 'auto', name: 'auto'} for donations when a customer is present and Automatic Tax is enabled. A complementary test would prevent regressions.
Suggested test addition (place within the “createDonationCheckoutSession” describe):
+ it('createDonationCheckoutSession adds customer_update when automatic tax is enabled and customer is provided', async function () { + api.configure({ + checkoutSessionSuccessUrl: '/success', + checkoutSessionCancelUrl: '/cancel', + checkoutSetupSessionSuccessUrl: '/setup-success', + checkoutSetupSessionCancelUrl: '/setup-cancel', + secretKey: '', + enableAutomaticTax: true + }); + + const mockCustomer = { + id: mockCustomerId, + email: mockCustomerEmail, + name: mockCustomerName + }; + + await api.createDonationCheckoutSession({ + priceId: 'priceId', + successUrl: '/success', + cancelUrl: '/cancel', + metadata: {}, + customer: mockCustomer + }); + + should.exist(mockStripe.checkout.sessions.create.firstCall.firstArg.customer_update); + should.deepEqual( + mockStripe.checkout.sessions.create.firstCall.firstArg.customer_update, + {address: 'auto', name: 'auto'} + ); + });
374-404: Nit: misleading describe nameThe block is named “createCheckoutSetupSession automatic tax flag” but exercises createCheckoutSession. Consider renaming for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ghost/core/core/server/services/stripe/StripeAPI.js(2 hunks)ghost/core/test/unit/server/services/stripe/StripeAPI.test.js(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ghost/core/test/unit/server/services/stripe/StripeAPI.test.js (1)
ghost/core/core/server/services/stripe/StripeService.js (1)
api(55-55)
🔇 Additional comments (9)
ghost/core/core/server/services/stripe/StripeAPI.js (4)
579-581: Customer auto-update now includes name — LGTMAdding name: 'auto' alongside address: 'auto' ensures Checkout can persist both fields to the customer when Automatic Tax is enabled and a customer is present.
660-663: Donation flow customer auto-update — LGTMMirroring the subscription path by setting customer_update to capture both address and name is correct and aligns the behavior across flows.
664-667: Donation flow tax ID collection — LGTMCollecting tax IDs for donations under Automatic Tax is consistent with the subscription path and the PR objective.
583-586: tax_id_collection Parameter Supported in API v2020-08-27Confirmed that Stripe Checkout Sessions on API version 2020-08-27 accept
"tax_id_collection": { "enabled": true }as documented. No changes are required—your implementation is compatible as-is.
ghost/core/test/unit/server/services/stripe/StripeAPI.test.js (5)
415-416: Expectation updated to include name in customer_update — LGTMThe assertion now correctly checks for both address and name auto-updates when Automatic Tax is enabled.
446-459: createCheckoutSession: tax_id_collection present when Automatic Tax enabled — LGTMGood coverage to ensure the Checkout Session creation args include tax_id_collection under the flag.
460-479: createCheckoutSession: tax_id_collection absent when Automatic Tax disabled — LGTMNegative-path coverage looks correct.
642-663: createDonationCheckoutSession: tax_id_collection present when Automatic Tax enabled — LGTMGreat to see donation coverage mirroring the subscription flow.
665-685: createDonationCheckoutSession: tax_id_collection absent when Automatic Tax disabled — LGTMNegative-path coverage looks good here as well.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
ghost/core/test/unit/server/services/stripe/StripeAPI.test.js (2)
491-493: Bug: the labs stub isn’t assigned tomockLabsIsSet, making later.withArgs(...)calls no-opsIn this describe block you do
sinon.stub(mockLabs, 'isSet')but don’t assign it tomockLabsIsSet. Subsequent uses (e.g., Line 515) callmockLabsIsSet.withArgs(...), which ends up configuring a different (restored) stub orundefined, not the active one attached tomockLabs.isSet. This can cause subtle, flaky behavior and defeats the intent of selectively configuring returns per flag.Fix by assigning the stub to
mockLabsIsSet:- sinon.stub(mockLabs, 'isSet'); + mockLabsIsSet = sinon.stub(mockLabs, 'isSet');
374-398: Add tests for labs-disabled +enableAutomaticTax: trueTo fully cover the gating logic, add a scenario where the Stripe Automatic Tax Labs flag is off while
enableAutomaticTaxis true—bothcustomer_updateandtax_id_collectionshould not be set.• File:
ghost/core/test/unit/server/services/stripe/StripeAPI.test.js
Under the existing
describe('createCheckoutSession automatic tax flag', ...)
add:it('does not add automatic-tax fields when labs flag is disabled', async function () { // Labs flag OFF, config ON mockLabsIsSet.withArgs('stripeAutomaticTax').returns(false); api.configure({ checkoutSessionSuccessUrl: '/success', checkoutSessionCancelUrl: '/cancel', checkoutSetupSessionSuccessUrl: '/setup-success', checkoutSetupSessionCancelUrl: '/setup-cancel', secretKey: '', enableAutomaticTax: true }); const mockCustomer = {id: mockCustomerId, customer_email: mockCustomerEmail, name: 'Example Customer'}; await api.createCheckoutSession('priceId', mockCustomer, {trialDays: null}); const args = mockStripe.checkout.sessions.create.firstCall.firstArg; should.not.exist(args.customer_update); should.not.exist(args.tax_id_collection); });• (Optional) Mirror this test in your
describeblock forcreateDonationCheckoutSession, asserting thattax_id_collectionis omitted when the Labs flag is off butenableAutomaticTaxis true.
🧹 Nitpick comments (4)
ghost/core/test/unit/server/services/stripe/StripeAPI.test.js (4)
404-416: Rename test for clarity: require a customer with an ID, not just “not undefined”The code path adds
customer_updateonly when a Stripe customer ID is present. The current title says “customer is not undefined,” which could misleadingly include objects without anid. Consider renaming to “... and customer has an ID”.- it('createCheckoutSession adds customer_update if automatic tax flag is enabled and customer is not undefined', async function () { + it('createCheckoutSession adds customer_update if automatic tax is enabled and customer has an ID', async function () {
642-671: Donation flow: also stub the Labs gate forstripeAutomaticTaxTo align with the PR objective (“when the stripeAutomaticTax feature flag is enabled”), explicitly set the Labs flag in this test, similar to the checkout tests. Right now this test relies only on
enableAutomaticTax, which doesn’t exercise the Labs gate.api.configure({ checkoutSessionSuccessUrl: '/success', checkoutSessionCancelUrl: '/cancel', checkoutSetupSessionSuccessUrl: '/setup-success', checkoutSetupSessionCancelUrl: '/setup-cancel', secretKey: '', enableAutomaticTax: true }); + mockLabsIsSet.withArgs('stripeAutomaticTax').returns(true);
673-694: Donation flow: assert Labs gate alongsideenableAutomaticTaxSame rationale as the previous comment: explicitly set the Labs flag to make the gating condition explicit and aligned with the feature flag semantics.
api.configure({ checkoutSessionSuccessUrl: '/success', checkoutSessionCancelUrl: '/cancel', checkoutSetupSessionSuccessUrl: '/setup-success', checkoutSetupSessionCancelUrl: '/setup-cancel', secretKey: '', enableAutomaticTax: true }); + mockLabsIsSet.withArgs('stripeAutomaticTax').returns(true);
696-716: Add a test: Labs flag off whileenableAutomaticTaxtrue should not addtax_id_collectionTo guard against regressions where the presence of the config might bypass the Labs gate, add a negative test like this:
it('createDonationCheckoutSession does not add tax_id_collection when Labs flag is disabled, even if enableAutomaticTax is true', async function () { mockLabsIsSet.withArgs('stripeAutomaticTax').returns(false); api.configure({ checkoutSessionSuccessUrl: '/success', checkoutSessionCancelUrl: '/cancel', checkoutSetupSessionSuccessUrl: '/setup-success', checkoutSetupSessionCancelUrl: '/setup-cancel', secretKey: '', enableAutomaticTax: true }); await api.createDonationCheckoutSession({ priceId: 'priceId', successUrl: '/success', cancelUrl: '/cancel', metadata: {}, customer: null, customerEmail: mockCustomerEmail }); should.not.exist(mockStripe.checkout.sessions.create.firstCall.firstArg.tax_id_collection); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ghost/core/core/server/services/stripe/StripeAPI.js(3 hunks)ghost/core/test/unit/server/services/stripe/StripeAPI.test.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/services/stripe/StripeAPI.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: E2E tests
- GitHub Check: Ghost-CLI tests
- GitHub Check: Acceptance tests (Node 22.13.1, sqlite3)
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Legacy tests (Node 22.13.1, mysql8)
- GitHub Check: Legacy tests (Node 22.13.1, sqlite3)
- GitHub Check: Lint
- GitHub Check: i18n
🔇 Additional comments (2)
ghost/core/test/unit/server/services/stripe/StripeAPI.test.js (2)
446-459: LGTM: asserts presence and shape oftax_id_collectionThis accurately validates that when automatic tax is enabled, the payload includes
tax_id_collection: {enabled: true}.
460-479: LGTM: verifiestax_id_collectionis omitted when disabledGood negative test to ensure no accidental inclusion.
betschki
force-pushed
the
add-vat-id-to-tax-beta
branch
from
6753e89 to
b01d5e1
Compare
|
@betschki Hey, could you rebase this? You have maintainer edits disabled. |
…abled no ref - enables tax_id_collection on the subscription, donation, and gift checkout sessions when the stripeAutomaticTax flag is on, so customers can enter a VAT/tax ID for proper tax compliance - centralised the automatic-tax session options behind a single _applyAutomaticTaxSessionOptions helper to remove the duplicated customer_update logic across the three checkout flows - customer_update now syncs both address and name so Stripe keeps the customer's tax record accurate
betschki
force-pushed
the
add-vat-id-to-tax-beta
branch
from
7985934 to
c6f6ee6
Compare
|
@9larsons done :) |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 3m 7s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 2m 35s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 2m 58s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 17s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 31s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 8s | View ↗ |
nx run-many -t lint -p ghost |
✅ Succeeded | 26s | View ↗ |
Additional runs (2) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-08 15:25:29 UTC
3 participants
Enables tax ID input field in Stripe checkout sessions when the stripeAutomaticTax feature flag is enabled. This allows for proper tax compliance and tax ID collection during subscription and donation checkout flows.