Following a discussion on french list koha-infos, it appears that, among other ones, 2 things are expected from the overdue_notices.pl script (but not currently achieved) : 1) It should be possible to send only one message grouping all branches overdue items with same delay level => by now there are as much messages sent as overdue items branches with same level ( a patron that have overdue with same level in library A and B will received 2 messages claiming the same items.) This patch try to solve this problem ( provided to use only a default notice model in Notices and Slips). It introduces a new option (a|all_in_one) that has to be followed by a branchcode from which will be set the sender address of the message. 2) each branch should be able to send claims including only its very overdue items => by now messages also include, if they exist, all overdues with same delay level from other branches, even if library option is used. Patch from bug 17470 aims to solve this problem. Test plan (almost entirely and shamefully copied from bug 17470 !) 0/ Have (or create) at least two branches B1 and B2. Have (or create) a default notice for all branches in Notices and Slips Module 1/ Make sure you're logged in with an account belonging to B1, or switch to branch B1 2/ checkout item I1 (belonging to B1) to patron P (preferably while specifying a due date < today) 3/ switch to branch B2 4/ checkout item I2 (belonging to B2) to patron P (preferably while specifying a due date < today) 5/ make sure you have defined "Overdue notice/status triggers" for that patron category, ie with Delay=1 so that "Overdue Notice" is selected and "Email" is checked. 6/ from cli run 'perl ./misc/cronjobs/overdue_notices.pl' (optionally specifying also '-library B1 -library B2') example (say that B2CODE is the branchcode of B2 branch ): ./overdue_notices.pl -t -a B2CODE 7/ Two new entries will be generated in message_queue DB table, coming from different branches that have the SAME list of overdue items 8/ either see the 'content' field in these entries, or run 'perl ./cronjobs/process_message_queue.pl' to receive the actual emails (provided the email is properly setup). Apply patch redo step 6/ You will see that only one message had been created. Its content shows both B1 and B2 overdue items and the sender address matches B2 address as well. Olivier Crouzet
Created attachment 57594 [details] [review] Bug 17649 - Only one message per borrower and overdue level
2 identical messages are still created in message_queue when 'perl ./misc/cronjobs/overdue_notices.pl' is run, both containing both I1 and I2. Both messages are sent from B1 with B1 addresses.
(In reply to Mika from comment #2) > 2 identical messages are still created in message_queue when 'perl > ./misc/cronjobs/overdue_notices.pl' is run, both containing both I1 and I2. > Both messages are sent from B1 with B1 addresses. Did you run overdue_notices.pl with the option -a that is introduced by the patch ? perl ./misc/cronjobs/overdue_notices.pl -a B2CODE I realize that my test plan was confusing as far as step 6 is concerned and the use of this new option. So here is a new test plan with corrected step 6 NEW TEST PLAN : 0/ Have (or create) at least two branches B1 and B2. Have (or create) a default notice for all branches in Notices and Slips Module 1/ Make sure you're logged in with an account belonging to B1, or switch to branch B1 2/ checkout item I1 (belonging to B1) to patron P (preferably while specifying a due date < today) 3/ switch to branch B2 4/ checkout item I2 (belonging to B2) to patron P (preferably while specifying a due date < today) 5/ make sure you have defined "Overdue notice/status triggers" for that patron category, ie with Delay=1 so that "Overdue Notice" is selected and "Email" is checked. 6/ from cli run 'perl ./misc/cronjobs/overdue_notices.pl' (optionally specifying also '-library B1 -library B2' ) 7/ Two new entries will be generated in message_queue DB table, coming from different branches that have the SAME list of overdue items 8/ either see the 'content' field in these entries, or run 'perl ./cronjobs/process_message_queue.pl' to receive the actual emails (provided the email is properly setup). Apply patch redo step 6 running the script with the new option -a (all_in_one). this option expect as mandatory parameter the branchcode of the library that you want to make sender of the message. say that 'B2CODE' is the branchcode of B2 branch, it will run like this : perl ./misc/cronjobs/overdue_notices.pl -a B2CODE You will see that only one message had been created. Its content shows both B1 and B2 overdue items and the sender address matches B2 address as well. Olivier crouzet
Created attachment 58398 [details] [review] Bug 17649 - Only one message per borrower and overdue level The patch had been modified to be compatible with bug 17470.
Looking at the test plan, it seems like it would be odd for B2 to send out a notice regarding B1 items. 1) What is the logic behind the notice coming from B2? 2) If there are multiple items overdue from multiple branches, should there be an option for notices to come from a neutral branch or maybe the patron's home branch? If you are going to make the notices more efficient (which I think we should), it might be a good place to introduce some new logic to the notices. Perhaps have the notice come from the patron's home branch, and have the items grouped by item.homebranch with branch heading? I don't think either of these would take a lot to do, and would make this a cleaner outcome of reducing the number of notices. Just a thought. The grouping is not absolutely necessary, but I do think a single notice should come from the patron's branch, or it should be a preference setting (overdueNoticeSender).
Please don't forget to assign your bugs to yourself :)
Conflict in misc/cronjobs/overdue_notices.pl <<<<<<< c967e010c0110e562c4cc71bc5d18837a2a956a0 AND issues.branchcode = ? AND items.itemlost = 0 ||||||| merged common ancestors AND issues.branchcode = ? ======= >>>>>>> Bug 17649 Only one message per borrower and overdue level Should be simple to solve. But why AND issues.branchcode = ? is removed in this patch?
Hello Victor, it 's not removed, it's related to "--all_in_one" option : $overduequery.= " AND issues.branchcode = ?" unless $all_in_one;
Indeed, it was just moved bellow :)
Created attachment 80925 [details] [review] Bug 17649 Only one message per borrower and overdue level The added option (-a|--all_in_one) allows to create only one message per borrower and delay level including overdues from whatever branches. A branchcode value (branches.branchcode) has to be given to choose which sender address will be set.
Rebased and fixed conflict. Easy to solve and shouldn't invalidate the signoff. SELECT biblio.*, items.*, issues.*, biblioitems.itemtype, branchname FROM issues,items,biblio, biblioitems, branches b WHERE items.itemnumber=issues.itemnumber AND biblio.biblionumber = items.biblionumber AND b.branchcode = items.homebranch AND biblio.biblionumber = biblioitems.biblionumber AND issues.borrowernumber = ? <<<<<<< 17e5691daaa0f6b73623198eded648c78e40056a AND issues.branchcode = ? AND items.itemlost = 0 ||||||| merged common ancestors AND issues.branchcode = ? ======= >>>>>>> Bug 17649 Only one message per borrower and overdue level AND TO_DAYS($date)-TO_DAYS(issues.date_due) >= 0 END_SQL $overduequery.= " AND issues.branchcode = ?" unless $all_in_one; my $sth2 = $dbh->prepare($overduequery);
I got confused with another patch. This one wasn't already signed off. So it needs one :) @Lyon 3 can I put the test plan from in comment #3 in the patch? Or is there a reason for it to not be there yet?
(In reply to Victor Grousset/tuxayo from comment #12) > I got confused with another patch. This one wasn't already signed off. So it > needs one :) > > @Lyon 3 can I put the test plan from in comment #3 in the patch? Or is there > a reason for it to not be there yet? Bonjour Victor Yes you can, with just a change for steps 2 and 3. Rather than "(preferably while specifying a due date < today)", that should be "specifying as due date the day before (1 day overdue)" Olivier
Can you respond to comment 5?
(In reply to Christopher Brannon from comment #14) > Can you respond to comment 5? The all_in_one option is precisely meant for libraries that have a centralized management of claims (and in any case, you are free to detail in the notice where patrons may check in the items). So, I don't think that patron's branch notion makes much sense. Olivier Crouzet
Can yopu rebase this on the master? Thank you.
Just looking into this a little bit: I think sending the "all branches all items" notice from the patron's homebranch would make a lot of sense as this is how the DUEDGST and PREDUEDGST also operate.
Created attachment 125298 [details] [review] Bug 17649: Only one message per borrower and overdue level The added option (-a|--all_in_one) allows to create only one message per borrower and delay level including overdues from whatever branches. A branchcode value (branches.branchcode) has to be given to choose which sender address will be set. == test plan == 0/ Have (or create) at least two branches B1 and B2. Have (or create) a default notice for all branches in Notices and Slips Module 1/ Make sure you're logged in with an account belonging to B1, or switch to branch B1 2/ checkout item I1 (belonging to B1) to patron P (specifying as due date the day before (1 day overdue)) 3/ switch to branch B2 4/ checkout item I2 (belonging to B2) to patron P (specifying as due date the day before (1 day overdue)) 5/ make sure you have defined "Overdue notice/status triggers" for that patron category, ie with Delay=1 so that "Overdue Notice" is selected and "Email" is checked. 6/ from cli run 'perl ./misc/cronjobs/overdue_notices.pl' (optionally specifying also '-library B1 -library B2' ) 7/ Two new entries will be generated in message_queue DB table, coming from different branches that have the SAME list of overdue items 8/ either see the 'content' field in these entries, or run 'perl ./cronjobs/process_message_queue.pl' to receive the actual emails (provided the email is properly setup). Apply patch redo step 6 running the script with the new option -a (all_in_one). this option expect as mandatory parameter the branchcode of the library that you want to make sender of the message. say that 'B2CODE' is the branchcode of B2 branch, it will run like this : perl ./misc/cronjobs/overdue_notices.pl -a B2CODE You will see that only one message had been created. Its content shows both B1 and B2 overdue items and the sender address matches B2 address as well. Rebased-on: 2021-09-24 Victor Grousset/tuxayo <victor@tuxayo.net>
Hi :) I'm not working on this anymore but since I did a rebase in the past, here is another one. The test plan of Olivier has been integrated with the requested changes(comment 13) It has not been tested, I'm just solving conflicts :P
Also, there are been some changes in Koha that require consideration: issues.branchcode was moved from the bulk of the SQL query to just after in an «if($owning_library)» https://git.koha-community.org/Koha-community/Koha/src/commit/9d6d641d1f8b77271800f43bc027b651f9aea52b/misc/cronjobs/overdue_notices.pl#L492 This patch historically removed issues.branchcode from the bulk of the SQL query to have instead «AND issues.branchcode = ?" unless $all_in_one;» So maybe these two aren't compatible as it is. Still worth testing the current patch to maybe expose that.
Patch doesn't apply
Created attachment 145695 [details] [review] Bug 17649: Only one message per borrower and overdue level The added option (-a|--all_in_one) allows to create only one message per borrower and delay level including overdues from whatever branches. A branchcode value (branches.branchcode) has to be given to choose which sender address will be set. == test plan == 0/ Have (or create) at least two branches B1 and B2. Have (or create) a default notice for all branches in Notices and Slips Module 1/ Make sure you're logged in with an account belonging to B1, or switch to branch B1 2/ checkout item I1 (belonging to B1) to patron P (specifying as due date the day before (1 day overdue)) 3/ switch to branch B2 4/ checkout item I2 (belonging to B2) to patron P (specifying as due date the day before (1 day overdue)) 5/ make sure you have defined "Overdue notice/status triggers" for that patron category, ie with Delay=1 so that "Overdue Notice" is selected and "Email" is checked. 6/ from cli run 'perl ./misc/cronjobs/overdue_notices.pl' (optionally specifying also '-library B1 -library B2' ) 7/ Two new entries will be generated in message_queue DB table, coming from different branches that have the SAME list of overdue items 8/ either see the 'content' field in these entries, or run 'perl ./cronjobs/process_message_queue.pl' to receive the actual emails (provided the email is properly setup). Apply patch redo step 6 running the script with the new option -a (all_in_one). this option expect as mandatory parameter the branchcode of the library that you want to make sender of the message. say that 'B2CODE' is the branchcode of B2 branch, it will run like this : perl ./misc/cronjobs/overdue_notices.pl -a B2CODE You will see that only one message had been created. Its content shows both B1 and B2 overdue items and the sender address matches B2 address as well.
Created attachment 148927 [details] [review] Bug 17649: Only one message per borrower and overdue level The added option (-a|--all_in_one) allows to create only one message per borrower and delay level including overdues from whatever branches. A branchcode value (branches.branchcode) has to be given to choose which sender address will be set. == test plan == 0/ Have (or create) at least two branches B1 and B2. Have (or create) a default notice for all branches in Notices and Slips Module 1/ Make sure you're logged in with an account belonging to B1, or switch to branch B1 2/ checkout item I1 (belonging to B1) to patron P (specifying as due date the day before (1 day overdue)) 3/ switch to branch B2 4/ checkout item I2 (belonging to B2) to patron P (specifying as due date the day before (1 day overdue)) 5/ make sure you have defined "Overdue notice/status triggers" for that patron category, ie with Delay=1 so that "Overdue Notice" is selected and "Email" is checked. 6/ from cli run 'perl ./misc/cronjobs/overdue_notices.pl' (optionally specifying also '-library B1 -library B2' ) 7/ Two new entries will be generated in message_queue DB table, coming from different branches that have the SAME list of overdue items 8/ either see the 'content' field in these entries, or run 'perl ./cronjobs/process_message_queue.pl' to receive the actual emails (provided the email is properly setup). Apply patch redo step 6 running the script with the new option -a (all_in_one). this option expect as mandatory parameter the branchcode of the library that you want to make sender of the message. say that 'B2CODE' is the branchcode of B2 branch, it will run like this : perl ./misc/cronjobs/overdue_notices.pl -a B2CODE You will see that only one message had been created. Its content shows both B1 and B2 overdue items and the sender address matches B2 address as well. Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised.
(In reply to Katrin Fischer from comment #17) > Just looking into this a little bit: I think sending the "all branches all > items" notice from the patron's homebranch would make a lot of sense as this > is how the DUEDGST and PREDUEDGST also operate. I agree with this, it would make sense to have the patron library be the sender of the notice, so the patron does not get a notice from some library they might not have a relationship to at all. Howeverm I do think this could be done as a followup and not be a blocker for the current patch.
Is the sending library that difficult to change before releasing? It would be nice to address this now, since it was brought up and discussed before signing off. Please? :)
Isn't there a pref for that (OverdueNoticeFrom)? Or am I misunderstanding the discussion.
(In reply to Caroline Cyr La Rose from comment #26) > Isn't there a pref for that (OverdueNoticeFrom)? Or am I misunderstanding > the discussion. Not that I can see. At least not in 22.05. But we are dealing specifically with grouped items, not who emails should be from in general.
Looked a bit more into this... (In reply to Christopher Brannon from comment #27) > (In reply to Caroline Cyr La Rose from comment #26) > > Isn't there a pref for that (OverdueNoticeFrom)? Or am I misunderstanding > > the discussion. > > Not that I can see. At least not in 22.05. But we are dealing specifically > with grouped items, not who emails should be from in general. There should be OverdueNoticeFrom, yes: https://koha-community.org/manual/22.11/en/html/circulationpreferences.html#overduenoticefrom "This system preference overrides the –frombranch parameter in the overdue_notices.pl cronjob. When set to “command-line option”, the library information will be that of the –frombranch parameter in the overdue_notices.pl cronjob or the checkout library if the parameter is absent. When set to “item home library”, the library information will be that of the overdue item’s home library. When set to “checkout library”, the library information will be that of the library at which the overdue item was checked out." There is also: Bug 32740 - Add a new option patron home library to OverdueNoticeFrom [Needs Signoff right now] I guess the problem is that with --all_in_one there can be more than one "item home library" and more than one "checkout library". To me it looks like it would make sense to either: - Set an explicit sender library for all messages, this should be possible with OverdueNoticeFrom = "command-line option" and "the --frombranch parameter in the overdue_notices.pl cronjob." - Set the patron home library as the sender (bug 32740) So when run with --all_in_one, that should only be an on/off switch for making sure all items are in one message, and the script should choose either the library in --frombranch or the patron home branch as the sender. Some combinations will not make sense, like having OverdueNoticeFrom = "item home library" and running with --all_in_one. Does that make sense? Setting to Failed QA.