Bug 9761 - Make it possible to confirm future hold requests at checkin time
Summary: Make it possible to confirm future hold requests at checkin time
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact:
URL:
Keywords:
Depends on: 9367
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-07 12:31 UTC by Marcel de Rooy
Modified: 2014-12-07 20:03 UTC (History)
3 users (show)

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


Attachments
Bug 9761: Make it possible to confirm future hold requests at checkin time (11.31 KB, patch)
2013-03-07 14:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9761: dbrev for ConfirmFutureHolds (2.32 KB, patch)
2013-03-07 14:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9761: Reformulating the preference description (1.77 KB, patch)
2013-03-07 17:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9761: Make it possible to confirm future hold requests at checkin time (11.33 KB, patch)
2013-03-08 15:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9761: dbrev for ConfirmFutureHolds (2.29 KB, patch)
2013-03-08 15:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9761: Reformulating the preference description (1.78 KB, patch)
2013-03-08 15:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9761: Make it possible to confirm future hold requests at checkin time (11.33 KB, patch)
2013-06-03 10:15 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9761: dbrev for ConfirmFutureHolds (2.34 KB, patch)
2013-06-03 10:15 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9761: dbrev for ConfirmFutureHolds (2.01 KB, patch)
2013-06-03 10:15 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9761: dbrev for ConfirmFutureHolds (2.34 KB, patch)
2013-06-03 10:17 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9761: Make it possible to confirm future hold requests at checkin time (11.38 KB, patch)
2013-06-03 10:18 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9761: dbrev for ConfirmFutureHolds (2.40 KB, patch)
2013-06-03 10:19 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9761: Reformulating the preference description (1.85 KB, patch)
2013-06-03 10:20 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 9761: Make it possible to confirm future hold requests at checkin time (11.42 KB, patch)
2013-09-18 12:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9761: dbrev for ConfirmFutureHolds (2.73 KB, patch)
2013-09-18 12:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9761: Reformulating the preference description (1.87 KB, patch)
2013-09-18 12:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9761: Preliminary measures for adding a unit test (1.92 KB, patch)
2013-09-18 14:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9761: Preliminary measures for adding a unit test (1.93 KB, patch)
2013-09-18 14:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9761: Unit tests for ConfirmFutureHolds changes (4.45 KB, patch)
2013-09-19 09:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9761: Unit tests for ConfirmFutureHolds changes (4.46 KB, patch)
2013-09-20 15:25 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2013-03-07 12:31:29 UTC
Make it possible to confirm future hold requests at checkin time.
Part of developments for bug 9721.
Comment 1 Marcel de Rooy 2013-03-07 14:09:58 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2013-03-07 14:11:19 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2013-03-07 14:32:41 UTC
Will also add some comments as results of code analysis for these patches here too.
Comment 4 Marcel de Rooy 2013-03-07 17:50:14 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2013-03-08 15:44:04 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2013-03-08 15:44:11 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2013-03-08 15:44:18 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2013-03-13 15:03:00 UTC
Just some comments on the changed code (making life somewhat easier for QA or RM most hopefully :)

Main change is in _Findgroupreserve: the where clause is adjusted to optionally include future reserve dates.

This routine is *only* used in CheckReserves :)
The circulation module calls CheckReserves for transferbook, CanBookBeIssued, AddReturn, CanBookBeRenewed.
This change is only meant for AddReturn.

When writing this patch, the Search and XSLT module used CheckReserves also for displaying status information. Bug 9367 converts that to GetReserveStatus, implicitly supporting this change too. Note that bug 9788 makes some adjustments on top of that, improving item status for future waits.

In the Reserves module, CheckReserves is used by GetOtherReserves (when confirming the hold: but you first need to recognize it!) and MoveReserve (for checkout: not relevant here). 

The call of CheckReserves after calling CanBookBeRenewed in circulation.pl is somewhat dubious.
The calls in opac-user.pl and opac-detail.pl are also for displaying status info (see above with reference to the other reports again).
Comment 9 Chris Cormack 2013-06-03 09:43:36 UTC
Comment on attachment 15980 [details] [review]
Bug 9761: Make it possible to confirm future hold requests at checkin time

Review of attachment 15980 [details] [review]:
-----------------------------------------------------------------

::: C4/Reserves.pm
@@ +1810,4 @@
>            AND reserves.reservedate    = reserveconstraints.reservedate )
>            OR  reserves.constrainttype='a' )
>            AND (reserves.itemnumber IS NULL OR reserves.itemnumber = ?)
> +          AND reserves.reservedate <= DATE_ADD(NOW(),INTERVAL ? DAY)

I was a little worried about adding this constraint, because this column is not indexed, but I tested with some explains and it should be ok.
Comment 10 Chris Cormack 2013-06-03 09:59:12 UTC
Passes all QA tests, testing for regressions now
Comment 11 Chris Cormack 2013-06-03 10:15:20 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2013-06-03 10:15:37 UTC Comment hidden (obsolete)
Comment 13 Chris Cormack 2013-06-03 10:15:53 UTC Comment hidden (obsolete)
Comment 14 Chris Cormack 2013-06-03 10:17:07 UTC Comment hidden (obsolete)
Comment 15 Chris Cormack 2013-06-03 10:18:43 UTC Comment hidden (obsolete)
Comment 16 Chris Cormack 2013-06-03 10:19:41 UTC Comment hidden (obsolete)
Comment 17 Chris Cormack 2013-06-03 10:20:17 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2013-09-18 12:12:43 UTC
Created attachment 21193 [details] [review]
Bug 9761: Make it possible to confirm future hold requests at checkin time

Description:
A new pref ConfirmFutureHolds is added. When confirming a hold at checkin time,
the number of days in this pref is taken into account when looking for reserves.
Note that this pref does not interfere with renewing, issuing or transferring
a book. For report Holds to pull, the default end date is calculated with this
new preference.
The use of ConfirmFutureHolds is useful only when future holds are allowed.

Test plan:
1) Enable future holds. Add a number of days into ConfirmFutureHolds.
2) Place a future hold within this number of days.
3) Run holds to pull report. Check default startdate and enddate.
4) Check this book in. Can you confirm the hold? Do not confirm.
5) Issue the book to another patron. You should not see a warning.
6) Renew the book for this patron via opac or staff. No warning either.
7) Check in again. Warning pops up again.
8) Transfer book. Switch branch. Check in. Hold found pops up. Do not confirm.
9) Back to first branch. Check in (with popup). Remove the hold. Add new future
hold past the number of days. Check in (no warn).

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 19 Marcel de Rooy 2013-09-18 12:21:54 UTC
Created attachment 21197 [details] [review]
Bug 9761: dbrev for ConfirmFutureHolds

Db rev for new pref ConfirmFutureHolds.

Test plan:
Run [your server]/cgi-bin/koha/installer/install.pl?step=3&op=updatestructure
Optionally, run a new install to test sysprefs.sql.
Or:
Edit sysprefs.sql, leave at least the last few lines including the new one.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 20 Marcel de Rooy 2013-09-18 12:22:35 UTC
Created attachment 21198 [details] [review]
Bug 9761: Reformulating the preference description

After some thought, I had to reformulate what this pref does.
No code changes here.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 21 Marcel de Rooy 2013-09-18 12:24:32 UTC
Rebased two patches for trivial reasons: withdrawn change, lexical order of sysprefs and updatedatabase.
Note that this set of patches is already in the Passed QA queue since June 3 without any visible attention after that time..
Comment 22 Marcel de Rooy 2013-09-18 14:34:55 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2013-09-18 14:38:51 UTC
Created attachment 21209 [details] [review]
Bug 9761: Preliminary measures for adding a unit test

Optionally add some branches and categories that may not exist.

Test plan:
Run the test with or without CPL branch or S (staff) category.
Verify that the test does not fail.
Comment 24 Marcel de Rooy 2013-09-18 14:40:58 UTC
Please wait for another followup adding a few specific tests
Comment 25 Marcel de Rooy 2013-09-19 09:46:59 UTC Comment hidden (obsolete)
Comment 26 Marcel de Rooy 2013-09-19 09:50:27 UTC
Will send a message if Chris and/or Kyle would be willing to run the adjusted Reserves.t unit test and provide an additional signoff.
Since these two (QA) followups just serve the purpose of getting these patches further, I did not change the report status for now.
Comment 27 Kyle M Hall 2013-09-20 15:25:36 UTC
Created attachment 21280 [details] [review]
Bug 9761: Unit tests for ConfirmFutureHolds changes

Adds tests for CheckReserves with lookahead parameter.
Adds tests for AddReturn with regard to future reserve messages.
The following test cases are added, resulting in 8 new tests:
a) Add a reserve without date, CheckReserve should return it
b) Add a reserve with future date, CheckReserve should not return it
c) Add a reserve with future date, CheckReserve should return it if lookahead
   is high enough
d) Check ResFound message of AddReturn for future hold

Test plan:
Run the test. No fails?

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Marcel de Rooy 2013-09-20 16:32:50 UTC
(In reply to Kyle M Hall from comment #27)
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Great. Thanks.
Comment 29 Galen Charlton 2013-09-25 00:42:41 UTC
Pushed to master.  Thanks, Marcel!