feat: support send lists in ESP classes

[dkoo]

All Submissions:

• Have you followed the Newspack Contributing guideline?
• Does your code follow the WordPress' coding standards and VIP Go coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

To be reviewed after #1629 (the base for this PR).

• Adds get_send_lists() methods to all ESP classes and to the Newspack_Newsletters_ESP_API_Interface schema
• Refactors get_lists() and get_segments() methods for some ESPs to allow for query args (so we only fetch those we need to, where the ESP's API supports filtering by name or IDs)
• Standardizes senderEmail, senderName, send_list_id and send_sublist_id meta fields to store that info in a standard format across all ESPs
• Deprecates and migrates provider-specific meta fields that are no longer needed due to the standardized fields
• Refactors retrieve and sync methods for all ESPs for greater consistency in the newsletterData output (all ESPs will now include lists and sublists keys to store send lists fetched from the ESP)
• Adds caching of Mailchimp audiences in addition to groups/tags/segments

How to test the changes in this Pull Request:

1. In wp shell, try out the new get_send_lists() methods. e.g. for MC:

> $esp = Newspack_Newsletters::get_service_provider_instance( 'mailchimp' )
= Newspack_Newsletters_Mailchimp {#2377
    +service: "mailchimp",
    +name: "Mailchimp",
    +controller: Newspack_Newsletters_Mailchimp_Controller {#2378},
  }

> $esp->get_send_lists()

> $esp->get_send_lists( [ 'type' => 'sublist' ] )

> $esp->get_send_lists( [ 'ids' => [ '123, '456' ] ] ) // where the IDs are actual list or sublist IDs from your connected ESP account

2. For MC, trigger the cache refresh and ensure that audience data is cached:

> wp cron event run newspack_nl_mailchimp_refresh_cache
Executed the cron event 'newspack_nl_mailchimp_refresh_cache' in 1.023s.

> wp option get newspack_nl_mailchimp_cache_lists
array (
  0 =>
  array (
    'id' => '123',
    'web_id' => 456
    'name' => 'My Audience',
    'stats' =>
    array (
      'member_count' => 47,
    ),
  ),
)

3. Check out the new retrieve output:

> $esp = Newspack_Newsletters::get_service_provider_instance( 'mailchimp' )
= Newspack_Newsletters_Mailchimp {#2377
    +service: "mailchimp",
    +name: "Mailchimp",
    +controller: Newspack_Newsletters_Mailchimp_Controller {#2378},
  }

> $data = $esp->retrieve( $post_id ) // where $post_id is a newsletter post

4. Ensure that the retrieve output includes list and sublist params:

> $data['lists']
= [
    Newspack\Newsletters\Send_List {#14951
      +"provider": "mailchimp",
      +"type": "list",
      +"entity_type": "audience",
      +"id": "123",
      +"name": "My Audience",
      +"count": 47,
      +"edit_link": "https://us10.admin.mailchimp.com/audience/contacts/?id=456",
      +"value": "123",
      +"label": "[AUDIENCE] My Audience (47 contacts)",
    },
  ]

> $data['sublists']
= [
    Newspack\Newsletters\Send_List {#14911
      +"provider": "mailchimp",
      +"type": "sublist",
      +"entity_type": "group",
      +"id": "789",
      +"name": "My Group",
      +"count": 15,
      +"edit_link": "https://us10.admin.mailchimp.com/audience/groups/?id=654",
      +"value": "789",
      +"label": "[GROUP] My Group (15 contacts)",
    },
  ]

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?

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1207717004877591

[codecov]

Codecov Report

Attention: Patch coverage is 3.31263% with 934 lines in your changes missing coverage. Please review.

Project coverage is 21.36%. Comparing base (5a44389) to head (896c900).
Report is 1 commits behind head on epic/update-editor-uis.

Files with missing lines	Patch %	Lines
...mailchimp/class-newspack-newsletters-mailchimp.php	0.76%	261 Missing ⚠️
...ct/class-newspack-newsletters-constant-contact.php	6.73%	194 Missing ⚠️
...ign/class-newspack-newsletters-active-campaign.php	3.29%	176 Missing ⚠️
...or/class-newspack-newsletters-campaign-monitor.php	3.47%	139 Missing ⚠️
includes/class-newspack-newsletters.php	0.00%	58 Missing ⚠️
...ass-newspack-newsletters-mailchimp-cached-data.php	5.55%	51 Missing ⚠️
...lass-newspack-newsletters-constant-contact-sdk.php	0.00%	26 Missing ⚠️
includes/class-send-lists.php	0.00%	11 Missing ⚠️
...lass-newspack-newsletters-mailchimp-controller.php	0.00%	5 Missing ⚠️
...wspack-newsletters-constant-contact-controller.php	0.00%	3 Missing ⚠️
... and 6 more

Additional details and impacted files

@@                     Coverage Diff                      @@
##             epic/update-editor-uis    #1631      +/-   ##
============================================================
- Coverage                     21.91%   21.36%   -0.55%     
- Complexity                     2443     2594     +151     
============================================================
  Files                            48       48              
  Lines                          9185     9510     +325     
============================================================
+ Hits                           2013     2032      +19     
- Misses                         7172     7478     +306     

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

[leogermani]

This is monumental!

I think this is the most important and impactful refactor we (you) are doing as part of the current efforts to improve the plugin stability. When we first starting discussion how we should improve the abstraction we were talking precisly about this. We knew it was a lot of work, and you nailed it!

I left one comment regarding the change on how the mailchimp cache is refreshed.

I'm seeing this warning, not 100% sure it is related to this PR:

 PHP Notice:  Function register_rest_route was called <strong>incorrectly</strong>. Namespace must not start or end with a slash.

btw, I see you are removing register_rest_route from the ESP classes, but I missed where they are going. I though I would see them in the parent class but I didn't. We don't need them anymore? Are they being added in a next PR?

Also this:

Cron reschedule event error for hook: newspack_nl_mailchimp_refresh_cache, Error code: invalid_schedule, Error message: Event schedule does not exist., Data: {"schedule":"every_10_minutes","args":[],"interval":600}

At one point, I also saw this error:

PHP Fatal error:  Uncaught TypeError: array_map(): Argument #2 ($array) must be of type array, WP_Error given in /newspack-repos/newspack-newsletters/includes/service-providers/constant_contact/class-newspack-newsletters-constant-contact.php:989

After switching back and forth between providers, it worked (my connection was authorized).

I think what happens is that the CC API is very flaky, so we need to be extra careful there. But maybe some similar error handling in other places where we just throw result of a method into an array_* function is desired.

---

I've read through the changes and ran the tests you suggested, in all ESPs. Changes look good! Maybe a few things passed but I'm assuming we can catch them in a more complete tests when we test the full integration with the front-end.

[leogermani]

Is this really needed?

It was intentional that each list cache should be refreshed in its own request. Even though we run them async, these requests can still timeout. And I know for a fact that in some sites with many Audiences, tags, groups, etc, trying to fetch everything from all lists at once will timeout.

Breaking it in chunks of lists, and refreshing them separate, was to avoid that. This would be a very hard thing to catch if it started failing.

If there isn't any strong reasons why we need this, I think we should leave it as it is - and maybe add a comment with the reasoning so we don't fall in the same trap again in the future.

[dkoo]

Hmm, in trunk it looks like we're fetching all lists and refreshing the cache for all here too.

The only difference in this PR is that we use the cached lists if they haven't expired (get_lists() will fetch if the cache is older than 10 minutes).

[leogermani]

Let me look at the diff with fresh eyes again on Monday. I thought there was a change in behavior that would refresh the cache for all lists in the same request. It it's not that it's fine

[dkoo]

Ah, I see what you mean. In this diff we're introducing caching of list data. But that doesn't mean we fetch and cache all of the list's sublists at the same time. It just means we're caching the lists array from the MC API /lists response. We still iterate through each list object and dispatch a separate refresh request for each list—this is unchanged from trunk.

		foreach ( $lists as $list ) {
			Newspack_Newsletters_Logger::log( 'Mailchimp cache: Dispatching request to refresh cache for list ' . $list['id'] );
			self::dispatch_refresh( $list['id'] );
		}

[dkoo]

I'm seeing this warning, not 100% sure it is related to this PR:

Yep, this should be fixed in b28ca0c.

btw, I see you are removing register_rest_route from the ESP classes, but I missed where they are going. I though I would see them in the parent class but I didn't. We don't need them anymore? Are they being added in a next PR?

These will no longer be needed after the changes in the next PR, because the ESP-specific REST routes to get lists/segments/etc. have been replaced by the Send Lists API, and the ESP-specific routes to set lists/segments/etc. on a campaign will happen as part of the sync request that's dispatched after we update the email HTML upon save—so there won't need to be any separate REST requests to get or set those things.

At one point, I also saw this error:

Yes, good call. e300157 adds error handling for the get_send_lists() methods.

[miguelpeixe]

While testing #1632 I found that this is throwing a fatal error when attempting to send a test email:

[11-Sep-2024 16:20:02 UTC] PHP Fatal error:  Uncaught Error: Call to a member function get_entity_type() on array in newspack-newsletters/includes/service-providers/mailchimp/class-newspack-newsletters-mailchimp.php:972

[miguelpeixe]

The same error occurred while trying to send the email

[miguelpeixe]

Nevermind, this is unrelated to this PR and caused by this change on #1632:

https://github.com/Automattic/newspack-newsletters/pull/1632/files#diff-961258e170b9bfe6e81621bab727a9a65947cd67cc016bf5ce1926dd8c78d77cR707-R713