Bug 33471 - Improve performance of hold pickup location verification for next available holds
Summary: Improve performance of hold pickup location verification for next available h...
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 33447 33470
Blocks:
  Show dependency treegraph
 
Reported: 2023-04-09 20:05 UTC by Nick Clemens
Modified: 2023-07-20 15:57 UTC (History)
4 users (show)

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


Attachments
Bug 33471: Add validate_pickup_location routine to Koha::Biblio (5.59 KB, patch)
2023-04-09 20:09 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33471: Add validate_pickup_location routine to Koha::Biblio (5.66 KB, patch)
2023-04-10 20:40 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 33471: Add validate_pickup_location routine to Koha::Biblio (5.82 KB, patch)
2023-07-14 11:38 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2023-04-09 20:05:54 UTC
When placing a new hold, or updating the location of an existing hold, we verify that the pickup location is valid.

To do so we call 'pickup_locations' on the bib, or the item.

The item case is trivial, however, for bibs we call Item->pickup_locations for every item on the bib. If we are simply validating a single location this can be quite expensive and unnecessary.

I propose adding a new function to specifically validate pickup location for a biblio
Comment 1 Nick Clemens 2023-04-09 20:09:38 UTC
Created attachment 149379 [details] [review]
Bug 33471: Add validate_pickup_location routine to Koha::Biblio

This patch adds a new routine, similar to pickup_locations except that
it short circuits once a location has been found as acceptable.

To test:
1 - Attempt placing a hold for an acceptable pickup location via the API
2 - Attempt to change pickup location to an invalid location, you are blocked
3 - Attempt to place a another hold via API for an invalid locatoin, you are blocked
4 - Apply patch
5 - Repeat
6 - Results should be the same
Comment 2 Emily Lamancusa 2023-04-10 20:40:17 UTC
Created attachment 149411 [details] [review]
Bug 33471: Add validate_pickup_location routine to Koha::Biblio

This patch adds a new routine, similar to pickup_locations except that
it short circuits once a location has been found as acceptable.

To test:
1 - Attempt placing a hold for an acceptable pickup location via the API
2 - Attempt to change pickup location to an invalid location, you are blocked
3 - Attempt to place a another hold via API for an invalid locatoin, you are blocked
4 - Apply patch
5 - Repeat
6 - Results should be the same

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 3 Marcel de Rooy 2023-05-12 08:49:45 UTC
Marking as enh
Comment 4 Marcel de Rooy 2023-07-14 07:27:57 UTC
++<<<<<<< HEAD
 +subtest 'pickup_locations() tests' => sub {
 +
 +    plan tests => 11;
++=======
+ subtest 'pickup_locations' => sub {
+     plan tests => 73;
++>>>>>>> Bug 33471: Add validate_pickup_location routine to Koha::Biblio

Parallel development ?
Comment 5 Nick Clemens 2023-07-14 11:38:45 UTC
Created attachment 153473 [details] [review]
Bug 33471: Add validate_pickup_location routine to Koha::Biblio

This patch adds a new routine, similar to pickup_locations except that
it short circuits once a location has been found as acceptable.

To test:
1 - Attempt placing a hold for an acceptable pickup location via the API
2 - Attempt to change pickup location to an invalid location, you are blocked
3 - Attempt to place a another hold via API for an invalid locatoin, you are blocked
4 - Apply patch
5 - Repeat
6 - Results should be the same

Patch tidied after sign off

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 6 Nick Clemens 2023-07-14 11:42:20 UTC
(In reply to Marcel de Rooy from comment #4)
> ++<<<<<<< HEAD
>  +subtest 'pickup_locations() tests' => sub {
>  +
>  +    plan tests => 11;
> ++=======
> + subtest 'pickup_locations' => sub {
> +     plan tests => 73;
> ++>>>>>>> Bug 33471: Add validate_pickup_location routine to Koha::Biblio
> 
> Parallel development ?

Yes, 33447 - Tomas' followup adjusted those lines
Comment 7 Jonathan Druart 2023-07-19 12:31:07 UTC
Hum, I am not convinced. You are actually caching the value (not exactly, almost) processed in Koha::Item->pickup_location but in Koha::Biblio->validate_pickup_location.
And assume that you know that Koha::Item->pickup_location return value depends on itype, homebranch, holdingbranch, ccode, branchcode.

What if the logic in Koha::Item->pickup_location changes and uses another parameter?


minor:
$item->pickup_locations( { patron => $patron } )->_resultset->get_column('branchcode')->all;

=> you should use Koha::Objects->get_column('branchcode')
Comment 8 Jonathan Druart 2023-07-19 12:35:57 UTC
Why not doing the caching in Koha::Item->pickup_locations? You could cache the branchcodes and return them. Less performance but more robust IMO.

Please ask for more feedback before implementing anything else, I am not 100% certain it's making sense.
Comment 9 Nick Clemens 2023-07-20 15:56:51 UTC
(In reply to Jonathan Druart from comment #8)
> Why not doing the caching in Koha::Item->pickup_locations? You could cache
> the branchcodes and return them. Less performance but more robust IMO.
> 
> Please ask for more feedback before implementing anything else, I am not
> 100% certain it's making sense.

Yeah, I can see that - use the L1 Cache in pickup locations, then if the routine it should be visible that the cache would need to be changed

But certainly we can improve what we are doing right now