Uses graphQl mutation to create/fetch cartId. #1988

sirugh · 2019-11-20T18:11:11Z

Description

NOTE: REST endpoint returns data for "authed" cart created through graphql. To repro, log in, add item to cart, then delete sign in token from local storage and refresh. Cart details should be fetched but you should not be able to add/update/remove. If you take an action on the cart it will reset to a new cart. This could be a bug in REST. It should not matter since those actions are moving to graphql which won't return data for unauthed user.

This PR precedes #1987.

Changes include:

Proper use of cart id for quote id

GraphQL endpoints use the masked (hashed) cartId regardless of auth state. REST uses the "actual" (table key value) cartId as quoteId for authed requests and masked cartId as quoteId for guest requests. This means that while we are in this intermediate state of some requests being through graphql (such as createCart and addItemToCart) we have to make sure the REST calls use the appropriate cartId.

Passing of fetch/invoker to createCart

The majority of this PR is to pass the mutation callback into the actions that may need to call createCart.

Cleanup of reducer state

We were passing and spreading unnecessary info to the store in the receive actions of certain asyncActions. I removed this as it was extraneous.

Related Issue

Closes PWA-140.

Acceptance

Verification Stakeholders

@jimbo

Specification

Verification Steps

Clear storage and load the app.
Check network for the graphql for createCart. Note the response cartId.
Check local storage and you should see the cartId in local storage.
Add an item to cart.
Update an item in cart.
Remove an item from cart.
Remove all items from cart.
Sign in, add an item, then expire the cart from the admin console and try to remove the item. It should still function same as it did before.

Screenshots / Screen Captures (if appropriate)

Checklist

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

…ateCart asyncAction with removal of cartId and an effect that refetches a new cart.

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

packages/venia-ui/lib/components/App/app.js

sirugh · 2019-11-20T18:14:37Z

packages/peregrine/lib/store/actions/user/asyncActions.js

 import { clearCheckoutDataFromStorage } from '../checkout';

 import actions from './actions';

 const { request } = Magento2;
 const storage = new BrowserPersistence();

-export const signIn = credentials =>


Should have deleted this in the #1904 but I didn't. Doing it now since I'm trying to clean up refs to getCartDetails.

Did you create a ticket to bring this action back? Would be nice if it referenced this comment on the off chance one of us doesn't pick it up.

You also left SIGN_IN in the action map, but I'm fine not touching it since it should be coming back.

I didn't -- I don't think it's necessary right now. You?

I would probably want it moved before the next release, just so we're providing a consistent pattern for other developers to follow. Given useSignIn doesn't have any data fetching or effects that make our custom hooks harder to understand, doesn't have to be right now.

PWAStudioBot · 2019-11-20T18:16: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-140.

Generated by 🚫 dangerJS against 53c61a7

packages/peregrine/lib/store/reducers/cart.js

…riggering recreation of cart

sirugh · 2019-11-21T17:38:38Z

packages/peregrine/lib/store/reducers/cart.js

@@ -74,10 +76,7 @@ const reducerMap = {
            isAddingItem: false
        };
    },
-    [actions.updateItem.request]: (state, { payload, error }) => {
-        if (error) {
-            return initialState;


We should not be passing error to the request sync actions, only the receive.

sirugh · 2019-11-21T17:39:16Z

packages/peregrine/lib/store/reducers/cart.js

@@ -43,7 +46,6 @@ const reducerMap = {
            return {
                ...state,
                detailsError: payload,
-                cartId: null,


Removing cartId should be done through removeCart.

This makes me nervous why this existed, but I couldn't find any components observing this state, so seems safe 🤞

sirugh · 2019-11-21T17:39:33Z

packages/peregrine/lib/store/reducers/cart.js

        return {
            ...state,
-            cartId: String(payload),


The only thing setting cartId should be getCart action.

For context, why did this action accept cartId as a payload in the first place, and why did the reducer take this opportunity to set it in state?

No idea. Requests are only meant to set loading states in redux right now. Even that will eventually move to UI I believe. At that point all we will are about are .receive sync actions.

jimbo · 2019-11-21T17:52:31Z

packages/peregrine/lib/store/actions/cart/asyncActions.js

+                        'Missing required information: cartId'
+                    );
+                    missingCartIdError.noCartId = true;
+                    throw missingCartIdError;


How do we reach this case? I don't mind throwing a helpful error, but I want to ensure that something like this should never occur.

We should never not have a cartId. These are just leftover from before. I can remove them. They won't matter when we move these functions to the UI components.

No, please don't remove them. It doesn't hurt to have a helpful error thrown if we reach a state we shouldn't.

I just wanted to make sure that we weren't expecting this case to come up.

jimbo · 2019-11-21T18:27:57Z

packages/peregrine/lib/store/reducers/cart.js

        return {
            ...state,
-            cartId: String(payload),


For context, why did this action accept cartId as a payload in the first place, and why did the reducer take this opportunity to set it in state?

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

packages/venia-ui/lib/components/App/app.js

tjwiebell

So clean; loved that a bulk of the change was in actions, and reviewing the UI code was super simple. Minor suggestions to remove legacy logic and clean things up. Very nice 👌

packages/peregrine/lib/store/actions/cart/asyncActions.js

tjwiebell · 2019-11-27T20:26:07Z

packages/peregrine/lib/store/reducers/cart.js

@@ -43,7 +46,6 @@ const reducerMap = {
            return {
                ...state,
                detailsError: payload,
-                cartId: null,


This makes me nervous why this existed, but I couldn't find any components observing this state, so seems safe 🤞

packages/venia-ui/lib/components/Header/cartTrigger.js

jimbo

I like this approach. This should help us get onto GraphQL without too much churn while we wait for Suspense to mature.

We'll need to do a pretty thorough QA pass on it.

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

jimbo · 2019-11-27T21:57:30Z

packages/peregrine/lib/store/actions/checkout/asyncActions.js

@@ -38,7 +43,7 @@ export const cancelCheckout = () =>
 export const resetCheckout = () =>
    async function thunk(dispatch) {
        await dispatch(closeDrawer());
-        await dispatch(createCart());
+        await dispatch(removeCart());


Should be fine.

tjwiebell

LGTM 👍

tjwiebell

Merge conflict resolution looks good 👍

… is authed or guest

tjwiebell

Discussed IRL, change is needed given GraphQL limitation in 2.3.3. Will re-visit once 2.3.4 drops, and we can fetch persistent Customer carts.

tjwiebell

Sorry 😬 please add a test for this

packages/peregrine/lib/store/actions/cart/__tests__/asyncActions.spec.js

tjwiebell

…ed in store

Uses graphQl mutation to create/fetch cartId. Replaces all use of cre…

5d776f8

…ateCart asyncAction with removal of cartId and an effect that refetches a new cart.

sirugh commented Nov 20, 2019

View reviewed changes

packages/peregrine/lib/talons/AuthModal/useAuthModal.js Outdated Show resolved Hide resolved

m2-community-project bot assigned sirugh Nov 20, 2019

m2-community-project bot added the Progress: review label Nov 20, 2019

sirugh commented Nov 20, 2019

View reviewed changes

packages/venia-ui/lib/components/App/app.js Outdated Show resolved Hide resolved

sirugh commented Nov 20, 2019

View reviewed changes

PWAStudioBot added pkg:peregrine pkg:venia-ui labels Nov 20, 2019

sirugh commented Nov 20, 2019

View reviewed changes

packages/peregrine/lib/store/reducers/cart.js Outdated Show resolved Hide resolved

sirugh added 6 commits November 20, 2019 15:04

Prevent creating a new cart when last cart fetch is in flight

3fca50d

Fix createAccount test

68a0efd

Fix cart asyncAction tests

9363009

Fix tests for app

b77b365

Fix tests for signIn

ec457e4

Fix tests for checkout asyncActions

cebb806

sirugh marked this pull request as ready for review November 20, 2019 23:19

sirugh added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Nov 20, 2019

sirugh added 5 commits November 20, 2019 18:15

Attempt to fix setting cart to undefined on load

fcf4b1d

Use correct quoteId instead of cartId. On error, remove cartId thus t…

e827ddd

…riggering recreation of cart

Fix cart reducer tests

db678f5

Remove incorrect use of await of sync dispatch

3867ed7

Fix cart tests

475e773

sirugh commented Nov 21, 2019

View reviewed changes

jimbo reviewed Nov 21, 2019

View reviewed changes

m2-community-project bot assigned jimbo Nov 21, 2019

Throw mutation error

68c5c39

tjwiebell suggested changes Nov 27, 2019

View reviewed changes

jimbo reviewed Nov 27, 2019

View reviewed changes

sirugh and others added 2 commits December 2, 2019 11:53

Address feedback

b53de4f

Merge branch 'develop' into rugh/createCart-graphql

0d27590

tjwiebell previously approved these changes Dec 2, 2019

View reviewed changes

Merge branch 'develop' into rugh/createCart-graphql

41d1aca

sirugh dismissed tjwiebell’s stale review via 41d1aca December 3, 2019 15:22

tjwiebell previously approved these changes Dec 3, 2019

View reviewed changes

lint and prettier

efcb62b

sirugh dismissed tjwiebell’s stale review via efcb62b December 3, 2019 15:32

tjwiebell previously approved these changes Dec 3, 2019

View reviewed changes

sirugh added 2 commits December 3, 2019 12:40

If cartId is in storage we will use it now regardless of whether user…

4ef1998

… is authed or guest

Removes outdated test

c6d6be1

sirugh dismissed tjwiebell’s stale review via c6d6be1 December 3, 2019 18:42

Merge branch 'develop' into rugh/createCart-graphql

df4b9b1

tjwiebell previously approved these changes Dec 3, 2019

View reviewed changes

Properly catch and handle errors from fetchCart

8c680b8

sirugh dismissed tjwiebell’s stale review via 8c680b8 December 3, 2019 20:25

tjwiebell suggested changes Dec 3, 2019

View reviewed changes

packages/peregrine/lib/store/actions/cart/__tests__/asyncActions.spec.js Show resolved Hide resolved

Add test for error

3a86b52

tjwiebell previously approved these changes Dec 3, 2019

View reviewed changes

sirugh added 2 commits December 4, 2019 11:25

Update details after cart creation to ensure cart details id is updat…

f0a7756

…ed in store

Update tests

53c61a7

sirugh dismissed tjwiebell’s stale review via 53c61a7 December 4, 2019 17:35

tjwiebell approved these changes Dec 4, 2019

View reviewed changes

revanth0212 merged commit f4ed209 into develop Dec 4, 2019

m2-community-project bot removed the Progress: review label Dec 4, 2019

sirugh deleted the rugh/createCart-graphql branch March 4, 2020 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uses graphQl mutation to create/fetch cartId. #1988

Uses graphQl mutation to create/fetch cartId. #1988

sirugh commented Nov 20, 2019 •

edited

Loading

sirugh Nov 20, 2019

tjwiebell Dec 2, 2019

sirugh Dec 2, 2019

tjwiebell Dec 2, 2019

PWAStudioBot commented Nov 20, 2019 •

edited

Loading

sirugh Nov 21, 2019

sirugh Nov 21, 2019

tjwiebell Nov 27, 2019

sirugh Nov 21, 2019

jimbo Nov 21, 2019

sirugh Nov 21, 2019

jimbo Nov 21, 2019

sirugh Nov 21, 2019

jimbo Nov 21, 2019

jimbo Nov 21, 2019

tjwiebell left a comment

tjwiebell Nov 27, 2019

jimbo left a comment

jimbo Nov 27, 2019

tjwiebell left a comment

tjwiebell left a comment

tjwiebell left a comment •

edited

Loading

tjwiebell left a comment

tjwiebell left a comment

Uses graphQl mutation to create/fetch cartId. #1988

Uses graphQl mutation to create/fetch cartId. #1988

Conversation

sirugh commented Nov 20, 2019 • edited Loading

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

Choose a reason for hiding this comment

PWAStudioBot commented Nov 20, 2019 • 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

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

tjwiebell left a comment

Choose a reason for hiding this 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

tjwiebell left a comment

Choose a reason for hiding this comment

tjwiebell left a comment

Choose a reason for hiding this comment

tjwiebell left a comment • edited Loading

Choose a reason for hiding this comment

tjwiebell left a comment

Choose a reason for hiding this comment

tjwiebell left a comment

Choose a reason for hiding this comment

sirugh commented Nov 20, 2019 •

edited

Loading

PWAStudioBot commented Nov 20, 2019 •

edited

Loading

tjwiebell left a comment •

edited

Loading