fix(my-account): handle email change esp sync errors by chickenn00dle · Pull Request #3792 · Automattic/newspack-plugin

chickenn00dle · 2025-03-03T19:59:27Z

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:

Closes https://app.asana.com/0/1208993180326452/1209215513701719

This and the partner newsletters PR account for errors when attempting to sync an email change with ESPs.

We account for two types of issues:

The email address we are updating to already exists in the ESP. In these cases, we delete the old contact and trigger an update request to the new contact.
Any other generic error is handled by rescheduling the sync for 24 hours.

How to test the changes in this Pull Request:

Be sure to also checkout Automattic/newspack-newsletters#1769 before testing.

Ensure the NEWSPACK_EMAIL_CHANGE_ENABLED constant is defined and true in wp-config.
Ensure your test site ESP is mailchimp and connected
Log into a reader account that exists in mailchimp and update the email address to a new one altogether
Complete verification then check mailchimp to ensure the email address updated correctly for the contact
Now repeat step 3, but this time update to an email address that exists in the ESP but does not exist in the test site (you may need to manually add the contact in MC).
Complete verification then check mailchimp to ensure the old contact has been removed and the one that you updated to still exists
Now go Engagement > Newsletters and replace the ESP key with something invalid
Repeat step 3 one more time
Complete verification then confirm a new newspack_esp_sync_email_change cron job has been scheduled (wp cron event list)
Fix the ESP key, then manually trigger the cron job (wp cron event run newspack_esp_sync_email_change)
Check mailchimp and verify the email update occurred as expected
Repeat the above steps for both Active Campaign and Constant Contact

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?

chickenn00dle · 2025-03-05T16:44:05Z

-			Logger::error( 'Error syncing email change: ' . $update->get_error_message() );
-			return false;
+			// If the update failed, retry in 24 hours.
+			\wp_schedule_single_event( time() + DAY_IN_SECONDS, self::SYNC_ESP_EMAIL_CHANGE_CRON_HOOK, [ $user_id, $new_email, $old_email ] );


I didn't set a limit to the number of retries here. Since we are handling the case of existing email address within the various provider upsert methods, any other types of errors should be resolvable I think. But let me know if we prefer to set a limit here instead!

miguelpeixe

In the ESP_Sync class we have sync() and schedule_sync() methods. This strategy and the sync_email_change() method are creating a duplicated strategy for managing contact synchronization.

I worry that this creates risks in the long term, which we recently worked on mitigating by centralizing how syncs behave.

Is it possible to leverage the existing methods from the ESP_Sync class for this?

chickenn00dle · 2025-03-06T18:57:57Z

Is it possible to leverage the existing methods from the ESP_Sync class for this?

Unfortunately, the ESP_Sync methods only work with Mailchimp and Active Campaign, but not Constant Contact.

I can probably modify these methods to trigger for constant contact for only this very specific case, but I think its cleaner to handle this logic separately instead and keep the sync methods clean.

Happy to go the other route if we prefer though!

miguelpeixe · 2025-03-06T19:21:16Z

Hmm, good point. Is it unsupported solely because of a missing "master list"?

If that's the case, we'd benefit more from allowing a sync to go through on CC without a master list. Perhaps return -1 here?

If there are other reasons or that's not a move we'd like to make now, we could also move the sync_email_change() as is to the ESP_Sync class. It'd be easier to refactor if we decide to in the future.

leogermani · 2025-03-06T19:22:24Z

I think there's no reason to not support Constant Contact fully in RAS, meaning also adding an option to select a "master list" for it

miguelpeixe

Thank you for revising!

github-actions · 2025-03-06T20:39:43Z

Hey @chickenn00dle, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

# [6.1.0-alpha.1](v6.0.1...v6.1.0-alpha.1) (2025-03-06) ### Bug Fixes * **modal-checkout:** password setup ([8fbfabd](8fbfabd)) * **my-account:** handle email change esp sync errors ([#3792](#3792)) ([3d9f294](3d9f294)) * **my-account:** send change email to old and new emails ([#3786](#3786)) ([710d53d](710d53d)) * **premium-newsletters:** show premium lists in post-checkout signup ([#3788](#3788)) ([ccb1526](ccb1526)) * spacer block handling with registration block ([e9b7beb](e9b7beb)) ### Features * **correction-blocks:** Correction box & Loop item + Template ([#3787](#3787)) ([c215dc2](c215dc2)) * **correction-blocks:** update corrections template ([#3793](#3793)) ([c7aea33](c7aea33)) * **my-account:** add email change cancellation option ([#3778](#3778)) ([600ad61](600ad61)) * **my-account:** sync admin email change with ESP/stripe ([#3799](#3799)) ([7179ffd](7179ffd)) * **my-account:** sync email change with esp ([#3780](#3780)) ([983c087](983c087)) * **my-account:** sync email change with stripe ([#3789](#3789)) ([4f45795](4f45795))

matticbot · 2025-03-06T20:47:41Z

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

# [6.1.0](v6.0.5...v6.1.0) (2025-03-18) ### Bug Fixes * **modal-checkout:** endpoint to refresh newsletter lists via REST ([#3841](#3841)) ([2b294e0](2b294e0)) * **modal-checkout:** password setup ([8fbfabd](8fbfabd)) * **my-account:** handle email change esp sync errors ([#3792](#3792)) ([3d9f294](3d9f294)) * **my-account:** send change email to old and new emails ([#3786](#3786)) ([710d53d](710d53d)) * **premium-newsletters:** show premium lists in post-checkout signup ([#3788](#3788)) ([ccb1526](ccb1526)) * spacer block handling with registration block ([e9b7beb](e9b7beb)) ### Features * **correction-blocks:** Correction box & Loop item + Template ([#3787](#3787)) ([c215dc2](c215dc2)) * **correction-blocks:** update corrections template ([#3793](#3793)) ([c7aea33](c7aea33)) * enable email change for newspack users ([#3824](#3824)) ([9c152a8](9c152a8)) * **my-account:** add email change cancellation option ([#3778](#3778)) ([600ad61](600ad61)) * **my-account:** sync admin email change with ESP/stripe ([#3799](#3799)) ([7179ffd](7179ffd)) * **my-account:** sync email change with esp ([#3780](#3780)) ([983c087](983c087)) * **my-account:** sync email change with stripe ([#3789](#3789)) ([4f45795](4f45795)) * **woo-member-commenting:** optional module for member commenting ([#3783](#3783)) ([90746c8](90746c8)) ### Reverts * Revert "refactor(corrections): remove corrections feature flag (#3797)" (#3825) ([afd01f2](afd01f2)), closes [#3797](#3797) [#3825](#3825)

matticbot · 2025-03-18T18:22:58Z

🎉 This PR is included in version 6.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

chickenn00dle mentioned this pull request Mar 3, 2025

fix: handle email change esp sync errors Automattic/newspack-newsletters#1769

Merged

6 tasks

Base automatically changed from feat/sync-email-change-with-esps to trunk March 4, 2025 16:01

chickenn00dle force-pushed the fix/handle-email-change-esp-sync-errors branch from fb5f8c9 to 94c733c Compare March 4, 2025 16:20

chickenn00dle marked this pull request as ready for review March 4, 2025 16:28

chickenn00dle requested a review from a team as a code owner March 4, 2025 16:28

chickenn00dle force-pushed the fix/handle-email-change-esp-sync-errors branch from 94c733c to ad90c7e Compare March 5, 2025 16:40

chickenn00dle commented Mar 5, 2025

View reviewed changes

chickenn00dle added the [Status] Needs Review The issue or pull request needs to be reviewed label Mar 5, 2025

miguelpeixe requested changes Mar 6, 2025

View reviewed changes

chickenn00dle requested a review from miguelpeixe March 6, 2025 18:58

chickenn00dle added 4 commits March 6, 2025 14:13

fix: include existing contact data when syncing

6d580a0

fix: don't use ESP_Sync method

ef57536

fix(my-account): handle esp email change sync errors

57f880e

fix: increase retry interval

bdd0d3d

fix: move sync method to esp sync class

75c7665

chickenn00dle force-pushed the fix/handle-email-change-esp-sync-errors branch from ad90c7e to 75c7665 Compare March 6, 2025 19:38

miguelpeixe approved these changes Mar 6, 2025

View reviewed changes

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 Mar 6, 2025

fix: use logger namespace

a54571d

chickenn00dle merged commit 3d9f294 into trunk Mar 6, 2025

chickenn00dle deleted the fix/handle-email-change-esp-sync-errors branch March 6, 2025 20:39

github-actions Bot added the needs-changelog label Mar 6, 2025

chickenn00dle removed the needs-changelog label Mar 6, 2025

matticbot added the released on @alpha label Mar 6, 2025

matticbot added the released label Mar 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(my-account): handle email change esp sync errors#3792

fix(my-account): handle email change esp sync errors#3792
chickenn00dle merged 6 commits into
trunkfrom
fix/handle-email-change-esp-sync-errors

chickenn00dle commented Mar 3, 2025 •

edited

Loading

Uh oh!

chickenn00dle Mar 5, 2025

Uh oh!

miguelpeixe left a comment

Uh oh!

chickenn00dle commented Mar 6, 2025

Uh oh!

miguelpeixe commented Mar 6, 2025

Uh oh!

leogermani commented Mar 6, 2025

Uh oh!

miguelpeixe left a comment

Uh oh!

github-actions Bot commented Mar 6, 2025

Uh oh!

matticbot commented Mar 6, 2025

Uh oh!

matticbot commented Mar 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

chickenn00dle commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

Uh oh!

chickenn00dle Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

miguelpeixe left a comment

Choose a reason for hiding this comment

Uh oh!

chickenn00dle commented Mar 6, 2025

Uh oh!

miguelpeixe commented Mar 6, 2025

Uh oh!

leogermani commented Mar 6, 2025

Uh oh!

miguelpeixe left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 6, 2025

Uh oh!

matticbot commented Mar 6, 2025

Uh oh!

matticbot commented Mar 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chickenn00dle commented Mar 3, 2025 •

edited

Loading