Make 'retry' property on errors from checkoutAfterProcessingWithSuccess/Error observers default to true if it's undefined #3261

Aljullu · 2020-10-09T11:11:19Z

Errors in WC Payments were not recoverable because the retry property was not specified in the error response, so it was considered false. This behavior was a bit inconsistent in our codebase, because retry needed to be truthy in CHECKOUT_AFTER_PROCESSING_WITH_SUCCESS, but was allowed to be undefined in CHECKOUT_AFTER_PROCESSING_WITH_ERROR.

This PR updates the Checkout Provider so if retry is undefined, it treats it as if it was true.

How to test the changes in this Pull Request:

Install WC Payments and set it up.
Submit an order using WC Payments with the card number 4000 0027 6000 3184, but in the validation popup, press on Fail authentication or Cancel.
Verify the Place Order button doesn't stay disabled and you can resubmit the order.

Changelog

Dev: errors produced on checkoutAfterProcessingWithSuccess/Error observers no longer need retry: true to allow the user to retry the payment. You can set retry: false if you want to force disabling submitting the checkout form again. In addition, a new shouldRetry function is available from the useEmitResponse hook which enables setting more complex retry rules.

github-actions · 2020-10-09T11:15:24Z

Size Change: +179 B (0%)

Total Size: 1.11 MB

Filename	Size	Change
`build/all-products.js`	36.1 kB	+22 B (0%)
`build/atomic-block-components/add-to-cart-frontend.js`	8.95 kB	+24 B (0%)
`build/atomic-block-components/add-to-cart.js`	7.53 kB	+23 B (0%)
`build/cart-frontend.js`	69.9 kB	+26 B (0%)
`build/cart.js`	38.5 kB	+30 B (0%)
`build/checkout-frontend.js`	85.7 kB	+25 B (0%)
`build/checkout.js`	41.8 kB	+29 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`build/active-filters-frontend.js`	8.82 kB	0 B
`build/active-filters.js`	8.86 kB	0 B
`build/all-products-frontend.js`	31.2 kB	0 B
`build/all-reviews.js`	9.79 kB	0 B
`build/atomic-block-components/add-to-cart~atomic-block-components/button.js`	3.18 kB	0 B
`build/atomic-block-components/add-to-cart~atomic-block-components/image~atomic-block-components/title.js`	335 B	0 B
`build/atomic-block-components/button-frontend.js`	2.02 kB	0 B
`build/atomic-block-components/button.js`	839 B	0 B
`build/atomic-block-components/category-list-frontend.js`	469 B	0 B
`build/atomic-block-components/category-list.js`	478 B	0 B
`build/atomic-block-components/image-frontend.js`	1.71 kB	0 B
`build/atomic-block-components/image.js`	1.15 kB	0 B
`build/atomic-block-components/price-frontend.js`	2.29 kB	0 B
`build/atomic-block-components/price.js`	2.32 kB	0 B
`build/atomic-block-components/rating-frontend.js`	524 B	0 B
`build/atomic-block-components/rating.js`	527 B	0 B
`build/atomic-block-components/sale-badge-frontend.js`	862 B	0 B
`build/atomic-block-components/sale-badge.js`	866 B	0 B
`build/atomic-block-components/sku-frontend.js`	386 B	0 B
`build/atomic-block-components/sku.js`	394 B	0 B
`build/atomic-block-components/stock-indicator-frontend.js`	568 B	0 B
`build/atomic-block-components/stock-indicator.js`	573 B	0 B
`build/atomic-block-components/summary-frontend.js`	918 B	0 B
`build/atomic-block-components/summary.js`	927 B	0 B
`build/atomic-block-components/tag-list-frontend.js`	467 B	0 B
`build/atomic-block-components/tag-list.js`	474 B	0 B
`build/atomic-block-components/title-frontend.js`	1.23 kB	0 B
`build/atomic-block-components/title.js`	1.06 kB	0 B
`build/attribute-filter-frontend.js`	18.1 kB	0 B
`build/attribute-filter.js`	12.4 kB	0 B
`build/blocks.js`	3.54 kB	0 B
`build/editor-rtl.css`	13.9 kB	0 B
`build/editor.css`	13.9 kB	0 B
`build/featured-category.js`	7.73 kB	0 B
`build/featured-product.js`	9.98 kB	0 B
`build/handpicked-products.js`	7.37 kB	0 B
`build/price-filter-frontend.js`	14.8 kB	0 B
`build/price-filter.js`	10.3 kB	0 B
`build/product-best-sellers.js`	7.45 kB	0 B
`build/product-categories.js`	3.23 kB	0 B
`build/product-category.js`	8.39 kB	0 B
`build/product-new.js`	7.61 kB	0 B
`build/product-on-sale.js`	8 kB	0 B
`build/product-search.js`	3.57 kB	0 B
`build/product-tag.js`	6.53 kB	0 B
`build/product-top-rated.js`	7.59 kB	0 B
`build/products-by-attribute.js`	8.33 kB	0 B
`build/reviews-by-category.js`	11.8 kB	0 B
`build/reviews-by-product.js`	13.4 kB	0 B
`build/reviews-frontend.js`	9.4 kB	0 B
`build/single-product-frontend.js`	33.8 kB	0 B
`build/single-product.js`	10.1 kB	0 B
`build/style-rtl.css`	18 kB	0 B
`build/style.css`	18 kB	0 B
`build/vendors-style-rtl.css`	1.03 kB	0 B
`build/vendors-style.css`	1.03 kB	0 B
`build/vendors.js`	417 kB	0 B
`build/vendors~atomic-block-components/price-frontend.js`	5.65 kB	0 B
`build/wc-blocks-data.js`	7.1 kB	0 B
`build/wc-blocks-middleware.js`	931 B	0 B
`build/wc-blocks-registry.js`	2.32 kB	0 B
`build/wc-payment-method-bacs.js`	790 B	0 B
`build/wc-payment-method-cheque.js`	787 B	0 B
`build/wc-payment-method-cod.js`	879 B	0 B
`build/wc-payment-method-paypal.js`	831 B	0 B
`build/wc-payment-method-stripe.js`	11.9 kB	0 B
`build/wc-settings.js`	2.33 kB	0 B
`build/wc-shared-context.js`	1.53 kB	0 B
`build/wc-shared-hocs.js`	1.66 kB	0 B

_{compressed-size-action}

senadir · 2020-10-09T11:18:30Z

feels weird to treat undefined as true, why not make sure payment methods return the correct value?

What's the behavior we're aiming for here?

Always retry unless specified
Never retry unless specified

senadir · 2020-10-09T11:19:18Z

Maybe, based on that, we can decide to rename this variable to make it obvious.

Aljullu · 2020-10-09T11:32:32Z

why not make sure payment methods return the correct value?

The idea is to provide good defaults, so there is less risk for payment methods to introduce bugs (like #3221).

I don't have any data supporting why the default should be true instead of false, so I'm happy to be proven wrong, but at least for Stripe and WC Payments we needed to support retrying after an error, and I can imagine that's the most common behavior among other payment methods.

In any case, checkoutAfterProcessingWithSuccess and checkoutAfterProcessingWithError are currently behaving differently, so whatever decision we end up taking, we need to make them work consistently.

Always retry unless specified

Correct, that's the behavior we are trying to achieve: always retry unless retry: false (see discussion in 978-GH-woocommerce-payments).

Note: If we were to rename retry to something else, that would probably be a breaking change.

senadir

I think this makes sense in the broader terms, maybe we can change the wording from 'undefined' to saying it defaults to true.

I'd probably also wait and see what Darren has to say about this.

nerrad

Looks good to me, I do wonder if we should add a shouldRetry function to the useEmitResponse hook to include this check and logic centralized there? That way the condition could just be:

if ( shouldRetry( response ) )

Which is a bit clear and keeps the actual default coercion logic in the useEmitResponse hook?

I'm going to pre approve anyways - up to you Albert whether you modify for the above or not.

Aljullu · 2020-10-09T15:51:34Z

Thanks for the reviews @senadir @nerrad!

I moved the logic of shouldRetry to useEmitResponse in 7fa0582 and updated the changelog notice of this PR accordingly.

Make 'retry' property default to true if it's undefined

cca9407

Aljullu added the status: needs review label Oct 9, 2020

Aljullu added this to the 3.6.0 milestone Oct 9, 2020

Aljullu requested a review from a team as a code owner October 9, 2020 11:11

Aljullu self-assigned this Oct 9, 2020

Aljullu requested review from senadir and removed request for a team October 9, 2020 11:11

Aljullu mentioned this pull request Oct 9, 2020

Checkout block: allow retrying after a failed payment Automattic/woocommerce-payments#978

Closed

senadir approved these changes Oct 9, 2020

View reviewed changes

woocommercebot added status: ready to merge and removed status: needs review labels Oct 9, 2020

nerrad approved these changes Oct 9, 2020

View reviewed changes

Create new shouldRetry method in useEmitResponse

7fa0582

Aljullu merged commit e267cd9 into trunk Oct 12, 2020

Aljullu deleted the fix/retry-undefined branch October 12, 2020 08:19

Aljullu added type: bug The issue/PR concerns a confirmed bug. block: checkout Issues related to the checkout block. labels Oct 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make 'retry' property on errors from checkoutAfterProcessingWithSuccess/Error observers default to true if it's undefined #3261

Make 'retry' property on errors from checkoutAfterProcessingWithSuccess/Error observers default to true if it's undefined #3261

Aljullu commented Oct 9, 2020 •

edited

github-actions bot commented Oct 9, 2020 •

edited

senadir commented Oct 9, 2020

senadir commented Oct 9, 2020

Aljullu commented Oct 9, 2020

senadir left a comment

nerrad left a comment •

edited

Aljullu commented Oct 9, 2020

Make 'retry' property on errors from checkoutAfterProcessingWithSuccess/Error observers default to true if it's undefined #3261

Make 'retry' property on errors from checkoutAfterProcessingWithSuccess/Error observers default to true if it's undefined #3261

Conversation

Aljullu commented Oct 9, 2020 • edited

How to test the changes in this Pull Request:

Changelog

github-actions bot commented Oct 9, 2020 • edited

senadir commented Oct 9, 2020

senadir commented Oct 9, 2020

Aljullu commented Oct 9, 2020

senadir left a comment

Choose a reason for hiding this comment

nerrad left a comment • edited

Choose a reason for hiding this comment

Aljullu commented Oct 9, 2020

Aljullu commented Oct 9, 2020 •

edited

github-actions bot commented Oct 9, 2020 •

edited

nerrad left a comment •

edited