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