feat(integrations): add subscription and donation metadata#4597
feat(integrations): add subscription and donation metadata#4597chickenn00dle merged 5 commits intotrunkfrom
Conversation
0980297 to
ec9dfe1
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ec9dfe1 to
33c8baf
Compare
There was a problem hiding this comment.
Pull request overview
Adds Reader Activation contact metadata for WooCommerce Subscriptions and Donations so ESP syncs can include richer subscription/donation lifecycle fields.
Changes:
- Implemented
Subscription::get_metadata()with 13 subscription-related fields (status, counts, dates, billing, product, coupon, last payment, cancellation reason). - Implemented
Donation::get_metadata()by extendingSubscription, filtering to donation-only, and adding donor status + one-time donation fallbacks and previous amount tracking. - Added unit tests + expanded WooCommerce test mocks; added a shared date formatter in the Contact_Metadata base class and extra sync logging.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit-tests/reader-activation-sync/class-test-subscription-metadata.php | New unit tests covering Subscription metadata fields and edge cases. |
| tests/unit-tests/reader-activation-sync/class-test-donation-metadata.php | New unit tests covering Donation metadata fields, donor labels, and switch/previous amount logic. |
| tests/mocks/wc-mocks.php | Extends WC mocks to support new metadata logic (products/items, coupons, related orders, order lookup). |
| includes/reader-activation/sync/contact-metadata/class-subscription.php | Implements Subscription metadata retrieval with caching and helpers. |
| includes/reader-activation/sync/contact-metadata/class-donation.php | Implements Donation metadata by reusing Subscription helpers + donation-specific fallbacks. |
| includes/reader-activation/sync/class-contact-sync.php | Adds additional logging around contact sync calls. |
| includes/reader-activation/sync/class-contact-metadata.php | Adds a shared date format constant and formatter helper. |
Comments suppressed due to low confidence (1)
tests/mocks/wc-mocks.php:318
- The mock WC_Subscription::get_date() only accepts one parameter, but production WC_Subscription::get_date() is commonly called with a timezone/context argument (e.g., get_date('start','site')). The new Subscription metadata code passes a second argument, which can trigger warnings/errors in tests on newer PHP versions; update the mock signature to accept an optional second parameter (and ignore it).
public function get_date( $type ) {
return $this->data['dates'][ $type ] ?? 0;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
includes/reader-activation/sync/contact-metadata/class-subscription.php
Outdated
Show resolved
Hide resolved
includes/reader-activation/sync/contact-metadata/class-donation.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tests/mocks/wc-mocks.php:318
- WC_Subscription::get_date() in this test mock only accepts one argument, but the new metadata code calls
$subscription->get_date( ..., 'site' )(matching the real WooCommerce Subscriptions signature). In PHP 8 this will throw an ArgumentCountError in unit tests. Update the mock method signature to accept the optional timezone/context parameter (or use variadic args) and ignore it as needed so tests exercise the production call shape.
public function get_date( $type ) {
return $this->data['dates'][ $type ] ?? 0;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
leogermani
left a comment
There was a problem hiding this comment.
Beautiful!
Works well and has a clean implementation!
The only blocker comment is that it's missing the rule to prioritize non-gift subscriptions over gifts in the current subscription.
I also left a few minor comments.
And I have one kind of a NIT, let me know what you think.
I think that instead of Donations extending Subscriptions, it would be better if both classes extended a common shared class. Only because the relationship would be more explicit and it's less likely that someone in the future makes a change to the Subscription class without realizing it also affects Donations...
|
|
||
| $active = $this->get_active_subscriptions(); | ||
| if ( ! empty( $active ) ) { | ||
| $this->current_subscription_cache = reset( $active ); |
There was a problem hiding this comment.
you are just getting the first review in the array. Will it be the most recent? How are they ordered?
There was a problem hiding this comment.
Looking at the current methods, it seems that they do the same thing... but worth checking if this is true.
Also, I think there's a requirement to favor non-gift subscriptions over gifts, so it's worth adding this rule as well
There was a problem hiding this comment.
you are just getting the first review in the array. Will it be the most recent? How are they ordered?
Yes. It should be the most recent. Subscriptions are populated here:
The wcs_get_users_subscriptions method returns an array of subscriptions in descending order by ID. We should be respecting this order when populating our user subscriptions cache and so the first entry should be the subscription with the largest ID (the most recent).
That said, if we feel this is too fragile, I'm happy to implement our own sorting logic.
There was a problem hiding this comment.
Good catch about non-gift preference! Added a commit to account for this here: df3220b
| return $this->current_subscription_cache; | ||
| } | ||
|
|
||
| foreach ( $this->get_user_subscriptions() as $subscription ) { |
There was a problem hiding this comment.
same question about order.
| /** | ||
| * The date format to use for all date fields, which is MM/DD/YYYY. | ||
| */ | ||
| const DATE_FORMAT = 'm/d/Y'; |
There was a problem hiding this comment.
The current default format is 'Y-m-d H:i:s'. Any reason to use a different one?
There was a problem hiding this comment.
Good question. No reason in particular. This was an arbitrary decision made by Claude 😝. Updated to use the current default in df3220b
There was a problem hiding this comment.
Oh wait. Sorry. This was not arbitrary. Looks like the metadata spec sheet defines the 'm/d/Y' format.
Is this a hard requirement? If so I'll revert this format change to what I had initially.
|
Thanks for your review @leogermani! I addressed most of your feedback and this is ready for another round of review.
My thinking is donations overlap subscriptions in functionality so much so that if there needs to be a change to the subscription class in the future, its very likely it will also need to be made to the donation class as well. There are also references to the donation class in the |
Makes sense |
leogermani
left a comment
There was a problem hiding this comment.
Thanks for the changes.
Approving for now, we can check with Katie and others about the date format later
|
Hey @chickenn00dle, good job getting this PR merged! 🎉 Now, the Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label. If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label. Thank you! ❤️ |
Summary
get_metadata()for theSubscriptioncontact metadata class with 13 get methods covering subscriber status, active count, dates, billing cycle, payments, product names, coupon codes, and cancellation reasons — all filtered to non-donation subscriptions only.get_metadata()for theDonationclass by extendingSubscription, inverting the filter to donation-only, and adding donor status labels (Monthly/Yearly/Ex-* Donor), one-time donation fallbacks, and previous donation amount tracking.Closes NPPD-1388
Test plan
wp-config.php:🤖 Generated with Claude Code