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

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

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() ?

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

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

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>

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>

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>

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>

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>

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>

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.

Created attachment 113451 [details] [review]
Bug 26963: Unit tests

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>

Created attachment 113453 [details] [review]
Bug 26963: Unit tests

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>

Created attachment 113455 [details] [review]
Bug 26963: (follow-up) Change subroutine name for QA tools

It didn't like the ending _at

(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

(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 :)

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.

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?

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).

Created attachment 113483 [details] [review]
Bug 26963: [GOLF] Don't delete existing data before tests

Created attachment 113498 [details] [review]
Bug 26963: [GOLF] Fix cases where we expected a list

Created attachment 113503 [details] [review]
Bug 26963: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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>

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>

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>

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>

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>

Created attachment 113509 [details] [review]
Bug 26963: [GOLF] Update mocked return of pickup_locations

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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>

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>

(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.

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>

Created attachment 113513 [details] [review]
Bug 26963: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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

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?

(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

Created attachment 113586 [details] [review]
Bug 26963: Ignore existing libraries

(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)

Pushed to master for 20.11, thanks to everybody involved!

I would think this is needed in 20.05 but the patchset does not apply clean, any chance we can get a rebase?

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

backported to 20.05.x for 20.05.06

missing dependencies, not backported to 19.11.x