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

Assorted error handling fixes #1641

Merged
merged 6 commits into from
Sep 4, 2024

Conversation

adekbadek
Copy link
Member

@adekbadek adekbadek commented Sep 3, 2024

All Submissions:

Changes proposed in this Pull Request:

Adds error handling in a few places to avoid triggering PHP Fatals, and updates the tests so they don't rely on network.

  • Improves handling of a variable. Newspack_Newsletters_Constant_Contact_SDK::get_contact can return false or WP_Error in case of failure, and the upsert_contact method was not handling the latter case.
  • Improves error handling of Mailchimp usage reports, if the API key is not provided
  • Updates the test_render_embed_blocks test not to rely on the network
  • Improves error handling in ActiveCampaign usage reports, in case of a network error
  • Fixes the failed CI builds (perf: remove CodeMirror/CSSLint to reduce bundle size & speed #1642)

Submitting as a hotfix because the lack of error handling triggers PHP Fatals, which pollute our logs.

How to test the changes in this Pull Request:

  • Smoke test, this should be self-explanatory.
  • Observe all tests are passing

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?

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes missing coverage. Please review.

Project coverage is 19.65%. Comparing base (b245716) to head (94c3748).
Report is 71 commits behind head on release.

Files with missing lines Patch % Lines
...pack-newsletters-active-campaign-usage-reports.php 66.66% 1 Missing ⚠️
...lass-newspack-newsletters-constant-contact-sdk.php 0.00% 1 Missing ⚠️
...s-newspack-newsletters-mailchimp-usage-reports.php 50.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             release    #1641      +/-   ##
=============================================
+ Coverage      17.74%   19.65%   +1.90%     
- Complexity      2316     2375      +59     
=============================================
  Files             43       45       +2     
  Lines           8758     8930     +172     
=============================================
+ Hits            1554     1755     +201     
+ Misses          7204     7175      -29     

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

@adekbadek adekbadek force-pushed the hotfix/constant-contact-error-contact branch from 36de778 to f3abbf1 Compare September 3, 2024 08:38
@adekbadek adekbadek changed the title fix(constant-contact): handle contact retrieval error Assorted error handling fixes Sep 3, 2024
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Nice.

I also see these errors when running tests. Not introduced by this PR, just sharing because it seems related

wp_die() called
Message: Error retrieving Mailchimp campaign.
Title: WordPress › Error
Args:
	response: 500
	code: wp_die
	exit: 1
	back_link: 
	link_url: 
	link_text: 
	text_direction: ltr
	charset: UTF-8
	additional_errors: array (
)
	newspack_newsletters_mailchimp_error: 

@github-actions github-actions bot added [Status] Approved Ready to merge and removed [Status] Needs Review labels Sep 3, 2024
@dkoo
Copy link
Contributor

dkoo commented Sep 3, 2024

@adekbadek @leogermani please look at #1642 to see if this fixes the timeouts we've been seeing on the release and build-distributable jobs.

@adekbadek adekbadek merged commit 791fdc3 into release Sep 4, 2024
9 checks passed
@adekbadek adekbadek deleted the hotfix/constant-contact-error-contact branch September 4, 2024 07:57
@adekbadek
Copy link
Member Author

@leogermani – the tests issue should also be resolved by this PR. In the last few failed CI test-php jobs, the offending test was test_render_embed_blocks, which is fixed here.

matticbot pushed a commit that referenced this pull request Sep 4, 2024
## [3.1.1](v3.1.0...v3.1.1) (2024-09-04)

### Bug Fixes

* error handling, tests, CI builds ([#1641](#1641), [#1642](#1642)) ([791fdc3](791fdc3))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.1.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Sep 4, 2024
# [3.2.0-alpha.2](v3.2.0-alpha.1...v3.2.0-alpha.2) (2024-09-04)

### Bug Fixes

* error handling, tests, CI builds ([#1641](#1641), [#1642](#1642)) ([791fdc3](791fdc3))
* handle missing Mailchimp API key ([83a0d6f](83a0d6f))
* **phpcs:** specify path in custom ruleset ref ([#1637](#1637)) ([28f5b50](28f5b50))

### Reverts

* "chore(deps-dev): bump @wordpress/browserslist-config from 6.5.0 to 6.6.0" ([f08f271](f08f271))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.2.0-alpha.2 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants