Catch possible fatal when sending notification emails and in that case remove from the queue the item that produced it. Also don't try to display basic notifications that would produce fatals.#2253
Merged
Conversation
Contributor
Author
|
@thomasplevy your opinion on this? |
d4bab78 to
6dcfabd
Compare
Contributor
|
smart strategy, should be good I think |
Contributor
Author
|
@gocodebox/engineering
^ done |
c999937 to
5dda903
Compare
5dda903 to
af24bad
Compare
4 tasks
04a7f15 to
1a6b396
Compare
…in that case remove from the queue the item that produced it
…on status to 'error' when an error occurs while preparing the email to be sent
…tion and set its status to error so that it'll be excluded from the next fetches.
1a6b396 to
51f8182
Compare
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2302
(Doesn't fix #2249 because that issue it's not related to the notification email errors.)
Although in that issue there's an example of error reporting of a notification email breaking, in this case because the order has been removed.
When this happens, since the notification emails sending is handled via a background process (in a batches queue), any unhandled fatal would just stuck the queue, and every time the queue needs to be processed the same fatal would occur and so on.
The idea of this PR is just to catch the fatal, log the error with the proper log handler, and remove the item from the queue, and move on.
For the user in #2249, the next time the queue will be run, the blocking item will be removed and notification emails will start to be sent. Is this good? They might be pretty old notifications...
I've implemented this because we really have to avoid to block that queue for whatever happened in one notification, because an error while sending a notification to a user blocks notifications to be sent to N other users.
On the other hand basic notifications will still break, so we have to really have more/better checks in the merge codes processing.I'm extending the idea to the basic notifications too.
I'm setting these notification status to
error. The downside of this is that even if you fix the error they wont' be displayed/sent anymore. Not a big deal I think, but maybe we can create a tool (ahah another one? yeah) that tries to load at least the errored basic notifications, and if they don't error anymore at least change their status so that they can show up in the user's dashboard.But these are edge cases, when an error occurs, except for devs messing with the code, it's because of something important for the notification doesn't exist anymore, e.g. an order has been deleted etc.
Also this adds the "no_found_rows" improvement to the
LLMS_Notifications_Queryas per #933How has this been tested?
manually, working on tests
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: