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(ras): sync purchase data only for most recent order/subscription #3086

Merged
merged 4 commits into from
May 13, 2024

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Apr 24, 2024

All Submissions:

Changes proposed in this Pull Request:

Tweaks the reader data sync behavior so that any purchase-related fields only relate to the reader's most recent active order/subscription, if any. It's possible for a single reader to have more than one subscription in different states, so in this case an active one should take precedence when it comes to contact data in the connected ESP.

How to test the changes in this Pull Request:

  1. Publish three different non-donation subscription-type products with different attributes (name, price, billing period, etc.) so they're easily distinguished in the connected ESP data.
  2. As a reader, purchase subscriptions for two of the products.
  3. As an admin, force immediate cancellation of the older subscription (in WooCommerce > Subscriptions > Edit Subscription, change the status to "Cancelled" or "Expired").
  4. On release, log in as the reader in a new session.
  5. In the connected ESP, observe that the contact's metadata reflects a mix of data from both subscriptions: e.g. the NP_Current Subscription End Date will reflect the time you changed the status of the cancelled subscription in step 3, but the NP_Membership Status, NP_Billing Cycle, * Payment Date fields, etc. will all reflect the more recent active one.
  6. Check out this branch and purchase a third subscription with the third product (so you should have one cancelled/expired sub and 2x active ones). Confirm that the contact data in the ESP now reflects only this most recent subscription, and that NP_Current Subscription End Date has been emptied out (because there's an active subscription, so there shouldn't be an end date).
  7. As an admin, cancel the older active subscription. Confirm that the contact data in the ESP still reflects only the remaining active subscription.

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 [Status] Needs Review The issue or pull request needs to be reviewed Reader Activation labels Apr 24, 2024
@dkoo dkoo self-assigned this Apr 24, 2024
@dkoo dkoo requested a review from a team as a code owner April 24, 2024 22:23
@dkoo dkoo changed the base branch from trunk to release April 24, 2024 22:23
Copy link
Member

@adekbadek adekbadek left a comment

Choose a reason for hiding this comment

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

In my testing, only the "total paid" field gets updated*. I've two products with different prices:

  1. Add subscription w/ product A -> ESP updated as expected
  2. Add subscription w/ product B -> only "total paid" field updated
  3. Cancel subscription for product A -> only "end date" is updated

In other words, fields don't get overwritten, only previously empty fields are updated.

* by which I mean both what I see in ESP (AC) and the value of $metadata here:

$metadata = array_merge( $order_metadata, $metadata );

@dkoo
Copy link
Contributor Author

dkoo commented May 8, 2024

In other words, fields don't get overwritten, only previously empty fields are updated.

@adekbadek hmm, I can't replicate this. Are your subscription products set to both "virtual" and "downloadable"? If not, their orders might be stuck in "processing" status, and they won't be found by the method that looks for the last completed order (get_last_successful_order). #3072 should resolve this for most sites.

@dkoo dkoo requested a review from adekbadek May 8, 2024 22:38
Copy link
Member

@adekbadek adekbadek left a comment

Choose a reason for hiding this comment

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

Are your subscription products set to both "virtual" and "downloadable"? If not, their orders might be stuck in "processing" status, and they won't be found

That was it! One of my test products was not "downloadable". Works fine now!

@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, 2024
@dkoo dkoo merged commit 2c7763a into release May 13, 2024
8 checks passed
@dkoo dkoo deleted the hotfix/current-subscription branch May 13, 2024 16:19
matticbot pushed a commit that referenced this pull request May 13, 2024
## [3.8.3](v3.8.2...v3.8.3) (2024-05-13)

### Bug Fixes

* **ras:** sync purchase data only for most recent order/subscription ([#3086](#3086)) ([2c7763a](2c7763a))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.8.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request May 15, 2024
# [4.0.0-alpha.4](v4.0.0-alpha.3...v4.0.0-alpha.4) (2024-05-15)

### Bug Fixes

* autocomplete orders only for virtual products ([#3111](#3111)) ([bfbe554](bfbe554))
* **google-login:** get the email from the /tokeninfo endpoint ([#3117](#3117)) ([3296f1a](3296f1a))
* **google-oauth:** use a custom table for transients ([#3106](#3106)) ([d4a2f5c](d4a2f5c))
* **oauth-transients:** remove redundant cleanup ([#3112](#3112)) ([c123c02](c123c02))
* **ras:** sync purchase data only for most recent order/subscription ([#3086](#3086)) ([2c7763a](2c7763a))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.0.0-alpha.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@matticbot
Copy link
Contributor

🎉 This PR is included in version 4.0.0-epic-ras-acc.1 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants