Conversation
a29d08a to
5ab6773
Compare
| } | ||
| } | ||
|
|
||
| // If Adaptive Pricing is not enabled, or if there was an error loading the AP elements, |
malithsen
left a comment
There was a problem hiding this comment.
Looks good. Left a couple of questions/suggestions, none of them are blocking.
| * @return void | ||
| */ | ||
| public function attach_currency_selector_element() { | ||
| if ( ! is_checkout() ) { |
There was a problem hiding this comment.
Should we also bail out if WC_Stripe_Feature_Flags::is_checkout_sessions_available() returns false
so that we don't attach this element when not supported.
There was a problem hiding this comment.
Good call. I will add the check. 👍
| */ | ||
| const ENDPOINT = 'https://api.stripe.com/v1/'; | ||
| const STRIPE_API_VERSION = '2024-06-20'; | ||
| const STRIPE_API_VERSION = '2025-09-30.clover'; |
There was a problem hiding this comment.
This seems like a significant update that might need its own PR, given that it will affect all payment flows (not just checkout sessions), but e2es seem to pass, so it looks like all is well.
There was a problem hiding this comment.
You are absolutely correct in calling this out. We have a separate PR for updating the Stripe API and JS versions #4977. I have included the changes here as well for testing. I will merge the other PR first to ensure there are no regressions.
| createStripe( key, locale, betas = [] ) { | ||
| const options = { | ||
| locale, | ||
| apiVersion: this.options.apiVersion, |
There was a problem hiding this comment.
Just to double-check, is this removal intentional?
There was a problem hiding this comment.
This is intentional. As we are using const STRIPE_API_VERSION = '2025-09-30.clover'; now, we do not need to pass the apiVersion here anymore. Also this param is unsupported when we are using the clover version.
| } | ||
|
|
||
| if ( forCheckoutSession ) { | ||
| return { |
There was a problem hiding this comment.
nit: Personally, I prefer to keep the return statements clean and extract this object into a variable, but that's mostly a stylistic preference and not a blocker.
📝 WalkthroughWalkthroughDetects adaptive pricing support, exposes flag to JS, adds a classic-checkout currency selector, alters PaymentElement initialization to prefer a Checkout-session adaptive-pricing flow with fallback to Elements, updates default-value handling for checkout sessions, and switches Stripe JS/API to the Clover bundle/version. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant CheckoutJS as Classic UPE JS
participant Server
participant Stripe
Browser->>CheckoutJS: Load checkout page (isAdaptivePricingSupported flag)
CheckoutJS->>Server: request createCheckoutSession (adaptive pricing)
Server->>Stripe: create Checkout Session (AP enabled)
Stripe-->>Server: session(client_secret)
Server-->>CheckoutJS: return client_secret
CheckoutJS->>Stripe: initCheckout(client_secret) / enable adaptivePricing
alt Checkout-session success
CheckoutJS->>CheckoutJS: merge default values forCheckoutSession
CheckoutJS->>Browser: mount PaymentElement + currency selector
else Failure or AP not supported
CheckoutJS->>Browser: load standard Stripe Elements, mount payment element
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@client/classic/upe/payment-processing.js`:
- Around line 260-277: The currency selector mount is called unguarded in the
else branch and will throw if the container (`#wc-stripe-currency-selector`) is
missing; update the code around the elements.createPaymentElement branch to
check for the presence of the container before calling
mountCurrencySelectorElement (or change mountCurrencySelectorElement to no-op if
container is absent). Specifically, in the else branch that creates
createdStripePaymentElement via
elements.createPaymentElement(paymentElementOptions), add a guard (e.g.,
document.querySelector or getElementById check) before invoking
mountCurrencySelectorElement(elements) so adaptive pricing on non-checkout pages
doesn't break initialization.
- Around line 84-96: The updatePaymentElementDefaultValues function currently
forces checkout-session defaults by passing a hard-coded flag; change callers to
pass the actual Elements mode flag (shouldLoadStripeElements) instead of true so
non-AP/standard Elements flows can refresh Link defaults when email/phone
change. Locate calls to updatePaymentElementDefaultValues and any direct calls
to getDefaultValues (e.g., the spots around the current uses mentioned: the call
that always passes true on line 233 and the other uses around 230-234 and
286-292) and replace the literal true with the shouldLoadStripeElements variable
(or the equivalent flag used elsewhere) so the default-values selection follows
the active Elements mode. Ensure updatePaymentElementDefaultValues signature
remains the same and that shouldLoadStripeElements is in scope where you update
the calls.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@client/classic/upe/payment-processing.js`:
- Around line 209-223: The current error handling for
api.getStripe().initCheckout is outdated: remove the unnecessary await on
initCheckout (it's now synchronous and returns a checkout object) and replace
the elements.error check with the new loadActions flow — call
checkout.loadActions() and await its result, then if result.type === 'error'
throw result.error to trigger your fallback; update references to elements vs
checkout accordingly (api.getStripe().initCheckout, checkout.loadActions,
result.type/result.error) so errors from Stripe.js Clover are correctly
detected.
🧹 Nitpick comments (1)
client/classic/upe/payment-processing.js (1)
272-289:getUpeSettings()(terms, billing field visibility) is omitted in the AP path — verify this is intentional.The standard path merges
getUpeSettings()(which includestermsandfields.billingDetailshiding) into the payment element options. The AP/checkout-session path skips this entirely. If the checkout session'screatePaymentElementsupports these options, their omission could cause duplicate billing fields or unexpected terms display.If intentional (e.g., checkout sessions manage fields differently), a brief comment would help future readers.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/classic/upe/payment-processing.js (1)
192-315: 🧹 Nitpick | 🔵 TrivialNo test coverage for the new adaptive-pricing code path.
The new AP branch in
createStripePaymentElement(checkout session creation,initCheckout/loadActions, fallback to standard elements, currency selector mounting, and theshouldLoadStripeElementscontrol flow) has no accompanying unit or E2E tests. Per the project coding guidelines, async checkout behavior changes must be paired with targeted automated tests.Key scenarios to cover at minimum:
- AP enabled,
checkoutSessionsCreateSessionsucceeds →initCheckout+loadActionscalled;elements.createPaymentElementused; currency selector mounted.- AP enabled,
checkoutSessionsCreateSessionfails → fallback toapi.getStripe().elements(options).- AP enabled,
loadActions()returns{ type: 'error' }→ fallback triggered.- AP disabled → standard
elements.create('payment', ...)path unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/classic/upe/payment-processing.js` around lines 192 - 315, Add automated tests covering the new adaptive-pricing code path in createStripePaymentElement: write unit tests that mock api.checkoutSessionsCreateSession, api.getStripe().initCheckout (and its loadActions) and api.getStripe().elements to assert behavior for each scenario; specifically test (1) AP enabled and checkoutSessionsCreateSession resolves -> initCheckout and loadActions are called, elements.createPaymentElement is used, and mountCurrencySelectorElement is invoked, (2) AP enabled and checkoutSessionsCreateSession rejects -> fallback to api.getStripe().elements(options) and elements.create('payment', ...) is used, (3) AP enabled and loadActions() returns { type: 'error' } -> fallback to elements from api.getStripe().elements, and (4) AP disabled -> original path uses elements.create('payment', ...) with getDefaultValues/getUpeSettings. For E2E, add a scenario that enables adaptive pricing and asserts the payment element mounts via createPaymentElement and the currency selector appears; for unit tests assert shouldLoadStripeElements toggles and gatewayUPEComponents[ paymentMethodType ].upeElement/elements are set appropriately. Ensure mocks verify initCheckout parameters include clientSecret and adaptivePricing.allowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/classic/upe/payment-processing.js`:
- Around line 197-199: The code assumes api.checkoutSessionsCreateSession()
always returns an object and uses response.data?.client_secret which will throw
if response is null/undefined; update the access to use optional chaining on
response (e.g. response?.data?.client_secret) and add a null-check/early-return
or error handling when clientSecret is undefined so the classic checkout flow
(checkoutSessionsCreateSession and clientSecret usage) gracefully handles a null
response instead of causing a TypeError.
- Around line 192-233: The Adaptive Pricing (AP) initialization block currently
runs whenever isAdaptivePricingSupported is true and can orphan an existing
PaymentIntent for methods that do not support deferred intents; modify the AP
guard to also verify the current payment method supports deferred intents (e.g.,
check a supportsDeferredIntent boolean on the payment method object) before
calling api.checkoutSessionsCreateSession(), initCheckout, or replacing
elements; ensure the condition becomes something like isAdaptivePricingSupported
&& paymentMethod.supportsDeferredIntent so non-deferred methods (BLIK, ACSS)
skip the Checkout Session flow and retain the original elements (so
validateElements() can still call elements.submit()), and preserve the existing
try/catch and fallback to shouldLoadStripeElements = true on error.
- Around line 195-233: The AP flow currently calls
api.checkoutSessionsCreateSession() inside createStripePaymentElement() per
payment method and mounts mountCurrencySelectorElement() per type, causing N
redundant sessions and conflicting currency selector mounts; fix by introducing
a module-level singleton/cache (e.g., a variable like
cachedAdaptivePricingSession or getAdaptivePricingSession()) that on first call
invokes api.checkoutSessionsCreateSession(), stores the response (client_secret
and related session data) and returns it for subsequent
createStripePaymentElement() invocations, update createStripePaymentElement() to
use this shared session instead of calling api.checkoutSessionsCreateSession()
directly, and ensure mountCurrencySelectorElement() is invoked once (guarded by
a flag or by attaching to the shared session) so the currency selector is
created/mounted a single time and updatePaymentElementDefaultValues() updates
the correct shared session element (not always
gatewayUPEComponents.card.upeElement).
---
Outside diff comments:
In `@client/classic/upe/payment-processing.js`:
- Around line 192-315: Add automated tests covering the new adaptive-pricing
code path in createStripePaymentElement: write unit tests that mock
api.checkoutSessionsCreateSession, api.getStripe().initCheckout (and its
loadActions) and api.getStripe().elements to assert behavior for each scenario;
specifically test (1) AP enabled and checkoutSessionsCreateSession resolves ->
initCheckout and loadActions are called, elements.createPaymentElement is used,
and mountCurrencySelectorElement is invoked, (2) AP enabled and
checkoutSessionsCreateSession rejects -> fallback to
api.getStripe().elements(options) and elements.create('payment', ...) is used,
(3) AP enabled and loadActions() returns { type: 'error' } -> fallback to
elements from api.getStripe().elements, and (4) AP disabled -> original path
uses elements.create('payment', ...) with getDefaultValues/getUpeSettings. For
E2E, add a scenario that enables adaptive pricing and asserts the payment
element mounts via createPaymentElement and the currency selector appears; for
unit tests assert shouldLoadStripeElements toggles and gatewayUPEComponents[
paymentMethodType ].upeElement/elements are set appropriately. Ensure mocks
verify initCheckout parameters include clientSecret and adaptivePricing.allowed.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/classic/upe/payment-processing.js (2)
274-295:⚠️ Potential issue | 🔴 Critical
elementsin the AP path is aStripeCheckoutobject —validateElementswill throwTypeErrorat submission.In the AP path,
elements(line 287) is theStripeCheckoutinstance returned byinitCheckout, not a Stripe Elements object. It is stored at line 293 asgatewayUPEComponents[paymentMethodType].elements.When
processPaymentruns (line 598):validateElements(elements)→elements.submit()— butStripeCheckouthas nosubmit()method, onlyconfirm(). This will throwTypeError: elements.submit is not a functionthe moment a shopper attempts payment in AP mode.While the PR acknowledges submission is deferred, this will surface immediately once submission is wired up unless the
processPaymentpath differentiates between the two object types. A guard or TODO comment is needed to prevent silent failures in the next PR.💡 Suggested guard in processPayment
// At the top of the processPayment IIFE, check for AP path + // TODO: AP path (StripeCheckout object) uses checkout.confirm() instead + // of validateElements + createPaymentMethod. Wire up in the next PR. + if ( ! shouldLoadStripeElements ) { + jQueryForm.removeClass( 'processing' ).unblock(); + return; + }Or separate the
elementstype fromcheckoutingatewayUPEComponents:gatewayUPEComponents[ paymentMethodType ].elements = elements; + gatewayUPEComponents[ paymentMethodType ].isCheckoutSession = ! shouldLoadStripeElements;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/classic/upe/payment-processing.js` around lines 274 - 295, The AP path is storing a StripeCheckout instance into gatewayUPEComponents[paymentMethodType].elements which will cause processPayment (which calls validateElements(elements) and then elements.submit()) to throw because StripeCheckout has no submit() method; update the AP branch where createdStripePaymentElement and elements are assigned (the initCheckout/StripeCheckout path) to either (a) store the checkout object under a different key (e.g., gatewayUPEComponents[paymentMethodType].checkout) and keep gatewayUPEComponents[paymentMethodType].elements strictly for Stripe Elements instances, or (b) add a runtime guard in processPayment/validateElements to detect a StripeCheckout object (or absence of submit) and call the correct method (confirm) or return a clear error/TODO comment; adjust references to createdStripePaymentElement, elements, and gatewayUPEComponents accordingly so processPayment only calls submit() on genuine Elements objects.
192-314: 🛠️ Refactor suggestion | 🟠 MajorNo test coverage for the new AP flow branching logic.
The AP path introduces three distinct execution branches (AP succeeds, AP fails → fallback, AP disabled), each affecting element type, currency selector mounting, and default-value binding. As per coding guidelines, behavior changes in
client/**/*.{js,jsx,ts,tsx}require Jest/Playwright coverage.Key paths to cover:
checkoutSessionsCreateSessionrejects → falls back to standard Elements.loadActions()returns{ type: 'error' }→ falls back.- AP succeeds →
gatewayUPEComponents.elementsis aStripeCheckoutobject;createPaymentElementandmountCurrencySelectorElementare called.- Currency selector container absent →
mountCurrencySelectorElementis a no-op.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/classic/upe/payment-processing.js` around lines 192 - 314, Add Jest/Playwright tests covering the three AP branches in the payment-processing flow: (1) simulate checkoutSessionsCreateSession rejecting so the code falls back to stripe.elements and verifies created element is from elements.create('payment'), gatewayUPEComponents[PAYMENT_METHOD_CARD].elements is the standard Elements instance, mountCurrencySelectorElement was not called, and default-value attach (attachDefaultValuesUpdateEvent) binds to billing_email/phone; (2) simulate a successful checkoutSessionsCreateSession but elements.loadActions() resolving to { type: 'error' } and assert the same fallback behavior as (1); and (3) simulate AP success where checkoutSessionsCreateSession returns client_secret, api.getStripe().initCheckout(...) returns a StripeCheckout-like object, elements.createPaymentElement is called, mountCurrencySelectorElement is invoked when container exists (and is a no-op when container absent), and gatewayUPEComponents[PAYMENT_METHOD_CARD].elements is the returned StripeCheckout instance; use mocks/spies for api.checkoutSessionsCreateSession, api.getStripe().initCheckout, elements.loadActions, elements.createPaymentElement, mountCurrencySelectorElement, and attachDefaultValuesUpdateEvent to assert calls and resulting state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/classic/upe/payment-processing.js`:
- Around line 209-225: The current flow calls elements.loadActions() before
mounting Stripe Elements; change the sequence so after calling
api.getStripe().initCheckout(...) you immediately call
elements.createPaymentElement(...) and mount the payment element (and mount
currencySelector) first, then call await elements.loadActions() and handle the
result/error; update code around elements (the result of
api.getStripe().initCheckout), elements.createPaymentElement,
paymentElement.mount, currencySelector.mount, and the existing await
elements.loadActions() so loadActions runs only after the elements are created
and mounted.
- Line 218: getDefaultValues(true) is being spread at the top level of
initCheckout(), placing defaultValues at the root where the Checkout Sessions
API does not accept it; move the returned shape into the elementsOptions for the
checkout elements so payment element-level options receive defaultValues.
Concretely, remove the top-level spread of getDefaultValues(true) in
initCheckout() and instead merge its returned object into the elementsOptions
passed to initCheckout (or into the options used to create/update the
PaymentElement), ensuring getDefaultValues() output is provided as
elementsOptions.defaultValues (so billingDetails like email/phone are used).
- Around line 195-233: The adaptive pricing block guarded by
isAdaptivePricingSupported must also check the payment method's
supportsDeferredIntent flag to avoid abandoning an existing
PaymentIntent/SetupIntent; update the conditional around the AP flow (the block
that calls api.checkoutSessionsCreateSession(), api.getStripe().initCheckout(),
and elements.loadActions()) to run only when both isAdaptivePricingSupported and
supportsDeferredIntent are true (e.g., if (isAdaptivePricingSupported &&
supportsDeferredIntent) { ... }), so non-deferred methods skip the
checkoutSessionsCreateSession() path and preserve the previously created intent.
---
Outside diff comments:
In `@client/classic/upe/payment-processing.js`:
- Around line 274-295: The AP path is storing a StripeCheckout instance into
gatewayUPEComponents[paymentMethodType].elements which will cause processPayment
(which calls validateElements(elements) and then elements.submit()) to throw
because StripeCheckout has no submit() method; update the AP branch where
createdStripePaymentElement and elements are assigned (the
initCheckout/StripeCheckout path) to either (a) store the checkout object under
a different key (e.g., gatewayUPEComponents[paymentMethodType].checkout) and
keep gatewayUPEComponents[paymentMethodType].elements strictly for Stripe
Elements instances, or (b) add a runtime guard in
processPayment/validateElements to detect a StripeCheckout object (or absence of
submit) and call the correct method (confirm) or return a clear error/TODO
comment; adjust references to createdStripePaymentElement, elements, and
gatewayUPEComponents accordingly so processPayment only calls submit() on
genuine Elements objects.
- Around line 192-314: Add Jest/Playwright tests covering the three AP branches
in the payment-processing flow: (1) simulate checkoutSessionsCreateSession
rejecting so the code falls back to stripe.elements and verifies created element
is from elements.create('payment'),
gatewayUPEComponents[PAYMENT_METHOD_CARD].elements is the standard Elements
instance, mountCurrencySelectorElement was not called, and default-value attach
(attachDefaultValuesUpdateEvent) binds to billing_email/phone; (2) simulate a
successful checkoutSessionsCreateSession but elements.loadActions() resolving to
{ type: 'error' } and assert the same fallback behavior as (1); and (3) simulate
AP success where checkoutSessionsCreateSession returns client_secret,
api.getStripe().initCheckout(...) returns a StripeCheckout-like object,
elements.createPaymentElement is called, mountCurrencySelectorElement is invoked
when container exists (and is a no-op when container absent), and
gatewayUPEComponents[PAYMENT_METHOD_CARD].elements is the returned
StripeCheckout instance; use mocks/spies for api.checkoutSessionsCreateSession,
api.getStripe().initCheckout, elements.loadActions,
elements.createPaymentElement, mountCurrencySelectorElement, and
attachDefaultValuesUpdateEvent to assert calls and resulting state.

Towards STRIPE-919
Changes proposed in this Pull Request:
initCheckout.Testing instructions
add/adaptive-pricing-in-classic-checkout-2)Note
Changelog entry
Changelog Entry Comment
Comment
Post merge