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: use user email for esp sync purposes #3520

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

leogermani
Copy link
Contributor

@leogermani leogermani commented Nov 4, 2024

All Submissions:

Changes proposed in this Pull Request:

When a user purchases something on a site with RAS activated and its info gets synced to the ESP, we were prioritizing the billing email over the user registered email.

This means that if the user registers with one email, but then uses a different email to make a purchase, it would create a new user in the ESP - but not in WP.

This PR makes sure we always use the user email for sync purposes.

Was there any specific reasons why we wanted to use billing email there? Do we really want this change?

How to test the changes in this Pull Request:

  1. on trunk
  2. Register with a user
  3. Make a donation or purchase something, but change your email in the checkout form
  4. Confirm ESP syncs info about this purchase using the modified email
  5. On this branch, confirm that all ESP syncs are tied to the original user email

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?

@leogermani leogermani added the [Status] Needs Review The issue or pull request needs to be reviewed label Nov 4, 2024
@leogermani leogermani self-assigned this Nov 4, 2024
@leogermani leogermani requested a review from a team as a code owner November 4, 2024 21:27
Copy link
Member

@adekbadek adekbadek left a comment

Choose a reason for hiding this comment

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

Was there any specific reasons why we wanted to use billing email there? Do we really want this change?

I don't think it was intentional, it was rather assumed that the billing email will be the same as the user email.

@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 Nov 8, 2024
@leogermani leogermani merged commit d6925ff into trunk Nov 12, 2024
8 checks passed
@leogermani leogermani deleted the fix/use-user-email-for-esp-sync branch November 12, 2024 15:05
Copy link

Hey @leogermani, 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 it to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[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.

2 participants