Bug 26922

Summary: SendAlerts does not correctly handle error on sending emails
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: System AdministrationAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrewfh, gmcharlt, martin.renvoize, tomascohen, victor
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00
Bug Depends on: 22343    
Bug Blocks:    
Attachments: Bug 26922: Correctly handle email sending error from SendAlerts
Bug 26922: Better error handling in SendAlerts
Bug 26922: Better error handling in SendAlerts
Bug 26922: Better error handling in SendAlerts
Bug 26922: Regression tests
Bug 26922: Better error handling in SendAlerts
Bug 26922: Regression tests

Description Jonathan Druart 2020-11-04 09:41:19 UTC
When an email is sent from C4::Letters::SendAlerts (for instance emailing a acquisition basket), the UI displays a "all good" message even if the email has not been sent properly.

To recreate:
1. Set KohaAdminEmailAddress to admin@example.org
2. Edit a vendor, set a valid email address
3. Create a new basket, a new order. Send the basket
As you did not configure a valid SMTP server, the email is not sent and logs displayed "unable to establish SMTP connection to (localhost) port 25", with the stracktrace.

The problem is coming from the return inside the catch (which returns from the catch, not the subroutine as expected).

There are 2 problematic occurrences in SendAlerts:
 360             try {
 361                 $mail->send_or_die({ transport => $library->smtp_server->transport });
 362             }
 363             catch {
 364                 carp "$_";
 365                 return { error => "$_" };
 366             };

 512         try {
 513             $mail->send_or_die({ transport => $library->smtp_server->transport });
 514         }
 515         catch {
 516             carp "$_";
 517             return { error => "$_" };
 518         };

=> SendAlerts will return 1 (from l.577) anyway.

And 1 occurrence in _send_message_by_email that should be corrected to avoid confusion.
Comment 1 Jonathan Druart 2020-11-17 15:08:31 UTC
Created attachment 113736 [details] [review]
Bug 26922: Correctly handle email sending error from SendAlerts
Comment 2 Jonathan Druart 2020-11-17 15:10:41 UTC
There is something weird going on.

With this patch I get a stacktrace of the "unable to establish SMTP connection to localhost port 25" error.
Which is okish.

The problem is that I've tried to turn off dev_install and see if the error was nicer, and... there is no error now.
(Worst, I put a warn in mainpage.pl and I did not find in which log file it landed)
Comment 3 Tomás Cohen Arazi 2020-11-19 16:16:05 UTC
Created attachment 113828 [details] [review]
Bug 26922: Better error handling in SendAlerts

This patch makes SendAlerts display a better error message when sending
fails.

To test:
1. Set KohaAdminEmailAddress to admin@example.org
2. Edit a vendor, set a valid email address
3. Create a new basket, a new order. Send the basket
=> FAIL: As you did not configure a valid SMTP server, the email is not sent and logs displayed "unable to establish SMTP connection to (localhost) port 25", with the stracktrace.
4. Apply this patch and reload all
5. Repeat 3
=> SUCCESS: A simpler message is displayed, the stacktrace remains in
the logs
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2020-11-19 16:16:24 UTC
I'll try to provide some tests for this later.
Comment 5 Victor Grousset/tuxayo 2020-11-19 17:55:46 UTC
> 3. Create a new basket, a new order. Send the basket
> => FAIL: As you did not configure a valid SMTP server, the email is not sent and logs displayed "unable to establish SMTP connection to (localhost) port 25", with the stracktrace.

Can't reproduce, in the UI I have «Order e-mail was sent to the vendor.»
The logs contain the error.

env: koha-testing-docker
code: master without patch applied

In the vendor setup I filled an email and ticked «Contact when ordering?»
Comment 6 Tomás Cohen Arazi 2020-11-19 17:56:55 UTC
(In reply to Victor Grousset/tuxayo from comment #5)
> > 3. Create a new basket, a new order. Send the basket
> > => FAIL: As you did not configure a valid SMTP server, the email is not sent and logs displayed "unable to establish SMTP connection to (localhost) port 25", with the stracktrace.
> 
> Can't reproduce, in the UI I have «Order e-mail was sent to the vendor.»
> The logs contain the error.
> 
> env: koha-testing-docker
> code: master without patch applied
> 
> In the vendor setup I filled an email and ticked «Contact when ordering?»

I clicked on the 'E-mail order' button.

https://snipboard.io/6Yn8W1.jpg
Comment 7 Victor Grousset/tuxayo 2020-11-19 19:49:19 UTC
Same and that's how I got (and still get after retrying) a success instead of a failure.

When I apply the patch and retry, I get.

>  ERROR! - unable to establish SMTP connection to localhost port 25 

Which is good!

Even if I don't get the same level of wrong behavior without the patch, the test seems to be a success right?
Comment 8 Tomás Cohen Arazi 2020-11-19 19:52:00 UTC
Yes, we are basically providing a better error message. The original issue was because of dev_install=0, which ended up in a variety of behaviors depending on the age of you docker image.
Comment 9 Victor Grousset/tuxayo 2020-11-19 21:01:36 UTC
Created attachment 113849 [details] [review]
Bug 26922: Better error handling in SendAlerts

This patch makes SendAlerts display a better error message when sending
fails.

To test:
1. Set KohaAdminEmailAddress to admin@example.org
2. Edit a vendor, set a valid email address
3. Create a new basket, a new order. Send the basket
=> FAIL: As you did not configure a valid SMTP server, the email is not sent and logs displayed "unable to establish SMTP connection to (localhost) port 25", with the stracktrace.
4. Apply this patch and reload all
5. Repeat 3
=> SUCCESS: A simpler message is displayed, the stacktrace remains in
the logs
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 10 Martin Renvoize 2020-11-20 10:48:21 UTC
Created attachment 113870 [details] [review]
Bug 26922: Better error handling in SendAlerts

This patch makes SendAlerts display a better error message when sending
fails.

To test:
1. Set KohaAdminEmailAddress to admin@example.org
2. Edit a vendor, set a valid email address
3. Create a new basket, a new order. Send the basket
=> FAIL: As you did not configure a valid SMTP server, the email is not sent and logs displayed "unable to establish SMTP connection to (localhost) port 25", with the stracktrace.
4. Apply this patch and reload all
5. Repeat 3
=> SUCCESS: A simpler message is displayed, the stacktrace remains in
the logs
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2020-11-20 10:49:45 UTC
This works well and passes the QA scripts.  Were you still going to add Unit tests to cover the change of return Tomas?
Comment 12 Tomás Cohen Arazi 2020-11-20 14:08:20 UTC
Created attachment 113884 [details] [review]
Bug 26922: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Martin Renvoize 2020-11-20 14:43:00 UTC
Created attachment 113894 [details] [review]
Bug 26922: Better error handling in SendAlerts

This patch makes SendAlerts display a better error message when sending
fails.

To test:
1. Set KohaAdminEmailAddress to admin@example.org
2. Edit a vendor, set a valid email address
3. Create a new basket, a new order. Send the basket
=> FAIL: As you did not configure a valid SMTP server, the email is not sent and logs displayed "unable to establish SMTP connection to (localhost) port 25", with the stracktrace.
4. Apply this patch and reload all
5. Repeat 3
=> SUCCESS: A simpler message is displayed, the stacktrace remains in
the logs
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2020-11-20 14:43:03 UTC
Created attachment 113895 [details] [review]
Bug 26922: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2020-11-20 14:43:26 UTC
Works well, covered by tests.. Thanks Tomas

Passing QA
Comment 16 Jonathan Druart 2020-11-20 14:44:42 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 17 Andrew Fuerste-Henry 2020-12-10 22:19:06 UTC
Missing dependencies, not backported to 20.05