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

Assorted changes #94

Merged
merged 3 commits into from
May 9, 2024
Merged

Assorted changes #94

merged 3 commits into from
May 9, 2024

Conversation

adekbadek
Copy link
Member

@adekbadek adekbadek commented Apr 25, 2024

All Submissions:

Changes proposed in this Pull Request:

  1. Changes the environment variable name used to enable the experimental auditing features
  2. Adds ESP contact "Network Registration Site" meta field on network reader creation or fetch (contribution originally by @dkoo committed to the data-integrity-improvements branch)
  3. Adds more data to backfilled users
  4. Fixes membership event timestamp for backfilled memberships

(Note: this is a part of integrating the changes from the data-integrity-improvements branch (#89))

How to test the changes in this Pull Request:

  1. Set NEWSPACK_NETWORK_EXPERIMENTAL_AUDITING_FEATURES environment variable to true, visit Newspack Network -> Nodes and the admin panel and observe there's a "Synchronizable Users" column present
  2. With Mailchimp or ActiveCampaign set as the ESP, register a new reader on a site
  3. In the ESP UI, observe the contact has a custom field "Network Registration Site" with the origin site assigned
  4. Disable this plugin on the Hub site and create a new user (with a first and last name), and a membership
  5. Enable this plugin and run the backfill CLI: wp newspack-network data-backfill all --start=2020-01-01 --end=2024-12-31 --live
  6. Visit the Event Log and observe the user_registered event has the first and last name in the event data
  7. Observe the membership event has the timestamp same as the membership start date

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?

@adekbadek adekbadek requested a review from a team as a code owner April 25, 2024 14:47
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

This is working great! The only issue I see is that when running the backfill script, it's fetching users only who registered on the site you're running the script on (so actual customer or subscriber users, not network_reader users). These users won't have the newspack_remote_site user meta, as this meta is only populated on network_reader users. So we'll need to manually add the network_registration_site meta when syncing to the ESP and assume that the registration site is the current site URL.

@adekbadek
Copy link
Member Author

@dkoo

it's fetching users only who registered on the site you're running the script on […] These users won't have the newspack_remote_site user meta

That's expected, the "original" users never have the newspack_remote_site user meta assigned, because the site is their origin. The backfill script does not change anything in that regard, unless I'm misreading your comment.

So we'll need to […] assume that the registration site is the current site URL.

Precisely! This is how this plugin worked all the time.

@adekbadek adekbadek requested a review from dkoo May 8, 2024 14:00
@dkoo
Copy link
Contributor

dkoo commented May 8, 2024

That's expected, the "original" users never have the newspack_remote_site user meta assigned, because the site is their origin. The backfill script does not change anything in that regard, unless I'm misreading your comment.

Got it, so just to clarify: the backfill script in this PR is only concerned with backfilling registration events for "original" user accounts. So the network_registration_site metadata will be populated on the other backfill process, when we sync contact data for all users (not just original ones, but also network_reader users) to the connected ESP.

@adekbadek
Copy link
Member Author

That's correct. The backfill script in this PR is supposed to be ran on each network site, and then the events will be synchronised.

@adekbadek adekbadek merged commit b177538 into trunk May 9, 2024
3 checks passed
@adekbadek adekbadek deleted the feat/assorted-changes branch May 9, 2024 07:12
@matticbot
Copy link
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.8.0 🎉

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