Sitelet https://web.archive.org/web/20201022011818/https://github.com/woocommerce/woocommerce/issues/27989
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

Unintended product change on order_item_product with non-unique SKU's #27989

Open
NielsdeBlaauw opened this issue Apr 28, 2020 · 1 comment
Open

Comments

@NielsdeBlaauw
Copy link

@NielsdeBlaauw NielsdeBlaauw commented Apr 28, 2020

If multiple products can have the same SKU, updating an order through the WooCommerce API (tested v2, v3 and v4) can unintentionally change products within an order.

To reproduce:

  1. Add filter to functions.php add_filter( 'wc_product_has_unique_sku', '__return_false' );
  2. Create product 1 with SKU non-unique. (product_id example: 1111)
  3. Create product 2 with the same SKU non-unique. (product_id example: 2222)
  4. Create an order with product 2 through the checkout
  5. GET the order from the WooCommerce API, (observe that product_id = 2222)
  6. PUT the exact same payload.
  7. GET the order from the WooCommerce API, (observe that product_id = 1111)

Cause

In wc/v2/orders and wc/v3/orders this is caused by https://github.com/woocommerce/woocommerce-rest-api/blob/58bab7a5563ddb0f4eede348ed3dbd1d4bca86d2/src/Controllers/Version2/class-wc-rest-orders-v2-controller.php#L597-L609

In wc/v4/orders it's https://github.com/woocommerce/woocommerce-rest-api/blob/a518b9cfd1dec42a59aa10240a31782bc1d43d13/src/Controllers/Version4/Requests/OrderRequest.php#L177-L178

If the order already exists, and a product is set, it should always have an SKU, so it wil (almost) always use the SKU method for finding the product_id. When multiple products can have the same SKU, wc_get_product_id_by_sku() always uses the first one found.

Possible solutions:

Use product_id first, SKU second.

This seems to be more intuitive (and might be faster), however it would probably mean breaking backward compatibility for API clients that only change the SKU, and not the product id when editing an order.

Add a check for wc_product_has_unique_sku

If apply_filters( 'wc_product_has_unique_sku', .... ); return false, it doesn't make sense to use the SKU to change the product in the API, and wc_get_product_id_by_sku() should not be used. However, not all data that is passed in the wc_product_has_unique_sku filter is available at this point (possibly no known product ID).

@vedanshujain
Copy link
Contributor

@vedanshujain vedanshujain commented Oct 16, 2020

I think we can do both of these together. It does make sense to disregard SKU if it's not guranteed to be unique, and also to prioritize ID if that is the case. So flow could be:
Check if SKU exists && Check if SKU is unique ? Use SKU : Check if ID exist ? Use ID : Throw Error("Non unique SKU")

@vedanshujain vedanshujain transferred this issue from woocommerce/woocommerce-rest-api Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.