fix: improve error handling in upsert contact method#2031
Conversation
| $error = new WP_Error( | ||
| 'newspack_newsletters_upsert_contact_error', | ||
| // Get a reader-friendly error message to show to the user. | ||
| $provider->get_reader_error_message( |
There was a problem hiding this comment.
Something tricky, but we don't want this friendly error message in the sync logs. We want the raw error message... The same one that goes into newspack_log a few lines above.
This get_reader_error_message exists for a reason.
For example, when an email is in compliance mode in Mailchimp, it returns this error message. But this was confusing and not helpful for readers, so we changed that in Mailchimp (see this method in the mailchimp provider). But in our logs, we get the raw error...
So that's the idea: a simpler error message for the reader, but the raw error in the logs for the publisher...
There was a problem hiding this comment.
Yeah... This is a somewhat hacky solution. The better alternative is a bigger refactor, which is to not return reader-facing message in the upsert() method. It's too low-level to worry about that.
I can repurpose this PR. Nothing changes in Automattic/newspack-plugin#4488 though
There was a problem hiding this comment.
actually maybe this is not hacky. And what we should do is to filter the error message for a better one here. That is too low level to replace error messages with more human readable ones... we should do this just before surfacing the messages to readers...
There was a problem hiding this comment.
I meant that my solution is somewhat hacky, because even though we should be sending "all errors", to make the reader-facing message one of those errors feels like an anti-pattern.
Yes, I agree that this is not the right place for the reader-facing message. We should move this closer to the UI methods.
There was a problem hiding this comment.
Fixed in 6fabbee. The upsert error response should no longer include the reader-facing message. This has moved to the relevant UI methods.
I looked through other upsert calls in newspack-plugin and they are all backend and sync related.
github-actions
bot
added
[Status] Approved
|
Hey @miguelpeixe, good job getting this PR merged! 🎉 Now, the 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! ❤️ |
# [3.29.0-alpha.1](v3.28.6...v3.29.0-alpha.1) (2026-03-05) ### Bug Fixes * iframe editor compatibility ([#2025](#2025)) ([cd15c02](cd15c02)) * improve error handling in upsert contact method ([#2031](#2031)) ([89a5937](89a5937)) * no parallel post-release jobs ([#2037](#2037)) ([8e4a738](8e4a738)) ### Features * **init-modal:** apply new modal style to init screen in editor ([#2041](#2041)) ([3caafdc](3caafdc))
|
🎉 This PR is included in version 3.29.0-alpha.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
All Submissions:
Changes proposed in this Pull Request:
Refactor the reader-friendly error logic so that the response from the
upsert()method still contains meaningful information about what happened.Further details and test instructions at Automattic/newspack-plugin#4488
How to test the changes in this Pull Request:
Follow the test instructions from #1754 and confirm there are no regressions.
Other information: