Bug 26963 - Improve Koha::Item::pickup_locations performance
Summary: Improve Koha::Item::pickup_locations performance
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P3 major (vote)
Assignee: Nick Clemens
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 22284
Blocks: 26988 27002 27062
  Show dependency treegraph
 
Reported: 2020-11-07 12:19 UTC by Kyle M Hall
Modified: 2020-11-20 09:31 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Koha::Item::pickup_locations is very inefficient, causing timeouts on records with large numbers of holds/items. This development refactors the underlying implementation, and also makes the method return a resultset, to delay as much as possible the DB access, and thus allowing for further filtering on the callers, through chaining.
Version(s) released in:
20.11.00, 20.05.06


Attachments
Bug 26963: Koha::Template::Plugin::Branches::pickup_locations is very inefficient, causing timeouts on records with large numbers of holds/items (2.79 KB, patch)
2020-11-07 12:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26963: Koha::Template::Plugin::Branches::pickup_locations is very inefficient, causing timeouts on records with large numbers of holds/items (3.83 KB, patch)
2020-11-07 12:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item (4.59 KB, patch)
2020-11-09 16:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item (4.62 KB, patch)
2020-11-09 16:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item (4.62 KB, patch)
2020-11-09 19:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item (4.82 KB, patch)
2020-11-10 10:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26963: Cache pickup locations for biblio level holds in the template (3.57 KB, patch)
2020-11-10 10:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item (4.89 KB, patch)
2020-11-10 16:54 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26963: Cache pickup locations for biblio level holds in the template (3.64 KB, patch)
2020-11-10 16:54 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item (4.94 KB, patch)
2020-11-10 17:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: Cache pickup locations for biblio level holds in the template (3.70 KB, patch)
2020-11-10 17:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item (4.94 KB, patch)
2020-11-10 17:17 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26963: Cache pickup locations for biblio level holds in the template (3.69 KB, patch)
2020-11-10 17:17 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26963: Unit tests (4.53 KB, patch)
2020-11-11 02:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item (4.94 KB, patch)
2020-11-11 02:32 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26963: Unit tests (4.53 KB, patch)
2020-11-11 02:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item (4.92 KB, patch)
2020-11-11 02:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26963: (follow-up) Change subroutine name for QA tools (4.77 KB, patch)
2020-11-11 02:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26963: [GOLF] Convert to ResultSets (6.67 KB, patch)
2020-11-11 11:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: [GOLF] Migrate unit tests into pickup_location (12.33 KB, patch)
2020-11-11 11:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: [GOLF] Don't delete existing data before tests (7.71 KB, patch)
2020-11-11 12:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26963: [GOLF] Fix cases where we expected a list (2.13 KB, patch)
2020-11-11 14:09 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26963: Unit tests (4.59 KB, patch)
2020-11-11 15:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item (4.97 KB, patch)
2020-11-11 15:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: (follow-up) Change subroutine name for QA tools (4.84 KB, patch)
2020-11-11 15:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: [GOLF] Convert to ResultSets (6.74 KB, patch)
2020-11-11 15:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: [GOLF] Migrate unit tests into pickup_location (12.40 KB, patch)
2020-11-11 15:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: [GOLF] Don't delete existing data before tests (7.77 KB, patch)
2020-11-11 15:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: [GOLF] Update mocked return of pickup_locations (945 bytes, patch)
2020-11-11 15:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: [GOLF] Fix cases where we expected a list (2.18 KB, patch)
2020-11-11 15:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: [GOLF] Fix up tests and cover case of undefined ccode (5.01 KB, patch)
2020-11-11 15:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26963: [GOLF] Fix up tests and cover case of undefined ccode (5.08 KB, patch)
2020-11-11 16:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: Unit tests (4.59 KB, patch)
2020-11-11 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item (4.97 KB, patch)
2020-11-11 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: (follow-up) Change subroutine name for QA tools (4.84 KB, patch)
2020-11-11 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: (QA follow-up) Convert to ResultSets (6.75 KB, patch)
2020-11-11 16:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: (QA follow-up) Migrate unit tests into pickup_location (12.41 KB, patch)
2020-11-11 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: (QA follow-up) Don't delete existing data before tests (7.78 KB, patch)
2020-11-11 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: (QA follow-up) Update mocked return of pickup_locations (954 bytes, patch)
2020-11-11 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: (QA follow-up) Fix cases where we expected a list (2.19 KB, patch)
2020-11-11 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: (QA follow-up) Fix up tests and cover case of undefined ccode (5.08 KB, patch)
2020-11-11 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26963: Unit tests (4.64 KB, patch)
2020-11-11 18:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item (5.03 KB, patch)
2020-11-11 18:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26963: (follow-up) Change subroutine name for QA tools (4.89 KB, patch)
2020-11-11 18:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26963: (QA follow-up) Convert to ResultSets (6.80 KB, patch)
2020-11-11 18:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26963: (QA follow-up) Migrate unit tests into pickup_location (12.46 KB, patch)
2020-11-11 18:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26963: (QA follow-up) Don't delete existing data before tests (7.83 KB, patch)
2020-11-11 18:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26963: (QA follow-up) Update mocked return of pickup_locations (1011 bytes, patch)
2020-11-11 18:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26963: (QA follow-up) Fix cases where we expected a list (2.25 KB, patch)
2020-11-11 18:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26963: (QA follow-up) Fix up tests and cover case of undefined ccode (5.14 KB, patch)
2020-11-11 18:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26963: Ignore existing libraries (1.97 KB, patch)
2020-11-13 08:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26963: [20.05.x] (24.05 KB, patch)
2020-11-17 12: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 Kyle M Hall 2020-11-07 12:19:17 UTC
If a record has a large number of holds ( > 50 I'd say ), and a large number of items, checking the pickup locations takes so long the script often times out. We need to find a way to cache the results so we don't have to spend so much time on expensive lookups.
Comment 1 Kyle M Hall 2020-11-07 12:24:55 UTC
Created attachment 113282 [details] [review]
Bug 26963: Koha::Template::Plugin::Branches::pickup_locations is very inefficient, causing timeouts on records with large numbers of holds/items

If a record has a large number of holds ( > 50 I'd say ), and a large number of items, checking the pickup locations takes so long the script often times out. We need to find a way to cache the results so we don't have to spend so much time on expensive lookups.

Test Plan:
1) Create a record with 100 items
2) Create 100 holds that are item level
3) You should start to see slow page loads as you create more holds
4) Apply this patch
5) Restart all the things!
6) Note the hold page now loads faster
7) Subsequent loads should be even faster due to the caching mechanism
Comment 2 Kyle M Hall 2020-11-07 12:45:35 UTC
Created attachment 113283 [details] [review]
Bug 26963: Koha::Template::Plugin::Branches::pickup_locations is very inefficient, causing timeouts on records with large numbers of holds/items

If a record has a large number of holds ( > 50 I'd say ), and a large number of items, checking the pickup locations takes so long the script often times out. We need to find a way to cache the results so we don't have to spend so much time on expensive lookups.

Test Plan:
1) Create a record with 100 items
2) Create 100 holds that are item level
3) You should start to see slow page loads as you create more holds
4) Apply this patch
5) Restart all the things!
6) Note the hold page now loads faster
7) Subsequent loads should be even faster due to the caching mechanism
Comment 3 Fridolin Somers 2020-11-09 07:34:40 UTC
Just to understand :
  $cache_key = "I" . $item->homebranch . "-" . $patron->branchcode
The use of homebranch and branchcode in key are logic on actual code in Koha::Items::pickup_locations().
But this code may change...

Couldn't we cache inside Koha::Items::pickup_locations() ?
Comment 4 Nick Clemens 2020-11-09 16:29:45 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens 2020-11-09 16:57:33 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens 2020-11-09 19:29:48 UTC Comment hidden (obsolete)
Comment 7 Nick Clemens 2020-11-10 10:54:43 UTC
Created attachment 113339 [details] [review]
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item

Currently When calling Koha::Template::Plugin::Branches::pickup_locations
we call pickup_location for each item of the bib, and for each item we get a list of possible
branches, we then check those branches against the transfer limits, this is inefficent

Given a system with 100 branches, and each branch having an item attached to one bib (100 items)
we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib

For me this patch reduced load time from 77 seconds to 18 seconds

To test:
1 - Find a bib
2 - Place 4 title level holds
3 - Add some branches and items for this bib to your system:
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"C"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"D"),pickup_location FROM branches;
  INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches;
4 - Set systempreferences:
    UseBranchTransferLimits = 'enforce'
     BranchTransferLimitsType = 'item type'
5 - Find the bib and click the holds tab
6 - Wait for a long time, it shoudl eventually load
7 - Apply patch and restart al the things
8 - Load the page again, it should be much faster
Comment 8 Nick Clemens 2020-11-10 10:54:46 UTC
Created attachment 113340 [details] [review]
Bug 26963: Cache pickup locations for biblio level holds in the template

When determining pickup locations for holds that have not specified an itemnumber we will get the same
results for each hold. This patch simply places the list of libraries into a template variable and reuses
it for each hold

To test:
1 - Have several title level holds with varying pickup locations
2 - Have some libraries that are not pickup locations
3 - View the holds table for the biblio with the holds
4 - Observe the dropdowns load correctly
5 - Apply patch
6 - Verify there are no changes
Comment 9 Andrew Fuerste-Henry 2020-11-10 16:54:51 UTC
Created attachment 113419 [details] [review]
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item

Currently When calling Koha::Template::Plugin::Branches::pickup_locations
we call pickup_location for each item of the bib, and for each item we get a list of possible
branches, we then check those branches against the transfer limits, this is inefficent

Given a system with 100 branches, and each branch having an item attached to one bib (100 items)
we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib

For me this patch reduced load time from 77 seconds to 18 seconds

To test:
1 - Find a bib
2 - Place 4 title level holds
3 - Add some branches and items for this bib to your system:
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"C"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"D"),pickup_location FROM branches;
  INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches;
4 - Set systempreferences:
    UseBranchTransferLimits = 'enforce'
     BranchTransferLimitsType = 'item type'
5 - Find the bib and click the holds tab
6 - Wait for a long time, it shoudl eventually load
7 - Apply patch and restart al the things
8 - Load the page again, it should be much faster

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 10 Andrew Fuerste-Henry 2020-11-10 16:54:54 UTC
Created attachment 113420 [details] [review]
Bug 26963: Cache pickup locations for biblio level holds in the template

When determining pickup locations for holds that have not specified an itemnumber we will get the same
results for each hold. This patch simply places the list of libraries into a template variable and reuses
it for each hold

To test:
1 - Have several title level holds with varying pickup locations
2 - Have some libraries that are not pickup locations
3 - View the holds table for the biblio with the holds
4 - Observe the dropdowns load correctly
5 - Apply patch
6 - Verify there are no changes

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 11 Martin Renvoize 2020-11-10 17:16:34 UTC
Created attachment 113422 [details] [review]
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item

Currently When calling Koha::Template::Plugin::Branches::pickup_locations
we call pickup_location for each item of the bib, and for each item we get a list of possible
branches, we then check those branches against the transfer limits, this is inefficent

Given a system with 100 branches, and each branch having an item attached to one bib (100 items)
we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib

For me this patch reduced load time from 77 seconds to 18 seconds

To test:
1 - Find a bib
2 - Place 4 title level holds
3 - Add some branches and items for this bib to your system:
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"C"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"D"),pickup_location FROM branches;
  INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches;
4 - Set systempreferences:
    UseBranchTransferLimits = 'enforce'
     BranchTransferLimitsType = 'item type'
5 - Find the bib and click the holds tab
6 - Wait for a long time, it shoudl eventually load
7 - Apply patch and restart al the things
8 - Load the page again, it should be much faster

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2020-11-10 17:16:38 UTC
Created attachment 113423 [details] [review]
Bug 26963: Cache pickup locations for biblio level holds in the template

When determining pickup locations for holds that have not specified an itemnumber we will get the same
results for each hold. This patch simply places the list of libraries into a template variable and reuses
it for each hold

To test:
1 - Have several title level holds with varying pickup locations
2 - Have some libraries that are not pickup locations
3 - View the holds table for the biblio with the holds
4 - Observe the dropdowns load correctly
5 - Apply patch
6 - Verify there are no changes

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Andrew Fuerste-Henry 2020-11-10 17:17:00 UTC
Created attachment 113424 [details] [review]
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item

Currently When calling Koha::Template::Plugin::Branches::pickup_locations
we call pickup_location for each item of the bib, and for each item we get a list of possible
branches, we then check those branches against the transfer limits, this is inefficent

Given a system with 100 branches, and each branch having an item attached to one bib (100 items)
we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib

For me this patch reduced load time from 77 seconds to 18 seconds

To test:
1 - Find a bib
2 - Place 4 title level holds
3 - Add some branches and items for this bib to your system:
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"C"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"D"),pickup_location FROM branches;
  INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches;
4 - Set systempreferences:
    UseBranchTransferLimits = 'enforce'
     BranchTransferLimitsType = 'item type'
5 - Find the bib and click the holds tab
6 - Wait for a long time, it shoudl eventually load
7 - Apply patch and restart al the things
8 - Load the page again, it should be much faster

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
Comment 14 Andrew Fuerste-Henry 2020-11-10 17:17:03 UTC
Created attachment 113425 [details] [review]
Bug 26963: Cache pickup locations for biblio level holds in the template

When determining pickup locations for holds that have not specified an itemnumber we will get the same
results for each hold. This patch simply places the list of libraries into a template variable and reuses
it for each hold

To test:
1 - Have several title level holds with varying pickup locations
2 - Have some libraries that are not pickup locations
3 - View the holds table for the biblio with the holds
4 - Observe the dropdowns load correctly
5 - Apply patch
6 - Verify there are no changes

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
Comment 15 Martin Renvoize 2020-11-10 17:20:37 UTC
This appears to work in testing.. but a few things worry me slightly.

I'm not seeing great unit tests for the branchlimits handling of the pickup_locations method and as we've not got new tests for the introduced private method I'd have to fail QA on that front.

As this fixes a bug I think we should write the tests to prove branchlimits side and probably go with the code as is.. however, as I dug into it, the code feels a bit manic in this area I feel like a bit of golfing down the line would be great.

I'd love to see the methods that are currently returning arrayrefs of Koha::Objects actually return proper Koha::Object sets which could then be chained properly.
Comment 16 Nick Clemens 2020-11-11 02:32:33 UTC
Created attachment 113451 [details] [review]
Bug 26963: Unit tests
Comment 17 Nick Clemens 2020-11-11 02:32:36 UTC
Created attachment 113452 [details] [review]
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item

Currently When calling Koha::Template::Plugin::Branches::pickup_locations
we call pickup_location for each item of the bib, and for each item we get a list of possible
branches, we then check those branches against the transfer limits, this is inefficent

Given a system with 100 branches, and each branch having an item attached to one bib (100 items)
we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib

For me this patch reduced load time from 77 seconds to 18 seconds

To test:
1 - Find a bib
2 - Place 4 title level holds
3 - Add some branches and items for this bib to your system:
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"C"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"D"),pickup_location FROM branches;
  INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches;
4 - Set systempreferences:
    UseBranchTransferLimits = 'enforce'
     BranchTransferLimitsType = 'item type'
5 - Find the bib and click the holds tab
6 - Wait for a long time, it shoudl eventually load
7 - Apply patch and restart al the things
8 - Load the page again, it should be much faster

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
Comment 18 Nick Clemens 2020-11-11 02:40:54 UTC
Created attachment 113453 [details] [review]
Bug 26963: Unit tests
Comment 19 Nick Clemens 2020-11-11 02:40:57 UTC
Created attachment 113454 [details] [review]
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item

Currently When calling Koha::Template::Plugin::Branches::pickup_locations
we call pickup_location for each item of the bib, and for each item we get a list of possible
branches, we then check those branches against the transfer limits, this is inefficent

Given a system with 100 branches, and each branch having an item attached to one bib (100 items)
we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib

For me this patch reduced load time from 77 seconds to 18 seconds

To test:
1 - Find a bib
2 - Place 4 title level holds
3 - Add some branches and items for this bib to your system:
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"C"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"D"),pickup_location FROM branches;
  INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches;
4 - Set systempreferences:
    UseBranchTransferLimits = 'enforce'
     BranchTransferLimitsType = 'item type'
5 - Find the bib and click the holds tab
6 - Wait for a long time, it shoudl eventually load
7 - Apply patch and restart al the things
8 - Load the page again, it should be much faster

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
Comment 20 Nick Clemens 2020-11-11 02:41:00 UTC
Created attachment 113455 [details] [review]
Bug 26963: (follow-up) Change subroutine name for QA tools

It didn't like the ending _at
Comment 21 Nick Clemens 2020-11-11 02:43:02 UTC
(In reply to Martin Renvoize from comment #15)
> I'm not seeing great unit tests for the branchlimits handling of the
> pickup_locations method

Tests added

> As this fixes a bug I think we should write the tests to prove branchlimits
> side and probably go with the code as is.. however, as I dug into it, the
> code feels a bit manic in this area I feel like a bit of golfing down the
> line would be great.
> 
> I'd love to see the methods that are currently returning arrayrefs of
> Koha::Objects actually return proper Koha::Object sets which could then be
> chained properly.

I agree strongly, but would like to see this on a new bug, and happy to help on that front.

This bug is a major issue and causing severe problems for our larger consortia and I would love to see it move forward
Comment 22 Jonathan Druart 2020-11-11 09:47:23 UTC
(In reply to Martin Renvoize from comment #15)
> I'd love to see the methods that are currently returning arrayrefs of
> Koha::Objects actually return proper Koha::Object sets which could then be
> chained properly.

I don't understand. Are you suggesting that Koha::Item->pickup_locations should return the Koha::Item object?

If it's a "getter" method I see no reason not to return what you had asked for :)
Comment 23 Martin Renvoize 2020-11-11 11:15:09 UTC
Created attachment 113480 [details] [review]
Bug 26963: [GOLF] Convert to ResultSets

This patch removes the previously introduced private method by
converting the arrayref returns to ResultSets appropriately and inlining
the filter search queries.
Comment 24 Martin Renvoize 2020-11-11 11:15:13 UTC
Created attachment 113481 [details] [review]
Bug 26963: [GOLF] Migrate unit tests into pickup_location

We wrote unit tests for _can_pickup_locations as part of this patchset,
but then I inlined the method whilst golfing. This patch moves those
tests into the existing pickup_locations test so we more thoroughly
cover the case where branch transfer limits are in play.

NOTE: The tests all assume that all items have an effective_itemtype and
ccode. I'm pretty sure items all have a type at this point, but I'm less
sure we enforce collection codes?
Comment 25 Martin Renvoize 2020-11-11 11:17:11 UTC
OK, So golfing was the best way for me to wrap my head around the code here :).

I intend to move those GOLF patches into their own bug and not hold up proceedings here... but I wanted to post them to show my working ;).

QA Question.. I've lost myself a little here.. can an item exist without a collection code?  If so, I think the tests should also test for this case.. which they currently don't do (And would currently fail for I believe).
Comment 26 Nick Clemens 2020-11-11 12:49:25 UTC
Created attachment 113483 [details] [review]
Bug 26963: [GOLF] Don't delete existing data before tests
Comment 27 Nick Clemens 2020-11-11 14:09:52 UTC
Created attachment 113498 [details] [review]
Bug 26963: [GOLF] Fix cases where we expected a list
Comment 28 Martin Renvoize 2020-11-11 15:52:41 UTC
Created attachment 113503 [details] [review]
Bug 26963: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2020-11-11 15:52:44 UTC
Created attachment 113504 [details] [review]
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item

Currently When calling Koha::Template::Plugin::Branches::pickup_locations
we call pickup_location for each item of the bib, and for each item we get a list of possible
branches, we then check those branches against the transfer limits, this is inefficent

Given a system with 100 branches, and each branch having an item attached to one bib (100 items)
we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib

For me this patch reduced load time from 77 seconds to 18 seconds

To test:
1 - Find a bib
2 - Place 4 title level holds
3 - Add some branches and items for this bib to your system:
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"C"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"D"),pickup_location FROM branches;
  INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches;
4 - Set systempreferences:
    UseBranchTransferLimits = 'enforce'
     BranchTransferLimitsType = 'item type'
5 - Find the bib and click the holds tab
6 - Wait for a long time, it shoudl eventually load
7 - Apply patch and restart al the things
8 - Load the page again, it should be much faster

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2020-11-11 15:52:47 UTC
Created attachment 113505 [details] [review]
Bug 26963: (follow-up) Change subroutine name for QA tools

It didn't like the ending _at

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2020-11-11 15:52:51 UTC
Created attachment 113506 [details] [review]
Bug 26963: [GOLF] Convert to ResultSets

This patch removes the previously introduced private method by
converting the arrayref returns to ResultSets appropriately and inlining
the filter search queries.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2020-11-11 15:52:54 UTC
Created attachment 113507 [details] [review]
Bug 26963: [GOLF] Migrate unit tests into pickup_location

We wrote unit tests for _can_pickup_locations as part of this patchset,
but then I inlined the method whilst golfing. This patch moves those
tests into the existing pickup_locations test so we more thoroughly
cover the case where branch transfer limits are in play.

NOTE: The tests all assume that all items have an effective_itemtype and
ccode. I'm pretty sure items all have a type at this point, but I'm less
sure we enforce collection codes?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2020-11-11 15:52:58 UTC
Created attachment 113508 [details] [review]
Bug 26963: [GOLF] Don't delete existing data before tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2020-11-11 15:53:01 UTC
Created attachment 113509 [details] [review]
Bug 26963: [GOLF] Update mocked return of pickup_locations

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2020-11-11 15:53:04 UTC
Created attachment 113510 [details] [review]
Bug 26963: [GOLF] Fix cases where we expected a list

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Nick Clemens 2020-11-11 15:58:13 UTC
Created attachment 113511 [details] [review]
Bug 26963: [GOLF] Fix up tests and cover case of undefined ccode

While this technically shouldn't happen, if a library creates itemtype limits, then flips the pref, those rules are still in the db until a ccode rule is saved.

To be extra safe, when checking for rules of one type, we should make sure the other type is undef - i.e. if looking for ccode rules, we should confirm the itype is undef for those rules

Additionally, we shouldn't set the barcode now that we are not deleting all items, so we use copynumber for our item identification field as it doesn't need to be unique in the DB

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 37 Martin Renvoize 2020-11-11 15:59:12 UTC
(In reply to Jonathan Druart from comment #22)
> (In reply to Martin Renvoize from comment #15)
> > I'd love to see the methods that are currently returning arrayrefs of
> > Koha::Objects actually return proper Koha::Object sets which could then be
> > chained properly.
> 
> I don't understand. Are you suggesting that Koha::Item->pickup_locations
> should return the Koha::Item object?
> 
> If it's a "getter" method I see no reason not to return what you had asked
> for :)

It is indeed a getter.. but to me it clearly sounds like a relation getter.. i.e that it should return a related set.. i.e. a Koha::Libraries object as apposed to an ARRAREF of Koha::Library objects... with a proper Koha::Libraries object you can further refine the results by chaining additional ->search calls for example.. which is the exact case we're running into here.
Comment 38 Martin Renvoize 2020-11-11 16:04:29 UTC
Created attachment 113512 [details] [review]
Bug 26963: [GOLF] Fix up tests and cover case of undefined ccode

While this technically shouldn't happen, if a library creates itemtype limits, then flips the pref, those rules are still in the db until a ccode rule is saved.

To be extra safe, when checking for rules of one type, we should make sure the other type is undef - i.e. if looking for ccode rules, we should confirm the itype is undef for those rules

Additionally, we shouldn't set the barcode now that we are not deleting all items, so we use copynumber for our item identification field as it doesn't need to be unique in the DB

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2020-11-11 16:07:47 UTC
Created attachment 113513 [details] [review]
Bug 26963: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Martin Renvoize 2020-11-11 16:07:50 UTC
Created attachment 113514 [details] [review]
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item

Currently When calling Koha::Template::Plugin::Branches::pickup_locations
we call pickup_location for each item of the bib, and for each item we get a list of possible
branches, we then check those branches against the transfer limits, this is inefficent

Given a system with 100 branches, and each branch having an item attached to one bib (100 items)
we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib

For me this patch reduced load time from 77 seconds to 18 seconds

To test:
1 - Find a bib
2 - Place 4 title level holds
3 - Add some branches and items for this bib to your system:
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"C"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"D"),pickup_location FROM branches;
  INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches;
4 - Set systempreferences:
    UseBranchTransferLimits = 'enforce'
     BranchTransferLimitsType = 'item type'
5 - Find the bib and click the holds tab
6 - Wait for a long time, it shoudl eventually load
7 - Apply patch and restart al the things
8 - Load the page again, it should be much faster

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 41 Martin Renvoize 2020-11-11 16:07:54 UTC
Created attachment 113515 [details] [review]
Bug 26963: (follow-up) Change subroutine name for QA tools

It didn't like the ending _at

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 42 Martin Renvoize 2020-11-11 16:07:57 UTC
Created attachment 113516 [details] [review]
Bug 26963: (QA follow-up) Convert to ResultSets

This patch removes the previously introduced private method by
converting the arrayref returns to ResultSets appropriately and inlining
the filter search queries.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 43 Martin Renvoize 2020-11-11 16:08:01 UTC
Created attachment 113517 [details] [review]
Bug 26963: (QA follow-up) Migrate unit tests into pickup_location

We wrote unit tests for _can_pickup_locations as part of this patchset,
but then I inlined the method whilst golfing. This patch moves those
tests into the existing pickup_locations test so we more thoroughly
cover the case where branch transfer limits are in play.

NOTE: The tests all assume that all items have an effective_itemtype and
ccode. I'm pretty sure items all have a type at this point, but I'm less
sure we enforce collection codes?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 44 Martin Renvoize 2020-11-11 16:08:04 UTC
Created attachment 113518 [details] [review]
Bug 26963: (QA follow-up) Don't delete existing data before tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 45 Martin Renvoize 2020-11-11 16:08:07 UTC
Created attachment 113519 [details] [review]
Bug 26963: (QA follow-up) Update mocked return of pickup_locations

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 46 Martin Renvoize 2020-11-11 16:08:11 UTC
Created attachment 113520 [details] [review]
Bug 26963: (QA follow-up) Fix cases where we expected a list

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Martin Renvoize 2020-11-11 16:08:14 UTC
Created attachment 113521 [details] [review]
Bug 26963: (QA follow-up) Fix up tests and cover case of undefined ccode

While this technically shouldn't happen, if a library creates itemtype limits, then flips the pref, those rules are still in the db until a ccode rule is saved.

To be extra safe, when checking for rules of one type, we should make sure the other type is undef - i.e. if looking for ccode rules, we should confirm the itype is undef for those rules

Additionally, we shouldn't set the barcode now that we are not deleting all items, so we use copynumber for our item identification field as it doesn't need to be unique in the DB

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Tomás Cohen Arazi 2020-11-11 18:25:50 UTC
Created attachment 113523 [details] [review]
Bug 26963: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 49 Tomás Cohen Arazi 2020-11-11 18:26:00 UTC
Created attachment 113524 [details] [review]
Bug 26963: Don't call 'can_be_transferred' for each possible library for each item

Currently When calling Koha::Template::Plugin::Branches::pickup_locations
we call pickup_location for each item of the bib, and for each item we get a list of possible
branches, we then check those branches against the transfer limits, this is inefficent

Given a system with 100 branches, and each branch having an item attached to one bib (100 items)
we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib

For me this patch reduced load time from 77 seconds to 18 seconds

To test:
1 - Find a bib
2 - Place 4 title level holds
3 - Add some branches and items for this bib to your system:
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"C"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"D"),pickup_location FROM branches;
  INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches;
4 - Set systempreferences:
    UseBranchTransferLimits = 'enforce'
     BranchTransferLimitsType = 'item type'
5 - Find the bib and click the holds tab
6 - Wait for a long time, it shoudl eventually load
7 - Apply patch and restart al the things
8 - Load the page again, it should be much faster

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 50 Tomás Cohen Arazi 2020-11-11 18:26:09 UTC
Created attachment 113525 [details] [review]
Bug 26963: (follow-up) Change subroutine name for QA tools

It didn't like the ending _at

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 51 Tomás Cohen Arazi 2020-11-11 18:26:17 UTC
Created attachment 113526 [details] [review]
Bug 26963: (QA follow-up) Convert to ResultSets

This patch removes the previously introduced private method by
converting the arrayref returns to ResultSets appropriately and inlining
the filter search queries.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 52 Tomás Cohen Arazi 2020-11-11 18:26:24 UTC
Created attachment 113527 [details] [review]
Bug 26963: (QA follow-up) Migrate unit tests into pickup_location

We wrote unit tests for _can_pickup_locations as part of this patchset,
but then I inlined the method whilst golfing. This patch moves those
tests into the existing pickup_locations test so we more thoroughly
cover the case where branch transfer limits are in play.

NOTE: The tests all assume that all items have an effective_itemtype and
ccode. I'm pretty sure items all have a type at this point, but I'm less
sure we enforce collection codes?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 53 Tomás Cohen Arazi 2020-11-11 18:26:31 UTC
Created attachment 113528 [details] [review]
Bug 26963: (QA follow-up) Don't delete existing data before tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 54 Tomás Cohen Arazi 2020-11-11 18:26:38 UTC
Created attachment 113529 [details] [review]
Bug 26963: (QA follow-up) Update mocked return of pickup_locations

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 55 Tomás Cohen Arazi 2020-11-11 18:26:45 UTC
Created attachment 113530 [details] [review]
Bug 26963: (QA follow-up) Fix cases where we expected a list

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 56 Tomás Cohen Arazi 2020-11-11 18:26:51 UTC
Created attachment 113531 [details] [review]
Bug 26963: (QA follow-up) Fix up tests and cover case of undefined ccode

While this technically shouldn't happen, if a library creates itemtype limits, then flips the pref, those rules are still in the db until a ccode rule is saved.

To be extra safe, when checking for rules of one type, we should make sure the other type is undef - i.e. if looking for ccode rules, we should confirm the itype is undef for those rules

Additionally, we shouldn't set the barcode now that we are not deleting all items, so we use copynumber for our item identification field as it doesn't need to be unique in the DB

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 57 Liz Rea 2020-11-12 15:22:31 UTC
To answer a question from yonks ago, an item can most certainly exist without a collection code. They are Authorised values and some libraries do not use them at all.

Liz
Comment 58 Jonathan Druart 2020-11-12 16:21:04 UTC
I think we should keep the number of tests, don't you?


I wrote a similar patch than "Don't delete existing data before tests" today:
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113551

I will drop mine, but I think this part makes sense:
+            next
+              unless grep { $pickup_location eq $_ } @branchcodes;


Do we agree that this change can be considered an enhancement?
Comment 59 Nick Clemens 2020-11-12 16:23:22 UTC
(In reply to Jonathan Druart from comment #58)
> I think we should keep the number of tests, don't you?
> 
> 
> I wrote a similar patch than "Don't delete existing data before tests" today:
> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113551
> 
> I will drop mine, but I think this part makes sense:
> +            next
> +              unless grep { $pickup_location eq $_ } @branchcodes;
> 
> 
> Do we agree that this change can be considered an enhancement?

I do keep the number of tests, I just calculate it based on the number of branches - I think the line you mention makes that unecessary though, so yes, that line should be copied here

This should be considered a bug - not an enhancement. The current code can crash a system with the number of DB requests generated and should be backported to avoid issues on any large systems
Comment 60 Jonathan Druart 2020-11-13 08:43:26 UTC
Created attachment 113586 [details] [review]
Bug 26963: Ignore existing libraries
Comment 61 Jonathan Druart 2020-11-13 10:29:44 UTC
(In reply to Nick Clemens from comment #59)
> This should be considered a bug - not an enhancement. The current code can
> crash a system with the number of DB requests generated and should be
> backported to avoid issues on any large systems

Ok, but we are working on 2 things at the same time, modifying the same code.
So which one goes first? This one or bug 26988?

We need a dependency between both, otherwise it does not work...

https://snipboard.io/u4TGrK.jpg

[2020/11/13 10:28:48] [ERROR] GET /api/v1/holds/8/pickup_locations: unhandled exception (Mojo::Exception)<<Not an ARRAY reference at /kohadevbox/koha/Koha/REST/V1/Holds.pm line 422.>> Koha::REST::Plugin::Exceptions::__ANON__ /kohadevbox/koha/Koha/REST/Plugin/Exceptions.pm (73)
Comment 62 Jonathan Druart 2020-11-13 13:24:20 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 63 Lucas Gass 2020-11-16 22:53:23 UTC
I would think this is needed in 20.05 but the patchset does not apply clean, any chance we can get a rebase?
Comment 64 Nick Clemens 2020-11-17 12:38:55 UTC
Created attachment 113729 [details] [review]
Bug 26963: [20.05.x]

Bug 26963: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 26963: Don't call 'can_be_transferred' for each possible library for each item

Currently When calling Koha::Template::Plugin::Branches::pickup_locations
we call pickup_location for each item of the bib, and for each item we get a list of possible
branches, we then check those branches against the transfer limits, this is inefficent

Given a system with 100 branches, and each branch having an item attached to one bib (100 items)
we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib

For me this patch reduced load time from 77 seconds to 18 seconds

To test:
1 - Find a bib
2 - Place 4 title level holds
3 - Add some branches and items for this bib to your system:
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"C"),pickup_location FROM branches;
  INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"D"),pickup_location FROM branches;
  INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches;
4 - Set systempreferences:
    UseBranchTransferLimits = 'enforce'
     BranchTransferLimitsType = 'item type'
5 - Find the bib and click the holds tab
6 - Wait for a long time, it shoudl eventually load
7 - Apply patch and restart al the things
8 - Load the page again, it should be much faster

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 26963: (follow-up) Change subroutine name for QA tools

It didn't like the ending _at

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 26963: (QA follow-up) Convert to ResultSets

This patch removes the previously introduced private method by
converting the arrayref returns to ResultSets appropriately and inlining
the filter search queries.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 26963: (QA follow-up) Migrate unit tests into pickup_location

We wrote unit tests for _can_pickup_locations as part of this patchset,
but then I inlined the method whilst golfing. This patch moves those
tests into the existing pickup_locations test so we more thoroughly
cover the case where branch transfer limits are in play.

NOTE: The tests all assume that all items have an effective_itemtype and
ccode. I'm pretty sure items all have a type at this point, but I'm less
sure we enforce collection codes?

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 26963: (QA follow-up) Don't delete existing data before tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 26963: (QA follow-up) Update mocked return of pickup_locations

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 26963: (QA follow-up) Fix cases where we expected a list

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 26963: (QA follow-up) Fix up tests and cover case of undefined ccode

While this technically shouldn't happen, if a library creates itemtype limits, then flips the pref, those rules are still in the db until a ccode rule is saved.

To be extra safe, when checking for rules of one type, we should make sure the other type is undef - i.e. if looking for ccode rules, we should confirm the itype is undef for those rules

Additionally, we shouldn't set the barcode now that we are not deleting all items, so we use copynumber for our item identification field as it doesn't need to be unique in the DB

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 26963: Ignore existing libraries
Comment 65 Lucas Gass 2020-11-17 15:30:16 UTC
backported to 20.05.x for 20.05.06
Comment 66 Aleisha Amohia 2020-11-17 20:24:18 UTC
missing dependencies, not backported to 19.11.x