Skip to content

W-21372336: Fail Order for redirect based payments#3707

Merged
rasbhat merged 3 commits intot/team404/sfp-on-pwafrom
rvishwanathbhat/fail-order-on-redirect
Mar 3, 2026
Merged

W-21372336: Fail Order for redirect based payments#3707
rasbhat merged 3 commits intot/team404/sfp-on-pwafrom
rvishwanathbhat/fail-order-on-redirect

Conversation

@rasbhat
Copy link

@rasbhat rasbhat commented Mar 3, 2026

Description

When we have a failed order on redirect based payments, checkout page just kept loading indefinitely.

Cause:
Previously, the code always called failOrder when payment failed or the return URL was invalid. But this caused an issue when either payment already failed on provider's end, or a webhook already failed it : the user landed on the checkout page, and tried to fail the same order again which caused the load indefinitely.

Fix: We try to fail an order if the order is still in a state we can fail. We also attempt to fail the order only once per page load (guardrail against multiple failOrders)

Testing:

  1. Tested failOrder with 3DS declined card:
Screenshot 2026-03-03 at 9 47 38 AM
  1. Tested Afterpay declined transaction:
Screenshot 2026-03-03 at 8 21 29 AM

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • (change1)

How to Test-Drive This PR

  • (step1)

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@rasbhat rasbhat requested a review from a team as a code owner March 3, 2026 15:38
@cc-prodsec
Copy link
Collaborator

cc-prodsec commented Mar 3, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.


const isError = !isValidReturnUrl()
const isHandled = useRef(false)
const failOrderCalledRef = useRef(false)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's an easy way to combine this with isHandled I think it would be more readable. If no easy way, then we can save that for a later refactor.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it. Seems like isHandled is already handling it. failOrderCalledRef was redundant. removed it.

}
})
} catch {
// Order may already be failed by webhook; avoid hanging
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we only have a catch block to contain the comment describing why we don't need a catch block?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need the catch to handle a few scenarios for us to avoid hanging. Listed the possible scenarios in a comment here.

// Navigate back to the checkout page to try again
navigate('/checkout')
// Redirect to cart when payment fails
navigate('/cart')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we hit this case it's because a gateway redirected the shopper back to the storefront, and the payment failed. We would like to navigate the shopper back to the start of the redirect which is the checkout page.

Copy link
Author

@rasbhat rasbhat Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, reverted back to navigate to checkout. Updated the screenshot in description that reflects it.

expect(mockNavigate).toHaveBeenCalledWith('/cart')
})

test('does not call failOrder when order already failed by webhook (e.g. 3DS declined)', async () => {
Copy link

@sf-mkosak sf-mkosak Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a test for scenario of the race condition that I think the catch block covers in the component code? e.g. the failOrder API fails because it's already in failed state.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like that might be a scenario too! Good catch! Listed it in the catch block comment and added a test for the same.


try {
const token = await getTokenWhenReady()
const currentOrder = await api.shopperOrders.getOrder({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a useOrder hook. Can we use that or is specific way to do it the way you are doing it here? Having to get and set bearer token in the component seems off

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. used useOrder. I was referring to express payments implementataion, where token is manually set.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If express is wrong then should we update it?

@rasbhat rasbhat force-pushed the rvishwanathbhat/fail-order-on-redirect branch from 323c664 to 5f0e22f Compare March 3, 2026 20:31
}
})
} catch (error) {
// Swallow so flow continues (invalidate, navigate). Causes: (1) Race: refetch
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this comment. IMO the fact the framework level design decisions bring us to this scenario where swallowing an error is the right path, implicates those decisions not our code as a consumer. I'm ok with this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there was a way to recover the basket even when fail order fails (and not just when it passes) then it might be consistent. That reopen flag isn't really reopening in a failure case.

The only difference is Express is throwing some toast, so doing nothing in catch is fine here
const basketRecovered = await attemptFailOrder(createdOrderNo)
if (basketRecovered) {
showErrorMessage(ERROR_MESSAGE_KEYS.FAIL_ORDER)
} else {
showErrorMessage(ERROR_MESSAGE_KEYS.ORDER_RECOVERY_FAILED)
if (usage !== EXPRESS_BUY_NOW) {
navigate('/cart')
}
}

@rasbhat rasbhat merged commit 10e8424 into t/team404/sfp-on-pwa Mar 3, 2026
14 of 16 checks passed
@rasbhat rasbhat deleted the rvishwanathbhat/fail-order-on-redirect branch March 3, 2026 22:33
rasbhat added a commit that referenced this pull request Mar 5, 2026
…/fail-order-on-redirect

W-21372336: Fail Order for redirect based payments
rasbhat added a commit that referenced this pull request Mar 5, 2026
…/fail-order-on-redirect

W-21372336: Fail Order for redirect based payments
rasbhat added a commit that referenced this pull request Mar 5, 2026
…/fail-order-on-redirect

W-21372336: Fail Order for redirect based payments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants