Automattic / woocommerce-subscriptions-core

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

Fix subscription needs payment to exclude early renewal orders #512

Closed james-allan closed 9 months ago

james-allan commented 9 months ago

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

Description

The $subscription->needs_payment() function use to check the last renewal or switch even if the last renewal order was an early renewal order.

Early renewal orders are optional and so should not have any bearing on whether a subscription needs payment.

If you look at the uses of needs_payment(), it's currently only used in a handful of places.

In my opinion none of these should use early renewal orders to determine if the subscription requires payment.

How to test this PR

  1. Purchase a subscription.
  2. Enable early renewal.
  3. On the subscription's my account page, click the renew early button.
  4. On checkout use a failing payment method. eg 4000000000000002
  5. Don't complete the payment.
  6. Go to the subscription my account page:
    • On trunk you cannot reactivate.
    • On this branch you can.

[!important] The subscription is on-hold but this won't be the case with the changes being introduced in https://github.com/woocommerce/woocommerce-subscriptions/pull/4545

Product impact

james-allan commented 9 months ago

@mattallan while testing this PR was observing a strange bug. If there was a failed early renewal order in the cart, doing other things like creating standard renewal orders would lead to it being created with a failed order status by default. So to replicate that bug you would:

  1. Follow the steps from the original PR description:
    1. Purchase a subscription.
    2. Enable early renewal.
    3. On the subscription's my account page, click the renew early button.
    4. On checkout use a failing payment method. eg 4000000000000002
    5. Don't complete the payment.
  2. While the cart still contains the failed early renewal order attempt. Go to the admin screen.
  3. Process a standard renewal order.
  4. Look at the order transition order note. It should suggest failed -> processing/completed.

This may be an intermittent bug. I wasn't able to replicate this even though I could observe the maybe_preserve_order_status() function returning a failed status while a failed early renewal was in the cart. 🤷‍♂️

In any case, this function is no longer needed and is very error prone given it affects all default order statuses while the cart contains a failed order.