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: bugs from Newsletters editor data refactor #1671

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Oct 14, 2024

All Submissions:

Changes proposed in this Pull Request:

This PR fixes several bugs in the Newsletters editor data refactor done in #1658 that became more apparent now that we've been able to test with various publisher accounts and real data.

How to test the changes in this Pull Request:

Handling for large contact counts in Send List summary (9d95c3c)

In the send list summaries for selected lists/sublists, we use Number.toLocaleString() to show large numbers in a localized string format for human readability, e.g. 10,000 contacts instead of 10000 contacts. However, we're erroneously using the %d placeholder in the summary string, which expects a true number instead of a stringified number, so these large numbers aren't getting populated as intended. This PR fixes it by using %s instead.

  1. On trunk, using Mailchimp, temporarily change this line to hardcode a very large number such as 1000000 for the count value
  2. Create or edit a newsletter
  3. Select an audience/list and save the draft
  4. Observe that the summary shown for the selected audience shows %d instead of the contact count number, and a JS console error [sprintf] expecting number but found string:
Screenshot 2024-10-14 at 5 42 39 PM Screenshot 2024-10-14 at 4 59 23 PM
  1. Check out this branch, refresh the editor, and confirm that the number is shown as a localized string like 1,000,000 contacts
Screenshot 2024-10-14 at 5 55 12 PM

Handling for Mailchimp accounts with multiple audiences (60ee0f3 and f59cece)

When fetching sublist info in the editor, we retain all previously fetched sublists in the Redux store and add newly fetched sublists to the store—this way, on subsequent searches or sidebar component mounts, we don't have to go all the way to the REST API again to rebuild this info on the store. However, for all ESPs, sublists are unique to a top-level list, so if you select a new top-level list and then interact with the sublist field, we shouldn't expect to see info for sublists that were previously fetched for the old list. This PR ensures that when selecting a top-level list, we empty out all existing sublist info in the store and post meta. It also avoids an infinite loop if you select an audience that has no tags/groups/segments at all.

  1. Connect your test site to a Mailchimp account with multiple audiences and many sublist entities (DM me if you need access to one)
  2. On trunk, create or edit a newsletter and select an audience. Confirm that the sublist suggestions shown in the sublist autocomplete field match the selected audience.
  3. Change the audience to a different audience. This time, observe that the sublist suggestions shown in the sublist autocomplete field DO NOT match the newly selected audience unless you save and refresh the editor page first.
  4. Check out this branch and repeat steps 2-3, and confirm that the sublist suggestions in the autocomplete field update to match the selected audience whenever you select a new audience.

Handling for Mailchimp accounts with many sublist entities (more than 10 per audience) (35d1aba)

Unique to Mailchimp, when setting the sublist for a campaign via their API, the shape of the payload we pass depends on the type of sublist we're setting (tag, segment, or group). Since the sublist is saved to the post only as an ID, we need to fetch info about the sublist to know what the payload should look like, so we use $provider->get_send_lists() to fetch the sublist's info by ID. However, this will only look for cached sublist info, and if we've previously only fetched a limited number of sublists to the cache (as might happen if you load up the editor after the previous cache has been invalidated, and we prefetch only 10 segments in the first fetch), the info we're looking for might not yet be in the cache. This PR fixes this scenario by ensuring we fetch info for up to 1000 sublists from the Mailchimp API when we go to build the sync payload.

  1. Connect your test site to a Mailchimp account with multiple audiences and many sublist entities (DM me if you need access to one)
  2. On trunk, create or edit a newsletter and select an audience.
  3. Select a sublist that's shown towards the bottom of the list of Groups or Tags in the Mailchimp account > Audience dashboard page.
  4. Save the newsletter with the selected audience + sublist and observe that a.) the sync fails (campaign data in MC is not updated to match the saved info) and b.) the PHP debug log shows a warning: PHP Warning: Undefined array key 0 in /srv/htdocs/wp-content/plugins/newspack-newsletters/includes/service-providers/mailchimp/class-newspack-newsletters-mailchimp.php on line 1054
  5. Check out this branch, repeat steps 2-4, and confirm that the sync succeeds and the PHP warning doesn't happen

Overzealous overwriting by layout campaign defaults (19ce5c8)

When setting a newsletter post to a custom newsletter layout with saved sender and send-to info (either on new post creation or by resetting the layout via the Layout sidebar), we expect the defaults stored in the layout to overwrite whatever sender/send-to meta the post has already. However, after the layout has been set, we don't want the layout defaults to overwrite anything anymore. This PR fixes things by ensuring that layout defaults are only taken into account immediately after selecting a new layout, and then cleared from the post upon the next manual save.

  1. On trunk, using any ESP, create a new layout from a post with saved sender and send-to info.
  2. Create a new newsletter post using this layout, or reset an existing post to this layout. Confirm that the sender/send-to info are preset to the layout defaults.
  3. Change the sender and send-to info for this post to something different from the layout defaults.
  4. Save the post and then refresh the editor. Observe that the sender and send-to info has reverted to the layout defaults instead of what you most recently saved.
  5. Check out this branch and repeat steps 2-4, but this time confirm that the sender and send-to info that you updated in step 3 persist after saving and refreshing the post.

Migrating legacy meta for AC (9aff88c)

We went through a lot of iterations of newsletter data schema and handling of "legacy" meta fields. Along the way it looks like this handling broke for ActiveCampaign lists and sublists. This PR fixes things for AC and also ensures that send list/sublist properties in the Redux store are always named the same thing across all ESPs.

  1. On the latest production release, using ActiveCampaign, create a newsletter post and set a send list and segment, then save. (You'll know you're using the right version if the list and segment UIs are still dropdowns instead of autocomplete fields.)
  2. Check out trunk and refresh this newsletter post in the editor. Observe that the send list and segment are not populated.
  3. Check out this branch and refresh once again. Confirm that send list and segment are populated with the correct selections from step 1.

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 self-assigned this Oct 14, 2024
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

Attention: Patch coverage is 20.68966% with 23 lines in your changes missing coverage. Please review.

Project coverage is 20.90%. Comparing base (0ae5cc5) to head (f59cece).

Files with missing lines Patch % Lines
...ign/class-newspack-newsletters-active-campaign.php 0.00% 10 Missing ⚠️
...mailchimp/class-newspack-newsletters-mailchimp.php 0.00% 8 Missing ⚠️
...or/class-newspack-newsletters-campaign-monitor.php 0.00% 4 Missing ⚠️
...ass-newspack-newsletters-mailchimp-cached-data.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk    #1671      +/-   ##
============================================
+ Coverage     20.85%   20.90%   +0.04%     
- Complexity     2658     2661       +3     
============================================
  Files            48       48              
  Lines          9776     9782       +6     
============================================
+ Hits           2039     2045       +6     
  Misses         7737     7737              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dkoo dkoo marked this pull request as ready for review October 15, 2024 23:36
@dkoo dkoo requested a review from a team as a code owner October 15, 2024 23:36
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