Skip to content

feat(my-account): sync email change with esp#3780

Merged
chickenn00dle merged 4 commits into
trunkfrom
feat/sync-email-change-with-esps
Mar 4, 2025
Merged

feat(my-account): sync email change with esp#3780
chickenn00dle merged 4 commits into
trunkfrom
feat/sync-email-change-with-esps

Conversation

@chickenn00dle

@chickenn00dle chickenn00dle commented Feb 24, 2025

Copy link
Copy Markdown
Contributor

All Submissions:

Changes proposed in this Pull Request:

Part of https://app.asana.com/0/1208993180326452/1209215513701719

This and the accompanying newsletters PR (Automattic/newspack-newsletters#1764) trigger email change updates in the connected ESP after an email change is verified.

How to test the changes in this Pull Request:

You will need to do three rounds of testing, one for MailChimp, one for Active Campaign, and one for Constant Contact

  1. Make sure the NEWSPACK_EMAIL_CHANGE_ENABLED is added and true in wp-config.
  2. With this and the newsletters PR checked out, ensure your test site is connected to the ESP
  3. Log into a reader account that exists in the ESP
  4. Go to my account, update the email address, and save changes
  5. Check the verification email and click the button to verify
  6. Ensure you are redirected to my account with a success message and the new email address showing
  7. Log into the ESP dashboard, and search for the new contact by email
  8. Also verify the old contact email is no longer present
  9. For mailchimp, check for a contact note indicating the contact was migrated, and the old contact email is now archived

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 chickenn00dle marked this pull request as ready for review February 27, 2025 22:15
@chickenn00dle chickenn00dle requested a review from a team as a code owner February 27, 2025 22:15
@chickenn00dle chickenn00dle added the [Status] Needs Review The issue or pull request needs to be reviewed label Feb 28, 2025

@adekbadek adekbadek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The happy-paths work, left some notes. I did test with AC and MC, but had trouble accessing CC (🤦) -> I'll test there later, hoping this was an intermittent error.

  1. The instructions neglected the step of configuring the NEWSPACK_EMAIL_CHANGE_ENABLED environment variable.
  2. WC will require a Display Name to be set, so updating the email address only won't be possible:
image
  1. While the address change is initiated, but not confirmed, the email input in My Account displays the new email address. Is that intentional? IMO the old address should be displayed, with a notice saying that a change request was initiated.
  2. An accidental edge-case: initiate the change, log out and in, click the verification link -> the link does not work ("Something went wrong" error notice), with no recourse because the email address field is disabled. Not sure if it's worth examining, but raising for completeness.

Comment thread includes/reader-revenue/my-account/class-woocommerce-my-account.php Outdated
@chickenn00dle chickenn00dle force-pushed the feat/sync-email-change-with-esps branch from e312953 to 1a9b190 Compare March 3, 2025 15:53
@chickenn00dle

Copy link
Copy Markdown
Contributor Author

Thanks for the review @adekbadek!

The instructions neglected the step of configuring the NEWSPACK_EMAIL_CHANGE_ENABLED environment variable.

Sorry about that! Derrick and Miguel had been reviewing the PRs associated with this feature, so I stopped including that step in the description. I should have remembered you would be coming back and kept it though. I've re-added.

WC will require a Display Name to be set, so updating the email address only won't be possible

Ah. Good catch. Once a display name is set, you will be able to update the email address only, but when its not you will have to set both. I'm not sure we should allow this to be empty though, so not sure we should "fix" this as Woo seems to expect this to have a value when this form is submitted. In any case, I will open another task to address this since this is unrelated to ESP sync.

While the address change is initiated, but not confirmed, the email input in My Account displays the new email address. Is that intentional? IMO the old address should be displayed, with a notice saying that a change request was initiated.

Good question. This is intentional and outlined in the scope here: https://newspackengineering.wordpress.com/2025/02/10/project-scope-change-email/

An accidental edge-case: initiate the change, log out and in, click the verification link -> the link does not work ("Something went wrong" error notice), with no recourse because the email address field is disabled. Not sure if it's worth examining, but raising for completeness.

This was actually fixed in another PR that was merged this morning. I've rebased so this should be resolved. Let me know if you still see this happening on your end though!

@chickenn00dle

Copy link
Copy Markdown
Contributor Author

FYI, task opened for the display name issue: https://app.asana.com/0/1208993180326452/1209560126738155/f

@github-actions 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 4, 2025
@chickenn00dle chickenn00dle merged commit 983c087 into trunk Mar 4, 2025
@chickenn00dle chickenn00dle deleted the feat/sync-email-change-with-esps branch March 4, 2025 16:01
@github-actions

github-actions Bot commented Mar 4, 2025

Copy link
Copy Markdown

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! ❤️

matticbot pushed a commit that referenced this pull request Mar 6, 2025
# [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

Copy link
Copy Markdown
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Mar 18, 2025
# [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

Copy link
Copy Markdown
Contributor

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

Labels

Has pending E2E PR released on @alpha released [Status] Approved The pull request has been reviewed and is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants