-
Notifications
You must be signed in to change notification settings - Fork 52
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(nrh): fixes for ESP contact syncing w/ NRH, newsletter signups #3779
base: release
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works accordingly to the testing instructions, but there's an ongoing discussion on the other PR (Automattic/newspack-newsletters#1763 (review)), which should be resolved before this is merged.
@adekbadek @miguelpeixe @leogermani following up from a comment in the other PR, which is no longer needed after 6b571e9. I updated the PR description here to explain what was happening and how this PR fixes it. |
Not sure if there's something wrong on my end, but I was not able to confirm most of the things in the testing instructions. = Testing synced metadata when registering via signup blocks = Both on = Testing contact syncing with NRH = I see the Fatal error going away and the actual sync happening. Also see the shortened list of fields on the admin. But I don't see neither "Registration Page" or the UTM params going through.... :/ Maybe we can debug it together |
@leogermani looks like there was an unrelated bug preventing Signup UTM fields from being set. Fixed in 826df46. Also, Registration Method was not affected by any of these bugs. Removed that from testing instructions
That doesn't match what I see...on this branch, I do see "Registration Page", but not on |
All Submissions:
Changes proposed in this Pull Request:
It's possible to enable Reader Activation contact syncing while also using NRH instead of WooCommerce as a reader revenue platform, but this is currently broken.
Handles ESP contact syncing when using NRH as a reader revenue platform, and/or WooCommerce Subscriptions is not available. WooCommerce itself is still a soft requirement for ESP syncing when using NRH, as we rely on its
WC_Customer
class for fetching basic user meta fields.Also fixes two scenarios where some contact metadata is lost before getting synced:
newspack_newsletters_contact_subscribed
action triggers anewsletter_subscribed
data event that contains the full contact metadata in the payload, but while the handler for this data event triggers a contact sync, it doesn't include the full metadata in the sync. This PR fixes the issue by including all metadata in the sync.Closes
1200550061930446/1209469667970736
.How to test the changes in this Pull Request:
Testing synced metadata when registering via signup blocks
release
, observe that some contact metadata fields are not synced to the ESP (NP_Registration Page, any Signup UTM fields).Testing contact syncing with NRH
release
, as a reader, sign up for newsletters via the Newsletter Subscription Form and Reader Registration blocks on a page that contains UTM parameters in the URL (e.g.?utm_medium=test&utm_campaign=NRH&utm_content=NRH%20content
. Observe that the contact sync to the connected ESP is logged, but without most of the RAS contact metadata fields. Also observe a fatal error in PHP:Fatal error: Uncaught Error: Call to undefined function wcs_get_users_subscriptions()
Other information: