-
Notifications
You must be signed in to change notification settings - Fork 49
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): strip unsuffixed UTM meta keys from normalized contact data #3623
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dkoo
added
[Status] Needs Review
The issue or pull request needs to be reviewed
Reader Activation
labels
Dec 12, 2024
6 tasks
…for-duplicate-merge-fields
miguelpeixe
approved these changes
Dec 16, 2024
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.
Both CLI commands tested well and the empty UTM is stripped out from the normalized data without any regressions I could find! 🎉
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
Dec 16, 2024
…e-merge-fields fix(ras): add script for fixing duplicate Mailchimp merge fields
matticbot
pushed a commit
that referenced
this pull request
Dec 16, 2024
## [5.10.1](v5.10.0...v5.10.1) (2024-12-16) ### Bug Fixes * **sync:** strip unsuffixed UTM meta keys from normalized contact data ([#3623](#3623)) ([5bbcc7b](5bbcc7b))
🎉 This PR is included in version 5.10.1 🎉 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
Labels
Reader Activation
released
[Status] Approved
The pull request has been reviewed and is ready to merge
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All Submissions:
Changes proposed in this Pull Request:
For ESP metadata data syncing, UTM-related meta keys must have a suffix to denote the particular UTM parameter. These are typically:
source
medium
campaign
term
content
Although some publishers/marketers might create their own non-standard parameters for custom analytics purposes. However, some UTM parameter suffix must be present in order to be synced to the ESP contact. We should never be syncing a meta field like
NP_Payment UTM:
to the ESP. We've seen this happen in the wild at times, resulting in useless unsuffixed meta fields being created in the connected ESP and polluting the publishers' data.This PR safeguards against this issue going forward by stripping unsuffixed UTM-related meta fields from normalized contact data.
Bad fields will not be automatically removed from connected ESP accounts that already have them. To remove them, use the new
wp newspack mailchimp merge-fields delete
CLI command included here.How to test the changes in this Pull Request:
I was unable to replicate the conditions in which unsuffixed UTM fields are added to contact metadata, but you can test by manually adding some using WP shell:
NEWSPACK_FORCE_ALLOW_ESP_SYNC
constant to wp-config.php and ensure that Newspack Subscriptions is in live mode in order to enable ESP contact syncing.release
, start awp shell
session.$contact = Newspack\Reader_Activation\Sync\WooCommerce::get_contact_from_customer( <user ID> );
$contact['metadata'] = array_merge( $contact['metadata'], [ 'NP_Signup UTM: ' => 'foo', 'payment_page_utm' => 'bar' ] );
$normalized = Newspack\Reader_Activation\Sync\Metadata::normalize_contact_data( $contact )
release
, observe that the unsuffixed UTM keys are still present on the normalized contact data. If this data is synced to the ESP usingNewspack\Reader_Activation\ESP_Sync::sync( $contact )
, the unsuffixed keys will be synced as contact meta fields in the ESP.wp shell
session and check out this branch.wp shell
session and repeat steps 3–6 but confirm that the unsuffixed keys are stripped from the contact data bynormalize_contact_data
.NP_Payment UTM
fields are still synced with the URL params.CLI command to delete merge fields
This PR also includes a new CLI command to delete Mailchimp merge fields. This can be used to identify and delete unwanted fields from the connected Mailchimp account.
NP_Signup UTM:
,NP_Payment UTM:
). Also make sure you have or add some suffixed fields too (NP_Signup UTM: campaign
,NP_Payment UTM: content
, etc.).wp newspack mailchimp merge-fields delete --fields=payment_page_utm,signup_page_utm --dry-run
. Confirm that the script reports (but doesn't delete) the matching unsuffixed fields, and does NOT report the suffixed fields:wp newspack mailchimp merge-fields delete --fields=payment_page_utm,signup_page_utm
and confirm that the fields are deleted from Mailchimp.wp newspack esp merge-fields delete --fields=payment_page_utm_content
) and confirm that the matching suffixed field is reported/deleted.Other information: