Bug 29501

Summary: gather_print_notices.pl does not use SMTP servers
Product: Koha Reporter: Magnus Enger <magnus>
Component: Command-line UtilitiesAssignee: Tomás Cohen Arazi <tomascohen>
Status: Pushed to stable --- QA Contact: Jonathan Druart <jonathan.druart+koha>
Severity: normal    
Priority: P5 - low CC: andrewfh, fridolin.somers, jonathan.druart+koha, kyle, martin.renvoize, nick, robin, tomascohen
Version: master   
Hardware: All   
OS: All   
URL: https://metacpan.org/pod/MIME::Lite#WAIT!
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29330
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.05
Bug Depends on: 22343    
Bug Blocks:    
Attachments: Bug 29501: Make gather_print_notices.pl use Koha::Email
Bug 29501: Remove MIME::Lite dependency
Bug 29501: (follow-up) Remove unused MIME::QuotedPrint dependency
Bug 29501: Make gather_print_notices.pl use Koha::Email
Bug 29501: Remove MIME::Lite dependency
Bug 29501: (follow-up) Remove unused MIME::QuotedPrint dependency
Bug 29501: Make gather_print_notices.pl use Koha::Email
Bug 29501: Remove MIME::Lite dependency
Bug 29501: (follow-up) Remove unused MIME::QuotedPrint dependency
Bug 29501: Use the default transport
Bug 29501: Make gather_print_notices.pl use Koha::Email
Bug 29501: Remove MIME::Lite dependency
Bug 29501: (follow-up) Remove unused MIME::QuotedPrint dependency
Bug 29501: Use the default transport

Description Magnus Enger 2021-11-17 10:32:37 UTC
Bug 22343 added the ability to specify SMTP servers for outgoing email, removing the need to run an MTA (like postfix) on the server. 

As far as I can tell, gather_print_notices.pl does it's own email sending with MIME::Lite, bypassing the message_queue and the SMTP server setup. So in order to use this as a cronjob to send print as email, you have to run an MTA anyway. 

Could this be fixed, so the script uses the SMTP server setup?
Comment 1 Tomás Cohen Arazi 2021-11-18 12:43:37 UTC
Created attachment 127789 [details] [review]
Bug 29501: Make gather_print_notices.pl use Koha::Email

This patch makes the cronjob script use Koha::Email and thus relying on
configured SMTP settings instead of just trying localhost.
Comment 2 Tomás Cohen Arazi 2021-11-18 12:43:42 UTC
Created attachment 127790 [details] [review]
Bug 29501: Remove MIME::Lite dependency

The library usage is discouraged by the current maintainer and we don't
use it in the codebase anymore.

To test:
1. Apply this patch
2. Run:
   $ git grep MIME::Lite
=> SUCCESS: No uses of the removed library
3. Sign off :-D
Comment 3 Tomás Cohen Arazi 2021-11-18 12:45:08 UTC
(In reply to Magnus Enger from comment #0)
> Bug 22343 added the ability to specify SMTP servers for outgoing email,
> removing the need to run an MTA (like postfix) on the server. 
> 
> As far as I can tell, gather_print_notices.pl does it's own email sending
> with MIME::Lite, bypassing the message_queue and the SMTP server setup. So
> in order to use this as a cronjob to send print as email, you have to run an
> MTA anyway. 
> 
> Could this be fixed, so the script uses the SMTP server setup?

Hi, I'm not familiar with the script enough to write a test plan. Can you test and amend the commit message to include the test plan?

Thanks!
Comment 4 Tomás Cohen Arazi 2021-11-18 12:47:31 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> Created attachment 127790 [details] [review] [review]
> Bug 29501: Remove MIME::Lite dependency
> 
> The library usage is discouraged by the current maintainer and we don't
> use it in the codebase anymore.

https://metacpan.org/pod/MIME::Lite#WAIT!
Comment 5 Tomás Cohen Arazi 2021-11-18 13:04:05 UTC
Created attachment 127794 [details] [review]
Bug 29501: (follow-up) Remove unused MIME::QuotedPrint dependency
Comment 6 Tomás Cohen Arazi 2022-03-07 21:23:17 UTC
Ping! Consistency? Complete the development?
Comment 7 Martin Renvoize 2022-03-08 11:44:09 UTC
Created attachment 131446 [details] [review]
Bug 29501: Make gather_print_notices.pl use Koha::Email

This patch makes the cronjob script use Koha::Email and thus relying on
configured SMTP settings instead of just trying localhost.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2022-03-08 11:44:13 UTC
Created attachment 131447 [details] [review]
Bug 29501: Remove MIME::Lite dependency

The library usage is discouraged by the current maintainer and we don't
use it in the codebase anymore.

To test:
1. Apply this patch
2. Run:
   $ git grep MIME::Lite
=> SUCCESS: No uses of the removed library
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2022-03-08 11:44:17 UTC
Created attachment 131448 [details] [review]
Bug 29501: (follow-up) Remove unused MIME::QuotedPrint dependency

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2022-03-08 11:46:32 UTC
Well.. this all appears to work for me.. so Signing Off.

However.. QA note.. shouldn't the 'send_or_die' call be wrapped in a try/catch block like elsewhere?
Comment 11 Jonathan Druart 2022-03-21 13:25:50 UTC
-        unless $format =~ m[^html$|^csv$|^ods$];
+        unless $format =~ m/^html$|^csv$|^ods$/;

Why? Unnecessary change.
Comment 12 Jonathan Druart 2022-03-21 13:26:04 UTC
It does not work for me, I don't see the email being sent.
Comment 13 Jonathan Druart 2022-03-21 13:29:30 UTC
You are missing the transport.
Comment 14 Jonathan Druart 2022-03-21 13:30:25 UTC
You also need to determinate which SMTP server to use (from which library?).
Comment 15 Tomás Cohen Arazi 2022-03-21 16:33:00 UTC
(In reply to Jonathan Druart from comment #11)
> -        unless $format =~ m[^html$|^csv$|^ods$];
> +        unless $format =~ m/^html$|^csv$|^ods$/;
> 
> Why? Unnecessary change.

I think I did it tepmorarily and then forgot, because of this:

https://snipboard.io/3GwSsV.jpg
Comment 16 Tomás Cohen Arazi 2022-03-21 18:23:08 UTC
Created attachment 131989 [details] [review]
Bug 29501: Make gather_print_notices.pl use Koha::Email

This patch makes the cronjob script use Koha::Email and thus relying on
configured SMTP settings instead of just trying localhost.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Tomás Cohen Arazi 2022-03-21 18:23:12 UTC
Created attachment 131990 [details] [review]
Bug 29501: Remove MIME::Lite dependency

The library usage is discouraged by the current maintainer and we don't
use it in the codebase anymore.

To test:
1. Apply this patch
2. Run:
   $ git grep MIME::Lite
=> SUCCESS: No uses of the removed library
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Tomás Cohen Arazi 2022-03-21 18:23:16 UTC
Created attachment 131991 [details] [review]
Bug 29501: (follow-up) Remove unused MIME::QuotedPrint dependency

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Tomás Cohen Arazi 2022-03-21 18:23:21 UTC
Created attachment 131992 [details] [review]
Bug 29501: Use the default transport

This patch makes the script pass the default SMTP transport to the
->send_or_die call.

The default is picked as this is the current behavior. New enhancements
could add the *library_id* to the message_queue table, and allow using
different transports depending on that. But it is out of the scope of
this bug.

To test:
1. Verify messages are being sent.
2. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 20 Tomás Cohen Arazi 2022-03-21 18:30:37 UTC
(In reply to Jonathan Druart from comment #14)
> You also need to determinate which SMTP server to use (from which library?).

Hi, I've tried to learn more about the feature to answer you, Jonathan. So correct me if I'm wrong.

The feature is not that evolved. It is designed to generate the notice(s) in a directory (shared mount?). Print notices are generated in several ways which should be better documented. CHECKOUT is not one of them :-D Thanks to Andrew I now know that you can generate a print notice when trapping a hold for a patron with no valid email.

Once you have that (it shows as 'print' on the patron's notices tab), I ran:

$ perl misc/cronjobs/gather_print_notices.pl /tmp --email tomascohen@theke.io,andrew@bywatersolutions.com --send --html


As you will notice, the --send addresses list is not tied to any Koha feature. You just pass the addresses... I can forsee a development in which we could add print notices addesses configuration to libraries and a --library parameter to the print notices, but it will still require added a library_id column to the message_queue table.

Anyway...

The follow-up makes it work. It uses the default configured SMTP server. My config:

 <smtp_server>
    <host>smtp.gmail.com</host>
    <port>587</port>
    <timeout>50</timeout>
    <ssl_mode>starttls</ssl_mode>
    <user_name>tomascohen@gmail.com</user_name>
    <password>xxx</password>
    <debug>1</debug>
 </smtp_server>


The password I used was generated on the 'generate applilcation password' on my Google account. debug=1 made the SMTP session super verbose. And email arrived at destination :-D
Comment 21 Jonathan Druart 2022-03-22 10:40:44 UTC
Created attachment 132003 [details] [review]
Bug 29501: Make gather_print_notices.pl use Koha::Email

This patch makes the cronjob script use Koha::Email and thus relying on
configured SMTP settings instead of just trying localhost.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Jonathan Druart 2022-03-22 10:40:49 UTC
Created attachment 132004 [details] [review]
Bug 29501: Remove MIME::Lite dependency

The library usage is discouraged by the current maintainer and we don't
use it in the codebase anymore.

To test:
1. Apply this patch
2. Run:
   $ git grep MIME::Lite
=> SUCCESS: No uses of the removed library
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2022-03-22 10:40:53 UTC
Created attachment 132005 [details] [review]
Bug 29501: (follow-up) Remove unused MIME::QuotedPrint dependency

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2022-03-22 10:40:57 UTC
Created attachment 132006 [details] [review]
Bug 29501: Use the default transport

This patch makes the script pass the default SMTP transport to the
->send_or_die call.

The default is picked as this is the current behavior. New enhancements
could add the *library_id* to the message_queue table, and allow using
different transports depending on that. But it is out of the scope of
this bug.

To test:
1. Verify messages are being sent.
2. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

JD amended patch: revert
-        unless $format =~ m[^html$|^csv$|^ods$];
+        unless $format =~ m/^html$|^csv$|^ods$/;
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Fridolin Somers 2022-04-05 15:19:04 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 26 Kyle M Hall 2022-04-08 12:32:37 UTC
Pushed to 21.11.x for 21.11.05