Bugzilla – Attachment 21993 Details for
Bug 11005
Centralize the UseBranchTransferLimits-check under one function
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function
Bug-11005-ENH---Centralize-the-UseBranchTransferLi.patch (text/plain), 14.96 KB, created by
Olli-Antti Kivilahti
on 2013-10-15 09:11:16 UTC
(
hide
)
Description:
Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function
Filename:
MIME Type:
Creator:
Olli-Antti Kivilahti
Created:
2013-10-15 09:11:16 UTC
Size:
14.96 KB
patch
obsolete
>From f9164052d483fc8ec64e81554fdbc91b94926f3e Mon Sep 17 00:00:00 2001 >From: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> >Date: Tue, 15 Oct 2013 12:08:23 +0300 >Subject: [PATCH] Bug 11005 [ENH] - Centralize the > UseBranchTransferLimits-check under one function > >Created a C4::Circulation::CheckBranchTransferAllowed() as a more convenient branch transfer check >than the previous multiline if-else mess. This is a more pleasant and readable approach to generalizing >branch transfer checks accross Koha. >Includes a error message as the return value so the precise reasons for denial need not be constructed >when needed. > >Unit tests included which test the new C4 function. >--- > C4/Circulation.pm | 99 ++++++++++-- > circ/branchtransfers.pl | 3 +- > .../Circulation/CheckBranchTransferAllowed.t | 180 +++++++++++++++++++++ > 3 files changed, 270 insertions(+), 12 deletions(-) > create mode 100644 t/db_dependent/Circulation/CheckBranchTransferAllowed.t > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index d74b97f..fb390f1 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -104,6 +104,7 @@ BEGIN { > &GetTransfersFromTo > &updateWrongTransfer > &DeleteTransfer >+ &CheckBranchTransferAllowed > &IsBranchTransferAllowed > &CreateBranchTransferLimit > &DeleteBranchTransferLimits >@@ -299,7 +300,8 @@ sub transferbook { > my $messages; > my $dotransfer = 1; > my $branches = GetBranches(); >- my $itemnumber = GetItemnumberFromBarcode( $barcode ); >+ my $item = GetItem(undef,$barcode,undef); >+ my $itemnumber = $item->{itemnumber}; > my $issue = GetItemIssue($itemnumber); > my $biblio = GetBiblioFromItemNumber($itemnumber); > >@@ -314,16 +316,10 @@ sub transferbook { > my $fbr = $biblio->{'holdingbranch'}; > > # if using Branch Transfer Limits >- if ( C4::Context->preference("UseBranchTransferLimits") == 1 ) { >- if ( C4::Context->preference("item-level_itypes") && C4::Context->preference("BranchTransferLimitsType") eq 'itemtype' ) { >- if ( ! IsBranchTransferAllowed( $tbr, $fbr, $biblio->{'itype'} ) ) { >- $messages->{'NotAllowed'} = $tbr . "::" . $biblio->{'itype'}; >- $dotransfer = 0; >- } >- } elsif ( ! IsBranchTransferAllowed( $tbr, $fbr, $biblio->{ C4::Context->preference("BranchTransferLimitsType") } ) ) { >- $messages->{'NotAllowed'} = $tbr . "::" . $biblio->{ C4::Context->preference("BranchTransferLimitsType") }; >- $dotransfer = 0; >- } >+ my $errMsg; >+ ($dotransfer, $errMsg) = CheckBranchTransferAllowed( $tbr, $fbr, $item, $biblio ); >+ if ( ! $dotransfer ) { >+ $messages->{'NotAllowed'} = $errMsg; > } > > # if is permanent... >@@ -3200,6 +3196,87 @@ my $exist=$sth->fetchrow ; > return $exist; > } > >+ >+=head2 CheckBranchTransferAllowed >+ >+A convenience function to easily check item transfer limits. >+ >+ ($allowed, $errorMessage) = CheckBranchTransferAllowed( $toBranch, $fromBranch, $item, $biblioitem ); >+ ($allowed, $errorMessage) = CheckBranchTransferAllowed( $toBranch, undef, $item, undef] ); >+ >+Checks if UseBranchTransferLimits global preference is in use. >+Checks if the given item can be transferred from $fromBranch to $toBranch. >+This is dependant on the global setting: >+* UseBranchTransferLimits >+and "Administration" -> "Library transfer limits" >+ >+C<$toBranch> = the transfer destination library's code >+C<$fromBranch> = OPTIONAL, the transfer departure library's code >+ DEFAULT: using the item's holdingbranch >+C<$item> = item-object from items-table >+C<$biblioitem> = OPTIONAL, biblioitem-object, is needed when using CCODE instead if >+ using itemtype to limit branch transfers. >+ If biblioitem-object is available, it should be provided for performance reasons. >+C<returns> if transfer is not allowed: (0, "$fromBranch->$toBranch->transfer limit code") >+ (1) if transfer allowed. >+=cut >+ >+sub CheckBranchTransferAllowed { >+ >+ #When we check for BranchTransferLimit global settings centrally, it makes >+ # using this functionality much easier. >+ unless ( C4::Context->preference("UseBranchTransferLimits") == 1 ) { >+ return 1; >+ } >+ >+ #Init parameter variables >+ my ( $toBranch, $fromBranch, $item, $biblioitem ) = @_; >+ >+ #Check the $fromBranch and set the DEFAULT value >+ $fromBranch = $item->{holdingbranch} if ! defined $fromBranch; >+ >+ if ( $toBranch eq $fromBranch ) { return 1; } ## Short circuit for speed. >+ >+ #This is the CCode or itemtype value used to find the correct transfer rule for this item. >+ my $code; >+ >+ ## Figure out the $code! >+ ## First we need to figure out are we using CCODE or itemtype, this limits do we need $biblioitem or not. >+ ## Since $biblioitem can be optional, we cannot count that it is defined. >+ if ( C4::Context->preference("BranchTransferLimitsType") eq 'itemtype' ) { >+ if (C4::Context->preference("item-level_itypes") && defined $item->{'itype'}) { >+ $code = $item->{'itype'}; #Easiest way to get the $code is from the item. >+ } >+ elsif (defined $biblioitem->{itemtype}) { >+ $code = $biblioitem->{itemtype} >+ } >+ #If code cannot be resolved from $item or $biblioitem, we need to escalate to the DB >+ else { >+ $biblioitem = C4::Biblio::GetBiblioFromItemNumber( $item->{itemnumber} ); >+ $code = $biblioitem->{itemtype} >+ } >+ >+ } else { >+ my $limitType = C4::Context->preference("BranchTransferLimitsType"); >+ if (defined $biblioitem->{ $limitType }) { >+ $code = $biblioitem->{ $limitType }; >+ } >+ else { >+ #collection code (ccode) is not present in the Biblio, so no point looking there. >+ # If the Item doesn't have a itemtype, then give up and let the transfer pass. >+ } >+ } >+ ## Phew we finally got the $code, now it's time to rumble! >+ >+ if (! IsBranchTransferAllowed($toBranch, $fromBranch, $code)) { >+ return 0, "$fromBranch->$toBranch->$code"; >+ } >+ else { >+ return 1; >+ } >+} >+ >+ > =head2 IsBranchTransferAllowed > > $allowed = IsBranchTransferAllowed( $toBranch, $fromBranch, $code ); >diff --git a/circ/branchtransfers.pl b/circ/branchtransfers.pl >index 3be7bb4..2e8a77f 100755 >--- a/circ/branchtransfers.pl >+++ b/circ/branchtransfers.pl >@@ -207,9 +207,10 @@ foreach my $code ( keys %$messages ) { > } > elsif ( $code eq "NotAllowed" ) { > warn "NotAllowed: $messages->{'NotAllowed'} to " . $branches->{ $messages->{'NotAllowed'} }->{'branchname'}; >+ > # Do we really want a error log message here? --atz > $err{errnotallowed} = 1; >- my ( $tbr, $typecode ) = split( /::/, $messages->{'NotAllowed'} ); >+ my ( $fbr, $tbr, $typecode ) = split( '->', $messages->{'NotAllowed'} ); > $err{tbr} = $branches->{ $tbr }->{'branchname'}; > $err{code} = $typecode; > $err{codeType} = $codeTypeDescription; >diff --git a/t/db_dependent/Circulation/CheckBranchTransferAllowed.t b/t/db_dependent/Circulation/CheckBranchTransferAllowed.t >new file mode 100644 >index 0000000..92b8a23 >--- /dev/null >+++ b/t/db_dependent/Circulation/CheckBranchTransferAllowed.t >@@ -0,0 +1,180 @@ >+use Modern::Perl; >+use Test::More tests => 20; >+ >+use C4::Circulation; >+use C4::Context; >+use C4::Record; >+ >+my $dbh = C4::Context->dbh; >+$dbh->{AutoCommit} = 0; >+$dbh->{RaiseError} = 1; >+ >+my $originalBranchTransferLimitsType = C4::Context->preference('BranchTransferLimitsType'); >+ >+### Preparing our tests we want to run ### >+sub runTestsForCCodeOrItemtype { >+ my ($itemCPLFull, $itemCPLLite, $biblioitem) = @_; >+ >+ print 'Running tests for '.C4::Context->preference("BranchTransferLimitsType")."\n"; >+ >+ #howto use: CheckBranchTransferAllowed( $toBranch, $fromBranch, $item, $biblioitem] ); >+ my $result = C4::Circulation::CheckBranchTransferAllowed( 'CPL', 'IPT', $itemCPLFull, $biblioitem ); >+ is ( $result, 1, "Successful branch transfer, full parameters" ); >+ >+ $result = C4::Circulation::CheckBranchTransferAllowed( 'IPT', 'CPL', $itemCPLFull, $biblioitem ); >+ is ( $result, 0, "Failing branch transfer, full parameters" ); >+ >+ $result = C4::Circulation::CheckBranchTransferAllowed( 'CPL', 'IPT', $itemCPLFull, undef ); >+ is ( $result, 1, "Successful branch transfer, full parameters, no Biblio defined" ); >+ >+ $result = C4::Circulation::CheckBranchTransferAllowed( 'IPT', 'CPL', $itemCPLFull, undef ); >+ is ( $result, 0, "Failing branch transfer, full parameters, no Biblio defined" ); >+ >+ $result = C4::Circulation::CheckBranchTransferAllowed( 'FFL', undef, $itemCPLFull, $biblioitem ); >+ is ( $result, 1, "Successful branch transfer, using defaults for \$fromBranch" ); >+ >+ $result = C4::Circulation::CheckBranchTransferAllowed( 'IPT', undef, $itemCPLFull, $biblioitem ); >+ is ( $result, 0, "Failing branch transfer, using defaults for \$fromBranch" ); >+ >+ $result = C4::Circulation::CheckBranchTransferAllowed( 'FFL', undef, $itemCPLFull, undef ); >+ is ( $result, 1, "Successful branch transfer, using minimum parameters" ); >+ >+ $result = C4::Circulation::CheckBranchTransferAllowed( 'IPT', undef, $itemCPLFull, undef ); >+ is ( $result, 0, "Failing branch transfer, using minimum parameters" ); >+ >+ $result = C4::Circulation::CheckBranchTransferAllowed( 'FFL', undef, $itemCPLLite, undef ); >+ is ( $result, 1, "Successful branch transfer, using minimum parameters, code is pulled from Biblio" ); >+ >+ if (C4::Context->preference("BranchTransferLimitsType") eq 'ccode') { >+ $result = C4::Circulation::CheckBranchTransferAllowed( 'IPT', undef, $itemCPLLite, undef ); >+ is ( $result, 1, "Not failing branch transfer even if should, because CCODE cannot be found from the item and it is not a part of the biblio." ); >+ } >+ else { >+ $result = C4::Circulation::CheckBranchTransferAllowed( 'IPT', undef, $itemCPLLite, undef ); >+ is ( $result, 0, "Failing branch transfer, using minimum parameters, itemtype is pulled from Biblio" ); >+ } >+ >+ >+} >+### Tests prepared >+ >+### Preparing our generic testing data ### >+ >+#Set the item variables >+my $ccode = 'FANTASY'; >+my $itemtype = 'BK'; >+ >+## Add a example Bibliographic record >+my $bibFramework = ''; #Using the default bibliographic framework. >+my $marcxml=qq(<?xml version="1.0" encoding="UTF-8"?> >+<record format="MARC21" type="Bibliographic"> >+ <leader>00000cim a22000004a 4500</leader> >+ <controlfield tag="001">1001</controlfield> >+ <controlfield tag="005">2013-06-03 07:04:07+02</controlfield> >+ <controlfield tag="007">ss||||j|||||||</controlfield> >+ <controlfield tag="008"> uuuu xxk|||||||||||||||||eng|c</controlfield> >+ <datafield tag="020" ind1=" " ind2=" "> >+ <subfield code="a">0-00-103147-3</subfield> >+ <subfield code="c">14.46 EUR</subfield> >+ </datafield> >+ <datafield tag="041" ind1="0" ind2=" "> >+ <subfield code="d">eng</subfield> >+ </datafield> >+ <datafield tag="084" ind1=" " ind2=" "> >+ <subfield code="a">83.5</subfield> >+ <subfield code="2">ykl</subfield> >+ </datafield> >+ <datafield tag="100" ind1="1" ind2=" "> >+ <subfield code="a">SHAKESPEARE, WILLIAM.</subfield> >+ </datafield> >+ <datafield tag="245" ind1="1" ind2="4"> >+ <subfield code="a">THE TAMING OF THE SHREW /</subfield> >+ <subfield code="c">WILLIAM SHAKESPEARE</subfield> >+ <subfield code="h">[ÃÃNITE].</subfield> >+ </datafield> >+ <datafield tag="260" ind1=" " ind2=" "> >+ <subfield code="a">LONDON :</subfield> >+ <subfield code="b">COLLINS.</subfield> >+ </datafield> >+ <datafield tag="300" ind1=" " ind2=" "> >+ <subfield code="a">2 ÃÃNIKASETTIA.</subfield> >+ </datafield> >+ <datafield tag="852" ind1=" " ind2=" "> >+ <subfield code="a">FI-Jm</subfield> >+ <subfield code="h">83.5</subfield> >+ </datafield> >+ <datafield tag="852" ind1=" " ind2=" "> >+ <subfield code="a">FI-Konti</subfield> >+ <subfield code="h">83.5</subfield> >+ </datafield> >+</record> >+); >+my $record=C4::Record::marcxml2marc($marcxml); >+ >+# Add a itemtype definition to the Record. >+my ( $biblioitemtypeTagid, $biblioitemtypeSubfieldid ) = >+ C4::Biblio::GetMarcFromKohaField( 'biblioitems.itemtype', $bibFramework ); >+my $itemtypeField = MARC::Field->new($biblioitemtypeTagid, '', '', >+ $biblioitemtypeSubfieldid => $itemtype); >+$record->append_fields( $itemtypeField ); >+ >+my ( $newBiblionumber, $newBiblioitemnumber ) = C4::Biblio::AddBiblio( $record, $bibFramework, { defer_marc_save => 1 } ); >+ >+## Add an item with a ccode. >+my ($item_bibnum, $item_bibitemnum); >+my ($itemCPLFull, $itemCPLFullId); #Item with a itemtype and ccode in its data. >+my ($itemCPLLite, $itemCPLLiteId); #Item with no itemtype nor ccode in its data. Forces to look for it from the biblio. >+($item_bibnum, $item_bibitemnum, $itemCPLFullId) = C4::Items::AddItem({ barcode => 'CPLFull', homebranch => 'CPL', holdingbranch => 'CPL', ccode => $ccode, itemtype => $itemtype}, $newBiblionumber);#, biblioitemnumber => $newBiblioitemnumber, biblionumber => $newBiblioitemnumber }); >+($item_bibnum, $item_bibitemnum, $itemCPLLiteId) = C4::Items::AddItem({ barcode => 'CPLLite', homebranch => 'CPL', holdingbranch => 'CPL'}, $newBiblionumber);# biblioitemnumber => $newBiblioitemnumber, biblionumber => $newBiblioitemnumber }); >+ >+ >+### Created the generic testing material. ### >+### Setting preferences for ccode use-case ### >+ >+C4::Context->set_preference("BranchTransferLimitsType", 'ccode'); >+ >+## Add the TransferLimit rules: >+## IPT -> CPL -> FFL -> IPT >+# to from >+C4::Circulation::CreateBranchTransferLimit( 'IPT', 'CPL', $ccode ); >+C4::Circulation::CreateBranchTransferLimit( 'CPL', 'FFL', $ccode ); >+C4::Circulation::CreateBranchTransferLimit( 'FFL', 'IPT', $ccode ); >+ >+## Ready to start testing ccode use-case ## >+ >+$itemCPLFull = C4::Items::GetItem($itemCPLFullId); >+$itemCPLLite = C4::Items::GetItem($itemCPLLiteId); >+my $biblioitem = C4::Biblio::GetBiblioFromItemNumber( $itemCPLFull->{itemnumber} ); >+ >+ >+runTestsForCCodeOrItemtype($itemCPLFull, $itemCPLLite, $biblioitem); >+ >+ >+ >+### ccode tested >+### Setting preferences for itemtype use-case ### >+ >+C4::Context->set_preference("BranchTransferLimitsType", 'itemtype'); >+ >+## Add the TransferLimit rules: >+## IPT -> CPL -> FFL -> IPT >+# to from >+C4::Circulation::CreateBranchTransferLimit( 'IPT', 'CPL', $itemtype ); >+C4::Circulation::CreateBranchTransferLimit( 'CPL', 'FFL', $itemtype ); >+C4::Circulation::CreateBranchTransferLimit( 'FFL', 'IPT', $itemtype ); >+ >+## Ready to start testing itemtype use-case ## >+ >+$itemCPLFull = C4::Items::GetItem($itemCPLFullId); >+$itemCPLLite = C4::Items::GetItem($itemCPLLiteId); >+$biblioitem = C4::Biblio::GetBiblioFromItemNumber( $itemCPLFull->{itemnumber} ); >+ >+ >+runTestsForCCodeOrItemtype($itemCPLFull, $itemCPLLite, $biblioitem); >+ >+### itemtype tested >+ >+### Reset default preferences >+C4::Context->set_preference("BranchTransferLimitsType", $originalBranchTransferLimitsType); >+ >+$dbh->rollback; >\ No newline at end of file >-- >1.8.1.2
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 11005
:
21851
|
21993
|
21995
|
21999
|
22320
|
29269