Bug 27863 - Cannot select different pickup locations even with AllowHoldsPolicyOverride on request.pl
Summary: Cannot select different pickup locations even with AllowHoldsPolicyOverride o...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 26988
Blocks: 27864 27865 27894
  Show dependency treegraph
 
Reported: 2021-03-04 15:21 UTC by Nick Clemens
Modified: 2021-06-14 21:35 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00


Attachments
Bug 27863: Unit tests (1.91 KB, patch)
2021-03-04 17:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27863: Add a way to override pickup locations constraints (2.49 KB, patch)
2021-03-04 17:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27863: Use the /libraries route when AllowHoldsPolicyOverride is set (3.05 KB, patch)
2021-03-05 19:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27863: Use the /libraries route when AllowHoldsPolicyOverride is set (3.11 KB, patch)
2021-03-08 17:22 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 27863: Use the /libraries route when AllowHoldsPolicyOverride is set (3.18 KB, patch)
2021-03-09 08:22 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-03-04 15:21:34 UTC
Currently the dropdown for 'pickup library' on request.pl for existing holds returns only libraries allowed by circulation rules

Previously libraries were able to select any library. In the case of a hold that violates rules staff would like the ability to override the allowed list if 'AllowHoldPolicyOverride' is enabled

The list of libraries should provide some visual feedback to indicate which libraries are allowed, and which ones can be forced
Comment 1 Tomás Cohen Arazi 2021-03-04 16:59:49 UTC
It needs to be implemented in a way we can request to override limits (you may still want to be able to retrieve *only* the valid pickup locations, for other purposes (e.g. for the visual feedback you mention).
Comment 2 Tomás Cohen Arazi 2021-03-04 17:56:12 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2021-03-04 17:56:17 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2021-03-04 17:59:52 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2021-03-05 18:04:23 UTC
I'm going other route to fix this.
Comment 6 Tomás Cohen Arazi 2021-03-05 19:31:05 UTC
Created attachment 117874 [details] [review]
Bug 27863: Use the /libraries route when AllowHoldsPolicyOverride is set

This patch makes the dropdown for changing the pickup location for
existing holds use the GET /libraries route instead of GET
/holds/:hold_id/pikcup_locations if the configuration allows the
override.

Terminology:
- Pickup location, means a library that is marked as valid pickup
  location on its configuration.

To test:
1. Have the circ rules set so 'Hold pickup library match' is set to
   'Item's home library' or 'Any'. The goal is to be able to
   differentiate when all libraries are returned, from the case only
   'valid pickup locations' are returned. The easiest one is 'Item's
   home library'.
2. Set AllowHoldsPolicyOverride
3. Pick a patron from a library that is not a valid pickup location and
   an item that belongs to a library that doesn't match any of the valid
   ones.
=> SUCCESS: Notice Koha allows you to choose any library (because of the
        syspref)
4. Place the hold.
5. Now check on the holds list for the bib, if you can use the dropdown
   to change the pickup location for the hold
=> SUCCESS: You can, sort of
=> FAIL: It doesn't show/search all pickup locations. It only shows the
        valid ones, even though we should be able to override it.
6. Apply this patch
7. Restart all just in case, and reload the holds page for the bib
8. Repeat 5
=> SUCCESS: You are presented all pickup locations!
9. Sign off :-D
Comment 7 Tomás Cohen Arazi 2021-03-05 19:35:43 UTC
Looking at how the templates use the Branches plugin to render pickup location lists, I believe this is the way to go.

I'll move the visual feedback part to a separate enhancement bug once I sort the best way to achieve it.
Comment 8 Andrew Fuerste-Henry 2021-03-08 16:58:54 UTC
When placing a new hold with pickup library match set to Item's Holding Library and AllowHoldPolicyOverride set to Allow, I am not given the option of setting a pickup branch other that the item's holdingbranch, with or without the patch. I should be able to pick whatever branch I want when placing the hold, rather than having to place it and then edit the pickup branch to something else.

The changes to pickup branch selection on existing holds works great! I agree that it'd be good to have some indicator within that dropdown of which branches follow the rules and which require an override, though it makes sense to follow up on that in a separate bug.
Comment 9 Tomás Cohen Arazi 2021-03-08 17:06:58 UTC
(In reply to Andrew Fuerste-Henry from comment #8)
> When placing a new hold with pickup library match set to Item's Holding
> Library and AllowHoldPolicyOverride set to Allow, I am not given the option
> of setting a pickup branch other that the item's holdingbranch, with or
> without the patch. I should be able to pick whatever branch I want when
> placing the hold, rather than having to place it and then edit the pickup
> branch to something else.
> 
> The changes to pickup branch selection on existing holds works great! I
> agree that it'd be good to have some indicator within that dropdown of which
> branches follow the rules and which require an override, though it makes
> sense to follow up on that in a separate bug.

Are you sure that's not *another* holds bug?
Comment 10 Andrew Fuerste-Henry 2021-03-08 17:22:53 UTC
Created attachment 117937 [details] [review]
Bug 27863: Use the /libraries route when AllowHoldsPolicyOverride is set

This patch makes the dropdown for changing the pickup location for
existing holds use the GET /libraries route instead of GET
/holds/:hold_id/pikcup_locations if the configuration allows the
override.

Terminology:
- Pickup location, means a library that is marked as valid pickup
  location on its configuration.

To test:
1. Have the circ rules set so 'Hold pickup library match' is set to
   'Item's home library' or 'Any'. The goal is to be able to
   differentiate when all libraries are returned, from the case only
   'valid pickup locations' are returned. The easiest one is 'Item's
   home library'.
2. Set AllowHoldsPolicyOverride
3. Pick a patron from a library that is not a valid pickup location and
   an item that belongs to a library that doesn't match any of the valid
   ones.
=> SUCCESS: Notice Koha allows you to choose any library (because of the
        syspref)
4. Place the hold.
5. Now check on the holds list for the bib, if you can use the dropdown
   to change the pickup location for the hold
=> SUCCESS: You can, sort of
=> FAIL: It doesn't show/search all pickup locations. It only shows the
        valid ones, even though we should be able to override it.
6. Apply this patch
7. Restart all just in case, and reload the holds page for the bib
8. Repeat 5
=> SUCCESS: You are presented all pickup locations!
9. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 11 Andrew Fuerste-Henry 2021-03-08 17:23:34 UTC
Tomas pointed out the dropdown on a new hold is to be handled in bug 27894. Therefore signing off here.
Comment 12 Martin Renvoize 2021-03-09 08:22:12 UTC
Created attachment 117958 [details] [review]
Bug 27863: Use the /libraries route when AllowHoldsPolicyOverride is set

This patch makes the dropdown for changing the pickup location for
existing holds use the GET /libraries route instead of GET
/holds/:hold_id/pikcup_locations if the configuration allows the
override.

Terminology:
- Pickup location, means a library that is marked as valid pickup
  location on its configuration.

To test:
1. Have the circ rules set so 'Hold pickup library match' is set to
   'Item's home library' or 'Any'. The goal is to be able to
   differentiate when all libraries are returned, from the case only
   'valid pickup locations' are returned. The easiest one is 'Item's
   home library'.
2. Set AllowHoldsPolicyOverride
3. Pick a patron from a library that is not a valid pickup location and
   an item that belongs to a library that doesn't match any of the valid
   ones.
=> SUCCESS: Notice Koha allows you to choose any library (because of the
        syspref)
4. Place the hold.
5. Now check on the holds list for the bib, if you can use the dropdown
   to change the pickup location for the hold
=> SUCCESS: You can, sort of
=> FAIL: It doesn't show/search all pickup locations. It only shows the
        valid ones, even though we should be able to override it.
6. Apply this patch
7. Restart all just in case, and reload the holds page for the bib
8. Repeat 5
=> SUCCESS: You are presented all pickup locations!
9. Sign off :-D

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2021-03-09 08:22:53 UTC
Tomas and I discussed this one at length and agreed upon the approach taken.

All works as expected now and passes QA script.

Passing QA
Comment 14 Jonathan Druart 2021-03-09 10:02:54 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 15 Fridolin Somers 2021-03-12 08:42:04 UTC
Does not apply because of Bug 27016
Please create a new patch for 20.11.x if needed