Bug 14364

Summary: Allow automatically canceled expired waiting holds to fill the next hold
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Hold requestsAssignee: Kyle M Hall <kyle>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, amy, andrewfh, black23, caroline.cyr-la-rose, cbrannon, costalc, cslone, genevieve.beaudry, gmcharlt, gwilliams, hector.hecaxmmx, jesse, jonathan.druart, katrin.fischer, kyle.m.hall, lucas, marjorie.barry-vila, martin.renvoize, nick, nicole, nugged, rcoert, tomascohen, veron, vfaafia29, wizzyrea
Version: masterKeywords: rel_22_11_candidate
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10827
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Caroline Cyr La Rose Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/632
Text to go in the release notes:
Version(s) released in:
22.11.00
Bug Depends on:    
Bug Blocks: 32467, 32455    
Attachments: Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
[SIGNED-OFF] Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364: (QA followup)
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364: (QA followup)
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364: (QA followup)
Bug 14364 [QA Followup] - Add unit tests
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364: (QA follow-up) Generate message for transfers as well
Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364: (QA follow-up) Generate message for transfers as well
Bug 14364: (follow-up) Cleanup duplicate code and adjust notices and prefs
Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364: (QA follow-up) Generate message for transfers as well
Bug 14364: (follow-up) Cleanup duplicate code and adjust notices and prefs
Bug 14364: Fix spelling and tests
Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364: (QA follow-up) Generate message for transfers as well
Bug 14364: (follow-up) Cleanup duplicate code and adjust notices and prefs
Bug 14364: Fix spelling and tests
Bug 14364: (QA follow-up) Fix capitalization
Bug 14364: (QA follow-up) Add email validation to ExpireReservesAutoFillEmail
Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold
Bug 14364: (QA follow-up) Generate message for transfers as well
Bug 14364: (follow-up) Cleanup duplicate code and adjust notices and prefs
Bug 14364: Fix spelling and tests
Bug 14364: (QA follow-up) Fix capitalization
Bug 14364: (QA follow-up) Add email validation to ExpireReservesAutoFillEmail

Description Kyle M Hall 2015-06-08 15:24:26 UTC
Right now, if a library automatically cancels expired waiting holds, a librarian must still re-checkin an item to trap the next available hold for that item. It would be better if the next hold was automatically trapped and the librarians receive an email notification so they can make any changes to the item if need be ( hold area, hold slip in item, etc ).
Comment 1 Kyle M Hall 2015-06-08 16:10:51 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2015-07-30 15:35:07 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-08-05 08:29:02 UTC
I would like to get another signoff on this one please.
Comment 4 Christopher Brannon 2015-09-22 14:42:51 UTC
I'm not sure how many libraries would like this workflow.  Having the system to make changes to the item and notify a patron before the staff have a chance to handle it is not a common practice.  I haven't had a chance to test it yet, but wouldn't the staff grabbing the item and checking it in to generate a slip cause it to send out another notification?

Having a hold notify a patron before the item is prepped can introduce some issues.  For example, what if the person comes in before the staff have read the e-mail and processed the book.  How do other staff find the item?  We often have patrons come to the desk literally minutes after the notification goes out, and the patron happens to be in the library.  This produces a kink in the workflow and has the potential to frustrate both patron and staff.

What would be preferred is the following (at least at our library):

1) Waiting hold expires or is cancelled.
2) Staff alerted and item status is changed so as not to show that it is available, but the hold is over.
3) Staff grab books and check them in.
4) Koha sees that they have expired or cancelled holds and asks if you would like to move the item on to the next hold (if one exists), or renew the hold.
5) A new slip is generated in either case, and the item is moved on or back on the hold shelf.

This is just my opinion, but I think my suggested workflow is more acceptable.

I'm not changing the status on the enhancement to in discussion, since this is an option and not a fundamental change, but I'm not sure how many folks will go for this.

Christopher
Comment 5 HB-NEKLS 2015-09-22 16:45:03 UTC
As a multi-library consortia, this feature would be unusable for us, especially using a single email field in the syspref -- it would be much better to use the email address in the branches table and email the holdingbranch email address of the item when a hold awaiting pickup is canceled. 

Additionally, I agree with Christopher's suggested workflow, having seen patrons come in for holds awaiting pickup before the items even make it to the circ desk.
Comment 6 Kyle M Hall 2015-09-24 15:30:53 UTC
(In reply to Christopher Brannon from comment #4)
> I'm not sure how many libraries would like this workflow.  Having the system
> to make changes to the item and notify a patron before the staff have a
> chance to handle it is not a common practice.  I haven't had a chance to
> test it yet, but wouldn't the staff grabbing the item and checking it in to
> generate a slip cause it to send out another notification?

Since the hold is already waiting at that point, another notification would not be sent.

> Having a hold notify a patron before the item is prepped can introduce some
> issues.  For example, what if the person comes in before the staff have read
> the e-mail and processed the book.  How do other staff find the item?  We
> often have patrons come to the desk literally minutes after the notification
> goes out, and the patron happens to be in the library.  This produces a kink
> in the workflow and has the potential to frustrate both patron and staff.
> 
> What would be preferred is the following (at least at our library):
> 
> 1) Waiting hold expires or is cancelled.
> 2) Staff alerted and item status is changed so as not to show that it is
> available, but the hold is over.
> 3) Staff grab books and check them in.
> 4) Koha sees that they have expired or cancelled holds and asks if you would
> like to move the item on to the next hold (if one exists), or renew the hold.
> 5) A new slip is generated in either case, and the item is moved on or back
> on the hold shelf.
> 
> This is just my opinion, but I think my suggested workflow is more
> acceptable.

That is essentially Koha's existing holds workflow. The only difference would be to make item's as unavailable if the item would fill the next hold. It's not insurmountable, but would definitely be a separate feater that could live along side of this one ( in an either/or fashion ).

> I'm not changing the status on the enhancement to in discussion, since this
> is an option and not a fundamental change, but I'm not sure how many folks
> will go for this.

Input is always appreciated. Indeed this is an entirely optional workflow that I'm sure will work great for some libraries ( like the one that requested the feature ) and not at all for others ( like you and Heather ).
Comment 7 Kyle M Hall 2015-09-24 15:33:45 UTC
(In reply to Heather Braum from comment #5)
> As a multi-library consortia, this feature would be unusable for us,
> especially using a single email field in the syspref -- it would be much
> better to use the email address in the branches table and email the
> holdingbranch email address of the item when a hold awaiting pickup is
> canceled. 

The new email pref is entirely optional. If it is not filled in, the feature defaults to the branch email, and if that is not filled in, the koha admin email address.

> Additionally, I agree with Christopher's suggested workflow, having seen
> patrons come in for holds awaiting pickup before the items even make it to
> the circ desk.

Please see my notes on that above! I don't think it's an insurmountable issue even without additional features. I imagine the patron would walk up, say "You've got book X on hold for me", and then the librarian would grab the book, check it in, and see that it is indeed waiting for them. At that point the librarian could check out the book as usual. If I'm missing anything please let me know. It's been a while since I've worked the circ desk ; )
Comment 8 Katrin Fischer 2015-10-02 08:59:01 UTC
See comment #3 - I agree as there is some discussion here, another sign-off would be good.
Comment 9 Nick Clemens 2015-11-17 22:02:30 UTC
simple notice conflicts, less simple Reserves.pm conflict
Comment 10 Kyle M Hall 2015-11-25 16:03:57 UTC Comment hidden (obsolete)
Comment 11 Claudio Costales 2015-11-25 17:41:00 UTC
This solution fixes bug#10827 "List of holds does not update correctly". If you agree I will set it as RESOLVED.
Comment 12 Katrin Fischer 2015-11-25 17:43:10 UTC
(In reply to bondiurbano from comment #11)
> This solution fixes bug#10827 "List of holds does not update correctly". If
> you agree I will set it as RESOLVED.

Maybe better to mark it as a duplicate. I wouldn't set it resolved until a solution has made it into master.
Comment 13 Claudio Costales 2015-11-25 17:45:49 UTC
Thanks Katrin. I will do it in that way.
Comment 14 Claudio Costales 2015-11-25 17:48:23 UTC
*** Bug 10827 has been marked as a duplicate of this bug. ***
Comment 15 Jonathan Druart 2015-12-02 10:27:39 UTC
Still waiting for another signoff.
Comment 16 Nick Clemens 2015-12-02 23:16:21 UTC
The holds transfer as expected, and the patron gets the email but I am not getting the admin email.

I also got the error below when running misc/cronjobs/holds/cancel_expired_holds.pl:

DBD::mysql::st execute failed: Duplicate entry '6' for key 'PRIMARY' [for Statement "
            INSERT INTO old_reserves
            SELECT * FROM reserves
            WHERE  reserve_id = ?
        " with ParamValues: 0='6'] at C4/Reserves.pm line 1080.
Can't locate object method "new" via package "C4::Dates" (perhaps you forgot to load "C4::Dates"?) at C4/Reserves.pm line 2073.
Comment 17 Héctor Eduardo Castro Avalos 2015-12-03 04:11:16 UTC
(In reply to Nick Clemens from comment #16)
> The holds transfer as expected, and the patron gets the email but I am not
> getting the admin email.
> 
> I also got the error below when running
> misc/cronjobs/holds/cancel_expired_holds.pl:
> 
> DBD::mysql::st execute failed: Duplicate entry '6' for key 'PRIMARY' [for
> Statement "
>             INSERT INTO old_reserves
>             SELECT * FROM reserves
>             WHERE  reserve_id = ?
>         " with ParamValues: 0='6'] at C4/Reserves.pm line 1080.
> Can't locate object method "new" via package "C4::Dates" (perhaps you forgot
> to load "C4::Dates"?) at C4/Reserves.pm line 2073.

You're right Nick, patch Fail QA since Marc has been removed C4::Dates from system.

Kyle, please remove C4::Dates and use Koha::DateUtils instead.

substitute  => { today => C4::Dates->new()->output() }, in line 2077

I add here Marc since he removed the C4::Dates from Koha.
Comment 18 Nick Clemens 2016-03-30 22:02:59 UTC Comment hidden (obsolete)
Comment 19 Nick Clemens 2016-03-30 22:18:06 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2016-04-03 13:14:32 UTC
Fixing sequence of attached patches so they apply cleanly with git-bz.
Comment 21 Katrin Fischer 2016-04-03 13:15:23 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2016-04-03 13:40:04 UTC
I think this could be useful to some libraries as it is, but I agree, there are some catches. Some future variation could be: Cancel the old hold, inform the library, but do not send the information to the next patron yet (this would allow the library to take care of the hold, slip, etc.) before the patron appears at the circulation desk.

Some notes from the initial code review:

1) The new notice template includes
+Copy: <<items.copynumber>>
but doesn't include the callnumber. I think it should be amended - without the callnumber it's not possible to identify the item in question.
Also there are some capitalization issues:
Canceled Hold Available for Different Patron

2) From the system preferene it's not clear that this feature is tied to the automatic cancellation:
ExpireReservesAutoFill
yes: "Do"
no: "Don't"
automatically fill the next hold using the item. If enabled, an email notification will be sent to either the email address defined in 

3) The tests for for ModReserveAffect have been adapted, but there are no new  tests the change itself. There are also no changes to the tests for the changed:
CancelExpiredReserves
CancelReserve

Failing for 3) - please take care of adding tests for this new functionality relfecting the changes in the routines.
Comment 23 Kyle M Hall 2016-06-13 12:41:27 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2016-06-13 12:41:36 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2016-06-13 12:41:40 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2016-07-02 12:44:22 UTC
(In reply to Jonathan Druart from comment #3)
> I would like to get another signoff on this one please.

(In reply to Katrin Fischer from comment #8)
> See comment #3 - I agree as there is some discussion here, another sign-off
> would be good.

(In reply to Jonathan Druart from comment #15)
> Still waiting for another signoff.

And... still waiting...
Comment 27 Owen Leonard 2017-04-11 17:19:40 UTC
Sorry, patch doesn't apply. I'll test ASAP after rebased patches are attached.
Comment 28 Kyle M Hall 2018-02-02 13:53:48 UTC Comment hidden (obsolete)
Comment 29 Claudio Costales 2018-10-03 15:11:36 UTC
This is a long waiting enhancement for many libraries. It would be nice to continue with it... Is it possible?
Comment 30 Kyle M Hall 2018-10-05 14:54:36 UTC
Created attachment 80126 [details] [review]
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold

Right now, if a library automatically cancels expired waiting holds, a
librarian must still re-checkin an item to trap the next available hold
for that item. It would be better if the next hold was automatically
trapped and the librarians receive an email notification so they can
make any changes to the item if need be ( hold area, hold slip in item,
etc ).

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Create a record with one item
 4) Place two holds on that record
 5) Check in the item and set it to waiting for the first patron
 6) Set ReservesMaxPickUpDelay to 1
 7) Enable ExpireReservesMaxPickUpDelay
 8) Enable ExpireReservesAutoFill
 9) Set an email address in ExpireReservesAutoFillEmail
10) Modify the holds waitingdate to be in the past
11) Run misc/cronjobs/holds/cancel_expired_holds.pl
12) Note the hold is now waiting for the next patron
12) Note a waiting hold notification email was sent to that patron
13) Note a hold changed notification email was sent to the library
Comment 31 Genevieve Beaudry 2019-02-15 21:04:20 UTC
at step 11, I get the following error:

Use of inherited AUTOLOAD for non-method Koha::Hold::ModReserveAffect() is deprecated. This will be fatal in Perl 5.28 at /inlibro/git/koha-master-dev-inlibro/Koha/Hold.pm line 385.
Can't locate object method "_columns" via package "177333" (perhaps you forgot to load "177333"?) at /inlibro/git/koha-master-dev-inlibro/Koha/Object.pm line 443.
Comment 32 Liz Rea 2019-03-20 14:49:30 UTC
I just tested this, and it doesn't fail in this way any more, but the feature doesn't work either - please have another look!

Cheers,
Liz
Comment 33 Claudio Costales 2019-05-11 21:50:14 UTC
Any chance to have it in 19.05?
Comment 34 Katrin Fischer 2019-05-11 22:21:18 UTC
Hi Claudio, we have reached 'feature freeze' - this means no new features will be pushed for 19.05 apart from a select few that the RM has marked with rel_19_05_candidate. See here:
http://lists.koha-community.org/pipermail/koha-devel/2019-April/045179.html
So chances are rather slim right now.
Comment 35 Claudio Costales 2019-05-11 22:45:58 UTC
Many thanks Katrin. What a pity...
Comment 36 Claudio Costales 2019-08-13 10:39:32 UTC
Any chance to go on with this?
Comment 37 Katrin Fischer 2019-08-15 20:46:22 UTC
We'd need Kyle to fix the problems noted by Liz now to get this moving again.
Comment 38 Kyle M Hall 2020-04-20 13:08:42 UTC
Created attachment 103297 [details] [review]
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold

Right now, if a library automatically cancels expired waiting holds, a
librarian must still re-checkin an item to trap the next available hold
for that item. It would be better if the next hold was automatically
trapped and the librarians receive an email notification so they can
make any changes to the item if need be ( hold area, hold slip in item,
etc ).

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Create a record with one item
 4) Place two holds on that record
 5) Check in the item and set it to waiting for the first patron
 6) Set ReservesMaxPickUpDelay to 1
 7) Enable ExpireReservesMaxPickUpDelay
 8) Enable ExpireReservesAutoFill
 9) Set an email address in ExpireReservesAutoFillEmail
10) Modify the holds waitingdate to be in the past
11) Run misc/cronjobs/holds/cancel_expired_holds.pl
12) Note the hold is now waiting for the next patron
12) Note a waiting hold notification email was sent to that patron
13) Note a hold changed notification email was sent to the library
Comment 39 Andrew Fuerste-Henry 2020-05-18 12:39:23 UTC
This patch is cranky about sample_notices.sql. I think it needs to get adapted to the new yaml format for that file?
Comment 40 Kyle M Hall 2020-05-18 12:53:53 UTC
Created attachment 105015 [details] [review]
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold

Right now, if a library automatically cancels expired waiting holds, a
librarian must still re-checkin an item to trap the next available hold
for that item. It would be better if the next hold was automatically
trapped and the librarians receive an email notification so they can
make any changes to the item if need be ( hold area, hold slip in item,
etc ).

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Create a record with one item
 4) Place two holds on that record
 5) Check in the item and set it to waiting for the first patron
 6) Set ReservesMaxPickUpDelay to 1
 7) Enable ExpireReservesMaxPickUpDelay
 8) Enable ExpireReservesAutoFill
 9) Set an email address in ExpireReservesAutoFillEmail
10) Modify the holds waitingdate to be in the past
11) Run misc/cronjobs/holds/cancel_expired_holds.pl
12) Note the hold is now waiting for the next patron
12) Note a waiting hold notification email was sent to that patron
13) Note a hold changed notification email was sent to the library
Comment 41 Victoria Faafia 2021-01-19 23:02:23 UTC
Applying: Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
Using index info to reconstruct a base tree...
M	C4/Reserves.pm
M	Koha/Hold.pm
M	installer/data/mysql/pl-PL/mandatory/sample_notices.sql
A	installer/data/mysql/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging installer/data/mysql/pl-PL/mandatory/sample_notices.sql
CONFLICT (content): Merge conflict in installer/data/mysql/pl-PL/mandatory/sample_notices.sql
Auto-merging installer/data/mysql/mandatory/sysprefs.sql
Auto-merging Koha/Hold.pm
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-14364---Allow-automatically-canceled-expired-w-4l6wf_.patch

patch doesn't apply
Comment 42 Kyle M Hall 2021-01-20 12:26:56 UTC
Created attachment 115427 [details] [review]
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold

Right now, if a library automatically cancels expired waiting holds, a
librarian must still re-checkin an item to trap the next available hold
for that item. It would be better if the next hold was automatically
trapped and the librarians receive an email notification so they can
make any changes to the item if need be ( hold area, hold slip in item,
etc ).

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Create a record with one item
 4) Place two holds on that record
 5) Check in the item and set it to waiting for the first patron
 6) Set ReservesMaxPickUpDelay to 1
 7) Enable ExpireReservesMaxPickUpDelay
 8) Enable ExpireReservesAutoFill
 9) Set an email address in ExpireReservesAutoFillEmail
10) Modify the holds waitingdate to be in the past
11) Run misc/cronjobs/holds/cancel_expired_holds.pl
12) Note the hold is now waiting for the next patron
12) Note a waiting hold notification email was sent to that patron
13) Note a hold changed notification email was sent to the library
Comment 43 Victoria Faafia 2021-01-20 21:49:13 UTC
Created attachment 115466 [details] [review]
Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold

Right now, if a library automatically cancels expired waiting holds, a
librarian must still re-checkin an item to trap the next available hold
for that item. It would be better if the next hold was automatically
trapped and the librarians receive an email notification so they can
make any changes to the item if need be ( hold area, hold slip in item,
etc ).

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Create a record with one item
 4) Place two holds on that record
 5) Check in the item and set it to waiting for the first patron
 6) Set ReservesMaxPickUpDelay to 1
 7) Enable ExpireReservesMaxPickUpDelay
 8) Enable ExpireReservesAutoFill
 9) Set an email address in ExpireReservesAutoFillEmail
10) Modify the holds waitingdate to be in the past
11) Run misc/cronjobs/holds/cancel_expired_holds.pl
12) Note the hold is now waiting for the next patron
12) Note a waiting hold notification email was sent to that patron
13) Note a hold changed notification email was sent to the library

Signed-off-by: Victoria Faafia <vfaafia29@gmail.com>
Comment 44 Katrin Fischer 2021-05-02 14:03:25 UTC
I am sorry, this no longer applies:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
Using index info to reconstruct a base tree...
M	C4/Reserves.pm
M	Koha/Hold.pm
M	installer/data/mysql/mandatory/sysprefs.sql
M	installer/data/mysql/pl-PL/mandatory/sample_notices.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging installer/data/mysql/pl-PL/mandatory/sample_notices.sql
CONFLICT (content): Merge conflict in installer/data/mysql/pl-PL/mandatory/sample_notices.sql
Auto-merging installer/data/mysql/mandatory/sysprefs.sql
Auto-merging Koha/Hold.pm
CONFLICT (content): Merge conflict in Koha/Hold.pm
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-14364---Allow-automatically-canceled-expired-w-834arw.patch
Comment 45 Nick Clemens 2021-05-05 13:12:25 UTC
Created attachment 120494 [details] [review]
Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold

Right now, if a library automatically cancels expired waiting holds, a
librarian must still re-checkin an item to trap the next available hold
for that item. It would be better if the next hold was automatically
trapped and the librarians receive an email notification so they can
make any changes to the item if need be ( hold area, hold slip in item,
etc ).

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Create a record with one item
 4) Place two holds on that record
 5) Check in the item and set it to waiting for the first patron
 6) Set ReservesMaxPickUpDelay to 1
 7) Enable ExpireReservesMaxPickUpDelay
 8) Enable ExpireReservesAutoFill
 9) Set an email address in ExpireReservesAutoFillEmail
10) Modify the holds waitingdate to be in the past
11) Run misc/cronjobs/holds/cancel_expired_holds.pl
12) Note the hold is now waiting for the next patron
12) Note a waiting hold notification email was sent to that patron
13) Note a hold changed notification email was sent to the library

Signed-off-by: Victoria Faafia <vfaafia29@gmail.com>
Comment 46 Nick Clemens 2021-05-05 14:10:12 UTC
Rebased, but I think we need an update here - the current patches rely on _koha_notify_hold to send the message to the library. That routine is not called when the hold is transferred, I think we need a unique subroutine for that message that is called whenever the hold is changed, whether transferred or waiting
Comment 47 Katrin Fischer 2021-05-05 18:30:31 UTC
(In reply to Nick Clemens from comment #46)
> Rebased, but I think we need an update here - the current patches rely on
> _koha_notify_hold to send the message to the library. That routine is not
> called when the hold is transferred, I think we need a unique subroutine for
> that message that is called whenever the hold is changed, whether
> transferred or waiting

Thx Nick, also for noticing this!
Comment 48 Kyle M Hall 2021-05-13 16:05:21 UTC
Created attachment 120941 [details] [review]
Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold

Right now, if a library automatically cancels expired waiting holds, a
librarian must still re-checkin an item to trap the next available hold
for that item. It would be better if the next hold was automatically
trapped and the librarians receive an email notification so they can
make any changes to the item if need be ( hold area, hold slip in item,
etc ).

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Create a record with one item
 4) Place two holds on that record
 5) Check in the item and set it to waiting for the first patron
 6) Set ReservesMaxPickUpDelay to 1
 7) Enable ExpireReservesMaxPickUpDelay
 8) Enable ExpireReservesAutoFill
 9) Set an email address in ExpireReservesAutoFillEmail
10) Modify the holds waitingdate to be in the past
11) Run misc/cronjobs/holds/cancel_expired_holds.pl
12) Note the hold is now waiting for the next patron
12) Note a waiting hold notification email was sent to that patron
13) Note a hold changed notification email was sent to the library

Signed-off-by: Victoria Faafia <vfaafia29@gmail.com>
Comment 49 Kyle M Hall 2021-05-13 16:05:34 UTC
Created attachment 120942 [details] [review]
Bug 14364: (QA follow-up) Generate message for transfers as well
Comment 50 Katrin Fischer 2021-07-25 00:53:09 UTC
Hi Kyle,

1) For the From address, please use the $library->inbound_email_address routine:

+        my $email =
+             C4::Context->preference('ExpireReservesAutoFillEmail')
+          || $library->{branchemail}
+          || C4::Context->preference('KohaAdminEmailAddress');

It will take the Reply-To addresses into account and make the behaviour match with other notices sent to the library.

2) Duplicated code

It looks like the code for sending the HOLD_CHANGED email is duplicated in sub _koha_notify_reserve and _koha_notify_hold_changed multiple times?

(also reserve should always be hold)

3) Improve sample email

I believe the copynumber is not helpful, we should include the itemcallnumber instead.

Please fix capitalization "Canceled Hold Available for Different Patron".

4) Pref descriptions

Please link ExpireReservesAutoFillEmail :)

Please adjust description for the change in 1).

"Send email notification of the new hold filled with a canceled item to"

Does canceled item work here? Maybe "hold filled with an expired hold item" instead?

5) Sample notices missing

You've only added the new notice to nb-NO, but we need it in various other files as well. See xt/sample_notices.t. It gives a bit confusing advice right now as it thinks the notice has to be deleted, but it has to be added to installer/data/mysql/en/mandatory/sample_notices.yml first and then the test can be rerun.
Comment 51 Nick Clemens 2021-12-20 14:05:11 UTC
Created attachment 128757 [details] [review]
Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold

Right now, if a library automatically cancels expired waiting holds, a
librarian must still re-checkin an item to trap the next available hold
for that item. It would be better if the next hold was automatically
trapped and the librarians receive an email notification so they can
make any changes to the item if need be ( hold area, hold slip in item,
etc ).

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Create a record with one item
 4) Place two holds on that record
 5) Check in the item and set it to waiting for the first patron
 6) Set ReservesMaxPickUpDelay to 1
 7) Enable ExpireReservesMaxPickUpDelay
 8) Enable ExpireReservesAutoFill
 9) Set an email address in ExpireReservesAutoFillEmail
10) Modify the holds waitingdate to be in the past
11) Run misc/cronjobs/holds/cancel_expired_holds.pl
12) Note the hold is now waiting for the next patron
12) Note a waiting hold notification email was sent to that patron
13) Note a hold changed notification email was sent to the library

Signed-off-by: Victoria Faafia <vfaafia29@gmail.com>
Comment 52 Nick Clemens 2021-12-20 14:05:16 UTC
Created attachment 128758 [details] [review]
Bug 14364: (QA follow-up) Generate message for transfers as well
Comment 53 Nick Clemens 2021-12-20 14:05:21 UTC
Created attachment 128759 [details] [review]
Bug 14364: (follow-up) Cleanup duplicate code and adjust notices and prefs

This patch removes a duplicated stanza left form moving routine
Changes the routines to use inbound_library_address
Improves the display if the system preferences
Updates the update file
Moves smaple notice
Comment 54 Katrin Fischer 2022-03-14 22:14:38 UTC
I am sorry, but patches no longer apply, could you please rebase?

Bug 14364 - Allow automatically canceled expired waiting holds to fill the next hold

128757 - Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold
128758 - Bug 14364: (QA follow-up) Generate message for transfers as well
128759 - Bug 14364: (follow-up) Cleanup duplicate code and adjust notices and prefs

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold
Using index info to reconstruct a base tree...
M	C4/Reserves.pm
M	Koha/Hold.pm
M	installer/data/mysql/mandatory/sysprefs.sql
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging installer/data/mysql/mandatory/sysprefs.sql
Auto-merging Koha/Hold.pm
Auto-merging C4/Reserves.pm
Applying: Bug 14364: (QA follow-up) Generate message for transfers as well
Applying: Bug 14364: (follow-up) Cleanup duplicate code and adjust notices and prefs
error: sha1 information is lacking or useless (C4/Reserves.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 14364: (follow-up) Cleanup duplicate code and adjust notices and prefs
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-14364-follow-up-Cleanup-duplicate-code-and-adj-QGOiaL.patch
Comment 55 Nick Clemens 2022-03-22 13:34:14 UTC
Created attachment 132019 [details] [review]
Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold

Right now, if a library automatically cancels expired waiting holds, a
librarian must still re-checkin an item to trap the next available hold
for that item. It would be better if the next hold was automatically
trapped and the librarians receive an email notification so they can
make any changes to the item if need be ( hold area, hold slip in item,
etc ).

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Create a record with one item
 4) Place two holds on that record
 5) Check in the item and set it to waiting for the first patron
 6) Set ReservesMaxPickUpDelay to 1
 7) Enable ExpireReservesMaxPickUpDelay
 8) Enable ExpireReservesAutoFill
 9) Set an email address in ExpireReservesAutoFillEmail
10) Modify the holds waitingdate to be in the past
11) Run misc/cronjobs/holds/cancel_expired_holds.pl
12) Note the hold is now waiting for the next patron
12) Note a waiting hold notification email was sent to that patron
13) Note a hold changed notification email was sent to the library

Signed-off-by: Victoria Faafia <vfaafia29@gmail.com>
Comment 56 Nick Clemens 2022-03-22 13:34:19 UTC
Created attachment 132020 [details] [review]
Bug 14364: (QA follow-up) Generate message for transfers as well
Comment 57 Nick Clemens 2022-03-22 13:34:25 UTC
Created attachment 132021 [details] [review]
Bug 14364: (follow-up) Cleanup duplicate code and adjust notices and prefs

This patch removes a duplicated stanza left form moving routine
Changes the routines to use inbound_library_address
Improves the display if the system preferences
Updates the update file
Moves smaple notice
Comment 58 Katrin Fischer 2022-04-24 11:25:07 UTC
1) Capitalization

It's all good, but in the database update statement:

Canceled Hold Available for Different Patron', '0', 'Canceled Hold Available for Different Patron', 'The patron picking up <<biblio.title>> (<<items.barcode>>) has changed to <<borrowers.firstname>> <<borrowers.surname>> (<<borrowers.cardnumber

2) Spelling

Should this be reply-to address?

+            - . If no address is defined here, the email will be sent to the library's reply to address.


2) Tests are failing :(

prove t/db_dependent/Holds/ExpireReservesAutoFill.t
t/db_dependent/Holds/ExpireReservesAutoFill.t .. 1/3     # No tests run!
t/db_dependent/Holds/ExpireReservesAutoFill.t .. 2/3 
#   Failed test 'No tests run for subtest "Test automatically canceled expired waiting holds to fill the next hold, without a transfer"'
#   at t/db_dependent/Holds/ExpireReservesAutoFill.t line 131.
Undefined subroutine &main::CancelExpiredReserves called at t/db_dependent/Holds/ExpireReservesAutoFill.t line 101.
# Looks like your test exited with 11 just after 2.
t/db_dependent/Holds/ExpireReservesAutoFill.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 2/3 subtests 

Test Summary Report
-------------------
t/db_dependent/Holds/ExpireReservesAutoFill.t (Wstat: 2816 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 3 tests but ran 2.
Files=1, Tests=2,  3 wallclock secs ( 0.03 usr  0.00 sys +  2.66 cusr  0.30 csys =  2.99 CPU)
Result: FAIL
Comment 59 Nick Clemens 2022-04-25 14:43:42 UTC
Created attachment 133812 [details] [review]
Bug 14364: Fix spelling and tests

Moved the installer to pl file
Adjusted capitalization / reply-to
Fix tests by importing needed fuction and adjusting a wrong call
Comment 60 Katrin Fischer 2022-04-26 14:55:38 UTC
Created attachment 133923 [details] [review]
Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold

Right now, if a library automatically cancels expired waiting holds, a
librarian must still re-checkin an item to trap the next available hold
for that item. It would be better if the next hold was automatically
trapped and the librarians receive an email notification so they can
make any changes to the item if need be ( hold area, hold slip in item,
etc ).

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Create a record with one item
 4) Place two holds on that record
 5) Check in the item and set it to waiting for the first patron
 6) Set ReservesMaxPickUpDelay to 1
 7) Enable ExpireReservesMaxPickUpDelay
 8) Enable ExpireReservesAutoFill
 9) Set an email address in ExpireReservesAutoFillEmail
10) Modify the holds waitingdate to be in the past
11) Run misc/cronjobs/holds/cancel_expired_holds.pl
12) Note the hold is now waiting for the next patron
12) Note a waiting hold notification email was sent to that patron
13) Note a hold changed notification email was sent to the library

Signed-off-by: Victoria Faafia <vfaafia29@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 61 Katrin Fischer 2022-04-26 14:55:45 UTC
Created attachment 133924 [details] [review]
Bug 14364: (QA follow-up) Generate message for transfers as well

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 62 Katrin Fischer 2022-04-26 14:55:52 UTC
Created attachment 133925 [details] [review]
Bug 14364: (follow-up) Cleanup duplicate code and adjust notices and prefs

This patch removes a duplicated stanza left form moving routine
Changes the routines to use inbound_library_address
Improves the display if the system preferences
Updates the update file
Moves smaple notice

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 63 Katrin Fischer 2022-04-26 14:55:59 UTC
Created attachment 133926 [details] [review]
Bug 14364: Fix spelling and tests

Moved the installer to pl file
Adjusted capitalization / reply-to
Fix tests by importing needed fuction and adjusting a wrong call

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 64 Katrin Fischer 2022-04-26 14:56:06 UTC
Created attachment 133927 [details] [review]
Bug 14364: (QA follow-up) Fix capitalization

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 65 Katrin Fischer 2022-04-26 14:56:14 UTC
Created attachment 133928 [details] [review]
Bug 14364: (QA follow-up) Add email validation to ExpireReservesAutoFillEmail

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 66 Tomás Cohen Arazi 2022-07-20 13:23:15 UTC
Please rebase.
Comment 67 Kyle M Hall 2022-07-20 14:25:52 UTC
Created attachment 137927 [details] [review]
Bug 14364: Allow automatically canceled expired waiting holds to fill the next hold

Right now, if a library automatically cancels expired waiting holds, a
librarian must still re-checkin an item to trap the next available hold
for that item. It would be better if the next hold was automatically
trapped and the librarians receive an email notification so they can
make any changes to the item if need be ( hold area, hold slip in item,
etc ).

Test Plan:
 1) Apply this patch
 2) Run updatedatabase.pl
 3) Create a record with one item
 4) Place two holds on that record
 5) Check in the item and set it to waiting for the first patron
 6) Set ReservesMaxPickUpDelay to 1
 7) Enable ExpireReservesMaxPickUpDelay
 8) Enable ExpireReservesAutoFill
 9) Set an email address in ExpireReservesAutoFillEmail
10) Modify the holds waitingdate to be in the past
11) Run misc/cronjobs/holds/cancel_expired_holds.pl
12) Note the hold is now waiting for the next patron
12) Note a waiting hold notification email was sent to that patron
13) Note a hold changed notification email was sent to the library

Signed-off-by: Victoria Faafia <vfaafia29@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 68 Kyle M Hall 2022-07-20 14:26:10 UTC
Created attachment 137928 [details] [review]
Bug 14364: (QA follow-up) Generate message for transfers as well

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 69 Kyle M Hall 2022-07-20 14:26:18 UTC
Created attachment 137929 [details] [review]
Bug 14364: (follow-up) Cleanup duplicate code and adjust notices and prefs

This patch removes a duplicated stanza left form moving routine
Changes the routines to use inbound_library_address
Improves the display if the system preferences
Updates the update file
Moves smaple notice

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 70 Kyle M Hall 2022-07-20 14:26:24 UTC
Created attachment 137930 [details] [review]
Bug 14364: Fix spelling and tests

Moved the installer to pl file
Adjusted capitalization / reply-to
Fix tests by importing needed fuction and adjusting a wrong call

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 71 Kyle M Hall 2022-07-20 14:26:30 UTC
Created attachment 137931 [details] [review]
Bug 14364: (QA follow-up) Fix capitalization

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 72 Kyle M Hall 2022-07-20 14:26:35 UTC
Created attachment 137932 [details] [review]
Bug 14364: (QA follow-up) Add email validation to ExpireReservesAutoFillEmail

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 73 Tomás Cohen Arazi 2022-07-29 19:08:41 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 74 Lucas Gass 2022-09-29 15:01:24 UTC
Enhancement will not be backported to 22.05.x series
Comment 75 Andrii Nugged 2022-12-13 03:40:11 UTC
> Changes the routines to use inbound_library_address

Ouch :). Now our all customers have all the rest messages sent from 

    $library->from_email_address;

_except_ 'HOLD's:

    $library->inbound_email_address

This slightly made a mess. 

Katrin/Nick: do we expect that "inbound_email_address" will be soon refactored to everywhere use?
Comment 76 Nick Clemens 2022-12-13 11:40:27 UTC
(In reply to Andrew Nugged from comment #75)
> > Changes the routines to use inbound_library_address
> 
> Ouch :). Now our all customers have all the rest messages sent from 
> 
>     $library->from_email_address;
> 
> _except_ 'HOLD's:
> 
>     $library->inbound_email_address
> 
> This slightly made a mess. 
> 
> Katrin/Nick: do we expect that "inbound_email_address" will be soon
> refactored to everywhere use?

I think the change to the 'from' is actually a mistake. 

Koha/Library.pm
171 NOTE: This is the address to use for 'reply_to' or 'to' fields; It should not usually be
172 used as the 'from' address for emails as it may lead to mail being caught by spam filters.
Comment 77 Katrin Fischer 2023-09-13 12:27:38 UTC
(In reply to Nick Clemens from comment #76)
> (In reply to Andrew Nugged from comment #75)
> > > Changes the routines to use inbound_library_address
> > 
> > Ouch :). Now our all customers have all the rest messages sent from 
> > 
> >     $library->from_email_address;
> > 
> > _except_ 'HOLD's:
> > 
> >     $library->inbound_email_address
> > 
> > This slightly made a mess. 
> > 
> > Katrin/Nick: do we expect that "inbound_email_address" will be soon
> > refactored to everywhere use?
> 
> I think the change to the 'from' is actually a mistake. 
> 
> Koha/Library.pm
> 171 NOTE: This is the address to use for 'reply_to' or 'to' fields; It
> should not usually be
> 172 used as the 'from' address for emails as it may lead to mail being
> caught by spam filters.

still valid?
Comment 78 Nick Clemens 2023-09-14 12:50:51 UTC
(In reply to Katrin Fischer from comment #77)
> (In reply to Nick Clemens from comment #76)
> > (In reply to Andrew Nugged from comment #75)
> > > > Changes the routines to use inbound_library_address
> > > 
> > > Ouch :). Now our all customers have all the rest messages sent from 
> > > 
> > >     $library->from_email_address;
> > > 
> > > _except_ 'HOLD's:
> > > 
> > >     $library->inbound_email_address
> > > 
> > > This slightly made a mess. 
> > > 
> > > Katrin/Nick: do we expect that "inbound_email_address" will be soon
> > > refactored to everywhere use?
> > 
> > I think the change to the 'from' is actually a mistake. 
> > 
> > Koha/Library.pm
> > 171 NOTE: This is the address to use for 'reply_to' or 'to' fields; It
> > should not usually be
> > 172 used as the 'from' address for emails as it may lead to mail being
> > caught by spam filters.
> 
> still valid?

See bug 32455