Bug 17649 - Create only one message per borrower and overdue level
Summary: Create only one message per borrower and overdue level
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal with 5 votes (vote)
Assignee: Koha Team University Lyon 3
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-17 11:03 UTC by Koha Team University Lyon 3
Modified: 2023-03-30 08:34 UTC (History)
17 users (show)

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


Attachments
Bug 17649 - Only one message per borrower and overdue level (4.41 KB, patch)
2016-11-17 11:08 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 17649 - Only one message per borrower and overdue level (5.36 KB, patch)
2016-12-22 17:19 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 17649 Only one message per borrower and overdue level (5.29 KB, patch)
2018-10-19 10:13 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 17649: Only one message per borrower and overdue level (6.24 KB, patch)
2021-09-24 19:50 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 17649: Only one message per borrower and overdue level (6.24 KB, patch)
2023-01-26 14:56 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 17649: Only one message per borrower and overdue level (6.31 KB, patch)
2023-03-29 14:39 UTC, Magnus Enger
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Koha Team University Lyon 3 2016-11-17 11:03:41 UTC
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
Comment 1 Koha Team University Lyon 3 2016-11-17 11:08:21 UTC Comment hidden (obsolete)
Comment 2 Mika 2016-12-01 01:24:52 UTC
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.
Comment 3 Koha Team University Lyon 3 2016-12-05 08:26:38 UTC
(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
Comment 4 Koha Team University Lyon 3 2016-12-22 17:19:23 UTC Comment hidden (obsolete)
Comment 5 Christopher Brannon 2017-04-08 21:58:44 UTC
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).
Comment 6 Katrin Fischer 2017-10-08 11:22:30 UTC
Please don't forget to assign your bugs to yourself :)
Comment 7 Victor Grousset/tuxayo 2018-06-21 16:29:35 UTC
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?
Comment 8 Koha Team University Lyon 3 2018-06-22 06:53:26 UTC
Hello Victor,
it 's not removed, it's related to "--all_in_one" option :
$overduequery.= " AND issues.branchcode = ?" unless $all_in_one;
Comment 9 Victor Grousset/tuxayo 2018-06-22 07:12:01 UTC
Indeed, it was just moved bellow :)
Comment 10 Victor Grousset/tuxayo 2018-10-19 10:13:45 UTC Comment hidden (obsolete)
Comment 11 Victor Grousset/tuxayo 2018-10-19 10:16:09 UTC
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);
Comment 12 Victor Grousset/tuxayo 2018-10-19 10:23:24 UTC
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?
Comment 13 Koha Team University Lyon 3 2018-10-19 12:26:50 UTC
(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
Comment 14 Christopher Brannon 2018-11-08 19:12:44 UTC
Can you respond to comment 5?
Comment 15 Koha Team University Lyon 3 2018-11-09 11:11:42 UTC
(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
Comment 16 Michal Denar 2019-04-27 19:38:03 UTC
Can yopu rebase this on the master?

Thank you.
Comment 17 Katrin Fischer 2021-09-15 13:01:28 UTC
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.
Comment 18 Victor Grousset/tuxayo 2021-09-24 19:50:01 UTC Comment hidden (obsolete)
Comment 19 Victor Grousset/tuxayo 2021-09-24 20:39:12 UTC
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
Comment 20 Victor Grousset/tuxayo 2021-09-24 20:49:53 UTC
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.
Comment 21 Caroline Cyr La Rose 2023-01-23 17:01:01 UTC
Patch doesn't apply
Comment 22 Hammat wele 2023-01-26 14:56:27 UTC Comment hidden (obsolete)
Comment 23 Magnus Enger 2023-03-29 14:39:57 UTC
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.
Comment 24 Magnus Enger 2023-03-29 14:44:53 UTC
(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.
Comment 25 Christopher Brannon 2023-03-29 17:17:16 UTC
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?  :)
Comment 26 Caroline Cyr La Rose 2023-03-29 17:25:16 UTC
Isn't there a pref for that (OverdueNoticeFrom)? Or am I misunderstanding the discussion.
Comment 27 Christopher Brannon 2023-03-29 20:07:00 UTC
(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.
Comment 28 Magnus Enger 2023-03-30 08:31:56 UTC
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.