Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Fix 3D secure payment errors #3272

Merged
merged 10 commits into from Oct 20, 2020

Conversation

Aljullu
Copy link
Contributor

@Aljullu Aljullu commented Oct 13, 2020

Fixes #2743.

This PR fixes a couple of issues with 3D secure payment errors:

  • After the error, it was not possible to make another payment without making a change in the card number. That was because we were not resetting the source id on error (5095d55).
  • Two errors were appearing instead of one. That's because the payment method error is created on onCheckoutAfterProcessingSuccess, when an error is created at that stage, it then triggers onCheckoutAfterProcessingError events. If no error was returned from onCheckoutAfterProcessingError events, we were adding a default one. That's why two errors were appearing: the one generated by the payment method in onCheckoutAfterProcessingSuccess and the default one generated by Blocks in onCheckoutAfterProcessingError.
    For now, I fixed it in Stripe's side saving the error message internally and returning it on onCheckoutAfterProcessingError (4a5c04e), but I feel that's a bit complicated for payment methods. For example, the same issue occurs in WC Payments so we will need to fix it there too. That makes me wonder if we could make the process easier. Maybe removing the default error message that is added on onCheckoutAfterProcessingError? Another option would be not to trigger onCheckoutAfterProcessingError after an error occurred in onCheckoutAfterProcessingSuccess. Thoughts @nerrad?
    Update: we decided to add a check before showing the default error notice. So if there is already an error notice, the default one won't be added in onCheckoutAfterProcessingError.
  • Fixed missing useEffect() dependencies (33280b5).
  • Typo (d97a0fa).

Screenshots

Peek 2020-10-13 10-22

How to test the changes in this Pull Request:

  1. Install Stripe payment gateway and set it up.
  2. Add products to the cart and go to a page with the Checkout block.
  3. Place an order using Stripe with the card number 4000 0027 6000 3184 (it will open an authentication modal).
  4. Click Cancel or fail the authentication.
  5. Verify only one error is displayed, inside the payment methods step.
  6. Without modifying the payment number, click on Place order again and verify the authentication modal appears again and you can place the order.

Now, let's pretend there is a payment gateway which doesn't add an error message when throwing an error. This way, we will ensure a default error notice is shown.

  1. In use-payment-intents.js, make the following change:
			if (
				response.type === emitResponse.responseTypes.ERROR &&
				response.retry
			) {
				setSourceId( '0' );
+				return { type: response.type, retry: response.retry };
			}
  1. Add products to the cart and go to a page with the Checkout block.
  2. Place an order using Stripe with the card number 4000 0027 6000 3184 (it will open an authentication modal).
  3. Click Cancel or fail the authentication.
  4. Verify an error is displayed with a generic text, at the top of the form.

Changelog

Fix a couple of issues when a 3D secure payment was aborted: now only one error notice appears and the payment can be retried with the same card number.

@Aljullu Aljullu added status: needs review type: bug The issue/PR concerns a confirmed bug. block: checkout Issues related to the checkout block. labels Oct 13, 2020
@Aljullu Aljullu added this to the 3.7.0 milestone Oct 13, 2020
@Aljullu Aljullu requested a review from a team as a code owner October 13, 2020 08:30
@Aljullu Aljullu self-assigned this Oct 13, 2020
@Aljullu Aljullu requested review from budzanowski and removed request for a team October 13, 2020 08:30
@github-actions
Copy link
Contributor

github-actions bot commented Oct 13, 2020

Size Change: +994 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/active-filters-frontend.js 8.81 kB -1 B
build/all-products-frontend.js 31.2 kB +2 B (0%)
build/all-products.js 36.1 kB +9 B (0%)
build/atomic-block-components/add-to-cart-frontend.js 8.95 kB -2 B (0%)
build/atomic-block-components/add-to-cart.js 7.52 kB -1 B
build/atomic-block-components/button-frontend.js 2.02 kB -1 B
build/atomic-block-components/price.js 2.32 kB +1 B
build/atomic-block-components/sku.js 395 B +1 B
build/atomic-block-components/title.js 1.06 kB +1 B
build/attribute-filter-frontend.js 18.2 kB -4 B (0%)
build/attribute-filter.js 12.4 kB +2 B (0%)
build/cart-frontend.js 70 kB +187 B (0%)
build/cart.js 38.7 kB +211 B (0%)
build/checkout-frontend.js 85.9 kB +211 B (0%)
build/checkout.js 42 kB +247 B (0%)
build/price-filter-frontend.js 14.8 kB -3 B (0%)
build/price-filter.js 10.3 kB -3 B (0%)
build/single-product-frontend.js 33.8 kB +1 B
build/vendors.js 417 kB -2 B (0%)
build/wc-payment-method-stripe.js 12 kB +138 B (1%)
ℹ️ View Unchanged
Filename Size Change
build/active-filters.js 8.85 kB 0 B
build/all-reviews.js 9.78 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.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.7 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/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 859 B 0 B
build/atomic-block-components/sale-badge.js 863 B 0 B
build/atomic-block-components/sku-frontend.js 386 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 917 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/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/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.56 kB 0 B
build/product-tag.js 6.53 kB 0 B
build/product-top-rated.js 7.58 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.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~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-settings.js 2.35 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

@Aljullu Aljullu force-pushed the fix/payment-method-validation-duplicate-errors branch from 5095d55 to eca892f Compare October 13, 2020 08:55
@nerrad
Copy link
Contributor

nerrad commented Oct 13, 2020

For now, I fixed it in Stripe's side saving the error message internally and returning it on onCheckoutAfterProcessingError (4a5c04e), but I feel that's a bit complicated for payment methods. For example, the same issue occurs in WC Payments so we will need to fix it there too. That makes me wonder if we could make the process easier. Maybe removing the default error message that is added on onCheckoutAfterProcessingError? Another option would be not to trigger onCheckoutAfterProcessingError after an error occurred in onCheckoutAfterProcessingSuccess. Thoughts @nerrad?

Ya I see the issue here. The intention behind the design was to always ensure that other things hooked into the event emitters can react to any errors that might have happened (which is why an error from the success emitter will in turn trigger the onCheckoutAfterProcessingWithErrors emitter).

I do agree we need to try and make things fairly simple for payment methods instead of the approach you've done so far in this pull. For subscribers registered to the events, they should only care about reacting to the events, not having to worry about internals causing duplicate notices etc.

I think removing the default error message that is added on onCheckoutAfterProcessingError might be the way to go here, but could we do that only if there is an error notice already queued up (otherwise if there are no existing error notices, let's add the default)?

@Aljullu Aljullu force-pushed the fix/payment-method-validation-duplicate-errors branch from 8aadd85 to 26069ef Compare October 15, 2020 11:11
@Aljullu
Copy link
Contributor Author

Aljullu commented Oct 15, 2020

I think removing the default error message that is added on onCheckoutAfterProcessingError might be the way to go here, but could we do that only if there is an error notice already queued up (otherwise if there are no existing error notices, let's add the default)?

Make sense, I needed to create a useCheckoutNotices() hook so we could check notices in all contexts. I updated the PR description and testing steps accordingly.

This PR is ready for review again.

Copy link
Contributor

@nerrad nerrad left a comment

Choose a reason for hiding this comment

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

The useStoreNotices hook exposes a hasNoticesOfType function. Would that suffice for doing the notice check and remove the need for the new useCheckoutNotices hook? That would reduce the new code added?

@Aljullu
Copy link
Contributor Author

Aljullu commented Oct 16, 2020

useStoreNotices() only exposes notices in the current context, but in this case we want to count if there are error notices in any of these three contexts: wc/checkout, wc/express-payment-area, wc/payment-area. I don't think useStoreNotices() has any way to achieve that?

@nerrad
Copy link
Contributor

nerrad commented Oct 16, 2020

Ahh, right! I am a bit wary about that if we add other notice contexts to checkout, this won't automatically account for them. Still it might be premature optimization to try and figure that out right now.

@budzanowski
Copy link
Contributor

As far as I can understand the code it makes sense to me and it works.

@Aljullu Aljullu force-pushed the fix/payment-method-validation-duplicate-errors branch from 63cd93b to 8be2654 Compare October 20, 2020 08:12
@Aljullu Aljullu merged commit ac99f33 into trunk Oct 20, 2020
@Aljullu Aljullu deleted the fix/payment-method-validation-duplicate-errors branch October 20, 2020 09:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
block: checkout Issues related to the checkout block. type: bug The issue/PR concerns a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2x confusing error notices + broken checkout when user cancels 3D secure payment
4 participants