From f9164052d483fc8ec64e81554fdbc91b94926f3e Mon Sep 17 00:00:00 2001 From: Olli-Antti Kivilahti 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 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( + + 00000cim a22000004a 4500 + 1001 + 2013-06-03 07:04:07+02 + ss||||j||||||| + uuuu xxk|||||||||||||||||eng|c + + 0-00-103147-3 + 14.46 EUR + + + eng + + + 83.5 + ykl + + + SHAKESPEARE, WILLIAM. + + + THE TAMING OF THE SHREW / + WILLIAM SHAKESPEARE + [ÄÄNITE]. + + + LONDON : + COLLINS. + + + 2 ÄÄNIKASETTIA. + + + FI-Jm + 83.5 + + + FI-Konti + 83.5 + + +); +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