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: ensure migrated Stripe subs have next_payment scheduled #2434

Merged
merged 2 commits into from
May 10, 2023

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented May 2, 2023

All Submissions:

Changes proposed in this Pull Request:

Plugs a hole in the migration script for migrating Stripe subscriptions to WooCommerce. When programmatically creating subscriptions via code, Woo does not automatically schedule next payment dates—this must be done explicitly. This PR ensures this is done, and also implements a script to retroactively look up any previously migrated subscriptions and schedule next payment dates for any that are missing them.

Note that the scheduled next payment date won't always be in line with the start date of the subscription (e.g. if started April 3, the next payment would normally be May 3), but it should fall within 2 weeks of that date. This is because the calculate_date method is based on the current timetamp rather than the subscription's start date. We were able to get around this in d4978df by explicitly setting the scheduled date to the next payment date from Stripe.

Also adds a --force flag to the sync-stripe-subscriptions-to-wc script which will allow us to migrate Stripe subscriptions that were created via a migration from Pico.

How to test the changes in this Pull Request:

  1. While on master or release, set the Reader Revenue platform for your test site to "Stripe".
  2. Make a monthly or annual donation via the SDB and confirm that the subscription gets created in your Stripe account.
  3. Run wp newspack stripe sync-stripe-subscriptions-to-wc via CLI to migrate the subscription to WooCommerce.
  4. In the WooCommerce > Subscriptions dashboard, observe that the subscription was migrated, but that it lacks a scheduled "next payment" date.
  5. Check out this branch. Run wp newspack stripe set-next-payment-dates-for-migrated-subscriptions.
  6. Confirm that the script outputs a message stating that your previous subscription was updated with a next payment date, e.g.

No next payment date set. Setting to YYYY-MM-DD.

  1. Make another new monthly or annual donation (still while on Stripe).
  2. Run wp newspack stripe sync-stripe-subscriptions-to-wc again and this time confirm that the migrated subscription is created with a scheduled next payment date.

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?

@dkoo dkoo added the [Status] Needs Review The issue or pull request needs to be reviewed label May 2, 2023
@dkoo dkoo self-assigned this May 2, 2023
@dkoo dkoo requested a review from a team as a code owner May 2, 2023 20:27
@leogermani
Copy link
Contributor

In the WooCommerce > Subscriptions dashboard, observe that the subscription was migrated, but that it lacks a scheduled "next payment" date.

I could not confirm this. Running on master the subscription has a Next payment date after the migration

@dkoo
Copy link
Contributor Author

dkoo commented May 10, 2023

I could not confirm this. Running on master the subscription has a Next payment date after the migration

Thanks for testing—that's interesting. In all cases when we ran the script in production without this change, every migrated subscription came in without a next payment date.

Can you confirm that migrated subs still come in with a next payment date with this change? If so, I think we can safely merge.

@leogermani
Copy link
Contributor

Yes, everything else worked fine

@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels May 10, 2023
@dkoo dkoo merged commit b3b32f0 into release May 10, 2023
@dkoo dkoo deleted the hotfix/stripe-migrated-subscriptions branch May 10, 2023 19:38
matticbot pushed a commit that referenced this pull request May 10, 2023
## [1.114.1](v1.114.0...v1.114.1) (2023-05-10)

### Bug Fixes

* ensure migrated Stripe subs have next_payment scheduled ([#2434](#2434)) ([b3b32f0](b3b32f0))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.114.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request May 10, 2023
# [1.115.0-alpha.2](v1.115.0-alpha.1...v1.115.0-alpha.2) (2023-05-10)

### Bug Fixes

* ensure migrated Stripe subs have next_payment scheduled ([#2434](#2434)) ([b3b32f0](b3b32f0))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.115.0-alpha.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @alpha released [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants