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

fix: find catalog product regardless of visibility #6089

Merged
merged 16 commits into from
Apr 13, 2020

Conversation

kieckhafer
Copy link
Member

@kieckhafer kieckhafer commented Feb 6, 2020

The original PR, #6085, was accidentally closed when we deleted the old release-3.0.0 branch. This is a copy of the original PR to the trunk branch.

Resolves #6077
Impact: minor
Type: bugfix

Issue

Cart and order transformations fail when the catalog item related to a cart or order item has since been deleted or hidden.

Solution

Remove deleted/hidden checks from the queries. Look up by product ID only. That way it will only fail if the document has been hard deleted, which can only be done outside of the official GraphQL API and should never be done outside of a development environment as a rule.

Centralize media lookup in the files plugin rather than catalog, and use the Media collection directly for it rather than copying media from CatalogProduct.

Breaking changes

None

Testing

See #6077 for reproduction steps and verify fixed.

@aldeed
Copy link
Contributor

aldeed commented Mar 2, 2020

For my benefit, copying in @kieckhafer 's review comment from the closed PR:

After further testing it seems that if the product is made visible / invisible, then this PR fixes the issue, and I can still see the item in the cart and I can see an order correctly, even after it's invisible.

However, if the variant or option that was ordered is made invisible, I still see the same errors.

@aldeed aldeed self-assigned this Mar 2, 2020
Move media code into files plugin, and do
not use the catalog as the source of media

Signed-off-by: Eric Dobbertin <[email protected]>
@aldeed aldeed self-requested a review March 5, 2020 12:41
aldeed
aldeed previously approved these changes Mar 5, 2020
@kieckhafer
Copy link
Member Author

kieckhafer commented Mar 5, 2020

FIXED: 7e5b787


When I try to publish a Product with variants, but no options on those variants, I see this error:

"TypeError: catalogProductVariant.options is not iterable",
            "    at publishProductToCatalogMedia (file:///usr/local/src/app/src/core-services/files/util/publishProductToCatalog.js:24:62)",

I can't add an official review since I made this PR (it was accidentally closed when we closed the 3.0 branch, I re-created it, but it is not my PR).

@kieckhafer
Copy link
Member Author

Once I am in checkout, I cannot get past the Add Address step, this error occurs when trying to save my address:

ReactionError: Catalog variant not found
      at file:///usr/local/src/app/src/core-services/cart/util/xformCartGroupToCommonOrder.js:35:17
      at Array.map (<anonymous>)
      at xformCartGroupToCommonOrder (file:///usr/local/src/app/src/core-services/cart/util/xformCartGroupToCommonOrder.js:26:35)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (internal/process/task_queues.js:94:5)
      at async Promise.all (index 0)
      at async getCommonOrders (file:///usr/local/src/app/src/core-services/cart/mutations/transformAndValidateCart.js:31:22)
      at async Object.setSurchargesOnCart [as fn] (file:///usr/local/src/app/src/plugins/surcharges/util/setSurchargesOnCart.js:27:24)
      at async file:///usr/local/src/app/src/core-services/cart/mutations/transformAndValidateCart.js:44:5
      at async Object.transformAndValidateCart (file:///usr/local/src/app/src/core-services/cart/mutations/transformAndValidateCart.js:41:3)

I can't add an official review since I made this PR (it was accidentally closed when we closed the 3.0 branch, I re-created it, but it is not my PR).

@aldeed
Copy link
Contributor

aldeed commented Mar 10, 2020

Status: The last error reported by @kieckhafer can't be fixed until we discuss and make some decisions in #6145

@aldeed aldeed assigned kieckhafer and unassigned aldeed Mar 31, 2020
@aldeed
Copy link
Contributor

aldeed commented Mar 31, 2020

@kieckhafer This is ready for another look. I implemented the suggestion from #6145. It feels a bit error-prone, but it seems to work okay in my manual testing.

  • Whenever a cart gets updated (adding or removing items, changing quantity, completing a checkout step), we verify that all the item products and variants are still published, visible, and not deleted.
  • Any items that are no longer valid are moved silently to cart.missingItems array.
  • missingItems have the same CartItem schema and can be requested in the GraphQL response with the same fields as items

In example storefront, this manifests in a few ways:

  • If you delete while navigating the site, there shouldn't be any immediate errors and they'll disappear from the cart as soon as you add another item, adjust quantities, etc.
  • If you delete while checking out, there shouldn't be any immediate errors and you'll see the preview and totals adjust after you complete the next checkout step.
  • If you delete while on the final review step of checkout, you'll see an error when you click Place Order, which is good (though we could make this more user friendly in the future).

It would be good to eventually request missingItems in example storefront so that the UI can explain more about why things are suddenly disappearing and why order placing is failing, but that's a separate project.

@kieckhafer
Copy link
Member Author

kieckhafer commented Apr 2, 2020

@aldeed I'm still seeing issues when items are deleted.

  1. When I add something to the cart, then make an item hidden, if i refresh the page of the storefront before moving to checkout, my cart goes missing, and I cannot create a new cart, I'm in a limbo where I have no cart and can't make one.
  2. If i'm in checkout and make my FINAL item hidden, then i get an error when trying to move through checkout, but just a server error, the UI just spins indefinitely:
Catalog variant not found (errorId=ck8j39t4w000h0pph4xua2jpf)
  path: [
    "selectFulfillmentOptionForGroup"
  ]
  --
  stack: ReactionError: Catalog variant not found
      at file:///usr/local/src/app/src/core-services/cart/util/xformCartGroupToCommonOrder.js:35:17

@aldeed
Copy link
Contributor

aldeed commented Apr 2, 2020

@kieckhafer Is this anonymous or are you logged in? Number 1 sounds like probably an issue with the Apollo caching in the storefront, and not necessarily something that can be fixed on the API side. Number 2 could be another API issue, but so far I can't reproduce. If you have more explicit reproduction steps that could help.

@kieckhafer
Copy link
Member Author

@aldeed tried both anonymous and logged in.

  1. definitely might be caching

  2. add a product to cart, start checkout. Add my address, then go to the shipping step. At this point, make the variant /option “hidden”, and then try to save the shipping option. This is where I get the error, and the shipping save button just spins forever.

@aldeed
Copy link
Contributor

aldeed commented Apr 7, 2020

Update: still unable to reproduce errors seen by @kieckhafer

@kieckhafer
Copy link
Member Author

@aldeed

If I make an item hidden pre-checkout, I see it removed from the cart and added to missingItems as expected.

Once I get into checkout though, there is still some odd behavior and I can't complete checkout. Perhaps this is expected, but I think based on your description I should be able to complete the order.

  • I have a cart with 3 variants in it, and go to checkout.
  • At any point in checkout (pre the last step), if I make one of the items hidden, I see it removed from the Total, however I still see the item in the checkout area. See this image, I removed the Large variant, and you can see the price reflects this, but all three are still visible:
    image

Then, once I get to the payment or final step, I get an error saying I can't checkout. This is mentioned in your description, so perhaps this is expected, but I would assume if I had the item removed before this step, I should still be able to checkout.

image

If this is expected, then I think the PR is good to go, but it seems like something is still off here.

@aldeed
Copy link
Contributor

aldeed commented Apr 10, 2020

@kieckhafer That is probably due to the fact that many of the checkout steps do not refetch the cart from the server. It's essentially the same issue as this: reactioncommerce/example-storefront#469

And this issue is also related: reactioncommerce/example-storefront#193

Maybe I will see how easy it is to fix those issues in the storefront. They're separate, so we could merge this API change in the meantime, or we could wait until we figure out the storefront changes.

@kieckhafer
Copy link
Member Author

@aldeed in that case this gets a 👍 from me. I don't have the option to merge the PR but go ahead and merge.

@aldeed aldeed merged commit d0c37e0 into trunk Apr 13, 2020
@aldeed aldeed deleted the fix-aldeed-6077-cart-order-catalog-lookup branch April 13, 2020 15:28
@kieckhafer kieckhafer mentioned this pull request Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xFormCartItems and xFormOrderItems should not rely on Catalog
2 participants