Bug 8367 - How long is a hold waiting for pickup at a more granular level
Summary: How long is a hold waiting for pickup at a more granular level
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 8362 10487
  Show dependency treegraph
 
Reported: 2012-07-05 13:35 UTC by Paul Poulain
Modified: 2020-10-21 08:54 UTC (History)
27 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
This patch adds a new circulation rule - holds_pickup_period - which will override the value set in the ReservesMaxPickUpDelay system preference for specific item types, or patron categories, or branches.
Version(s) released in:


Attachments
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (23.22 KB, patch)
2012-09-11 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (24.74 KB, patch)
2012-10-31 13:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup (1.57 KB, patch)
2012-11-21 16:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (24.77 KB, patch)
2012-11-21 17:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup (1.57 KB, patch)
2012-11-21 17:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup (1.63 KB, patch)
2012-11-22 09:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8367: Add more granular level for ReservesMaxPickUpDelay (25.24 KB, patch)
2013-02-15 08:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup (1.67 KB, patch)
2013-02-15 08:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay (22.64 KB, patch)
2013-02-15 15:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay (22.60 KB, patch)
2013-02-18 12:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay (23.23 KB, patch)
2013-02-22 10:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay (23.29 KB, patch)
2013-02-25 17:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (2.66 KB, patch)
2013-02-25 18:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (25.50 KB, patch)
2013-06-18 08:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup (1.63 KB, patch)
2013-06-18 08:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Followup Add more granular level for ReservesMaxPickUpDelay (22.89 KB, patch)
2013-06-18 08:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (2.98 KB, patch)
2013-06-18 08:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (38.36 KB, patch)
2013-06-18 09:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (2.94 KB, patch)
2013-06-18 09:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (38.23 KB, patch)
2013-06-18 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (2.65 KB, patch)
2013-06-18 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Followup FIX a sql statement (bad conflict merge) (1.79 KB, patch)
2013-06-18 13:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8367: Add more granular level for ReservesMaxPickUpDelay (38.28 KB, patch)
2013-06-21 12:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (2.65 KB, patch)
2013-06-21 12:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8367: Followup FIX a sql statement (bad conflict merge) (1.85 KB, patch)
2013-06-21 12:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (2.77 KB, patch)
2013-07-03 09:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Fix the maxpickupdate when we set waitingdate to NOW() (1.85 KB, patch)
2013-07-03 09:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Fix the maxpickupdate when we set waitingdate to NOW() (1.90 KB, patch)
2013-07-03 10:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (38.47 KB, patch)
2013-08-13 13:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Add more granular level for ReservesMaxPickUpDelay (38.47 KB, patch)
2013-08-13 13:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Followup FIX a sql statement (bad conflict merge) (1.84 KB, patch)
2013-08-13 13:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Skip to next open day if max pickup date is a holiday (3.17 KB, patch)
2013-08-13 13:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367: Fix the maxpickupdate when we set waitingdate to NOW() (1.91 KB, patch)
2013-08-13 13:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8367 - How long is a hold waiting for pickup at a more granular level (42.82 KB, patch)
2014-10-27 11:17 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 8367 - How long is a hold waiting for pickup at a more granular level (60.17 KB, patch)
2014-10-28 14:13 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 8367 - How long is a hold waiting for pickup at a more granular level (62.05 KB, patch)
2014-10-29 08:51 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
using this to obsolete previous patches (1 bytes, text/plain)
2014-10-29 08:56 UTC, Olli-Antti Kivilahti
Details
Bug 8367 - How long is a hold waiting for pickup at a more granular level (61.97 KB, patch)
2014-10-29 13:46 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 8367 - How long is a hold waiting for pickup at a more granular level (62.58 KB, patch)
2014-10-29 17:29 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 8367 - How long is a hold waiting for pickup at a more granular level (58.32 KB, patch)
2015-09-18 13:13 UTC, j.kylmala
Details | Diff | Splinter Review
Bug 8367 - How long is a hold waiting for pickup at a more granular level (58.50 KB, patch)
2015-09-22 08:13 UTC, j.kylmala
Details | Diff | Splinter Review
Bug 8367: How long is a hold waiting for pickup at a more granular level (58.96 KB, patch)
2018-10-02 00:22 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm (26.22 KB, patch)
2018-10-02 00:25 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: How long is a hold waiting for pickup at a more granular level (58.96 KB, patch)
2018-10-02 01:28 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm (28.32 KB, patch)
2018-10-02 01:28 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: Removed unneccessary warns and commented code (1.33 KB, patch)
2018-10-02 01:30 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: Moved database update out of updatedatabase.pl into atomicupdates dir (5.03 KB, patch)
2018-10-02 22:10 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves() (4.87 KB, patch)
2018-10-02 22:12 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: Fixed how expirationdate is calculated when ExcludeHolidaysFromMaxPickUpDelay syspref is enabled (917 bytes, patch)
2018-10-03 00:17 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: reserves.lastpickupdate calculation checks and uses ExcludeHolidaysFromMaxPickUpDelay syspref (1.37 KB, patch)
2018-10-03 05:01 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 8367: How long is a hold waiting for pickup at a more granular level (58.19 KB, patch)
2019-08-21 23:41 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: How long is a hold waiting for pickup at a more granular level (60.47 KB, patch)
2019-08-21 23:45 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm (26.39 KB, patch)
2019-08-21 23:46 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: Removed unneccessary warns and commented code (1.36 KB, patch)
2019-08-21 23:47 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm (26.39 KB, patch)
2019-08-21 23:47 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: Removed unneccessary warns and commented code (1.36 KB, patch)
2019-08-21 23:49 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: Moved database update out of updatedatabase.pl into atomicupdates dir (5.16 KB, patch)
2019-08-21 23:51 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves() (4.41 KB, patch)
2019-08-21 23:52 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: Fixed how expirationdate is calculated when ExcludeHolidaysFromMaxPickUpDelay syspref is enabled (926 bytes, patch)
2019-08-21 23:53 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: reserves.lastpickupdate calculation checks and uses ExcludeHolidaysFromMaxPickUpDelay syspref (1.38 KB, patch)
2019-08-21 23:54 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: (follow-up) Updates to DBIx class files (8.68 KB, patch)
2019-08-21 23:55 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: Fixes to make the reserves workflow work with new schema (2.85 KB, patch)
2019-08-21 23:56 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: Fixing display of lastpickupdate (4.82 KB, patch)
2019-08-21 23:56 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: How long is a hold waiting for pickup at a more granular level (77.18 KB, patch)
2019-08-22 00:45 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves() (5.56 KB, patch)
2019-08-22 00:46 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: (follow-up) Updates to DBIx class files (8.68 KB, patch)
2019-08-22 00:47 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: Fixes to make the workflow of master fit, including display of lastpickupdate (7.43 KB, patch)
2019-08-22 00:47 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 8367: How long is a hold waiting for pickup at a more granular level (76.57 KB, patch)
2019-09-19 09:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves() (5.56 KB, patch)
2019-09-19 09:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 8367: (follow-up) Updates to DBIx class files (8.67 KB, patch)
2019-09-19 09:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 8367: Fixes to make the workflow of master fit, including display of lastpickupdate (7.42 KB, patch)
2019-09-19 09:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 8367: Add holds_pickup_period circulation rule (13.84 KB, patch)
2020-04-21 22:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2012-07-05 13:35:33 UTC
The syspref reservemaxpickupdelay says how long a hold is waiting for patron pickup. This could be set at issuingrule level, to let a student of library A 4 days and a teacher of library B  10 days
Comment 1 Jonathan Druart 2012-09-11 12:55:29 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2012-10-29 15:57:26 UTC
I tried to manually fix the merge errors and ended up getting this error:

The 'days' parameter (undef) to DateTime::Duration::new was an 'undef', which is not one of the allowed types: scalar
 at /usr/lib/perl5/DateTime/Duration.pm line 31
	DateTime::Duration::new(undef, 'days', undef) called at /usr/lib/perl5/DateTime.pm line 1640
	DateTime::add('DateTime=HASH(0xa1d3494)', 'days', undef) called at /home/oleonard/kohaclone/C4/Reserves.pm line 552
	C4::Reserves::GetMaxPickupDate('HASH(0xa11168c)', undef) called at /home/oleonard/kohaclone/circ/waitingreserves.pl line 107

Not sure if this is a real error or whether I made a mistake in merging?
Comment 3 Jonathan Druart 2012-10-31 13:39:04 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2012-10-31 13:41:23 UTC
(In reply to comment #2)

> Not sure if this is a real error or whether I made a mistake in merging?

Maybe both :)
The merge was not easy to resolve and errors exist in the previous patch.
Comment 5 Kyle M Hall 2012-11-21 14:17:00 UTC
This patch looks good in all cases except for waitingreserves.pl. Even though my hold is past its pickup window, it is showing in the waiting holds tab, and not in the holds over tab is it should be.
Comment 6 Jonathan Druart 2012-11-21 14:40:15 UTC
Hi Kyle,

What is the value filled for the "Available since-until" column ?

I have 2 holds and 1 by tab:
first have "19/11/2012 - 26/11/2012", second (over) "28/10/2012 - 04/11/2012".

And it is what is expected.
Comment 7 Kyle M Hall 2012-11-21 16:47:33 UTC
On circulation.pl it shows:
Item 1 waiting at Centerville until 11/20/2012

On waitingreserves.pl it shows:
11/19/2012 - 11/26/2012

so it appears to be using the new issue rules setting for circulation.pl, but it's still using the system preference for waitingreserves.pl

I have my rule set to 1 day, and my system pref set to 7.
Comment 8 Kyle M Hall 2012-11-21 16:59:18 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2012-11-21 17:16:34 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2012-11-21 17:16:51 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2012-11-22 09:19:52 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2012-11-22 09:23:46 UTC
(In reply to comment #10)
> Created attachment 13604 [details] [review]
> Bug 8367: Add more granular level for ReservesMaxPickUpDelay - Followup

Good catch Kyle, Thanks!

It would be worth having an other SO for these patches
Comment 13 Kyle M Hall 2012-12-14 16:13:51 UTC
It's been a month without another signer. I think it's time to move these patches to QA.
Comment 14 Paul Poulain 2012-12-14 16:22:12 UTC
(In reply to comment #13)
> It's been a month without another signer. I think it's time to move these
> patches to QA.

agreed (and agreed also because now QAers are supposed to test the patch)
Comment 15 Marcel de Rooy 2013-02-15 08:15:09 UTC
QA: Looking at this now. Rebasing it first..
Comment 16 Marcel de Rooy 2013-02-15 08:16:10 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2013-02-15 08:16:23 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2013-02-15 09:29:41 UTC
QA Comment:
I like the feature! But IMO it needs some more attention before pushing it: 

Issue while resolving merge conflict in circ/waitingreserves.pl: 
The dateformat template var has been removed in master (was in the patch) but is still used in one line of template (waitingreserves.pl). Please check. 

IMO it is confusing that the old pref exists besides the new column in issuing rules. I would suggest simplification by obsoleting pref ReservesMaxPickUpDelay.
At this time behavior is not so clear anymore. Note that there is no comment either on the preferences form that this syspref could be overruled by issuing rules.
And the other way around: If no rule is found, the pref still holds. Does the user still understand that from the rules form? 

GetMaxPickupDate: It may be worth to consider if it would be easier to add a maxpickupdate to the reserves table instead of recalculating it "everywhere".
GetMaxPickupDelay: Only called by GetMaxPickupDate. 
A serious issue IMO is the FIXME in CancelExpiredReserves. Instead of calling GetMaxPickupDelay, you just use the pref value!
I understand that from performance point of view, but this is "killing" the feature.
When using the cron job to remove holds, the new pickup delay should be used not the old one.
Note that you could easily resolve this by looking at a maxpickupdate when adding this to the reserve table and calculating it at confirm hold time. 

Minor representation issue: In the opac user form I see the max pickup date. Would it be consistent to show it on staff catalogue detail too (Holds tab)? 

Setting to Failed QA, mainly for the cancelling holds issue.
Comment 19 Marcel de Rooy 2013-02-15 09:33:54 UTC
Just another thought:
If you would remove the pref, please add a db change to copy it into the new column of the existing issuing rules.
Comment 20 Jonathan Druart 2013-02-15 15:47:50 UTC
(In reply to comment #18)
> QA Comment:
> Note that you could easily resolve this by looking at a maxpickupdate when
> adding this to the reserve table and calculating it at confirm hold time. 

Hi Marcel,
Thanks for QAing this patch!
It is a great idea even though it was not so easy :)
A patch is coming.
Comment 21 Jonathan Druart 2013-02-15 15:48:14 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2013-02-15 17:02:32 UTC
Questions, does this take into account days the library is closed? It would make sense for the expiration day for pickups to skip to the next open day if they land on a holiday. This is something that needed has needed to be addressed for some time now.
Comment 23 Marcel de Rooy 2013-02-18 09:25:04 UTC
(In reply to comment #20)
> It is a great idea even though it was not so easy :)
Yes, I see. In circulation nothing seems to be easy..
Will try to have a look at your patches again later this week.

Just a question from quickly glancing through your code. In CancelExpiredReserves:
+        my $query = "SELECT * FROM reserves WHERE TO_DAYS( NOW() ) - TO_DAYS( waitingdate ) > maxpickupdate AND found = 'W' AND priority = 0";
Does that work? Looks to me that it doesn't. Shouldn't you just look for maxpickupdate < now?
Comment 24 Marcel de Rooy 2013-02-18 09:27:11 UTC
(In reply to comment #22)
> Questions, does this take into account days the library is closed? It would
> make sense for the expiration day for pickups to skip to the next open day
> if they land on a holiday. This is something that needed has needed to be
> addressed for some time now.

Looks like that this report does not do that. I agree that it should, but you could say that it is beyond scope here? Followup or additional report? 
But this report does pave the way. We only have to refine the routine where maxpickupdate is calculated now.
Comment 25 Katrin Fischer 2013-02-18 09:41:22 UTC
Hm, there is a downside to the new column too - we loose a bit of flexibility. Say a library closes for some time and changes the pickup delay to be longer, so the holds don't go stale - with the patch in effect they would have to do a database update to those holds. It's only one use case I have come up with so far. There might be other disadvantages/advantages of both ways of doing it.
Comment 26 Jonathan Druart 2013-02-18 12:24:23 UTC
(In reply to comment #23)
> Just a question from quickly glancing through your code. In
> CancelExpiredReserves:
> +        my $query = "SELECT * FROM reserves WHERE TO_DAYS( NOW() ) -
> TO_DAYS( waitingdate ) > maxpickupdate AND found = 'W' AND priority = 0";
> Does that work? Looks to me that it doesn't. Shouldn't you just look for
> maxpickupdate < now?

Yes, of course!
(new patch coming)

(In reply to comment #24)
> (In reply to comment #22)
> > Questions, does this take into account days the library is closed? It would
> > make sense for the expiration day for pickups to skip to the next open day
> > if they land on a holiday. This is something that needed has needed to be
> > addressed for some time now.
> 
> Looks like that this report does not do that. I agree that it should, but
> you could say that it is beyond scope here? Followup or additional report? 
> But this report does pave the way. We only have to refine the routine where
> maxpickupdate is calculated now.

Yes, that can be developed later without investing too much effort.


(In reply to comment #25)
> Hm, there is a downside to the new column too - we loose a bit of
> flexibility. Say a library closes for some time and changes the pickup delay
> to be longer, so the holds don't go stale - with the patch in effect they
> would have to do a database update to those holds. It's only one use case I
> have come up with so far. There might be other disadvantages/advantages of
> both ways of doing it.

You are right. But do you think many librarians often change this value?
I don't see how to manage the feature and this use case.
We can imagine a script that modifies directly the max pickup date in DB according some variables.
Comment 27 Jonathan Druart 2013-02-18 12:24:38 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2013-02-18 12:25:50 UTC
(In reply to comment #25)
> Hm, there is a downside to the new column too - we loose a bit of
> flexibility. Say a library closes for some time and changes the pickup delay
> to be longer, so the holds don't go stale - with the patch in effect they
> would have to do a database update to those holds. It's only one use case I
> have come up with so far. There might be other disadvantages/advantages of
> both ways of doing it.

What is Koha doing if I adjust the loan period? It does not change the due dates in the system. The due date was set with the active guidelines of that moment. You could say: Doing the same for holds is consistent :)

If the library closes, they should stop confirming holds in due time, limiting the number of waiting holds. Pending holds are no problem. The pickup date should be calculated at confirm hold time.
They could also stop the cron job for cancelling holds until they processed the holds queue after reopening? Would they really need a db update?
Comment 29 Katrin Fischer 2013-02-18 12:35:17 UTC
As I said - I think thre are arguments for both approaches.  :)

Only I am thinking that this would not work:
>If the library closes, they should stop confirming holds in due time, limiting >the number of waiting holds. Pending holds are no problem. The pickup date >should be calculated at confirm hold time.

While that might, but requires you have access to the system or your support vendor lets you do it:
>They could also stop the cron job for cancelling holds until they processed the >holds queue after reopening? Would they really need a db update?
Comment 30 Katrin Fischer 2013-02-18 14:33:59 UTC
Ah sorry, I missed half the comments:

@Jonathan: No, I don't think it happens often that the value changes. I just have been thinking about the consequences of not recalculating. I didn't think about the calendar - that would be a solution (see bug 8735)
Maybe making it possible to change the date from the interface (out of scope here) would be nice too at some point.
Comment 31 Marcel de Rooy 2013-02-22 07:21:32 UTC
QA: Looking again here now..
Comment 32 Marcel de Rooy 2013-02-22 08:33:58 UTC
Preliminary comments:
Report Holds awaiting pickup:
A DateTime object can only be compared to another DateTime object (DateTime=HASH(0x53d7020), 2013-02-22). at /usr/share/koha/testclone/circ/waitingreserves.pl line 110

Another serious issue: During upgrade all reserves need a maxpickupdate. Now they are null.

Before I would signoff and pass qa, it would be very useful to have another signoff on this one. After testing your changes thorougly, another external signoff (maybe from Kyle?) is welcome too. The nature of these patches asks for more extensive testing. Please note that a more detailed test plan could help.

For now setting status to Failed QA.
Comment 33 Marcel de Rooy 2013-02-22 09:17:22 UTC
Please have a look too at bug 5786.
Comment 34 Jonathan Druart 2013-02-22 10:52:10 UTC
(In reply to comment #32)
> Preliminary comments:
> Report Holds awaiting pickup:
> A DateTime object can only be compared to another DateTime object
> (DateTime=HASH(0x53d7020), 2013-02-22). at
> /usr/share/koha/testclone/circ/waitingreserves.pl line 110
> 
> Another serious issue: During upgrade all reserves need a maxpickupdate. Now
> they are null.

Fixed.


> Before I would signoff and pass qa, it would be very useful to have another
> signoff on this one. After testing your changes thorougly, another external
> signoff (maybe from Kyle?) is welcome too. The nature of these patches asks
> for more extensive testing.

Yes! I tested some cases but there are so many specific cases, it is difficult to test everything alone. 

> Please note that a more detailed test plan could help.

Since the patch affects all the pages related to holds, I don't know how to write a complete test plan.

(In reply to comment #33)
> Please have a look too at bug 5786.

I did. I think my patch is closer to be pushed to master than bug 5786.
I didn't find where the new column issuingrules.reservesmaxpickupdelay is used elsewhere than in the issuingrules management (add/remove in DB + in tt files).
Comment 35 Jonathan Druart 2013-02-22 10:52:47 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2013-02-25 17:24:24 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2013-02-25 18:19:04 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2013-02-25 18:23:31 UTC
(In reply to comment #37)
> Created attachment 15659 [details] [review] [review]
> Bug 8367: Skip to next open day if max pickup date is a holiday

I should note this also fixes a bug, where the call to GetReservesFromItemnumber assumed it would receive a reserve hashref, but it does not.
Comment 39 Owen Leonard 2013-03-19 01:03:11 UTC
The last patch needs a commit message with a test plan.
Comment 40 Jonathan Druart 2013-06-14 13:22:04 UTC
(In reply to comment #39)
> The last patch needs a commit message with a test plan.

Kyle, since this patch is from you, could you provide a test plan please?
Comment 41 Kyle M Hall 2013-06-17 17:06:00 UTC
(In reply to comment #40)
> (In reply to comment #39)
> > The last patch needs a commit message with a test plan.
> 
> Kyle, since this patch is from you, could you provide a test plan please?

Sure!

1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup location
4) Edit the calendar for the given location, and make the pickup date a holiday
4) View waitingreserves.pl, the last pickup date should now be the increased by one day

I would add this to the patch, but at the moment the first patch isn't applying.
Comment 42 Jonathan Druart 2013-06-18 08:30:05 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2013-06-18 08:30:13 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2013-06-18 08:30:17 UTC Comment hidden (obsolete)
Comment 45 Jonathan Druart 2013-06-18 08:30:21 UTC Comment hidden (obsolete)
Comment 46 Jonathan Druart 2013-06-18 08:31:19 UTC
I rebased 4 patches and added a test plan for the last one.
Thanks Kyle!
Comment 47 Jonathan Druart 2013-06-18 09:17:00 UTC Comment hidden (obsolete)
Comment 48 Jonathan Druart 2013-06-18 09:17:06 UTC Comment hidden (obsolete)
Comment 49 Jonathan Druart 2013-06-18 09:18:29 UTC
I squashed the 3 first patches, it has become too difficult to rebase them...
It needs a SO.
Comment 50 Jonathan Druart 2013-06-18 09:25:53 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2013-06-18 09:25:58 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2013-06-18 09:56:22 UTC
Note: Unit tests for GetMaxPickupDate are provided by bug 10487 (depends on 10289)
Comment 53 Kyle M Hall 2013-06-18 12:01:26 UTC
When I tried to create my first rule on a fresh database with the first patch only, I got this error:

smart-rules.pl: DBD::mysql::st execute failed: called with 18 bind variables when 17 are needed at /home/koha/kohaclone/admin/smart-rules.pl line 137., referer: http://staff.kohadev/cgi-bin/koha/admin/smart-rules.pl
Comment 54 Jonathan Druart 2013-06-18 13:14:52 UTC Comment hidden (obsolete)
Comment 55 Kyle M Hall 2013-06-21 12:49:10 UTC Comment hidden (obsolete)
Comment 56 Kyle M Hall 2013-06-21 12:49:27 UTC Comment hidden (obsolete)
Comment 57 Kyle M Hall 2013-06-21 12:49:47 UTC Comment hidden (obsolete)
Comment 58 Jonathan Druart 2013-06-24 14:58:51 UTC
(In reply to Kyle M Hall from comment #56)
> Created attachment 19181 [details] [review] [review]
> Bug 8367: Skip to next open day if max pickup date is a holiday

Kyle,
I don't understand how this patch can worked. The maxpickupdate is stored in the DB, when adding or updating an hold.
The patch works but not with this test plan.
Comment 59 Kyle M Hall 2013-07-02 16:42:26 UTC
You're right, I think the test plan should be:

Test plan:
1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup
location
4) Cancel this hold
5) Edit the calendar for the given location, and make the pickup date a
holiday
6) Repeat step 2
7) View waitingreserves.pl, the last pickup date for this new hold should now be the increased by one day
Comment 60 Jonathan Druart 2013-07-03 09:55:23 UTC
Kyle, I agree with your test plan. I am signing off your patch. But I think I found a bug, could you take a look at this new patch please?
Comment 61 Jonathan Druart 2013-07-03 09:55:48 UTC Comment hidden (obsolete)
Comment 62 Jonathan Druart 2013-07-03 09:55:58 UTC Comment hidden (obsolete)
Comment 63 Katrin Fischer 2013-07-03 10:10:15 UTC
Hmm, I think NOW() is a mysqlism. I know we probably have lots of those in the codebase already, but maybe worth trying not to add more and find best practices for replacing them? 

Mtj suggested using CURRENT_TIMESTAMP() instead on IRC:
http://dev.mysql.com/doc/refman/5.1/en/date-and-time-functions.html
Comment 64 Jonathan Druart 2013-07-03 10:16:45 UTC
(In reply to Katrin Fischer from comment #63)
> Hmm, I think NOW() is a mysqlism. I know we probably have lots of those in
> the codebase already, but maybe worth trying not to add more and find best
> practices for replacing them? 
> 
> Mtj suggested using CURRENT_TIMESTAMP() instead on IRC:
> http://dev.mysql.com/doc/refman/5.1/en/date-and-time-functions.html

I don't add it, it already exists.
Comment 65 Kyle M Hall 2013-07-03 10:21:19 UTC Comment hidden (obsolete)
Comment 66 Katrin Fischer 2013-07-03 10:22:30 UTC
Ok, maybe we could take a look at the NOW()s another time.
Comment 67 Kyle M Hall 2013-07-03 10:26:42 UTC
(In reply to Katrin Fischer from comment #66)
> Ok, maybe we could take a look at the NOW()s another time.

Yes, I would suggest filing a bug report to take care of any NOW()'s in the codebase. It seems likely however, that the upcoming DBIx::Class switchover will take care of those anyway.
Comment 68 Katrin Fischer 2013-08-03 17:54:14 UTC
Hi Kyle,
there is a conflict in Reserves.pm I am not sure how to fix - could you take a look please? I think it's because your patch for using reserve_id is in now, so it might need a little work there.
Comment 69 Jonathan Druart 2013-08-13 13:10:14 UTC Comment hidden (obsolete)
Comment 70 Jonathan Druart 2013-08-13 13:11:08 UTC Comment hidden (obsolete)
Comment 71 Jonathan Druart 2013-08-13 13:11:16 UTC Comment hidden (obsolete)
Comment 72 Jonathan Druart 2013-08-13 13:11:22 UTC Comment hidden (obsolete)
Comment 73 Jonathan Druart 2013-08-13 13:11:28 UTC Comment hidden (obsolete)
Comment 74 Jonathan Druart 2013-08-13 13:12:09 UTC
(In reply to Katrin Fischer from comment #68)
> Hi Kyle,
> there is a conflict in Reserves.pm I am not sure how to fix - could you take
> a look please? I think it's because your patch for using reserve_id is in
> now, so it might need a little work there.

Conflicts solved.
Comment 75 Katrin Fischer 2013-09-22 12:26:43 UTC
Hi Jonathan,

I am sorry, there are a couple of new conflicts now in this patch:
Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 8367: Add more granular level for ReservesMaxPickUpDelay
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-user.pl
CONFLICT (content): Merge conflict in opac/opac-user.pl
Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8367: Add more granular level for ReservesMaxPickUpDelay
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-8367-Add-more-granular-level-for-ReservesMaxPi-BWSyFY.patch
Comment 76 Agnes Rivers-Moore 2014-10-01 19:45:09 UTC
We would like the option to have a shorter pickup period for some item types.
Comment 77 Paul Poulain 2014-10-02 08:03:15 UTC
(In reply to Agnes Rivers-Moore from comment #76)
> We would like the option to have a shorter pickup period for some item types.

Sorry, but this patch does not apply anymore, and should be rewritten almost from scratch because another patch has been pushed before this one, that make what Jonathan made invalid.

So, without funding to re-do the work, we (BibLibre) will not work on it anymore, sorry.
Comment 78 Olli-Antti Kivilahti 2014-10-24 18:17:29 UTC
Hi there gentlemen and esteemed gentle lady!

I just finished refactoring this feature to master and I am looking forward to sharing the results ASAP I get it better tested.

Our bookmobiles are suffering from constantly canceled/billed reserves and this feature solves a big issue for them.

Unfortunately this feature breaks a lot of our in-house work, but such is life :)

I hope you can sharpen your "Sign off"-daggers and start stabbing, presuming you or your client is looking forward to getting this feature to master.
Comment 79 Olli-Antti Kivilahti 2014-10-27 11:17:04 UTC Comment hidden (obsolete)
Comment 80 Olli-Antti Kivilahti 2014-10-27 11:21:25 UTC
Making unit tests for the smart-rules.pl is rather tedious with the lack of proper getters/setters for rules. Need to introduce a C4::SmartRules.pm to access them more conveniently. I hope this is not getting way out of scope for this feture?
Comment 81 Katrin Fischer 2014-10-27 11:23:50 UTC
Hm, introducing a new module just for testing seems a bit out of scope :) Have you taken a look at some of the other tests for recently added features? I think there are some for the new automatic renewal and maybe others that could be useful.
Comment 82 Olli-Antti Kivilahti 2014-10-28 14:13:34 UTC Comment hidden (obsolete)
Comment 83 Olli-Antti Kivilahti 2014-10-29 08:51:26 UTC Comment hidden (obsolete)
Comment 84 Olli-Antti Kivilahti 2014-10-29 08:56:18 UTC Comment hidden (obsolete)
Comment 85 Olli-Antti Kivilahti 2014-10-29 13:46:07 UTC Comment hidden (obsolete)
Comment 86 Paola Rossi 2014-10-29 16:39:44 UTC
Kind Olli-Antti Kivilahti,

I've applied the patch against master 3.17.00.035.

I've upgraded the DB from a one that had been filled with some holds.

After having applied, by mysql command line, on the DB I saw the new field "lastpickupdate" of the table "reserves" as required; in all the holds this new field was set to NULL.

About this step:
4. In the 4 templates, you can see the 'Last pickup date' for an hold
   (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt,
   opac-user.tt)

in circ/circulation.tt (CIRC, Check out, Holds) and members/moremember.tt (Patrons, patron X, Details, Holds) there was the title "Last pickup date" of the new column (the 2-nd from the left) of the table, but the datas in the cells below it were the titles. All the other datas on the right were shifted on the left by 1 column, and the last column "Delete?" of the table had nothing in the cells below it.

I created a new hold for a patron without holds, and the problem was the same on both the *.tt .

About opac-user.tt, I logged in to OPAC as a patron with n holds. Here ("your summary", tag "Holds (n)") I didn't see any new column.

So I pass the patch to "Failed QA" status.
Comment 87 Olli-Antti Kivilahti 2014-10-29 17:29:17 UTC Comment hidden (obsolete)
Comment 88 Olli-Antti Kivilahti 2014-10-29 17:34:11 UTC
(In reply to Paola Rossi from comment #86)
> Kind Olli-Antti Kivilahti,
> 
Thank you so much for testing this patch Ms. or Mrs. Rossi!

> I've applied the patch against master 3.17.00.035.
> 
> I've upgraded the DB from a one that had been filled with some holds.
> 
> After having applied, by mysql command line, on the DB I saw the new field
> "lastpickupdate" of the table "reserves" as required; in all the holds this
> new field was set to NULL.
> 
Sorry, my test plans were not complete. I added the following clause:
0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF
   'ReservesMaxPickUpDelay' is bigger than 0. Otherwise existing reserves
   won't get UPDATEd with the new lastpickupdate-values.

> About this step:
> 4. In the 4 templates, you can see the 'Last pickup date' for an hold
>    (circ/circulation.tt, circ/waitingreserves.tt, members/moremember.tt,
>    opac-user.tt)
> 
> in circ/circulation.tt (CIRC, Check out, Holds) and members/moremember.tt
> (Patrons, patron X, Details, Holds) there was the title "Last pickup date"
> of the new column (the 2-nd from the left) of the table, but the datas in
> the cells below it were the titles. All the other datas on the right were
> shifted on the left by 1 column, and the last column "Delete?" of the table
> had nothing in the cells below it.
> 
> I created a new hold for a patron without holds, and the problem was the
> same on both the *.tt .
> 
Are you sure you used the english language? I have encountered this issue mostly with not-up-to-date language templates. I retested this feature and was unable to replicate your report.

> About opac-user.tt, I logged in to OPAC as a patron with n holds. Here
> ("your summary", tag "Holds (n)") I didn't see any new column.
> 
Sorry, updated the TEST PLAN as follows:
5. In opac-user.pl the last pickup date is displayed in the status-column if
   available.

> So I pass the patch to "Failed QA" status.
Thanks for testing! Hope to hear from you soon :)
Comment 89 Paola Rossi 2014-10-30 16:30:17 UTC
(In reply to Olli-Antti Kivilahti from comment #88)
> [...] Hope to hear from you soon 

Thanks, Olli-Antti, for your kind feedbacks.

Today I've applied the patch against master 3.17.00.043.

After having applied, I've created a hold reserving an item that was checked out. So the reserve/hold's status became "W" after checking in it. On the browser I refreshed the cache (Ctrl r, Ctrl F5): so I saw the hold by the 4 *.tt as required in step 4.

In particular: on OPAC, for a logged patron with holds, the lastpickupdate was in the "until" part of the "Status" column of the hold.
On circ/waitingreserves.tt (Home › Circulation › Holds awaiting pickup) the lastpickupdate was in the "until" part of the first column "Available" of "n Hold(s) waiting" tag.
 
The tables with holds with NULL lastpickupdate (holds created reserving items not checked out, as allowed by syspref AllowOnShelfHolds set to "Allow") were shown OK too.

So I pass the patch back to "Needs Signoff" status.

I've changed the syspref 'ReservesMaxPickUpDelay' from the default 0 value, thank you again.
Comment 90 Olli-Antti Kivilahti 2014-10-30 16:56:00 UTC
(In reply to Paola Rossi from comment #89)
> (In reply to Olli-Antti Kivilahti from comment #88)
> > [...] Hope to hear from you soon 
> 
> Thanks, Olli-Antti, for your kind feedbacks.
> 
> Today I've applied the patch against master 3.17.00.043.
> 
> After having applied, I've created a hold reserving an item that was checked
> out. So the reserve/hold's status became "W" after checking in it. On the
> browser I refreshed the cache (Ctrl r, Ctrl F5): so I saw the hold by the 4
> *.tt as required in step 4.
> 
> In particular: on OPAC, for a logged patron with holds, the lastpickupdate
> was in the "until" part of the "Status" column of the hold.
> On circ/waitingreserves.tt (Home › Circulation › Holds awaiting pickup) the
> lastpickupdate was in the "until" part of the first column "Available" of "n
> Hold(s) waiting" tag.
>  
> The tables with holds with NULL lastpickupdate (holds created reserving
> items not checked out, as allowed by syspref AllowOnShelfHolds set to
> "Allow") were shown OK too.
> 
> So I pass the patch back to "Needs Signoff" status.
> 
> I've changed the syspref 'ReservesMaxPickUpDelay' from the default 0 value,
> thank you again.

Nice to hear from you!

Is there anything here that prevents your from signing this off?
I believe this feature needs more than one sign off to be considered fit for QA and yours would be a great start!
Comment 91 Paola Rossi 2014-11-05 10:53:49 UTC
[I beg you pardon for the length of this comment, my delay in answering and possible inaccuracies.]

I've applied the patch against master 3.17.00.044 [HEAD 10860].

To test the patch IMHO on ADMIN it is better:
- (as required by the test plan) kept the value of syspref "ReservesMaxPickupDelay" > 0
- set the syspref "EnhancedMessagingPreferences" to ON (default is OFF)

To test the patch IMHO on ADMIN it could be better:
- kept the default value "Don't allow" to the syspref "AllowOnShelfHolds"
- set the "Charge" of the item-types to 0.00 (default 5.00) for all types
- [remember to set the circulation rules]
- set the syspref "KohaAdminEmailAddress" to a value not filtered by antispam's own emailing-system [to receive notices' email]
- [remeber to refresh the cache [Ctrl r - Ctrl F5] after seeing master's behaviour].

At my tests, the RESERVESLIP and HOLD notices are allowed to be cloned to your specific library, but I saw that koha uses only the "ALL LIBRARIES" notices, and never the cloned ones.
So I changed those 2 notices to test the 2 parts of the test plan about LETTER.

To test the part of the test plan about the RESERVESLIP notice, i.e. see below:
*** TEST: LETTER PLACEHOLDER ***,
IMHO the functions to test are "Confirm"/"Print and confirm", on cheching in a record already on hold by another patron.

To test the last part of the test plan about the HOLD notice, i.e. see below:
*** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION ****,
if I'm not wrong :
- set the notice's email [i.e. "Primary email"] of the test patron to your own email
- if this patron had a found/status W hold, you'll receive a mail soon after:
perl ./misc/cronjobs/advance_notices.pl  -c
perl ./misc/cronjobs/process_message_queue.pl

IMHO it would be better explicitly adding a further last part to the test plan about the provided:
t/db_dependent/Holds.t
[perhaps other parts of the preamble could need a testing action too.]

At my tests the upgrade of the DB is OK.

The 4 *.tt are OK.
NB1.: on ADMIN, in the "Circ and fines issuing rules", perhaps the new column "Holds wait for pickup (day)" could be better set at the right of "Holds allowed (count)", at the right of all the 4 columns about renewal, and not among these 4 ones.

NB2.: on CIRC, on waitingreserves.pl (="Holds awaiting pickup"), on the "Holds over" tag, the present string:
"Holds listed here have been awaiting pickup for more than days." could be modified. In master it was:
"Holds listed here have been awaiting pickup for more than X days." where X was the value of the syspref "ReservesMaxPickUpDelay" which has been removed by this patch.

NB3.: On opac-user.pl, for a logged in user with W holds, the W holds had not neigher the "Cancel" icon nor the "Suspend" icon. [I didn't check the master about this behaviour, sorry. If this patch had changed the behaviour, this regression would be on error, and this point will be added to the errors' list.]

PROBLEMS:

NB4.: [MASTER] at my tests the "W" found/status of a hold/reserve is set in only 1 case:
it is set to a hold, that previously had been opened on a biblio record which was already checked out (found=NULL), and then checked in (found=W) without "Ignore" submit. [I'd like to have a feedback about it, please, because all my tests were about this assumption.]
NB5.: [MASTER] So, at my tests, the waitingdate of a hold/reserve can be only the current date, i.e. the current day on which I check-in a biblio record. [I'd like to have a feedback about it, please, because all my tests were about this assumption.]

1) I saw no reserves.lastpickupdate nor the reserves.waitingdate on RESERVESLIP notice. But I think that this problem is out of this bug, because the master too behaves so. [On the contrary, on master (and after applying too) the reserves.reservesnotes is correctly shown by the notice.]
2) Whilst a check out lasts 1 more day for an intervening holiday, the hold's period doesn't: I'd like to have a feedback about this, please. If the W hold would last 1 more day, the patch would be in error.
3) On upgrading the DB, by syspref "ReservesMaxPickupDelay" > 0, all the holds for all the patrons' categories and item types would receive the same lasting period. Whilst the patch, once applied, could assign a different value to each of them. I think this remarkable to get coherent DB datas.
4) The found/status "W" meaning is about the lasting period available to the patron to pick up the document. But when a "Transfer" is needed from the check-out' library, to the hold's library, koha [master] doesn't set the found/status to "W", but to NULL/T: I think that this behaviour could be evaluated, that it can be a master's bug, and, if this is the case, that this patch could be tested after having resolved this bug on a DB with such cases.
5)  prove t/db_dependent/Holds.t
t/db_dependent/Holds.t .. 1/46
#   Failed test 'GetLastPickupDate(): Not using Calendar'
#   at t/db_dependent/Holds.t line 436.
#          got: '2014-11-10'
#     expected: '2014-11-05'
 
#   Failed test 'GetLastPickupDate(): Moving lastpickupdate over holidays, but not affected by them'
#   at t/db_dependent/Holds.t line 448.
#          got: '2014-11-10'
#     expected: '2014-11-05'
 
#   Failed test 'MoveWaitingdate(): Moving to past'
#   at t/db_dependent/Holds.t line 401.
#          got: ''
#     expected: '1'
 
#   Failed test 'Waiting reserve with lastpickupdate for 2014-11-04 totally canceled'
#   at t/db_dependent/Holds.t line 367.
#          got: '1'
#     expected: '0'
# Looks like you failed 4 tests of 46.
t/db_dependent/Holds.t .. Dubious, test returned 4 (wstat 1024, 0x400)
Failed 4/46 subtests
 
Test Summary Report
-------------------
t/db_dependent/Holds.t (Wstat: 1024 Tests: 46 Failed: 4)
  Failed tests:  37, 39, 41, 44
  Non-zero exit status: 4
Files=1, Tests=46,  1 wallclock secs ( 0.01 usr  0.01 sys +  0.91 cusr  0.29 csys =  1.22 CPU)
Result: FAIL

For the 5-th problem I pass the patch to "Failed QA" status.
Comment 92 Paola Rossi 2014-11-06 13:21:45 UTC
I beg your pardon for the following comment, further in addition to the 91-th comment.

It's about the syspref "AllowOnShelfHolds". As the manual 3.16 says:
"Asks: ___ hold requests to be placed on items that are not checked out."

At my tests, the default value is "Don't allow" instead of the manual's default "Allow". The "Allow" status is the most interesting one, of course.

[on MASTER]
A) About a multi branch system, with "Transfer" from L1 to L2 ["if the library system is a multi branch system and patrons used the hold system to request items from other libraries"], the 3 status/found (of the just created hold, of the transferred hold by L1, and also after cancelling the Transfer by L2's receiving] are all NULL. IMHO the status/found of the hold could be "W" after cancelling.
B) About reserving from home ["if the library wanted to allow users to place holds on items [NOT OUT!] from home"], IMHO the status/found of the hold could be "W", but at my tests it's NULL.

If A) and B) be bugs, and fixed, all these new "W" cases of a hold should be tested in this patch 8367 too.
Comment 93 Paola Rossi 2014-12-23 16:14:09 UTC
I've tried to apply the patch against master 3.19.00.002

Applying: Bug 8367 - How long is a hold waiting for pickup at a more granular level
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Holds.t
Auto-merging svc/holds
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/waitingreserves.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/waitingreserves.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging koha-tmpl/intranet-tmpl/prog/en/js/holds.js
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging circ/waitingreserves.pl
Auto-merging C4/Reserves.pm
Auto-merging C4/Letters.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8367 - How long is a hold waiting for pickup at a more granular level

So I pass the patch to "Patch doesn't apply" status.
Comment 94 j.kylmala 2015-09-18 07:23:22 UTC
I'll start working on this and I decided to create a new patch and implement the same changes on it.
Comment 95 j.kylmala 2015-09-18 13:13:21 UTC Comment hidden (obsolete)
Comment 96 j.kylmala 2015-09-22 08:13:08 UTC
Created attachment 42754 [details] [review]
Bug 8367 - How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold
   (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl)
5. In opac-user.pl the last pickup date is displayed in the status-column if available.
5. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ****** ******
*** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)*
          ****** ******
1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>"
   somewhere.
2. Make a hold for a borrower.
3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually
   the (current date + the 'Holds pickup wait' in your issuing rules) in days.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup
   location
4) Cancel this hold
5) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
6) Repeat step 2
7) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.
Comment 97 j.kylmala 2015-09-22 08:16:57 UTC
After this patch gets to master, the system preference ExpireReservesMaxPickUpDelay could to be changed to something like ExpireReservesAfterLastPickupDay. I guess it is then more descriptive. There was about 15-20 files depending on ExpireReservesMaxPickUpDelay preference, so it would be better to do maybe a new bug report for it?
Comment 98 I'm just a bot 2016-03-06 21:50:06 UTC
fatal: previous rebase directory .git/rebase-apply still exists but mbox given. at ./getter.pl line 196.
Comment 99 Hugh Rundle 2017-12-29 00:24:03 UTC
Is anyone still working on this? Being able to provide extended hold dates (e.g. for items set aside for our housebound patrons) would be very useful.
Comment 100 Alex Buckley 2018-10-02 00:21:56 UTC
Hi there

I have gone through and rebased this, master had progressed quite a lot since this patch was added and so I had to make a few assumptions during the rebase. 

Please let me know if changes need to be made as a result of the rebase. 

I have also corrected the commit messages to the format 'Bug XXXX: '

I have also written two other patches which correct template issues, and remove the check for the value of the 'ReserveMaxPickupDelay' syspref in Koha/Hold.pm in the set_waiting() function. 

This check is unneccessary as the main patch on this bug report removes the ReserveMaxPickupDelay syspref, and so I replaced this with a retrieval of the appropriate issuingrule and setting the exprationdate = waitingdate + issuingrule.lastpickupwait 

I am currently working on the unit tests t/db_dependent/Hold.t and so I am assigning this the status of 'Assigned' until I have attached a commit with the corrected t/db_dependent/Hold.t
Comment 101 Alex Buckley 2018-10-02 00:22:50 UTC
Created attachment 79790 [details] [review]
Bug 8367: How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold
   (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl)
5. In opac-user.pl the last pickup date is displayed in the status-column if available.
5. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ****** ******
*** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)*
          ****** ******
1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>"
   somewhere.
2. Make a hold for a borrower.
3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually
   the (current date + the 'Holds pickup wait' in your issuing rules) in days.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup
   location
4) Cancel this hold
5) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
6) Repeat step 2
7) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.

Conflicts:
	installer/data/mysql/updatedatabase.pl
Comment 102 Alex Buckley 2018-10-02 00:25:12 UTC
Created attachment 79791 [details] [review]
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm

Fixed waitingreserves.tt template so the borrowernumber is
successfully passed to waitingreserves.pl upon cancelling a hold, so
users can now successfully cancel holds from the waitingreserves.pl
interface.

Removed check for ReserveMaxPickupdateDelay in Koha/Hold.pm as this
syspref is removed in the previous patch. Instead the
expirationdate is calculated the same way as the lastpickupdatedate that is
retrieving the holdpickupwait value for the appropriate issuingrule and then
adding that onto the waiting date.

Same test plan as main patch

Sponsored-By: Brimbank Library, Australia
Comment 103 Alex Buckley 2018-10-02 01:28:15 UTC
Created attachment 79798 [details] [review]
Bug 8367: How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold
   (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl)
5. In opac-user.pl the last pickup date is displayed in the status-column if available.
5. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ****** ******
*** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)*
          ****** ******
1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>"
   somewhere.
2. Make a hold for a borrower.
3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually
   the (current date + the 'Holds pickup wait' in your issuing rules) in days.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup
   location
4) Cancel this hold
5) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
6) Repeat step 2
7) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.

Conflicts:
	installer/data/mysql/updatedatabase.pl
Comment 104 Alex Buckley 2018-10-02 01:28:39 UTC
Created attachment 79799 [details] [review]
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm

Fixed waitingreserves.tt template so the borrowernumber is
successfully passed to waitingreserves.pl upon cancelling a hold, so
users can now successfully cancel holds from the waitingreserves.pl
interface.

Removed check for ReserveMaxPickupdateDelay in Koha/Hold.pm as this
syspref is removed in the previous patch. Instead the
expirationdate is calculated the same way as the lastpickupdatedate that is
retrieving the holdpickupwait value for the appropriate issuingrule and then
adding that onto the waiting date.

Same test plan as main patch

Sponsored-By: Brimbank Library, Australia
Comment 105 Alex Buckley 2018-10-02 01:30:34 UTC
Created attachment 79800 [details] [review]
Bug 8367: Removed unneccessary warns and commented code

Sponsored-By: Brimbank Library, Australia
Comment 106 Alex Buckley 2018-10-02 22:10:59 UTC
Created attachment 79853 [details] [review]
Bug 8367: Moved database update out of updatedatabase.pl into atomicupdates dir
Comment 107 Alex Buckley 2018-10-02 22:12:50 UTC
Created attachment 79854 [details] [review]
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves()

Added check for holidays to expirationdate calculation in
Koha/Hold.pm so the expirationdate respects holidays as the
lastpickupdate does.

Changed C4/Reserve/CancelExpiredReserves() to check the
reserves.lastpickupdate rather than rserves.expirationdate. Therefore it will now automatically expire holds
if they have not been picked up within the time period specified in the
hold pickup delay defined in the issuing rules (which is added to
reserves.waitingdate to calculate the reserves.lastpickupdate) AND the
ExpireReservesMaxPickUpDelay syspref is enabled.

Sponsored-By: Brimbank Library, Australia
Comment 108 Alex Buckley 2018-10-03 00:17:40 UTC
Created attachment 79855 [details] [review]
Bug 8367: Fixed how expirationdate is calculated when ExcludeHolidaysFromMaxPickUpDelay syspref is enabled

Sponsored-By: Brimbank Library, Australia
Comment 109 Alex Buckley 2018-10-03 05:01:37 UTC
Created attachment 79859 [details] [review]
Bug 8367: reserves.lastpickupdate calculation checks and uses ExcludeHolidaysFromMaxPickUpDelay syspref

Sponsored-By: Brimbank Library, Australia
Comment 110 Katrin Fischer 2019-04-28 13:38:30 UTC
Hi Alex, is assigned the correct status here?
Comment 111 Laurie 2019-06-06 16:47:09 UTC
Is this bug still in process? We could really use the granularity to assign different wait items on different item types. Thank you
Comment 112 Hayley Mapley 2019-08-21 23:41:34 UTC
Created attachment 92412 [details] [review]
Bug 8367: How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold
   (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl)
5. In opac-user.pl the last pickup date is displayed in the status-column if available.
5. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ****** ******
*** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)*
          ****** ******
1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>"
   somewhere.
2. Make a hold for a borrower.
3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually
   the (current date + the 'Holds pickup wait' in your issuing rules) in days.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup
   location
4) Cancel this hold
5) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
6) Repeat step 2
7) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.

Conflicts:
	installer/data/mysql/updatedatabase.pl
Comment 113 Hayley Mapley 2019-08-21 23:45:07 UTC
Created attachment 92413 [details] [review]
Bug 8367: How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the 3 templates of staff client, you can see the 'Last pickup date' for an hold
   (circ/circulation.pl, circ/waitingreserves.pl, members/moremember.pl)
5. In opac-user.pl the last pickup date is displayed in the status-column if available.
5. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ****** ******
*** *** *** TEST: LETTER PLACEHOLDER (no code modifications here, just regression)*
          ****** ******
1. Modify the RESERVESLIP->email Letter. Add "Last pickup date<<reserves.lastpickupdate>>"
   somewhere.
2. Make a hold for a borrower.
3. Catch the hold and print the reserve slip. Verify that the lastpickupdate is actually
   the (current date + the 'Holds pickup wait' in your issuing rules) in days.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Note your ReservesMaxPickUpDelay setting.
2) Create and trap a hold
3) View waitingreserves.pl, note the last pickup date, and the pickup
   location
4) Cancel this hold
5) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
6) Repeat step 2
7) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD-letter, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.

Conflicts:
	installer/data/mysql/updatedatabase.pl
Comment 114 Hayley Mapley 2019-08-21 23:46:02 UTC
Created attachment 92414 [details] [review]
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm

Fixed waitingreserves.tt template so the borrowernumber is
successfully passed to waitingreserves.pl upon cancelling a hold, so
users can now successfully cancel holds from the waitingreserves.pl
interface.

Removed check for ReserveMaxPickupdateDelay in Koha/Hold.pm as this
syspref is removed in the previous patch. Instead the
expirationdate is calculated the same way as the lastpickupdatedate that is
retrieving the holdpickupwait value for the appropriate issuingrule and then
adding that onto the waiting date.

Same test plan as main patch

Sponsored-By: Brimbank Library, Australia
Comment 115 Hayley Mapley 2019-08-21 23:47:22 UTC
Created attachment 92415 [details] [review]
Bug 8367: Removed unneccessary warns and commented code

Sponsored-By: Brimbank Library, Australia
Comment 116 Hayley Mapley 2019-08-21 23:47:54 UTC
Created attachment 92416 [details] [review]
Bug 8367: Template fixes + fixes to C4/Reserves.pm and Koha/Hold.pm

Fixed waitingreserves.tt template so the borrowernumber is
successfully passed to waitingreserves.pl upon cancelling a hold, so
users can now successfully cancel holds from the waitingreserves.pl
interface.

Removed check for ReserveMaxPickupdateDelay in Koha/Hold.pm as this
syspref is removed in the previous patch. Instead the
expirationdate is calculated the same way as the lastpickupdatedate that is
retrieving the holdpickupwait value for the appropriate issuingrule and then
adding that onto the waiting date.

Same test plan as main patch

Sponsored-By: Brimbank Library, Australia
Comment 117 Hayley Mapley 2019-08-21 23:49:21 UTC
Created attachment 92417 [details] [review]
Bug 8367: Removed unneccessary warns and commented code

Sponsored-By: Brimbank Library, Australia
Comment 118 Hayley Mapley 2019-08-21 23:51:37 UTC
Created attachment 92418 [details] [review]
Bug 8367: Moved database update out of updatedatabase.pl into atomicupdates dir
Comment 119 Hayley Mapley 2019-08-21 23:52:30 UTC
Created attachment 92419 [details] [review]
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves()

Added check for holidays to expirationdate calculation in
Koha/Hold.pm so the expirationdate respects holidays as the
lastpickupdate does.

Changed C4/Reserve/CancelExpiredReserves() to check the
reserves.lastpickupdate rather than rserves.expirationdate. Therefore it will now automatically expire holds
if they have not been picked up within the time period specified in the
hold pickup delay defined in the issuing rules (which is added to
reserves.waitingdate to calculate the reserves.lastpickupdate) AND the
ExpireReservesMaxPickUpDelay syspref is enabled.

Sponsored-By: Brimbank Library, Australia
Comment 120 Hayley Mapley 2019-08-21 23:53:04 UTC
Created attachment 92420 [details] [review]
Bug 8367: Fixed how expirationdate is calculated when ExcludeHolidaysFromMaxPickUpDelay syspref is enabled

Sponsored-By: Brimbank Library, Australia
Comment 121 Hayley Mapley 2019-08-21 23:54:25 UTC
Created attachment 92421 [details] [review]
Bug 8367: reserves.lastpickupdate calculation checks and uses ExcludeHolidaysFromMaxPickUpDelay syspref

Sponsored-By: Brimbank Library, Australia
Comment 122 Hayley Mapley 2019-08-21 23:55:27 UTC
Created attachment 92422 [details] [review]
Bug 8367: (follow-up) Updates to DBIx class files

Ran the update_dbix_class_files.pl script to regenerate the
Koha/Schema/Result/ files again to match the database changes

Making the atomicupdate file idempotent

Sponsored-by: Brimbank Library, Australia
Authored-by: Hayley Mapley and Alex Buckley
Comment 123 Hayley Mapley 2019-08-21 23:56:22 UTC
Created attachment 92423 [details] [review]
Bug 8367: Fixes to make the reserves workflow work with new schema

Sponsored-by: Brimbank library, Australia
Comment 124 Hayley Mapley 2019-08-21 23:56:58 UTC
Created attachment 92424 [details] [review]
Bug 8367: Fixing display of lastpickupdate

Changed circ/waitingreserves.pl to correctly display lastpickupdate in
Circulation>Holds awaiting pickup.
Changed includes/holds-table.inc to display lastpickupdate in
opac-user.pl in Holds tab.

Sponsored-by: Brimbank District Council
Authored-by: Hayley Mapley and Alex Buckley (Catalyst IT)
Comment 125 Hayley Mapley 2019-08-22 00:45:48 UTC
Created attachment 92425 [details] [review]
Bug 8367: How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the following 3 templates of staff client, you can see the 'Last pickup date' for a hold
   -circ/circulation.pl,
    Circulation>Check outs>Search for Patron>Holds tab
   -circ/waitingreserves.pl
    Circulation>Holds awaiting pickup
   -members/moremember.pl
   Patrons>Search for patron>Holds tab
5. In opac-user.pl the last pickup date is displayed in the Status column if available.
6. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Create and trap a hold
2) View waitingreserves.pl (Circulation>Holds awaiting pickup), note the last pickup date, and the pickup
   location
3) Cancel this hold
4) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
5) Repeat step 2
6) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD_SLIP, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower and print the slip. Verify that the
lastpickupdate is actually (currentdate + the 'Holds pickup wait' in
        your issuing rules) in days.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.

Originally Authored-by: Joonas Kylmälä <j.kylmala@gmail.com>
Taken up by: Hayley Mapley and Alex Buckley (Catalyst IT)
Sponsored-by: Brimbank District Council
Comment 126 Hayley Mapley 2019-08-22 00:46:14 UTC
Created attachment 92426 [details] [review]
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves()

Added check for holidays to expirationdate calculation in
Koha/Hold.pm so the expirationdate respects holidays as the
lastpickupdate does.

Changed C4/Reserve/CancelExpiredReserves() to check the
reserves.lastpickupdate rather than rserves.expirationdate. Therefore it will now automatically expire holds
if they have not been picked up within the time period specified in the
hold pickup delay defined in the issuing rules (which is added to
reserves.waitingdate to calculate the reserves.lastpickupdate) AND the
ExpireReservesMaxPickUpDelay syspref is enabled.

Sponsored-By: Brimbank Library, Australia
Comment 127 Hayley Mapley 2019-08-22 00:47:14 UTC
Created attachment 92427 [details] [review]
Bug 8367: (follow-up) Updates to DBIx class files

Ran the update_dbix_class_files.pl script to regenerate the
Koha/Schema/Result/ files again to match the database changes

Making the atomicupdate file idempotent

Sponsored-by: Brimbank Library, Australia
Authored-by: Hayley Mapley and Alex Buckley
Comment 128 Hayley Mapley 2019-08-22 00:47:38 UTC
Created attachment 92428 [details] [review]
Bug 8367: Fixes to make the workflow of master fit, including display of lastpickupdate

Changed circ/waitingreserves.pl to correctly display lastpickupdate in
Circulation>Holds awaiting pickup.
Changed includes/holds-table.inc to display lastpickupdate in
opac-user.pl in Holds tab.

Sponsored-by: Brimbank District Council
Authored-by: Hayley Mapley and Alex Buckley (Catalyst IT)
Comment 129 Stefan Berndtsson 2019-08-30 13:29:44 UTC
There seems to be a bug in Koha/Hold.pm, set_waiting() where GetReservesControlBranch() is called as a method rather than a function causing the package name to become the first argument.


This line:

my $controlbranch = C4::Reserves->GetReservesControlBranch( $item, $patron );

Should be:

my $controlbranch = C4::Reserves::GetReservesControlBranch( $item, $patron );


...if I'm not mistaken. The current code causes an error with checkin on the pickup library for the book.
Comment 130 David Gustafsson 2019-09-19 09:22:26 UTC
Created attachment 92958 [details] [review]
Bug 8367: How long is a hold waiting for pickup at a more granular level

You can now specify a pickup delay for an hold function of a patron
category and/or a item type and/or a library.
The lastpickupdate-column is set based on the given instruction and lands only on an
open library day.
Also cancel_expired_reserves.pl now cancels only on open library days after the last pickup
date has expired.
And waitingreserves.pl respects the new DB column.

If ReservesMaxPickUpDelay is not set prior to the databaseupdate, the holdspickupwait-directive
is never defined, thus disabling this feature until the holdspickupwait is set.
If the issuingrules holdspickupwait is 0 or less, this feature is disabled. So each issuingrule can disable
this feature for the material it matches, while being fully functional with other rules.

This patch adds:
- a new column in the issuing rules, including help topics.
- a new column reserves.lastpickupdate (+old_reserves)
  It contains the waitingdate + the corresponding "holds pickup wait".
  Each time the waitingdate is modified, this value will be modified too.
- a new field issuingrules.holdspickupwait
- a new function C4::Reserves::MoveWaitingdate() to help in testing this feature.
- a new function C4::Reserves::GetLastPickupDate() to calculate the proper date from Calendar.

This patch removes:
- the ReservesMaxPickUpDelay syspref.
- the ExpireReservesOnHolidays syspref, because it became obsolete due to the changes in
  handling the last pickup date with Koha Calendar.
- Also removes a code section from C4::Letters::_parseLetter() which worked blatantly
  disrespectfully towards Koha Calendar.
- TODO figure out how to rename existing letter placeholders <<expirationdate>> with <<lastpickupdate>>

Unit tests included.

$$$$$$$$$$$$$
$ TEST PLAN $
$$$$$$$$$$$$$

0. BEFORE UPDATING THE DATABASE! MAKE SURE THAT THE SYSPREF 'ReservesMaxPickUpDelay' is bigger than 0.
   Otherwise existing reserves won't get UPDATEd with the new lastpickupdate-values.

          ****** ******
*** *** *** TEST: BASIC WORKFLOW *
          ****** ******
1. Check there is no regression with a normal reserve workflow.
2. Add one or more issuingrules.
3. Update the new column 'Holds pickup wait' in your issuing rules.
4. In the following 3 templates of staff client, you can see the 'Last pickup date' for a hold
   -circ/circulation.pl,
    Circulation>Check outs>Search for Patron>Holds tab
   -circ/waitingreserves.pl
    Circulation>Holds awaiting pickup
   -members/moremember.pl
   Patrons>Search for patron>Holds tab
5. In opac-user.pl the last pickup date is displayed in the Status column if available.
6. According to a library and an item type, the lastpickupdate value will be
   equal to the waiting date + the "holds pickup wait" defined moved to the
   next open library day.

          ** ** ** ** ** **
*** *** *** TEST: RESPECTING CALENDAR HOLIDAYS *
          ** ** ** ** ** **
1) Create and trap a hold
2) View waitingreserves.pl (Circulation>Holds awaiting pickup), note the last pickup date, and the pickup
   location
3) Cancel this hold
4) Edit the calendar for the given location, and make the pickup date a
   single, unrepeated, holiday. Then make the next day a weekly repeating holiday.
   Then make the next two days an annual holiday repeatable every year.
5) Repeat step 2
6) View waitingreserves.pl, the last pickup date for this new hold
   should now be increased by four days.

          ** ** ** ** ** **
*** *** *** TEST: LETTER lastpickupdate PLACEHOLDER AND REGRESSION *
          ** ** ** ** ** **
1. Modify the HOLD_SLIP, add <<reserves.lastpickupdate>> and <<reserves.waitingdate>>
   somewhere.
2. Catch a hold for a borrower and print the slip. Verify that the
lastpickupdate is actually (currentdate + the 'Holds pickup wait' in
        your issuing rules) in days.
3. See the members/notices.pl for the message queued and verify that the
   lastpickupdate-placeholder is set in your desired datetime format and
   that it is not on a holiday. Verify that the waitingdate is today.

Originally Authored-by: Joonas Kylmälä <j.kylmala@gmail.com>
Taken up by: Hayley Mapley and Alex Buckley (Catalyst IT)
Sponsored-by: Brimbank District Council
Comment 131 David Gustafsson 2019-09-19 09:22:31 UTC
Created attachment 92959 [details] [review]
Bug 8367: Altered expirationdate calc in Koha/Hold.pm and altered C4/Reserves/CancelExpiredReserves()

Added check for holidays to expirationdate calculation in
Koha/Hold.pm so the expirationdate respects holidays as the
lastpickupdate does.

Changed C4/Reserve/CancelExpiredReserves() to check the
reserves.lastpickupdate rather than rserves.expirationdate. Therefore it will now automatically expire holds
if they have not been picked up within the time period specified in the
hold pickup delay defined in the issuing rules (which is added to
reserves.waitingdate to calculate the reserves.lastpickupdate) AND the
ExpireReservesMaxPickUpDelay syspref is enabled.

Sponsored-By: Brimbank Library, Australia
Comment 132 David Gustafsson 2019-09-19 09:22:37 UTC
Created attachment 92960 [details] [review]
Bug 8367: (follow-up) Updates to DBIx class files

Ran the update_dbix_class_files.pl script to regenerate the
Koha/Schema/Result/ files again to match the database changes

Making the atomicupdate file idempotent

Sponsored-by: Brimbank Library, Australia
Authored-by: Hayley Mapley and Alex Buckley
Comment 133 David Gustafsson 2019-09-19 09:22:43 UTC
Created attachment 92961 [details] [review]
Bug 8367: Fixes to make the workflow of master fit, including display of lastpickupdate

Changed circ/waitingreserves.pl to correctly display lastpickupdate in
Circulation>Holds awaiting pickup.
Changed includes/holds-table.inc to display lastpickupdate in
opac-user.pl in Holds tab.

Sponsored-by: Brimbank District Council
Authored-by: Hayley Mapley and Alex Buckley (Catalyst IT)
Comment 134 David Gustafsson 2019-09-19 09:29:25 UTC
It seems like this block was incorrectly removed, probably as part of recent rebase:

-                [% UNLESS ( borrowers ) %]
-                    [% IF borrowernumber and patron %]
-                            <div class="col-sm-2 col-sm-pull-10">
-                                <aside>
-                            [% INCLUDE 'circ-menu.inc' %]
-                                </aside>
-                            </div> <!-- /.col-sm-2 col-sm-pull-10 -->
-                    [% END %]
-                [% END %]

This makes the sidebar menu disappear on the circulation page. Removed this change from the "Bug 8367: How long is a hold waiting for pickup at a more granular level"-commit.
Comment 135 Stefan Berndtsson 2019-09-24 09:21:37 UTC
I'm really unclear of the relationship between the two columns lastpickupdate and expirationdate from this patch.

This patch introduces lastpickupdate, but set_waiting() sets expirationdate. Cancelling of an expired hold is then checked against lastpickupdate (used to be expirationdate).

All of the rest of the Koha code that is not affected by this patch uses expirationdate.

Why is lastpickupdate there? Why is cancelling comparing against something that set_waiting() didn't set?
Comment 136 Stefan Berndtsson 2019-09-24 10:57:58 UTC
This patch also explicitly sets 'W' without checking if it should be transferred or not.

To be more specific, it sets 'T' for transfer, then immediately after sets 'W' anyway, using an added ModReserveStatus() call.
Comment 137 Aleisha Amohia 2020-04-21 22:40:18 UTC
Created attachment 103425 [details] [review]
Bug 8367: Add holds_pickup_period circulation rule

So that pickup delay can have a different value per patron category,
   item type or branch.

To test:

1) Update database, restart services
2) Set ReservesMaxPickUpDelay syspref (if not already set)
3) Edit your circulation rules and set a value under 'Holds pickup
period (day) that is DIFFERENT from ReservesMaxPickUpDelay
4) Place a hold on a biblio
5) Check in an item from that biblio and confirm the hold as waiting
6) Confirm the expiration date is calculated using the 'Holds pickup
period' value instead of the ReservesMaxPickUpDelay syspref
7) Revert the waiting status and delete the hold
8) Re-place the hold on the biblio
9) Create a holiday with a date that will overlap with the 'Holds pickup
period'
10) Check in an item from that biblio and confirm the hold as waiting
11) Confirm the expiration date is calculated using the 'Holds pickup
period' value AND considers the special holiday
12) Confirm tests pass t/db_dependent/Holds/WaitingReserves.t

I've also added this to the TalkingTech_itiva_outbound.pl cronjob, I'm
not sure how to test this. Please test properly if you can, otherwise
maybe put some warns in and confirm the correct expiration date is
calculated.

Sponsored-by: Catalyst IT
Comment 138 David Nind 2020-09-28 10:32:21 UTC
Patch doesn't apply 8-(.. 

(The status was changed earlier in the month - not sure whether just changing the status sends a message or not).
Comment 139 Aleisha Amohia 2020-09-28 20:38:59 UTC
(In reply to David Nind from comment #138)
> Patch doesn't apply 8-(.. 
> 
> (The status was changed earlier in the month - not sure whether just
> changing the status sends a message or not).

It does! :)

This is on my list to get to when I have time.