Handling invalid braintree nonce error while placing order. #2405

revanth0212 · 2020-05-19T16:24:29Z

Description

This PR handles the use case of an invalid nonce being submitted to the backend during the place order mutation. This can happen if a user has created a payment nonce and has not checkout for a while, enough for the nonce to expire.

The UI would never know to refresh the payment information until the backend fails with the invalid payment nonce error when the user clicks on the place order button.

I have handled payment information errors in this PR, but also made it possible for other developers to add other error handlers in Shipping Information and Shipping Method.

Checkout Page
     \__ useCheckoutPage
     \__ Shipping Information 
     \__ Shipping Method
     \__ Payment Information
              \__ usePaymentInformation

This PR introduces a new error class called CheckoutError which will be passed from the useCheckoutPage talon to Shipping Information, Shipping Method, and Payment Information. Each of those components should have handlers to handle this kind of an error. In this PR, I have added a handler for the CheckoutError in the usePaymentInformation talon.

Related Issue

Closes PWA-556

Verification Stakeholders

@jimbo
@sirugh

Verification Steps

Go to the deployed app and add an item into the cart.
Go to the checkout page.
Complete Shipping Information, Shipping Method, and Payment Information.
A payment nonce should have been created and update on the cart in the backend. Now we have to invalidate that nonce to create the error. Use grapiql or any method to submit a mutation on the cart to replace the nonce with an invalid nonce.

Here is the mutation:

mutation setSelectedPaymentMethod($cartId: String!, $paymentNonce: String!) {
  setPaymentMethodOnCart(input: {cart_id: $cartId, payment_method: {code: "braintree", braintree: {payment_method_nonce: $paymentNonce, is_active_payment_token_enabler: false}}}) {
    cart {
      id
      selected_payment_method {
        code
        title
      }
    }
  }
}

and the query params:

{
  "cartId": "CURRENT_CART_ID",
  "paymentNonce": "ANY_STRING"
}

Once that is a success, click the place order button, it should show an error toast saying the payment method is invalid or something on those lines and the UI should ask you to re-enter the payment information.
Now if you click the place order button, it should work fine.

Screenshots / Screen Captures (if appropriate)

Demo LIink - The error message in the demo has been changed. I have stripped off GraphQL error: string from the error message. Below is the updated screen shot for that.

Error screenshot:

Checklist

I have added tests to cover my changes, if necessary.

PWAStudioBot · 2020-05-19T16:37:20Z

	Fails
🚫	A version label is required. A maintainer must add one.

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Associated JIRA tickets: PWA-556.

Generated by 🚫 dangerJS against d7905ac

revanth0212 · 2020-05-19T16:40:05Z

packages/venia-ui/lib/components/CheckoutPage/checkoutPage.js

    } = talonProps;

    const [, { addToast }] = useToasts();

    useEffect(() => {
        if (hasError) {
+            const message =
+                error && error.message
+                    ? error.message


Can we display the same error we receive from the GraphQl server? At times, it can contain text like GQL Error: ******* which might not be useful for the user.

devops-pwa-codebuild · 2020-05-19T16:41:46Z

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2405.pwa-venia.com : LH Performance Expected 0.85 Actual 0.58, LH Best Practices Expected 1 Actual 0.92
https://pr-2405.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.36, LH Best Practices Expected 1 Actual 0.92
https://pr-2405.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.5, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92

revanth0212 · 2020-05-19T16:47:34Z

packages/peregrine/lib/talons/CheckoutPage/PaymentInformation/usePaymentInformation.js

@@ -105,6 +113,19 @@ export const usePaymentInformation = props => {
        [onSave]
    );

+    const clearPaymentDetails = useCallback(() => {


This will make sure even if the user refreshes the page, the summary won't be shown because hasData will be false.

revanth0212 · 2020-05-19T16:50:23Z

packages/peregrine/lib/talons/CheckoutPage/PaymentInformation/usePaymentInformation.js

@@ -115,6 +136,27 @@ export const usePaymentInformation = props => {
        onCompleted: onPaymentDetailsQueryCompleted
    });

+    const handleExiredPaymentError = useCallback(() => {


I have tried to reset the selected_payment_method on the cart as well, but the GQL server does not allow us to do so. Once set, the selected_payment_method can not be changed to "" which is the default value when a cart is created.

Yea I remember this being a thing. Fortunately we just display "done" state based on the completion of the child method and not based on selected_payment_method.

Suggested change

const handleExiredPaymentError = useCallback(() => {

const handleExpiredPaymentError = useCallback(() => {

Make sure to update all references to this constant.

sirugh

Here's some initial pre-ux review feedback :) This is pretty close to what I had imagined the solution being. Basically just need to reset to step 3 (payment info) and display the edit form (wipe nonce/setDoneEditing(false).

sirugh · 2020-05-19T16:37:53Z

packages/peregrine/lib/talons/CheckoutPage/useCheckoutPage.js

+         * the step to PAYMENT, we will make the user re-enter the
+         * payment information.
+         */
+        setCheckoutStep(CHECKOUT_STEP.PAYMENT);


Instead of threading a callback to the child you can just do this in a try/catch inside handlePlaceOrder.

I am not sure how will throwing an error in the parent will propagate to the child.

placeOrderError is still that same error that is thrown but my point is that logic specific to checkout page should be done within the checkout page callback and logic specific to error handling for payment information such as removing the nonce should be done down the line. You've got a combination of these two things, I'm just asking that you split them.

Commented below in another thread about my thoughts.

#2394 had logic in the place order catch block for setting review order button to false and setting checkout step to payment. If we're going to delegate the errors to the children for handling then we don't need the catch logic anymore (though we still should log the error for development purposes). So I would remove lines 139 and 140 in this file and perhaps replace them with a comment about how the child components handle submission errors.

Additionally, we also started passing setCheckoutStep and resetShouldSubmit to payment information.

Instead of writing the error handler here in checkout, could you define it in the usePaymentInformation talon? We don't need to define it here and pass it down as nothing else uses it.

sirugh · 2020-05-19T16:40:51Z

packages/peregrine/lib/talons/CheckoutPage/CheckoutError.js

+         * This is temp stuff, need to have a better check.
+         */
+        return this.error.graphQLErrors.map(({ message }) =>
+            message.includes(paymentErrorMessage)


Maybe it doesn't matter that the payment is specifically expired. I'm not sure of all the ways that the specific error could be thrown but perhaps we don't even need a custom error class. With GQL mutations we can handle errors as they occur which means we won't ever be using this class for handling something like a submitShippingMethodError since that error will be handled by the mutation invoker, useShippingMethod.js.

There are different errors that can happen during the place order mutation and some of them are not related to payment information. Check this out: https://devdocs.magento.com/guides/v2.3/graphql/mutations/place-order.html#errors

Now that we have an error class, we can pass that error to different child components and let them deal with it instead of adding this in the useCheckoutPage's placeOrder function.

Oh nice find on the doc! May as well add a link in a comment at the top of this file.

Also funny enough, the transaction was declined error isn't on that page 😆

We may have to refine/add new error checks to extend this error class to other checkout page steps.

sirugh · 2020-05-19T16:42:22Z

packages/peregrine/lib/talons/CheckoutPage/PaymentInformation/usePaymentInformation.js

+        clearPaymentDetails({ variables: { cartId } });
+        resetReviewOrderButtonClicked();
+        setHasData(false);
+        onError();


Use a try-catch in the checkout page so you don't have to thread the onError down to the child nor do you need to reset the review order button.

If I have the try-catch and the throw statement in the useCheckoutPage talon, how will the children catch it? It will only propagate upwards, right?

I commented this in multiple places, but for posterity: placeOrderError can be passed down (or some combination of the error).

You are right, I can have the try-catch and send the rest of the props down the PaymentInformation component/talon. But I feel, sending down a function to call when an error occurs is better than having some logic in try-catch in the parent and some in the child even though the function sent down to the child executes in the parent context.

That said, the function in question, revertPaymentInformationDone sent as onError is called as the last function in handleExiredPaymentError. If instead of sending it as a prop, we call it in the try-catch, it will be called first. The child component won't have control over when to call the function.

TLDR, I feel comfortable sending it as a prop and having the child components handle the error and the onError handler. In this case, it is PaymentInformation, but once we expand this to check for errors in ShippingInformation and ShippingMethods they can call the handlers themselves when they think is the right time.

OK I get. it! In the future we may get other errors during placeOrder that are related to other checkout components besides payment info. So this pattern would delegate the responsibility for handling the error to the component that caused it.

I'm on board with this but I do think we can at least move resetReviewOrderButtonClicked to useCheckoutPage handler as that's a global concern we will want to do every time we get an error.

packages/peregrine/lib/talons/CheckoutPage/PaymentInformation/usePaymentInformation.js

sirugh · 2020-05-19T16:47:11Z

packages/peregrine/lib/talons/CheckoutPage/PaymentInformation/usePaymentInformation.js

+                cart: {
+                    __typename: 'Cart',
+                    id: cartId,
+                    paymentNonce: null


I don't think that usePaymentInformation should know about the nonce as it is specific to braintree/credit card.

That said, after working on payment information I understand the component heirarchy a bit more. CreditCard is not mounted if you are in a "done editing" view which means that there's no way to pass the error to that component and have it wipe CC specific data like nonce or billing address.

Exactly. Also, it is not a rule but it can be an understanding that any payment method which has to save something related to payment on the cart in the cache, should place it in the key called paymentNonce. That way, we can handle clearing off of the cache for any payment method in the payment information component.

any payment method which has to save something related to payment on the cart in the cache, should place it in the key called paymentNonce.

I like the idea here but I don't think paymentNonce is a good name for it 😄Probably worth a small refactor here to rename this property. Perhaps selectedPaymentDetails?

@sirugh should we still change the prop? If so, I would like to handle it as part of a diff PR. I would not like to expand the scope of this PR. What do you think?

I think paymentNonce should be fine as a name. I'd also be concerned about tearing that name out of every place we have it currently.

packages/venia-ui/lib/components/CheckoutPage/PaymentInformation/paymentInformation.gql.js

packages/venia-ui/lib/components/CheckoutPage/checkoutPage.js

sirugh · 2020-05-21T17:17:22Z

packages/peregrine/lib/talons/CheckoutPage/CheckoutError.js

 class CheckoutError extends Error {
    constructor(gqlError, ...params) {
        super(params);
        this.name = 'CheckoutError';
-        this.message = gqlError.message;
+        this.message = removeGQLTag(gqlError.message);


I don't think we have to explicitly strip out the gql tag. There is a property on the graphql errors that is just the message after the colon. Probably something like graphqlErrors[0].message. I'd have to debug it to see the specific props but I know it's there.

gqlError.message is the generic error from the server. Since graphqlErrors is an array and each one of them has a meesage field, how would we know which one to display? Either we can display all the graphqlErrors.message's or just show the generic message for the whole error object which is gqlError.message.

I am fine with either. Let me know what you think.

I wonder if error.message always equivalent to the message from the first element in graphQLErrors with GraphQL Error.

In all other cases that we reference graphqlErrors in this codebase we either iterate over each element in the array and handle it separately or we treat the first element in the array as the one we will "handle" either by displaying a relevant message or logging it.

Replacing the prefixed text seems fine though, especially for the root message of the error class.

packages/peregrine/lib/talons/CheckoutPage/CheckoutError.js

sirugh · 2020-05-28T17:53:01Z

packages/venia-ui/lib/components/CheckoutPage/PaymentInformation/paymentInformation.gql.js

+export const GET_PAYMENT_NONCE = gql`
+    query getPaymentNonce($cartId: String!) {
+        cart(cart_id: $cartId) @connection(key: "Cart") {
+            id


Remove id and wrap it in eslint-disable for required fields. If you include id it will make a network request :(

sirugh · 2020-05-28T18:01:44Z

packages/peregrine/lib/talons/CheckoutPage/useCheckoutPage.js

+         * the step to PAYMENT, we will make the user re-enter the
+         * payment information.
+         */
+        setCheckoutStep(CHECKOUT_STEP.PAYMENT);


#2394 had logic in the place order catch block for setting review order button to false and setting checkout step to payment. If we're going to delegate the errors to the children for handling then we don't need the catch logic anymore (though we still should log the error for development purposes). So I would remove lines 139 and 140 in this file and perhaps replace them with a comment about how the child components handle submission errors.

Additionally, we also started passing setCheckoutStep and resetShouldSubmit to payment information.

Instead of writing the error handler here in checkout, could you define it in the usePaymentInformation talon? We don't need to define it here and pass it down as nothing else uses it.

sirugh · 2020-05-29T17:44:45Z

packages/peregrine/lib/talons/CheckoutPage/PaymentInformation/usePaymentInformation.js

 import { CHECKOUT_STEP } from '../useCheckoutPage';

 /**
 *
 * @param {Function} props.onSave callback to be called when user clicks review order button
+ * @param {Object} props.checkoutError an instance of the `CheckoutError` error that has been generated using the error from the place order mutation
+ * @param {DocumentNode} props.queries.getPaymentDetailsQuery query to fetch selected payment method and payment nonce details


Replace this with the nonce query doc. the other query is already included in the docs on line 16.

sirugh

Aside from the comment about the extra doc line added this is good to go for QA.

dpatil-magento · 2020-06-01T17:29:54Z

QA Pass, good to merge after approval.

supernova-at

New commits look good

jimbo

Looks good. Some minor cleanup to do but I think this'll be good once that's done. 👍

jimbo · 2020-05-21T19:01:16Z

packages/peregrine/lib/talons/CheckoutPage/PaymentInformation/usePaymentInformation.js

@@ -115,6 +138,27 @@ export const usePaymentInformation = props => {
        onCompleted: onPaymentDetailsQueryCompleted
    });

+    const handleExiredPaymentError = useCallback(() => {


Suggested change

const handleExiredPaymentError = useCallback(() => {

const handleExpiredPaymentError = useCallback(() => {

jimbo · 2020-06-02T18:31:19Z

packages/peregrine/lib/talons/CheckoutPage/CheckoutError.js

+ * https://devdocs.magento.com/guides/v2.3/graphql/mutations/place-order.html#errors
+ */
+
+const paymentErrorMessage =


Maybe leave a comment explaining that this a heuristic, rather than something to be rendered?

This may be the correct use case for an enum containing all of the constants from the documentation linked elsewhere. Perhaps import this one?

jimbo · 2020-06-02T18:43:16Z

packages/peregrine/lib/talons/CheckoutPage/PaymentInformation/usePaymentInformation.js

+                cart: {
+                    __typename: 'Cart',
+                    id: cartId,
+                    paymentNonce: null


I think paymentNonce should be fine as a name. I'd also be concerned about tearing that name out of every place we have it currently.

jimbo · 2020-06-02T18:44:58Z

packages/peregrine/lib/talons/CheckoutPage/PaymentInformation/usePaymentInformation.js

@@ -115,6 +136,27 @@ export const usePaymentInformation = props => {
        onCompleted: onPaymentDetailsQueryCompleted
    });

+    const handleExiredPaymentError = useCallback(() => {


Suggested change

const handleExiredPaymentError = useCallback(() => {

const handleExpiredPaymentError = useCallback(() => {

Make sure to update all references to this constant.

jimbo · 2020-06-02T18:49:05Z

packages/peregrine/lib/talons/CheckoutPage/PaymentInformation/usePaymentInformation.js

+
+    useEffect(() => {
+        if (
+            checkoutError &&


Maybe remove this, if it's redundant with the next line.

revanth0212 added 3 commits May 18, 2020 16:45

Added invalid payment handling.

d3adb35

Removed payment info clear method.

277e69f

Clearing payment details from cache.

f8fd4c1

m2-community-project bot added this to Ready for Review in Pull Request Progress May 19, 2020

PWAStudioBot added pkg:peregrine pkg:venia-ui labels May 19, 2020

revanth0212 commented May 19, 2020

View reviewed changes

m2-community-project bot assigned revanth0212 May 19, 2020

m2-community-project bot moved this from Ready for Review to Review in Progress in Pull Request Progress May 19, 2020

m2-community-project bot added the Progress: review label May 19, 2020

revanth0212 commented May 19, 2020

View reviewed changes

Adding JSDocs and Proptypes.

64b1fd6

revanth0212 marked this pull request as ready for review May 19, 2020 16:51

sirugh reviewed May 19, 2020

View reviewed changes

m2-community-project bot assigned sirugh and zetlen May 19, 2020

Addressed PR comments.

dd57ca1

sirugh reviewed May 21, 2020

View reviewed changes

packages/peregrine/lib/talons/CheckoutPage/CheckoutError.js Outdated Show resolved Hide resolved

Minor.

9fb344e

sirugh mentioned this pull request May 22, 2020

[PWA-245] Shipping Information (Authenticated) #2380

Merged

m2-community-project bot assigned tjwiebell May 22, 2020

revanth0212 added 3 commits May 28, 2020 11:43

Merged develop into current.

1d53fdc

Minor.

c23abdc

Fixed checkout error prop type.

d959953

sirugh requested changes May 28, 2020

View reviewed changes

Merged develop into current.

67c2488

Addressed PR comments.

b6d684e

sirugh reviewed May 29, 2020

View reviewed changes

sirugh previously approved these changes May 29, 2020

View reviewed changes

Minor.

484c173

revanth0212 dismissed sirugh’s stale review via 484c173 May 29, 2020 18:13

dpatil-magento and others added 3 commits June 1, 2020 10:03

Merge branch 'develop' into revanth/payment_info_expired_nonce_handling

03d578d

Fixed tests.

f44524d

Added tests for error workflow handling.

f2f3c4a

Merge branch 'develop' into revanth/payment_info_expired_nonce_handling

6395c43

supernova-at previously approved these changes Jun 2, 2020

View reviewed changes

m2-community-project bot assigned supernova-at Jun 2, 2020

jimbo suggested changes Jun 2, 2020

View reviewed changes

m2-community-project bot assigned jimbo Jun 2, 2020

Addressed PR comments.

d7905ac

revanth0212 dismissed supernova-at’s stale review via d7905ac June 2, 2020 20:48

jimbo approved these changes Jun 2, 2020

View reviewed changes

dpatil-magento merged commit a80e883 into develop Jun 2, 2020

m2-community-project bot moved this from Review in Progress to Done in Pull Request Progress Jun 2, 2020

dpatil-magento deleted the revanth/payment_info_expired_nonce_handling branch June 2, 2020 21:35

m2-community-project bot removed the Progress: review label Jun 2, 2020

	const handleExiredPaymentError = useCallback(() => {
	const handleExpiredPaymentError = useCallback(() => {

Handling invalid braintree nonce error while placing order. #2405

Handling invalid braintree nonce error while placing order. #2405

Conversation

revanth0212 commented May 19, 2020 • edited

Description

Related Issue

Verification Stakeholders

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented May 19, 2020 • edited

Choose a reason for hiding this comment

devops-pwa-codebuild commented May 19, 2020 • edited

revanth0212 May 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revanth0212 May 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revanth0212 May 21, 2020 • edited

Choose a reason for hiding this comment

sirugh May 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirugh left a comment

Choose a reason for hiding this comment

dpatil-magento commented Jun 1, 2020

supernova-at left a comment

Choose a reason for hiding this comment

jimbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revanth0212 commented May 19, 2020 •

edited

PWAStudioBot commented May 19, 2020 •

edited

devops-pwa-codebuild commented May 19, 2020 •

edited

revanth0212 May 19, 2020 •

edited

revanth0212 May 19, 2020 •

edited

revanth0212 May 21, 2020 •

edited

sirugh May 21, 2020 •

edited