From 356ffccb730e12ab3031d082b5d519cb5d2853db Mon Sep 17 00:00:00 2001 From: Kyle M Hall 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 | 232 +++++++++++++++++++++++++++++++++------------------- 1 files changed, 147 insertions(+), 85 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index b6c6cc0..d56fb30 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,82 @@ 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') ) { + + # can tolerate undef $borrowernumber + _FixAccountForLostAndReturned( $item->{'itemnumber'}, + $borrowernumber, $barcode ); + $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 +1921,74 @@ 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 ) = + 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 || $messages->{'NotIssued'} ) + && !$resfound + && $branch ne $hbr + && !$messages->{'WrongTransfer'} ) + { + if ( + C4::Context->preference("AutomaticItemReturn") + || ( + C4::Context->preference("UseBranchTransferLimits") + && !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