Bug 26990

Summary: No feedback if holds override is disabled and hold fails
Product: Koha Reporter: Nick Clemens <nick>
Component: Hold requestsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: aleisha, david, gmcharlt, jonathan.druart, kyle, lucas
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06
Bug Depends on: 22284    
Bug Blocks:    
Attachments: Bug 26990: (bug 22284 follow-up) Prevent hold to be placed if cannot be transferred
Bug 26990: (bug 22284 follow-up) Prevent hold to be placed if cannot be transferred
Bug 26990: (bug 22284 follow-up) Prevent hold to be placed if cannot be transferred

Description Nick Clemens 2020-11-10 14:21:50 UTC
placerequest.pl checks CanItemBeReserved and correctly fails a hold, however, an item that is not holdable because it cannot be transferred is selectable while placing holds and will fail silently as placerequest.pl doesn't set any params for feedback if a hold fails, it expects the hold screen to block any unwanted holds

To recreate:
1 - Set preferences:
     UseBranchTransferLimits: enforce
     BranchTransferLimitsType: itype
     AllowHoldPolicyOverride: Don't allow
2 - Set Centerville 'Book' items to not be allowed to transfer to any other library
3 - Make sure Hold policy is set to 'any library'
4 - Find a record with a Centerville item and other items
5 - Attempt to place an item level hold on the Centerville item
6 - You are taken to the holds list, but your hold is not placed
7 - Nothing indicates why hold has failed
Comment 1 Jonathan Druart 2020-11-11 13:37:41 UTC
Ensure that the pickup library is different than CPL
Comment 2 Jonathan Druart 2020-11-11 13:52:55 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2020-11-11 13:54:37 UTC
Caused by:
  commit 546a3b6d4d6333fe938572f0247dfcbe4a4d35a8
  Bug 22284: New message, new column and filter pickup locations in reserve/request.tt
Comment 4 Jonathan Druart 2020-11-11 13:54:53 UTC
-                my $can_item_be_reserved = CanItemBeReserved( $patron->borrowernumber, $itemnumber, $pickup )->{status};
-                $item->{not_holdable} = $can_item_be_reserved unless $can_item_be_reserved eq 'OK';
+                my $can_item_be_reserved = CanItemBeReserved( $patron->borrowernumber, $itemnumber )->{status};

The $pickup parameter disappears.
Comment 5 Jonathan Druart 2020-11-11 14:00:24 UTC
Created attachment 113497 [details] [review]
Bug 26990: (bug 22284 follow-up) Prevent hold to be placed if cannot be transferred

There is a missing parameter to CanItemBeReserved, we need to know if
the hold can be placed at a given pickup library.

Test plan:
1 - Set preferences:
     UseBranchTransferLimits: enforce
     BranchTransferLimitsType: itype
     AllowHoldPolicyOverride: Don't allow
2 - Set Centerville 'Book' items to not be allowed to transfer to any other library
3 - Make sure Hold policy is set to 'any library'
4 - Find a record with a Centerville item and other items
5 - Attempt to place an item level hold on the Centerville item (with a
pickup library different than Centerville)
=> Without this patch you are taken to the holds list, but your hold is not placed
Nothing indicates why hold has failed
=> With this patch you cannot select the item from Centerville
"Cannot be transferred to pickup library"
Comment 6 David Nind 2020-11-11 18:36:23 UTC
Created attachment 113532 [details] [review]
Bug 26990: (bug 22284 follow-up) Prevent hold to be placed if cannot be transferred

There is a missing parameter to CanItemBeReserved, we need to know if
the hold can be placed at a given pickup library.

Test plan:
1 - Set preferences:
     UseBranchTransferLimits: enforce
     BranchTransferLimitsType: itype
     AllowHoldPolicyOverride: Don't allow
2 - Set Centerville 'Book' items to not be allowed to transfer to any other library
3 - Make sure Hold policy is set to 'any library'
4 - Find a record with a Centerville item and other items
5 - Attempt to place an item level hold on the Centerville item (with a
pickup library different than Centerville)
=> Without this patch you are taken to the holds list, but your hold is not placed
Nothing indicates why hold has failed
=> With this patch you cannot select the item from Centerville
"Cannot be transferred to pickup library"

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2020-11-11 18:43:28 UTC
My notes from testing 

Step 2:
1. Go to Administration > Patrons and circulation > Library transfer limits.
2. Make sure Centerville is selected as the library.
3. For BK click on clear all then save.
4. Note that CPL still stays selected.

Step 4, before step 4 change the library in the staff interface to a library other than Centerville.
Comment 8 Kyle M Hall 2020-11-12 13:09:43 UTC
Created attachment 113554 [details] [review]
Bug 26990: (bug 22284 follow-up) Prevent hold to be placed if cannot be transferred

There is a missing parameter to CanItemBeReserved, we need to know if
the hold can be placed at a given pickup library.

Test plan:
1 - Set preferences:
     UseBranchTransferLimits: enforce
     BranchTransferLimitsType: itype
     AllowHoldPolicyOverride: Don't allow
2 - Set Centerville 'Book' items to not be allowed to transfer to any other library
3 - Make sure Hold policy is set to 'any library'
4 - Find a record with a Centerville item and other items
5 - Attempt to place an item level hold on the Centerville item (with a
pickup library different than Centerville)
=> Without this patch you are taken to the holds list, but your hold is not placed
Nothing indicates why hold has failed
=> With this patch you cannot select the item from Centerville
"Cannot be transferred to pickup library"

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Jonathan Druart 2020-11-12 16:29:02 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 10 Lucas Gass 2020-11-16 22:46:56 UTC
backported to 20.05.x for 20.05.06
Comment 11 Aleisha Amohia 2020-11-17 05:43:31 UTC
missing dependencies, not backported to 19.11.x