From 05c9c93b7f7d9cfe5f2942d64a91d5c0f4fd7a2a Mon Sep 17 00:00:00 2001
From: Kyle M Hall <kyle@bywatersolutions.com>
Date: Wed, 29 May 2013 09:11:38 -0400
Subject: [PATCH] Bug 10374 - Clean up and unify CircControl related code - Followup - Clean up AddReturn
This patch tidys up C4::Circulation::AddReturn. It rearranges some
comments to make them more sensible, and addresses the FIXME for an
unitelligable comment. This patch makes no functional changes to the
code.
---
C4/Circulation.pm | 231 +++++++++++++++++++++++++++++++++-------------------
1 files changed, 146 insertions(+), 85 deletions(-)
diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index b6c6cc0..c9744a7 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1710,61 +1710,78 @@ patron who last borrowed the book.
sub AddReturn {
my ( $barcode, $branch, $exemptfine, $dropbox ) = @_;
- if ($branch and not GetBranchDetail($branch)) {
- warn "AddReturn error: branch '$branch' not found. Reverting to " . C4::Context->userenv->{'branch'};
+ if ( $branch and not GetBranchDetail($branch) ) {
+ warn "AddReturn error: branch '$branch' not found. Reverting to "
+ . C4::Context->userenv->{'branch'};
undef $branch;
}
- $branch = C4::Context->userenv->{'branch'} unless $branch; # we trust userenv to be a safe fallback/default
+
+ $branch = C4::Context->userenv->{'branch'}
+ unless $branch; # we trust userenv to be a safe fallback/default
+
my $messages;
my $borrower;
my $biblio;
my $doreturn = 1;
my $validTransfert = 0;
- my $stat_type = 'return';
+ my $stat_type = 'return';
# get information on item
- my $itemnumber = GetItemnumberFromBarcode( $barcode );
- unless ($itemnumber) {
- return (0, { BadBarcode => $barcode }); # no barcode means no item or borrower. bail out.
- }
- my $issue = GetItemIssue($itemnumber);
-# warn Dumper($iteminformation);
- if ($issue and $issue->{borrowernumber}) {
- $borrower = C4::Members::GetMemberDetails($issue->{borrowernumber})
- or die "Data inconsistency: barcode $barcode (itemnumber:$itemnumber) claims to be issued to non-existant borrowernumber '$issue->{borrowernumber}'\n"
- . Dumper($issue) . "\n";
- } else {
+ my $itemnumber = GetItemnumberFromBarcode($barcode);
+
+ # no barcode means no item or borrower. bail out.
+ return ( 0, { BadBarcode => $barcode } ) unless ($itemnumber);
+
+ my $issue = GetItemIssue($itemnumber);
+
+ if ( $issue and $issue->{borrowernumber} ) {
+ $borrower = C4::Members::GetMemberDetails( $issue->{borrowernumber} )
+ or die
+ "Data inconsistency: barcode $barcode (itemnumber:$itemnumber) claims to be"
+ . " issued to non-existant borrowernumber '$issue->{borrowernumber}'\n"
+ . Dumper($issue) . "\n";
+ }
+ else {
$messages->{'NotIssued'} = $barcode;
+
# even though item is not on loan, it may still be transferred; therefore, get current branch info
$doreturn = 0;
+
# No issue, no borrowernumber. ONLY if $doreturn, *might* you have a $borrower later.
# Record this as a local use, instead of a return, if the RecordLocalUseOnReturn is on
- if (C4::Context->preference("RecordLocalUseOnReturn")) {
- $messages->{'LocalUse'} = 1;
- $stat_type = 'localuse';
+ if ( C4::Context->preference("RecordLocalUseOnReturn") ) {
+ $messages->{'LocalUse'} = 1;
+ $stat_type = 'localuse';
}
}
my $item = GetItem($itemnumber) or die "GetItem($itemnumber) failed";
- # full item data, but no borrowernumber or checkout info (no issue)
- # we know GetItem should work because GetItemnumberFromBarcode worked
- my $hbr = GetBranchItemRule($item->{'homebranch'}, $item->{'itype'})->{'returnbranch'} || "homebranch";
- # get the proper branch to which to return the item
- $hbr = $item->{$hbr} || $branch ;
- # if $hbr was "noreturn" or any other non-item table value, then it should 'float' (i.e. stay at this branch)
- my $borrowernumber = $borrower->{'borrowernumber'} || undef; # we don't know if we had a borrower or not
+ # full item data, but no borrowernumber or checkout info (no issue)
+ # we know GetItem should work because GetItemnumberFromBarcode worked
+ my $hbr =
+ GetBranchItemRule( $item->{'homebranch'}, $item->{'itype'} )->{'returnbranch'}
+ || "homebranch";
+
+ # get the proper branch to which to return the item
+ # if $hbr was "noreturn" or any other non-item table value, then it should 'float' (i.e. stay at this branch)
+ $hbr = $item->{$hbr} || $branch;
+
+ my $borrowernumber = $borrower->{'borrowernumber'}
+ || undef; # we don't know if we had a borrower or not
# check if the book is in a permanent collection....
# FIXME -- This 'PE' attribute is largely undocumented. afaict, there's no user interface that reflects this functionality.
- if ( $hbr ) {
- my $branches = GetBranches(); # a potentially expensive call for a non-feature.
+ if ($hbr) {
+
+ # a potentially expensive call for a non-feature.
+ my $branches = GetBranches();
$branches->{$hbr}->{PE} and $messages->{'IsPermanent'} = $hbr;
}
# check if the return is allowed at this branch
- my ($returnallowed, $message) = CanBookBeReturned($item, $branch);
- unless ($returnallowed){
+ my ( $returnallowed, $message ) = CanBookBeReturned( $item, $branch );
+ unless ($returnallowed) {
$messages->{'Wrongbranch'} = {
Wrongbranch => $branch,
Rightbranch => $message
@@ -1773,7 +1790,7 @@ sub AddReturn {
return ( $doreturn, $messages, $issue, $borrower );
}
- if ( $item->{'wthdrawn'} ) { # book has been cancelled
+ if ( $item->{'wthdrawn'} ) { # book has been cancelled
$messages->{'wthdrawn'} = 1;
$doreturn = 0 if C4::Context->preference("BlockReturnOfWithdrawnItems");
}
@@ -1781,12 +1798,14 @@ sub AddReturn {
# case of a return of document (deal with issues and holdingbranch)
my $today = DateTime->now( time_zone => C4::Context->tz() );
if ($doreturn) {
- my $datedue = $issue->{date_due};
+ my $datedue = $issue->{date_due};
$borrower or warn "AddReturn without current borrower";
if ($dropbox) {
+
# don't allow dropbox mode to create an invalid entry in issues (issuedate > today)
# FIXME: check issuedate > returndate, factoring in holidays
- $issue->{'overdue'} = DateTime->compare($issue->{'date_due'}, $today ) == -1 ? 1 : 0;
+ $issue->{'overdue'} =
+ DateTime->compare( $issue->{'date_due'}, $today ) == -1 ? 1 : 0;
}
if ($borrowernumber) {
@@ -1818,56 +1837,80 @@ sub AddReturn {
}
- ModItem({ onloan => undef }, $issue->{'biblionumber'}, $item->{'itemnumber'});
+ ModItem(
+ { onloan => undef },
+ $issue->{'biblionumber'},
+ $item->{'itemnumber'}
+ );
}
# the holdingbranch is updated if the document is returned to another location.
# this is always done regardless of whether the item was on loan or not
- if ($item->{'holdingbranch'} ne $branch) {
- UpdateHoldingbranch($branch, $item->{'itemnumber'});
+ if ( $item->{'holdingbranch'} ne $branch ) {
+ UpdateHoldingbranch( $branch, $item->{'itemnumber'} );
$item->{'holdingbranch'} = $branch; # update item data holdingbranch too
}
ModDateLastSeen( $item->{'itemnumber'} );
# check if we have a transfer for this document
- my ($datesent,$frombranch,$tobranch) = GetTransfers( $item->{'itemnumber'} );
+ my ( $datesent, $frombranch, $tobranch ) =
+ GetTransfers( $item->{'itemnumber'} );
# if we have a transfer to do, we update the line of transfers with the datearrived
if ($datesent) {
if ( $tobranch eq $branch ) {
- my $sth = C4::Context->dbh->prepare(
- "UPDATE branchtransfers SET datearrived = now() WHERE itemnumber= ? AND datearrived IS NULL"
- );
+ my $sth = C4::Context->dbh->prepare("
+ UPDATE branchtransfers
+ SET datearrived = now()
+ WHERE itemnumber= ?
+ AND datearrived IS NULL
+ ");
$sth->execute( $item->{'itemnumber'} );
+
+ ShelfToCart( $item->{'itemnumber'} )
+ if ( C4::Context->preference("ReturnToShelvingCart") );
+
# if we have a reservation with valid transfer, we can set it's status to 'W'
- ShelfToCart( $item->{'itemnumber'} ) if ( C4::Context->preference("ReturnToShelvingCart") );
- C4::Reserves::ModReserveStatus($item->{'itemnumber'}, 'W');
- } else {
+ C4::Reserves::ModReserveStatus( $item->{'itemnumber'}, 'W' );
+ }
+ else {
$messages->{'WrongTransfer'} = $tobranch;
$messages->{'WrongTransferItem'} = $item->{'itemnumber'};
}
$validTransfert = 1;
- } else {
- ShelfToCart( $item->{'itemnumber'} ) if ( C4::Context->preference("ReturnToShelvingCart") );
+ }
+ else {
+ ShelfToCart( $item->{'itemnumber'} )
+ if ( C4::Context->preference("ReturnToShelvingCart") );
}
# fix up the accounts.....
if ( $item->{'itemlost'} ) {
$messages->{'WasLost'} = 1;
- if ( C4::Context->preference('RefundLostItemFeeOnReturn' ) ) {
- _FixAccountForLostAndReturned($item->{'itemnumber'}, $borrowernumber, $barcode); # can tolerate undef $borrowernumber
+ if ( C4::Context->preference('RefundLostItemFeeOnReturn') ) {
+ _FixAccountForLostAndReturned( $item->{'itemnumber'},
+ $borrowernumber, $barcode )
+ ; # can tolerate undef $borrowernumber
$messages->{'LostItemFeeRefunded'} = 1;
}
}
# fix up the overdues in accounts...
if ($borrowernumber) {
- my $fix = _FixOverduesOnReturn($borrowernumber, $item->{itemnumber}, $exemptfine, $dropbox);
- defined($fix) or warn "_FixOverduesOnReturn($borrowernumber, $item->{itemnumber}...) failed!"; # zero is OK, check defined
-
+
+ # zero is OK, check defined
+ my $fix =
+ _FixOverduesOnReturn( $borrowernumber, $item->{itemnumber},
+ $exemptfine, $dropbox );
+
+ warn "_FixOverduesOnReturn($borrowernumber, "
+ . "$item->{itemnumber}...) failed!"
+ unless defined($fix);
+
if ( $issue->{overdue} && $issue->{date_due} ) {
-# fix fine days
+
+ # fix fine days
my $debardate =
_debar_user_on_return( $borrower, $item, $issue->{date_due}, $today );
$messages->{Debarred} = $debardate if ($debardate);
@@ -1876,57 +1919,75 @@ sub AddReturn {
# find reserves.....
# if we don't have a reserve with the status W, we launch the Checkreserves routine
- my ($resfound, $resrec);
- ($resfound, $resrec, undef) = C4::Reserves::CheckReserves( $item->{'itemnumber'} ) unless ( $item->{'wthdrawn'} );
+ my ( $resfound, $resrec );
+ ( $resfound, $resrec, undef ) =
+ C4::Reserves::CheckReserves( $item->{'itemnumber'} )
+ unless ( $item->{'wthdrawn'} );
if ($resfound) {
- $resrec->{'ResFound'} = $resfound;
+ $resrec->{'ResFound'} = $resfound;
$messages->{'ResFound'} = $resrec;
}
- # update stats?
# Record the fact that this book was returned.
- UpdateStats(
- $branch, $stat_type, '0', '',
- $item->{'itemnumber'},
- $biblio->{'itemtype'},
- $borrowernumber, undef, $item->{'ccode'}
- );
+ UpdateStats( $branch, $stat_type, '0', '', $item->{'itemnumber'},
+ $biblio->{'itemtype'}, $borrowernumber, undef, $item->{'ccode'} );
# Send a check-in slip. # NOTE: borrower may be undef. probably shouldn't try to send messages then.
my $circulation_alert = 'C4::ItemCirculationAlertPreference';
- my %conditions = (
+ my %conditions = (
branchcode => $branch,
categorycode => $borrower->{categorycode},
item_type => $item->{itype},
notification => 'CHECKIN',
);
- if ($doreturn && $circulation_alert->is_enabled_for(\%conditions)) {
- SendCirculationAlert({
- type => 'CHECKIN',
- item => $item,
- borrower => $borrower,
- branch => $branch,
- });
+ if ( $doreturn && $circulation_alert->is_enabled_for( \%conditions ) ) {
+ SendCirculationAlert(
+ {
+ type => 'CHECKIN',
+ item => $item,
+ borrower => $borrower,
+ branch => $branch,
+ }
+ );
}
-
- logaction("CIRCULATION", "RETURN", $borrowernumber, $item->{'itemnumber'})
- if C4::Context->preference("ReturnLog");
-
- # FIXME: make this comment intelligible.
- #adding message if holdingbranch is non equal a userenv branch to return the document to homebranch
- #we check, if we don't have reserv or transfert for this document, if not, return it to homebranch .
-
- if (($doreturn or $messages->{'NotIssued'}) and !$resfound and ($branch ne $hbr) and not $messages->{'WrongTransfer'}){
- if ( C4::Context->preference("AutomaticItemReturn" ) or
- (C4::Context->preference("UseBranchTransferLimits") and
- ! IsBranchTransferAllowed($branch, $hbr, $item->{C4::Context->preference("BranchTransferLimitsType")} )
- )) {
- $debug and warn sprintf "about to call ModItemTransfer(%s, %s, %s)", $item->{'itemnumber'},$branch, $hbr;
- $debug and warn "item: " . Dumper($item);
- ModItemTransfer($item->{'itemnumber'}, $branch, $hbr);
+
+ logaction( "CIRCULATION", "RETURN", $borrowernumber, $item->{'itemnumber'} )
+ if C4::Context->preference("ReturnLog");
+
+ # If this logged in branch is not the holdingbranch, add the message WasTransferred
+ # Then, check to see if this item has either a reserve here or a valid transfer,
+ # if not, then we need to send it back to the home branch.
+ if ( ( $doreturn or $messages->{'NotIssued'} )
+ and !$resfound
+ and $branch ne $hbr
+ and !$messages->{'WrongTransfer'} )
+ {
+ if (
+ C4::Context->preference("AutomaticItemReturn")
+ or (
+ C4::Context->preference("UseBranchTransferLimits")
+ and !IsBranchTransferAllowed(
+ $branch,
+ $hbr,
+ $item->{ C4::Context->preference("BranchTransferLimitsType")
+ }
+ )
+ )
+ )
+ {
+ if ($debug) {
+ warn sprintf "about to call ModItemTransfer(%s, %s, %s)",
+ $item->{'itemnumber'}, $branch, $hbr;
+ warn "item: " . Dumper($item);
+ }
+
+ ModItemTransfer( $item->{'itemnumber'}, $branch, $hbr );
$messages->{'WasTransfered'} = 1;
- } else {
- $messages->{'NeedsTransfer'} = 1; # TODO: instead of 1, specify branchcode that the transfer SHOULD go to, $item->{homebranch}
+ }
+ else {
+
+ # TODO: instead of 1, specify branchcode that the transfer SHOULD go to, $item->{homebranch}
+ $messages->{'NeedsTransfer'} = 1;
}
}
return ( $doreturn, $messages, $issue, $borrower );
--
1.7.2.5