Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Use graphql to add item to cart #1987

Merged
merged 19 commits into from
Dec 11, 2019
Merged

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented Nov 19, 2019

Description

  • Updates addItemToCart to pass mutations for adding different product types to the cart.

  • Updates updateItemInCart to pass the necessary mutations for add item since addItem action was used as a fallback case.

  • Updates retry logic for graphQL response in the addItem error catch.

Related Issue

Closes PWA-139.

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Add a simple product to cart (no options ie jewelry). The cart should display properly with the item.
  2. Add a configurable product to cart (a dress or something). The cart should display properly with the item.
  3. Log in and add an item to the cart. It should display properly.
  4. Log out and add an item to the cart. It should display properly.
  5. Edit an item in the cart. It should update properly.
  6. Remove an item from the cart. It should be removed properly.

Screenshots / Screen Captures (if appropriate)

Checklist

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

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Nov 19, 2019

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

Generated by 🚫 dangerJS against b9dfa79

@sirugh sirugh force-pushed the rugh/addItemToCart-graphql branch 3 times, most recently from 8efd829 to cdb77c3 Compare November 20, 2019 00:02
@sirugh sirugh changed the title Rugh/add item to cart graphql Use graphql to add item to cart Nov 20, 2019
@sirugh sirugh changed the base branch from develop to rugh/createCart-graphql November 20, 2019 18:14
@sirugh sirugh force-pushed the rugh/addItemToCart-graphql branch 2 times, most recently from 43bbd1f to d732f21 Compare November 20, 2019 18:21
@sirugh sirugh marked this pull request as ready for review November 27, 2019 16:26
@sirugh sirugh force-pushed the rugh/addItemToCart-graphql branch 2 times, most recently from 64183b4 to 4c15cd3 Compare December 3, 2019 22:10
@@ -49,22 +43,6 @@ export const useMiniCart = props => {
setIsEditingItem(false);
}, []);

const handleUpdateItemInCart = useCallback(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this to cartOptions where it was more appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, found it! I assume this is the remnant of a re-factor, but I would consider removing the try/catch if its not reachable (in the other file).

@@ -134,6 +145,16 @@ export const addItemToCart = (payload = {}) => {
};
};

// Returns true if the cart is invalid.
function hasRetryableGqlError(error) {
Copy link
Contributor Author

@sirugh sirugh Dec 4, 2019

Choose a reason for hiding this comment

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

This function feels nasty. I don't like matching some string value from the API. Would rather we be able to use some error id map. GQL always returns 200 so we no longer get 404 for invalid carts. I guess it depends how resilient category is in the graphql error response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out that category: "graphql-no-such-entity" is used if anything within the response is not found. To ensure we only retry on an invalid cart, and not on an invalid/oos item, we should use the message string.

@sirugh sirugh changed the base branch from rugh/createCart-graphql to develop December 4, 2019 20:02
@sirugh sirugh changed the base branch from develop to rugh/createCart-graphql December 4, 2019 20:02
tjwiebell
tjwiebell previously approved these changes Dec 9, 2019
@dpatil-magento
Copy link
Contributor

dpatil-magento commented Dec 9, 2019

@sirugh @tjwiebell Add item to cart and place order >> now close checkout panel by clicking close icon or by clicking in greyed out area and try adding same product again. Product is not getting added to cart.
Note - Once we open cart manually after that it starts working.
Works fine on develop branch.

@tjwiebell
Copy link
Contributor

@sirugh - Looks like the cartId check was removed, so its now possible to enter with a null cartId after checkout. We assumed this would fall into the retry logic with error message pattern matching, but its throwing a different message because of type validation, null !== String!. Would like to push this through, some let me know if you'd like me to just a push a solution and get this approved w/o ya.

Possible solutions:

  1. Pattern match this new message in isInvalidCart
  2. Re-add init cart if cartId not present before add

Message
Error: GraphQL error: Variable "$cartId" got invalid value null; Expected non-nullable type String! not to be null.

method: 'POST',
body: JSON.stringify({ cartItem })
const { cart } = getState();
const { cartId } = cart;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this can be null after an order is placed, and a subsequent add calls this with a null cartId.

@tjwiebell suggested two options, but I think it may be better to figure out why the cart is deleted but never re-created after order. It should definitely be created every time the cartId is deleted.

@sirugh
Copy link
Contributor Author

sirugh commented Dec 9, 2019

@dpatil-magento I believe I "fixed" the issue :) Tried it myself and I'm able to add immediately after checkout.

@dpatil-magento
Copy link
Contributor

@sirugh Recent commit has broken checkout flow. After placing an order as guest/registered user, error is being displayed.

image

@dpatil-magento
Copy link
Contributor

@sirugh Dont see error anymore but after placing order for fraction of second checkout form displayed again (Slow 3G screen captured below). Also "Create an Account" button is not functional anymore and original bug has reappeared (after placing order unable to add product to cart).

image

@sirugh
Copy link
Contributor Author

sirugh commented Dec 10, 2019

The checkout form being visible for a fraction of a second is because we await the completion of checkout before we show the receipt. Something must be modifying the editing state in between submission though -- I can look into it.

^ Turns out this is just visible because we now await a create cart call, which takes a little longer, before we show the next step. We probably don't need to await that call though, I can remove the await.

As for the createAccount button not working on receipt, I can also fix.

@dpatil-magento
Copy link
Contributor

@sirugh Looks good now. Can be merged once review is approved.

@dpatil-magento dpatil-magento merged commit 8e675f4 into develop Dec 11, 2019
@sirugh sirugh deleted the rugh/addItemToCart-graphql branch March 4, 2020 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:peregrine pkg:venia-ui version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants