feat(ras): esp sync tools #3359

miguelpeixe · 2024-08-22T15:01:37Z

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:

1200530782742699-as-1207717004877609/f

Moves the logic for syncing Woo customers and orders to the ESP from the newspack-subscription-migrations plugin to here.

The original class was refactored so it provides generic methods to be extended by the existing CLI and new admin functionality. Its name has also changed from "Woo Sync" to "ESP Sync", given Woo is part of the contact data but the goal is to sync readers to an ESP.

Some changes to the generic logic are worth mentioning:

can_esp_sync( bool $return_errors ) was implemented so it can be called across CLI and admin functionality with proper error handling
get_batch_of_customers() was changed to get_batch_of_readers(). It's now using \Newspack\Reader_Activation::get_reader_roles() because although they are customers, every WP user can be a customer and the goal is to sync RAS readers
The sync( $contact, $context ) method (previously sync_contact()) was modified to support any ESP through the Newspack_Newsletters_Contacts::upsert() method. For that, a new helper method is proposed for RAS so we can get the master list given any provider

The CLI command changed from wp newspack woo resync to wp newspack esp sync, but preserves all the original functionality and args support. The exception is for the flag --migrated-subscriptions, which requires the newspack-subscription-migrations plugin to be installed, otherwise it returns an error. A new flag was added for --sync-context to be passed down to Newspack_Newsletters_Contacts::upsert() (--context is reserved).

The new functionality allows an admin user to sync contacts through the dashboard user table, both individually and through the bulk action dropdown:

Single	Bulk

How to test the changes in this Pull Request:

CLI

Run wp newspack esp sync --dry-run for a full dry run
Confirm the command runs successfully, going through all the readers in your database
Choose a reader and run wp newspack esp sync --user-ids=USER_ID with their user ID
Confirm via PHP logs that [Newspack_Newsletters_Contacts::upsert]: Adding contact to list(s): LIST_ID. Provider is PROVIDER. is called and you can inspect the normalized contact data
Do the same but now with 2 user IDs: wp newspack esp sync --user-ids=USER_ID1,USER_ID2
Confirm via PHP logs the 2 entries as in step 4
Grab 2 Woo Subscription IDs and run: wp newspack esp sync --subscription-ids=ID1,ID2
Confirm the command runs without issues and the PHP log entries for the ::upsert() is correct
Grab 2 Woo Order IDs and run: wp newspack esp sync --order-ids=ID1,ID2
Confirm the command runs without issues and the PHP log entries for the ::upsert() is correct
Cancel a reader subscription and run a sync for active subs only, along with a reader with an active subscription: wp newspack esp sync --active-only --user-ids=USER_ID1,USER_ID2
Confirm only the reader with an active sub got synced
Test dry run syncs modifying --offset, --max-batches, and --batch-size to confirm these values are applied correctly

Admin

Navigate to "Users", in WP dashboard
Hover the rows and confirm you see a "Resync contact to ESP" row action link
Click and confirm you get a success message and the PHP logs show ::upsert() was called correctly
Select multiple readers, click on the Bulk actions dropdown, select "Resync to the ESP", and apply
Confirm you get a success message and the PHP logs show ::upsert() was called correctly for every selected contact

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?

leogermani

Now I see why you fell on a rabbit hole

It's really confusing how things are duplicate. The current connectors and what you have here... the normalize_contact filter acting at the end of it all...

I'm not sure how much repetition is between this and the what happens at normalize_contact_data if any. I bet you are making it all better in the upcoming refactors... we need to look at all usages of the newspack_esp_sync_normalize_contact filter.

I left a few minor comments.

CLI works great. wp-admin works great

Bulk action on wp-admin, as expected, times out if I try to run it on a full page of users... I'm not sure we want that... IMO, either we set up a proper way to do it async, or via Scheduled actions, or we should leave it out

leogermani · 2024-08-27T15:37:10Z

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

+		$user_id  = $is_order ? $order->get_customer_id() : $user_id_or_order;
+		$user     = \get_userdata( $user_id );
+
+		// Backfill Network Registration Site field if needed.


this looks weird here. Newspack Network should handle this, adding the field via filters as it already does

Network does this using the newspack_data_events_reader_registered_metadata filter currently present in our Connectors. That should be enough

https://github.com/Automattic/newspack-network/blob/trunk/includes/class-esp-metadata-sync.php#L26

It's part of the original tool in newspack-subscription-migrations, I didn't want to change it. So you're saying it's redundant logic and should no longer be in this sync?

Need to check. If that filter is still being called in this flow, things should be fine. but need to test

I've just tested it and it doesn't work.

I think we need to make sure that this sync goes through the same filters the current one goes. Or we need to adapt Newspack Network and add another filter to do the same thing. I prefer if Network could do this using only one filter.

Another integration to look at and see if still works is the Woo team ID -> #3325

You mean it doesn't work without it, right? So we leave it for now. I also prefer for this highly specific piece code to not live inside this class, but I don't want to change more things in this PR.

Another integration to look at and see if still works is the Woo team ID -> #3325

This should work the same, the filters didn't change and the Newspack_Newsletters class, which runs its normalization after the ::upsert() call, still behaves as intended.

So we leave it for now. I also prefer for this highly specific piece code to not live inside this class, but I don't want to change more things in this PR.

I would sleep better if we fixed this in this PR, but it's ok if you promise this is being fixed in the follow up PRs - but they can't take long then. This is really ugly and can lead to problems if we decide to do any refactors on the network plugin

includes/reader-activation/class-esp-sync-admin.php

includes/cli/class-ras-esp-sync.php

leogermani · 2024-08-27T18:50:28Z

includes/cli/class-ras-esp-sync.php

+			false === $config['migrated_only']
+		) {
+			static::log( __( 'Syncing all readers...', 'newspack-plugin' ) );
+			$user_ids = static::get_batch_of_readers( $config['batch_size'], $config['offset'] );


"all" readers here is not accurate, and can lead the user to believe something went wrong and the command is not respecting the chosen filter.

This is the case here. If no arguments are passed, it'll batch through every reader account.

hmm when I ran it with "active only" (or some other filter) I got the "Syncing all readers... " message. will try to reproduce it

Fixed in 8b3ccad

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

miguelpeixe · 2024-08-27T23:01:14Z

Thank you for the review!

Now I see why you fell on a rabbit hole

Right? 😄

I'm not sure how much repetition is between this and the what happens at normalize_contact_data if any. I bet you are making it all better in the upcoming refactors... we need to look at all usages of the newspack_esp_sync_normalize_contact filter.

Yes, in my draft PR that method and all other metadata related methods from the Newspack_Newsletters class are moving to a Metadata class. The normalize_contact_data should no longer be a filter and should be called right before the ::upsert() inside the sync() method for the ESP_Sync class. Less fragile and more readable.

Bulk action on wp-admin, as expected, times out if I try to run it on a full page of users... I'm not sure we want that... IMO, either we set up a proper way to do it async, or via Scheduled actions, or we should leave it out

I like the idea of using AS for this, but we should also care for the feedback. How will we notify the state of the async request to the admin? Certainly something for another PR.

I'm ok leaving it as is but adding an error message if more than, let's say, 5 users are selected. The admin will receive an error notice: "Exceeded selection limit. Please select no more than 5 users per execution." They'll adjust and be able to use some batch action.

leogermani · 2024-08-28T14:59:07Z

I'm ok leaving it as is but adding an error message if more than, let's say, 5 users are selected. The admin will receive an error notice: "Exceeded selection limit. Please select no more than 5 users per execution." They'll adjust and be able to use some batch action.

This could work. I don't love it but... maybe get a third opinion?

miguelpeixe · 2024-08-28T16:32:15Z

As we discussed internally, b98bf91 implements AS without feedback on batch success. The admin should receive the following notice when running the batch action:

The link takes the admin to Woo's AS status page, querying by the hook name (newspack_sync_admin_batch).

If AS is not available (Woo not installed), the bulk action is not available.

leogermani · 2024-08-28T18:57:06Z

includes/cli/class-ras-esp-sync.php

+	 */
+	private static function get_batch_of_readers( $batch_size, $offset = 0 ) {
+		$roles = Reader_Activation::get_reader_roles();
+		if ( defined( 'NEWSPACK_NETWORK_READER_ROLE' ) && ! in_array( NEWSPACK_NETWORK_READER_ROLE, $roles, true ) ) {


This is not necessary. Newspack Network, if active, will filter get_reader_roles and add its role there

Well, another reason why this should've been reader roles all along 😄

Fixed in e625328

leogermani · 2024-08-28T19:08:32Z

includes/reader-activation/class-esp-sync-admin.php

+			$scheduled_syncs = absint( wp_unslash( $_GET['scheduled-sync-contacts'] ) );
+			$message         = sprintf(
+				// translators: %1$d: number of contacts resynced. %2$d: estimated time in seconds.
+				__( '%1$d contacts scheculed for resync to the ESP. This should take approximately %2$d seconds.', 'newspack-plugin' ),


Looking good. Nice that we don't show the option if ActionScheduler is not present.

As a suggestion, I think we should just say that the contacts are going to be synced in background and not give an estimate.

There is only 10 contacts in a page, so the max amount we are going to show is 20 seconds

20 seconds doesn't sound that long to justify all the hassle

If it really were only 20 seconds it would not timeout.

Each action is processed by AS in an independent request, so there is the overhead of making a request, etc. It takes a little more.... and how long it takes is a bit unpredictable...

idk, just feel that this info there doesn't help.

"They will be processed in a little while..." idk

We can say it'll take "a couple of minutes". It's a good enough estimate.

Updated in 0d554eb

# [5.4.0-alpha.1](v5.3.3...v5.4.0-alpha.1) (2024-08-29) ### Bug Fixes * **esp-meta:** handle state of the 'Woo Team' meta ([#3352](#3352)) ([ba5ea1e](ba5ea1e)) * **esp-wc-metadata:** last payment date & amount handling ([#3363](#3363)) ([d1abbfe](d1abbfe)) * **guest-author:** enqueue the guest author admin script selectively ([0bf37af](0bf37af)) * make email template fetching deterministic ([#3341](#3341)) ([ace91aa](ace91aa)) * woocommerce connection tests ([#3372](#3372)) ([5cea128](5cea128)) * **woocommerce-connection:** handle explicit UTM meta fields meta ([#3371](#3371)) ([bf9e997](bf9e997)) ### Features * **esp-sync:** sync membership data regardless of subscription ([#3353](#3353)) ([9f7d1de](9f7d1de)) * **ga:** disable tracking for editors regardless of RA status ([81323c3](81323c3)) * **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))

matticbot · 2024-08-29T19:04:07Z

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

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:52Z

🎉 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))

miguelpeixe added 3 commits August 21, 2024 12:02

feat(ras): helper method for ESP master list

3037cd4

fix: ensure bool return

063bc1b

feat(ras): woo sync tools

e17ccf0

miguelpeixe requested a review from a team as a code owner August 22, 2024 15:01

miguelpeixe self-assigned this Aug 22, 2024

miguelpeixe added the [Status] Needs Review The issue or pull request needs to be reviewed label Aug 22, 2024

miguelpeixe mentioned this pull request Aug 22, 2024

feat: woocommerce data sync tools Automattic/newspack-newsletters#1624

Closed

6 tasks

miguelpeixe added 6 commits August 22, 2024 12:04

chore: update textdomain

499bd5b

chore: use namespace for WooCommerce_Connection

3b5848d

chore: rename class

3d09777

chore: remove superfluous check

fdda98d

fix: method calling

ba4292d

chore: woo sync is now an ESP sync

7658806

miguelpeixe changed the title ~~feat(ras): woo sync tools~~ feat(ras): esp sync tools Aug 22, 2024

miguelpeixe added 7 commits August 22, 2024 15:34

feat: check master list and make ::sync() protected

406bd7c

chore: fix doc

24ecd80

chore: adjust wording

8a7f0b2

feat: support custom context

f808a92

chore: update context defaults

098dca9

chore: rename method

f80495c

fix: class access

3bd55da

Base automatically changed from fix/ras-esp-master-list-helper to trunk August 26, 2024 12:26

miguelpeixe added 2 commits August 27, 2024 09:22

Merge branch 'trunk' into feat/woo-sync-tools

44ca1e4

Merge branch 'trunk' into feat/woo-sync-tools

a256b54

leogermani requested changes Aug 27, 2024

View reviewed changes

miguelpeixe added 2 commits August 27, 2024 19:31

fix: method calling and remove unused code

97b8056

fix: method calling

a8656cd

miguelpeixe requested a review from leogermani August 27, 2024 23:01

Merge branch 'trunk' into feat/woo-sync-tools

6ff08d8

feat: admin bulk action via ActionScheduler

b98bf91

feat: add AS sorting for convenience

bdc3d9e

leogermani reviewed Aug 28, 2024

View reviewed changes

miguelpeixe added 3 commits August 28, 2024 16:40

chore: remove redundant code

e625328

chore: adjust admin notice

0d554eb

fix: cli logging for active subscriptions

8b3ccad

miguelpeixe requested a review from leogermani August 29, 2024 15:52

leogermani approved these changes Aug 29, 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 Aug 29, 2024

miguelpeixe merged commit d7dd754 into trunk Aug 29, 2024

miguelpeixe deleted the feat/woo-sync-tools branch August 29, 2024 16:29

miguelpeixe restored the feat/woo-sync-tools branch August 29, 2024 18:36

miguelpeixe deleted the feat/woo-sync-tools branch August 29, 2024 18:37

matticbot added the released on @alpha label Aug 29, 2024

This was referenced Aug 29, 2024

feat(ras): unify ESP connector strategy for data events #3360

Merged

fix(sync): use newspack_esp_sync_contact filter for the network_registration_site field Automattic/newspack-network#132

Merged

miguelpeixe mentioned this pull request Sep 6, 2024

feat(ras): sync class #3362

Merged

6 tasks

feat(ras): esp sync tools #3359

feat(ras): esp sync tools #3359

Uh oh!

Conversation

miguelpeixe commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

CLI

Admin

Other information:

Uh oh!

leogermani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miguelpeixe commented Aug 27, 2024

Uh oh!

leogermani commented Aug 28, 2024

Uh oh!

miguelpeixe commented Aug 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matticbot commented Aug 29, 2024

Uh oh!

matticbot commented Oct 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miguelpeixe commented Aug 22, 2024 •

edited

Loading