Bug 8735 - Expire holds waiting only on days the library is open
Summary: Expire holds waiting only on days the library is open
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 9004
  Show dependency treegraph
 
Reported: 2012-09-06 15:15 UTC by Kyle M Hall
Modified: 2015-06-04 23:32 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8735 - Expire holds waiting only on days the library is open (4.14 KB, patch)
2012-09-06 15:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (4.14 KB, patch)
2012-09-06 15:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (5.54 KB, patch)
2012-11-09 17:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (5.20 KB, patch)
2012-11-09 17:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (6.50 KB, patch)
2013-02-05 20:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (5.22 KB, patch)
2013-02-05 20:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (5.98 KB, patch)
2013-02-06 13:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (5.65 KB, patch)
2013-02-06 13:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (5.07 KB, patch)
2013-02-06 14:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8735 - Expire holds waiting only on days the library is open (5.16 KB, patch)
2013-03-20 12:37 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open - Followup - Switch from C4::Calendar to Koha::Calendar (1.79 KB, patch)
2013-04-23 16:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (5.54 KB, patch)
2013-05-02 11:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open - Followup - Switch from C4::Calendar to Koha::Calendar (2.27 KB, patch)
2013-05-02 11:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (5.26 KB, patch)
2014-01-21 20:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open - Followup - Switch from C4::Calendar to Koha::Calendar (2.27 KB, patch)
2014-01-21 20:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (5.29 KB, patch)
2014-01-30 17:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open - Followup - Switch from C4::Calendar to Koha::Calendar (2.27 KB, patch)
2014-01-30 17:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 [QA Followup] - Add Unit Tests (2.89 KB, patch)
2014-04-17 18:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (5.29 KB, patch)
2014-06-04 18:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open - Followup - Switch from C4::Calendar to Koha::Calendar (2.27 KB, patch)
2014-06-04 18:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 [QA Followup] - Add Unit Tests (2.71 KB, patch)
2014-06-04 18:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 [QA Followup 2] - Remove extra period, add bug number to db update (1.93 KB, patch)
2014-06-04 18:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (5.33 KB, patch)
2014-07-31 12:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open - Followup - Switch from C4::Calendar to Koha::Calendar (2.31 KB, patch)
2014-07-31 12:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8735 [QA Followup] - Add Unit Tests (2.75 KB, patch)
2014-07-31 12:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8735 [QA Followup 2] - Remove extra period, add bug number to db update (1.97 KB, patch)
2014-07-31 12:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open (5.39 KB, patch)
2014-07-31 13:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8735 - Expire holds waiting only on days the library is open - Followup - Switch from C4::Calendar to Koha::Calendar (2.36 KB, patch)
2014-07-31 13:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8735 [QA Followup] - Add Unit Tests (2.81 KB, patch)
2014-07-31 13:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8735 [QA Followup 2] - Remove extra period, add bug number to db update (2.03 KB, patch)
2014-07-31 13:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-09-06 15:15:06 UTC
There is a problem with expiring reserves. 

Let us say an items has two holds placed on it, and is set to waiting for the first hold. This hold is then canceled for waiting too long, yet it will remain sitting on the holds shelf until a librarian either returns it to the shelf, or sets it too waiting for the next person in line to get it. 

During the interim, the item will appear to be available for checkout. This is not a problem most of the time, as the holds expire late at night and the librarians will take care of it in the morning, but over a holiday weekend, it is possible that an item will remain in this state for days at a time.

We can fix this situation by adding a system preference to decide of waiting holds should be canceled on holidays, or not.
Comment 1 Kyle M Hall 2012-09-06 15:21:34 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-09-06 15:22:39 UTC Comment hidden (obsolete)
Comment 3 Jared Camins-Esakov 2012-11-07 02:53:40 UTC
This patch seems to be missing a change in a .pref file. Please add this file and attach a new patch.
Comment 4 Kyle M Hall 2012-11-09 17:26:20 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2012-11-09 17:27:05 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2012-11-09 17:28:51 UTC
(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!
Comment 7 Liz Rea 2012-12-28 02:29:49 UTC
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>
Comment 8 Liz Rea 2013-01-06 21:10:10 UTC
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.
Comment 9 Kyle M Hall 2013-01-07 12:26:58 UTC
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
Comment 10 Kyle M Hall 2013-02-05 20:33:32 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2013-02-05 20:34:34 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2013-02-06 13:55:51 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2013-02-06 13:57:49 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2013-02-06 14:05:47 UTC Comment hidden (obsolete)
Comment 15 Paul Poulain 2013-03-20 11:23:58 UTC
Patch tested with a sandbox, by Leila, Sonia and Frido <koha.aixmarseille@gmail.com>
Comment 16 Fridolin SOMERS 2013-03-20 12:37:21 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2013-04-02 10:04:33 UTC
Kyle, why didn't you use Koha::Calendar instead of C4::Calendar ?
Comment 18 Jonathan Druart 2013-04-23 09:40:47 UTC
This patch should use Koha::Calendar instead of C4::Calendar.
Marked as Failed QA.
Comment 19 Kyle M Hall 2013-04-23 16:32:31 UTC Comment hidden (obsolete)
Comment 20 David Cook 2013-05-02 08:35:06 UTC
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?
Comment 21 Kyle M Hall 2013-05-02 11:48:32 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2013-05-02 11:48:46 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2013-05-02 11:53:16 UTC
> 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.
Comment 24 I'm just a bot 2013-09-18 11:12:52 UTC
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".
Comment 25 Chris Cormack 2013-12-23 20:06:12 UTC
I tried to fix the conflicts, but the conflict in the .sql is too massive for me to do safely
Comment 26 Kyle M Hall 2014-01-21 20:51:45 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2014-01-21 20:52:13 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2014-01-24 13:09:44 UTC
Kyle, could you provide unit tests please?
Comment 29 Kyle M Hall 2014-01-30 17:01:29 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2014-01-30 17:01:34 UTC Comment hidden (obsolete)
Comment 31 Katrin Fischer 2014-04-15 08:36:30 UTC
Hm, it looks like the last thing done here was fixing the patch again - should this be 'Needs Signoff'?
Comment 32 Kyle M Hall 2014-04-17 11:12:49 UTC
I think the followup is so small it should be considered a qa followup.
Comment 33 Katrin Fischer 2014-04-17 14:56:29 UTC
Ah, I see now that I had missed Jonathan's comment 28: 'Kyle, could you provide unit tests please?' I second that
Comment 34 Kyle M Hall 2014-04-17 18:33:39 UTC Comment hidden (obsolete)
Comment 35 Katrin Fischer 2014-04-20 09:20:22 UTC
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.
Comment 36 Kyle M Hall 2014-06-04 18:23:56 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2014-06-04 18:24:27 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2014-06-04 18:24:30 UTC Comment hidden (obsolete)
Comment 39 Kyle M Hall 2014-06-04 18:24:33 UTC Comment hidden (obsolete)
Comment 40 Marcel de Rooy 2014-07-31 12:42:02 UTC Comment hidden (obsolete)
Comment 41 Marcel de Rooy 2014-07-31 12:42:09 UTC Comment hidden (obsolete)
Comment 42 Marcel de Rooy 2014-07-31 12:42:15 UTC Comment hidden (obsolete)
Comment 43 Marcel de Rooy 2014-07-31 12:42:20 UTC Comment hidden (obsolete)
Comment 44 Marcel de Rooy 2014-07-31 12:42:47 UTC
Trivial rebase.
Will take a look now for QA.
Comment 45 Marcel de Rooy 2014-07-31 13:36:57 UTC
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>
Comment 46 Marcel de Rooy 2014-07-31 13:37:01 UTC
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>
Comment 47 Marcel de Rooy 2014-07-31 13:37:05 UTC
Created attachment 30401 [details] [review]
Bug 8735 [QA Followup] - Add Unit Tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 48 Marcel de Rooy 2014-07-31 13:37:09 UTC
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>
Comment 49 Tomás Cohen Arazi 2014-09-05 15:34:03 UTC
Patches pushed to master.

Thanks and nice job, Kyle!