|
Description
Magnus Enger
2021-11-17 10:32: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. 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 (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! (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! Created attachment 127794 [details] [review] Bug 29501: (follow-up) Remove unused MIME::QuotedPrint dependency Ping! Consistency? Complete the development? 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> 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> Created attachment 131448 [details] [review] Bug 29501: (follow-up) Remove unused MIME::QuotedPrint dependency Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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? - unless $format =~ m[^html$|^csv$|^ods$]; + unless $format =~ m/^html$|^csv$|^ods$/; Why? Unnecessary change. It does not work for me, I don't see the email being sent. You are missing the transport. You also need to determinate which SMTP server to use (from which library?). (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 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> 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> Created attachment 131991 [details] [review] Bug 29501: (follow-up) Remove unused MIME::QuotedPrint dependency Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> (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 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> 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> 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> 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> Pushed to master for 22.05, thanks to everybody involved [U+1F984] Pushed to 21.11.x for 21.11.05 |
