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: more fixes for Mailchimp RAS data sync #2780

Merged
merged 6 commits into from
Dec 4, 2023

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Nov 28, 2023

All Submissions:

Changes proposed in this Pull Request:

Adds Registration Method and Registration Page metadata to reader_registered data events fired by the auth form. Currently new registrations via this method aren't capturing this info.

How to test the changes in this Pull Request:

  1. On master, visit any page in a new session and click the "Sign In" button in the header, then "I don't have an account". Register a new account via this form.
  2. In the connected ESP, observe that the synced contact doesn't have any NP_Registration Page or NP_Registration Method values.
  3. Check out this branch and fix: detect and ignore duplicate merge fields newspack-newsletters#1370, repeat with a different email address, and confirm that the synced contact has a NP_Registration Page with the URL of the page you were viewing when you clicked "Sign In", and a NP_Registration Method of auth-form.

This PR now also contains an alternative solution for the issue described in Automattic/newspack-newsletters#1365. Please follow testing instructions there to test this solution.

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?

@dkoo dkoo added the [Status] Needs Review The issue or pull request needs to be reviewed label Nov 28, 2023
@dkoo dkoo self-assigned this Nov 28, 2023
@dkoo dkoo requested a review from a team as a code owner November 28, 2023 22:36
Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

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

I'm not sure this is producing the desired data.

The referer is stored as the URL of the page I'm at when registering, I expected it to be the page before.

The current_page_url is stored with the full URL as a path, which is what home_url( $path ) does:

https://example.com/https://example.com/path

The usage of add_query_arg() is also not clear to me. If no value is being added in the first argument, what's the purpose of this function?

@dkoo
Copy link
Contributor Author

dkoo commented Nov 29, 2023

I was mainly copying what the Register block is doing, but let me revisit in case the functions here are producing different results.

@miguelpeixe
Copy link
Member

Strange... In the context of the registration block the referer is expected to consist only of the path (/2023/02/07/accumsan-nam-mauris-erat-ultrices-nascetur-porta-ornare-est-class-cursus-massa-lectus/). I hope that's not broken there 🤔

@miguelpeixe
Copy link
Member

The same code works well for the registration block 🤔

  "data": {
    "user_id": 175,
    "email": "[email protected]",
    "metadata": {
      "lists": [
        "newspack-1128"
      ],
      "referer": "/register/",
      "current_page_url": "https://site.com/register/",
      "registration_method": "registration-block",
      "newspack_popup_id": false
    }
  }

@dkoo
Copy link
Contributor Author

dkoo commented Nov 29, 2023

The difference between the Register block and the auth form is that the latter submits registration request via REST API, where as the Register block more simply POSTs the form data. So 6431780 accounts for this by passing the referrer to the auth form as a hidden form field, and assuming that the result of wp_get_raw_referer() is actually the page the reader is on when registering, because in the context of the REST API the "referrer" is the page that submitted the request.

@dkoo dkoo changed the title fix: add registration method and page when registering via auth form fix: more fixes for Mailchimp RAS data sync Dec 1, 2023
Copy link
Member

@miguelpeixe miguelpeixe left a comment

Choose a reason for hiding this comment

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

🎉

The normalize_contact_data() method is working as intended and the sync is going through without issues through the Mailchimp::put() method.

The NP_Registration Page and NP_Referrer Path fields are also being populated with the expected values.

@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 Dec 1, 2023
@dkoo
Copy link
Contributor Author

dkoo commented Dec 1, 2023

Thanks for the review! Holding for deployment until Monday.

@dkoo dkoo merged commit ef4bfe7 into release Dec 4, 2023
@dkoo dkoo deleted the hotfix/registration-meta-via-auth-form branch December 4, 2023 17:09
matticbot pushed a commit that referenced this pull request Dec 4, 2023
## [2.11.5](v2.11.4...v2.11.5) (2023-12-04)

### Bug Fixes

* more fixes for Mailchimp RAS data sync ([#2780](#2780)) ([ef4bfe7](ef4bfe7))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.11.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Dec 4, 2023
# [2.12.0-alpha.3](v2.12.0-alpha.2...v2.12.0-alpha.3) (2023-12-04)

### Bug Fixes

* more fixes for Mailchimp RAS data sync ([#2780](#2780)) ([ef4bfe7](ef4bfe7))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.12.0-alpha.3 🎉

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
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