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

fix(sync): use newspack_esp_sync_contact filter for the network_registration_site field #132

Merged
merged 5 commits into from
Sep 4, 2024

Conversation

miguelpeixe
Copy link
Member

@miguelpeixe miguelpeixe commented Aug 30, 2024

Automattic/newspack-plugin#3359 introduced a new newspack_esp_sync_contact filter, which should be used for any contact data manipulation during a sync.

This change also removes an unnecessary hook that had no effect.

How to test

  1. Make sure your Hub and Node sites are able to sync contact:
  • Enable RAS
  • Enable ESP Sync in Newspack > Engagement > Advanced
  • Make sure to select a Master list
  • Make sure the "Network Registration Site" field is enabled
  • Make sure testing sites have define( 'NEWSPACK_ALLOW_READER_SYNC', true );
  • Make sure the main plugin is running latest trunk
  • Also make sure you have define( 'NEWSPACK_LOG_LEVEL', 3); to be able to see ESP sync logs
  1. Watch the logs tail -f /tmp/2024-mm-dd--newspack_esp_sync

  2. Visit the Node site as an anonymous visitor and register

  3. Confirm you see a RAS Reader registration log entry and the data includes the "Registration Site" with the Node url

  4. Wait for the event to propagate to the Hub, (or run wp newspack-network process-webhooks)

  5. Confirm you now see a RAS Newspack Network: User propagated from another site in the network. in the Hub's logs, and that the data includes the "Registration Site" with the Node url

  6. Now as an anonymous reader visit the Hub and register

  7. Make sure you see the RAS Reader registration event on the Hub's log, with "Registration Site"

  8. Go to the Node and Pull the latest events in Newspack Network > Node Settings

  9. Confirrm you see the RAS Newspack Network: User propagated from another site in the network. in the Node's logs, with the "Registration Site" filled

Co-authored-by: leogermani <leogermani@automattic.com>
@miguelpeixe
Copy link
Member Author

@leogermani Do you want to take over this PR to look at the newspack_register_reader_metadata hook or is it safe to just remove it?

@leogermani
Copy link
Contributor

Yes I want to test it in more depth

@leogermani
Copy link
Contributor

@miguelpeixe I've tested and successfully removed that filter that weren't doing anything.

I've updated the test instructions if you want to have a look

@miguelpeixe
Copy link
Member Author

Tested well for me.

@leogermani leogermani merged commit 3755d07 into trunk Sep 4, 2024
4 checks passed
@leogermani leogermani deleted the fix/esp-sync-contact branch September 4, 2024 20:52
matticbot pushed a commit that referenced this pull request Sep 20, 2024
# [2.2.0-alpha.1](v2.1.0...v2.2.0-alpha.1) (2024-09-20)

### Bug Fixes

* **sync:** use `newspack_esp_sync_contact` filter for the `network_registration_site` field ([#132](#132)) ([3755d07](3755d07))

### Features

* subscriptions and  memberships sync reimplementation ([#122](#122)) ([04fc845](04fc845))
@matticbot
Copy link
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Oct 8, 2024
# [2.2.0](v2.1.0...v2.2.0) (2024-10-08)

### Bug Fixes

* **sync:** use `newspack_esp_sync_contact` filter for the `network_registration_site` field ([#132](#132)) ([3755d07](3755d07))

### Features

* subscriptions and  memberships sync reimplementation ([#122](#122)) ([04fc845](04fc845))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.2.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