Bugzilla – Attachment 104789 Details for
Bug 24368
Koha::Libraries->pickup_locations needs refactoring/ratifying
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 24368: Remove Koha::Libraries->pickup_locations
Bug-24368-Remove-KohaLibraries-pickuplocations.patch (text/plain), 19.60 KB, created by
Jonathan Druart
on 2020-05-12 14:33:06 UTC
(
hide
)
Description:
Bug 24368: Remove Koha::Libraries->pickup_locations
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2020-05-12 14:33:06 UTC
Size:
19.60 KB
patch
obsolete
>From c56b11f67a50c2cf34802a6eb0696a3188540298 Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >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 <tomascohen@theke.io> > >Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >--- > 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 24368
:
104615
|
104616
|
104789
|
104790
|
104830
|
104831