Bug 23357 - overdue_notices.pl can send library doubled emails for patrons with no email/sms if both transports enabled
Summary: overdue_notices.pl can send library doubled emails for patrons with no email/...
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-22 13:41 UTC by Nick Clemens
Modified: 2021-02-04 13:01 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 23357: Check if generating a duplicate print notice before generating notice (4.55 KB, patch)
2019-07-22 13:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23357: Check if generating a duplicate print notice before generating notice (4.60 KB, patch)
2020-03-09 09:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23357: [ALTERNATIVE PATH] Do not generate twice the same print notice (3.71 KB, patch)
2020-03-12 13:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23357: (follow-up) Do not generate twice the same print notice (3.78 KB, patch)
2020-03-12 14:14 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-07-22 13:41:06 UTC
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
Comment 1 Nick Clemens 2019-07-22 13:46:24 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
Comment 2 Martin Renvoize 2020-03-09 09:31:16 UTC
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>
Comment 3 Martin Renvoize 2020-03-09 09:31:55 UTC
Code scans well and test plan passes.  No obvious regressions, signing off.

Thanks Nick
Comment 4 Jonathan Druart 2020-03-12 13:39:20 UTC
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?
Comment 5 Jonathan Druart 2020-03-12 13:42:29 UTC
Created attachment 100634 [details] [review]
Bug 23357: [ALTERNATIVE PATH] Do not generate twice the same print notice
Comment 6 Jonathan Druart 2020-03-12 13:43:34 UTC
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                     }
Comment 7 Nick Clemens 2020-03-12 14:14:42 UTC
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>
Comment 8 Nick Clemens 2020-03-12 14:15:37 UTC
(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!
Comment 9 Jonathan Druart 2020-03-12 14:41:06 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2020-03-12 15:02:37 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2020-03-12 15:04:10 UTC
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
Comment 12 Jonathan Druart 2020-03-12 15:04:40 UTC
So situation 1 does not pass with our patches.