Description
Nick Clemens (kidclamp)
2019-07-22 13:41:06 UTC
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. |