Description
Marcel de Rooy
2015-07-13 13:14:13 UTC
Created attachment 41010 [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). Created attachment 41011 [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. Created attachment 41241 [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. Be care, AddReserve prototype changed by bug 14526 (contrainst param removed). Created attachment 41807 [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). Created attachment 41808 [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. Created attachment 42014 [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. Unit test rebased for 9809 Created attachment 42100 [details] [review] Bug 14526: (follow-up) add a space before equals sign Created attachment 42101 [details] [review] Bug 14526: MoveReserve should look at future holds too Test plan works. Did also my own checking and tried to find any problems this change might cause. Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Created attachment 42102 [details] [review] Bug 14526: Add some unit tests for MoveReserve Test plan worked. Code looks just what it should look like. Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Created attachment 42144 [details] [review] Bug 14526: (follow-up) add a space before equals sign Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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. 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> 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> 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> Patches pusehd to master. Thanks Marcel! |