Bug 34972 - Canceling a waiting hold from the holds over tab can make the next hold unfillable
Summary: Canceling a waiting hold from the holds over tab can make the next hold unfil...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Emily Lamancusa
QA Contact: Marcel de Rooy
URL:
Keywords:
: 29348 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-10-02 17:47 UTC by Emily Lamancusa
Modified: 2024-05-23 14:14 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.06,23.05.12


Attachments
Bug 34972: Add unit tests for GetOtherReserves (3.27 KB, patch)
2024-01-10 16:34 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34972: GetOtherReserves should not alter hold states (4.64 KB, patch)
2024-01-10 16:34 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34972: Add unit tests for GetOtherReserves (3.33 KB, patch)
2024-03-19 18:35 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34972: GetOtherReserves should not alter hold states (4.73 KB, patch)
2024-03-19 18:35 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34972: (follow-up) Update returns.pl to initiate item transfer directly (2.82 KB, patch)
2024-03-19 18:35 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34972: Add unit tests for GetOtherReserves (3.40 KB, patch)
2024-03-22 17:41 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 34972: GetOtherReserves should not alter hold states (4.79 KB, patch)
2024-03-22 17:41 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 34972: (follow-up) Update returns.pl to initiate item transfer directly (2.88 KB, patch)
2024-03-22 17:41 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 34972: Add tests for ModReservesCancelAll (5.30 KB, patch)
2024-04-03 20:39 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34972: Remove GetOtherReserves (13.24 KB, patch)
2024-04-03 20:39 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34972: Remove GetOtherReserves (12.71 KB, patch)
2024-05-01 20:54 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34972: Add tests for ModReservesCancelAll (5.35 KB, patch)
2024-05-02 02:09 UTC, David Nind
Details | Diff | Splinter Review
Bug 34972: Remove GetOtherReserves (12.76 KB, patch)
2024-05-02 02:09 UTC, David Nind
Details | Diff | Splinter Review
Bug 34972: Add tests for ModReservesCancelAll (5.45 KB, patch)
2024-05-03 09:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34972: Remove GetOtherReserves (12.86 KB, patch)
2024-05-03 09:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34972: (QA follow-up) Remove some ModReserveCancelAll imports (1.39 KB, patch)
2024-05-03 09:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34972: (QA follow-up) Remove second transfer from Circulation.t (1.74 KB, patch)
2024-05-03 09:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emily Lamancusa 2023-10-02 17:47:06 UTC
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.
Comment 1 Emily Lamancusa 2023-11-24 18:47:43 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.
Comment 2 Emily Lamancusa 2023-11-24 18:57:15 UTC
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.
Comment 3 Emily Lamancusa 2024-01-10 16:34:02 UTC
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
Comment 4 Emily Lamancusa 2024-01-10 16:34:04 UTC
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
Comment 5 David Nind 2024-01-14 21:05:34 UTC
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"
Comment 6 Emily Lamancusa 2024-01-19 21:35:53 UTC
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.
Comment 7 Emily Lamancusa 2024-03-19 18:35:27 UTC
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
Comment 8 Emily Lamancusa 2024-03-19 18:35:29 UTC
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
Comment 9 Emily Lamancusa 2024-03-19 18:35:31 UTC
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.
Comment 10 Andrew Fuerste-Henry 2024-03-22 17:41:07 UTC
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>
Comment 11 Andrew Fuerste-Henry 2024-03-22 17:41:09 UTC
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>
Comment 12 Andrew Fuerste-Henry 2024-03-22 17:41:11 UTC
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>
Comment 13 Marcel de Rooy 2024-03-29 10:11:10 UTC
Looking here
Comment 14 Marcel de Rooy 2024-03-29 10:56:47 UTC
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.
Comment 15 Emily Lamancusa 2024-04-03 13:51:12 UTC
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.
Comment 16 Emily Lamancusa 2024-04-03 20:39:44 UTC
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
Comment 17 Emily Lamancusa 2024-04-03 20:39:46 UTC
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
Comment 18 Emily Lamancusa 2024-04-03 20:41:43 UTC
Setting back to Needs Signoff, because these patches are different enough from the previous ones that they should be retested
Comment 19 Emily Lamancusa 2024-04-22 20:55:20 UTC
*** Bug 29348 has been marked as a duplicate of this bug. ***
Comment 20 David Nind 2024-05-01 01:14:01 UTC
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.
Comment 21 Emily Lamancusa 2024-05-01 20:37:15 UTC
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).
Comment 22 Emily Lamancusa 2024-05-01 20:54:29 UTC
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
Comment 23 Emily Lamancusa 2024-05-01 21:01:26 UTC
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.
Comment 24 David Nind 2024-05-02 02:09:19 UTC
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>
Comment 25 David Nind 2024-05-02 02:09:22 UTC
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>
Comment 26 David Nind 2024-05-02 02:12:05 UTC
(In reply to Emily Lamancusa from comment #21)

Thanks Emily!

I have now signed off. (I hope I tested it correctly!)

David
Comment 27 Marcel de Rooy 2024-05-03 07:00:04 UTC
Looking here
Comment 28 Marcel de Rooy 2024-05-03 07:30:16 UTC
(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 :)
Comment 29 Marcel de Rooy 2024-05-03 09:30:48 UTC
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>
Comment 30 Marcel de Rooy 2024-05-03 09:30:51 UTC
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>
Comment 31 Marcel de Rooy 2024-05-03 09:30:53 UTC
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>
Comment 32 Marcel de Rooy 2024-05-03 09:30:56 UTC
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>
Comment 33 Marcel de Rooy 2024-05-03 09:31:26 UTC
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.
Comment 34 Emily Lamancusa 2024-05-06 17:34:24 UTC
(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...
Comment 35 Marcel de Rooy 2024-05-06 18:48:33 UTC
(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
Comment 36 Katrin Fischer 2024-05-07 13:56:02 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 37 Fridolin Somers 2024-05-22 12:44:56 UTC
Pushed to 23.11.x for 23.11.06
Comment 38 Lucas Gass 2024-05-23 14:14:14 UTC
Backported to 23.05.x for upcoming 23.05.12