[bug] Use gql persistence for checkout step between refreshes. #2354

sirugh · 2020-04-22T21:33:25Z

Description

The checkout step is used to inform guest checkout flow which step we're in and to prevent early submission before all the data necessary is present. We use gql to store and persist the checkoutStep across refreshes so that a user returning to checkout is presented at the point in time they left off before.

Related Issue

Closes PWA-523.

Acceptance

Verification Stakeholders

Specification

Verification Steps

Go to the cart page.
Check boxes in gift options and add a message.
Refresh the page and check that section. Your selections/message should persist.
Go to checkout and refresh the page after you submit each step. The page should render the step you were just on instead of cascading from each previous step.
Delete apollo-cache-persist entry ONLY from local storage and refresh. The page should render what looks like a cascade while the effects trigger each onSave after fetching data. As there is no way to store nonce info for the payment info summary view, this step will appear as if you never submitted it now.
Log in/out. All $Cart should be deleted and the Cart entry should be recreated with a new id.

Screenshots / Screen Captures (if appropriate)

Checklist

I have updated the documentation accordingly, if necessary.
I have added tests to cover my changes, if necessary.

Signed-off-by: sirugh <rugh@adobe.com>

sirugh · 2020-04-22T21:36:06Z

packages/venia-ui/lib/components/CartPage/PriceAdjustments/GiftOptions/giftOptions.js

@@ -93,7 +93,7 @@ const SET_GIFT_OPTIONS_QUERY = gql`
            include_gift_receipt: $include_gift_receipt
            include_printed_card: $include_printed_card
            gift_message: $gift_message
-        ) @client
+        ) @client @connection(key: "set_gift_options")


These were missed during the PII pass in #2322

sirugh · 2020-04-22T21:37:32Z

packages/venia-ui/lib/drivers/adapter.js

+    if (!initialized) {
+        // TODO: Replace with app skeleton. See PWA-547.
+        return null;
+    }


I broke this in #2322. We have to wait persistence otherwise we end up not restoring the cache from local storage which is counter to why we even include persistence layer.

sirugh · 2020-04-22T21:38:45Z

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

+            // TODO: Replace with heuristic check against cart data. Requires
+            // fetching more than just total quantity for checkout details query
+            // "cart" arg will have server result.
+            return cart.checkoutStep || 1;


This seems fine to me right now but if we wanted to get rid of the onSave callbacks that we require each component make we could have some heuristic that checks all cart data and sets the current step according to whatever data is missing. Good idea from @tjwiebell :)

PWAStudioBot · 2020-04-22T21:47:26Z

	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-523.

Generated by 🚫 dangerJS against 1b61023

Signed-off-by: sirugh <rugh@adobe.com>

sirugh · 2020-04-23T16:34:21Z

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


-    const [getCheckoutStep, { data: stepData, client }] = useLazyQuery(
-        getCheckoutStepQuery


I replaced this query with a client side resolver for the checkoutStep property of Cart types. The resolver handles the default state as well, setting the step to 1 if undefined.

In the future we could also remove the need for onSave callbacks by moving the determination of step into the resolver and passing all checkout data necessary to figure out the step.

For example instead of shippingdata.city indicating completion of shipping info step in an effect inside that component we would just check it in the resolver.

sirugh · 2020-04-23T16:37:33Z

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

@@ -159,7 +156,7 @@ export const useCheckoutPage = props => {
        hasError: !!placeOrderError,
        isCartEmpty: !(checkoutData && checkoutData.cart.total_quantity),
        isGuestCheckout: !isSignedIn,
-        isLoading: checkoutLoading,
+        isLoading: !checkoutCalled || (checkoutCalled && checkoutLoading),


There's a moment in render where we aren't loading but also haven't yet called the query. We want to wait to render anything until we've loaded the step at a minimum.

Signed-off-by: sirugh <rugh@adobe.com>

tjwiebell

Looks good, I think you can use a single regex to pattern match though.

tjwiebell · 2020-04-24T18:08:27Z

packages/peregrine/lib/talons/AuthModal/useAuthModal.js

        await deleteCacheEntry(apolloClient, key => key.match(/^Cart/));
+        await deleteCacheEntry(apolloClient, key => key.match(/^\$Cart/));


Maybe add a comment about the dual rule (Apollo caches weird when it can't compute a key).

Suggested change

await deleteCacheEntry(apolloClient, key => key.match(/^Cart/));

await deleteCacheEntry(apolloClient, key => key.match(/^\$Cart/));

await deleteCacheEntry(apolloClient, key => key.match(/^\$?Cart/));

This is an interesting point - we're explicit about which keys to delete, but by combining them in a single regex/function call that point is kind of obscured. Do you think its worth it to combine? I can add a comment above each call just to explain, at least.

Using regex obscures what you're doing for most anyway, I don't see any problem with being more efficient. If you were using a string method I would agree, eg. key.startsWith.

tjwiebell · 2020-04-24T18:10:12Z

packages/peregrine/lib/talons/CheckoutPage/OrderConfirmationPage/useCreateAccount.js

                await deleteCacheEntry(apolloClient, key => key.match(/^Cart/));
+                await deleteCacheEntry(apolloClient, key =>
+                    key.match(/^\$Cart/)
+                );


Suggested change

await deleteCacheEntry(apolloClient, key => key.match(/^Cart/));

await deleteCacheEntry(apolloClient, key =>

key.match(/^\$Cart/)

);

await deleteCacheEntry(apolloClient, key =>

key.match(/^\$?Cart/)

);

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

tjwiebell · 2020-04-24T18:14:55Z

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

@@ -118,6 +124,7 @@ export const useCheckoutPage = props => {

        // Delete stale cart data from apollo
        await deleteCacheEntry(apolloClient, key => key.match(/^Cart/));
+        await deleteCacheEntry(apolloClient, key => key.match(/^\$Cart/));


Merge the rules together, suggestions above.

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

tjwiebell · 2020-04-24T18:20:31Z

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

+        !!chosenShippingMethodData.cart.shipping_addresses[0]
+            .selected_shipping_method;


Our components together guard against this use case, but someone just using this talon might still enter this block w/o setting an address. Let's guard against it (don't apply this from GitHub, I'm sure formatting is off).

Suggested change

!!chosenShippingMethodData.cart.shipping_addresses[0]

.selected_shipping_method;

!!chosenShippingMethodData.cart.shipping_addresses.length && chosenShippingMethodData.cart.shipping_addresses[0].selected_shipping_method;

Good call. I'll make the change.

Fixing in 6130923

tjwiebell · 2020-04-24T18:22:22Z

packages/peregrine/lib/talons/CreateAccount/useCreateAccount.js

@@ -97,6 +97,9 @@ export const useCreateAccount = props => {

                // Delete stale cart data from apollo
                await deleteCacheEntry(apolloClient, key => key.match(/^Cart/));
+                await deleteCacheEntry(apolloClient, key =>
+                    key.match(/^\$Cart/)


Merge rules

tjwiebell · 2020-04-24T18:22:29Z

packages/peregrine/lib/talons/SignIn/useSignIn.js

@@ -63,6 +63,9 @@ export const useSignIn = props => {

                // Delete stale cart data from apollo
                await deleteCacheEntry(apolloClient, key => key.match(/^Cart/));
+                await deleteCacheEntry(apolloClient, key =>
+                    key.match(/^\$Cart/)


Merge rules

dpatil-magento · 2020-04-25T01:19:12Z

@sirugh If I click Review Order button and then do browser refresh >> Both Review Order and Place Order buttons are displayed.

sirugh · 2020-04-27T14:20:59Z

If I click Review Order button and then do browser refresh >> Both Review Order and Place Order buttons are displayed.

Correct - the logic for when to display that button is currently held within the Payment Information component. When either #2320 is merged or this PR we will have to double check this case. Just keep that in mind after you merge one or the other.

Signed-off-by: sirugh <rugh@adobe.com>

jimbo

Minor style changes requested, but overall I agree with this approach. 👍

jimbo · 2020-04-28T18:17:38Z

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

+            called: checkoutCalled,
+            client,
+            loading: checkoutLoading
+        }


Try to avoid nested destructuring.

jimbo · 2020-04-28T18:23:42Z

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

-        [setCheckoutStep]
+        () =>
+            checkoutStep === CHECKOUT_STEP.SHIPPING_ADDRESS &&
+            setCheckoutStep(CHECKOUT_STEP.SHIPPING_METHOD),


If you have a function that doesn't return anything, such as this one, just open a function body and use an if statement.

jimbo · 2020-04-28T18:33:05Z

packages/peregrine/lib/talons/CreateAccount/useCreateAccount.js

-                await deleteCacheEntry(apolloClient, key => key.match(/^Cart/));
+                await deleteCacheEntry(apolloClient, key =>
+                    key.match(/^\$?Cart/)
+                );


Consider extracting this function into a utility, since we're doing this three times in this PR alone. Perhaps deleteStaleCartData?

Signed-off-by: sirugh <rugh@adobe.com>

dpatil-magento · 2020-04-29T18:57:41Z

QA Pass, good to merge after recent commits are reviewed.

tjwiebell

Most recent changes look good 👍

sirugh added 2 commits April 22, 2020 15:08

Prevent cart id persistence in cache in gift options

4c3934d

Use resolver to handle local checkoutStep state on Cart type

1a8deba

Signed-off-by: sirugh <rugh@adobe.com>

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

Cleanup

638cb2a

Signed-off-by: sirugh <rugh@adobe.com>

sirugh commented Apr 22, 2020

View reviewed changes

m2-community-project bot assigned sirugh Apr 22, 2020

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

m2-community-project bot added the Progress: review label Apr 22, 2020

sirugh commented Apr 22, 2020

View reviewed changes

PWAStudioBot added pkg:peregrine pkg:venia-ui labels Apr 22, 2020

sirugh changed the title ~~Rugh/pwa 523 checkout step fix on refresh~~ [bug] Use gql persistence for checkout step between refreshes. Apr 23, 2020

sirugh added 2 commits April 23, 2020 10:30

Safely create resolvers for apollo client

c67c738

Signed-off-by: sirugh <rugh@adobe.com>

Move persistor check within async function

75d7022

Signed-off-by: sirugh <rugh@adobe.com>

sirugh commented Apr 23, 2020

View reviewed changes

sirugh added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Apr 23, 2020

Disable template strings for client directives

433ab48

Signed-off-by: sirugh <rugh@adobe.com>

sirugh marked this pull request as ready for review April 23, 2020 16:52

tjwiebell mentioned this pull request Apr 24, 2020

Bug in CheckoutPage: useShippingMethod missing useEffect to call onSave #2357

Closed

8 tasks

sirugh added 2 commits April 24, 2020 09:58

Also delete inmemory persisted cache items

a5fda39

Signed-off-by: sirugh <rugh@adobe.com>

prettier

b225784

Signed-off-by: sirugh <rugh@adobe.com>

tjwiebell suggested changes Apr 24, 2020

View reviewed changes

m2-community-project bot assigned tjwiebell Apr 24, 2020

m2-community-project bot moved this from Review in Progress to Changes Requested in Pull Request Progress Apr 24, 2020

m2-community-project bot added Progress: changes requested and removed Progress: review labels Apr 24, 2020

Merge branch 'develop' into rugh/pwa-523-checkout-step-fix-on-refresh

25c6b32

revanth0212 mentioned this pull request Apr 27, 2020

Checkout Page Payment Information #2320

Merged

sirugh added 2 commits April 27, 2020 10:11

Protects against case where address not yet provided

6130923

Signed-off-by: sirugh <rugh@adobe.com>

Simplify to single regex for cache entry deletion

acf7910

Signed-off-by: sirugh <rugh@adobe.com>

sirugh force-pushed the rugh/pwa-523-checkout-step-fix-on-refresh branch from 111f0bd to acf7910 Compare April 27, 2020 17:07

tjwiebell previously approved these changes Apr 27, 2020

View reviewed changes

jimbo previously approved these changes Apr 28, 2020

View reviewed changes

m2-community-project bot assigned jimbo Apr 28, 2020

m2-community-project bot moved this from Changes Requested to Reviewer Approved in Pull Request Progress Apr 28, 2020

m2-community-project bot added Progress: approved and removed Progress: changes requested labels Apr 28, 2020

sirugh and others added 2 commits April 28, 2020 14:11

Merge branch 'develop' into rugh/pwa-523-checkout-step-fix-on-refresh

c44e05e

Feedback

9deb452

Signed-off-by: sirugh <rugh@adobe.com>

sirugh dismissed stale reviews from jimbo and tjwiebell via 9deb452 April 28, 2020 19:20

Merge branch 'develop' into rugh/pwa-523-checkout-step-fix-on-refresh

2a24059

m2-community-project bot moved this from Reviewer Approved to Review in Progress in Pull Request Progress Apr 29, 2020

m2-community-project bot added Progress: review and removed Progress: approved labels Apr 29, 2020

tjwiebell approved these changes Apr 29, 2020

View reviewed changes

Merge branch 'develop' into rugh/pwa-523-checkout-step-fix-on-refresh

1b61023

dpatil-magento merged commit 947b3c1 into develop Apr 30, 2020

dpatil-magento deleted the rugh/pwa-523-checkout-step-fix-on-refresh branch April 30, 2020 15:47

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

m2-community-project bot removed the Progress: review label Apr 30, 2020

sirugh added version: Minor This changeset includes functionality added in a backwards compatible manner. and removed version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. labels Jun 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bug] Use gql persistence for checkout step between refreshes. #2354

[bug] Use gql persistence for checkout step between refreshes. #2354

sirugh commented Apr 22, 2020 •

edited

sirugh Apr 22, 2020

sirugh Apr 22, 2020

sirugh Apr 22, 2020

PWAStudioBot commented Apr 22, 2020 •

edited

sirugh Apr 23, 2020 •

edited

sirugh Apr 23, 2020

tjwiebell left a comment

tjwiebell Apr 24, 2020

sirugh Apr 24, 2020

tjwiebell Apr 27, 2020

sirugh Apr 27, 2020

tjwiebell Apr 24, 2020

sirugh Apr 27, 2020

tjwiebell Apr 24, 2020

sirugh Apr 27, 2020

tjwiebell Apr 24, 2020

sirugh Apr 24, 2020

sirugh Apr 27, 2020

tjwiebell Apr 24, 2020

sirugh Apr 27, 2020

tjwiebell Apr 24, 2020

sirugh Apr 27, 2020

dpatil-magento commented Apr 25, 2020

sirugh commented Apr 27, 2020

jimbo left a comment

jimbo Apr 28, 2020

jimbo Apr 28, 2020

sirugh Apr 28, 2020

jimbo Apr 28, 2020

sirugh Apr 28, 2020

dpatil-magento commented Apr 29, 2020

tjwiebell left a comment


		const [getCheckoutStep, { data: stepData, client }] = useLazyQuery(
		getCheckoutStepQuery

		await deleteCacheEntry(apolloClient, key => key.match(/^Cart/));
		await deleteCacheEntry(apolloClient, key => key.match(/^\$Cart/));

		!!chosenShippingMethodData.cart.shipping_addresses[0]
		.selected_shipping_method;

[bug] Use gql persistence for checkout step between refreshes. #2354

[bug] Use gql persistence for checkout step between refreshes. #2354

Conversation

sirugh commented Apr 22, 2020 • edited

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PWAStudioBot commented Apr 22, 2020 • edited

sirugh Apr 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjwiebell 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

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

dpatil-magento commented Apr 25, 2020

sirugh commented Apr 27, 2020

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

dpatil-magento commented Apr 29, 2020

tjwiebell left a comment

Choose a reason for hiding this comment

sirugh commented Apr 22, 2020 •

edited

PWAStudioBot commented Apr 22, 2020 •

edited

sirugh Apr 23, 2020 •

edited