Bug 33471

Summary: Improve performance of hold pickup location verification for next available holds
Product: Koha Reporter: Nick Clemens <nick>
Component: Hold requestsAssignee: Nick Clemens <nick>
Status: ASSIGNED --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: emily.lamancusa, gmcharlt, jonathan.druart, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 33447, 33470    
Bug Blocks:    
Attachments: Bug 33471: Add validate_pickup_location routine to Koha::Biblio
Bug 33471: Add validate_pickup_location routine to Koha::Biblio
Bug 33471: Add validate_pickup_location routine to Koha::Biblio

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