We try hard to catch bad email addresses on entry, but we don't always succeed for cases like patron imports. However, with the introduction of exceptions in Koha::Email->create on invalid addresses we now die upon meeting such a case in the database. We should instead wrap this in a try/catch block and report the failure in the database and then move on to the next message in the queue instead of exploding.
Created attachment 123469 [details] [review] Bug 28803: Add unit tests This patch updates Letters.t to confirm that invalid email addresses in the message_queue should not throw an exception when sending mail but instead set the status to failed and pass error details to the end user.
Created attachment 123470 [details] [review] Bug 28803: Add invalid email handling in _send_message_by_email This patch adds a try/catch block around the call to Koha::Email->create to catch and handle invalid emails being passed in the parameters. The message is marked as 'failed' with an error_code of 'INVALID_EMAIL' and the exception message being recorded in the delivery_note.
Whilst writing tests for this, I found some issues with bug 14723 and have hence added a dependency on bug 28813 which I will use to fix those issues and add more comprehensive tests.
Created attachment 123475 [details] [review] Bug 28803: Add unit tests This patch updates Letters.t to confirm that invalid email addresses in the message_queue should not throw an exception when sending mail but instead set the status to failed and pass error details to the end user.
Created attachment 123476 [details] [review] Bug 28803: Add invalid email handling in _send_message_by_email This patch adds a try/catch block around the call to Koha::Email->create to catch and handle invalid emails being passed in the parameters. The message is marked as 'failed' with an error_code of 'INVALID_EMAIL'.
Rebased on top of bug 28813. I'd have liked to have enhanced the failure_code handling so we could continue to pass in which email parameter failed validation as the first version of this patch did but I felt that was out of scope here.
Created attachment 124000 [details] [review] Bug 28803: Add unit tests This patch updates Letters.t to confirm that invalid email addresses in the message_queue should not throw an exception when sending mail but instead set the status to failed and pass error details to the end user. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 124001 [details] [review] Bug 28803: Add invalid email handling in _send_message_by_email This patch adds a try/catch block around the call to Koha::Email->create to catch and handle invalid emails being passed in the parameters. The message is marked as 'failed' with an error_code of 'INVALID_EMAIL'. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
This is the right short-term solution for this issue. A more global solution should be implemented on bug 28870.
+ [% ELSIF ( QUEUED_MESSAGE.failure_code == "INVALID_EMAIL" ) %]Invalid email address found [% borrowernumber | html %] This is probably the most obvious reason, but actually we only know that Koha::Email->create crashed (or Email::Stuffer->new).
Created attachment 124131 [details] [review] Bug 28803: Add unit tests This patch updates Letters.t to confirm that invalid email addresses in the message_queue should not throw an exception when sending mail but instead set the status to failed and pass error details to the end user. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124132 [details] [review] Bug 28803: Add invalid email handling in _send_message_by_email This patch adds a try/catch block around the call to Koha::Email->create to catch and handle invalid emails being passed in the parameters. The message is marked as 'failed' with an error_code of 'INVALID_EMAIL'. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
- # FIXME must be 'failed' - is( $message_1->{status}, 'pending', 'Invalid KohaAdminEmailAddress => status pending' ); + is( $message_1->{status}, 'failed', 'Invalid KohaAdminEmailAddress => status failed' ); That's very pleasant to read :)
(In reply to Jonathan Druart from comment #13) > - # FIXME must be 'failed' > - is( $message_1->{status}, 'pending', 'Invalid KohaAdminEmailAddress => > status pending' ); > + is( $message_1->{status}, 'failed', 'Invalid KohaAdminEmailAddress => > status failed' ); > > That's very pleasant to read :) Haha, it made me smile too :)
This is tight to bug 28870. I think we should push both altogether. However there is one thing that seems wrong to me: + my $email = try { + Koha::Email->create( [SKIP] + } + catch { + _set_message_status( + { + message_id => $message->{'message_id'}, + status => 'failed', + failure_code => 'INVALID_EMAIL' + } + ); + return 0; + }; Koha::Email->create is raising different exception, and Email::Stuffer certainly raised its own. Shouldn't we actually base this on top of bug 28870 and simply call (the newly added) Koha::Email->is_valid to check if the email if valid?
IMO we should base this on top of bug 28870 and provide a separate simple patch for backporting.
The easy way here is actually to keep the existent patches and replace "INVALID_EMAIL" by "UNKNOWN_ERROR", and we follow-up on a separate bug reports with the different exceptions/errors possible. What do you think?
(In reply to Jonathan Druart from comment #17) > The easy way here is actually to keep the existent patches and replace > "INVALID_EMAIL" by "UNKNOWN_ERROR", and we follow-up on a separate bug > reports with the different exceptions/errors possible. > > What do you think? Seems reasonable to me.. I think.. Must admit though, my brain has moved on a bit since I wrote this..
Created attachment 124746 [details] [review] Bug 28803: Replace INVALID_EMAIL with UNKNOWN_ERROR There can be other exceptions/errors raised by Koha::Email->create. On a follow-up bug report we should deal with the different possible errors to handle them properly
Everything ok with this approach then? Please keep moving!
Sorry, not sure what more needs doing here?
(In reply to Martin Renvoize from comment #21) > Sorry, not sure what more needs doing here? Confirmation from you and Tomas that the last patch is what we want.
Well.. all the exceptions thrown by Koha::Email->create are around invalid email addresses so I'm not entirely comfortable with 'Unknown error'.. that kinda feels like "Give up now" to me whereas the actual fix is fairly simple. Problem is.. we don't, yet, pass the 'fields' param in the BadParameter exceptions that are thrown and I'm not sure we've got a field in the messages table to hold that data anyway to pass it to the template. So.. all in all.. I dunno. Personally, we have the patch in production now on all our servers without your follow-up.. and find it helpful. I also don't really think this needs to require either of the depends. Yes, they help to reduce the times it'll get thrown, but this patch eliminates a vital issue whereby sending ANY massages in the queue can be thwarted by one bad email address server wide.
Created attachment 124829 [details] [review] Bug 28803: (follow-up) Error details improvement This patch updates the exceptions thrown by Koha::Email to include the parameter that failed email validation and then updates the failure code to include this parameter and finally display this field in the template.
My final patch is an alternative to Jonathans.. it tries to improve the feedback given to the end user by embedding the parameter that failed validation into the error code and then adding this to the template output.
Is the last patch failing to apply for anyone else? Applying: Bug 28803: (follow-up) Error details improvement .git/rebase-apply/patch:71: trailing whitespace. Koha::Exceptions::BadParameter->throw({ parameter => $address, error => error: sha1 information is lacking or useless (C4/Letters.pm). error: could not build fake ancestor Patch failed at 0001 Bug 28803: (follow-up) Error details improvement
Created attachment 124878 [details] [review] Bug 28803: Add unit tests This patch updates Letters.t to confirm that invalid email addresses in the message_queue should not throw an exception when sending mail but instead set the status to failed and pass error details to the end user.
Created attachment 124879 [details] [review] Bug 28803: Add invalid email handling in _send_message_by_email This patch adds a try/catch block around the call to Koha::Email->create to catch and handle invalid emails being passed in the parameters. The message is marked as 'failed' with an error_code of 'INVALID_EMAIL'.
Created attachment 124880 [details] [review] Bug 28803: (follow-up) Error details improvement This patch updates the exceptions thrown by Koha::Email to include the parameter that failed email validation and then updates the failure code to include this parameter and finally display this field in the template.
Fresh patches attached... should work now Kyle, sorry about that... sha1 issues are the bane.
(In reply to Martin Renvoize from comment #30) > Fresh patches attached... should work now Kyle, sorry about that... sha1 > issues are the bane. I looks like your patches conflict when applied on top of 28870? Even applied directly to master, I'm getting unit test failures: t/db_dependent/Letters.t .. 51/83 # Failed test 'Failure code set correctly for invalid email parameter' # at t/db_dependent/Letters.t line 959. # got: 'INVALID_EMAIL:HASH(0x55de0d87b468)->parameter' # expected: 'INVALID_EMAIL' # Looks like you failed 1 test of 7.
Created attachment 124886 [details] [review] Bug 28803: Add unit tests This patch updates Letters.t to confirm that invalid email addresses in the message_queue should not throw an exception when sending mail but instead set the status to failed and pass error details to the end user.
Created attachment 124887 [details] [review] Bug 28803: Add invalid email handling in _send_message_by_email This patch adds a try/catch block around the call to Koha::Email->create to catch and handle invalid emails being passed in the parameters. The message is marked as 'failed' with an error_code of 'INVALID_EMAIL'.
Created attachment 124888 [details] [review] Bug 28803: (follow-up) Error details improvement This patch updates the exceptions thrown by Koha::Email to include the parameter that failed email validation and then updates the failure code to include this parameter and finally display this field in the template.
Rebased for bug 28870.. I really wish the dependencies hadn't been jumbled up so much.. this was a trivial and critical fix for older Koha's and now it's got a chain of bugs to apply. Oh well.. this is how things go sometimes I suppose.
Created attachment 124900 [details] [review] Bug 28803: Add unit tests This patch updates Letters.t to confirm that invalid email addresses in the message_queue should not throw an exception when sending mail but instead set the status to failed and pass error details to the end user. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 124901 [details] [review] Bug 28803: Add invalid email handling in _send_message_by_email This patch adds a try/catch block around the call to Koha::Email->create to catch and handle invalid emails being passed in the parameters. The message is marked as 'failed' with an error_code of 'INVALID_EMAIL'. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 124902 [details] [review] Bug 28803: (follow-up) Error details improvement This patch updates the exceptions thrown by Koha::Email to include the parameter that failed email validation and then updates the failure code to include this parameter and finally display this field in the template. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Isn't UNKNOWN_ERROR missing in notices.tt?
Created attachment 125114 [details] [review] Bug 28803: (QA folow-up) Reinstate UNKNOWN_ERROR in templates I can't see how this will ever get thrown, but as it's there we should have the template handling included.
Created attachment 125115 [details] [review] Bug 28803: (QA follow-up) Reinstate UNKNOWN_ERROR in templates I can't see how this will ever get thrown, but as it's there we should have the template handling included.
Done... isn't this already PQA..
(In reply to Martin Renvoize from comment #42) > Done... isn't this already PQA.. There is only 1 SO-by line on the patches.
(In reply to Martin Renvoize from comment #41) > Created attachment 125115 [details] [review] [review] > Bug 28803: (QA follow-up) Reinstate UNKNOWN_ERROR in templates > > I can't see how this will ever get thrown, but as it's there we should > have the template handling included. If the SMTP server is unavailable it at least comes.
Created attachment 125343 [details] [review] Bug 28803: Add unit tests This patch updates Letters.t to confirm that invalid email addresses in the message_queue should not throw an exception when sending mail but instead set the status to failed and pass error details to the end user. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 125344 [details] [review] Bug 28803: Add invalid email handling in _send_message_by_email This patch adds a try/catch block around the call to Koha::Email->create to catch and handle invalid emails being passed in the parameters. The message is marked as 'failed' with an error_code of 'INVALID_EMAIL'. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 125345 [details] [review] Bug 28803: (follow-up) Error details improvement This patch updates the exceptions thrown by Koha::Email to include the parameter that failed email validation and then updates the failure code to include this parameter and finally display this field in the template. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 125346 [details] [review] Bug 28803: (QA follow-up) Reinstate UNKNOWN_ERROR in templates I can't see how this will ever get thrown, but as it's there we should have the template handling included. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
(In reply to Joonas Kylmälä from comment #44) > (In reply to Martin Renvoize from comment #41) > > Created attachment 125115 [details] [review] [review] [review] > > Bug 28803: (QA follow-up) Reinstate UNKNOWN_ERROR in templates > > > > I can't see how this will ever get thrown, but as it's there we should > > have the template handling included. > > If the SMTP server is unavailable it at least comes. Really? I didn't think that was the case when calling 'create'.. if it is the case I don't understand why, we don't do anything with the SMTP server until the call to send_or_die. It doesn't cause any harm having this, but I still haven't been able to trigger it myself.
(In reply to Martin Renvoize from comment #49) > (In reply to Joonas Kylmälä from comment #44) > > (In reply to Martin Renvoize from comment #41) > > > Created attachment 125115 [details] [review] [review] [review] [review] > > > Bug 28803: (QA follow-up) Reinstate UNKNOWN_ERROR in templates > > > > > > I can't see how this will ever get thrown, but as it's there we should > > > have the template handling included. > > > > If the SMTP server is unavailable it at least comes. > > Really? I didn't think that was the case when calling 'create'.. if it is > the case I don't understand why, we don't do anything with the SMTP server > until the call to send_or_die. > > It doesn't cause any harm having this, but I still haven't been able to > trigger it myself. I confirm it's needed: https://snipboard.io/10dPzT.jpg kohadev-koha@kohadevbox:/kohadevbox/koha$ perl misc/cronjobs/process_message_queue.pl unable to establish SMTP connection to (localhost) port 25 Trace begun at /usr/share/perl5/Email/Sender/Transport/SMTP.pm line 224 Email::Sender::Transport::SMTP::_throw('Email::Sender::Transport::SMTP=HASH(0x55bd3b7b9bd0)', 'unable to establish SMTP connection to (localhost) port 25') called at /usr/share/perl5/Email/Sender/Transport/SMTP.pm line 174
Pushed to master for 21.11, thanks to everybody involved!
Please add patches for 21.05.x if needed.
*** Bug 28996 has been marked as a duplicate of this bug. ***
(In reply to Kyle M Hall from comment #52) > Please add patches for 21.05.x if needed. Patches apply on 21.05.x I suppose it's thanks to bug 28870
Pushed to 21.05.x for 21.05.05
I've backported unit test to 20.11.x (just removed failure code part) : 2712dbeca6 Bug 28803: Add unit tests And since Bug 28813 is not in 20.11.x, I applied a simplified version based on Bug 28996 : c8a36a1b95 Bug 28803: (20.11.x) Add invalid email handling in _send_message_by_email
Pushed to 20.11.x for 20.11.12
Not backported to 20.05.x which has reached end-of-line and won't be supported anymore. Not worth the risks of a tricky backport for a last release. Which is already late due to a blocking bug. So I won't seek more trouble ^^"
*** Bug 24787 has been marked as a duplicate of this bug. ***