Description
Nick Clemens
2021-03-04 15:21:34 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). Created attachment 117757 [details] [review] Bug 27863: Unit tests Created attachment 117758 [details] [review] Bug 27863: Add a way to override pickup locations constraints If AllowHoldPolicyOverride is set, staff members are able to choose any branch marked as pickup location for a hold. The route for retrieving the valid pickup locations should provide a way to request the override. This patch does that, based on work from bug 27760. A new parameter is added: an x-koha-override header, with the value of 'any' that will force the override if the syspref is enabled. To test: 1. Apply this patches 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => SUCCESS: Tests pass! 3. Sign off :-D I think this is the right approach for this series of bugs, related to rendering pickup locations. One doubt remains: I picked the 'any' value for x-koha-override. But the implementation does a Koha::Libraries->search({ pickup_location => 1 }) when the override is passed and conditions met... and I was wondering, could we want to override that condition as well? If we consider that could be necessary, then we should use something like 'object_pickup_locations' (sorry, didn't find something better) and then pick some other keyword for pickup_location => 1, and reserve 'any' for overriding them all. Hope my doubt makes sense, and I get some valuable feedback. Thanks! I'm going other route to fix this. 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 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. 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. (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? 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> Tomas pointed out the dropdown on a new hold is to be handled in bug 27894. Therefore signing off here. 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> 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 Pushed to master for 21.05, thanks to everybody involved! Does not apply because of Bug 27016 Please create a new patch for 20.11.x if needed |