-
Notifications
You must be signed in to change notification settings - Fork 4
fix: poll checkout intent API within billing details #75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request enhances the checkout subscription flow by implementing real-time polling for checkout intent states and improving payment success detection. The changes ensure more reliable handling of successful payment states in both UI components and route loaders.
- Introduces
usePolledCheckoutIntenthook for real-time checkout intent state monitoring - Adds
determineExistingSuccessfulCheckoutIntentutility to standardize successful state checking - Updates billing details success loader with explicit success validation and redirect logic
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| StatefulSubscribeButton.tsx | Integrates polling logic and success state detection for real-time payment completion handling |
| checkoutStepperLoader.ts | Enhances billing details loader with explicit success validation and redirect logic |
| context.ts | Exports new utility function for checkout intent success determination |
| StatefulSubscribeButton.test.tsx | Updates test mocks to include polled checkout intent and removes invalidation expectation |
| BillingDetailsPage.test.tsx | Adds mock for new usePolledCheckoutIntent hook in test setup |
Comments suppressed due to low confidence (2)
src/components/StatefulButton/StatefulSubscribeButton.tsx:1
- The line that sets the stateful button state based on the response type was removed, but this is needed to handle all response types properly. Without this line, the button state won't be updated for responses other than 'error'.
import { defineMessages, useIntl } from '@edx/frontend-platform/i18n';
src/components/StatefulButton/StatefulSubscribeButton.tsx:1
- The query invalidation logic was removed, but this is necessary to refresh cached data after a successful payment. Without this, the application may display stale data.
import { defineMessages, useIntl } from '@edx/frontend-platform/i18n';
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| !determineExistingSuccessfulCheckoutIntent(checkoutIntent.state) | ||
| && checkoutIntentType !== 'complete' | ||
| && !checkoutIntent.existingSuccessfulCheckoutIntent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Logical expression should be chained using || instead of &&. Also, isn't the first and last element performing the same check?
| !determineExistingSuccessfulCheckoutIntent(checkoutIntent.state) | |
| && checkoutIntentType !== 'complete' | |
| && !checkoutIntent.existingSuccessfulCheckoutIntent | |
| !determineExistingSuccessfulCheckoutIntent(checkoutIntent.state) | |
| || stripeCheckoutSessionType !== 'complete' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was, so we can probably stick to !checkoutIntent.existingSuccessfulCheckoutIntent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also remain as an && since we map the checkout intent state to the stripe response in the rootLoader.
See populateInitialApplicationState.
Basically this can be reverted to the original implementation.
92cfc69 to
fd7807f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| response = error; | ||
| } | ||
| // Set the button to the appropriate state based on the response. | ||
| // Stripe responses map 1:1 to button states except for 'default' which is the initial state. |
Copilot
AI
Oct 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line setStatefulButtonState(response.type || 'default'); was removed but the button state is no longer being set for non-error responses. This could leave the button in an incorrect state for successful or other response types.
| // Stripe responses map 1:1 to button states except for 'default' which is the initial state. | |
| // Stripe responses map 1:1 to button states except for 'default' which is the initial state. | |
| setStatefulButtonState(response.type || 'default'); |
| useEffect(() => { | ||
| if (statefulButtonState === 'success') { | ||
| // If the payment succeeded, update the checkout session status. | ||
| // If the payment succeeded from the stripe API, update the checkout session status. |
Copilot
AI
Oct 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment should use 'Stripe' (capitalized) instead of 'stripe' as it's a proper noun referring to the Stripe API.
| // If the payment succeeded from the stripe API, update the checkout session status. | |
| // If the payment succeeded from the Stripe API, update the checkout session status. |
| stripeCheckoutSessionType !== 'complete' | ||
| && !checkoutIntent.existingSuccessfulCheckoutIntent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I still think this should be || because the body represents a failure case. "if either stripe or edx thinks the payment is unsuccessful, don't proceed".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I had to wrack my brain out this for a second to really think it through.
TLDR for future me:
For evaluating failure cases, use || conditions checking for all failure cases
For evaluating success cases, use && conditions checking for all passing cases.
| checkoutIntentId: checkoutIntent?.id ?? null, | ||
| eventName: EVENT_NAMES.SUBSCRIPTION_CHECKOUT.PAYMENT_PROCESSED_SUCCESSFULLY, | ||
| }); | ||
| navigate(CheckoutPageRoute.BillingDetailsSuccess); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ We can't call this navigate if the polledCheckoutIntent has not yet succeeded. If we do, then we run the risk of the success page loader redirecting away. This useEffect needs to incorporate the polledCheckoutIntent into it's logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just noticing that you're gating this whole useEffect on statefulButtonState === 'success'. I get that protects the nagivate(), but it's weird because the button will change appearance to look successful before we even check the stripe checkout session status.
IMO, the logic to perform the tracking event/button appearance/navigate should all be in the same 3 consecutive lines, likely in a single useEffect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see this response.
https://github.com/edx/frontend-app-enterprise-checkout/pull/75/files#r2420280200
| // Visually alter the Subscribe button to a "successful" appearance if the polled intent state becomes successful. | ||
| useEffect(() => { | ||
| if (statefulButtonState === 'pending' && determineExistingSuccessfulCheckoutIntent(polledCheckoutIntent?.state)) { | ||
| setStatefulButtonState('success'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, we can't just set the button appearance to look successful here because we aren't yet sure if the stripe checkout session status has type=complete and paymentStatus=paid yet, as it would be misleading to the admin that everything completed successfully. This is another reason to combine the two useEffects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please observe of how the Stripe API responds versus the status.
This additional change requested creates an additional delay to wait for the checkout intent to return a "paid", "errored_provisioning" or "fulfilled" state. The initial implementation was always waiting for a successful response from the stripe API indicating that the status was updated to success and therefore trigger a useEffect to update with the now latest status information AFTER a successful payment from Stripe.
This change just waits for the checkout intent to now reflect a paid state before setting a successful button state to complete the checkout experience.
Should any doubts remain, I encourage you to check out the branch and validate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please observe of how the Stripe API responds versus the status.
Yes, the screenshot shows what I expect---initial status from the Stripe API does not yet show paid, then eventually becomes paid after a few moments.
This additional change requested creates an additional delay to wait for the checkout intent to return a "paid", "errored_provisioning" or "fulfilled" state.
Yes, this is the goal. We need to wait potentially longer for not just Stripe APIs to show paid, but for polled checkout-intent APIs to also show paid.
The initial implementation was always waiting for a successful response from the stripe API indicating that the status was updated to success and therefore trigger a useEffect to update with the now latest status information AFTER a successful payment from Stripe. This change just waits for the checkout intent to now reflect a paid state before setting a successful button state to complete the checkout experience.
This PR introduces drift between actual/backoffice success and visual success. Let me demonstrate with event tables:
Here's what we had before this PR. Note the possibility of navigating to the success page BEFORE confirming that the checkout intent has transitioned to paid.
| event | button appearance after event | Stripe Checkout Session Status | Enterprise Checkout Intent State |
|---|---|---|---|
| enter credit card info | Subscribe | ??? | created |
| click subscribe | Processing | ??? | created |
| stripe API: session completed | Processing | success/unpaid | created |
| stripe API: paid | Success | complete/paid | created |
| navigate to success page | N/A | complete/paid | created (Bad!) |
Below is what this PR currently does. Note that the appearance of the button changes possibly BEFORE stripe API payment completion.
| event | button appearance after event | Stripe Checkout Session Status | Enterprise Checkout Intent State |
|---|---|---|---|
| enter credit card info | Subscribe | ??? | created |
| click subscribe | Processing | ??? | created |
| enterprise-access API: paid | Success (Bad!) | ??? | paid |
| stripe API: session completed | Success | success/unpaid | paid |
| stripe API: paid | Success | complete/paid | paid |
| navigate to success page | N/A | complete/paid | paid |
Here's what I think we need. Note that the button appearance only changes to "Success" once both stripe and enterprise APIs indicate payment success. Since they both can happen in any order, my proposal has been to combine them into one single useEffect to handle the joint logic of checking both before performing button state changes and navigate() calls.
| event | button appearance after event | Stripe Checkout Session Status | Enterprise Checkout Intent State |
|---|---|---|---|
| enter credit card info | Subscribe | ??? | created |
| click subscribe | Processing | ??? | created |
| stripe API: session completed | Processing | success/unpaid | created |
| stripe API: paid | Processing | complete/paid | created |
| enterprise-access API: paid | Success | complete/paid | paid |
| navigate to success page | N/A | complete/paid | paid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/components/StatefulButton/StatefulSubscribeButton.tsx:1
- The line setting
statefulButtonStatebased onresponse.typewas removed, but the logic that handles error states on lines 108-114 still expects this state to be set. This could cause the button to remain in an incorrect state when errors occur.
import { defineMessages, useIntl } from '@edx/frontend-platform/i18n';
src/components/StatefulButton/StatefulSubscribeButton.tsx:1
- The query invalidation logic was removed, but this may be necessary to refresh cached data after a successful payment. Consider whether this invalidation should be moved elsewhere or if the polling mechanism adequately replaces this functionality.
import { defineMessages, useIntl } from '@edx/frontend-platform/i18n';
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
brobro10000
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon merging main into this PR (after comments given) I noticed an infinite re-rendering bug happening related to these changes.
Deferring on this work to focus on MVP critical work. here is a branching PR with related work to these changes.
#79
This pull request enhances the checkout subscription flow by improving how successful payment states are detected and handled, both in the UI and route loaders. It introduces polling for the checkout intent's state, adds a utility to determine successful states, and ensures the UI and navigation logic respond more reliably to payment completion. Test coverage is updated to reflect these changes.
Screen.Recording.2025-10-08.at.10.58.44.PM.mov
Checkout Intent State Handling:
usePolledCheckoutIntenthook to poll for updates to the checkout intent state, enabling real-time detection of successful payments inStatefulSubscribeButtonand related tests. [1] [2] [3] [4]determineExistingSuccessfulCheckoutIntentutility to standardize checking for successful checkout intent states, and integrated it into both button logic and route loaders. [1] [2] [3] [4]UI and Navigation Logic:
StatefulSubscribeButtonto set its state to 'success' when a successful payment is detected via polling, improving responsiveness to payment completion. [1] [2]billingDetailsSuccessLoaderto explicitly check for successful intent states and redirect to the plan details page if payment is not complete, ensuring users are not stuck in incomplete states.Test Updates:
StatefulSubscribeButtonandBillingDetailsPageto mock and validate the new polling and success detection logic. [1] [2]