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

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

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

mattallan
Copy link
Contributor

@mattallan mattallan commented Nov 20, 2024

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

image

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
Copy link
Contributor Author

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' ] )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant