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
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
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>
Marking as enh
++<<<<<<< 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 ?
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>
(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
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')
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.
(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