Bugzilla – Attachment 76288 Details for
Bug 18072
Add Koha objects for Branch Transfer Limits
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 18072: Only accept Koha::Library in parameters
Bug-18072-Only-accept-KohaLibrary-in-parameters.patch (text/plain), 12.05 KB, created by
Kyle M Hall (khall)
on 2018-06-22 13:52:43 UTC
(
hide
)
Description:
Bug 18072: Only accept Koha::Library in parameters
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2018-06-22 13:52:43 UTC
Size:
12.05 KB
patch
obsolete
>From 9355c5a39f22ea4499f950bdd3a467e6895040f9 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >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). >--- > 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.15.2 (Apple Git-101.1)
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=76288">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 18072
:
59979
|
59980
|
59981
|
59982
|
60009
|
60010
|
60011
|
60116
|
60117
|
60782
|
60783
|
60784
|
63683
|
63684
|
63685
|
63686
|
63687
|
66724
|
67049
|
67050
|
67051
|
67052
|
67053
|
67054
|
67209
|
76282
|
76283
|
76284
|
76285
|
76286
|
76287
|
76288
|
76292
|
76293
|
76294
|
76295
|
76296
|
76297
|
76298