From 904d9d54c9e976d41172bc2387eebf0c73e71a40 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 19 Sep 2017 10:29:25 -0300 Subject: [PATCH] Bug 18072: Only accept Koha::Library in parameters I do not think we should allowed branchode and Koha::Library objects, it adds confusion in callers (we never know if we have a branchcode of a library object). Signed-off-by: Kyle M Hall Signed-off-by: Nick Clemens --- Koha/Biblio.pm | 30 +++++---------------------- Koha/Item.pm | 32 ++++++---------------------- t/db_dependent/Koha/Biblios.t | 39 ++++++++++++++--------------------- t/db_dependent/Koha/Items.t | 31 ++++++++++------------------ 4 files changed, 37 insertions(+), 95 deletions(-) diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm index 2f9d95235c..1dca4d9cf6 100644 --- a/Koha/Biblio.pm +++ b/Koha/Biblio.pm @@ -37,8 +37,6 @@ use Koha::Item::Transfer::Limits; use Koha::Libraries; use Koha::Subscriptions; -use Koha::Exceptions::Library; - =head1 NAME Koha::Biblio - Koha Biblio Object class @@ -119,10 +117,10 @@ iterating each item of a biblio with Koha::Item->can_be_transferred(). Takes HASHref that can have the following parameters: MANDATORY PARAMETERS: - $to : Koha::Library or branchcode string + $to : Koha::Library OPTIONAL PARAMETERS: - $from : Koha::Library or branchcode string # if given, only items from that - # holdingbranch are considered + $from : Koha::Library # if given, only items from that + # holdingbranch are considered Returns 1 if at least one of the item of a biblio can be transferred to $to_library, otherwise 0. @@ -132,26 +130,8 @@ to $to_library, otherwise 0. sub can_be_transferred { my ($self, $params) = @_; - my $to = $params->{'to'}; - my $from = $params->{'from'}; - if (ref($to) ne 'Koha::Library') { - my $tobranchcode = defined $to ? $to : ''; - $to = Koha::Libraries->find($tobranchcode); - unless ($to) { - Koha::Exceptions::Library::NotFound->throw( - error => "Library '$tobranchcode' not found.", - ); - } - } - if ($from && ref($from) ne 'Koha::Library') { - my $frombranchcode = defined $from ? $from : ''; - $from = Koha::Libraries->find($frombranchcode); - unless ($from) { - Koha::Exceptions::Library::NotFound->throw( - error => "Library '$frombranchcode' not found.", - ); - } - } + my $to = $params->{to}; + my $from = $params->{from}; return 1 unless C4::Context->preference('UseBranchTransferLimits'); my $limittype = C4::Context->preference('BranchTransferLimitsType'); diff --git a/Koha/Item.pm b/Koha/Item.pm index 37610e3c32..964359ecf8 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -32,8 +32,6 @@ use Koha::Item::Transfers; use Koha::Patrons; use Koha::Libraries; -use Koha::Exceptions::Library; - use base qw(Koha::Object); =head1 NAME @@ -204,10 +202,10 @@ BranchTransferLimitsType to use either an itemnumber or ccode as an identifier Takes HASHref that can have the following parameters: MANDATORY PARAMETERS: - $to : Koha::Library or branchcode string + $to : Koha::Library OPTIONAL PARAMETERS: - $from : Koha::Library or branchcode string # if not given, item holdingbranch - # will be used instead + $from : Koha::Library # if not given, item holdingbranch + # will be used instead Returns 1 if item can be transferred to $to_library, otherwise 0. @@ -220,28 +218,10 @@ multiple items of the same biblio. sub can_be_transferred { my ($self, $params) = @_; - my $to = $params->{'to'}; - my $from = $params->{'from'}; - if (ref($to) ne 'Koha::Library') { - my $tobranchcode = defined $to ? $to : ''; - $to = Koha::Libraries->find($tobranchcode); - unless ($to) { - Koha::Exceptions::Library::NotFound->throw( - error => "Library '$tobranchcode' not found.", - ); - } - } - if (defined $from && ref($from) ne 'Koha::Library') { - my $frombranchcode = defined $from ? $from : ''; - $from = Koha::Libraries->find($frombranchcode); - unless ($from) { - Koha::Exceptions::Library::NotFound->throw( - error => "Library '$frombranchcode' not found.", - ); - } - } + my $to = $params->{to}; + my $from = $params->{from}; - $to = $to->branchcode; + $to = $to->branchcode; $from = defined $from ? $from->branchcode : $self->holdingbranch; return 1 if $from eq $to; # Transfer to current branch is allowed diff --git a/t/db_dependent/Koha/Biblios.t b/t/db_dependent/Koha/Biblios.t index 61f74331c1..17ab937e51 100644 --- a/t/db_dependent/Koha/Biblios.t +++ b/t/db_dependent/Koha/Biblios.t @@ -135,35 +135,35 @@ subtest 'waiting_or_in_transit' => sub { }; subtest 'can_be_transferred' => sub { - plan tests => 11; + plan tests => 8; t::lib::Mocks::mock_preference('UseBranchTransferLimits', 1); t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'itemtype'); - my $library1 = $builder->build( { source => 'Branch' } )->{branchcode}; - my $library2 = $builder->build( { source => 'Branch' } )->{branchcode}; - my $library3 = $builder->build( { source => 'Branch' } )->{branchcode}; + my $library1 = $builder->build_object( { class => 'Koha::Libraries' } ); + my $library2 = $builder->build_object( { class => 'Koha::Libraries' } ); + my $library3 = $builder->build_object( { class => 'Koha::Libraries' } ); my ($bibnum, $title, $bibitemnum) = create_helper_biblio('ONLY1'); my ($item_bibnum, $item_bibitemnum, $itemnumber) - = AddItem({ homebranch => $library1, holdingbranch => $library1 }, $bibnum); + = AddItem({ homebranch => $library1->branchcode, holdingbranch => $library1->branchcode }, $bibnum); my $item = Koha::Items->find($itemnumber); my $biblio = Koha::Biblios->find($bibnum); is(Koha::Item::Transfer::Limits->search({ - fromBranch => $library1, - toBranch => $library2, + fromBranch => $library1->branchcode, + toBranch => $library2->branchcode, })->count, 0, 'There are no transfer limits between libraries.'); ok($biblio->can_be_transferred({ to => $library2 }), 'Some items of this biblio can be transferred between libraries.'); my $limit = Koha::Item::Transfer::Limit->new({ - fromBranch => $library1, - toBranch => $library2, + fromBranch => $library1->branchcode, + toBranch => $library2->branchcode, itemtype => $item->effective_itemtype, })->store; is(Koha::Item::Transfer::Limits->search({ - fromBranch => $library1, - toBranch => $library2, + fromBranch => $library1->branchcode, + toBranch => $library2->branchcode, })->count, 1, 'Given we have added a transfer limit that applies for all ' .'of this biblio\s items,'); is($biblio->can_be_transferred({ to => $library2 }), 0, @@ -171,29 +171,20 @@ subtest 'can_be_transferred' => sub { .'libraries.'); is($biblio->can_be_transferred({ to => $library2, from => $library1 }), 0, 'We get the same result also if we pass the from-library parameter.'); - $item->holdingbranch($library2)->store; + $item->holdingbranch($library2->branchcode)->store; is($biblio->can_be_transferred({ to => $library2 }), 1, 'Given one of the ' .'items is already located at to-library, then the transfer is possible.'); - $item->holdingbranch($library1)->store; + $item->holdingbranch($library1->branchcode)->store; my ($item_bibnum2, $item_bibitemnum2, $itemnumber2) - = AddItem({ homebranch => $library1, holdingbranch => $library3 }, $bibnum); + = AddItem({ homebranch => $library1->branchcode, holdingbranch => $library3->branchcode }, $bibnum); my $item2 = Koha::Items->find($itemnumber2); is($biblio->can_be_transferred({ to => $library2 }), 1, 'Given we added ' .'another item that should have no transfer limits applying on, then ' .'the transfer is possible.'); - $item2->holdingbranch($library1)->store; + $item2->holdingbranch($library1->branchcode)->store; is($biblio->can_be_transferred({ to => $library2 }), 0, 'Given all of items' .' of the biblio are from same, transfer limited library, then transfer' .' is not possible.'); - throws_ok { $biblio->can_be_transferred({ to => undef }); } - 'Koha::Exceptions::Library::NotFound', - 'Exception thrown when no library given.'; - throws_ok { $biblio->can_be_transferred({ to => 'heaven' }); } - 'Koha::Exceptions::Library::NotFound', - 'Exception thrown when invalid library is given.'; - throws_ok { $biblio->can_be_transferred({ to => $library2, from => 'hell' }); } - 'Koha::Exceptions::Library::NotFound', - 'Exception thrown when invalid library is given.'; }; $schema->storage->txn_rollback; diff --git a/t/db_dependent/Koha/Items.t b/t/db_dependent/Koha/Items.t index 3d997b2f86..87ebfb3718 100644 --- a/t/db_dependent/Koha/Items.t +++ b/t/db_dependent/Koha/Items.t @@ -122,52 +122,43 @@ subtest 'checkout' => sub { }; subtest 'can_be_transferred' => sub { - plan tests => 8; + plan tests => 5; t::lib::Mocks::mock_preference('UseBranchTransferLimits', 1); t::lib::Mocks::mock_preference('BranchTransferLimitsType', 'itemtype'); - my $library1 = $builder->build( { source => 'Branch' } )->{branchcode}; - my $library2 = $builder->build( { source => 'Branch' } )->{branchcode}; + my $library1 = $builder->build_object( { class => 'Koha::Libraries' } ); + my $library2 = $builder->build_object( { class => 'Koha::Libraries' } ); my $item = Koha::Item->new({ biblionumber => $biblioitem->{biblionumber}, biblioitemnumber => $biblioitem->{biblioitemnumber}, - homebranch => $library1, - holdingbranch => $library1, + homebranch => $library1->branchcode, + holdingbranch => $library1->branchcode, itype => 'test', barcode => "newbarcode", })->store; $nb_of_items++; is(Koha::Item::Transfer::Limits->search({ - fromBranch => $library1, - toBranch => $library2, + fromBranch => $library1->branchcode, + toBranch => $library2->branchcode, })->count, 0, 'There are no transfer limits between libraries.'); ok($item->can_be_transferred({ to => $library2 }), 'Item can be transferred between libraries.'); my $limit = Koha::Item::Transfer::Limit->new({ - fromBranch => $library1, - toBranch => $library2, + fromBranch => $library1->branchcode, + toBranch => $library2->branchcode, itemtype => $item->effective_itemtype, })->store; is(Koha::Item::Transfer::Limits->search({ - fromBranch => $library1, - toBranch => $library2, + fromBranch => $library1->branchcode, + toBranch => $library2->branchcode, })->count, 1, 'Given we have added a transfer limit,'); is($item->can_be_transferred({ to => $library2 }), 0, 'Item can no longer be transferred between libraries.'); is($item->can_be_transferred({ to => $library2, from => $library1 }), 0, 'We get the same result also if we pass the from-library parameter.'); - throws_ok { $item->can_be_transferred({ to => undef }); } - 'Koha::Exceptions::Library::NotFound', - 'Exception thrown when no library given.'; - throws_ok { $item->can_be_transferred({ to => 'heaven' }); } - 'Koha::Exceptions::Library::NotFound', - 'Exception thrown when invalid library is given.'; - throws_ok { $item->can_be_transferred({ to => $library2, from => 'hell' }); } - 'Koha::Exceptions::Library::NotFound', - 'Exception thrown when invalid library is given.'; }; $retrieved_item_1->delete; -- 2.39.5