Checkout Page Payment Information

[revanth0212]

Description

Guest Payment Workflow for the new Venia Checkout workflow.

As a guest, I should be able to add a credit card in the checkout page and use it for the checkout process.

Without a payment method selected, the review order button will be disabled. Once the user clicks the review order button, the braintree drop-in will make a request for the payment nonce and save it in the apollo cache.

If a payment nonce is present in the cache, it is an indication of the payment information step being complete. Hence the payment information summary should be shown instead.

The payment information summary component should have an edit option to edit the original payment method's data. Note: For security purposes, credit card details will not be stored. They should be entered again to complete the payment edit workflow.

This component saves some stuff in the cache and some on the remote GQL server. It stores payment details like type of card and last 4 digits in the cache but the nonce in the remote server due to incompatibilities between what GQL supports and what UX wants.

Sample Credit Cards for testing: https://developers.braintreepayments.com/guides/credit-cards/testing-go-live/ruby

Deployed version: http://pr-2320.pwa-venia.com

Related Issue

Closes PWA-183

Verification Stakeholders

@soumya-ashok
@schensley
@jimbo

Verification Steps

1. Go to the Checkout page.
2. Complete the first 2 steps.
3. The payment information section should show up.

Screenshots / Screen Captures (if appropriate)

Checklist

1. Once order confirmation PR is merged, we will have to make sure that it is deleting the payment nonce and billing address data from the cache that this component is adding.

[sirugh]

Is this array still necessary anymore? Now all the steps result in the same text being rendered.

[revanth0212]

Done 👍

Changed to something more meaningful.

[sirugh]

What is the difference between this effect and this teardown on line 71?

[revanth0212]

I am not sure of the internals of the dropping, but Andy added it as part of the initial drop in work that he did for the mini cart. Here I had to move it into a diff function because of a case that can arise due to payment service failing.

Here is some background information.

The credit card checkout process is a 3 step process:

1. Save billing address information on the cart
2. Get payment nonce from braintree by submitting credit card information
3. Save the payment nonce that we got from braintree in step 2.

After step 2, the braintree dropin changes it UI from a form to a summary kind of component which does not confine with out UX.

If the save service in step 3 fails, the dropin needs to be reset to the form. This is where tear down comes into picture. When that happens, we send a signal to the braintree dropin to teardown, which in braintree terms is reset so the user can enter the new payment information and submit again.

[sirugh]

The reason I asked was to try to get an understanding of both cases and see if there was some simplification we could make. In this instance it looks like the original .teardown() call was used as a sort of cleanup where the modal/dropin was unmounted. That doesn't seem to need the same logic as you've done in your async calls so there's nothing to do here.

There is an edge case I can forsee causing some errors be thrown. If shouldTeardownDropin is true the effect will run triggering teardown/recreation of the dropin instance. If a user closes the form during this effect's execution an error will throw because the dropin will mount without a target. That's why we added the didClose logic in the past.

[dpatil-magento]

@revanth0212
2 (Continuing) : When total amount is $0.00, i see Graphql call returns error instead of accepting it. Order total can be $0.00 when gift card is applied.

3. Enter Payment info and hit review order button. Now either reload page ...or come back to Checkout page.... user is asked to enter Payment info again, its not saved as Shipping info/Shipping method does.

[revanth0212]

@revanth0212
2 (Continuing) : When total amount is $0.00, i see Graphql call returns error instead of accepting it. Order total can be $0.00 when gift card is applied.

1. Enter Payment info and hit review order button. Now either reload page ...or come back to Checkout page.... user is asked to enter Payment info again, its not saved as Shipping info/Shipping method does.

The first one is a graphql bug. Second is being handled in PWA-523

[soumya-ashok]

Thanks for clarifying @revanth0212 ! The change is UX approved.

[sirugh]

Now that I look a little closer at this effect I realize it is not functioning as intended with didClose. The original effect from the dropin file defined didClose within the same closure as the definition for the createDropinInstance function. This meant that when the component unmounted and the effect's callback was invoked the didClose referenced within the createDropinInstance function was updated and handled correctly.

Here's a simplified example. The whole point is to create a thing if we're mounted or ignore it if we're unmounted.

const [thing, setThing] = useState();

useEffect(() => {
    let unmounted = false;
    async function attach() {
        const ref = await createThingThatNeedsDom();

        // If `unmounted` is still "false" by the time the awaited function resolves we set the ref in state
        // but if it is "true" because we unmounted then we should just cleanup and never set state.
        if (unmounted) {
            ref.cleanup();
        }
        else {
            setThing(ref);
        }
    }

    // Run the async attach.
    attach();

    return () => { 
        unmounted = true;
    }
}, [
    // No dependencies because this component runs on mount.
]);

I've provided this example as a way to show you how it was used in the past because as implemented, createDropinInstance can break if we unmount before we get to setDropinInstance(...). I realize you broke out the creation function because you need the logic twice but it doesn't make sense as written now. createDropinInstance(didClose); will always be called with false.

Please refactor this to correctly utilize the previous logic.

[dpatil-magento]

@revanth0212 As discussed, check if you can handle this scenario same as Shipping info. On hitting review order button on blank billing fields... we are submitting to GraphQL which return error on UI like below. It's good if we can check all mandatory fields are entered before we submit to graphql.

[dpatil-magento]

Above reported fixes looks good. This UI validation needs to be handled - If you try to save any address other than US. Like India, Switzerland which have state drop down >> I see error -

[dpatil-magento]

QA Pass, need review on recent commits.

[sirugh]

I saw your message in Slack - down to talk tomorrow morning when I get online.

[sirugh]

Is there no way to validate these fields on blur? Check out how other forms do it. I'm not sure why we didn't use validation on the old mini cart.

[revanth0212]

Discussed this on the call, commenting for notes.

We can not use the same techniques as in the previous forms, because of the way the form is submitted. In other forms, we submit using the onSubmit prop on the form so the validation is done by the form. In this case, we submit manually, form just serves as a context provider. Hence we have to perform the form validation manually before submitting it.

[sirugh]

Why is this form being submitted if isBillingAddressSame is true? Do we need to submit anything here? I would think if the billing and shipping address are the same (as in the user clicked the checkbox) then we would have sent that information via the graphQL cart mutation which means that this form shouldn't even render.

[revanth0212]

The billing address fields are always rendered, they are just hidden if the checkbox is checked. This was due to a review comment way up on the top 😅 . Hence when I have written a wrapper around the isRequired to only validate if the boolean is false.

[sirugh]

Why did you remove this query? We were previously validating the countries but that has now changed to just checking that the field is there, not that it's valid. If the change to validation was necessary (I'm not sure it is as we can just hide the form right?) then lets try to find a way to include country validation so we don't lose this functionality.

Edit: Add comment here w/ ticket for global country validation

[revanth0212]

We were using this query to get the list of countries. The list of countries are being used to validate the region code. But the region code validator that we have only works for US. It does not work for other countries. Hence I am doing what Shipping Address form does, only validate if it is present or not.

This is not the right place for that. A robust validate function needs to be written into the new Region component to handle such cases. It will be handled in https://jira.corp.magento.com/browse/PWA-552.

[sirugh]

This isn't quite right. We still get an error because it's possible for the component to attempt to attach to a container which is no longer mounted. To repro, submit a shipping method so that the payment info begins to load. Once you see the loading spinner click the venia logo in the header to navigate back to the home page. You'll see the error in console. You should add a try-catch around the createDropinInstance() call here.

[revanth0212]

I tried the same before committing my code and I was not able to reproduce. I tried now, and it is intermittent. Maybe I am clicking it before the initializing is done. Changing now.

[sirugh]

And that's a wrap!

updated!

[luongvm]

Finally 🥳 congrats!