fix(mailchimp): fatal error with invalid API keys

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

Fixes a fatal error when trying to fetch audience/list data using an invalid Mailchimp API key. The fatal was happening because our Mailchimp integration's API response validation logic only considered status codes of 400 or 404 to be errors, but an invalid API key error is returned with a status code of 401, and unhandled WP errors are returned with status codes of 500. So this PR will handle any response with a status code of 400 or greater as an error.

How to test the changes in this Pull Request:

1. On trunk, connect your site to Mailchimp using a valid API key.
2. In the Mailchimp dashboard, revoke that API key.
3. Using WP CLI, trigger the cron job to refresh the Mailchimp cache (or just wait >20 minutes): wp cron event run newspack_nl_mailchimp_refresh_cache
4. Visit Newspack > Engagement and Newsletters > Settings admin pages and observe fatal errors in each.
5. Check out this branch and refresh both admin pages, and confirm no fatal error and a more helpful error message shown in both places:

6. Confirm that you can update the API key to a new valid key and that saving persists the key and restores the connection to the ESP.

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

[adekbadek]

I was not able to complete the testing steps until I've emptied the cache (newspack_nl_mailchimp_cache_lists option). With the wrong key, the list retrieval doesn't update the lists and fails silently, keeping the cached lists.

[dkoo]

I was not able to complete the testing steps until I've emptied the cache (newspack_nl_mailchimp_cache_lists option). With the wrong key, the list retrieval doesn't update the lists and fails silently, keeping the cached lists.

@adekbadek aha, you're correct. 41db9b6 should fix this by returning a proper error in this scenario. So now, if you run wp cron event run newspack_nl_mailchimp_refresh_cache or wait 20 minutes for the cache to refresh, the cached data will no longer be returned if we fail to refresh the cache, and the error message should appear in the UIs.

[leogermani]

I was not able to complete the testing steps until I've emptied the cache (newspack_nl_mailchimp_cache_lists option). With the wrong key, the list retrieval doesn't update the lists and fails silently, keeping the cached lists.

@adekbadek aha, you're correct. 41db9b6 should fix this by returning a proper error in this scenario. So now, if you run wp cron event run newspack_nl_mailchimp_refresh_cache or wait 20 minutes for the cache to refresh, the cached data will no longer be returned if we fail to refresh the cache, and the error message should appear in the UIs.

Wait! That's by design!

From the docs:

If the cache refresh fails, we will store the error in a separate option, and will only surface it to the user after 20 minutes.
In every admin page we will display a generic Warning message, telling the user to go to Newsletters > Settings to see the errors.
In Newsletters > Settings we will output the errors details.

Maybe what we can do is clear the cache if the API key changes

[codecov]

Codecov Report

Attention: Patch coverage is 4.00000% with 24 lines in your changes missing coverage. Please review.

Project coverage is 20.35%. Comparing base (44c1b12) to head (017efcc).
Report is 40 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...ass-newspack-newsletters-mailchimp-cached-data.php	4.54%	21 Missing ⚠️
includes/class-newspack-newsletters-settings.php	0.00%	2 Missing ⚠️
...mailchimp/class-newspack-newsletters-mailchimp.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk    #1690      +/-   ##
============================================
- Coverage     20.38%   20.35%   -0.04%     
- Complexity     2664     2670       +6     
============================================
  Files            48       48              
  Lines         10619    10640      +21     
============================================
+ Hits           2165     2166       +1     
- Misses         8454     8474      +20     

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

[dkoo]

Thanks for clarifying, @leogermani! I'll spin up another commit with that change

[dkoo]

@leogermani 84044fc and 6d7602e implement the cache clear on API key change, and also add an error message to be shown when the cron job to refresh the cache fails. This will result in the following error being shown in both the Newsletters > Settings and Engagement > Newsletters pages 20 minutes after the cache refresh fails:

It will also avoid a fatal error in the newsletter editor. Instead, errors will be shown at the top of the editor and in the Newsletter Campaign sidebar:

Testing instructions remain the same.

[leogermani]

Having an optional parameter before a required paramater throws a PHP deprecated error

Suggested change

	private static function maybe_add_error( $list_id = null, $error ) {
	private static function maybe_add_error( $list_id, $error ) {

[dkoo]

Ah, $list_id needs to be optional because it's null if we're adding an error about refreshing the cache for audience data. 017efcc should fix it by making $error also optional.

[leogermani]

All looks good to me. The addition to clear the cache on api key change is a good one. ( I left one minor suggestion )

There is just one problem not related to this PR.

I think we need to revisit what we did here (cc @adekbadek):

I've changed the api key, invalidated the cache, and then added an invalid key.

I then triggered the cron job to refresh the cache but I never saw an error message being stored and displayed (I modified the code to skip that time before showing errors)

The reason was this.

With invalid API keys, fetch_lists simply returns an empty array and an error is never triggered.

If you look at the code in that PR, all the time after instantiating the api, we call the validate method, which throws in case of errors. That's why these methods are always invoked in a try catch statement. We should let the API instantiation throw as well, otherwise it won't trigger errors when it should

Update: Here's a PR to fix this -> #1698

[dkoo]

Thanks, @leogermani! Originally this PR tried to avoid returning an empty array, but that meant that a workflow-blocking error would be thrown for temporary Mailchimp API hiccups, too. Glad you figured out a less obtrusive solution in #1698.

changes applied

[github-actions]

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

Thank you! ❤️

[matticbot]

🎉 This PR is included in version 3.5.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[matticbot]

🎉 This PR is included in version 3.5.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀