Order confirmation page multi refresh fix #2433

revanth0212 · 2020-05-26T20:45:22Z

Description

useCheckoutPage talon has a useEffect that fetches checkout details everytime the cartId changes. When the user places an order, a new cartId will be created because the old one will be purged. Due to this, the useEffect was being triggered and in the process isLoading was changing to true. Because of this, the user will see a loading screen for a bit and the order confirmation page will be reloaded. This is un-intended behavior.

To fix this, I have added a check to only call the checkout details query if the order has not been placed.

The talon involved in this PR does not have a tests file defined. I need to create and add some tests.

Related Issue

Closes PWA-580

Verification Stakeholders

@sirugh

Verification Steps

Go to the deployed app and try to check out a product.
When the order confirmation page loads, there should not be page flashes.

Checklist

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

PWAStudioBot · 2020-05-26T21:00:25Z

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

Generated by 🚫 dangerJS against b98ec0f

devops-pwa-codebuild · 2020-05-26T21:04:00Z

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-2433.pwa-venia.com : LH Performance Expected 0.85 Actual 0.57, LH Best Practices Expected 1 Actual 0.92
https://pr-2433.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.32, LH Best Practices Expected 1 Actual 0.92
https://pr-2433.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.51, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92

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

revanth0212 · 2020-05-26T22:02:07Z

I am gonna add more tests soon.

sirugh · 2020-05-27T16:11:39Z

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

+         * `cartId`, since the order has been placed and the UI
+         * will be in the order confirmation step.
+         */
+        if (cartId && !placedOrder) {


@tjwiebell had a really good idea in his auth shipping PR. Instead of conditions in effects to invoke lazy queries we can just use the skip property. The original intent behind the lazy query was to re-fetch data on UI components that are always rendered like the cart trigger. It needed to know if cart id changed so it could re-query for new data.

const { data: checkoutData } = useQuery(getCheckoutDetails, { skip: !cartId, variables: { cartId } });

However after I mention this I realize that it is currently possible for a user to log out/log in while in cart/checkout (via left drawer) which would require we re-fetch the data. I don't know if this would re-fetch in those cases.

I am fine doing it either way. I like this style as well. Just a small change though, the skip variable value will be !cartId || placeOrderData.

However after I mention this I realize that it is currently possible for a user to log out/log in while in cart/checkout which would require we re-fetch the data. I don't know if this would re-fetch in those cases.

Ideally speaking it should. Technically speaking, it should since the cartId changes once you log in or out.

I have tried this and it worked like a charm. When I logged in and out, I could see that the query was being fired.

What do you guys think? Should we stick with this approach?

I say yes.

Did it work when logging in and logging out? The checkout page reflected the user state ie showed authed user cart after logging in and showed empty cart after logging out?

Yeah, it did. Because when we log we use the cartId associated with the user and the query would pick that up and do its magic. Once the user logs out, a new cartId is created so the query checks and realizes there are not cart items on it. That's my understanding.

Quick point, I have tried both the use cases we are discussing here, useEffect and skip, unfortunately, they are a bit diff. I have documented it in this video.

TLDR, useEffect does not get invoked if the value of the variable changes to the same value like, 123 -> 123 but the skip variable does not recognize that. It just sees that the value has changed but not the actual value. It does not compare the new value to the old value. When ever the value changes, if that value passes the conditional, the useQuery will perform its action.

Found the piece of code that is causing the loading screen flash.

The loading screen is controlled by the boolean isLoading in the useCheckoutPage talon. This is how the isLoading is defined:

isLoading: !checkoutCalled || (checkoutCalled && checkoutLoading)

It reads as:

if the checkout details query is not called, show the loading screen

if it is called and is loading, show the loading screen

If you pay attention to the 2 screenshots below, in case of useEffect, checkoutCalled is not set to true till it is called, ie. the useEffect is executed.

In case of useQuery, it is called when the component is rendered, hence checkoutCalled is set to true even on the first render but it is not loading yet. Hence the isLoading boolean is false first, then when checkoutLoading is actually set to true, isLoading is set to true.

I hope this explanation of mine is understandable 🤣

useEffect

skip

Maybe we should revisit the apollo docs for loading states: https://www.apollographql.com/docs/react/data/queries/#inspecting-loading-states

I do wonder, since apollo docs all just seem to use loading, do we need to check called? Do the children render before loading is true?

Pstephen the savior. Using the networkStatus variable from the useQuery worked dude. 3bca3e0

Should I be happy that the tests failed to catch the change or sad that I have to update them? 🤔

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

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

tjwiebell · 2020-05-27T17:12:22Z

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

@@ -0,0 +1,209 @@
+import React from 'react';


Thank you @revanth0212 for adding this test coverage 🙏 was just about to do the same in scope of my PR, glad I took a peek at this.

…/github.com/magento/pwa-studio into revanth/order_confirmation_multi_reload_fix

supernova-at

I'd really like to see the Apollo network statuses split out for re-use.

supernova-at · 2020-05-28T15:14:24Z

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

+     */
+    const isLoading = useMemo(
+        () =>
+            checkoutQueryNetworkStatus ? checkoutQueryNetworkStatus < 7 : true,


The comment is nice but I'd also like us to have a utility enumeration that exports Apollo's network status codes to make this more readable.

https://github.com/apollographql/apollo-client/blob/master/src/core/networkStatus.ts

This would become something like

checkoutQueryNetworkStatus ? checkoutQueryNetworkStatus < ApolloNetworkStatuses.READY : true

That source file also exposes an isNetworkStatusInFlight utility function that seems to be performing the same check you are here. Other places in our code could benefit from this as well.

Nice idea @supernova-at. Will do.

I don't think we should export the network status from this talon. It can be something from util or a defaults file. I can do that if that is what you are expecting here.

Also another point, isNetworkStatusInFlight differs from our implementation. The else case in isNetworkStatusInFlight returns false but in our case, it has to return true.

Yeah, I was definitely thinking a separate file that this and other talons could use.

supernova-at · 2020-05-28T15:26:39Z

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

+
+    expect(talonProps.isLoading).toBeTruthy();
+
+    getCheckoutDetailsQueryResult.mockReturnValueOnce({


Starting from this line down should be its own separate test. "isLoading should be set to false if the checkout details query networkStatus is greater than or equal to 7".

I'd also test seven (7) explicitly since that's where the errors will likely occur.

supernova-at · 2020-05-28T15:28:30Z

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

+    });
+});
+
+test('isGuestCheckout should be negation of isSignedIn from useUserContext', () => {


Careful of tests like this, in my opinion this is a little too implementation-specific and therefore brittle.

@supernova-at this is a good point and leads me to believe we should drop the isGuestCheckout value and just be returning isSignedIn for use in the UI component.

This raises a question for dev time though. In most of our talons we return some simple boolean states such as loading or render flags such as isSignedIn. How should we test those?

I wanted to test the fact that it is dependent on isSignedIn which is from another talon. I wanted to do this because, if someone changes the logic, the test has to fail.

All this is based on my assumptions about testing such booleans. Not sure if that is the right assumption.

@sirugh nice idea. Good topic to discuss.

revanth0212 · 2020-05-29T16:45:04Z

Reverted NETOWRK_STATUS changes 🤣 b176795

supernova-at

Looks good! One minor comment but I'll leave it up to you whether you want to change it or not.

supernova-at · 2020-05-29T18:56:11Z

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

+    const isLoading = useMemo(() => {
+        const checkoutQueryInFlight = checkoutQueryNetworkStatus
+            ? checkoutQueryNetworkStatus < 7
+            : true;


Since network status is 1-based, here's an alternate approach that reads a little easier in my opinion:

const checkoutQueryInFlight = !checkoutQueryNetworkStatus || checkoutQueryNetworkStatus < 7;

Nice approach Andy, but I would like to stick to the ternary operator approach. In my opinion, in this case, it is easy to read than the || approach.

revanth0212 added 3 commits May 26, 2020 14:03

Refetch checkout details only if place order mutation is not called.

3011f29

Minor.

36d97ce

Reverting temp change.

54d1080

revanth0212 added the version: Patch This changeset includes backwards compatible bug fixes. label May 26, 2020

PWAStudioBot added the pkg:peregrine label May 26, 2020

revanth0212 added 2 commits May 26, 2020 16:57

Using apollo client which is same as client from useLazyQuery.

9838ae6

Added initial snapshot test. More to follow.

ed8fded

revanth0212 commented May 26, 2020

View reviewed changes

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

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

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

Added test to replicate the issue being fixed.

cbf0cd0

sirugh reviewed May 27, 2020

View reviewed changes

m2-community-project bot assigned sirugh May 27, 2020

supernova-at requested changes May 27, 2020

View reviewed changes

m2-community-project bot assigned supernova-at May 27, 2020

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

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

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

sirugh reviewed May 27, 2020

View reviewed changes

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

tjwiebell reviewed May 27, 2020

View reviewed changes

client -> apolloClient

e4e5115

revanth0212 and others added 6 commits May 27, 2020 14:40

useEffect -> skip

3bca3e0

Addressed PR comments.

b1ff493

Updated tests.

0efb656

Merge branch 'develop' into revanth/order_confirmation_multi_reload_fix

97d05a7

Added more tests.

d295865

Merge branch 'revanth/order_confirmation_multi_reload_fix' of https:/…

55a136f

…/github.com/magento/pwa-studio into revanth/order_confirmation_multi_reload_fix

supernova-at requested changes May 28, 2020

View reviewed changes

revanth0212 added 5 commits May 28, 2020 10:44

Added more tests.

b40c013

Addressed PR comments.

caf16a2

Merged develop into current.

dcf424c

Reverted network status changes.

b176795

Merged develop into current.

42f4178

supernova-at approved these changes May 29, 2020

View reviewed changes

Merge branch 'develop' into revanth/order_confirmation_multi_reload_fix

b98ec0f

dpatil-magento merged commit c33e48f into develop May 29, 2020

dpatil-magento deleted the revanth/order_confirmation_multi_reload_fix branch May 29, 2020 21:16

m2-community-project bot removed the Progress: review label May 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Order confirmation page multi refresh fix #2433

Order confirmation page multi refresh fix #2433

revanth0212 commented May 26, 2020

PWAStudioBot commented May 26, 2020 •

edited

Loading

devops-pwa-codebuild commented May 26, 2020 •

edited

Loading

revanth0212 commented May 26, 2020

sirugh May 27, 2020 •

edited

Loading

revanth0212 May 27, 2020 •

edited

Loading

revanth0212 May 27, 2020 •

edited

Loading

sirugh May 27, 2020

revanth0212 May 27, 2020

revanth0212 May 27, 2020 •

edited

Loading

revanth0212 May 27, 2020 •

edited

Loading

sirugh May 27, 2020 •

edited

Loading

revanth0212 May 27, 2020 •

edited

Loading

revanth0212 May 27, 2020

tjwiebell May 27, 2020

supernova-at left a comment

supernova-at May 28, 2020

supernova-at May 28, 2020

revanth0212 May 28, 2020 •

edited

Loading

supernova-at May 28, 2020

supernova-at May 28, 2020

supernova-at May 28, 2020

sirugh May 28, 2020

revanth0212 May 28, 2020 •

edited

Loading

revanth0212 commented May 29, 2020

supernova-at left a comment

supernova-at May 29, 2020

revanth0212 May 29, 2020


		expect(talonProps.isLoading).toBeTruthy();

		getCheckoutDetailsQueryResult.mockReturnValueOnce({

Order confirmation page multi refresh fix #2433

Order confirmation page multi refresh fix #2433

Conversation

revanth0212 commented May 26, 2020

Description

Related Issue

Verification Stakeholders

Verification Steps

Checklist

PWAStudioBot commented May 26, 2020 • edited Loading

devops-pwa-codebuild commented May 26, 2020 • edited Loading

revanth0212 commented May 26, 2020

sirugh May 27, 2020 • edited Loading

Choose a reason for hiding this comment

revanth0212 May 27, 2020 • edited Loading

Choose a reason for hiding this comment

revanth0212 May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revanth0212 May 27, 2020 • edited Loading

Choose a reason for hiding this comment

revanth0212 May 27, 2020 • edited Loading

Choose a reason for hiding this comment

sirugh May 27, 2020 • edited Loading

Choose a reason for hiding this comment

revanth0212 May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernova-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revanth0212 May 28, 2020 • edited Loading

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 28, 2020 • edited Loading

Choose a reason for hiding this comment

revanth0212 commented May 29, 2020

supernova-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PWAStudioBot commented May 26, 2020 •

edited

Loading

devops-pwa-codebuild commented May 26, 2020 •

edited

Loading

sirugh May 27, 2020 •

edited

Loading

revanth0212 May 27, 2020 •

edited

Loading

revanth0212 May 27, 2020 •

edited

Loading

revanth0212 May 27, 2020 •

edited

Loading

revanth0212 May 27, 2020 •

edited

Loading

sirugh May 27, 2020 •

edited

Loading

revanth0212 May 27, 2020 •

edited

Loading

revanth0212 May 28, 2020 •

edited

Loading

revanth0212 May 28, 2020 •

edited

Loading