fix: change the current product criteria for sync #3416

leogermani · 2024-09-11T21:24:46Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Updates the criteria to get what is considered the "Current product" for sync purposes

How to test the changes in this Pull Request:

Just for testing, make the get_current_product_order_for_sync method public
Create a user with different sets of purchases, and make sure the method always return the expected result, according to tis description
Make sure to test subscriptions, expired subscriptions, one time donations, recurring donations and a user with only a regular purchase (not subscription and not donation)

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?

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1208262205728470

dkoo

Works as described! A minor nitpick, and a non-blocking question below. Also, do you think it's worth adding a unit test where we mock some users who own various types of products (and some who own none), to test the logic? Or is that overkill?

dkoo · 2024-09-17T21:30:30Z

includes/reader-activation/sync/class-woocommerce.php

+			array_keys( \wcs_get_users_subscriptions( $user_id ) ),
+			function( $acc, $subscription_id ) {
+				$subscription = \wcs_get_subscription( $subscription_id );
+				if ( $subscription->has_status( [ 'on-hold', 'cancelled', 'expired' ] ) ) {


Nit: to keep things consistent, should we use this constant? e.g.

Suggested change

if ( $subscription->has_status( [ 'on-hold', 'cancelled', 'expired' ] ) ) {

if ( ! $subscription->has_status( WooCommerce_Connection::ACTIVE_SUBSCRIPTION_STATUSES ) ) {

That was actually intentional, but open for discussion.

My idea is that maybe we don't want to consider all non-active statuses there. Because what we are looking for are former donors/subscribers.

For example, "pending" could represent someone who never actually finished a subscription, right? or even "trash" or "deleted".

btw, in some cases "on-hold" could be the status of a subscription that was never active, it just had a failed payment and never completed...

wdyt?

These are good points! In that case maybe it'd be a good idea to create a constant for FORMER_SUBSCRIPTION_STATUSES or something similar so that we can more easily explain why we're looking at these statuses in particular?

tests/unit-tests/reader-activation-sync.php

leogermani · 2024-09-18T18:45:03Z

Also, do you think it's worth adding a unit test where we mock some users who own various types of products (and some who own none), to test the logic? Or is that overkill?

It's not overkill, I did some drafts but it takes a lot of mocking... and also it would basically require that we write tests for methods such as get_active_subscriptions_for_user, get_most_recent_cancelled_or_expired_subscription and get_one_time_donation_order_for_user... and all of them rely heavily on woocommerce plugins. I just thought it was too much effort for this one PR that needed to be shipped - and since the methods are small and simple enough, I didn't feel it was critical

dkoo

I think it's okay to proceed without the extra tests—if we decide we do want them, that can be another PR in the future so they don't block this release.

# [5.4.0-alpha.6](v5.4.0-alpha.5...v5.4.0-alpha.6) (2024-09-20) ### Bug Fixes * change the current product criteria for sync ([#3416](#3416)) ([28a84bc](28a84bc)) * **esp-sync:** sync Connected Account field ([#3414](#3414)) ([61c02bc](61c02bc)) * hide My Account links if not relevant ([#3394](#3394)) ([0d039d8](0d039d8)) * **phpcs:** specify path in custom ruleset ref ([#3384](#3384)) ([b143e74](b143e74)) * prevent PHP notice while checking my-account page ([#3435](#3435)) ([146a26f](146a26f)) * **ras-sync:** deprecate redundant Signup_Page meta field ([#3439](#3439)) ([61d6de8](61d6de8)) * **reader-registration-block:** fix initial newsletter checkbox state ([1890efe](1890efe)) * replace `newspack_image_credits_placeholder` default value ([#3433](#3433)) ([c754fc2](c754fc2)) * **sync:** method name for `membership_saved` handler ([#3399](#3399)) ([2c0bf26](2c0bf26)) * **sync:** place esp sync admin features behind a constant ([#3438](#3438)) ([20a0970](20a0970)) * **sync:** remove localized number format ([#3434](#3434)) ([2243a5d](2243a5d)) ### Features * add a new action to when a ras setting is updated ([#3357](#3357)) ([35d3492](35d3492)) * **ga4:** detect gate interaction blocks ([#3408](#3408)) ([e14913c](e14913c)) * media kit page handling ([#3358](#3358)) ([4454850](4454850)) * **ras:** sync class ([#3362](#3362)) ([88acbee](88acbee)) * **ras:** unify ESP connector strategy for data events ([#3360](#3360)) ([7080864](7080864)) * **reader-activation:** ESP-related tweaks ([#3381](#3381)) ([ac68b67](ac68b67)) * remove Woo Membersip sync fields ([#3411](#3411)) ([28052e8](28052e8)) * **sync:** add ESP sync notice to RAS wizard ([#3400](#3400)) ([f9acd56](f9acd56))

matticbot · 2024-09-20T18:22:46Z

🎉 This PR is included in version 5.4.0-alpha.6 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

# [5.5.0-alpha.1](v5.4.0...v5.5.0-alpha.1) (2024-10-03) ### Bug Fixes * change the current product criteria for sync ([#3416](#3416)) ([28a84bc](28a84bc)) * **esp-meta:** handle state of the 'Woo Team' meta ([#3352](#3352)) ([ba5ea1e](ba5ea1e)) * **esp-sync:** sync Connected Account field ([#3414](#3414)) ([61c02bc](61c02bc)) * **esp-wc-metadata:** last payment date & amount handling ([#3363](#3363)) ([d1abbfe](d1abbfe)) * **guest-author:** enqueue the guest author admin script selectively ([0bf37af](0bf37af)) * hide My Account links if not relevant ([#3394](#3394)) ([0d039d8](0d039d8)) * make email template fetching deterministic ([#3341](#3341)) ([ace91aa](ace91aa)) * **phpcs:** specify path in custom ruleset ref ([#3384](#3384)) ([b143e74](b143e74)) * prevent PHP notice while checking my-account page ([#3435](#3435)) ([146a26f](146a26f)) * **ras-sync:** deprecate redundant Signup_Page meta field ([#3439](#3439)) ([61d6de8](61d6de8)) * **reader-registration-block:** fix initial newsletter checkbox state ([1890efe](1890efe)) * replace `newspack_image_credits_placeholder` default value ([#3433](#3433)) ([c754fc2](c754fc2)) * **sync:** method name for `membership_saved` handler ([#3399](#3399)) ([2c0bf26](2c0bf26)) * **sync:** place esp sync admin features behind a constant ([#3438](#3438)) ([20a0970](20a0970)) * **sync:** remove localized number format ([#3434](#3434)) ([2243a5d](2243a5d)) * wizards - update type check conditional for `custom_logo` ([#3442](#3442)) ([125f756](125f756)) * woocommerce connection tests ([#3372](#3372)) ([5cea128](5cea128)) * **woocommerce-connection:** handle explicit UTM meta fields meta ([#3371](#3371)) ([bf9e997](bf9e997)) ### Features * add a new action to when a ras setting is updated ([#3357](#3357)) ([35d3492](35d3492)) * **esp-sync:** sync membership data regardless of subscription ([#3353](#3353)) ([9f7d1de](9f7d1de)) * **ga4:** detect gate interaction blocks ([#3408](#3408)) ([e14913c](e14913c)) * **ga:** disable tracking for editors regardless of RA status ([81323c3](81323c3)) * media kit page handling ([#3358](#3358)) ([4454850](4454850)) * **memberships:** add memberships-related body classes ([b56b9d8](b56b9d8)) * **ras:** esp sync tools ([#3359](#3359)) ([d7dd754](d7dd754)) * **ras:** helper method for ESP master list ([#3355](#3355)) ([ec56d5b](ec56d5b)) * **ras:** sync class ([#3362](#3362)) ([88acbee](88acbee)) * **ras:** unify ESP connector strategy for data events ([#3360](#3360)) ([7080864](7080864)) * **reader-activation:** ESP-related tweaks ([#3381](#3381)) ([ac68b67](ac68b67)) * remove Woo Membersip sync fields ([#3411](#3411)) ([28052e8](28052e8)) * **sync:** add ESP sync notice to RAS wizard ([#3400](#3400)) ([f9acd56](f9acd56))

matticbot · 2024-10-03T20:54:07Z

🎉 This PR is included in version 5.5.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

# [5.5.0](v5.4.1...v5.5.0) (2024-10-08) ### Bug Fixes * cancelled subscriptions sync ([#3466](#3466)) ([b605a7f](b605a7f)) * change the current product criteria for sync ([#3416](#3416)) ([28a84bc](28a84bc)) * **esp-meta:** handle state of the 'Woo Team' meta ([#3352](#3352)) ([ba5ea1e](ba5ea1e)) * **esp-sync:** sync Connected Account field ([#3414](#3414)) ([61c02bc](61c02bc)) * **esp-wc-metadata:** last payment date & amount handling ([#3363](#3363)) ([d1abbfe](d1abbfe)) * **guest-author:** enqueue the guest author admin script selectively ([0bf37af](0bf37af)) * hide My Account links if not relevant ([#3394](#3394)) ([0d039d8](0d039d8)) * make email template fetching deterministic ([#3341](#3341)) ([ace91aa](ace91aa)) * **phpcs:** specify path in custom ruleset ref ([#3384](#3384)) ([b143e74](b143e74)) * prevent PHP notice while checking my-account page ([#3435](#3435)) ([146a26f](146a26f)) * **ras-sync:** deprecate redundant Signup_Page meta field ([#3439](#3439)) ([61d6de8](61d6de8)) * **reader-registration-block:** fix initial newsletter checkbox state ([1890efe](1890efe)) * replace `newspack_image_credits_placeholder` default value ([#3433](#3433)) ([c754fc2](c754fc2)) * **sync:** method name for `membership_saved` handler ([#3399](#3399)) ([2c0bf26](2c0bf26)) * **sync:** place esp sync admin features behind a constant ([#3438](#3438)) ([20a0970](20a0970)) * **sync:** remove localized number format ([#3434](#3434)) ([2243a5d](2243a5d)) * wizards - update type check conditional for `custom_logo` ([#3442](#3442)) ([125f756](125f756)) * woocommerce connection tests ([#3372](#3372)) ([5cea128](5cea128)) * **woocommerce-connection:** handle explicit UTM meta fields meta ([#3371](#3371)) ([bf9e997](bf9e997)) ### Features * add a new action to when a ras setting is updated ([#3357](#3357)) ([35d3492](35d3492)) * **esp-sync:** sync membership data regardless of subscription ([#3353](#3353)) ([9f7d1de](9f7d1de)) * **ga4:** detect gate interaction blocks ([#3408](#3408)) ([e14913c](e14913c)) * **ga:** disable tracking for editors regardless of RA status ([81323c3](81323c3)) * media kit page handling ([#3358](#3358)) ([4454850](4454850)) * **memberships:** add memberships-related body classes ([b56b9d8](b56b9d8)) * **ras:** esp sync tools ([#3359](#3359)) ([d7dd754](d7dd754)) * **ras:** helper method for ESP master list ([#3355](#3355)) ([ec56d5b](ec56d5b)) * **ras:** sync class ([#3362](#3362)) ([88acbee](88acbee)) * **ras:** unify ESP connector strategy for data events ([#3360](#3360)) ([7080864](7080864)) * **reader-activation:** ESP-related tweaks ([#3381](#3381)) ([ac68b67](ac68b67)) * remove Woo Membersip sync fields ([#3411](#3411)) ([28052e8](28052e8)) * **sync:** add ESP sync notice to RAS wizard ([#3400](#3400)) ([f9acd56](f9acd56))

leogermani and others added 5 commits September 11, 2024 18:24

fix: change the current product criteria for sync

65c5c51

fix: undefined/unused variable and $donation_product type

6eb3715

test: update tests

93bdcf7

test: remove failing test

d57560f

test: improve tests

da0a642

leogermani self-assigned this Sep 17, 2024

leogermani added the [Status] Needs Review The issue or pull request needs to be reviewed label Sep 17, 2024

leogermani marked this pull request as ready for review September 17, 2024 18:21

leogermani requested a review from a team as a code owner September 17, 2024 18:21

dkoo requested changes Sep 17, 2024

View reviewed changes

dkoo approved these changes Sep 18, 2024

View reviewed changes

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 Sep 18, 2024

leogermani added 2 commits September 18, 2024 17:06

chore: use constant to hold former subscriber statuses

e0b2778

test: bring back deleted tests with mock

2f1a305

dkoo approved these changes Sep 18, 2024

View reviewed changes

leogermani merged commit 28a84bc into trunk Sep 18, 2024
10 checks passed

leogermani deleted the fix/current-product-criteria branch September 18, 2024 21:22

matticbot added the released on @alpha label Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: change the current product criteria for sync #3416

fix: change the current product criteria for sync #3416

leogermani commented Sep 11, 2024 •

edited

Loading

dkoo left a comment

dkoo Sep 17, 2024

leogermani Sep 18, 2024

dkoo Sep 18, 2024

leogermani Sep 18, 2024

leogermani commented Sep 18, 2024

dkoo left a comment

matticbot commented Sep 20, 2024

matticbot commented Oct 3, 2024

	if ( $subscription->has_status( [ 'on-hold', 'cancelled', 'expired' ] ) ) {
	if ( ! $subscription->has_status( WooCommerce_Connection::ACTIVE_SUBSCRIPTION_STATUSES ) ) {

fix: change the current product criteria for sync #3416

fix: change the current product criteria for sync #3416

Conversation

leogermani commented Sep 11, 2024 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

dkoo left a comment

Choose a reason for hiding this comment

dkoo Sep 17, 2024

Choose a reason for hiding this comment

leogermani Sep 18, 2024

Choose a reason for hiding this comment

dkoo Sep 18, 2024

Choose a reason for hiding this comment

leogermani Sep 18, 2024

Choose a reason for hiding this comment

leogermani commented Sep 18, 2024

dkoo left a comment

Choose a reason for hiding this comment

matticbot commented Sep 20, 2024

matticbot commented Oct 3, 2024

leogermani commented Sep 11, 2024 •

edited

Loading