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(mailchimp): remove cache and improve logs when preparing merge fields #1644

Closed
wants to merge 3 commits into from

Conversation

miguelpeixe
Copy link
Member

All Submissions:

Changes proposed in this Pull Request:

We should always fetch the most recent merge fields when deciding whether a new merge field should be created, which is not possible behind a cache layer. We're currently having issues with duplicate fields being created due to outdated data coming from cache.

This PR removes the cache layer when fetching merge fields on prepare_merge_fields() and improves logs to benefit from the new logging strategy via the newspack_log action hook.

The base is alpha so we can get this out for the next release.

How to test the changes in this Pull Request:

  1. Make sure you have RAS configured with Mailchimp as your ESP
  2. Edit your Mailchimp audience, delete a basic RAS merge field (NP_Account), and create new random merge fields until it reaches the limit of 30
  3. Sign in as a reader and confirm the following is logged after reader_logged_in is dispatched:
Failed to create merge field NP_Account. Error response: You have exceeded the maximum number of 30 merge fields for this list.
  1. Delete the random merge fields you've just created and manually create two fields named NP_Account
  2. Sign in as the reader again and confirm you get a log similar to the following:
Duplicate merge field NP_Account found with tag MMERGE25.	
  1. Delete the 2 NP_Account merge fields
  2. Sign in as the reader once more and confirm, after reader_logged_in dispatch, that the merge field is created and populated with the user ID

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?

@miguelpeixe
Copy link
Member Author

Closing in favor of #1645

@miguelpeixe miguelpeixe closed this Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant