Bug 14526

Summary: MoveReserve should look at future holds too
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Hold requestsAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: gmcharlt, jonathan.druart, joonas.kylmala, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9809
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 14526: MoveReserve should look at future holds too
Bug 14526: Add some unit tests for MoveReserve
Bug 14526: Add some unit tests for MoveReserve
Bug 14526: MoveReserve should look at future holds too
Bug 14526: Add some unit tests for MoveReserve
Bug 14526: Add some unit tests for MoveReserve
Bug 14526: (follow-up) add a space before equals sign
Bug 14526: MoveReserve should look at future holds too
Bug 14526: Add some unit tests for MoveReserve
Bug 14526: (follow-up) add a space before equals sign
Bug 14526: MoveReserve should look at future holds too
Bug 14526: Add some unit tests for MoveReserve
Bug 14526: (follow-up) add a space before equals sign

Description Marcel de Rooy 2015-07-13 13:14:13 UTC
MoveReserve is called when checking out a book (AddIssue).
When filling a hold, it moves a reserve to old_reserves (with ModReserveFill).

But if the hold is a future one (say tomorrow), and it is confirmed today and waiting for pickup, and the patron comes a day sooner, it should be moved too when checking out.

Small patch forthcoming.
Comment 1 Marcel de Rooy 2015-07-15 13:06:00 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2015-07-15 13:06:04 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2015-07-30 19:12:41 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-08-21 10:31:59 UTC
Be care, AddReserve prototype changed by bug 14526 (contrainst param removed).
Comment 5 Jonathan Druart 2015-08-21 10:39:45 UTC
I meant bug 9809.
Comment 6 Marcel de Rooy 2015-08-24 06:41:54 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2015-08-24 06:42:00 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2015-08-24 06:43:50 UTC
Rebased (unit test for bug 14464)
Comment 9 Marcel de Rooy 2015-08-27 08:55:02 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2015-08-27 08:55:21 UTC
Unit test rebased for 9809
Comment 11 Joonas Kylmälä 2015-08-31 05:08:08 UTC Comment hidden (obsolete)
Comment 12 Joonas Kylmälä 2015-08-31 05:16:01 UTC Comment hidden (obsolete)
Comment 13 Joonas Kylmälä 2015-08-31 05:47:17 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2015-09-01 10:40:17 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2015-09-01 10:42:05 UTC
Thx Joonas for signing off.
I have moved your patch to the third position in the list to prevent problems applying them.
Please take care to keep patches on a report in the right order. Thanks.
Comment 16 Jonathan Druart 2015-09-01 13:00:46 UTC
Created attachment 42150 [details] [review]
Bug 14526: MoveReserve should look at future holds too

At checkout a hold for the same borrower is considered to be filled.
It is consistent to do the same for holds of the same borrower within
[ConfirmFutureHolds] days (if non-zero).

This goal is achieved by adjusting the CheckReserves call within
MoveReserve, adding the lookahead parameter.
I used this occasion to revisit other calls of CheckReserves:
- transferbook: no need to add lookahead; a future hold should not block
  a transfer;
- CanBookBeIssued: no lookahead; future hold does not block an issue;
- CanBookBeRenewed: idem.
- GetOtherReserves (only used in circ/returns): this call might be a
  candidate for lookahead too, but I leave that for another report. It is
  in the context of checkin and transfer, not checkout.

Test plan:
[1] Set ConfirmFutureHolds to zero days. (You may also need to enable
    AllowHoldDateInFuture.)
[2] Place a hold with borrower A on biblio X for tomorrow. Also place a hold
    with borrower B on X for today. (Use biblio level holds.)
[3] Check out item Y of X to borrower A. Ignore the warning for borrower B
    and do not cancel the hold of B (so: confirm checkout).
    Verify that X has still two holds.
[4] Check in Y (without confirming a hold).
[5] Enable ConfirmFutureHolds, say 2 days.
[6] Check out Y to A again. Ignore the warning for B (no cancel). Verify that
    X now only has one hold for borrower B (the hold for A was filled).

Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2015-09-01 13:00:50 UTC
Created attachment 42151 [details] [review]
Bug 14526: Add some unit tests for MoveReserve

The change in MoveReserve from the previous patch begs for a test.
Here we add some to Reserves.t.
In all six tests we place a hold, move it and check the reserves again.
Depending on the status of ConfirmFutureHolds, and the reservedate the
hold should be moved or not.

Test plan:
Run the unit test.
Bonus: If you run Reserves.t by applying this patch but without the first
patch that changed MoveReserve, tests 60 and 61 should fail:
    not ok 60 - MoveReserve filled future hold now
    not ok 61 - MoveReserve filled future waiting hold now
This may further illustrate the need of the first patch.

Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2015-09-01 13:00:56 UTC
Created attachment 42152 [details] [review]
Bug 14526: (follow-up) add a space before equals sign

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Tomás Cohen Arazi 2015-09-07 14:28:01 UTC
Patches pusehd to master.

Thanks Marcel!