Removing wc-checkout-draft status from the get related order IDs method

wjrosa commented 1 month ago

Fixes 4181-gh-woocommerce/woocommerce-subscriptions

Description

If a user upgrades their Subscription multiple times in a single month, the prices appear incorrectly calculated. This is happening since the introduction of the new order status: wc-checkout-draft (it was introduced with the blocks theme, more information here).

The reason was that when calculating the switch fee, we were retrieving the last switch order, and that was including the current draft order (from the current checkout).

The fix filters the new status from the main method (get_related_order_ids).

How to test this PR

Create a variable subscription product with three variations. All should have the same matching billing period.
Subscribe at the least expensive variation
Upgrade to the second-most expensive variation (this prorated sign-up fee is calculated correctly)
Upgrade to the most expensive variation
On trunk, this signup fee will be calculated incorrectly
On this branch the correct value should be displayed

Product impact

[x] Added changelog entry (or does not apply)
[x] Will this PR affect WooCommerce Subscriptions? yes
[x] Will this PR affect WooCommerce Payments? yes

mattallan commented 1 month ago

This is happening since the introduction of the new order status: wc-checkout-draft (it was introduced with the blocks theme, more information here).

I haven't looked at the code or tested this yet but I had a general question. Does this mean the bug is only reproducible with the block Checkout and not when using the shortcode checkout?

wjrosa commented 1 month ago

This is happening since the introduction of the new order status: wc-checkout-draft (it was introduced with the blocks theme, more information here).

I haven't looked at the code or tested this yet but I had a general question. Does this mean the bug is only reproducible with the block Checkout and not when using the shortcode checkout?

Hi Matt! For this part of the fix, yes. Still it would not be enough to fix the main issue (without the other PR).

wjrosa commented 2 weeks ago

@wjrosa I think this approach has some issues with it. I left a comment inline about why I think excluding the wc-checkout-draft status from this function may be problematic.

I'm curious though. In the PR description you mention this:

The reason was that when calculating the switch fee, we were retrieving the last switch order, and that was including the current draft order (from the current checkout).

I wonder if we just need to exclude draft switch orders at that point, not from the cache.

Oh Ok, James! I understand your concern. I decided to include a new param $include_draft in these functions to keep them working just like before. On the subscriptions repo, I will open another PR changing only the get_last_switch_order method to pass false to this new param. What do you think? Draft PR is here

james-allan commented 2 weeks ago

@wjrosa I don't think will work as intended. If you run this code snippet for example, it will return the newly created renewal order even though it has the checkout draft status.

$subscription = wcs_get_subscription( 7634 );

$renewal_order = wcs_create_renewal_order( $subscription );
$renewal_order->set_status( 'wc-checkout-draft' );
$renewal_order->save();

echo $subscription->get_last_order( 'all', 'renewal', false );

This is because this PR introduces a new arg and then passes that arg all the way down to the cache level.

WC_Subscription::get_last_order()
   ↳ WC_Subscription::get_related_order_ids()
      ↳ WCS_Related_Order_Store::get_related_order_ids()
         ↳ parent::get_related_order_ids()

The problem is that WCS_Related_Order_Store::get_related_order_ids() returns the values in the cache if they exists (ie this whole section of the function, including your change, is bypassed). The parent::get_related_order_ids() function is only called to populate the cache.

So, because our cache is already populated, parent::get_related_order_ids() is not called and the status filter isn't applied.

I'd recommend that we don't pass this arg that far down. IMO WCS_Related_Order_Store should be left unchanged. It is responsible for maintaining the cache of raw related order IDs - unfettered and unfiltered. Code that sits on top of the cache and utilises those values can do whatever they want to filter the result as they see fit though. So, you could pass it down to the WC_Subscription::get_related_order_ids() function, however that function only deals with IDs and I don't think we want to change that by fetching subscription objects for performance reasons - that's also more the responsibility of get_related_orders().

With all of that in mind, I don't think we should really pass this parameter down past get_last_order(). I think get_last_order() could just loop over the results after the array is sorted and then find the first order that matches the criteria.

Also, what do you think about broadening this param? Something like $exclude_statuses = [] will help make it a bit more usable for the longer term if we ever needed to find the last paid order we could do $subscription->get_last_order( 'id', 'renewal', [ 'pending', 'failed' ] )

Let me know if that makes sense. 😅 I'm happy to work on an example.

wjrosa commented 2 weeks ago

@wjrosa I don't think will work as intended. If you run this code snippet for example, it will return the newly created renewal order even though it has the checkout draft status.
$subscription = wcs_get_subscription( 7634 );

$renewal_order = wcs_create_renewal_order( $subscription );
$renewal_order->set_status( 'wc-checkout-draft' );
$renewal_order->save();

echo $subscription->get_last_order( 'all', 'renewal', false );
This is because this PR introduces a new arg and then passes that arg all the way down to the cache level.
WC_Subscription::get_last_order()
   ↳ WC_Subscription::get_related_order_ids()
      ↳ WCS_Related_Order_Store::get_related_order_ids()
         ↳ parent::get_related_order_ids()
The problem is that WCS_Related_Order_Store::get_related_order_ids() returns the values in the cache if they exists (ie this whole section of the function, including your change, is bypassed). The parent::get_related_order_ids() function is only called to populate the cache.

So, because our cache is already populated, parent::get_related_order_ids() is not called and the status filter isn't applied.

I'd recommend that we don't pass this arg that far down. IMO WCS_Related_Order_Store should be left unchanged. It is responsible for maintaining the cache of raw related order IDs - unfettered and unfiltered. Code that sits on top of the cache and utilises those values can do whatever they want to filter the result as they see fit though. So, you could pass it down to the WC_Subscription::get_related_order_ids() function, however that function only deals with IDs and I don't think we want to change that by fetching subscription objects for performance reasons - that's also more the responsibility of get_related_orders().

With all of that in mind, I don't think we should really pass this parameter down past get_last_order(). I think get_last_order() could just loop over the results after the array is sorted and then find the first order that matches the criteria.

Also, what do you think about broadening this param? Something like $exclude_statuses = [] will help make it a bit more usable for the longer term if we ever needed to find the last paid order we could do $subscription->get_last_order( 'id', 'renewal', [ 'pending', 'failed' ] )

Let me know if that makes sense. 😅 I'm happy to work on an example.

Thanks for the detailed argument, James! It makes total sense to me. I have updated the main implementation following your suggestion. I will update the woocommerce-subscriptions PR now to make use of it.

Automattic / woocommerce-subscriptions-core