Performance improvements: rendering the Subscription Relationship column and checking if order is a renewal, switch etc.

[mattallan]

Part of #731

Description

When a merchant visits the WooCommerce > Orders list table, WooCommerce Subscriptions adds a custom "Subscription Relationship" column that displays an icon based on what type of order it is:

1. Parent (initial subscription) Order
2. Renewal Order
3. Resubscribe Order

As reported in #731, our render_contains_subscription_column_content() function currently performs unnecessary queries which impacts overall site performance and performance when viewing/managing orders via the WooCommerce > Orders table. After looking at this function, I noticed a couple of issues that we should address:

1. wcs_order_contains_subscription() unnecessarily loads subscription objects into memory.
2. We already have the Order object when rendering the row/column so we should use it instead of passing the order ID around.
3. There's no efficient helper function to check if an order is a parent order.
4. All of our wcs_order_contains_{order_type}() functions are doing a lot when all we really need to do is check for meta on an order.

This PR targets all of these issues:

• dd1299a - Introduces a new wcs_is_parent_order( $order ) function which runs a much more performant query to check if an order is a parent order.
• 44321ec - Refactors all of our wcs_order_contains_renewal(), wcs_order_contains_resubscribe() and wcs_order_contains_switch() functions to use our WCS_Related_Order_Store class to check if the order belongs to a renewal, resubscribe and switch, without having to load subscription objects into memory.
• 976c961 - Refactors the render_contains_subscription_column_content() to call the updated/more performant wcs_order_contains_{order_type}( $order ) functions directly instead of going through wcs_order_contains_subscription( $order_id, {order_type} )
• fc91135 - Updates wcs_order_contains_subscription() to use more performant method for checking parent order.

How to test this PR

Important

The changes in this PR impact potentially a lot of critical subscription flows. We should make sure these functions are heavily unit tested and test our critical flows to assess impact.

Product impact

• Added changelog entry (or does not apply)
• Will this PR affect WooCommerce Subscriptions? yes/no/tbc, add issue ref
• Will this PR affect WooCommerce Payments? yes/no/tbc, add issue ref
• Added deprecated functions, hooks or classes to the spreadsheet

[mattallan]

Breakdown of these changes

Checking if an order is renewal/resubscribe

Old

1. Get order object: wc_get_order( $order_id )
2. Get subscription id from meta: $order->get_meta( '_subscription_renewal', false );
3. Load in subscription object: wcs_get_subscription( $subscription_id );
4. Check if $subscription is not empty.

This branch

1. Get subscription id from meta: $order->get_meta( '_subscription_renewal', false );
2. Check if $subscription_id is exists.

Important

As seen, these changes will no longer load a subscription object into memory which means we're no longer checking if the subscription ID stored in _subscription_renewal is actually a valid subscription object. This means with the changes in this PR, wcs_order_contains_renewal() and other similar "contains_x()" functions, will now return true even if the subscription no longer exists.

To determine if this is a bug or not, we should ask ourselves whether an order should revert to a standalone/regular order if the subscription has been deleted 🤔

I can't think of good reason why we should revert an order to a regular order if the linked subscription has been removed and I believe it would still be good to know that this order was reference to a subscription renewal or resubscribe etc.

If we consider this to be a bug, one way we could address this is to hook onto woocommerce_subscription_deleted and delete any relation meta for this subscription. This would prevent us having to load a subscription into memory just to check if the relation is still valid.

Checking if an order is a parent

Old

1. Get order object: wc_get_order( $order_id )
2. Query for subscriptions: wcs_get_orders( [ 'limit' => -1, 'parent' => 1234, 'type' => 'shop_subscription' ] )
3. Load in subscription object: wcs_get_subscription( $subscription_id );
4. Count subscriptions found.

This branch

1. Query for subscriptions: wcs_get_orders( [ 'limit' => 1, 'parent' => 1234, 'type' => 'shop_subscription' ] )

[james-allan]

These changes look good to me. On a change by change comparison the changes in this PR only improve the performance of the wcs_order_contains_x functions.

Diving into the root difference, it's essentially bypassing mid level functions to avoid a feature of those mid level functions which is to instantiate the subscription objects returned.

Your concerns around checking if those subscriptions still exist is fair, however, I don't think we need to be concerned with that too much.

Deleting subscriptions isn't something we routinely do or even expect. Even if it was a thing, the relationship stored in order meta would be incorrect and the UI's displaying of that relationship on the front end would be a reflection of incorrect data. That is, I agree with your earlier comment that the "fix" would be to make sure that meta was deleted on subscription deletion.

I left a couple of comments and questions. Let me know what you think of them.

[james-allan]

🗒️ Comment
I've confirmed that this change is only a more direct call.

The old flow was:

• wcs_get_subscriptions_for_renewal_order()
• ↳ Wrapper of wcs_get_subscriptions_for_order( $order, array( 'order_type' => 'renewal' ) )
• ↳ WCS_Related_Order_Store::instance()->get_related_subscription_ids( $order, 'renewal' ).

When called in this way, it leads to this exact same:
WCS_Related_Order_Store::instance()->get_related_subscription_ids( $order, 'renewal' );

This change only bypasses creating subscription objects and the logic at the start of wcs_get_subscriptions_for_order().

Ideally, interactions with the implementation layer (ie the data store) would be limited to lower level functions with higher functions being agnostic to how the order & subscription relationship is stored.

To put that into effect, we'd essentially need to refactor wcs_get_subscriptions_for_order() and introduce a wcs_get_subscription_ids_for_order()

This is essentially how we do things on the WC_Subscription get related order side.

ie we have a base ID based function (get_related_order_ids()) and a get_related_orders() which uses the base IDs function to return the objects.

I'm not sure what your thoughts are on that. If you think there will be more cases where all we want to do is get the subscription IDs, rather than objects, then it might be worth considering given performance is a focus at the moment.

[james-allan]

❓ Question
I wonder if this parent order logic should be moved to the bottom of this if/else block.

The "is parent" check involves a database query to find if there exists a subscription with this order as it's parent whereas the other related order checks just check if the order which is already in memory contains metadata.

Therefore it would be an performance improvement when checking if an order contains any relationship. eg renewal and parent