Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Order confirmation page multi refresh fix #2433

Merged
merged 19 commits into from
May 29, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Should return correct shape 1`] = `
Object {
"checkoutStep": null,
"error": null,
"handlePlaceOrder": [Function],
"handleReviewOrder": [Function],
"handleSignIn": [Function],
"hasError": false,
"isCartEmpty": true,
"isGuestCheckout": true,
"isLoading": true,
"isUpdating": false,
"orderDetailsData": null,
"orderDetailsLoading": false,
"orderNumber": null,
"placeOrderLoading": false,
"resetReviewOrderButtonClicked": [Function],
"reviewOrderButtonClicked": false,
"setIsUpdating": [Function],
"setPaymentInformationDone": [Function],
"setShippingInformationDone": [Function],
"setShippingMethodDone": [Function],
}
`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,209 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

import {
useLazyQuery,
useApolloClient,
useMutation
} from '@apollo/react-hooks';
import { act } from 'react-test-renderer';

import { useCheckoutPage } from '../useCheckoutPage';
import createTestInstance from '../../../util/createTestInstance';
import { useCartContext } from '../../../context/cart';

/**
* Mocks
*/

jest.mock('@apollo/react-hooks', () => {
return {
useLazyQuery: jest.fn(),
useApolloClient: jest.fn(),
useMutation: jest.fn()
};
});

jest.mock('../../../context/user', () => ({
useUserContext: jest.fn().mockReturnValue([{ isSignedIn: false }])
}));

jest.mock('../../../context/app', () => ({
useAppContext: jest.fn().mockReturnValue([{}, { toggleDrawer: jest.fn() }])
}));

jest.mock('../../../context/cart', () => ({
useCartContext: jest
.fn()
.mockReturnValue([
{ cartId: '123' },
{ createCart: jest.fn(), removeCart: jest.fn() }
])
}));

jest.mock('../../../Apollo/clearCartDataFromCache', () => ({
clearCartDataFromCache: jest.fn()
}));

/**
* Constants
*/

const createCartMutation = 'createCartMutation';
const placeOrderMutation = 'placeOrderMutation';
const getCheckoutDetailsQuery = 'getCheckoutDetailsQuery';
const getOrderDetailsQuery = 'getOrderDetailsQuery';

const props = {
mutations: { createCartMutation, placeOrderMutation },
queries: { getCheckoutDetailsQuery, getOrderDetailsQuery }
};

const readQuery = jest.fn();
const writeQuery = jest.fn();
const client = { readQuery, writeQuery };

const getOrderDetails = jest.fn();
const getOrderDetailsQueryResult = jest.fn().mockReturnValue([
getOrderDetails,
{
data: null,
loading: false,
called: false
}
]);
const getCheckoutDetails = jest.fn();
const getCheckoutDetailsQueryResult = jest.fn().mockReturnValue([
getCheckoutDetails,
{
data: null,
loading: false,
called: false
}
]);

const createCart = jest.fn();
const createCartMutationResult = jest.fn().mockReturnValue([
createCart,
{
error: null,
loading: false,
data: null
}
]);
const placeOrder = jest.fn();
const placeOrderMutationResult = jest.fn().mockReturnValue([
placeOrder,
{
error: null,
loading: false,
data: null
}
]);

/**
* Helpers
*/

const Component = props => {
const talonProps = useCheckoutPage(props);

return <i talonProps={talonProps} />;
};

const getTalonProps = props => {
const tree = createTestInstance(<Component {...props} />);
const { root } = tree;
const { talonProps } = root.findByType('i').props;

const update = newProps => {
act(() => {
tree.update(<Component {...{ ...props, ...newProps }} />);
});

return root.findByType('i').props.talonProps;
};

return { talonProps, tree, update };
};

/**
* beforeAll
*/

beforeAll(() => {
useLazyQuery.mockImplementation(query => {
if (query === getCheckoutDetailsQuery) {
return getCheckoutDetailsQueryResult();
} else if (query === getOrderDetailsQuery) {
return getOrderDetailsQueryResult();
} else {
return [jest.fn(), {}];
}
});

useMutation.mockImplementation(mutation => {
if (mutation === createCartMutation) {
return createCartMutationResult();
} else if (mutation === placeOrderMutation) {
return placeOrderMutationResult();
} else {
return [jest.fn(), {}];
}
});

useApolloClient.mockReturnValue(client);
});

/**
* Tests
*/

test('Should return correct shape', () => {
const { talonProps } = getTalonProps(props);

expect(talonProps).toMatchSnapshot();
});

test('Should get checkout details if cartId changes and order has not been placed yet', () => {
useCartContext.mockReturnValueOnce([
{ cartId: '123' },
{ createCart: jest.fn(), removeCart: jest.fn() }
]);

const { update } = getTalonProps(props);

expect(getCheckoutDetails.mock.calls[0][0]).toMatchObject({
variables: { cartId: '123' }
});
expect(getCheckoutDetails.mock.calls.length).toBe(1);

useCartContext.mockReturnValueOnce([
{ cartId: 'abc' },
{ createCart: jest.fn(), removeCart: jest.fn() }
]);

update();

expect(getCheckoutDetails.mock.calls[1][0]).toMatchObject({
variables: { cartId: 'abc' }
});
expect(getCheckoutDetails.mock.calls.length).toBe(2);

useCartContext.mockReturnValueOnce([
{ cartId: 'xyz' },
{ createCart: jest.fn(), removeCart: jest.fn() }
]);
placeOrderMutationResult.mockReturnValueOnce([
placeOrder,
{
error: null,
loading: false,
data: {
placeOrder: { order: { order_number: '000000' } }
}
}
]);

update();

expect(getCheckoutDetails.mock.calls.length).toBe(2);
});
29 changes: 18 additions & 11 deletions packages/peregrine/lib/talons/CheckoutPage/useCheckoutPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export const useCheckoutPage = props => {
false
);

const apolloClient = useApolloClient();
const client = useApolloClient();
const [isUpdating, setIsUpdating] = useState(false);

const [, { toggleDrawer }] = useAppContext();
Expand Down Expand Up @@ -56,12 +56,7 @@ export const useCheckoutPage = props => {

const [
getCheckoutDetails,
{
data: checkoutData,
called: checkoutCalled,
client,
loading: checkoutLoading
}
{ data: checkoutData, called: checkoutCalled, loading: checkoutLoading }
] = useLazyQuery(getCheckoutDetailsQuery);

const checkoutStep = checkoutData && checkoutData.cart.checkoutStep;
Expand Down Expand Up @@ -131,13 +126,13 @@ export const useCheckoutPage = props => {

await removeCart();

await clearCartDataFromCache(apolloClient);
await clearCartDataFromCache(client);

await createCart({
fetchCartId
});
}, [
apolloClient,
client,
cartId,
createCart,
fetchCartId,
Expand All @@ -147,14 +142,26 @@ export const useCheckoutPage = props => {
]);

useEffect(() => {
if (cartId) {
const placedOrder = !!placeOrderData;

/**
* Get checkout details everytime `cartId` changes
* but not if the place order mutation is called
* and `cartId` changes because of that.This is
* because after the place order mutation is completed
* old `cartId` will be purged and a new one will be
* created. we wont need checkout details for that
* `cartId`, since the order has been placed and the UI
* will be in the order confirmation step.
*/
if (cartId && !placedOrder) {
Copy link
Contributor

@sirugh sirugh May 27, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@revanth0212 revanth0212 May 27, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@revanth0212 revanth0212 May 27, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@revanth0212 revanth0212 May 27, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@revanth0212 revanth0212 May 27, 2020

Choose a reason for hiding this comment

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

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:

  1. if the checkout details query is not called, show the loading screen
  2. 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

image

skip

image

Copy link
Contributor

@sirugh sirugh May 27, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@revanth0212 revanth0212 May 27, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

getCheckoutDetails({
variables: {
cartId
}
});
}
}, [cartId, getCheckoutDetails]);
}, [cartId, placeOrderData, getCheckoutDetails]);

return {
checkoutStep,
Expand Down