feat(backfill): WP CLI script to backfill reader_registered, donation_new events #51

adekbadek · 2024-02-14T07:52:21Z

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:

Adds a WP CLI script to backfill historical data.

How to test the changes in this Pull Request:

Switch newspack-plugin to feat/tweaks-for-network-update branch (Feat/tweaks for network update newspack-plugin#2935)
Disable this plugin on both the hub and a node site, in order to build up historical data
On both sites (using different email addresses):
1. register a user,
2. make a donation
3. make a recurring donation and the cancel the related subscription in WC
Activate the newspack-network plugin on both sites
Verify that the new user registrations and donations are not present in the Hub's event log
Run wp newspack-network data-backfill reader_registered --start=2024-01-01 --end=2024-12-31 on the Hub site, observe the output the dry run and confirm nothing was added to the event log.
1. Do the same with donation_new as the argument instead of reader_registered.
2. And then with donation_subscription_cancelled
Try with --verbose flag for more output
Run wp newspack-network data-backfill reader_registered --start=2024-01-01 --end=2024-12-31 --live on the Hub site and observe the event was added to the event log.
1. Do the same with donation_new as the argument instead of reader_registered.
2. And then with donation_subscription_cancelled
Repeat the last three steps on the Node site, but instead of the event log look as the scheduled events in the Node Settings
Run in the live mode again to ensure that duplicate events are not added to the event log or the scheduled events* (in the CLI output, observe it's noted that no events were processed and how many were skipped as duplicates)
After the scheduled event is transmitted, observe it was added to the Hub's event log
Run the script with all as action name argument and observe it has the same effect as running the above two scripts one after another

* unless the webhook request queue has be cleared already, but the event log should handle deduplication itself anyway

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

This is looking good, I left a couple of comments, but I wanted to suggest an architectural change so this is more aligned to the rest of the plugin.

Instead of having everything inside the Data_Backfill class, we should keep it just with the CLI command.

Then we can create a new namespace, something like Newspack_Network\Backfillers, where we can have one class per event, following the same nomenclature define in Accepted_Actions and which we use in other places.

The Backfiller_Abstract_Class could implement a method that is common to every event, what is currently the process_event_entity.

And each class need to implement its own get_events method, that will return an array with Income_Event objects (instances of their specific classes, and not the abstract one). The process_event_entity method would receive such array and process each item.

In the command callback, we would not need the switch case https://github.com/Automattic/newspack-network/pull/51/files#diff-2fbd8473ad6861f6fec58b5890525ac5092f375cbe554e54f604a9871e490ed8R324 as we would simply create an instance of the backfiller for the required action.

And we could fallback to the default class, which would simply return that this action is not backfillable (something like we do here ->

newspack-network/includes/hub/stores/class-event-log.php

Lines 52 to 56 in bcb0615

    
           $class_name = 'Newspack_Network\\Hub\\Stores\\Event_Log_Items\\' . Accepted_Actions::ACTIONS[ $item->action_name ]; 
        
           if ( ! class_exists( $class_name ) ) { 
        
           	$class_name = 'Newspack_Network\\Hub\\Stores\\Event_Log_Items\\Generic'; 
        
           } 
        
           $results[] = new $class_name(

I hope this makes sense to you.

I think it has a number of advantages, not only to keep consistency with the rest of the plugin, but also to keep small classes and avoid this class to grow indefinitely. It will also be easier to implement new backfillers for new actions, as all you need to know is implement one class, no need to touch several places in the code.

Since all the work is done and this is basically moving code around, I'm happy to do it if you prefer to focus on other tasks. Let me know!

includes/cli/class-data-backfill.php

leogermani · 2024-02-19T22:19:13Z

includes/cli/class-data-backfill.php

+	 */
+	private static function process_reader_registered( $start, $end, $live, $verbose ) {
+		// Get all users registered between $start and $end.
+		$users = get_users(


We could filter out users with newspack_remote_id metadata. No need to backfill users that came from other sites. We want users that were originally registered on this site

Oh, great suggestion! Done in 111a7b4

leogermani · 2024-02-19T22:28:35Z

includes/incoming-events/class-abstract-incoming-event.php

 	 */
-	public function __construct( $site, $data, $timestamp ) {
+	public function __construct( $site, $data, $timestamp, $action_name = false ) {


we shouldn't need this. See my comment on the suggested architecture

includes/cli/class-data-backfill.php

adekbadek · 2024-02-21T13:53:24Z

Re: architectural change – sounds good, but for later. There's already another PR - #62 - relying on this PR.

leogermani · 2024-02-21T18:02:07Z

includes/cli/class-data-backfill.php

+		}
+		foreach ( $orders as $order ) {
+			$order_id = $order->get_id();
+			$order_data = \Newspack\Data_Events\Utils::get_order_data( $order_id );


we need to check if it is a donation related order. see my review on the newspack plugin's PR

Addressed in the newspack-plugin PR

includes/cli/class-data-backfill.php

leogermani · 2024-02-21T18:53:40Z

includes/cli/class-data-backfill.php

+			self::$progress = \WP_CLI\Utils\make_progress_bar( 'Processing subscriptions', count( $subscriptions ) );
+		}
+		foreach ( $subscriptions as $subscription ) {
+			$subscription_data = \Newspack\Data_Events\Utils::get_subscription_data( $subscription );


again we need to check if it's a donation subscription here. See my comment on the main plugin PR

What happened to me was that I had a bunch of cancellations for non-donation subscriptions. My backfill data was then full of subscription_cancelled events with empy data (because the check was on the Utils function).

So the check should not be there, and be here instead, so we don't include subscription data at all.

Handled in 0153127

leogermani · 2024-02-22T12:14:53Z

When you run the command with all, the totals at the end is always zeros

leogermani

Looking good

I left one comment about the checks in donation_subscription_cancelled and donation_new, but other than that it seems to be working fine!

Also left a minor comment about the command output

leogermani · 2024-02-22T12:23:43Z

Oh, I added the comments to the commit, not to this PR.

Let me add them here as well for visibility

We also need to check if the data is empty for the order in donation_new, otherwise we get events with empty data for non donation orders.

And I think these checks should trigger a continue instead of a return

adekbadek · 2024-02-22T14:49:07Z

When you run the command with all, the totals at the end is always zeros

Fixed in f9cc61e

Refactor/backfillers

feat(backfill-cli): handle membership events

leogermani · 2024-02-22T22:35:58Z

@adekbadek I have performed a bunch of tests and all seems to be working fine, including Memberships.

I'll leave it as pre-approved to unblock you. If you could run a few more tests it would be nice. This will need to be tested with real data anyway.

And I hope you like the refactor :)

# [1.3.0-alpha.1](v1.2.0...v1.3.0-alpha.1) (2024-02-23) ### Bug Fixes * memberships sync ([#63](#63)) ([0a54f1d](0a54f1d)) ### Features * add hub bookmarks to nodes ([#56](#56)) ([54700a0](54700a0)) * add option to manually sync users ([#53](#53)) ([3ec1b19](3ec1b19)) * display membership plans from the network ([#59](#59)) ([6fa01fd](6fa01fd)) * enhance network subscriptions and orders search ([#55](#55)) ([bcb0615](bcb0615)) * Node connection using a link ([#58](#58)) ([721f8b2](721f8b2)) * **wp-cli:** add command to backfill events ([#51](#51)) ([13d2498](13d2498)) * **wp-cli:** add process pending webhook requests command ([#67](#67)) ([7dbd8dc](7dbd8dc)) * **wp-cli:** sync all events from the Hub ([#65](#65)) ([dc595ca](dc595ca))

matticbot · 2024-02-23T15:15:07Z

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

# [1.3.0](v1.2.0...v1.3.0) (2024-02-28) ### Bug Fixes * backfill duplicate handling; woo links in wp-admin-bar ([#71](#71), [#72](#72)) ([bbce13b](bbce13b)) * cli commands ([#73](#73)) ([ff563ac](ff563ac)) * memberships sync ([#63](#63)) ([0a54f1d](0a54f1d)) * missing woocommerce-memberships handling ([76dbdf7](76dbdf7)) ### Features * add hub bookmarks to nodes ([#56](#56)) ([54700a0](54700a0)) * add option to manually sync users ([#53](#53)) ([3ec1b19](3ec1b19)) * display membership plans from the network ([#59](#59)) ([6fa01fd](6fa01fd)) * enhance network subscriptions and orders search ([#55](#55)) ([bcb0615](bcb0615)) * Node connection using a link ([#58](#58)) ([721f8b2](721f8b2)) * **wp-cli:** add command to backfill events ([#51](#51)) ([13d2498](13d2498)) * **wp-cli:** add process pending webhook requests command ([#67](#67)) ([7dbd8dc](7dbd8dc)) * **wp-cli:** sync all events from the Hub ([#65](#65)) ([dc595ca](dc595ca))

matticbot · 2024-02-28T13:09:49Z

🎉 This PR is included in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

adekbadek force-pushed the feat/events-backfill-cli branch from 3505a9e to bc295f3 Compare February 16, 2024 09:52

adekbadek and others added 2 commits February 16, 2024 13:20

feat(backfill): WP CLI script to backfill reader_registered events

fc7dfeb

feat: process donation_new events

188ff65

adekbadek force-pushed the feat/events-backfill-cli branch from bc295f3 to 188ff65 Compare February 16, 2024 13:44

adekbadek mentioned this pull request Feb 16, 2024

Feat/tweaks for network update Automattic/newspack-plugin#2935

Merged

6 tasks

adekbadek changed the title ~~feat(backfill): WP CLI script to backfill reader_registered events~~ feat(backfill): WP CLI script to backfill reader_registered, donation_new events Feb 16, 2024

adekbadek added 2 commits February 16, 2024 21:15

feat: prevent duplicate events in the event log

4f03b8f

test: update phpunit-polyfillls

ecf40bf

adekbadek force-pushed the feat/events-backfill-cli branch from 00da47a to ecf40bf Compare February 16, 2024 20:15

feat: handle all actions

e575930

adekbadek marked this pull request as ready for review February 16, 2024 20:22

adekbadek requested a review from a team as a code owner February 16, 2024 20:22

adekbadek added the [Status] Needs Review label Feb 16, 2024

adekbadek added 2 commits February 19, 2024 13:40

feat: handle donation_subscription_cancelled events backfill

2233b7e

feat: better --verbose flag handling

b74a619

leogermani requested changes Feb 19, 2024

View reviewed changes

adekbadek added 2 commits February 21, 2024 14:18

feat: only query for local users

111a7b4

feat(backfill-cli): handle membership events

baa982c

leogermani requested changes Feb 21, 2024

View reviewed changes

feat: ignore non-donation subscription cancellations

0153127

adekbadek requested a review from leogermani February 22, 2024 09:54

leogermani requested changes Feb 22, 2024

View reviewed changes

adekbadek added 2 commits February 22, 2024 15:42

fix: fixes

21648e6

fix: 'all' variant success message

f9cc61e

adekbadek requested a review from leogermani February 22, 2024 14:49

leogermani approved these changes Feb 22, 2024

View reviewed changes

leogermani added [Status] Approved and removed [Status] Needs Review labels Feb 22, 2024

leogermani added 10 commits February 22, 2024 17:05

Merge branch 'feat/events-backfill-cli' into feat/backfill-memberships

5a0074a

chore: refactor backfillers

d0ea9f8

Merge branch 'trunk' into feat/events-backfill-cli

eea0813

Merge branch 'feat/events-backfill-cli' into feat/backfill-memberships

18f9898

Merge branch 'feat/backfill-memberships' into refactor/backfillers

9b9ccdf

feat: add membership backfiller

9efe435

fix: check for verbose

7bfcbb8

fix: typos

3be8b84

fix: exclude external memberships

17f1db6

fix: avoid actions to trigger user updates

11c39cf

leogermani mentioned this pull request Feb 22, 2024

Refactor/backfillers #68

Merged

leogermani added 2 commits February 22, 2024 19:32

Merge pull request #68 from Automattic/refactor/backfillers

bf6f43b

Refactor/backfillers

Merge pull request #62 from Automattic/feat/backfill-memberships

210ceb2

feat(backfill-cli): handle membership events

leogermani and others added 3 commits February 22, 2024 19:38

chore: remove unused param

03b66fb

chore: tweaks

ec040cb

fix: get donation orders in the donation_new backfill

1543a59

adekbadek merged commit 13d2498 into trunk Feb 23, 2024
3 checks passed

adekbadek deleted the feat/events-backfill-cli branch February 23, 2024 08:57

matticbot added the released on @alpha label Feb 23, 2024

matticbot added the released label Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backfill): WP CLI script to backfill reader_registered, donation_new events #51

feat(backfill): WP CLI script to backfill reader_registered, donation_new events #51

adekbadek commented Feb 14, 2024 •

edited

Loading

leogermani left a comment •

edited

Loading

leogermani Feb 19, 2024

adekbadek Feb 21, 2024

leogermani Feb 19, 2024

adekbadek commented Feb 21, 2024 •

edited

Loading

leogermani Feb 21, 2024

adekbadek Feb 22, 2024

leogermani Feb 21, 2024

adekbadek Feb 22, 2024

leogermani commented Feb 22, 2024

leogermani left a comment

leogermani commented Feb 22, 2024

adekbadek commented Feb 22, 2024

leogermani commented Feb 22, 2024

matticbot commented Feb 23, 2024

matticbot commented Feb 28, 2024

	$class_name = 'Newspack_Network\\Hub\\Stores\\Event_Log_Items\\' . Accepted_Actions::ACTIONS[ $item->action_name ];
	if ( ! class_exists( $class_name ) ) {
	$class_name = 'Newspack_Network\\Hub\\Stores\\Event_Log_Items\\Generic';
	}
	$results[] = new $class_name(

feat(backfill): WP CLI script to backfill reader_registered, donation_new events #51

feat(backfill): WP CLI script to backfill reader_registered, donation_new events #51

Conversation

adekbadek commented Feb 14, 2024 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

leogermani left a comment • edited Loading

Choose a reason for hiding this comment

leogermani Feb 19, 2024

Choose a reason for hiding this comment

adekbadek Feb 21, 2024

Choose a reason for hiding this comment

leogermani Feb 19, 2024

Choose a reason for hiding this comment

adekbadek commented Feb 21, 2024 • edited Loading

leogermani Feb 21, 2024

Choose a reason for hiding this comment

adekbadek Feb 22, 2024

Choose a reason for hiding this comment

leogermani Feb 21, 2024

Choose a reason for hiding this comment

adekbadek Feb 22, 2024

Choose a reason for hiding this comment

leogermani commented Feb 22, 2024

leogermani left a comment

Choose a reason for hiding this comment

leogermani commented Feb 22, 2024

adekbadek commented Feb 22, 2024

leogermani commented Feb 22, 2024

matticbot commented Feb 23, 2024

matticbot commented Feb 28, 2024

adekbadek commented Feb 14, 2024 •

edited

Loading

leogermani left a comment •

edited

Loading

adekbadek commented Feb 21, 2024 •

edited

Loading