From 222de11a8269277dde86a2ace67dfb8dd5537a01 Mon Sep 17 00:00:00 2001 From: Olli-Antti Kivilahti Date: Mon, 7 Oct 2013 17:22:18 +0300 Subject: [PATCH] Refactored the IsBranchTransferAllowed() and it's dependencies. Created test cases. http://bugs.koha-community.org/show_bug.cgi?id=11005 --- C4/Circulation.pm | 85 ++++++++-- C4/Reserves.pm | 19 +-- .../Circulation/UseBranchTransferLimits.t | 178 +++++++++++++++++++++ 3 files changed, 251 insertions(+), 31 deletions(-) create mode 100644 t/db_dependent/Circulation/UseBranchTransferLimits.t diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 00bc733..0dca13d 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -299,7 +299,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 +315,9 @@ 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 ($doTransfer, $message) = IsBranchTransferAllowed( $tbr, $fbr, $item, $biblio ); + if ( ! $doTransfer ) { + $messages->{'NotAllowed'} = $message; } # if is permanent... @@ -3241,17 +3235,74 @@ return $exist; =head2 IsBranchTransferAllowed - $allowed = IsBranchTransferAllowed( $toBranch, $fromBranch, $code ); - -Code is either an itemtype or collection doe depending on the pref BranchTransferLimitsType + $allowed = IsBranchTransferAllowed( $toBranch, $fromBranch, $item, $biblioitem ); + $allowed = IsBranchTransferAllowed( $toBranch, undef, $item, undef] ); + +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, $toBranch . transfer limit code) + (1) if transfer allowed. =cut sub IsBranchTransferAllowed { - my ( $toBranch, $fromBranch, $code ) = @_; + #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! + - my $limitType = C4::Context->preference("BranchTransferLimitsType"); + my $limitType = C4::Context->preference("BranchTransferLimitsType"); my $dbh = C4::Context->dbh; my $sth = $dbh->prepare("SELECT * FROM branch_transfer_limits WHERE toBranch = ? AND fromBranch = ? AND $limitType = ?"); @@ -3260,7 +3311,7 @@ sub IsBranchTransferAllowed { ## If a row is found, then that combination is not allowed, if no matching row is found, then the combination *is allowed* if ( $limit->{'limitId'} ) { - return 0; + return 0, $toBranch . "::" . $code; } else { return 1; } diff --git a/C4/Reserves.pm b/C4/Reserves.pm index c9a10d9..faea699 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -528,22 +528,13 @@ sub CanItemBeReserved{ } } + $pickupLocation = defined $pickupLocation ? $pickupLocation : $borrower->{branchcode}; # if using Branch Transfer Limits - if ( C4::Context->preference("UseBranchTransferLimits") == 1 ) { - - my $biblio = C4::Biblio::GetBiblioFromItemNumber($itemnumber); - my $pickupLocation = defined $pickupLocation ? $pickupLocation : $borrower->{branchcode}; - - if ( C4::Context->preference("item-level_itypes") && C4::Context->preference("BranchTransferLimitsType") eq 'itemtype' ) { - if ( ! C4::Circulation::IsBranchTransferAllowed( $pickupLocation, $item->{homebranch}, $biblio->{'itype'} ) ) { - return 0; - } - } elsif ( ! C4::Circulation::IsBranchTransferAllowed( $pickupLocation, $item->{homebranch}, $biblio->{ C4::Context->preference("BranchTransferLimitsType") } ) ) { - return 0; - } - } - + if ( ! C4::Circulation::IsBranchTransferAllowed( $pickupLocation, $item->{holdingbranch}, $item ) ) { + return 0; + } + return 1; } #-------------------------------------------------------------------------------- diff --git a/t/db_dependent/Circulation/UseBranchTransferLimits.t b/t/db_dependent/Circulation/UseBranchTransferLimits.t new file mode 100644 index 0000000..b94e97b --- /dev/null +++ b/t/db_dependent/Circulation/UseBranchTransferLimits.t @@ -0,0 +1,178 @@ +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: IsBranchTransferAllowed( $toBranch, $fromBranch, $item, $biblioitem] ); + my $result = C4::Circulation::IsBranchTransferAllowed( 'CPL', 'IPT', $itemCPLFull, $biblioitem ); + is ( $result, 1, "Successful branch transfer, full parameters" ); + + $result = C4::Circulation::IsBranchTransferAllowed( 'IPT', 'CPL', $itemCPLFull, $biblioitem ); + is ( $result, 0, "Failing branch transfer, full parameters" ); + + $result = C4::Circulation::IsBranchTransferAllowed( 'CPL', 'IPT', $itemCPLFull, undef ); + is ( $result, 1, "Successful branch transfer, full parameters, no Biblio defined" ); + + $result = C4::Circulation::IsBranchTransferAllowed( 'IPT', 'CPL', $itemCPLFull, undef ); + is ( $result, 0, "Failing branch transfer, full parameters, no Biblio defined" ); + + $result = C4::Circulation::IsBranchTransferAllowed( 'FFL', undef, $itemCPLFull, $biblioitem ); + is ( $result, 1, "Successful branch transfer, using defaults for \$fromBranch" ); + + $result = C4::Circulation::IsBranchTransferAllowed( 'IPT', undef, $itemCPLFull, $biblioitem ); + is ( $result, 0, "Failing branch transfer, using defaults for \$fromBranch" ); + + $result = C4::Circulation::IsBranchTransferAllowed( 'FFL', undef, $itemCPLFull, undef ); + is ( $result, 1, "Successful branch transfer, using minimum parameters" ); + + $result = C4::Circulation::IsBranchTransferAllowed( 'IPT', undef, $itemCPLFull, undef ); + is ( $result, 0, "Failing branch transfer, using minimum parameters" ); + + $result = C4::Circulation::IsBranchTransferAllowed( '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::IsBranchTransferAllowed( '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::IsBranchTransferAllowed( '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); \ No newline at end of file -- 1.8.1.2