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

fix(wcs): account for more on-hold subscription cases #3710

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

Conversation

chickenn00dle
Copy link
Contributor

@chickenn00dle chickenn00dle commented Jan 31, 2025

All Submissions:

Changes proposed in this Pull Request:

See https://newspackengineering.wordpress.com/2025/01/10/accounting-for-all-on-hold-subscriptions/#comment-1691

This PR ensures our on-hold duration logic accounts for all of the following cases:

Transition to expired

  • Manual or no-retry subscriptions
  • Manual holds for customer support reasons

Transition to trash

  • Failed parent orders

(The other cases in the p2 comment should already be accounted for.)

How to test the changes in this Pull Request:

Setup

You'll need to set up some subscriptions on a test site before beginning testing. Here are some useful tips for this part:

  • To setup no-retry subscriptions, add the following to wp-config then disable auto retries via WooCommerce > Settings > Subscription menu: define( 'NEWSPACK_PREVENT_WC_ALLOW_FAILED_PAYMENT_RETRIES_OVERRIDE', true );
  • To setup subscriptions that end up in the on-hold status after retries, add the following filter to the site: add_filter( 'newspack_subscriptions_expiration_enabled', '__return_false' );, then follow these steps. Also use this filter to setup on-hold subscriptions with failed parent orders.
  • To setup manual subscriptions that have passed the on-hold duration, use the following via wp shell to manually fix the start and next_payment dates:
$s = wcs_get_subscription(SUBSCRIPTION_ID);
$s->update_dates([ 'start' => 'YYYY-MM-DD HH:MM:SS', 'next_payment' => 'YYYY-MM-DD HH:MM:SS' ]);

Set up the following subscriptions (all within the on-hold duration except for the last on this list):

  • A non-manual subscriptions that has been put on hold via the admin menu
  • A non-manual subscription that has been put on hold as the result of a failed renewal (with the retry system disabled)
  • A non-manual subscription that has been put on hold after all retries have failed
  • A non-manual subscription that has been put on hold after a failed parent order (paid with Stripe test card 4000 0000 0000 0002 for example)
  • A manual subscription that has been put on hold via the admin menu
  • A manual subscription that has been put on hold as the result of needing a renewal payment
  • Some manual and non-manual subscriptions with next payment dates that are beyond the sites on-hold duration

Once you finish setting up, be sure to remove both the constant and filter before testing.

Testing

  1. Run the following cli command: wp newspack migrate-expired-subscriptions --verbose
  2. Verify the output of the command indicates:
  • non-manual subscriptions that have been put on hold by admin are scheduled to expire after the on-hold duration + next payment date
  • non-manual subscriptions with failed renewals when the retry system was disabled are scheduled to expire after the on-hold duration + next payment date
  • non-manual subscriptions that have went through all retries are scheduled a final retry
  • failed parent order subscriptions are trashed
  • manual subscriptions are scheduled to expire after the on-hold duration + next payment date + 7 day grace period
  • subscriptions that are beyond the on-hold duration are expired immediately
  1. As a reader, attempt to purchase a subscription with the following decline card, then abandon the cart: 4000 0000 0000 0002
  2. Via wp-admin ensure there is a trashed subscription as a result of the failed payment
  3. Set another active subscription to manual, then look for a newly scheduled newspack_subscriptions_expiration_enabled event via the action scheduler (WooCommerce > Status > Scheduled Actions > Pending)
  4. Finally, run the cli command above in live mode (with the --live flag) and verify all subscriptions behave as described above.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@chickenn00dle chickenn00dle marked this pull request as ready for review January 31, 2025 22:05
@chickenn00dle chickenn00dle requested a review from a team as a code owner January 31, 2025 22:05
@chickenn00dle chickenn00dle added the [Status] Needs Review The issue or pull request needs to be reviewed label Jan 31, 2025
@chickenn00dle
Copy link
Contributor Author

Looks like the failing phpunit tests are also failing on trunk and are unrelated to these changes, so I'm gonna leave them as is as not to make this PR even more complicated.

@chickenn00dle
Copy link
Contributor Author

tbh it was super tedious to make sure all cases were accounted for, and setting up subscriptions to test this is a bit annoying, so let me know if you want to hop on a call when reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs Review The issue or pull request needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant