Bug 33470 - Don't calculate overridden values when placing a hold via the REST API
Summary: Don't calculate overridden values when placing a hold via the REST API
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 33471
  Show dependency treegraph
 
Reported: 2023-04-09 18:24 UTC by Nick Clemens (kidclamp)
Modified: 2023-12-28 20:47 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.06,22.05.14


Attachments
Bug 33470: Don't calculate overridden hold limits (4.15 KB, patch)
2023-04-09 18:34 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33470: Don't calculate overridden hold limits (4.22 KB, patch)
2023-04-10 19:49 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 33470: Don't calculate overridden hold limits (4.29 KB, patch)
2023-05-03 12:13 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2023-04-09 18:24:54 UTC
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
Comment 1 Nick Clemens (kidclamp) 2023-04-09 18:34:19 UTC
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
Comment 2 Emily Lamancusa 2023-04-10 19:49:10 UTC
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>
Comment 3 Emily Lamancusa 2023-04-10 19:56:26 UTC
Testing notes:
Step 0 - Set AllowHoldPolicyOverride to "Allow"
Comment 4 Jonathan Druart 2023-05-03 07:20:15 UTC
Please provide tests.
Comment 5 Jonathan Druart 2023-05-03 07:21:16 UTC
Sorry, I misread there is no behaviour changes expected here.
Comment 6 Jonathan Druart 2023-05-03 07:23:06 UTC
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?
Comment 7 Nick Clemens (kidclamp) 2023-05-03 11:08:56 UTC
(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?
Comment 8 Jonathan Druart 2023-05-03 12:12:35 UTC
Hum indeed, sorry!
Comment 9 Jonathan Druart 2023-05-03 12:13:35 UTC
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>
Comment 10 Tomás Cohen Arazi 2023-05-05 13:15:36 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 11 Martin Renvoize 2023-05-12 14:05:48 UTC
Many hands makes light work, thankyou everyone!

Pushed to 22.11.x for the next release
Comment 12 Lucas Gass 2023-06-13 15:47:38 UTC
Backported to 22.05.x for upcoming 22.05.14
Comment 13 Danyon Sewell 2023-07-20 00:08:01 UTC
Does this need to be backported to 21.11.x?