Reaction: xFormCartItems and xFormOrderItems should not rely on Catalog

[janus-reith]

Issue Description

xFormCartItems is currently used to query a cart in the storefront. If that catalog Item can't be found as it has isVisible: false, isDeleted: trueor is actually just not present in the db, the query will throw an error. A user would by now have no way of removing that defunct cart Item.

The xFormOrderItems is used in the admin to display orders, and behaves similar. If the catalog item is not present, the query wil fail and the admin crashes.

IMHO both functions should not have to rely on the catalog item at all, as all the relevant data is present in the respective order or cart item.

Steps to Reproduce

Cart Item:

1. Put an item in the cart
2. Set it to invisible in the admin
3. Reload the strorefront, the cart badge will display items but the cart is shown as empty.

Order Item:

1. Complete an order
2. Set the item used for the order to invisible in the admin
3. Go to the orders admin page, click that order, page will crash.

Possible Solution

The catalog Item is only used to get the correct media for the item and should not be relevant for the basic functionality of these queries. All potential updates to that item or additional data from outside of that object should not be a requirement.

Options could be:

• For now, simply omit the media if not present and return the item data without it.
• Not preferred but simple to plug in without messing up existing data: Directly search the Media collection for matching entries like it is done for the product admin.
• Put the media, or at lest the primaryImage directly on the cart/order item.

Versions

release-3.0.0

[spencern]

Thanks for the report @janus-reith

[aldeed]

There are a couple things here:

• We need to come up with a general plan for how to handle visibility and deletion after cart/order. This is also tracked in https://github.com/reactioncommerce/reaction/issues/5237 and won't be solved for the 3.0 release, but hopefully sometime soon.
• If we change this to match how we do most other fields, we should probably copy all media URLs to the cart/order items at the time they are placed. But we'll need to have better support for updating images in these places, etc., which I believe is why we didn't do that originally.
• Since both of the above are more complex, I think a quick fix here would be to remove all the isVisible/isDeleted selectors from the mongo query (look up by ID only), plus adjust the code a bit to hopefully still work if for some reason the item was hard deleted. Does that sound good @janus-reith ?

[aldeed]

6085 is a PR for my last bullet point above

[kieckhafer]

From @Niko0918 in #5237 , which I'm closing in favor of this ticket, as it calls out the same issue:

1. You add "Sample Product" to the shopping cart.
2. You delete this "Sample Product" with admin panel.
3. You Refresh shopping cart and get this err.

  7 |  * @param {Object} cartItem Object of the `CartItem` type
   8 |  * @returns {Object} Data suitable for tracking a `CartItem`
   9 |  */
> 10 | export default function getCartItemTrackingData(cartItem) {
  11 |   const variant = { ...cartItem };
  12 |   let url;

( Because when you delete some product with admin panel then this deleted product will not be removed automatically from shopping cart too. So you will get this err because this product doesn't exist. )

@aldeed's response in that ticket:

There might be a few things here.

• a variant maybe should not be able to be hard deleted if it's in any carts or orders
• a variant that is soft deleted should be included in the cart items query even though it is soft deleted because it's still in the cart? Or we should have a setting/prompt for whether to remove from all carts.

[janus-reith]

@aldeed Sounds reasonable. For now I also just solved it with a fallback if no catalog item is present. But I'm not sure how I feel about skipping the isDeleted/isVisible check and if it really is necessary. As the only implication here is if a cart item image is displayed, I wonder: If I would add an image to a product in the admin, then set it to hidden or deleted, would I expect an user to still see it anywhere? For sure these considerations would also apply both when we copy over the media to cart, as also to the remaining cart item in general, but for now Im not sure if the added value of having Images for not available items would justify leaving the common Pattern (isDeleted = if true, consider it deleted, isVisible = if false, none of its info should be available without special permission).

We might need to reconsider if the catalog item should really stay the source of truth, or if there might be an efficient way to rework the pattern used for products somehow to have something separated which cart, orders and catalog could all reference, while still keeping it efficient to lookup and update.

[kieckhafer]

@janus-reith can you share what you've done for this fallback? Perhaps that's a better solution for now until we can have a bigger discussion on all of this.

[janus-reith]

@kieckhafer I left the Catalog.find query as is and only changed the xformOrderItem/xformCartItem part for each function
https://github.com/reactioncommerce/reaction/blob/d43ec57ee8c2405e2e2383b089695dd68449dff6/src/core-services/cart/xforms/xformCartItems.js#L35
https://github.com/reactioncommerce/reaction/blob/d43ec57ee8c2405e2e2383b089695dd68449dff6/src/core-services/orders/xforms/xformOrderItems.js#L36

Now it doesn't throw an error in there, but simply skips further checks for media being present on it and simply returns the reshaped object at the end of the function like it also would with an item present, just without the images.

[aldeed]

Thanks @janus-reith. That does solve the problem but at the cost of losing the media. I'm trying out a couple other ideas:

• look up the missing hidden/archived products and variants and extend them onto catalogItems, just for purposes of this transform
• and/or refactor everything into the xformCatalogProductMedia plugin function

I'd really like to do the second one either way because it will avoid some unnecessary db lookups for people who have replaced that plugin with a different image service.

Whatever we figure out, this fix will be in a patch release soon after 3.0. It is important, but not worth holding back the release.