[Bug 14526] New: MoveReserve should look at future holds too
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Bug ID: 14526 Summary: MoveReserve should look at future holds too Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: minor Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |m.de.rooy@rijksmuseum.nl |ity.org | Status|NEW |ASSIGNED Patch complexity|--- |Small patch -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 --- Comment #1 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41010 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41010&action=edit 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). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41011 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41011&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41011|0 |1 is obsolete| | --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41241 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41241&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=9809 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Be care, AddReserve prototype changed by bug 14526 (contrainst param removed). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14337 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14155 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14097 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13930 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13903 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=12632 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I meant bug 9809. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14337 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14155 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14097 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13930 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13903 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also|http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=12632 | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41010|0 |1 is obsolete| | --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41807 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41807&action=edit 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). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41241|0 |1 is obsolete| | --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 41808 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=41808&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Rebased (unit test for bug 14464) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41808|0 |1 is obsolete| | --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 42014 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42014&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Unit test rebased for 9809 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 --- Comment #11 from Joonas Kylmälä <j.kylmala@gmail.com> --- Created attachment 42100 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42100&action=edit Bug 14526: (follow-up) add a space before equals sign -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Joonas Kylmälä <j.kylmala@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #41807|0 |1 is obsolete| | --- Comment #12 from Joonas Kylmälä <j.kylmala@gmail.com> --- Created attachment 42101 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42101&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Joonas Kylmälä <j.kylmala@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42014|0 |1 is obsolete| | --- Comment #13 from Joonas Kylmälä <j.kylmala@gmail.com> --- Created attachment 42102 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42102&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Joonas Kylmälä <j.kylmala@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |j.kylmala@gmail.com Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42100|0 |1 is obsolete| | --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 42144 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42144&action=edit Bug 14526: (follow-up) add a space before equals sign Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 --- Comment #15 from 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #42101|0 |1 is obsolete| | Attachment #42102|0 |1 is obsolete| | Attachment #42144|0 |1 is obsolete| | --- Comment #16 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 42150 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42150&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 42151 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42151&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 42152 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=42152&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14526 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |tomascohen@gmail.com --- Comment #19 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pusehd to master. Thanks Marcel! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org