This patch seems to be missing a change in a .pref file. Please add this file and attach a new patch.

(In reply to comment #3)
> This patch seems to be missing a change in a .pref file. Please add this
> file and attach a new patch.

Fixed!

Notes for when I come back to this Monday:

ExpireReservesMaxPickUpDelay = allow
ReservesMaxPickUpDelay = 1 (day)
Holiday on the calendar 
Cron set to run daily (package crons) 

should expire sunday night (friday and saturday are holidays, hold is over after 1 day).

<crosses fingers>

I need to run this test again - it didn't appear to work but that could be due to many factors that are my issue not the patch.

Here's a test plan you can use to test it without waiting overnight.

Test Plan:
1) Set ExpireReservesMaxPickUpDelay
2) Set ReservesMaxPickUpDelay to 1
3) Place a hold, set it to waiting
4) Using the MySQL console, modify the waiting date and set it to yesterday.
5) Set today as a holiday for the pickup branch in question.
6) Run misc/cronjobs/holds/cancel_expired_holds.pl
7) The hold should remain unchanged
8) Remove today as a holiday
9) Run misc/cronjobs/holds/cancel_expired_holds.pl again
10) The hold should now be canceled

Patch tested with a sandbox, by Leila, Sonia and Frido <koha.aixmarseille@gmail.com>

Kyle, why didn't you use Koha::Calendar instead of C4::Calendar ?

This patch should use Koha::Calendar instead of C4::Calendar.
Marked as Failed QA.

Hey Kyle, I couldn't get the patch to apply on the latest master.

Applying: Bug 8735 - Expire holds waiting only on days the library is open
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Failed to merge in the changes.

As for the actual bug, I'm not sure that I could reproduce it. Before applying the patch, I used your test plan to the letter (expecting for my hold to disappear on a holiday), but when I set my "waiting date" to yesterday and ran "misc/cronjobs/holds/cancel_expired_holds.pl", it didn't clear out the reserve (regardless of the day being a holiday). If I set it for the day before yesterday (both as a holiday and not), I got:

DBD::mysql::st execute failed: Duplicate entry '60' for key 'PRIMARY' at dev/lib//C4/Reserves.pm line 1051.

It cleared out the entry from the reserves table, but that doesn't seem very elegant. Is that how it's supposed to look?

--

I guess what I'm asking is...could your test plan include a "Before patch" and an "After patch" so that I can see what buggy behaviour is being fixed?

> As for the actual bug, I'm not sure that I could reproduce it. Before
> applying the patch, I used your test plan to the letter (expecting for my
> hold to disappear on a holiday), but when I set my "waiting date" to
> yesterday and ran "misc/cronjobs/holds/cancel_expired_holds.pl", it didn't
> clear out the reserve (regardless of the day being a holiday). If I set it
> for the day before yesterday (both as a holiday and not), I got:
> 
> DBD::mysql::st execute failed: Duplicate entry '60' for key 'PRIMARY' at
> dev/lib//C4/Reserves.pm line 1051.
> 
> It cleared out the entry from the reserves table, but that doesn't seem very
> elegant. Is that how it's supposed to look?

The test plan should have had "the day before yesterday" instead of "yesterday". I don't know why you would be getting an error like that ( I get no errors when I test this patch ), but it sounds like your reserves and old_reserves tables are messed up. I think you may have duplicate reserve_id's between the two tables, which would cause that error. I do not believe your error has anything to do with this patch though.

Applying: Bug 8735 - Expire holds waiting only on days the library is open
Using index info to reconstruct a base tree...
M	C4/Reserves.pm
M	installer/data/mysql/sysprefs.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
Patch failed at 0001 Bug 8735 - Expire holds waiting only on days the library is open
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

I tried to fix the conflicts, but the conflict in the .sql is too massive for me to do safely

Kyle, could you provide unit tests please?

Hm, it looks like the last thing done here was fixing the patch again - should this be 'Needs Signoff'?

I think the followup is so small it should be considered a qa followup.

Ah, I see now that I had missed Jonathan's comment 28: 'Kyle, could you provide unit tests please?' I second that

Hi Kyle,

a few small things:
- database update is missing the bug number and a )
- there is a .. in the pref description
... I would have fixed those, but:

ok 29 - Hold cannot be trapped for damaged item with AllowHoldsOnDamagedItems disabled
No branchcode argument to new.  Should be C4::Calendar->new(branchcode => $branchcode) at t/db_dependent/Holds.t line 299
# Looks like you planned 32 tests but ran 29.
# Looks like your test exited with 255 just after 29.

Trivial rebase.
Will take a look now for QA.

Created attachment 30399 [details] [review]
Bug 8735 - Expire holds waiting only on days the library is open

Signed-off-by: Leila <koha.aixmarseille@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 30400 [details] [review]
Bug 8735 - Expire holds waiting only on days the library is open - Followup - Switch from C4::Calendar to Koha::Calendar

Test Plan:
 1) Set ExpireReservesMaxPickUpDelay
 2) Set ReservesMaxPickUpDelay to 1
 3) Place a hold, set it to waiting
 4) Using the MySQL console, modify the waiting date and set it to the
    day before yesterday.
 5) Set today as a holiday for the pickup branch in question.
 6) Run misc/cronjobs/holds/cancel_expired_holds.pl
 7) The hold should remain unchanged
 8) Remove today as a holiday
 9) Run misc/cronjobs/holds/cancel_expired_holds.pl again
10) The hold should now be canceled

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 30401 [details] [review]
Bug 8735 [QA Followup] - Add Unit Tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 30402 [details] [review]
Bug 8735 [QA Followup 2] - Remove extra period, add bug number to db update

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Patches pushed to master.

Thanks and nice job, Kyle!