From 4ada0f6b60ab0b4df7f26b0875614f2aa1f25507 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Wed, 11 Nov 2020 01:24:15 +0000 Subject: [PATCH] Bug 26963: [20.05.x] Bug 26963: Unit tests Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi 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 Signed-off-by: Bob Bennhoff Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi Bug 26963: (follow-up) Change subroutine name for QA tools It didn't like the ending _at Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi 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 Signed-off-by: Tomas Cohen Arazi 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 Signed-off-by: Tomas Cohen Arazi Bug 26963: (QA follow-up) Don't delete existing data before tests Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi Bug 26963: (QA follow-up) Update mocked return of pickup_locations Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi Bug 26963: (QA follow-up) Fix cases where we expected a list Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi 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 Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi Bug 26963: Ignore existing libraries --- Koha/Biblio.pm | 2 +- Koha/Item.pm | 61 ++++--- Koha/Library.pm | 2 +- Koha/Template/Plugin/Branches.pm | 2 +- reserve/request.pl | 2 +- t/db_dependent/Koha/Item.t | 275 ++++++++++++++++++++---------- t/db_dependent/Template/Plugin/Branches.t | 3 +- 7 files changed, 231 insertions(+), 116 deletions(-) diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm index f430f4135a..3529a6878c 100644 --- a/Koha/Biblio.pm +++ b/Koha/Biblio.pm @@ -220,7 +220,7 @@ sub pickup_locations { my @pickup_locations; foreach my $item_of_bib ($self->items->as_list) { - push @pickup_locations, @{ $item_of_bib->pickup_locations( {patron => $patron} ) }; + push @pickup_locations, @{ $item_of_bib->pickup_locations( {patron => $patron} )->as_list() }; } my %seen; diff --git a/Koha/Item.pm b/Koha/Item.pm index c72c843ea4..a599d9834b 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -530,6 +530,7 @@ sub can_be_transferred { $limittype => $limittype eq 'itemtype' ? $self->effective_itemtype : $self->ccode })->count ? 0 : 1; + } =head3 pickup_locations @@ -551,39 +552,59 @@ sub pickup_locations { my $branchitemrule = C4::Circulation::GetBranchItemRule( $circ_control_branch, $self->itype ); - my @libs; if(defined $patron) { - return \@libs if $branchitemrule->{holdallowed} == 3 && !$self->home_branch->validate_hold_sibling( {branchcode => $patron->branchcode} ); - return \@libs if $branchitemrule->{holdallowed} == 1 && $self->home_branch->branchcode ne $patron->branchcode; + return Koha::Libraries->new()->empty if $branchitemrule->{holdallowed} == 3 && !$self->home_branch->validate_hold_sibling( {branchcode => $patron->branchcode} ); + return Koha::Libraries->new()->empty if $branchitemrule->{holdallowed} == 1 && $self->home_branch->branchcode ne $patron->branchcode; } + my $pickup_libraries = Koha::Libraries->search(); if ($branchitemrule->{hold_fulfillment_policy} eq 'holdgroup') { - @libs = $self->home_branch->get_hold_libraries; - push @libs, $self->home_branch unless scalar(@libs) > 0; + $pickup_libraries = $self->home_branch->get_hold_libraries; } elsif ($branchitemrule->{hold_fulfillment_policy} eq 'patrongroup') { my $plib = Koha::Libraries->find({ branchcode => $patron->branchcode}); - @libs = $plib->get_hold_libraries; - push @libs, $self->home_branch unless scalar(@libs) > 0; + $pickup_libraries = $plib->get_hold_libraries; } elsif ($branchitemrule->{hold_fulfillment_policy} eq 'homebranch') { - push @libs, $self->home_branch; + $pickup_libraries = Koha::Libraries->search({ branchcode => $self->homebranch }); } elsif ($branchitemrule->{hold_fulfillment_policy} eq 'holdingbranch') { - push @libs, $self->holding_branch; - } else { - @libs = Koha::Libraries->search({ + $pickup_libraries = Koha::Libraries->search({ branchcode => $self->holdingbranch }); + }; + + return $pickup_libraries->search( + { pickup_location => 1 - }, { + }, + { order_by => ['branchname'] - })->as_list; - } - - my @pickup_locations; - foreach my $library (@libs) { - if ($library->pickup_location && $self->can_be_transferred({ to => $library })) { - push @pickup_locations, $library; } + ) unless C4::Context->preference('UseBranchTransferLimits'); + + my $limittype = C4::Context->preference('BranchTransferLimitsType'); + my ($ccode, $itype) = (undef, undef); + if( $limittype eq 'ccode' ){ + $ccode = $self->ccode; + } else { + $itype = $self->itype; } + my $limits = Koha::Item::Transfer::Limits->search( + { + fromBranch => $self->holdingbranch, + ccode => $ccode, + itemtype => $itype, + }, + { columns => ['toBranch'] } + ); - return \@pickup_locations; + return $pickup_libraries->search( + { + pickup_location => 1, + branchcode => { + '-not_in' => $limits->_resultset->as_query + } + }, + { + order_by => ['branchname'] + } + ); } =head3 article_request_type diff --git a/Koha/Library.pm b/Koha/Library.pm index b3f763a9ef..e496e51444 100644 --- a/Koha/Library.pm +++ b/Koha/Library.pm @@ -160,7 +160,7 @@ sub get_hold_libraries { @hold_libraries = grep { !$seen{ $_->id }++ } @hold_libraries; - return @hold_libraries; + return Koha::Libraries->search({ branchcode => { '-in' => [ keys %seen ] } }); } =head3 validate_hold_sibling diff --git a/Koha/Template/Plugin/Branches.pm b/Koha/Template/Plugin/Branches.pm index 05932dac68..f0bbf40cd7 100644 --- a/Koha/Template/Plugin/Branches.pm +++ b/Koha/Template/Plugin/Branches.pm @@ -117,7 +117,7 @@ sub pickup_locations { if ($item) { $item = Koha::Items->find($item) unless ref($item) eq 'Koha::Item'; - @libraries = @{ $item->pickup_locations( { patron => $patron } ) } + @libraries = $item->pickup_locations( { patron => $patron } ) if defined $item; } elsif ($biblio) { diff --git a/reserve/request.pl b/reserve/request.pl index 1d5e59065b..0ae6a3f317 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -577,7 +577,7 @@ foreach my $biblionumber (@biblionumbers) { $item->{pickup_locations_code} = 'all'; } else { my $arr_locations = Koha::Items->find($itemnumber) - ->pickup_locations( { patron => $patron } ); + ->pickup_locations( { patron => $patron } )->as_list(); $item->{pickup_locations} = join( ', ', map { $_->unblessed->{branchname} } @$arr_locations); diff --git a/t/db_dependent/Koha/Item.t b/t/db_dependent/Koha/Item.t index 20547d71b4..b71ba4d1da 100644 --- a/t/db_dependent/Koha/Item.t +++ b/t/db_dependent/Koha/Item.t @@ -156,30 +156,12 @@ subtest "as_marc_field() tests" => sub { }; subtest 'pickup_locations' => sub { - plan tests => 114; + plan tests => 66; $schema->storage->txn_begin; my $dbh = C4::Context->dbh; - # Cleanup database - Koha::Holds->search->delete; - $dbh->do('DELETE FROM issues'); - Koha::Patrons->search->delete; - Koha::Items->search->delete; - Koha::Libraries->search->delete; - Koha::CirculationRules->search->delete; - Koha::CirculationRules->set_rules( - { - categorycode => undef, - itemtype => undef, - branchcode => undef, - rules => { - reservesallowed => 25, - } - } - ); - my $root1 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1, branchcode => undef } } ); my $root2 = $builder->build_object( { class => 'Koha::Library::Groups', value => { ft_local_hold_group => 1, branchcode => undef } } ); my $library1 = $builder->build_object( { class => 'Koha::Libraries', value => { pickup_location => 1, } } ); @@ -192,89 +174,106 @@ subtest 'pickup_locations' => sub { my $group2_1 = $builder->build_object( { class => 'Koha::Library::Groups', value => { parent_id => $root2->id, branchcode => $library3->branchcode } } ); my $group2_2 = $builder->build_object( { class => 'Koha::Library::Groups', value => { parent_id => $root2->id, branchcode => $library4->branchcode } } ); - my $biblioitem = $builder->build( { source => 'Biblioitem' } ); - - my $item1 = Koha::Item->new({ - biblionumber => $biblioitem->{biblionumber}, - biblioitemnumber => $biblioitem->{biblioitemnumber}, - homebranch => $library1->branchcode, - holdingbranch => $library2->branchcode, - barcode => '1', - itype => 'test', - })->store; - - my $item3 = Koha::Item->new({ - biblionumber => $biblioitem->{biblionumber}, - biblioitemnumber => $biblioitem->{biblioitemnumber}, - homebranch => $library3->branchcode, - holdingbranch => $library4->branchcode, - barcode => '3', - itype => 'test', - })->store; + our @branchcodes = ( + $library1->branchcode, $library2->branchcode, + $library3->branchcode, $library4->branchcode + ); + + my $item1 = $builder->build_sample_item( + { + homebranch => $library1->branchcode, + holdingbranch => $library2->branchcode, + copynumber => 1, + ccode => 'Gollum' + } + )->store; + + my $item3 = $builder->build_sample_item( + { + homebranch => $library3->branchcode, + holdingbranch => $library4->branchcode, + copynumber => 3, + itype => $item1->itype, + } + )->store; + + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => $item1->itype, + branchcode => undef, + rules => { + reservesallowed => 25, + } + } + ); + my $patron1 = $builder->build_object( { class => 'Koha::Patrons', value => { branchcode => $library1->branchcode, firstname => '1' } } ); my $patron4 = $builder->build_object( { class => 'Koha::Patrons', value => { branchcode => $library4->branchcode, firstname => '4' } } ); + my $all_count = Koha::Libraries->search({ pickup_location => 1})->count(); + my $results = { - "1-1-1-any" => 3, - "1-1-1-holdgroup" => 2, - "1-1-1-patrongroup" => 2, - "1-1-1-homebranch" => 1, + "1-1-1-any" => $all_count, + "1-1-1-holdgroup" => 2, + "1-1-1-patrongroup" => 2, + "1-1-1-homebranch" => 1, "1-1-1-holdingbranch" => 1, - "1-1-2-any" => 3, - "1-1-2-holdgroup" => 2, - "1-1-2-patrongroup" => 2, - "1-1-2-homebranch" => 1, + "1-1-2-any" => $all_count, + "1-1-2-holdgroup" => 2, + "1-1-2-patrongroup" => 2, + "1-1-2-homebranch" => 1, "1-1-2-holdingbranch" => 1, - "1-1-3-any" => 3, - "1-1-3-holdgroup" => 2, - "1-1-3-patrongroup" => 2, - "1-1-3-homebranch" => 1, + "1-1-3-any" => $all_count, + "1-1-3-holdgroup" => 2, + "1-1-3-patrongroup" => 2, + "1-1-3-homebranch" => 1, "1-1-3-holdingbranch" => 1, - "1-4-1-any" => 0, - "1-4-1-holdgroup" => 0, - "1-4-1-patrongroup" => 0, - "1-4-1-homebranch" => 0, + "1-4-1-any" => 0, + "1-4-1-holdgroup" => 0, + "1-4-1-patrongroup" => 0, + "1-4-1-homebranch" => 0, "1-4-1-holdingbranch" => 0, - "1-4-2-any" => 3, - "1-4-2-holdgroup" => 2, - "1-4-2-patrongroup" => 1, - "1-4-2-homebranch" => 1, + "1-4-2-any" => $all_count, + "1-4-2-holdgroup" => 2, + "1-4-2-patrongroup" => 1, + "1-4-2-homebranch" => 1, "1-4-2-holdingbranch" => 1, - "1-4-3-any" => 0, - "1-4-3-holdgroup" => 0, - "1-4-3-patrongroup" => 0, - "1-4-3-homebranch" => 0, + "1-4-3-any" => 0, + "1-4-3-holdgroup" => 0, + "1-4-3-patrongroup" => 0, + "1-4-3-homebranch" => 0, "1-4-3-holdingbranch" => 0, - "3-1-1-any" => 0, - "3-1-1-holdgroup" => 0, - "3-1-1-patrongroup" => 0, - "3-1-1-homebranch" => 0, + "3-1-1-any" => 0, + "3-1-1-holdgroup" => 0, + "3-1-1-patrongroup" => 0, + "3-1-1-homebranch" => 0, "3-1-1-holdingbranch" => 0, - "3-1-2-any" => 3, - "3-1-2-holdgroup" => 1, - "3-1-2-patrongroup" => 2, - "3-1-2-homebranch" => 0, + "3-1-2-any" => $all_count, + "3-1-2-holdgroup" => 1, + "3-1-2-patrongroup" => 2, + "3-1-2-homebranch" => 0, "3-1-2-holdingbranch" => 1, - "3-1-3-any" => 0, - "3-1-3-holdgroup" => 0, - "3-1-3-patrongroup" => 0, - "3-1-3-homebranch" => 0, + "3-1-3-any" => 0, + "3-1-3-holdgroup" => 0, + "3-1-3-patrongroup" => 0, + "3-1-3-homebranch" => 0, "3-1-3-holdingbranch" => 0, - "3-4-1-any" => 0, - "3-4-1-holdgroup" => 0, - "3-4-1-patrongroup" => 0, - "3-4-1-homebranch" => 0, + "3-4-1-any" => 0, + "3-4-1-holdgroup" => 0, + "3-4-1-patrongroup" => 0, + "3-4-1-homebranch" => 0, "3-4-1-holdingbranch" => 0, - "3-4-2-any" => 3, - "3-4-2-holdgroup" => 1, - "3-4-2-patrongroup" => 1, - "3-4-2-homebranch" => 0, + "3-4-2-any" => $all_count, + "3-4-2-holdgroup" => 1, + "3-4-2-patrongroup" => 1, + "3-4-2-homebranch" => 0, "3-4-2-holdingbranch" => 1, - "3-4-3-any" => 3, - "3-4-3-holdgroup" => 1, - "3-4-3-patrongroup" => 1, - "3-4-3-homebranch" => 0, + "3-4-3-any" => $all_count, + "3-4-3-holdgroup" => 1, + "3-4-3-patrongroup" => 1, + "3-4-3-homebranch" => 0, "3-4-3-holdingbranch" => 1 }; @@ -292,21 +291,23 @@ subtest 'pickup_locations' => sub { } } ); - my @pl = @{ $item->pickup_locations( { patron => $patron} ) }; + my @pl = $item->pickup_locations( { patron => $patron} )->as_list; my $ha_value=$ha==3?'holdgroup':($ha==2?'any':'homebranch'); foreach my $pickup_location (@pl) { + next + unless grep { $pickup_location eq $_ } @branchcodes; is( ref($pickup_location), 'Koha::Library', 'Object type is correct' ); } ok( scalar(@pl) == $results->{ - $item->barcode . '-' + $item->copynumber . '-' . $patron->firstname . '-' . $ha . '-' . $hfp }, 'item' - . $item->barcode + . $item->copynumber . ', patron' . $patron->firstname . ', holdallowed: ' @@ -315,12 +316,12 @@ subtest 'pickup_locations' => sub { . $hfp . ' should return ' . $results->{ - $item->barcode . '-' + $item->copynumber . '-' . $patron->firstname . '-' . $ha . '-' . $hfp } - . ' but returns ' + . ' and returns ' . scalar(@pl) ); @@ -338,6 +339,98 @@ subtest 'pickup_locations' => sub { } } + # Now test that branchtransferlimits will further filter the pickup locations + + my $item_no_ccode = $builder->build_sample_item( + { + homebranch => $library1->branchcode, + holdingbranch => $library2->branchcode, + itype => $item1->itype, + } + )->store; + + t::lib::Mocks::mock_preference('UseBranchTransferLimits', 1); + t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'itemtype'); + Koha::CirculationRules->set_rules( + { + branchcode => undef, + itemtype => $item1->itype, + rules => { + holdallowed => 1, + hold_fulfillment_policy => 1, + returnbranch => 'any' + } + } + ); + $builder->build_object( + { + class => 'Koha::Item::Transfer::Limits', + value => { + toBranch => $library1->branchcode, + fromBranch => $library2->branchcode, + itemtype => $item1->itype, + ccode => undef, + } + } + ); + + my $pickup_locations = $item1->pickup_locations( { patron => $patron1 } )->as_list; + is( scalar @$pickup_locations, $all_count - 1, "With a transfer limits we get back the libraries that are pickup locations minus 1 limited library"); + + $builder->build_object( + { + class => 'Koha::Item::Transfer::Limits', + value => { + toBranch => $library4->branchcode, + fromBranch => $library2->branchcode, + itemtype => $item1->itype, + ccode => undef, + } + } + ); + + $pickup_locations = $item1->pickup_locations( { patron => $patron1 } )->as_list; + is( scalar @$pickup_locations, $all_count - 2, "With 2 transfer limits we get back the libraries that are pickup locations minus 2 limited libraries"); + + t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'ccode'); + $pickup_locations = $item1->pickup_locations( { patron => $patron1 } )->as_list; + is( scalar @$pickup_locations, $all_count, "With no transfer limits of type ccode we get back the libraries that are pickup locations"); + + $pickup_locations = $item_no_ccode->pickup_locations( { patron => $patron1 } )->as_list; + is( scalar @$pickup_locations, $all_count, "With no transfer limits of type ccode and an item with no ccode we get back the libraries that are pickup locations"); + + $builder->build_object( + { + class => 'Koha::Item::Transfer::Limits', + value => { + toBranch => $library2->branchcode, + fromBranch => $library2->branchcode, + itemtype => undef, + ccode => $item1->ccode, + } + } + ); + + $pickup_locations = $item1->pickup_locations( { patron => $patron1 } )->as_list; + is( scalar @$pickup_locations, $all_count - 1, "With a transfer limits we get back the libraries that are pickup locations minus 1 limited library"); + + $builder->build_object( + { + class => 'Koha::Item::Transfer::Limits', + value => { + toBranch => $library4->branchcode, + fromBranch => $library2->branchcode, + itemtype => undef, + ccode => $item1->ccode, + } + } + ); + + $pickup_locations = $item1->pickup_locations( { patron => $patron1 } )->as_list; + is( scalar @$pickup_locations, $all_count - 2, "With 2 transfer limits we get back the libraries that are pickup locations minus 2 limited libraries"); + + t::lib::Mocks::mock_preference('UseBranchTransferLimits', 0); + $schema->storage->txn_rollback; }; diff --git a/t/db_dependent/Template/Plugin/Branches.t b/t/db_dependent/Template/Plugin/Branches.t index f6141d9345..4c149dfc69 100644 --- a/t/db_dependent/Template/Plugin/Branches.t +++ b/t/db_dependent/Template/Plugin/Branches.t @@ -137,7 +137,8 @@ subtest 'pickup_locations() tests' => sub { $item_class->mock( 'pickup_locations', sub { - return [$library_1]; + return Koha::Libraries->search( + { branchcode => $library_1->branchcode } ); } ); -- 2.11.0