Description
Emily Lamancusa (emlam)
2023-10-02 17:47:06 UTC
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. 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. Created attachment 160775 [details] [review] 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 Created attachment 160776 [details] [review] 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 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" 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. Created attachment 163471 [details] [review] 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 Created attachment 163472 [details] [review] 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 Created attachment 163473 [details] [review] 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. Created attachment 163728 [details] [review] 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> Created attachment 163729 [details] [review] 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> Created attachment 163730 [details] [review] 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> Looking here 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. 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. Created attachment 164392 [details] [review] 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 Created attachment 164393 [details] [review] 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 Setting back to Needs Signoff, because these patches are different enough from the previous ones that they should be retested *** Bug 29348 has been marked as a duplicate of this bug. *** 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. 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). Created attachment 166024 [details] [review] 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 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. Created attachment 166028 [details] [review] 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> Created attachment 166029 [details] [review] 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> (In reply to Emily Lamancusa from comment #21) Thanks Emily! I have now signed off. (I hope I tested it correctly!) David Looking here (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 :) Created attachment 166100 [details] [review] 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> Created attachment 166101 [details] [review] 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> Created attachment 166102 [details] [review] 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> Created attachment 166103 [details] [review] 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> 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. (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... (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 Pushed for 24.05! Well done everyone, thank you! Pushed to 23.11.x for 23.11.06 Backported to 23.05.x for upcoming 23.05.12 |