Skip to content
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

Improve Checkout error handling in JS #5202

Merged
merged 5 commits into from
Apr 15, 2020

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Apr 10, 2020

  • Switch from old success/error to modern then/catch structure: .catch() will get a few more errors then .errors().
  • Add try/catch inside catch to detect any errors parsing the response error payload including when the response doesnt include any error or flash information. In these cases log to bugsnag (throw the error again) and inform the user so we dont have silent checkout errors.

What? Why?

Related to #5192

What should we test?

We need to test checkout error scenarios:

  • Redirect to cart page if insufficient stock: go to checkout with variant X in the cart, go to the backoffice and make on_hand zero for variant X, return to checkout page and click "place order", you should be redirected to the cart page with a message insufficient stock
  • Try to checkout with invalid email address, for example, "ofn@org"
  • Try to pay with an invalid credit card (bogus or stripe payment methods)
  • Try to pay with a StripeSCA card where authentication is required for example 4000 0027 6000 3184 and fail the authentication on the stripe test page, you should see an flash error on the checkout page when you are redirected to the checkout page.

Release notes

Changelog Category: Changed
Improved resiliency of the checkout code that handles error scenarios.

@luisramos0 luisramos0 changed the title Switch from old success/error to modern then/catch structure Checkout error handling in JS: switch from old success/error to modern then/catch structure Apr 10, 2020
@luisramos0 luisramos0 changed the title Checkout error handling in JS: switch from old success/error to modern then/catch structure Improve Checkout error handling in JS Apr 10, 2020
@luisramos0 luisramos0 self-assigned this Apr 10, 2020
@luisramos0 luisramos0 changed the title Improve Checkout error handling in JS DRAFT Improve Checkout error handling in JS Apr 13, 2020
@luisramos0 luisramos0 force-pushed the improve_checkout_js branch from 2ce1f5b to c1203d8 Compare April 14, 2020 10:21
@luisramos0 luisramos0 force-pushed the improve_checkout_js branch from c1203d8 to 47a9356 Compare April 14, 2020 13:02
else
throw response unless response.data.flash

@errors = response.data.errors
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot find/understand what this @errors does but I dont want to remove it and break things...

@luisramos0 luisramos0 force-pushed the improve_checkout_js branch from eb45528 to cedf1b2 Compare April 14, 2020 13:31
@luisramos0 luisramos0 marked this pull request as ready for review April 14, 2020 13:31
@luisramos0 luisramos0 changed the title DRAFT Improve Checkout error handling in JS Improve Checkout error handling in JS Apr 14, 2020
@luisramos0 luisramos0 added the bug-s2 The bug is affecting any of the non-critical features described in S1 and there is no workaround. label Apr 14, 2020
@luisramos0
Copy link
Contributor Author

tagging bug-s2 for clarity.

@filipefurtad0 filipefurtad0 self-assigned this Apr 15, 2020
@filipefurtad0 filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Apr 15, 2020
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Apr 15, 2020

Hi @luisramos0,

Had a look at the described test scenarios:

  1. Checked that the user gets redirected to cart page if the products' stock is reduced by the admin/managers during customers journey from cart to checkout. This red banner appears, with "An item in your cart has become unavailable." as well as the red text "Insufficient stock available, only 2 remaining".
  • however, at this stage, the user gets "stuck" in the /cart page. Even if the number of items is reduced till the "insufficient stock" warning disappears, one does not seem to be able to proceed to checkout or to the shopfront. Here, the only way out, i.e. workaround is either a) pressing the trash-bin and removing the items from the cart, or b) using the address bar to type an address. I am pretty sure, this is not introduced by this PR - as I have observed this recently - so I will open an issue on this one.
  1. Checking out by clicking the "place order" with invalid email address like ofn@ofn displays the error "Customer E-mail is invalid"

image

The field itself accepts an address like ofn@ofn; it does reject the email address if the @ sign is removed, thus preventing the user from clicking "place order".

  1. Paying with an invalid credit card (bogus or stripe payment methods) -> Generates the red banner "Error: Your card number is invalid."

  2. Trying to pay with a StripeSCA card 4000 0027 6000 3184 and fail the authentication on the stripe test page, shows the flash error on the checkout page, after being redirected to the checkout page.

image

  • The error message displays the correct content but I am not too sure about using the underscores on this message. This makes it perhaps less readable user-friendly.
  1. I added the test-case in which one completes authentication but uses a card with insufficient funds. The correct error message is displayed "Your card has insufficient funds." in the red banner, on the checkout page.

I'd say this is ready to go. I will open issues on the respective issues found on 1), an S3 maybe? I think the comment in 4) probably does not qualify as an issue, I guess.

@filipefurtad0 filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Apr 15, 2020
@luisramos0
Copy link
Contributor Author

thanks Filipe, nice testing!!!

  1. I am not sure but I think there is already an issue in gh for that, please try to search for it before creating a new one.

  2. yes, in stripeSCA I decided to show the user the message that comes from Stripe directly. It's not nice but it's probably more informative than a generic error message. I think we can create a lower priority issue for the stripeSCA backlog.

@luisramos0 luisramos0 merged commit 8532fa1 into openfoodfoundation:master Apr 15, 2020
@luisramos0 luisramos0 deleted the improve_checkout_js branch April 15, 2020 21:30
@luisramos0
Copy link
Contributor Author

thanks Filipe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-s2 The bug is affecting any of the non-critical features described in S1 and there is no workaround.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants