Bug 30687 - Unable to override hold policy if no pickup locations are available
Summary: Unable to override hold policy if no pickup locations are available
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Aleisha Amohia
QA Contact: Kyle M Hall
URL:
Keywords: regression
Depends on: 31169
Blocks:
  Show dependency treegraph
 
Reported: 2022-05-05 04:19 UTC by Aleisha Amohia
Modified: 2023-01-01 22:44 UTC (History)
9 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 30687: Override hold policy when no valid pickup location (2.40 KB, patch)
2022-05-05 04:35 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30687: Override hold policy when no valid pickup location (2.47 KB, patch)
2022-07-12 15:39 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 30687: Override hold policy when no valid pickup location (2.52 KB, patch)
2022-09-15 16:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30687: Override hold policy when no valid pickup location (2.58 KB, patch)
2022-09-16 15:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30687: (QA follow-up) Allow pickup location choice when overridden (2.06 KB, patch)
2022-09-16 15:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30687 (alternative patch): Always allow to force hold pickup location (4.48 KB, patch)
2022-11-09 07:06 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 30687 (alternative patch): Always allow to force hold pickup location (4.52 KB, patch)
2022-12-05 09:48 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 30687: [alternate-alternate] Allow pickup locatoin to be forced when override is allowed (7.71 KB, patch)
2022-12-23 15:08 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30687: [alternate-alternate] Allow pickup locatoin to be forced when override is allowed (7.76 KB, patch)
2023-01-01 22:14 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2022-05-05 04:19:21 UTC
Set the following settings:

Reserves policy by item type for Branch A:
Item type: BK
Reserve policy: From home library 
Reserve pickup library match: any library
Return policy: Item returns home

AllowHoldPolicyOverride: Allow

Create a biblio with an item:
Item home library: Branch A
Item type: BK

Log in to the staff client as a Patron that belongs to Branch B.

Go to place a hold on the Biblio. Notice that you are blocked because there is no valid pickup location, and unable to override it.

Previously we could override this restriction, but we now can't.
Comment 1 Aleisha Amohia 2022-05-05 04:35:52 UTC
Created attachment 134625 [details] [review]
Bug 30687: Override hold policy when no valid pickup location

This patch allows hold policy to be overridden when there is no valid
pickup location and AllowHoldPolicyOverride is enabled. This is
re-implementing old behaviour that has been lost.

To test:

1. Go to Administration -> Circulation and fines rules. Ensure there is
a default circulation rule and default reserves policy.
2. Change the library using the dropdown and scroll to the bottom of the
page to set a reserves policy by item type for Library A, which the
following settings:
- Item type: BK
- Reserve policy: From home library
- Reserve pickup library match: any library
- Return policy: Item returns home
3. Go to Administration -> system preferences. Set the
AllowHoldPolicyOverride setting to Allow.
4. Create a biblio with an item where the item's home library is Library
A and the itemtype is BK.
5. Log in to the staff client as a Patron that belongs to Library B.
6. Search for the biblio you just created and try to request a hold for
yourself. Notice there is a blocking notice "Cannot place reserve", the
"Place reserve" button is disabled, and the item has a "No valid pickup
location" error.
7. Apply the patch and refresh the page or re-request the hold.
8. Confirm you can now place the hold, acknowledging that you must
override hold policy to do so (note the exclamation point icon).
9. Set the AllowHoldPolicyOverride system preference to "Don't allow".
10. Repeat Step 6. Confirm you are blocked again from requesting the
hold and cannot override it.

Sponsored-by: Chartered Accountants Australia and New Zealand
Comment 2 Biblibre Sandboxes 2022-07-12 15:39:46 UTC
Created attachment 137643 [details] [review]
Bug 30687: Override hold policy when no valid pickup location

This patch allows hold policy to be overridden when there is no valid
pickup location and AllowHoldPolicyOverride is enabled. This is
re-implementing old behaviour that has been lost.

To test:

1. Go to Administration -> Circulation and fines rules. Ensure there is
a default circulation rule and default reserves policy.
2. Change the library using the dropdown and scroll to the bottom of the
page to set a reserves policy by item type for Library A, which the
following settings:
- Item type: BK
- Reserve policy: From home library
- Reserve pickup library match: any library
- Return policy: Item returns home
3. Go to Administration -> system preferences. Set the
AllowHoldPolicyOverride setting to Allow.
4. Create a biblio with an item where the item's home library is Library
A and the itemtype is BK.
5. Log in to the staff client as a Patron that belongs to Library B.
6. Search for the biblio you just created and try to request a hold for
yourself. Notice there is a blocking notice "Cannot place reserve", the
"Place reserve" button is disabled, and the item has a "No valid pickup
location" error.
7. Apply the patch and refresh the page or re-request the hold.
8. Confirm you can now place the hold, acknowledging that you must
override hold policy to do so (note the exclamation point icon).
9. Set the AllowHoldPolicyOverride system preference to "Don't allow".
10. Repeat Step 6. Confirm you are blocked again from requesting the
hold and cannot override it.

Sponsored-by: Chartered Accountants Australia and New Zealand

Signed-off-by: Daniel <daniel.jones@cheshiresharedservices.gov.uk>
Comment 3 Daniel Jones 2022-07-12 15:41:46 UTC
Signing off as test plan works as described.

One thing I noticed was that I couldn't complete the hold by checking in the item. Is this expected behaviour?
Comment 4 Aleisha Amohia 2022-07-13 04:23:09 UTC
(In reply to Daniel Jones from comment #3)
> Signing off as test plan works as described.
> 
> One thing I noticed was that I couldn't complete the hold by checking in the
> item. Is this expected behaviour?

Do you mean you couldn't set the hold as waiting? Or fill the hold?

What do you mean by complete the hold?
Comment 5 Daniel Jones 2022-07-13 08:22:31 UTC
(In reply to Aleisha Amohia from comment #4)
> (In reply to Daniel Jones from comment #3)
> > Signing off as test plan works as described.
> > 
> > One thing I noticed was that I couldn't complete the hold by checking in the
> > item. Is this expected behaviour?
> 
> Do you mean you couldn't set the hold as waiting? Or fill the hold?
> 
> What do you mean by complete the hold?

The hold cannot be filled. Once it is checked in, the book just asks to be returned to its home library, and not sent to the patron's library.
Comment 6 Katrin Fischer 2022-07-15 10:58:15 UTC
Hi Aleisha, could you please look into Daniel's comment? If the hold cannot be filled at the pickup-location this might not work as intended.
Comment 7 Kyle M Hall 2022-09-15 16:54:14 UTC
(In reply to Katrin Fischer from comment #6)
> Hi Aleisha, could you please look into Daniel's comment? If the hold cannot
> be filled at the pickup-location this might not work as intended.

The library community has spoken loud and clearly in the past and present, and the consensus is that librarians should be able to force holds that are unfillable. In the past I attempted to limit forced holds to those that would be filled, and what we found out is that libraries utilize those unfillable holds for a variety of reasons. We ended up reverting those changes in community. We also have partners with this very same issue Aleisha is describing. I think it's safe to say that the questions of "can this hold be placed" and "can this hold be filled" are very much separate questions. In that spirit, I think the question of "should this item be trapped instead of transferred back" should be dealt with on a separate bug report.
Comment 8 Kyle M Hall 2022-09-15 16:56:16 UTC
Created attachment 140683 [details] [review]
Bug 30687: Override hold policy when no valid pickup location

This patch allows hold policy to be overridden when there is no valid
pickup location and AllowHoldPolicyOverride is enabled. This is
re-implementing old behaviour that has been lost.

To test:

1. Go to Administration -> Circulation and fines rules. Ensure there is
a default circulation rule and default reserves policy.
2. Change the library using the dropdown and scroll to the bottom of the
page to set a reserves policy by item type for Library A, which the
following settings:
- Item type: BK
- Reserve policy: From home library
- Reserve pickup library match: any library
- Return policy: Item returns home
3. Go to Administration -> system preferences. Set the
AllowHoldPolicyOverride setting to Allow.
4. Create a biblio with an item where the item's home library is Library
A and the itemtype is BK.
5. Log in to the staff client as a Patron that belongs to Library B.
6. Search for the biblio you just created and try to request a hold for
yourself. Notice there is a blocking notice "Cannot place reserve", the
"Place reserve" button is disabled, and the item has a "No valid pickup
location" error.
7. Apply the patch and refresh the page or re-request the hold.
8. Confirm you can now place the hold, acknowledging that you must
override hold policy to do so (note the exclamation point icon).
9. Set the AllowHoldPolicyOverride system preference to "Don't allow".
10. Repeat Step 6. Confirm you are blocked again from requesting the
hold and cannot override it.

Sponsored-by: Chartered Accountants Australia and New Zealand

Signed-off-by: Daniel <daniel.jones@cheshiresharedservices.gov.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Katrin Fischer 2022-09-16 07:27:46 UTC
(In reply to Kyle M Hall from comment #7)
> (In reply to Katrin Fischer from comment #6)
> > Hi Aleisha, could you please look into Daniel's comment? If the hold cannot
> > be filled at the pickup-location this might not work as intended.
> 
> The library community has spoken loud and clearly in the past and present,
> and the consensus is that librarians should be able to force holds that are
> unfillable. In the past I attempted to limit forced holds to those that
> would be filled, and what we found out is that libraries utilize those
> unfillable holds for a variety of reasons. We ended up reverting those
> changes in community. We also have partners with this very same issue
> Aleisha is describing. I think it's safe to say that the questions of "can
> this hold be placed" and "can this hold be filled" are very much separate
> questions. In that spirit, I think the question of "should this item be
> trapped instead of transferred back" should be dealt with on a separate bug
> report.

I am actually confused about this one and feel like I have been missing out on the discussion.

How do libraries utilize holds that can never be filled? I'd be interested in examples to understand better. At the moment there is no way to utilize those unless you change the item values or you cancel the hold as it can otherwise not move to filled.

I don't remember from testing, but does this unlock all pickup locations or will a hold with no pickup location be placed? (see bug 31086).
Comment 10 Kyle M Hall 2022-09-16 12:51:47 UTC
I think nick could answer that question better than me. I just know they need to ;)

Additionally, I think Aleisha's report does vary a bit from my own reports. What we are finding in 21.11 if that if some items should be holdable with an override, but at least one item is never holdable because of a lack of pickup location, it becomes impossible to place an overridden hold.
Comment 11 Nick Clemens 2022-09-16 14:14:44 UTC
(In reply to Kyle M Hall from comment #10)
> I think nick could answer that question better than me. I just know they
> need to ;)

I do believe a hold that does not trigger will still resolve when item is checked out to the patron - so libraries can utilize these with reports
Additionally - due to floating/transfers/limits etc. there are times when a hold is unfillable currently, but can be filled in the future when the item is at another location.
Comment 12 Nick Clemens 2022-09-16 15:11:38 UTC
Created attachment 140712 [details] [review]
Bug 30687: Override hold policy when no valid pickup location

This patch allows hold policy to be overridden when there is no valid
pickup location and AllowHoldPolicyOverride is enabled. This is
re-implementing old behaviour that has been lost.

To test:

1. Go to Administration -> Circulation and fines rules. Ensure there is
a default circulation rule and default reserves policy.
2. Change the library using the dropdown and scroll to the bottom of the
page to set a reserves policy by item type for Library A, which the
following settings:
- Item type: BK
- Reserve policy: From home library
- Reserve pickup library match: any library
- Return policy: Item returns home
3. Go to Administration -> system preferences. Set the
AllowHoldPolicyOverride setting to Allow.
4. Create a biblio with an item where the item's home library is Library
A and the itemtype is BK.
5. Log in to the staff client as a Patron that belongs to Library B.
6. Search for the biblio you just created and try to request a hold for
yourself. Notice there is a blocking notice "Cannot place reserve", the
"Place reserve" button is disabled, and the item has a "No valid pickup
location" error.
7. Apply the patch and refresh the page or re-request the hold.
8. Confirm you can now place the hold, acknowledging that you must
override hold policy to do so (note the exclamation point icon).
9. Set the AllowHoldPolicyOverride system preference to "Don't allow".
10. Repeat Step 6. Confirm you are blocked again from requesting the
hold and cannot override it.

Sponsored-by: Chartered Accountants Australia and New Zealand

Signed-off-by: Daniel <daniel.jones@cheshiresharedservices.gov.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens 2022-09-16 15:11:45 UTC
Created attachment 140713 [details] [review]
Bug 30687: (QA follow-up) Allow pickup location choice when overridden

The original test plan did not recreate behaviour for me, but I simply added:
Default checkout, hold and return policy
Hold policy: from home library
Hold pickup library match: Item hold group (with no groups defined)

Then I couldn't place hold before, but could place hold after patch, however,
I couldn't choose a pickup location for the item level hold and the hold failed

To test:
1 - Set rules as above
2 - Confirm you can choose a library for item specific hold
3 - Confirm all locations are marked with exclamation point
4 - Confirm hold can successfully be placed

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens 2022-09-16 15:13:08 UTC
(In reply to Katrin Fischer from comment #9)
> I don't remember from testing, but does this unlock all pickup locations or
> will a hold with no pickup location be placed? (see bug 31086).

It will "unlock" all pickup locations, however, they are displayed with a note/exclamation point to indicate that they are violating policy

This was done previously as it is believed override should allow you to override holdability and pickup rules
Comment 15 Julian Maurice 2022-09-22 12:57:15 UTC
I tried to solve the same issue but fixed it differently (I didn't know about this bug report until now)
I tested this bug's patches and it doesn't work for me. I used the following test plan:
1. Apply patch
2. Disable AllowHoldPolicyOverride
3. Create a biblio B with an item I at library A.
4. Configure this library A to not be a pickup location
5. Add a "Default holds policy by item type" for item I type where "Hold
   pickup library match" is "item's home library"
6. Try to place a hold on biblio B
   You should not be able to place a hold because there is no valid
   pickup locations
7. Enable AllowHoldPolicyOverride
8. Try to place a hold on biblio B
   You should now see all valid pickup locations in a dropdown list
   (with an exclamation mark in front of each option) with none selected
   by default
9. Verify you can place a title-level hold and an item-level hold

At step 8, I still see the error message "No items are available to be placed on hold." and the message " No valid pickup location " in the items table. "Place hold" buttons are disabled.

Here's my patch: https://git.biblibre.com/biblibre/kohac/commit/a1fe134264a855a13b2586545475b5256deef10d

Should I submit my patch in another bug report (as this one is already Passed QA) ?
Comment 16 Tomás Cohen Arazi 2022-10-03 17:33:23 UTC
(In reply to Julian Maurice from comment #15)
> I tried to solve the same issue but fixed it differently (I didn't know
> about this bug report until now)
> I tested this bug's patches and it doesn't work for me.

Please FQA in such cases.

> Should I submit my patch in another bug report (as this one is already
> Passed QA) ?

I'd say you should attach your patch here for other devs review in context (test plan, etc).

Thanks!
Comment 17 Julian Maurice 2022-11-09 07:06:44 UTC
Created attachment 143457 [details] [review]
Bug 30687 (alternative patch): Always allow to force hold pickup location

If AllowHoldPolicyOverride is enabled and only some pickup locations are
available, you still have the possibility to force one of the others
pickup locations.
But when there are zero pickup locations available, that is not
possible.

This patch change that by always displaying the list of pickup locations
when AllowHoldPolicyOverride is enabled.

Test plan:
1. Apply patch
2. Disable AllowHoldPolicyOverride
3. Create a biblio B with an item I at library A.
4. Configure this library A to not be a pickup location
5. Add a "Default holds policy by item type" for item I type where "Hold
   pickup library match" is "item's home library"
6. Try to place a hold on biblio B
   You should not be able to place a hold because there is no valid
   pickup locations
7. Enable AllowHoldPolicyOverride
8. Try to place a hold on biblio B
   You should now see all valid pickup locations in a dropdown list
   (with an exclamation mark in front of each option) with none selected
   by default
9. Verify you can place a title-level hold and an item-level hold
Comment 18 Catrina Berka 2022-11-14 22:15:40 UTC
Patch didn't apply in sandbox
Comment 19 Julian Maurice 2022-12-05 09:48:36 UTC
Created attachment 144427 [details] [review]
Bug 30687 (alternative patch): Always allow to force hold pickup location

If AllowHoldPolicyOverride is enabled and only some pickup locations are
available, you still have the possibility to force one of the others
pickup locations.
But when there are zero pickup locations available, that is not
possible.

This patch change that by always displaying the list of pickup locations
when AllowHoldPolicyOverride is enabled.

Test plan:
1. Apply patch
2. Disable AllowHoldPolicyOverride
3. Create a biblio B with an item I at library A.
4. Configure this library A to not be a pickup location
5. Add a "Default holds policy by item type" for item I type where "Hold
   pickup library match" is "item's home library"
6. Try to place a hold on biblio B
   You should not be able to place a hold because there is no valid
   pickup locations
7. Enable AllowHoldPolicyOverride
8. Try to place a hold on biblio B
   You should now see all valid pickup locations in a dropdown list
   (with an exclamation mark in front of each option) with none selected
   by default
9. Verify you can place a title-level hold and an item-level hold
Comment 20 Julian Maurice 2022-12-05 09:49:18 UTC
Rebased the alternative patch
Comment 21 Julian Maurice 2022-12-05 09:50:53 UTC
(In reply to Catrina Berka from comment #18)
> Patch didn't apply in sandbox

I'm not sure if a sandbox can be used to test this bug, because it has two sets of patches that try to fix the bug in different ways. It doesn't make sense to apply all patches in this case.
Comment 22 Fridolin Somers 2022-12-22 07:12:39 UTC
I rise to major because a lot of librarians asked about this regression
Comment 23 Nick Clemens 2022-12-23 15:08:57 UTC
Created attachment 144825 [details] [review]
Bug 30687: [alternate-alternate] Allow pickup locatoin to be forced when override is allowed

This is Julian's patch with some extra cleanup to reduce repeated code

If AllowHoldPolicyOverride is enabled and only some pickup locations are
available, you still have the possibility to force one of the others
pickup locations.
But when there are zero pickup locations available, that is not
possible.

This patch change that by always displaying the list of pickup locations
when AllowHoldPolicyOverride is enabled.

Test plan:
1. Apply patch
2. Disable AllowHoldPolicyOverride
3. Create a biblio B with an item I at library A.
4. Configure this library A to not be a pickup location
5. Add a "Default holds policy by item type" for item I type where "Hold
   pickup library match" is "item's home library"
6. Try to place a hold on biblio B
   You should not be able to place a hold because there is no valid
   pickup locations
7. Enable AllowHoldPolicyOverride
8. Try to place a hold on biblio B
   You should now see all valid pickup locations in a dropdown list
   (with an exclamation mark in front of each option) with none selected
   by default
9. Verify you can place a title-level hold and an item-level hold


Current status: Needs Signoff
Comment 24 David Nind 2023-01-01 22:14:44 UTC
Created attachment 144896 [details] [review]
Bug 30687: [alternate-alternate] Allow pickup locatoin to be forced when override is allowed

This is Julian's patch with some extra cleanup to reduce repeated code

If AllowHoldPolicyOverride is enabled and only some pickup locations are
available, you still have the possibility to force one of the others
pickup locations.
But when there are zero pickup locations available, that is not
possible.

This patch change that by always displaying the list of pickup locations
when AllowHoldPolicyOverride is enabled.

Test plan:
1. Apply patch
2. Disable AllowHoldPolicyOverride
3. Create a biblio B with an item I at library A.
4. Configure this library A to not be a pickup location
5. Add a "Default holds policy by item type" for item I type where "Hold
   pickup library match" is "item's home library"
6. Try to place a hold on biblio B
   You should not be able to place a hold because there is no valid
   pickup locations
7. Enable AllowHoldPolicyOverride
8. Try to place a hold on biblio B
   You should now see all valid pickup locations in a dropdown list
   (with an exclamation mark in front of each option) with none selected
   by default
9. Verify you can place a title-level hold and an item-level hold

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2023-01-01 22:44:42 UTC
I've tested and signed off the alternate-alternate patch.

I've drafted a release note - I don't feel confident enough that I've got this right. Could someone check this, edit where appropriate, and add? Thanks! I'm akso not sure if this was introduced in 21.11.

Draft release note:

This fixes a regression introduced in Koha 21.11(???). Previously, if hold overrides were enabled (the AllowHoldPolicyOverride system preference) and there was no pickup location available (for example, if a library was not a pickup location), then it was still possible to select an alternative pickup location. This restores this ability - by default no pickup location is selected, but any location can now be selected from the dropdown list (each option now has an exclamation mark at the end with the message "This pickup location is not allowed according to circulation rules").

Note: the test plan says the exclamation mark is at the start of the option, but for me it displayed at the end.

Also, does this need a note about how some libraries want to be able to place unfillable holds for a variety of reasons? (see comment #7 and following discussions).