https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33471 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 149379 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149379&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33471 Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #149379|0 |1 is obsolete| | --- Comment #2 from Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> --- Created attachment 149411 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=149411&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33471 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl Severity|normal |enhancement --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Marking as enh -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33471 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- ++<<<<<<< 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 ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33471 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #149411|0 |1 is obsolete| | --- Comment #5 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 153473 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153473&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33471 --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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') -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33471 --- Comment #9 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.