search

Automattic / woocommerce-subscriptions-core

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

Exclude subscriptions from WooCommerce core's automatic cleanup of unpaid orders #595

Closed james-allan closed 2 months ago

james-allan commented 2 months ago

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

Description

When WooCommerce core automatically cancels unpaid orders that have "expired" relative to the hold stock setting, the query that fetches those unpaid orders also includes subscriptions.

This generally doesn't cause any issues because most of the time unpaid parent orders are cancelled by this process and cancelling an unpaid parent order would also cancel the subscription.

Where this causes issues is if the parent order is excluded from this process but the subscription isn't.

For example, on the original issue the parent order was successfully paid but a bug at the time caused the subscription to not be activated. Because the parent order was paid it wasn't collected by this flow but the subscription was. Or as @mattallan mentions in https://github.com/woocommerce/woocommerce-subscriptions/issues/3754#issuecomment-2033695884, gateways (eg Stripe) may exclude the order if payment is in progress and the store has a very short hold stock setting.

This PR fixes this by 1) excluding shop_subscription from the original query and 2) making sure a subscription will never be cancelled in this way by using the woocommerce_cancel_unpaid_order hook.

How to test this PR

  1. Go to WooCommerce → Settings → Products → Inventory
  2. Set your Hold Stock setting to 1 minute.

Screenshot 2024-04-04 at 3 46 10 pm

  1. Go to WooCommerce → Subscription → Add Subscription
  2. Create a subscription.
  3. Leave it in a pending state.
  4. Wait a minute and run the following code snippet.
    • On trunk the subscription would have been cancelled.'
    • On this branch the subscription is excluded from the query and never cancelled. You can also unhook the exclude_subscriptions_from_order_cleanup function from wc_order_types to confirm the fallback also works. 
do_action( 'woocommerce_cancel_unpaid_orders' );

Product impact

james-allan commented 2 months ago

I've decided to remove the code that attempted to remove the shop_subscription from the query. It was too risky imo to be hooked onto wc_order_types and remove the shop_subscription type while guaranteeing that it would never inadvertently remove it from other uses of that function.

If we want a more robust way to improve the this query, we'll need to change WC core to make it more robust.

FWIW, the way this process works, WC are already potentially creating a long list of orders that will clog up this query by the fact it only cancels orders created via the checkout. If there's buildup of orders created via the REST API for example, they will constantly be returned and excluded - subscriptions are just another type.

james-allan commented 2 months ago

Noting that I forgot to add a changelog to this PR and so I've pushed one here: https://github.com/Automattic/woocommerce-subscriptions-core/commit/12261aacadc6ac48202c1c76597d67445f22857d