| Summary: | Improve performance of hold pickup location verification for next available holds | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Nick Clemens (kidclamp) <nick> |
| Component: | Hold requests | Assignee: | Nick Clemens (kidclamp) <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: | ||
| Circulation function: | |||
| 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 (kidclamp)
2023-04-09 20:05:54 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 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 |