[Bug 34972] New: Canceling a waiting hold from the holds over tab can make the next hold unfillable
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Bug ID: 34972 Summary: Canceling a waiting hold from the holds over tab can make the next hold unfillable Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Hold requests Assignee: koha-bugs@lists.koha-community.org Reporter: emily.lamancusa@montgomerycountymd.gov QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com When a staff member cancels a hold by clicking the button in the "holds waiting over 10 days" report, Koha incompletely attempts to assign that item to the next matching hold. Koha assigns the itemnumber to the hold, but does *not* set found status of the hold. Instead, it prompts the staff member, "Please retain this item and check it in to process the hold." This causes a few problems: - The itemnumber is immediately assigned to the hold without an opportunity for staff input, even if HoldsAutoFill is disabled. - If the staff member does not check in the book afterward (whether intentionally or through human error), it leaves the hold in an invalid state: it is tied to the itemnumber, but it is neither "found" nor an item-level hold. - Since the itemnumber is assigned, other items can't fill the hold. However, since the "found" field was never set, the option to "revert waiting status" or "revert transit status" isn't available. Essentially, a bib-level hold is suddenly behaving like an item-level hold, with no way for staff to identify or fix the issue. This is especially a problem if the item is missing from the holds shelf for some reason, and the staff member cancels the waiting hold first and then marks the item "lost". This makes the next-in-line hold unfillable unless the item is found. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |emily.lamancusa@montgomeryc |ity.org |ountymd.gov -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #1 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- It looks like the immediate bug is in GetOtherReserves in C4::Reserves. GetOtherReserves is written as though it should update the matched hold to be either waiting or in-transit, but it doesn't actually do so - it ends up just assigning the itemnumber. There are three possible scenarios here: a) The matched hold was already found. The same itemnumber as before is set. Status is unaffected. Result: nothing changes, and the hold was already in the correct state to begin with. No problem. b) The matched hold was an item-level hold. The same itemnumber as before is set. Status is unaffected. Result: nothing changes, and the hold remains in a valid state. No problem (unless the caller was expecting the hold to be Found, in which case that would be a separate bug). c) The matched hold is the highest-priority, unassigned bib-level hold. The itemnumber is set. Status is unaffected. Result: a specific item is now assigned to the hold, but the hold is not Found (e.g. waiting, in-transit, or in processing). The bib-level hold has been unintentionally converted to an item-level hold with no notification and no way to revert. In practice, GetOtherReserves does not alter the hold in any meaningful way EXCEPT for producing this invalid state in a few niche situations. In all of those niche situations, the user is prompted to explicitly confirm, cancel, or revert the hold. In the few niche situations where the user is able to ignore the prompt (thereby leaving the hold in the invalid state), the hold status shouldn't be updated anyway. We should simply remove the code from GetOtherReserves that attempts to modify the hold. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #2 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Also making a note for later: GetOtherReserves should probably be removed entirely. Once the bad code is removed, it's basically just a wrapper for CheckReserves that returns a trivial additional message. It's also only called in two places, and has almost no test coverage. ModReserveMinusPriority (which is called by GetOtherReserves and is the actual culprit here) should also be removed. It also has almost no test coverage, only has one remaining reference outside of GetOtherReserves, and is currently redundant and unnecessary in that place also. Will make follow-up bugs for those after I upload patches for this one. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #3 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 160775 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=160775&action=edit Bug 34972: Add unit tests for GetOtherReserves To test: 1. Apply this patch only 2. Prove -v t/db_dependent/Circulation.t --> Tests fail --> Note verbose output for Tests for GetOtherReserves: ok 3 - GetOtherReserves should not set found status not ok 4 - GetOtherReserves should not set itemnumber on biblio-level hold --> The itemnumber is set on a biblio-level hold, but the found status is not set! 3. Apply the other patch 4. Prove -v t/db_dependent/Circulation.t --> Tests pass -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #4 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 160776 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=160776&action=edit Bug 34972: GetOtherReserves should not alter hold states GetOtherReserves attempts to set the waiting/transit status for the next hold on the list when applicable, but in practice it either leaves the hold state unchanged, or sets the itemnumber without setting the found status (erroneously converting bib-level holds to item-level holds). The latter situation only occurs when the user has been prompted to confirm, cancel, or revert the hold, and is able to ignore the prompt. In those situations, the hold's state should not change. GetOtherReserves does not need to change the hold's state, and attempting to do so produces no effect except for erroneously converting bib-level holds to item-level holds in certain situations, so this patch removes that code. To test: 1. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 2. Check in an item from that record to fill Patron A's hold 3. Set the hold's expiration date to yesterday by accessing the database in the command line: - In a ktd shell prompt, open the db client with koha-mysql kohadev - UPDATE reserves SET expirationdate = DATE_SUB(CURDATE(), INTERVAL 1 DAY) WHERE borrowernumber = <Patron A's borrowernumber> 4. Go to Circulation > Holds Awaiting Pickup, and find the hold in the "holds waiting past their expiration date" tab 5. Click the "Cancel hold" button in the Actions column next to the hold 6. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 7. Cancel Patron B's hold 8. Place 2 new holds on the record: one for Patron A at the logged-in library, and one for Patron B at a different library 9. Check in an item to fill Patron A's hold 10. Repeat steps 3-5 to expire and cancel Patron A's hold 11. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a transit status 12. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 13. Check in an item from that record to fill Patron A's hold 14. Check in the same item again. A modal will pop up, saying that the hold is already waiting 15. In the modal, choose a cancellation reason and click "Cancel hold" --> A new modal will pop up to fill Patron B's hold 16. Click "Ignore" on the modal for Patron B's hold 17. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 18. Apply patch 19. Repeat steps 1-6 --> Note that Patron B's hold is still a bib-level/"next available" hold 20. Repeat steps 7-11 --> Note that Patron B's hold is still a bib-level/"next available" hold 21. Repeat steps 12-17 --> Note that Patron B's hold is still a bib-level/"next available" hold -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |david@davidnind.com --- Comment #5 from David Nind <david@davidnind.com> --- Thanks for the great test plan Emily! The tests failed for me after applying the second patch: 1. One test failed: not ok 58 - Checkout should correctly terminate a transfer 2. The number of tests to run needs increasing from 67 to 68: "Looks like you planned 67 tests but ran 68" Testing notes (using KTD): 1. For the patrons, I used Mary (Patron A) and Henry (Patron B). 2. For the record, I used Programming perl (biblionumber = 262). One other thing noted - both before and after patches, so not related to this bug: Step 19 + 20 - Repeating step 5 Message is incomplete - after pick-up at, no location is shown. The message says: "This item is on hold for pick-up at Programming Perl / is on hold for Acevedo, Henry. Please retain this item and check it in to process the hold. OK" -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #6 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Thanks for testing, David! It looks like returns.pl is using GetOtherReserves to initiate the item transfer (i.e. add it to the branchtransfers table) - but not to put the hold into the Transit status. In fact, GetOtherReserves doesn't even try to put the hold into the Transit status. returns.pl already imports ModItemTransfer...why not trigger the item transfer directly when confirming the hold? (Notably, the current code calls GetOtherReserves both before and after prompting the user to confirm the transfer, so it's initiating a transfer before confirming whether it should!) I still think the way to go is remove side effects from GetOtherReserves, and have returns.pl initiate the transfer directly (at the correct time). I'll work on a follow-up. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Laura Escamilla <Laura.escamilla@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |Laura.escamilla@bywatersolu | |tions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Christine <chlee@pascolibraries.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |chlee@pascolibraries.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #160775|0 |1 is obsolete| | --- Comment #7 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 163471 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163471&action=edit Bug 34972: Add unit tests for GetOtherReserves To test: 1. Apply this patch only 2. Prove -v t/db_dependent/Circulation.t --> Tests fail --> Note verbose output for Tests for GetOtherReserves: ok 3 - GetOtherReserves should not set found status not ok 4 - GetOtherReserves should not set itemnumber on biblio-level hold --> The itemnumber is set on a biblio-level hold, but the found status is not set! 3. Apply the other patch 4. Prove -v t/db_dependent/Circulation.t --> Tests pass -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #160776|0 |1 is obsolete| | --- Comment #8 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 163472 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163472&action=edit Bug 34972: GetOtherReserves should not alter hold states GetOtherReserves attempts to set the waiting/transit status for the next hold on the list when applicable, but in practice it either leaves the hold state unchanged, or sets the itemnumber without setting the found status (erroneously converting bib-level holds to item-level holds). The latter situation only occurs when the user has been prompted to confirm, cancel, or revert the hold, and is able to ignore the prompt. In those situations, the hold's state should not change. GetOtherReserves does not need to change the hold's state, and attempting to do so produces no effect except for erroneously converting bib-level holds to item-level holds in certain situations, so this patch removes that code. To test: 1. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 2. Check in an item from that record to fill Patron A's hold 3. Set the hold's expiration date to yesterday by accessing the database in the command line: - In a ktd shell prompt, open the db client with koha-mysql kohadev - UPDATE reserves SET expirationdate = DATE_SUB(CURDATE(), INTERVAL 1 DAY) WHERE borrowernumber = <Patron A's borrowernumber> 4. Go to Circulation > Holds Awaiting Pickup, and find the hold in the "holds waiting past their expiration date" tab 5. Click the "Cancel hold" button in the Actions column next to the hold 6. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 7. Cancel Patron B's hold 8. Place 2 new holds on the record: one for Patron A at the logged-in library, and one for Patron B at a different library 9. Check in an item to fill Patron A's hold 10. Repeat steps 3-5 to expire and cancel Patron A's hold 11. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a transit status 12. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 13. Check in an item from that record to fill Patron A's hold 14. Check in the same item again. A modal will pop up, saying that the hold is already waiting 15. In the modal, choose a cancellation reason and click "Cancel hold" --> A new modal will pop up to fill Patron B's hold 16. Click "Ignore" on the modal for Patron B's hold 17. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 18. Apply patch 19. Repeat steps 1-6 --> Note that Patron B's hold is still a bib-level/"next available" hold 20. Repeat steps 7-11 --> Note that Patron B's hold is still a bib-level/"next available" hold 21. Repeat steps 12-17 --> Note that Patron B's hold is still a bib-level/"next available" hold 22. Prove t/db_dependent/Circulation.t 23. Prove t/db_dependent/Koha/Holds.t --> Tests pass -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #9 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 163473 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163473&action=edit Bug 34972: (follow-up) Update returns.pl to initiate item transfer directly returns.pl was relying on GetOtherReserves to update branchtransfers when putting a hold in transit. It should be calling ModItemTransfer directly. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Andrew Fuerste-Henry <andrewfh@dubcolib.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Andrew Fuerste-Henry <andrewfh@dubcolib.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163471|0 |1 is obsolete| | --- Comment #10 from Andrew Fuerste-Henry <andrewfh@dubcolib.org> --- Created attachment 163728 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163728&action=edit Bug 34972: Add unit tests for GetOtherReserves To test: 1. Apply this patch only 2. Prove -v t/db_dependent/Circulation.t --> Tests fail --> Note verbose output for Tests for GetOtherReserves: ok 3 - GetOtherReserves should not set found status not ok 4 - GetOtherReserves should not set itemnumber on biblio-level hold --> The itemnumber is set on a biblio-level hold, but the found status is not set! 3. Apply the other patch 4. Prove -v t/db_dependent/Circulation.t --> Tests pass Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Andrew Fuerste-Henry <andrewfh@dubcolib.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163472|0 |1 is obsolete| | --- Comment #11 from Andrew Fuerste-Henry <andrewfh@dubcolib.org> --- Created attachment 163729 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163729&action=edit Bug 34972: GetOtherReserves should not alter hold states GetOtherReserves attempts to set the waiting/transit status for the next hold on the list when applicable, but in practice it either leaves the hold state unchanged, or sets the itemnumber without setting the found status (erroneously converting bib-level holds to item-level holds). The latter situation only occurs when the user has been prompted to confirm, cancel, or revert the hold, and is able to ignore the prompt. In those situations, the hold's state should not change. GetOtherReserves does not need to change the hold's state, and attempting to do so produces no effect except for erroneously converting bib-level holds to item-level holds in certain situations, so this patch removes that code. To test: 1. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 2. Check in an item from that record to fill Patron A's hold 3. Set the hold's expiration date to yesterday by accessing the database in the command line: - In a ktd shell prompt, open the db client with koha-mysql kohadev - UPDATE reserves SET expirationdate = DATE_SUB(CURDATE(), INTERVAL 1 DAY) WHERE borrowernumber = <Patron A's borrowernumber> 4. Go to Circulation > Holds Awaiting Pickup, and find the hold in the "holds waiting past their expiration date" tab 5. Click the "Cancel hold" button in the Actions column next to the hold 6. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 7. Cancel Patron B's hold 8. Place 2 new holds on the record: one for Patron A at the logged-in library, and one for Patron B at a different library 9. Check in an item to fill Patron A's hold 10. Repeat steps 3-5 to expire and cancel Patron A's hold 11. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a transit status 12. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 13. Check in an item from that record to fill Patron A's hold 14. Check in the same item again. A modal will pop up, saying that the hold is already waiting 15. In the modal, choose a cancellation reason and click "Cancel hold" --> A new modal will pop up to fill Patron B's hold 16. Click "Ignore" on the modal for Patron B's hold 17. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 18. Apply patch 19. Repeat steps 1-6 --> Note that Patron B's hold is still a bib-level/"next available" hold 20. Repeat steps 7-11 --> Note that Patron B's hold is still a bib-level/"next available" hold 21. Repeat steps 12-17 --> Note that Patron B's hold is still a bib-level/"next available" hold 22. Prove t/db_dependent/Circulation.t 23. Prove t/db_dependent/Koha/Holds.t --> Tests pass Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Andrew Fuerste-Henry <andrewfh@dubcolib.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163473|0 |1 is obsolete| | --- Comment #12 from Andrew Fuerste-Henry <andrewfh@dubcolib.org> --- Created attachment 163730 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163730&action=edit Bug 34972: (follow-up) Update returns.pl to initiate item transfer directly returns.pl was relying on GetOtherReserves to update branchtransfers when putting a hold in transit. It should be calling ModItemTransfer directly. Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | CC| |m.de.rooy@rijksmuseum.nl --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Looking here -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Hi Emily, Brave effort to try improve here :) This code is a mine field. Some small observations: my $patron = Koha::Patrons->find( $nextreservinfo ); Does not look good. We should just pass the borrowernumber to find. Cleaner code. L615 my ( $messages, $nextreservinfo ) = GetOtherReserves($reserve->{itemnumber}); Confusing that we start here with another $messages while the former one came from AddReturn. General I am just wondering how much sense it makes to still have GetOtherReserves. Couldnt we just obsolete it here now? Note that the name is quite misleading. IIUC it checks if the next reserve is waiting or transit. Around the first call I am wondering what happens now if you came from the $cancel_reserve branch of the if statement. After that you go to GetOtherReserves. Which changed behavior and does nothing now. Did it formerly handle the next reserve after the cancelled one? If I am coming from the else branch, I know if the hold is waiting or transit (look at diffBranchSend). So why still call GetOtherReserves to find out what you already know? Around L600 the second call. There is just one route to the call now. You already know again diffBranchSend. Why call GetOtherReserves? Appreciate some feedback. Thanks. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #15 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Hi Marcel, thanks for taking a look!
I am just wondering how much sense it makes to still have GetOtherReserves. Couldnt we just obsolete it here now?
I agree that GetOtherReserves should be removed. I was originally planning on submitting a trivial bugfix here and doing the refactor on a follow-up bug. I ended up needing to do some of the refactoring here anyway, though, so at this point I think you're right that it makes more sense to obsolete it now. I'll work on a revised patchset.
Around the first call I am wondering what happens now if you came from the $cancel_reserve branch of the if statement. After that you go to GetOtherReserves. Which changed behavior and does nothing now. Did it formerly handle the next reserve after the cancelled one?
It still does identify the next reserve after the cancelled one (but it does that by calling CheckReserves, which could be called directly instead). It was *supposed to* fully handle the next reserve, i.e. set it to waiting or put it in transit, but it didn't do that properly (see comment 1). The rest are fair points; I'll address them in the new patchset as well. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #16 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 164392 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164392&action=edit Bug 34972: Add tests for ModReservesCancelAll To test: 1. Apply this patch only 2. prove t/db_dependent/Koha/Holds.t --> Tests pass 3. Apply the other patch 4. prove t/db_dependent/Koha/Holds.test --> Tests still pass -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #17 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 164393 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164393&action=edit Bug 34972: Remove GetOtherReserves GetOtherReserves attempts to set the waiting/transit status for the next hold on the list when applicable, but in practice it either leaves the hold state unchanged, or sets the itemnumber without setting the found status (erroneously converting bib-level holds to item-level holds). The latter situation only occurs when the user has been prompted to confirm, cancel, or revert the hold, and is able to ignore the prompt. In those situations, the hold's state should not change. GetOtherReserves does not need to change the hold state, and it does not do so correctly. Besides that, it does not do much other than call CheckReserves, and is only used in 3 places. This patch removes GetOtherReserves, and refactors returns.pl and C4::Reserves::ModReserveCancelAll to call CheckReserves directly instead. To test: 1. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 2. Check in an item from that record to fill Patron A's hold 3. Set the hold's expiration date to yesterday by accessing the database in the command line: - In a ktd shell prompt, open the db client with koha-mysql kohadev - UPDATE reserves SET expirationdate = DATE_SUB(CURDATE(), INTERVAL 1 DAY) WHERE borrowernumber = <Patron A's borrowernumber> 4. Go to Circulation > Holds Awaiting Pickup, and find the hold in the "holds waiting past their expiration date" tab 5. Click the "Cancel hold" button in the Actions column next to the hold 6. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status in the command line: - In a ktd shell prompt, open the db client with koha-mysql kohadev - UPDATE reserves SET expirationdate = DATE_SUB(CURDATE(), INTERVAL 1 DAY) WHERE borrowernumber = <Patron A's borrowernumber> 4. Go to Circulation > Holds Awaiting Pickup, and find the hold in the "holds waiting past their expiration date" tab 5. Click the "Cancel hold" button in the Actions column next to the hold 6. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 7. Cancel Patron B's hold 8. Place 2 new holds on the record: one for Patron A at the logged-in library, and one for Patron B at a different library 9. Check in an item to fill Patron A's hold 10. Repeat steps 3-5 to expire and cancel Patron A's hold 11. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a transit status 12. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 13. Check in an item from that record to fill Patron A's hold 14. Check in the same item again. A modal will pop up, saying that the hold is already waiting 15. In the modal, choose a cancellation reason and click "Cancel hold" --> A new modal will pop up to fill Patron B's hold 16. Click "Ignore" on the modal for Patron B's hold 17. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 18. Apply patch 19. Repeat steps 1-6 --> Note that Patron B's hold is still a bib-level/"next available" hold 20. Repeat steps 7-11 --> Note that Patron B's hold is still a bib-level/"next available" hold 21. Repeat steps 12-17 --> Note that Patron B's hold is still a bib-level/"next available" hold Make sure correct behavior is unchanged: 22. Cancel Patron B's hold 23. Place 2 new holds on the record: one for Patron A at the logged-in library, and one for Patron B at a different library 24. Check in an item from that record to fill Patron A's hold 25. Check in the same item again. A modal will pop up, saying that the hold is already waiting 26. In the modal, choose a cancellation reason and click "Cancel hold" --> A new modal will pop up to fill Patron B's hold 27. Click "Print slip, transfer, and confirm" on the modal for Patron B's hold --> Confirm that the information on the slip is correct --> Confirm that the hold is correctly put in transit 22. Set HoldsAutoFill and HoldsAutoFillPrintSlip to "Do" 23. Place a bib-level hold for the logged-in library 24. Check in an item from that bib --> Confirm the information on the slip is correct --> Confirm the hold is correctly assigned and set to waiting 25. Place a bib-level hold for a different library 26. Check in an item from that bib --> Confirm the information on the slip is correct --> Confirm the hold is correctly put in transit 27. Change the logged-in branch to match the hold pickup location 28. Check the item in --> Confirm the information on the slip is correct --> Confirm the hold is correctly assigned and set to waiting 29. Repeat steps 22-26 --> Confirm a correct hold slip pops up for Patron B's hold --> Confirm that Patron B's hold is correctly put in transit 30. Cancel Patron B's hold 31. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 33. Repeat steps 24-26 --> Confirm a correct hold slip pops up for Patron B's hold --> Confirm Patron B's hold is correctly set to Waiting 34. Prove t/db_dependent/Circulation.t 35. Prove t/db_dependent/Koha/Holds.t --> Tests pass -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163728|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163729|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163730|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Needs Signoff --- Comment #18 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Setting back to Needs Signoff, because these patches are different enough from the previous ones that they should be retested -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #19 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- *** Bug 29348 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #20 from David Nind <david@davidnind.com> --- I had a go at re-testing, but I found things a little confusing and then had some errors (step 19). The tests passed before and after the patches. Testing notes (so far), using KTD: - Step 4: When I cancel the hold, there is a message saying "Please retain this item and check it in to process the hold." for Patron B. - Steps 6, 4, and 5; then there is step 6 again - I'm assuming these can be ignored. For example the SQL in the first step 6 says "Patron A's borrowernumber", however there is only one row in the table and that is the hold for Patron B. - Step 11: . For the Holds section of the bibliographic record it shows as an item level hold, as per the test plan . On the normal view: "In transit from Centerville to Midway since 04/30/2024 There is an item level hold on this item (priority = 1)." . If I change the library to Midway, Circulation > Transfers > Transfers to receive shows the transfer . I cancelled the transfer and deleted the hold - Step 15: . No new modal popped up after selecting a cancellation reason the cancel hold . Normal view status: There is an item level hold on this item (priority = 1). . Holds section details: Only item XXXXX . Patron details: not showing as waiting for pickup . Circulation > Holds awaiting pickup: no holds showing - Step 19: repeat steps 1-6 - for step 2, I get this error: Can't locate object method "damaged" via package "32" (perhaps you forgot to load "32"?) at /kohadevbox/koha/C4/Reserves.pm line 858 in C4::Reserves::CheckReserves at /kohadevbox/koha/C4/Reserves.pm line 858 855: 856: return unless $item; # bail if we got nothing. 857: 858: return if ( $item->damaged && !C4::Context->preference('AllowHoldsOnDamagedItems') ); 859: 860: # if item is not for loan it cannot be reserved either..... 861: # except where items.notforloan < 0 : This indicates the item is holdable. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #21 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Thank you for testing and the detailed notes, David!
- Step 4: When I cancel the hold, there is a message saying "Please retain this item and check it in to process the hold." for Patron B.
Yes, that's the current behavior. Sometimes staff miss that step of checking the item in, though, which can cause the bug this patch is meant to fix.
- Steps 6, 4, and 5; then there is step 6 again - I'm assuming these can be ignored. For example the SQL in the first step 6 says "Patron A's borrowernumber", however there is only one row in the table and that is the hold for Patron B.
Oops, thanks for catching that! I'll fix the commit message.
- Step 11: . For the Holds section of the bibliographic record it shows as an item level hold, as per the test plan . On the normal view: "In transit from Centerville to Midway since 04/30/2024 There is an item level hold on this item (priority = 1)." . If I change the library to Midway, Circulation > Transfers > Transfers to receive shows the transfer . I cancelled the transfer and deleted the hold
It looks like the Holds Awaiting Pickup page is initiating the item transfer, but not marking the hold as In Transit. Which is less than ideal, but not in scope for this bug. I'll be more specific in that part of the test plan, though.
- Step 15: . No new modal popped up after selecting a cancellation reason the cancel hold . Normal view status: There is an item level hold on this item (priority = 1). . Holds section details: Only item XXXXX . Patron details: not showing as waiting for pickup . Circulation > Holds awaiting pickup: no holds showing
- Step 19: repeat steps 1-6 - for step 2, I get this error:
Can't locate object method "damaged" via package "32" (perhaps you forgot to load "32"?) at /kohadevbox/koha/C4/Reserves.pm line 858 in C4::Reserves::CheckReserves at /kohadevbox/koha/C4/Reserves.pm line 858 855: 856: return unless $item; # bail if we got nothing. 857: 858: return if ( $item->damaged && !C4::Context->preference('AllowHoldsOnDamagedItems') ); 859: 860: # if item is not for loan it cannot be reserved either..... 861: # except where items.notforloan < 0 : This indicates the item is holdable.
I do recreate these last two. Step 15 is still on main, so it must be an unrelated regression - I'll file a separate bug report for that. It has the same result as clicking "Ignore" on the second modal, so it won't block testing here (once I fix the second issue, which is definitely caused by the patch). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164393|0 |1 is obsolete| | --- Comment #22 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 166024 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166024&action=edit Bug 34972: Remove GetOtherReserves GetOtherReserves attempts to set the waiting/transit status for the next hold on the list when applicable, but in practice it either leaves the hold state unchanged, or sets the itemnumber without setting the found status (erroneously converting bib-level holds to item-level holds). The latter situation only occurs when the user has been prompted to confirm, cancel, or revert the hold, and is able to ignore the prompt. In those situations, the hold's state should not change. GetOtherReserves does not need to change the hold state, and it does not do so correctly. Besides that, it does not do much other than call CheckReserves, and is only used in 3 places. This patch removes GetOtherReserves, and refactors returns.pl and C4::Reserves::ModReserveCancelAll to call CheckReserves directly instead. To test: 1. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 2. Check in an item from that record to fill Patron A's hold 3. Set the hold's expiration date to yesterday by accessing the database in the command line: - In a ktd shell prompt, open the db client with koha-mysql kohadev - UPDATE reserves SET expirationdate = DATE_SUB(CURDATE(), INTERVAL 1 DAY) WHERE borrowernumber = <Patron A's borrowernumber> 4. Go to Circulation > Holds Awaiting Pickup, and find the hold in the "holds waiting past their expiration date" tab 5. Click the "Cancel hold" button in the Actions column next to the hold (do not check in the book) 6. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 7. Cancel Patron B's hold 8. Place 2 new holds on the record: one for Patron A at the logged-in library, and one for Patron B at a different library 9. Check in an item to fill Patron A's hold 10. Repeat steps 3-5 to expire and cancel Patron A's hold 11. Return to the Holds tab of the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold, and there is no "Revert transit status" button 12. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 13. Check in an item from that record to fill Patron A's hold 14. Check in the same item again. A modal will pop up, saying that the hold is already waiting 15. In the modal, choose a cancellation reason and click "Cancel hold" --> A new modal will pop up to fill Patron B's hold 16. Click "Ignore" on the modal for Patron B's hold 17. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 18. Apply patch 19. Repeat steps 1-6 --> Note that Patron B's hold is still a bib-level/"next available" hold 20. Repeat steps 7-11 --> Note that Patron B's hold is still a bib-level/"next available" hold 21. Repeat steps 12-17 --> Note that Patron B's hold is still a bib-level/"next available" hold Make sure correct behavior is unchanged: 22. Cancel Patron B's hold 23. Place 2 new holds on the record: one for Patron A at the logged-in library, and one for Patron B at a different library 24. Check in an item from that record to fill Patron A's hold 25. Check in the same item again. A modal will pop up, saying that the hold is already waiting 26. In the modal, choose a cancellation reason and click "Cancel hold" --> A new modal will pop up to fill Patron B's hold 27. Click "Print slip, transfer, and confirm" on the modal for Patron B's hold --> Confirm that the information on the slip is correct --> Confirm that the hold is correctly put in transit 22. Set HoldsAutoFill and HoldsAutoFillPrintSlip to "Do" 23. Place a bib-level hold for the logged-in library 24. Check in an item from that bib --> Confirm the information on the slip is correct --> Confirm the hold is correctly assigned and set to waiting 25. Place a bib-level hold for a different library 26. Check in an item from that bib --> Confirm the information on the slip is correct --> Confirm the hold is correctly put in transit 27. Change the logged-in branch to match the hold pickup location 28. Check the item in --> Confirm the information on the slip is correct --> Confirm the hold is correctly assigned and set to waiting 29. Repeat steps 22-26 --> Confirm a correct hold slip pops up for Patron B's hold --> Confirm that Patron B's hold is correctly put in transit 30. Cancel Patron B's hold 31. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 33. Repeat steps 24-26 --> Confirm a correct hold slip pops up for Patron B's hold --> Confirm Patron B's hold is correctly set to Waiting 34. Prove t/db_dependent/Circulation.t 35. Prove t/db_dependent/Koha/Holds.t --> Tests pass -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #23 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Everything mentioned above should be fixed and ready for testing again. Testing note for steps 15-16: there seems to be a regression in main currently - when checking in a waiting hold and cancelling it from the check-in modal, a second modal does not pop up to fill the next hold. Until that is fixed, skip step 16 - the result will be the same. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164392|0 |1 is obsolete| | --- Comment #24 from David Nind <david@davidnind.com> --- Created attachment 166028 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166028&action=edit Bug 34972: Add tests for ModReservesCancelAll To test: 1. Apply this patch only 2. prove t/db_dependent/Koha/Holds.t --> Tests pass 3. Apply the other patch 4. prove t/db_dependent/Koha/Holds.test --> Tests still pass Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #166024|0 |1 is obsolete| | --- Comment #25 from David Nind <david@davidnind.com> --- Created attachment 166029 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166029&action=edit Bug 34972: Remove GetOtherReserves GetOtherReserves attempts to set the waiting/transit status for the next hold on the list when applicable, but in practice it either leaves the hold state unchanged, or sets the itemnumber without setting the found status (erroneously converting bib-level holds to item-level holds). The latter situation only occurs when the user has been prompted to confirm, cancel, or revert the hold, and is able to ignore the prompt. In those situations, the hold's state should not change. GetOtherReserves does not need to change the hold state, and it does not do so correctly. Besides that, it does not do much other than call CheckReserves, and is only used in 3 places. This patch removes GetOtherReserves, and refactors returns.pl and C4::Reserves::ModReserveCancelAll to call CheckReserves directly instead. To test: 1. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 2. Check in an item from that record to fill Patron A's hold 3. Set the hold's expiration date to yesterday by accessing the database in the command line: - In a ktd shell prompt, open the db client with koha-mysql kohadev - UPDATE reserves SET expirationdate = DATE_SUB(CURDATE(), INTERVAL 1 DAY) WHERE borrowernumber = <Patron A's borrowernumber> 4. Go to Circulation > Holds Awaiting Pickup, and find the hold in the "holds waiting past their expiration date" tab 5. Click the "Cancel hold" button in the Actions column next to the hold (do not check in the book) 6. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 7. Cancel Patron B's hold 8. Place 2 new holds on the record: one for Patron A at the logged-in library, and one for Patron B at a different library 9. Check in an item to fill Patron A's hold 10. Repeat steps 3-5 to expire and cancel Patron A's hold 11. Return to the Holds tab of the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold, and there is no "Revert transit status" button 12. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 13. Check in an item from that record to fill Patron A's hold 14. Check in the same item again. A modal will pop up, saying that the hold is already waiting 15. In the modal, choose a cancellation reason and click "Cancel hold" --> A new modal will pop up to fill Patron B's hold 16. Click "Ignore" on the modal for Patron B's hold 17. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 18. Apply patch 19. Repeat steps 1-6 --> Note that Patron B's hold is still a bib-level/"next available" hold 20. Repeat steps 7-11 --> Note that Patron B's hold is still a bib-level/"next available" hold 21. Repeat steps 12-17 --> Note that Patron B's hold is still a bib-level/"next available" hold Make sure correct behavior is unchanged: 22. Cancel Patron B's hold 23. Place 2 new holds on the record: one for Patron A at the logged-in library, and one for Patron B at a different library 24. Check in an item from that record to fill Patron A's hold 25. Check in the same item again. A modal will pop up, saying that the hold is already waiting 26. In the modal, choose a cancellation reason and click "Cancel hold" --> A new modal will pop up to fill Patron B's hold 27. Click "Print slip, transfer, and confirm" on the modal for Patron B's hold --> Confirm that the information on the slip is correct --> Confirm that the hold is correctly put in transit 22. Set HoldsAutoFill and HoldsAutoFillPrintSlip to "Do" 23. Place a bib-level hold for the logged-in library 24. Check in an item from that bib --> Confirm the information on the slip is correct --> Confirm the hold is correctly assigned and set to waiting 25. Place a bib-level hold for a different library 26. Check in an item from that bib --> Confirm the information on the slip is correct --> Confirm the hold is correctly put in transit 27. Change the logged-in branch to match the hold pickup location 28. Check the item in --> Confirm the information on the slip is correct --> Confirm the hold is correctly assigned and set to waiting 29. Repeat steps 22-26 --> Confirm a correct hold slip pops up for Patron B's hold --> Confirm that Patron B's hold is correctly put in transit 30. Cancel Patron B's hold 31. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 33. Repeat steps 24-26 --> Confirm a correct hold slip pops up for Patron B's hold --> Confirm Patron B's hold is correctly set to Waiting 34. Prove t/db_dependent/Circulation.t 35. Prove t/db_dependent/Koha/Holds.t --> Tests pass Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #26 from David Nind <david@davidnind.com> --- (In reply to Emily Lamancusa from comment #21) Thanks Emily! I have now signed off. (I hope I tested it correctly!) David -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED --- Comment #27 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Looking here -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #28 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Emily Lamancusa from comment #22)
To test: 1. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 2. Check in an item from that record to fill Patron A's hold 3. Set the hold's expiration date to yesterday by accessing the database in the command line: - In a ktd shell prompt, open the db client with koha-mysql kohadev - UPDATE reserves SET expirationdate = DATE_SUB(CURDATE(), INTERVAL 1 DAY) WHERE borrowernumber = <Patron A's borrowernumber> 4. Go to Circulation > Holds Awaiting Pickup, and find the hold in the "holds waiting past their expiration date" tab 5. Click the "Cancel hold" button in the Actions column next to the hold (do not check in the book) 6. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status
7. Cancel Patron B's hold 8. Place 2 new holds on the record: one for Patron A at the logged-in library, and one for Patron B at a different library 9. Check in an item to fill Patron A's hold 10. Repeat steps 3-5 to expire and cancel Patron A's hold 11. Return to the Holds tab of the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold, and there is no "Revert transit status" button
12. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 13. Check in an item from that record to fill Patron A's hold 14. Check in the same item again. A modal will pop up, saying that the hold is already waiting 15. In the modal, choose a cancellation reason and click "Cancel hold" --> A new modal will pop up to fill Patron B's hold 16. Click "Ignore" on the modal for Patron B's hold 17. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status
18. Apply patch 19. Repeat steps 1-6 --> Note that Patron B's hold is still a bib-level/"next available" hold 20. Repeat steps 7-11 --> Note that Patron B's hold is still a bib-level/"next available" hold 21. Repeat steps 12-17 --> Note that Patron B's hold is still a bib-level/"next available" hold
Make sure correct behavior is unchanged:
22. Cancel Patron B's hold 23. Place 2 new holds on the record: one for Patron A at the logged-in library, and one for Patron B at a different library 24. Check in an item from that record to fill Patron A's hold 25. Check in the same item again. A modal will pop up, saying that the hold is already waiting 26. In the modal, choose a cancellation reason and click "Cancel hold" --> A new modal will pop up to fill Patron B's hold 27. Click "Print slip, transfer, and confirm" on the modal for Patron B's hold --> Confirm that the information on the slip is correct --> Confirm that the hold is correctly put in transit
22. Set HoldsAutoFill and HoldsAutoFillPrintSlip to "Do"
23. Place a bib-level hold for the logged-in library 24. Check in an item from that bib --> Confirm the information on the slip is correct --> Confirm the hold is correctly assigned and set to waiting 25. Place a bib-level hold for a different library 26. Check in an item from that bib --> Confirm the information on the slip is correct --> Confirm the hold is correctly put in transit 27. Change the logged-in branch to match the hold pickup location 28. Check the item in --> Confirm the information on the slip is correct --> Confirm the hold is correctly assigned and set to waiting
29. Repeat steps 22-26 --> Confirm a correct hold slip pops up for Patron B's hold --> Confirm that Patron B's hold is correctly put in transit 30. Cancel Patron B's hold 31. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 33. Repeat steps 24-26 --> Confirm a correct hold slip pops up for Patron B's hold --> Confirm Patron B's hold is correctly set to Waiting
34. Prove t/db_dependent/Circulation.t 35. Prove t/db_dependent/Koha/Holds.t --> Tests pass
I nominate this one for test plan of the year :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #166028|0 |1 is obsolete| | --- Comment #29 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 166100 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166100&action=edit Bug 34972: Add tests for ModReservesCancelAll To test: 1. Apply this patch only 2. prove t/db_dependent/Koha/Holds.t --> Tests pass 3. Apply the other patch 4. prove t/db_dependent/Koha/Holds.test --> Tests still pass Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #166029|0 |1 is obsolete| | --- Comment #30 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 166101 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166101&action=edit Bug 34972: Remove GetOtherReserves GetOtherReserves attempts to set the waiting/transit status for the next hold on the list when applicable, but in practice it either leaves the hold state unchanged, or sets the itemnumber without setting the found status (erroneously converting bib-level holds to item-level holds). The latter situation only occurs when the user has been prompted to confirm, cancel, or revert the hold, and is able to ignore the prompt. In those situations, the hold's state should not change. GetOtherReserves does not need to change the hold state, and it does not do so correctly. Besides that, it does not do much other than call CheckReserves, and is only used in 3 places. This patch removes GetOtherReserves, and refactors returns.pl and C4::Reserves::ModReserveCancelAll to call CheckReserves directly instead. To test: 1. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 2. Check in an item from that record to fill Patron A's hold 3. Set the hold's expiration date to yesterday by accessing the database in the command line: - In a ktd shell prompt, open the db client with koha-mysql kohadev - UPDATE reserves SET expirationdate = DATE_SUB(CURDATE(), INTERVAL 1 DAY) WHERE borrowernumber = <Patron A's borrowernumber> 4. Go to Circulation > Holds Awaiting Pickup, and find the hold in the "holds waiting past their expiration date" tab 5. Click the "Cancel hold" button in the Actions column next to the hold (do not check in the book) 6. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 7. Cancel Patron B's hold 8. Place 2 new holds on the record: one for Patron A at the logged-in library, and one for Patron B at a different library 9. Check in an item to fill Patron A's hold 10. Repeat steps 3-5 to expire and cancel Patron A's hold 11. Return to the Holds tab of the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold, and there is no "Revert transit status" button 12. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 13. Check in an item from that record to fill Patron A's hold 14. Check in the same item again. A modal will pop up, saying that the hold is already waiting 15. In the modal, choose a cancellation reason and click "Cancel hold" --> A new modal will pop up to fill Patron B's hold 16. Click "Ignore" on the modal for Patron B's hold 17. Return to the bib record and look at Patron B's hold --> Note that Patron B's hold is now an item-level hold and does not have a waiting status 18. Apply patch 19. Repeat steps 1-6 --> Note that Patron B's hold is still a bib-level/"next available" hold 20. Repeat steps 7-11 --> Note that Patron B's hold is still a bib-level/"next available" hold 21. Repeat steps 12-17 --> Note that Patron B's hold is still a bib-level/"next available" hold Make sure correct behavior is unchanged: 22. Cancel Patron B's hold 23. Place 2 new holds on the record: one for Patron A at the logged-in library, and one for Patron B at a different library 24. Check in an item from that record to fill Patron A's hold 25. Check in the same item again. A modal will pop up, saying that the hold is already waiting 26. In the modal, choose a cancellation reason and click "Cancel hold" --> A new modal will pop up to fill Patron B's hold 27. Click "Print slip, transfer, and confirm" on the modal for Patron B's hold --> Confirm that the information on the slip is correct --> Confirm that the hold is correctly put in transit 22. Set HoldsAutoFill and HoldsAutoFillPrintSlip to "Do" 23. Place a bib-level hold for the logged-in library 24. Check in an item from that bib --> Confirm the information on the slip is correct --> Confirm the hold is correctly assigned and set to waiting 25. Place a bib-level hold for a different library 26. Check in an item from that bib --> Confirm the information on the slip is correct --> Confirm the hold is correctly put in transit 27. Change the logged-in branch to match the hold pickup location 28. Check the item in --> Confirm the information on the slip is correct --> Confirm the hold is correctly assigned and set to waiting 29. Repeat steps 22-26 --> Confirm a correct hold slip pops up for Patron B's hold --> Confirm that Patron B's hold is correctly put in transit 30. Cancel Patron B's hold 31. Place 2 bib-level holds for 2 different patrons (Patron A and Patron B) on the same record, both for pickup at the logged-in library 33. Repeat steps 24-26 --> Confirm a correct hold slip pops up for Patron B's hold --> Confirm Patron B's hold is correctly set to Waiting 34. Prove t/db_dependent/Circulation.t 35. Prove t/db_dependent/Koha/Holds.t --> Tests pass Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #31 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 166102 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166102&action=edit Bug 34972: (QA follow-up) Remove some ModReserveCancelAll imports Not used? Dont import. Which actually only leaves circ/waitingreserves.pl as the only 'real' caller. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #32 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 166103 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166103&action=edit Bug 34972: (QA follow-up) Remove second transfer from Circulation.t Removing the manual transfer and rightaway doing the Reserve transfer. One test description was misleading too. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #33 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- QA Comment: First well done! This code is a minefield. Before passing QA, please look at my question under [1]. [1] Last question? - C4::Items::ModItemTransfer( - $itemnumber, - $item->holdingbranch, - $checkreserves->{'branchcode'}, + ModItemTransfer( $itemnumber, $userenv_branch, $diffBranchSend, 'Reserve' ); Are we sure that this new transfer should be from $userenv_branch in all cases ? (Noting that the check already referred to userenv_branch before this change.) The second call looks like: + ModItemTransfer( $itemnumber, $item->holdingbranch, $reserve->{branchcode}, 'Reserve' ); And that just convinces me more (at least, at first glance)? But the other way around: why not userenv_branch here, since that is the branch where we are checking in now? They should be equal here since we called AddReturn already. So second call is fine. [2] my ( $messages, $nextreservinfo ) = ModReserveCancelAll( $item, $borrowernumber ); Not for now, but $nextreservinfo is just a borrowernumber ! Comparing with: my ( undef, $nextreservinfo, undef ) = CheckReserves($item); Here it is a hash including a borrowernumber. [3] ModReserveCancelAll Outside scope too but the name is (also) quite confusing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Signed Off --- Comment #34 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- (In reply to Marcel de Rooy from comment #33)
QA Comment:
[1] Last question? - C4::Items::ModItemTransfer( - $itemnumber, - $item->holdingbranch, - $checkreserves->{'branchcode'}, + ModItemTransfer( $itemnumber, $userenv_branch, $diffBranchSend, 'Reserve' ); Are we sure that this new transfer should be from $userenv_branch in all cases ? (Noting that the check already referred to userenv_branch before this change.)
The second call looks like: + ModItemTransfer( $itemnumber, $item->holdingbranch, $reserve->{branchcode}, 'Reserve' ); And that just convinces me more (at least, at first glance)? But the other way around: why not userenv_branch here, since that is the branch where we are checking in now? They should be equal here since we called AddReturn already. So second call is fine.
As you say, in the second case they should be equal since it's explicitly called after AddReturn. In practice, the first case is always called after AddReturn as well, since we only reach that step after the user has responded to a modal. But that is neither obvious from the code nor strictly enforced, so I used $userenv_branch to be safe (and to be consistent with the immediately preceeding code). Presumably, the item is in the hand of the staff member performing the return, so transferring from the staff member's current location makes sense. I can't think of any situations where we shouldn't make that assumption in returns.pl. Totally agree about ModReserveCancelAll. It and ModReserveMinusPriority should both be in line for removal, really... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #35 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Emily Lamancusa from comment #34)
As you say, in the second case they should be equal since it's explicitly called after AddReturn. In practice, the first case is always called after AddReturn as well, since we only reach that step after the user has responded to a modal. But that is neither obvious from the code nor strictly enforced, so I used $userenv_branch to be safe (and to be consistent with the immediately preceeding code). Presumably, the item is in the hand of the staff member performing the return, so transferring from the staff member's current location makes sense. I can't think of any situations where we shouldn't make that assumption in returns.pl.
Passing QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |24.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 --- Comment #36 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|24.05.00 |24.05.00,23.11.06 released in| | CC| |fridolin.somers@biblibre.co | |m Status|Pushed to main |Pushed to stable --- Comment #37 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.11.x for 23.11.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|24.05.00,23.11.06 |24.05.00,23.11.06,23.05.12 released in| | Status|Pushed to stable |Pushed to oldstable CC| |lucas@bywatersolutions.com --- Comment #38 from Lucas Gass <lucas@bywatersolutions.com> --- Backported to 23.05.x for upcoming 23.05.12 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=37650 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |35721 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35721 [Bug 35721] Replace ModItemTransfer calls in circ/returns.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34972 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED CC| |wainuiwitikapark@catalyst.n | |et.nz Status|Pushed to oldstable |RESOLVED --- Comment #39 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Not backporting to 22.11 unless requested -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org