search

Automattic / woocommerce-subscriptions-core

Subscriptions core package for WooCommerce
Other
80 stars 29 forks source link

Update the cart hash when paying for renewal orders on checkout block #543

Closed james-allan closed 7 months ago

james-allan commented 7 months ago

Fixes https://github.com/woocommerce/woocommerce-subscriptions/issues/4592

Description

When paying for a renewal orders via the block checkout there's still a bug that can cause new orders to be created and those orders aren't associated with any subscription.

From testing I discovered that this occurs because the WC core function that determines if a new order needs to be created, it calls (is_valid_draft_order()), inside that function is a $order->has_status( 'checkout-draft' ) and then a cart hash check.

In https://github.com/Automattic/woocommerce-subscriptions-core/pull/498 we attempted to fix this by introducing a new function hooked into that checkout-draft check to make sure it would update the cart hash prior to the cart hash check, however that function had additional logic to target the very specific flow. That logic included a is_rest_api check and a check on whether a session variable matches the orders ID.

Both those checks failed, that caused the cart hash to not update and resulted in an new order being created.

This PR fixes that by making removing those stricter checks.

However, this effectively means that any has_status( 'checkout-draft' ) will attempt to check if the cart contains a renewal order that matches the order being checked.

I'm sure how we can make this logic tighter.

How to test this PR

I'm not entirely sure what causes the cart hash to be mismatched so I've included all the things I think may be relevant, some may not be.

  1. Enable shipping and taxes.
  2. Create a block checkout page.
  3. Add shipping methods for different zones. eg free shipping for x country and flat rate for another country.
  4. Purchase any subscription product.
  5. Go to the edit subscription screen and create a pending renewal order.
  6. Attempt to pay for the renewal order via the block checkout.
    1. On trunk you'll notice that a new order is created and its unlinked to a subscription.
    2. On this branch the paid order ID should match the one you created in step 5 and it should be linked to a renewal order.

Product impact

james-allan commented 7 months ago

I noticed that we have a helper function wcs_is_checkout_blocks_api_request() (link) that also seems to be impacted by something that has changed in core or something to do with this specific flow that is bypassing that logic.

I've asked the checkout blocks team if there's a standard way we should be recognising these store api requests. p1700024991302199-slack-C8X6Q7XQU

james-allan commented 7 months ago

I've been trying to reproduce the original issue on trunk using the steps listed here https://github.com/Automattic/woocommerce-subscriptions-core/pull/498 but I'm not able to get new orders that aren't linked to the subscription.

I could still replicate locally on trunk so I spent some time trying to understand it. Strap yourself in because it's a doozy 😵

It seems as though you need to line up some really specific circumstances. There's possibly other ways to replicate, however this is what I did:

  1. Enable taxes.
  2. Prices are inclusive of tax.
  3. Tax rate 10%.
  4. Product $5/period.
  5. Purchase that $5 subscription.
  6. Create a pending renewal order for that subscription.
  7. Place that subscription renewal in your cart by clicking the pay link.
  8. Go to the cart page.
  9. Remove the renewal order item.
  10. Add a different new item to your cart.
  11. Complete the checkout.
  12. Reattempt the renewal order payment.
  13. At that point it should create a new order.

The first time you put the renewal order into your cart it updates the order item's tax data with a different rounding precision 0.454546 vs 0.454545. The next time you put the order into your cart, that rounding difference leads to a different cart hash, and therefore a new order.

Kinda dumb luck I managed to find this originally without realising this was the hoop you have to jump through.