Description
Marcel de Rooy
2023-03-29 15:01:24 UTC
Created attachment 149256 [details] [review] Bug 33360: Add Koha::Notice::Util for mail domain limits Includes unit test. Test plan: Run t/db_dependent/Koha/Notice_Util.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 149257 [details] [review] Bug 33360: Extend Koha::Notice::Util with domain groups Created attachment 149258 [details] [review] Bug 33360: Incorporate Koha::Notice::Util in Letters:SendQueuedMessages This is still work in progess. I stumbled over the where parameter now while converting stuff to a Koha object search. Not sure if we resolved such a thing somewhere else already. Could we automatically convert an old school SQL WHERE condition to correct Koha object/DBIx conditions? <Joubu> marcelr: 33360 - see misc/cronjobs/membership_expiry.pl:199 $upcoming_mem_expires = $upcoming_mem_expires->search( \$where_literal ) if @where; (In reply to Marcel de Rooy from comment #5) > <Joubu> marcelr: 33360 - see misc/cronjobs/membership_expiry.pl:199 > $upcoming_mem_expires = $upcoming_mem_expires->search( \$where_literal ) if > @where; Yes, that would be enough! Still patches forthcoming. Created attachment 149546 [details] [review] Bug 33360: Add Koha::Notice::Util for mail domain limits Ground work in new module. Includes unit test. Test plan: Run t/db_dependent/Koha/Notice_Util.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 149547 [details] [review] Bug 33360: Extend Koha::Notice::Util with domain groups The functionality is extended a bit here by: [1] Allowing to combine domains in a shared count. [2] Only counting sent messages for the specified domains when it is really needed. Test plan: Run t/db_dependent/Koha/Notice_Util.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 149548 [details] [review] Bug 33360: Incorporate Koha::Notice::Util in Letters:SendQueuedMessages Minimal adjustment to get the feature working. Will still be improved in follow-up. Test plan: [1] Add a domain limit to your koha-conf: <message_domain_limits> <domain><name>gmail.com</name><limit>1</limit> <unit>1h</unit></domain> </message_domain_limits> Replace gmail.com by the domain you want to send to. Restart all. [2] Disable cron job for message queue. [3] Generate two pending notices in the queue. [4] Run process_message_queue.pl [5] Check that one is sent, one is still pending. [6] Run again. [7] Check: nothing is sent, last one still pending. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 149549 [details] [review] Bug 33360: Improving limit behavior in SendQueuedMessages Includes: [1] Do no longer use the limit in the sql selection, but apply the limit as a maximum for the number of sent messages. This is more practical in terms of not flooding your mail server (and the receiving ones). [2] Replace call of _get_unsent_messages by Koha objects search. [3] Do no longer report the number of messages seen, but report the number actually 'sent'. [4] If we lookup the to_address but we need to delay a message, save the email address for the next run. Also optimizing patron lookup in _send_message_by_email. [5} Add support for $where parameter in SendQueuedMessages. Used by process_message_queue.pl. [6] Handle scalar/array for letter_code and type parameter too. Test plan: [1] Adjust your domain limit settings in koha-conf. Use notices to three domains. Group A and B. <message_domain_limits> <domain><name>A</name><limit>1</limit><unit>1h</unit></domain> <domain><name>B/name><belongs_to>A</belongs_to></domain> <domain><name>C</name><limit>1</limit><unit>1h</unit></domain> </message_domain_limits> Replace A, B and C with your choice. Do not forget the belongs_to. Restart all. [2] Disable cron job for message queue. [3] Generate two notices for each domain A, B and C (in that order). Make sure that borrowers involved have correct address. [4] Run process_message_queue.pl -limit 1 [5] Check that one is sent for A, 5 pending. [6] Run process_message_queue.pl -limit 2 [7] Check that one is sent for C, 4 pending. [8] Run process_message_queue.pl (without limit). [9] Check that nothing is sent, 4 pending. [10] Check that message_queue.to_address is filled for those 4. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 149550 [details] [review] Bug 33360: Adjust Letters.t for limit parameter and domain limits Includes: [1] Add a counter in the send_or_die mock. [2] Correct from processed count to sent count. [3] More extensive testing for limit parameter and domain limits. Test plan: Run t/db_dependent/Letters.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 149551 [details] [review] Bug 33360: Update koha-conf.xml Test plan: We already tested it. Just look at changes in this patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 149552 [details] [review] Bug 33360: (follow-up) Adjust wording in verbose mode Koha reports 'sending mail' while we actually are still processing mail. It could still be delayed, fail, etc. Test plan: Run process_message_queue in verbose mode. If a message get processed, you should see: Processing email message to patron: 51 at /usr/share/koha/C4/Letters.pm line 1008. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Note for testers/QA: An adjusted version already runs in production on 21.11 for me. And was 'put to the test' with a large mailing already. Created attachment 149645 [details] [review] Bug 33360: Add Koha::Notice::Util for mail domain limits Ground work in new module. Includes unit test. Test plan: Run t/db_dependent/Koha/Notice_Util.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 149646 [details] [review] Bug 33360: Extend Koha::Notice::Util with domain groups The functionality is extended a bit here by: [1] Allowing to combine domains in a shared count. [2] Only counting sent messages for the specified domains when it is really needed. Test plan: Run t/db_dependent/Koha/Notice_Util.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 149647 [details] [review] Bug 33360: Incorporate Koha::Notice::Util in Letters:SendQueuedMessages Minimal adjustment to get the feature working. Will still be improved in follow-up. Test plan: [1] Add a domain limit to your koha-conf: <message_domain_limits> <domain><name>gmail.com</name><limit>1</limit> <unit>1h</unit></domain> </message_domain_limits> Replace gmail.com by the domain you want to send to. Restart all. [2] Disable cron job for message queue. [3] Generate two pending notices in the queue. [4] Run process_message_queue.pl [5] Check that one is sent, one is still pending. [6] Run again. [7] Check: nothing is sent, last one still pending. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 149648 [details] [review] Bug 33360: Improving limit behavior in SendQueuedMessages Includes: [1] Do no longer use the limit in the sql selection, but apply the limit as a maximum for the number of sent messages. This is more practical in terms of not flooding your mail server (and the receiving ones). [2] Replace call of _get_unsent_messages by Koha objects search. [3] Do no longer report the number of messages seen, but report the number actually 'sent'. [4] If we lookup the to_address but we need to delay a message, save the email address for the next run. Also optimizing patron lookup in _send_message_by_email. [5} Add support for $where parameter in SendQueuedMessages. Used by process_message_queue.pl. [6] Handle scalar/array for letter_code and type parameter too. Test plan: [1] Adjust your domain limit settings in koha-conf. Use notices to three domains. Group A and B. <message_domain_limits> <domain><name>A</name><limit>1</limit><unit>1h</unit></domain> <domain><name>B/name><belongs_to>A</belongs_to></domain> <domain><name>C</name><limit>1</limit><unit>1h</unit></domain> </message_domain_limits> Replace A, B and C with your choice. Do not forget the belongs_to. Restart all. [2] Disable cron job for message queue. [3] Generate two notices for each domain A, B and C (in that order). Make sure that borrowers involved have correct address. [4] Run process_message_queue.pl -limit 1 [5] Check that one is sent for A, 5 pending. [6] Run process_message_queue.pl -limit 2 [7] Check that one is sent for C, 4 pending. [8] Run process_message_queue.pl (without limit). [9] Check that nothing is sent, 4 pending. [10] Check that message_queue.to_address is filled for those 4. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 149649 [details] [review] Bug 33360: Adjust Letters.t for limit parameter and domain limits Includes: [1] Add a counter in the send_or_die mock. [2] Correct from processed count to sent count. [3] More extensive testing for limit parameter and domain limits. Test plan: Run t/db_dependent/Letters.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 149650 [details] [review] Bug 33360: Update koha-conf.xml Test plan: We already tested it. Just look at changes in this patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 149651 [details] [review] Bug 33360: (follow-up) Adjust wording in verbose mode Koha reports 'sending mail' while we actually are still processing mail. It could still be delayed, fail, etc. Test plan: Run process_message_queue in verbose mode. If a message get processed, you should see: Processing email message to patron: 51 at /usr/share/koha/C4/Letters.pm line 1008. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> *** Bug 30013 has been marked as a duplicate of this bug. *** All working well in testing, not obvious code issues. Signing Off Created attachment 149662 [details] [review] Bug 33360: Add Koha::Notice::Util for mail domain limits Ground work in new module. Includes unit test. Test plan: Run t/db_dependent/Koha/Notice_Util.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 149663 [details] [review] Bug 33360: Extend Koha::Notice::Util with domain groups The functionality is extended a bit here by: [1] Allowing to combine domains in a shared count. [2] Only counting sent messages for the specified domains when it is really needed. Test plan: Run t/db_dependent/Koha/Notice_Util.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 149664 [details] [review] Bug 33360: Incorporate Koha::Notice::Util in Letters:SendQueuedMessages Minimal adjustment to get the feature working. Will still be improved in follow-up. Test plan: [1] Add a domain limit to your koha-conf: <message_domain_limits> <domain><name>gmail.com</name><limit>1</limit> <unit>1h</unit></domain> </message_domain_limits> Replace gmail.com by the domain you want to send to. Restart all. [2] Disable cron job for message queue. [3] Generate two pending notices in the queue. [4] Run process_message_queue.pl [5] Check that one is sent, one is still pending. [6] Run again. [7] Check: nothing is sent, last one still pending. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 149665 [details] [review] Bug 33360: Improving limit behavior in SendQueuedMessages Includes: [1] Do no longer use the limit in the sql selection, but apply the limit as a maximum for the number of sent messages. This is more practical in terms of not flooding your mail server (and the receiving ones). [2] Replace call of _get_unsent_messages by Koha objects search. [3] Do no longer report the number of messages seen, but report the number actually 'sent'. [4] If we lookup the to_address but we need to delay a message, save the email address for the next run. Also optimizing patron lookup in _send_message_by_email. [5} Add support for $where parameter in SendQueuedMessages. Used by process_message_queue.pl. [6] Handle scalar/array for letter_code and type parameter too. Test plan: [1] Adjust your domain limit settings in koha-conf. Use notices to three domains. Group A and B. <message_domain_limits> <domain><name>A</name><limit>1</limit><unit>1h</unit></domain> <domain><name>B/name><belongs_to>A</belongs_to></domain> <domain><name>C</name><limit>1</limit><unit>1h</unit></domain> </message_domain_limits> Replace A, B and C with your choice. Do not forget the belongs_to. Restart all. [2] Disable cron job for message queue. [3] Generate two notices for each domain A, B and C (in that order). Make sure that borrowers involved have correct address. [4] Run process_message_queue.pl -limit 1 [5] Check that one is sent for A, 5 pending. [6] Run process_message_queue.pl -limit 2 [7] Check that one is sent for C, 4 pending. [8] Run process_message_queue.pl (without limit). [9] Check that nothing is sent, 4 pending. [10] Check that message_queue.to_address is filled for those 4. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 149666 [details] [review] Bug 33360: Adjust Letters.t for limit parameter and domain limits Includes: [1] Add a counter in the send_or_die mock. [2] Correct from processed count to sent count. [3] More extensive testing for limit parameter and domain limits. Test plan: Run t/db_dependent/Letters.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 149667 [details] [review] Bug 33360: Update koha-conf.xml Test plan: We already tested it. Just look at changes in this patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 149668 [details] [review] Bug 33360: (follow-up) Adjust wording in verbose mode Koha reports 'sending mail' while we actually are still processing mail. It could still be delayed, fail, etc. Test plan: Run process_message_queue in verbose mode. If a message get processed, you should see: Processing email message to patron: 51 at /usr/share/koha/C4/Letters.pm line 1008. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> QA thoughts: Considering the SMTP configuration is web based, it would be nice if the controls for this were integrated into it. (In reply to Kyle M Hall from comment #32) > QA thoughts: Considering the SMTP configuration is web based, it would be > nice if the controls for this were integrated into it. Thanks Kyle for QA. Happy to put some further improvements on a follow-up. (In reply to Kyle M Hall from comment #32) > QA thoughts: Considering the SMTP configuration is web based, it would be > nice if the controls for this were integrated into it. Agreed! Pushed to master for 23.05. Nice work everyone, thanks! (In reply to Tomás Cohen Arazi from comment #35) > Pushed to master for 23.05. > > Nice work everyone, thanks! Top ! (In reply to Tomás Cohen Arazi from comment #34) > (In reply to Kyle M Hall from comment #32) > > QA thoughts: Considering the SMTP configuration is web based, it would be > > nice if the controls for this were integrated into it. > > Agreed! Bug 33537 Enhancement - not backporting to 22.11.x Nice work everyone! (In reply to Kyle M Hall from comment #32) > QA thoughts: Considering the SMTP configuration is web based, it would be > nice if the controls for this were integrated into it. 100% this yes please |