Bug 14526 - MoveReserve should look at future holds too
Summary: MoveReserve should look at future holds too
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-13 13:14 UTC by Marcel de Rooy
Modified: 2016-06-21 21:40 UTC (History)
4 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 14526: MoveReserve should look at future holds too (2.35 KB, patch)
2015-07-15 13:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14526: Add some unit tests for MoveReserve (4.39 KB, patch)
2015-07-15 13:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14526: Add some unit tests for MoveReserve (4.39 KB, patch)
2015-07-30 19:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14526: MoveReserve should look at future holds too (2.35 KB, patch)
2015-08-24 06:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14526: Add some unit tests for MoveReserve (4.55 KB, patch)
2015-08-24 06:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14526: Add some unit tests for MoveReserve (4.33 KB, patch)
2015-08-27 08:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14526: (follow-up) add a space before equals sign (966 bytes, patch)
2015-08-31 05:08 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 14526: MoveReserve should look at future holds too (2.45 KB, patch)
2015-08-31 05:16 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 14526: Add some unit tests for MoveReserve (4.43 KB, patch)
2015-08-31 05:47 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 14526: (follow-up) add a space before equals sign (1.04 KB, patch)
2015-09-01 10:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14526: MoveReserve should look at future holds too (2.52 KB, patch)
2015-09-01 13:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14526: Add some unit tests for MoveReserve (4.51 KB, patch)
2015-09-01 13:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14526: (follow-up) add a space before equals sign (1.07 KB, patch)
2015-09-01 13:00 UTC, Jonathan Druart
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 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!