When placing a hold via the REST api, we can pass an override to force the hold even if pickup locations are not valid, or hold is not allowed. When this is passed, the calculations that determine if the hold would be allowed are moot, and should not be performed
Created attachment 149378 [details] [review] Bug 33470: Don't calculate overridden hold limits This patch can largely be checked in the code to ensure it makes sense, it moves a conditional up one level for the two different checks that could prevent holds To test: 1 - Attempt to place a hold via the REST API with an invalid pickup location Set pickup location as no in branches to make this easy 2 - Confirm you get an error 3 - Attempt again with header (bug 27760): x-koha-override: any 4 - Hold is placed 5 - Repeat 1-4 above but with an item that cannot be held 6 - Apply patch 7 - Repeat 1-4 above 8 - Results should be the same
Created attachment 149410 [details] [review] Bug 33470: Don't calculate overridden hold limits This patch can largely be checked in the code to ensure it makes sense, it moves a conditional up one level for the two different checks that could prevent holds To test: 1 - Attempt to place a hold via the REST API with an invalid pickup location Set pickup location as no in branches to make this easy 2 - Confirm you get an error 3 - Attempt again with header (bug 27760): x-koha-override: any 4 - Hold is placed 5 - Repeat 1-4 above but with an item that cannot be held 6 - Apply patch 7 - Repeat 1-4 above 8 - Results should be the same Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Testing notes: Step 0 - Set AllowHoldPolicyOverride to "Allow"
Please provide tests.
Sorry, I misread there is no behaviour changes expected here.
What's the point of the changes then? Be faster? There are behaviour changes, 400 instead of 403 in case of invalid pickup locations. Maybe that's why it has been written that way in the first place?
(In reply to Jonathan Druart from comment #6) > What's the point of the changes then? Be faster? Yes, performance. Biblio->pickup_locations can take a long time when there are many items/branches > There are behaviour changes, 400 instead of 403 in case of invalid pickup > locations. Maybe that's why it has been written that way in the first place? (In reply to Jonathan Druart from comment #6) > What's the point of the changes then? Be faster? > > There are behaviour changes, 400 instead of 403 in case of invalid pickup > locations. Maybe that's why it has been written that way in the first place? I get a 400 for an invalid pickup location before and after, can you describe the case you are seeing changed?
Hum indeed, sorry!
Created attachment 150581 [details] [review] Bug 33470: Don't calculate overridden hold limits This patch can largely be checked in the code to ensure it makes sense, it moves a conditional up one level for the two different checks that could prevent holds To test: 1 - Attempt to place a hold via the REST API with an invalid pickup location Set pickup location as no in branches to make this easy 2 - Confirm you get an error 3 - Attempt again with header (bug 27760): x-koha-override: any 4 - Hold is placed 5 - Repeat 1-4 above but with an item that cannot be held 6 - Apply patch 7 - Repeat 1-4 above 8 - Results should be the same Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 23.05. Nice work everyone, thanks!
Many hands makes light work, thankyou everyone! Pushed to 22.11.x for the next release
Backported to 22.05.x for upcoming 22.05.14
Does this need to be backported to 21.11.x?