Bugzilla – Attachment 22320 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), 38.03 KB, created by
Olli-Antti Kivilahti
on 2013-10-23 15:12:45 UTC
(
hide
)
Description:
Bug 11005 [ENH] - Centralize the UseBranchTransferLimits-check under one function
Filename:
MIME Type:
Creator:
Olli-Antti Kivilahti
Created:
2013-10-23 15:12:45 UTC
Size:
38.03 KB
patch
obsolete
>From a4d741868d7fcda6ac5a7649613abbfbd48c2d1e 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::CanItemBeTransferred() 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. > >Also a small fix to branchtransfers.pl to make a warning happen only during debugging. > >Unit tests included which test the new C4 function. >--- > C4/Circulation.pm | 423 +++++++++++++--------- > circ/branchtransfers.pl | 5 +- > t/db_dependent/Circulation/CanItemBeTransferred.t | 262 ++++++++++++++ > 3 files changed, 515 insertions(+), 175 deletions(-) > create mode 100644 t/db_dependent/Circulation/CanItemBeTransferred.t > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index d2f5516..29debf3 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -60,55 +60,56 @@ use Date::Calc qw( > use vars qw($VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS); > > BEGIN { >- require Exporter; >+ require Exporter; > $VERSION = 3.07.00.049; # for version checking >- @ISA = qw(Exporter); >+ @ISA = qw(Exporter); > >- # FIXME subs that should probably be elsewhere >- push @EXPORT, qw( >- &barcodedecode >+ # FIXME subs that should probably be elsewhere >+ push @EXPORT, qw( >+ &barcodedecode > &LostItem > &ReturnLostItem >- ); >- >- # subs to deal with issuing a book >- push @EXPORT, qw( >- &CanBookBeIssued >- &CanBookBeRenewed >- &AddIssue >- &AddRenewal >- &GetRenewCount >- &GetItemIssue >- &GetItemIssues >- &GetIssuingCharges >- &GetIssuingRule >+ ); >+ >+ # subs to deal with issuing a book >+ push @EXPORT, qw( >+ &CanBookBeIssued >+ &CanBookBeRenewed >+ &AddIssue >+ &AddRenewal >+ &GetRenewCount >+ &GetItemIssue >+ &GetItemIssues >+ &GetIssuingCharges >+ &GetIssuingRule > &GetBranchBorrowerCircRule > &GetBranchItemRule >- &GetBiblioIssues >- &GetOpenIssue >- &AnonymiseIssueHistory >+ &GetBiblioIssues >+ &GetOpenIssue >+ &AnonymiseIssueHistory > &CheckIfIssuedToPatron > &IsItemIssued >- ); >+ ); > >- # subs to deal with returns >- push @EXPORT, qw( >- &AddReturn >+ # subs to deal with returns >+ push @EXPORT, qw( >+ &AddReturn > &MarkIssueReturned >- ); >- >- # subs to deal with transfers >- push @EXPORT, qw( >- &transferbook >- &GetTransfers >- &GetTransfersFromTo >- &updateWrongTransfer >- &DeleteTransfer >+ ); >+ >+ # subs to deal with transfers >+ push @EXPORT, qw( >+ &transferbook >+ &GetTransfers >+ &GetTransfersFromTo >+ &updateWrongTransfer >+ &DeleteTransfer >+ &CanItemBeTransferred > &IsBranchTransferAllowed > &CreateBranchTransferLimit > &DeleteBranchTransferLimits > &TransferSlip >- ); >+ ); > > # subs to deal with offline circulation > push @EXPORT, qw( >@@ -161,30 +162,30 @@ sub barcodedecode { > my $branch = C4::Branch::mybranch(); > $filter = C4::Context->preference('itemBarcodeInputFilter') unless $filter; > $filter or return $barcode; # ensure filter is defined, else return untouched barcode >- if ($filter eq 'whitespace') { >- $barcode =~ s/\s//g; >- } elsif ($filter eq 'cuecat') { >- chomp($barcode); >- my @fields = split( /\./, $barcode ); >- my @results = map( decode($_), @fields[ 1 .. $#fields ] ); >- ($#results == 2) and return $results[2]; >- } elsif ($filter eq 'T-prefix') { >- if ($barcode =~ /^[Tt](\d)/) { >- (defined($1) and $1 eq '0') and return $barcode; >+ if ($filter eq 'whitespace') { >+ $barcode =~ s/\s//g; >+ } elsif ($filter eq 'cuecat') { >+ chomp($barcode); >+ my @fields = split( /\./, $barcode ); >+ my @results = map( decode($_), @fields[ 1 .. $#fields ] ); >+ ($#results == 2) and return $results[2]; >+ } elsif ($filter eq 'T-prefix') { >+ if ($barcode =~ /^[Tt](\d)/) { >+ (defined($1) and $1 eq '0') and return $barcode; > $barcode = substr($barcode, 2) + 0; # FIXME: probably should be substr($barcode, 1) >- } >+ } > return sprintf("T%07d", $barcode); > # FIXME: $barcode could be "T1", causing warning: substr outside of string > # Why drop the nonzero digit after the T? > # Why pass non-digits (or empty string) to "T%07d"? >- } elsif ($filter eq 'libsuite8') { >- unless($barcode =~ m/^($branch)-/i){ #if barcode starts with branch code its in Koha style. Skip it. >- if($barcode =~ m/^(\d)/i){ #Some barcodes even start with 0's & numbers and are assumed to have b as the item type in the libsuite8 software >+ } elsif ($filter eq 'libsuite8') { >+ unless($barcode =~ m/^($branch)-/i){ #if barcode starts with branch code its in Koha style. Skip it. >+ if($barcode =~ m/^(\d)/i){ #Some barcodes even start with 0's & numbers and are assumed to have b as the item type in the libsuite8 software > $barcode =~ s/^[0]*(\d+)$/$branch-b-$1/i; > }else{ >- $barcode =~ s/^(\D+)[0]*(\d+)$/$branch-$1-$2/i; >- } >- } >+ $barcode =~ s/^(\D+)[0]*(\d+)$/$branch-$1-$2/i; >+ } >+ } > } elsif ($filter eq 'EAN13') { > my $ean = CheckDigits('ean'); > if ( $ean->is_valid($barcode) ) { >@@ -193,7 +194,7 @@ sub barcodedecode { > } else { > warn "# [$barcode] not valid EAN-13/UPC-A\n"; > } >- } >+ } > return $barcode; # return barcode, modified or not > } > >@@ -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) = CanItemBeTransferred( $tbr, $fbr, $item, $biblio ); >+ if ( ! $dotransfer ) { >+ $messages->{'NotAllowed'} = $errMsg; > } > > # if is permanent... >@@ -371,15 +367,15 @@ sub transferbook { > sub TooMany { > my $borrower = shift; > my $biblionumber = shift; >- my $item = shift; >+ my $item = shift; > my $cat_borrower = $borrower->{'categorycode'}; > my $dbh = C4::Context->dbh; >- my $branch; >- # Get which branchcode we need >- $branch = _GetCircControlBranch($item,$borrower); >- my $type = (C4::Context->preference('item-level_itypes')) >- ? $item->{'itype'} # item-level >- : $item->{'itemtype'}; # biblio-level >+ my $branch; >+ # Get which branchcode we need >+ $branch = _GetCircControlBranch($item,$borrower); >+ my $type = (C4::Context->preference('item-level_itypes')) >+ ? $item->{'itype'} # item-level >+ : $item->{'itemtype'}; # biblio-level > > # given branch, patron category, and item type, determine > # applicable issuing rule >@@ -696,15 +692,15 @@ sub CanBookBeIssued { > > my $item = GetItem(GetItemnumberFromBarcode( $barcode )); > my $issue = GetItemIssue($item->{itemnumber}); >- my $biblioitem = GetBiblioItemData($item->{biblioitemnumber}); >- $item->{'itemtype'}=$item->{'itype'}; >+ my $biblioitem = GetBiblioItemData($item->{biblioitemnumber}); >+ $item->{'itemtype'}=$item->{'itype'}; > my $dbh = C4::Context->dbh; > > # MANDATORY CHECKS - unless item exists, nothing else matters > unless ( $item->{barcode} ) { > $issuingimpossible{UNKNOWN_BARCODE} = 1; > } >- return ( \%issuingimpossible, \%needsconfirmation ) if %issuingimpossible; >+ return ( \%issuingimpossible, \%needsconfirmation ) if %issuingimpossible; > > # > # DUE DATE is OK ? -- should already have checked. >@@ -737,7 +733,7 @@ sub CanBookBeIssued { > # BORROWER STATUS > # > if ( $borrower->{'category_type'} eq 'X' && ( $item->{barcode} )) { >- # stats only borrower -- add entry to statistics table, and return issuingimpossible{STATS} = 1 . >+ # stats only borrower -- add entry to statistics table, and return issuingimpossible{STATS} = 1 . > &UpdateStats(C4::Context->userenv->{'branch'},'localuse','','',$item->{'itemnumber'},$item->{'itemtype'},$borrower->{'borrowernumber'}, undef, $item->{'ccode'}); > ModDateLastSeen( $item->{'itemnumber'} ); > return( { STATS => 1 }, {}); >@@ -807,12 +803,12 @@ sub CanBookBeIssued { > my ($blocktype, $count) = C4::Members::IsMemberBlocked($borrower->{'borrowernumber'}); > if ($blocktype == -1) { > ## patron has outstanding overdue loans >- if ( C4::Context->preference("OverduesBlockCirc") eq 'block'){ >- $issuingimpossible{USERBLOCKEDOVERDUE} = $count; >- } >- elsif ( C4::Context->preference("OverduesBlockCirc") eq 'confirmation'){ >- $needsconfirmation{USERBLOCKEDOVERDUE} = $count; >- } >+ if ( C4::Context->preference("OverduesBlockCirc") eq 'block'){ >+ $issuingimpossible{USERBLOCKEDOVERDUE} = $count; >+ } >+ elsif ( C4::Context->preference("OverduesBlockCirc") eq 'confirmation'){ >+ $needsconfirmation{USERBLOCKEDOVERDUE} = $count; >+ } > } elsif($blocktype == 1) { > # patron has accrued fine days > $issuingimpossible{USERBLOCKEDREMAINING} = $count; >@@ -821,7 +817,7 @@ sub CanBookBeIssued { > # > # JB34 CHECKS IF BORROWERS DONT HAVE ISSUE TOO MANY BOOKS > # >- my ($current_loan_count, $max_loans_allowed) = TooMany( $borrower, $item->{biblionumber}, $item ); >+ my ($current_loan_count, $max_loans_allowed) = TooMany( $borrower, $item->{biblionumber}, $item ); > # if TooMany max_loans_allowed returns 0 the user doesn't have permission to check out this book > if (defined $max_loans_allowed && $max_loans_allowed == 0) { > $needsconfirmation{PATRON_CANT} = 1; >@@ -1179,7 +1175,7 @@ AddIssue does the following things : > sub AddIssue { > my ( $borrower, $barcode, $datedue, $cancelreserve, $issuedate, $sipmode) = @_; > my $dbh = C4::Context->dbh; >- my $barcodecheck=CheckValidBarcode($barcode); >+ my $barcodecheck=CheckValidBarcode($barcode); > if ($datedue && ref $datedue ne 'DateTime') { > $datedue = dt_from_string($datedue); > } >@@ -1193,42 +1189,42 @@ sub AddIssue { > > } > } >- if ($borrower and $barcode and $barcodecheck ne '0'){#??? wtf >- # find which item we issue >- my $item = GetItem('', $barcode) or return; # if we don't get an Item, abort. >- my $branch = _GetCircControlBranch($item,$borrower); >- >- # get actual issuing if there is one >- my $actualissue = GetItemIssue( $item->{itemnumber}); >- >- # get biblioinformation for this item >- my $biblio = GetBiblioFromItemNumber($item->{itemnumber}); >- >- # >- # check if we just renew the issue. >- # >- if ($actualissue->{borrowernumber} eq $borrower->{'borrowernumber'}) { >- $datedue = AddRenewal( >- $borrower->{'borrowernumber'}, >- $item->{'itemnumber'}, >- $branch, >- $datedue, >- $issuedate, # here interpreted as the renewal date >- ); >- } >- else { >+ if ($borrower and $barcode and $barcodecheck ne '0'){#??? wtf >+ # find which item we issue >+ my $item = GetItem('', $barcode) or return; # if we don't get an Item, abort. >+ my $branch = _GetCircControlBranch($item,$borrower); >+ >+ # get actual issuing if there is one >+ my $actualissue = GetItemIssue( $item->{itemnumber}); >+ >+ # get biblioinformation for this item >+ my $biblio = GetBiblioFromItemNumber($item->{itemnumber}); >+ >+ # >+ # check if we just renew the issue. >+ # >+ if ($actualissue->{borrowernumber} eq $borrower->{'borrowernumber'}) { >+ $datedue = AddRenewal( >+ $borrower->{'borrowernumber'}, >+ $item->{'itemnumber'}, >+ $branch, >+ $datedue, >+ $issuedate, # here interpreted as the renewal date >+ ); >+ } >+ else { > # it's NOT a renewal >- if ( $actualissue->{borrowernumber}) { >- # This book is currently on loan, but not to the person >- # who wants to borrow it now. mark it returned before issuing to the new borrower >- AddReturn( >- $item->{'barcode'}, >- C4::Context->userenv->{'branch'} >- ); >- } >+ if ( $actualissue->{borrowernumber}) { >+ # This book is currently on loan, but not to the person >+ # who wants to borrow it now. mark it returned before issuing to the new borrower >+ AddReturn( >+ $item->{'barcode'}, >+ C4::Context->userenv->{'branch'} >+ ); >+ } > > MoveReserve( $item->{'itemnumber'}, $borrower->{'borrowernumber'}, $cancelreserve ); >- # Starting process for transfer job (checking transfert and validate it if we have one) >+ # Starting process for transfer job (checking transfert and validate it if we have one) > my ($datesent) = GetTransfers($item->{'itemnumber'}); > if ($datesent) { > # updating line of branchtranfert to finish it, and changing the to branch value, implement a comment for visibility of this case (maybe for stats ....) >@@ -1445,7 +1441,7 @@ sub GetIssuingRule { > my $sth = $dbh->prepare( "select * from issuingrules where categorycode=? and itemtype=? and branchcode=? and issuelength is not null" ); > my $irule; > >- $sth->execute( $borrowertype, $itemtype, $branchcode ); >+ $sth->execute( $borrowertype, $itemtype, $branchcode ); > $irule = $sth->fetchrow_hashref; > return $irule if defined($irule) ; > >@@ -1782,7 +1778,7 @@ sub AddReturn { > if ($doreturn) { > my $datedue = $issue->{date_due}; > $borrower or warn "AddReturn without current borrower"; >- my $circControlBranch; >+ my $circControlBranch; > if ($dropbox) { > # define circControlBranch only if dropbox mode is set > # don't allow dropbox mode to create an invalid entry in issues (issuedate > today) >@@ -2596,27 +2592,27 @@ sub AddRenewal { > > # Send a renewal slip according to checkout alert preferencei > if ( C4::Context->preference('RenewalSendNotice') eq '1') { >- my $borrower = C4::Members::GetMemberDetails( $borrowernumber, 0 ); >- my $circulation_alert = 'C4::ItemCirculationAlertPreference'; >- my %conditions = ( >- branchcode => $branch, >- categorycode => $borrower->{categorycode}, >- item_type => $item->{itype}, >- notification => 'CHECKOUT', >- ); >- if ($circulation_alert->is_enabled_for(\%conditions)) { >- SendCirculationAlert({ >- type => 'RENEWAL', >- item => $item, >- borrower => $borrower, >- branch => $branch, >- }); >- } >+ my $borrower = C4::Members::GetMemberDetails( $borrowernumber, 0 ); >+ my $circulation_alert = 'C4::ItemCirculationAlertPreference'; >+ my %conditions = ( >+ branchcode => $branch, >+ categorycode => $borrower->{categorycode}, >+ item_type => $item->{itype}, >+ notification => 'CHECKOUT', >+ ); >+ if ($circulation_alert->is_enabled_for(\%conditions)) { >+ SendCirculationAlert({ >+ type => 'RENEWAL', >+ item => $item, >+ borrower => $borrower, >+ branch => $branch, >+ }); >+ } > } > > # Log the renewal > UpdateStats( $branch, 'renew', $charge, '', $itemnumber, $item->{itype}, $borrowernumber, undef, $item->{'ccode'}); >- return $datedue; >+ return $datedue; > } > > sub GetRenewCount { >@@ -2931,7 +2927,7 @@ sub SendCirculationAlert { > my %message_name = ( > CHECKIN => 'Item_Check_in', > CHECKOUT => 'Item_Checkout', >- RENEWAL => 'Item_Checkout', >+ RENEWAL => 'Item_Checkout', > ); > my $borrower_preferences = C4::Members::Messaging::GetMessagingPreferences({ > borrowernumber => $borrower->{borrowernumber}, >@@ -2979,20 +2975,20 @@ This function validate the line of brachtransfer but with the wrong destination > =cut > > sub updateWrongTransfer { >- my ( $itemNumber,$waitingAtLibrary,$FromLibrary ) = @_; >- my $dbh = C4::Context->dbh; >+ my ( $itemNumber,$waitingAtLibrary,$FromLibrary ) = @_; >+ my $dbh = C4::Context->dbh; > # first step validate the actual line of transfert . >- my $sth = >- $dbh->prepare( >- "update branchtransfers set datearrived = now(),tobranch=?,comments='wrongtransfer' where itemnumber= ? AND datearrived IS NULL" >- ); >- $sth->execute($FromLibrary,$itemNumber); >+ my $sth = >+ $dbh->prepare( >+ "update branchtransfers set datearrived = now(),tobranch=?,comments='wrongtransfer' where itemnumber= ? AND datearrived IS NULL" >+ ); >+ $sth->execute($FromLibrary,$itemNumber); > > # second step create a new line of branchtransfer to the right location . >- ModItemTransfer($itemNumber, $FromLibrary, $waitingAtLibrary); >+ ModItemTransfer($itemNumber, $FromLibrary, $waitingAtLibrary); > > #third step changing holdingbranch of item >- UpdateHoldingbranch($FromLibrary,$itemNumber); >+ UpdateHoldingbranch($FromLibrary,$itemNumber); > } > > =head2 UpdateHoldingbranch >@@ -3004,7 +3000,7 @@ Simple methode for updating hodlingbranch in items BDD line > =cut > > sub UpdateHoldingbranch { >- my ( $branch,$itemnumber ) = @_; >+ my ( $branch,$itemnumber ) = @_; > ModItem({ holdingbranch => $branch }, undef, $itemnumber); > } > >@@ -3124,9 +3120,9 @@ sub CheckRepeatableHolidays{ > my($itemnumber,$week_day,$branchcode)=@_; > my $dbh = C4::Context->dbh; > my $query = qq|SELECT count(*) >- FROM repeatable_holidays >- WHERE branchcode=? >- AND weekday=?|; >+ FROM repeatable_holidays >+ WHERE branchcode=? >+ AND weekday=?|; > my $sth = $dbh->prepare($query); > $sth->execute($branchcode,$week_day); > my $result=$sth->fetchrow; >@@ -3152,12 +3148,12 @@ sub CheckSpecialHolidays{ > my ($years,$month,$day,$itemnumber,$branchcode) = @_; > my $dbh = C4::Context->dbh; > my $query=qq|SELECT count(*) >- FROM `special_holidays` >- WHERE year=? >- AND month=? >- AND day=? >+ FROM `special_holidays` >+ WHERE year=? >+ AND month=? >+ AND day=? > AND branchcode=? >- |; >+ |; > my $sth = $dbh->prepare($query); > $sth->execute($years,$month,$day,$branchcode); > my $countspecial=$sth->fetchrow ; >@@ -3181,11 +3177,11 @@ sub CheckRepeatableSpecialHolidays{ > my ($month,$day,$itemnumber,$branchcode) = @_; > my $dbh = C4::Context->dbh; > my $query=qq|SELECT count(*) >- FROM `repeatable_holidays` >- WHERE month=? >- AND day=? >+ FROM `repeatable_holidays` >+ WHERE month=? >+ AND day=? > AND branchcode=? >- |; >+ |; > my $sth = $dbh->prepare($query); > $sth->execute($month,$day,$branchcode); > my $countspecial=$sth->fetchrow ; >@@ -3198,15 +3194,96 @@ sub CheckValidBarcode{ > my ($barcode) = @_; > my $dbh = C4::Context->dbh; > my $query=qq|SELECT count(*) >- FROM items >+ FROM items > WHERE barcode=? >- |; >+ |; > my $sth = $dbh->prepare($query); > $sth->execute($barcode); > my $exist=$sth->fetchrow ; > return $exist; > } > >+ >+=head2 CanItemBeTransferred >+ >+A convenience function to easily check item transfer limits. >+ >+ ($allowed, $errorMessage) = CanItemBeTransferred( $toBranch, $fromBranch, $item, $biblioitem ); >+ ($allowed, $errorMessage) = CanItemBeTransferred( $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 CanItemBeTransferred { >+ >+ #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 $item->{ $limitType }) { >+ $code = $item->{ $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 ); >@@ -3216,23 +3293,23 @@ Code is either an itemtype or collection doe depending on the pref BranchTransfe > =cut > > sub IsBranchTransferAllowed { >- my ( $toBranch, $fromBranch, $code ) = @_; >+ my ( $toBranch, $fromBranch, $code ) = @_; > >- if ( $toBranch eq $fromBranch ) { return 1; } ## Short circuit for speed. >+ if ( $toBranch eq $fromBranch ) { return 1; } ## Short circuit for speed. > >- my $limitType = C4::Context->preference("BranchTransferLimitsType"); >- my $dbh = C4::Context->dbh; >+ 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 = ?"); >- $sth->execute( $toBranch, $fromBranch, $code ); >- my $limit = $sth->fetchrow_hashref(); >+ my $sth = $dbh->prepare("SELECT * FROM branch_transfer_limits WHERE toBranch = ? AND fromBranch = ? AND $limitType = ?"); >+ $sth->execute( $toBranch, $fromBranch, $code ); >+ my $limit = $sth->fetchrow_hashref(); > >- ## 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; >- } else { >- return 1; >- } >+ ## 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; >+ } else { >+ return 1; >+ } > } > > =head2 CreateBranchTransferLimit >diff --git a/circ/branchtransfers.pl b/circ/branchtransfers.pl >index 3be7bb4..4e1ef17 100755 >--- a/circ/branchtransfers.pl >+++ b/circ/branchtransfers.pl >@@ -206,10 +206,11 @@ foreach my $code ( keys %$messages ) { > $err{errbadcode} = 1; > } > elsif ( $code eq "NotAllowed" ) { >- warn "NotAllowed: $messages->{'NotAllowed'} to " . $branches->{ $messages->{'NotAllowed'} }->{'branchname'}; >+ warn "NotAllowed: $messages->{'NotAllowed'} to " . $branches->{ $messages->{'NotAllowed'} }->{'branchname'} if $ENV{DEBUG}; >+ > # 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/CanItemBeTransferred.t b/t/db_dependent/Circulation/CanItemBeTransferred.t >new file mode 100644 >index 0000000..a74b074 >--- /dev/null >+++ b/t/db_dependent/Circulation/CanItemBeTransferred.t >@@ -0,0 +1,262 @@ >+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 runTestsForCCode { >+ my ($itemCPLFull, $itemCPLLite, $biblioitem) = @_; >+ >+ print 'Running tests for '.C4::Context->preference("BranchTransferLimitsType")."\n"; >+ >+ #howto use: CanItemBeTransferred( $toBranch, $fromBranch, $item, $biblioitem] ); >+ my $result = C4::Circulation::CanItemBeTransferred( 'CPL', 'IPT', $itemCPLFull, $biblioitem ); >+ is ( $result, 1, "Successful branch transfer, full parameters" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'IPT', 'CPL', $itemCPLFull, $biblioitem ); >+ is ( $result, 'CPL->IPT->FANTASY', "Failing branch transfer, full parameters" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'CPL', 'IPT', $itemCPLFull, undef ); >+ is ( $result, 1, "Successful branch transfer, full parameters, no Biblio defined" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'IPT', 'CPL', $itemCPLFull, undef ); >+ is ( $result, 'CPL->IPT->FANTASY', "Failing branch transfer, full parameters, no Biblio defined" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'FFL', undef, $itemCPLFull, $biblioitem ); >+ is ( $result, 1, "Successful branch transfer, using defaults for \$fromBranch" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'IPT', undef, $itemCPLFull, $biblioitem ); >+ is ( $result, 'CPL->IPT->FANTASY', "Failing branch transfer, using defaults for \$fromBranch" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'FFL', undef, $itemCPLFull, undef ); >+ is ( $result, 1, "Successful branch transfer, using minimum parameters" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'IPT', undef, $itemCPLFull, undef ); >+ is ( $result, 'CPL->IPT->FANTASY', "Failing branch transfer, using minimum parameters" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'FFL', undef, $itemCPLLite, undef ); >+ is ( $result, 1, "Successful branch transfer, using minimum parameters" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'IPT', undef, $itemCPLLite, undef ); >+ is ( $result, 1, "Not failing branch transfer, because CCODE cannot be found from the item and it is not a part of the biblio." ); >+} >+ >+ >+sub runTestsForItype { >+ my ($itemCPLFull, $itemCPLLite, $biblioitem) = @_; >+ >+ print 'Running tests for '.C4::Context->preference("BranchTransferLimitsType")."\n"; >+ >+ #howto use: CanItemBeTransferred( $toBranch, $fromBranch, $item, $biblioitem] ); >+ my $result = C4::Circulation::CanItemBeTransferred( 'CPL', 'IPT', $itemCPLFull, $biblioitem ); >+ is ( $result, 1, "Successful branch transfer, full parameters" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'IPT', 'CPL', $itemCPLFull, $biblioitem ); >+ is ( $result, 'CPL->IPT->BK', "Failing branch transfer, full parameters" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'CPL', 'IPT', $itemCPLFull, undef ); >+ is ( $result, 1, "Successful branch transfer, full parameters, no Biblio defined" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'IPT', 'CPL', $itemCPLFull, undef ); >+ is ( $result, 'CPL->IPT->BK', "Failing branch transfer, full parameters, no Biblio defined" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'FFL', undef, $itemCPLFull, $biblioitem ); >+ is ( $result, 1, "Successful branch transfer, using defaults for \$fromBranch" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'IPT', undef, $itemCPLFull, $biblioitem ); >+ is ( $result, 'CPL->IPT->BK', "Failing branch transfer, using defaults for \$fromBranch" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'FFL', undef, $itemCPLFull, undef ); >+ is ( $result, 1, "Successful branch transfer, using minimum parameters" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'IPT', undef, $itemCPLFull, undef ); >+ is ( $result, 'CPL->IPT->BK', "Failing branch transfer, using minimum parameters" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'FFL', undef, $itemCPLLite, undef ); >+ is ( $result, 1, "Successful branch transfer, using minimum parameters, itemtype is pulled from Biblio" ); >+ >+ $result = C4::Circulation::CanItemBeTransferred( 'IPT', undef, $itemCPLLite, undef ); >+ is ( $result, 'CPL->IPT->BK', "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; >+if ( C4::Context->preference("marcflavour") eq "UNIMARC" ) { >+ $marcxml=qq( >+ <?xml version="1.0" encoding="UTF-8"?> >+ <record >+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" >+ xsi:schemaLocation="http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd" >+ xmlns="http://www.loc.gov/MARC21/slim"> >+ <leader>01534njm a2200229 4500</leader> >+ <controlfield tag="001">4172</controlfield> >+ <datafield tag="071" ind1=" " ind2=" "> >+ <subfield code="a">8344482</subfield> >+ </datafield> >+ <datafield tag="090" ind1=" " ind2=" "> >+ <subfield code="a">4172</subfield> >+ </datafield> >+ <datafield tag="100" ind1=" " ind2=" "> >+ <subfield code="a">20040408 frey50 </subfield> >+ </datafield> >+ <datafield tag="126" ind1=" " ind2=" "> >+ <subfield code="a">agbxhx cd</subfield> >+ </datafield> >+ <datafield tag="200" ind1="1" ind2=" "> >+ <subfield code="a">The Beatles anthology 1965-1967</subfield> >+ <subfield code="e">vol.2</subfield> >+ <subfield code="f">The Beatles</subfield> >+ <subfield code="b">CD</subfield> >+ </datafield> >+ <datafield tag="210" ind1=" " ind2="1"> >+ <subfield code="a">Null</subfield> >+ <subfield code="c">Emi music</subfield> >+ <subfield code="d">1996</subfield> >+ </datafield> >+ <datafield tag="322" ind1=" " ind2="1"> >+ <subfield code="a">anthologie des beatles concernant l'époque musicale de 1965 à 1968 période la plus expérimentale et prolifique du groupe</subfield> >+ </datafield> >+ <datafield tag="327" ind1="1" ind2=" "> >+ <subfield code="a">Real love</subfield> >+ <subfield code="a">Yes it is</subfield> >+ </datafield> >+ <datafield tag="345" ind1=" " ind2=" "> >+ <subfield code="a">CVS</subfield> >+ <subfield code="b">0724383444823</subfield> >+ <subfield code="c">disque compact</subfield> >+ <subfield code="d">34 E</subfield> >+ </datafield> >+ </record> >+ ); >+} >+else { # Using Marc21 by default >+ $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); >+ >+# This should work regardless of the Marc flavour. >+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} ); >+ >+ >+runTestsForCCode($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} ); >+ >+ >+runTestsForItype($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