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(esp-sync): schedule second sync upon subscription reactivation, just in case #3603

Merged
merged 9 commits into from
Dec 9, 2024

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Dec 3, 2024

All Submissions:

Changes proposed in this Pull Request:

We fire an ESP contact sync whenever a Woo subscription status is updated. When a subscription undergoes automatic renewal, it rapidly changes status to on-hold when payment processing begins, and then to active once successful payment is confirmed. However, since this can happen so quickly it's possible that sync after the active update can happen before the on-hold sync completes (a sort of race condition).

Rather than trying to untangle the Woo Subscriptions logic here, the easiest thing to do is to schedule a second "backup" sync a few minutes later when a subscription status changes to active. This should ensure that the ESP contact is updated to active after a successful renewal even if the race condition occurs.

How to test the changes in this Pull Request:

  1. Check out this branch.
  2. Manually renew a subscription in WP admin.
  3. Confirm that the ESP contact is synced when the subscription is changed to on-hold status and again when the status changes to active (but note that it might not always happen in this order).
  4. After the active sync completes, run wp cron event list and confirm that a non-repeating newspack_scheduled_esp_sync job is scheduled roughly two minutes in the future.
  5. Wait ~2 minutes or run wp cron event run newspack_scheduled_esp_sync and confirm that the contact is synced to the ESP again with the correct "active" status.
  6. (BONUS) Renew several subscriptions in rapid succession (within a single 2-minute window) and confirm that all subscriptions get a second sync ~2 minutes later, with the correct contact data.

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 Dec 3, 2024
@dkoo dkoo self-assigned this Dec 3, 2024
@dkoo dkoo requested a review from a team as a code owner December 3, 2024 23:48
@dkoo dkoo requested a review from leogermani December 4, 2024 17:38
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful. It works.

I just have one comment/question/suggestion... Maybe we could use a different name for the Data Even, instead of subscription_renewed...

This name gives the impression that it will be triggered after a successful renewal, and might lead to confusion when people hook into it in the future. Maybe "renewal_attempt"?

@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 Dec 6, 2024
@dkoo
Copy link
Contributor Author

dkoo commented Dec 6, 2024

@leogermani good suggestion! 660fe4d updates the data event name (and the handlers/methods that use it) to clarify that distinction. (EDIT: and 0d6b92a removes some unintended changes that got swept up in this commit.)

Co-authored-by: leogermani <leogermani@automattic.com>
@dkoo dkoo merged commit 9334295 into release Dec 9, 2024
7 checks passed
@dkoo dkoo deleted the fix/schedule-backup-sync branch December 9, 2024 17:09
matticbot pushed a commit that referenced this pull request Dec 9, 2024
## [5.8.2](v5.8.1...v5.8.2) (2024-12-09)

### Bug Fixes

* **esp-sync:** schedule second sync upon subscription reactivation, just in case ([#3603](#3603)) ([9334295](9334295))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 5.8.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 [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