In overdue_notices.pl we have a check to avoid sending a patron two notices if they are missing an email address and an sms number, however, we don't apply this check to generating the notice content and so send the library the same info twice
Created attachment 91693 [details] [review] Bug 23357: Check if generating a duplicate print notice before generating notice To test: 1 - Check out an overdue to a patron 2 - Set a notice trigger for the same number of days overdue for the patrons patron type 3 - Set the trigger to send via email and sms You may need to define SMSSendDriver preference to enable SMS, you can put 'Email' 4 - perl misc/cronjobs/overdue_notices.pl -v -t 5 - Check the patrons record, 1 notice generated 6 - Check the db: SELECT * FROM message_queue; 7 - Note the message 'Overdue notices' contains a doulbed content for the notice 8 - DELETE * FROM message_queue; 9 - Apply patch 10 - perl misc/cronjobs/overdue_notices.pl -v -t 11 - Chekc the patron's account, 1 message in 'Notices' 12 - Check the DB SELECT * FROM message_queue; 13 - The 'Overdue notices' notice is not doubled
Created attachment 100325 [details] [review] Bug 23357: Check if generating a duplicate print notice before generating notice To test: 1 - Check out an overdue to a patron 2 - Set a notice trigger for the same number of days overdue for the patrons patron type 3 - Set the trigger to send via email and sms You may need to define SMSSendDriver preference to enable SMS, you can put 'Email' 4 - perl misc/cronjobs/overdue_notices.pl -v -t 5 - Check the patrons record, 1 notice generated 6 - Check the db: SELECT * FROM message_queue; 7 - Note the message 'Overdue notices' contains a doulbed content for the notice 8 - DELETE * FROM message_queue; 9 - Apply patch 10 - perl misc/cronjobs/overdue_notices.pl -v -t 11 - Chekc the patron's account, 1 message in 'Notices' 12 - Check the DB SELECT * FROM message_queue; 13 - The 'Overdue notices' notice is not doubled Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Code scans well and test plan passes. No obvious regressions, signing off. Thanks Nick
Nick, what about this change: unless ( $effective_mtt eq 'print' and $print_sent == 1 ) { - if ( ($mtt eq 'email' and not scalar @emails_to_use) or ($mtt eq 'sms' and not $data->{smsalertnumber}) ) { + if ( $effective_mtt eq 'print' and not $print_sent ) { push @output_chunks, It makes the code more readable and less error prone, what do you think?
Created attachment 100634 [details] [review] Bug 23357: [ALTERNATIVE PATH] Do not generate twice the same print notice
It avoid to repeat the same test: 683 if ( ($mtt eq 'email' and not scalar @emails_to_use) or ($mtt eq 'sms' and not $data->{smsalertnumber}) ) { 684 # email or sms is requested but not exist, do a print. 685 $effective_mtt = 'print'; 686 }
Created attachment 100639 [details] [review] Bug 23357: (follow-up) Do not generate twice the same print notice Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Jonathan Druart from comment #5) > Created attachment 100634 [details] [review] [review] > Bug 23357: [ALTERNATIVE PATH] Do not generate twice the same print notice This is a followup yes? I applied both and tested, works well, I can't see any issue in the code. Thanks!
Situation 1 - Overdue 3 days for Patron: email + print A. No email defined, overdue -t generates * on master, 2 notices OK 1 print for patron 1 email for staff * on 23357 (only first patch) 1 print for patron * on 23357_2 (2 patches) OK 1 print for patron 1 email for staff B. With email defined * on master, 2 notices 1 print for patron 1 email for patron * on 23357 (only first patch) 1 print for patron 1 email for patron * on 23357_2 (2 patches) OK 1 print for patron 1 email for patron 1 email for staff Situation 2 - Overdue 3 days for Patron: email only A. No email defined, overdue -t generates * on master, 2 notices OK 1 print for patron 1 email for staff * on 23357 (only first patch) OK 1 print for patron 1 email for staff * on 23357_2 (2 patches) OK 1 print for patron 1 email for staff B. With email defined * on master, 2 notices 1 email for patron * on 23357 (only first patch) 1 email for patron * on 23357_2 (2 patches) 1 email for patron There is no OK in Situation 2 for B
Previous comment is not correct, -t does not mean "trigger an email for staff"
Situation 1 - Overdue 3 days for Patron: email + print A. No email defined, overdue -t generates * on master, 2 notices OK 1 print for patron 1 email for staff * on 23357 (only first patch) 1 print for patron * on 23357_2 (2 patches) OK 1 print for patron 1 email for staff B. With email defined * on master, 2 notices OK 1 print for patron 1 email for patron * on 23357 (only first patch) OK 1 print for patron 1 email for patron * on 23357_2 (2 patches) 1 print for patron 1 email for patron 1 email for staff Situation 2 - Overdue 3 days for Patron: email only A. No email defined, overdue -t generates * on master, 2 notices OK 1 print for patron 1 email for staff * on 23357 (only first patch) OK 1 print for patron 1 email for staff * on 23357_2 (2 patches) OK 1 print for patron 1 email for staff B. With email defined * on master, 2 notices OK 1 email for patron * on 23357 (only first patch) OK 1 email for patron * on 23357_2 (2 patches) OK 1 email for patron
So situation 1 does not pass with our patches.