From c56b11f67a50c2cf34802a6eb0696a3188540298 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Fri, 8 May 2020 16:01:58 -0300 Subject: [PATCH] Bug 24368: Remove Koha::Libraries->pickup_locations This patch removes the unused method, and cleans the tests. To test: 1. Verify Koha::Libraries->pickup_locations is not used in the code: $ git grep 'Koha::Libraries\->pickup_locations' => SUCCESS: no uses 2. Apply this patch 3. Run: $ kshell k$ prove t/db_dependent/Koha/Libraries.t => SUCCESS: Tests pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi Signed-off-by: Jonathan Druart --- Koha/Libraries.pm | 72 +------ t/db_dependent/Koha/Libraries.t | 335 +------------------------------- 2 files changed, 2 insertions(+), 405 deletions(-) diff --git a/Koha/Libraries.pm b/Koha/Libraries.pm index 27e4a232ed..8a45cc26ad 100644 --- a/Koha/Libraries.pm +++ b/Koha/Libraries.pm @@ -38,77 +38,7 @@ Koha::Libraries - Koha Library Object set class =head1 API -=head2 Class Methods - -=cut - -=head3 pickup_locations - -Returns available pickup locations (Koha::Library objects) for - A. a specific item - B. a biblio - C. none of the above, simply all libraries with pickup_location => 1 - -This method determines the pickup location by two factors: - 1. is the library configured as pickup location - 2. can a specific item / at least one of the items of a biblio be transferred - into the library - -OPTIONAL PARAMETERS: - item # Koha::Item object / itemnumber, find pickup locations for item - biblio # Koha::Biblio object / biblionumber, find pickup locations for biblio - -If no parameters are given, all libraries with pickup_location => 1 are returned. - -=cut - -sub pickup_locations { - my ($self, $params) = @_; - - my $item = $params->{'item'}; - my $biblio = $params->{'biblio'}; - if ($biblio && $item) { - Koha::Exceptions::BadParameter->throw( - error => "Koha::Libraries->pickup_locations takes either 'biblio' or " - ." 'item' as parameter, but not both." - ); - } - - # Select libraries that are configured as pickup locations - my $libraries = $self->search({ - pickup_location => 1 - }, { - order_by => ['branchname'] - }); - - return $libraries->unblessed unless $item or $biblio; - return $libraries->unblessed - unless C4::Context->preference('UseBranchTransferLimits'); - my $limittype = C4::Context->preference('BranchTransferLimitsType'); - - if ($item) { - unless (ref($item) eq 'Koha::Item') { - $item = Koha::Items->find($item); - return $libraries->unblessed unless $item; - } - } else { - unless (ref($biblio) eq 'Koha::Biblio') { - $biblio = Koha::Biblios->find($biblio); - return $libraries->unblessed unless $biblio; - } - } - - my @pickup_locations; - foreach my $library ($libraries->as_list) { - if ($item && $item->can_be_transferred({ to => $library })) { - push @pickup_locations, $library->unblessed; - } elsif ($biblio && $biblio->can_be_transferred({ to => $library })) { - push @pickup_locations, $library->unblessed; - } - } - - return wantarray ? @pickup_locations : \@pickup_locations; -} +=head2 Class methods =head3 search_filtered diff --git a/t/db_dependent/Koha/Libraries.t b/t/db_dependent/Koha/Libraries.t index 5b44ea552d..ff9f6a9f0a 100644 --- a/t/db_dependent/Koha/Libraries.t +++ b/t/db_dependent/Koha/Libraries.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 10; +use Test::More tests => 9; use C4::Biblio; use C4::Context; @@ -88,339 +88,6 @@ is( $srstages->count, 3, 'Correctly fetched stockrotationstages associated with isa_ok( $srstages->next, 'Koha::StockRotationStage', "Relationship correctly creates Koha::Objects." ); -subtest 'pickup_locations' => sub { - plan tests => 2; - - Koha::CirculationRules->set_rules( - { - branchcode => undef, - itemtype => undef, - rules => { - holdallowed => 2, - hold_fulfillment_policy => 'any', - returnbranch => 'any' - } - } - ); - - my $from = Koha::Library->new({ - branchcode => 'zzzfrom', - branchname => 'zzzfrom', - branchnotes => 'zzzfrom', - })->store; - my $to = Koha::Library->new({ - branchcode => 'zzzto', - branchname => 'zzzto', - branchnotes => 'zzzto', - })->store; - - - my $biblio = $builder->build_sample_biblio({ itemtype => 'DUMMY' }); - my $itemtype = $biblio->itemtype; - my $item_info = { - biblionumber => $biblio->biblionumber, - library => $from->branchcode, - itype => $itemtype - }; - my $item1 = $builder->build_sample_item({%$item_info}); - my $item2 = $builder->build_sample_item({%$item_info}); - my $item3 = $builder->build_sample_item({%$item_info}); - my $patron1 = $builder->build_object( { class => 'Koha::Patrons', value => { branchcode => $from->branchcode } } ); - - subtest 'UseBranchTransferLimits = OFF' => sub { - plan tests => 5; - - t::lib::Mocks::mock_preference('UseBranchTransferLimits', 0); - t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'itemtype'); - t::lib::Mocks::mock_preference('item-level_itypes', 1); - Koha::Item::Transfer::Limits->delete; - Koha::Item::Transfer::Limit->new({ - fromBranch => $from->branchcode, - toBranch => $to->branchcode, - itemtype => $biblio->itemtype, - })->store; - my $total_pickup = Koha::Libraries->search({ - pickup_location => 1 - })->count; - my $pickup = Koha::Libraries->pickup_locations({ biblio => $biblio->biblionumber, patron => $patron1 }); - is(C4::Context->preference('UseBranchTransferLimits'), 0, 'Given system ' - .'preference UseBranchTransferLimits is switched OFF,'); - is(@{$pickup}, $total_pickup, 'Then the total number of pickup locations ' - .'equal number of libraries with pickup_location => 1'); - - t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'itemtype'); - t::lib::Mocks::mock_preference('item-level_itypes', 1); - $pickup = Koha::Libraries->pickup_locations({ biblio => $biblio->biblionumber, patron => $patron1 }); - is(@{$pickup}, $total_pickup, '...when ' - .'BranchTransferLimitsType = itemtype and item-level_itypes = 1'); - t::lib::Mocks::mock_preference('item-level_itypes', 0); - $pickup = Koha::Libraries->pickup_locations({ biblio => $biblio->biblionumber, patron => $patron1 }); - is(@{$pickup}, $total_pickup, '...as well as when ' - .'BranchTransferLimitsType = itemtype and item-level_itypes = 0'); - t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'ccode'); - $pickup = Koha::Libraries->pickup_locations({ biblio => $biblio->biblionumber, patron => $patron1 }); - is(@{$pickup}, $total_pickup, '...as well as when ' - .'BranchTransferLimitsType = ccode'); - t::lib::Mocks::mock_preference('item-level_itypes', 1); - }; - - subtest 'UseBranchTransferLimits = ON' => sub { - plan tests => 4; - t::lib::Mocks::mock_preference('UseBranchTransferLimits', 1); - - is(C4::Context->preference('UseBranchTransferLimits'), 1, 'Given system ' - .'preference UseBranchTransferLimits is switched ON,'); - - subtest 'Given BranchTransferLimitsType = itemtype and ' - .'item-level_itypes = ON' => sub { - plan tests => 11; - - t::lib::Mocks::mock_preference('BranchTransferLimitsType','itemtype'); - t::lib::Mocks::mock_preference('item-level_itypes', 1); - Koha::Item::Transfer::Limits->delete; - my $limit = Koha::Item::Transfer::Limit->new({ - fromBranch => $from->branchcode, - toBranch => $to->branchcode, - itemtype => $item1->effective_itemtype, - })->store; - ok($item1->effective_itemtype eq $item2->effective_itemtype - && $item1->effective_itemtype eq $item3->effective_itemtype, - 'Given all items of a biblio have same the itemtype,'); - is($limit->itemtype, $item1->effective_itemtype, 'and given there ' - .'is an existing transfer limit for that itemtype,'); - my $pickup = Koha::Libraries->pickup_locations({ biblio => $biblio->biblionumber, patron => $patron1 }); - my $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 0, 'Then the to-library of which the limit applies for, ' - .'is not included in the list of pickup libraries.'); - $pickup = Koha::Libraries->pickup_locations({ item => $item1, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 0, 'The same applies when asking pickup locations of ' - .'a single item.'); - my $others = Koha::Libraries->search({ - pickup_location => 1, - branchcode => { 'not in' => [$limit->toBranch] }})->count; - is(@{$pickup}, $others, 'However, the number of other pickup ' - .'libraries is correct.'); - $item2->itype('BK')->store; - ok($item1->effective_itemtype ne $item2->effective_itemtype, - 'Given one of the item in this biblio has a different itemtype,'); - is(Koha::Item::Transfer::Limits->search({ - itemtype => $item2->effective_itemtype })->count, 0, 'and it is' - .' not restricted by transfer limits,'); - $pickup = Koha::Libraries->pickup_locations({ biblio => $biblio->biblionumber, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 1, 'Then the to-library of which the limit applies for, ' - .'is included in the list of pickup libraries.'); - $pickup = Koha::Libraries->pickup_locations({ item => $item2, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 1, 'The same applies when asking pickup locations of ' - .'a that particular item.'); - Koha::Item::Transfer::Limits->delete; - $pickup = Koha::Libraries->pickup_locations({ biblio => $biblio->biblionumber, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 1, 'Given we deleted transfer limit, the previously ' - .'transfer-limited library is included in the list.'); - $pickup = Koha::Libraries->pickup_locations({ item => $item1, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 1, 'The same applies when asking pickup locations of ' - .'a single item.'); - }; - - subtest 'Given BranchTransferLimitsType = itemtype and ' - .'item-level_itypes = OFF' => sub { - plan tests => 9; - - t::lib::Mocks::mock_preference('BranchTransferLimitsType','itemtype'); - t::lib::Mocks::mock_preference('item-level_itypes', 0); - $biblio->biblioitem->itemtype('BK')->store; - Koha::Item::Transfer::Limits->delete; - my $limit = Koha::Item::Transfer::Limit->new({ - fromBranch => $from->branchcode, - toBranch => $to->branchcode, - itemtype => $item1->effective_itemtype, - })->store; - - ok($item1->effective_itemtype eq 'BK', - 'Given items use biblio-level itemtype,'); - is($limit->itemtype, $item1->effective_itemtype, 'and given there ' - .'is an existing transfer limit for that itemtype,'); - my $pickup = Koha::Libraries->pickup_locations({ biblio => $biblio->biblionumber, patron => $patron1 }); - my $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 0, 'Then the to-library of which the limit applies for, ' - .'is not included in the list of pickup libraries.'); - $pickup = Koha::Libraries->pickup_locations({ item => $item1, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 0, 'The same applies when asking pickup locations of ' - .'a single item.'); - my $others = Koha::Libraries->search({ - pickup_location => 1, - branchcode => { 'not in' => [$limit->toBranch] }})->count; - is(@{$pickup}, $others, 'However, the number of other pickup ' - .'libraries is correct.'); - Koha::Item::Transfer::Limits->delete; - $pickup = Koha::Libraries->pickup_locations({ biblio => $biblio->biblionumber, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 1, 'Given we deleted transfer limit, the previously ' - .'transfer-limited library is included in the list.'); - $limit = Koha::Item::Transfer::Limit->new({ - fromBranch => $from->branchcode, - toBranch => $to->branchcode, - itemtype => $item1->itype, - })->store; - ok($item1->itype ne $item1->effective_itemtype - && $limit->itemtype eq $item1->itype, 'Given we have added a limit' - .' matching ITEM-level itemtype,'); - $pickup = Koha::Libraries->pickup_locations({ biblio => $biblio->biblionumber, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 1, 'Then the limited branch is still included as a pickup' - .' library.'); - $pickup = Koha::Libraries->pickup_locations({ item => $item1, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 1, 'The same applies when asking pickup locations of ' - .'a single item.'); - }; - - subtest 'Given BranchTransferLimitsType = ccode' => sub { - plan tests => 10; - - t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'ccode'); - $item1->ccode('hi')->store; - $item2->ccode('hi')->store; - $item3->ccode('hi')->store; - Koha::Item::Transfer::Limits->delete; - my $limit = Koha::Item::Transfer::Limit->new({ - fromBranch => $from->branchcode, - toBranch => $to->branchcode, - ccode => $item1->ccode, - })->store; - - is($limit->ccode, $item1->ccode, 'Given there ' - .'is an existing transfer limit for that ccode,'); - my $pickup = Koha::Libraries->pickup_locations({ biblio => $biblio->biblionumber, patron => $patron1 }); - my $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 0, 'Then the to-library of which the limit applies for, ' - .'is not included in the list of pickup libraries.'); - $pickup = Koha::Libraries->pickup_locations({ item => $item1, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 0, 'The same applies when asking pickup locations of ' - .'a single item.'); - my $others = Koha::Libraries->search({ - pickup_location => 1, - branchcode => { 'not in' => [$limit->toBranch] }})->count; - is(@{$pickup}, $others, 'However, the number of other pickup ' - .'libraries is correct.'); - $item3->ccode('yo')->store; - ok($item1->ccode ne $item3->ccode, - 'Given one of the item in this biblio has a different ccode,'); - is(Koha::Item::Transfer::Limits->search({ - ccode => $item3->ccode })->count, 0, 'and it is' - .' not restricted by transfer limits,'); - $pickup = Koha::Libraries->pickup_locations({ biblio => $biblio->biblionumber, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 1, 'Then the to-library of which the limit applies for, ' - .'is included in the list of pickup libraries.'); - $pickup = Koha::Libraries->pickup_locations({ item => $item3, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 1, 'The same applies when asking pickup locations of ' - .'a that particular item.'); - Koha::Item::Transfer::Limits->delete; - $pickup = Koha::Libraries->pickup_locations({ biblio => $biblio->biblionumber, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 1, 'Given we deleted transfer limit, the previously ' - .'transfer-limited library is included in the list.'); - $pickup = Koha::Libraries->pickup_locations({ item => $item1, patron => $patron1 }); - $found = 0; - foreach my $lib (@{$pickup}) { - if ($lib->{branchcode} eq $limit->toBranch) { - $found = 1; - } - } - is($found, 1, 'The same applies when asking pickup locations of ' - .'a single item.'); - }; - }; -}; - $schema->storage->txn_rollback; subtest '->get_effective_marcorgcode' => sub { -- 2.20.1