Bug 3749 - QueryRemoveStopwords shouldn't link to stop words
[koha.git] / C4 / Circulation.pm
index fe28409..1778a1c 100644 (file)
@@ -32,6 +32,7 @@ use C4::Calendar;
 use C4::Accounts;
 use C4::ItemCirculationAlertPreference;
 use C4::Message;
+use C4::Debug;
 use Date::Calc qw(
   Today
   Today_and_Now
@@ -339,16 +340,7 @@ sub TooMany {
     my $dbh             = C4::Context->dbh;
        my $branch;
        # Get which branchcode we need
-       if (C4::Context->preference('CircControl') eq 'PickupLibrary'){
-               $branch = C4::Context->userenv->{'branch'}; 
-       }
-       elsif (C4::Context->preference('CircControl') eq 'PatronLibrary'){
-        $branch = $borrower->{'branchcode'}; 
-       }
-       else {
-               # items home library
-               $branch = $item->{'homebranch'};
-       }
+       $branch = _GetCircControlBranch($item,$borrower);
        my $type = (C4::Context->preference('item-level_itypes')) 
                        ? $item->{'itype'}         # item-level
                        : $item->{'itemtype'};     # biblio-level
@@ -680,9 +672,8 @@ sub CanBookBeIssued {
     #
     unless ( $duedate ) {
         my $issuedate = strftime( "%Y-%m-%d", localtime );
-        my $branch = (C4::Context->preference('CircControl') eq 'PickupLibrary') ? C4::Context->userenv->{'branch'} :
-                     (C4::Context->preference('CircControl') eq 'PatronLibrary') ? $borrower->{'branchcode'}        :
-                     $item->{'homebranch'};     # fallback to item's homebranch
+
+        my $branch = _GetCircControlBranch($item,$borrower);
         my $itype = ( C4::Context->preference('item-level_itypes') ) ? $item->{'itype'} : $biblioitem->{'itemtype'};
         my $loanlength = GetLoanLength( $borrower->{'categorycode'}, $itype, $branch );
         $duedate = CalcDateDue( C4::Dates->new( $issuedate, 'iso' ), $loanlength, $branch, $borrower );
@@ -771,12 +762,20 @@ sub CanBookBeIssued {
             $sth->execute($item->{'itemtype'});
             my $notforloan=$sth->fetchrow_hashref();
             $sth->finish();
-            if ($notforloan->{'notforloan'} == 1){
-                $issuingimpossible{NOT_FOR_LOAN} = 1;
+            if ($notforloan->{'notforloan'}) {
+                if (!C4::Context->preference("AllowNotForLoanOverride")) {
+                    $issuingimpossible{NOT_FOR_LOAN} = 1;
+                } else {
+                    $needsconfirmation{NOT_FOR_LOAN_FORCING} = 1;
+                }
             }
         }
         elsif ($biblioitem->{'notforloan'} == 1){
-            $issuingimpossible{NOT_FOR_LOAN} = 1;
+            if (!C4::Context->preference("AllowNotForLoanOverride")) {
+                $issuingimpossible{NOT_FOR_LOAN} = 1;
+            } else {
+                $needsconfirmation{NOT_FOR_LOAN_FORCING} = 1;
+            }
         }
     }
     if ( $item->{'wthdrawn'} && $item->{'wthdrawn'} == 1 )
@@ -790,7 +789,7 @@ sub CanBookBeIssued {
     }
     if ( C4::Context->preference("IndependantBranches") ) {
         my $userenv = C4::Context->userenv;
-        if ( ($userenv) && ( $userenv->{flags} != 1 ) ) {
+        if ( ($userenv) && ( $userenv->{flags} % 2 != 1 ) ) {
             $issuingimpossible{NOTSAMEBRANCH} = 1
               if ( $item->{C4::Context->preference("HomeOrHoldingBranch")} ne $userenv->{branch} );
         }
@@ -900,9 +899,7 @@ sub AddIssue {
        if ($borrower and $barcode and $barcodecheck ne '0'){
                # find which item we issue
                my $item = GetItem('', $barcode) or return undef;       # if we don't get an Item, abort.
-               my $branch = (C4::Context->preference('CircControl') eq 'PickupLibrary') ? C4::Context->userenv->{'branch'} :
-                     (C4::Context->preference('CircControl') eq 'PatronLibrary') ? $borrower->{'branchcode'}        : 
-                     $item->{'homebranch'};     # fallback to item's homebranch
+               my $branch = _GetCircControlBranch($item,$borrower);
                
                # get actual issuing if there is one
                my $actualissue = GetItemIssue( $item->{itemnumber});
@@ -1003,6 +1000,9 @@ sub AddIssue {
             C4::Context->userenv->{'branch'}    # branchcode
         );
         $sth->finish;
+        if ( C4::Context->preference('ReturnToShelvingCart') ) { ## ReturnToShelvingCart is on, anything issued should be taken off the cart.
+          CartToShelf( $item->{'itemnumber'} );
+        }
         $item->{'issues'}++;
         ModItem({ issues           => $item->{'issues'},
                   holdingbranch    => C4::Context->userenv->{'branch'},
@@ -1051,7 +1051,7 @@ sub AddIssue {
         }
     }
 
-    logaction("CIRCULATION", "ISSUE", $borrower->{'borrowernumber'}, $biblio->{'biblionumber'}) 
+    logaction("CIRCULATION", "ISSUE", $borrower->{'borrowernumber'}, $biblio->{'biblionumber'})
         if C4::Context->preference("IssueLog");
   }
   return ($datedue);   # not necessarily the same as when it came in!
@@ -1121,7 +1121,7 @@ sub GetLoanLength {
 
 =head2 GetIssuingRule
 
-FIXME - This is a copy-paste of GetLoanLength 
+FIXME - This is a copy-paste of GetLoanLength
 as a stop-gap.  Do not wish to change API for GetLoanLength 
 this close to release, however, Overdues::GetIssuingRules is broken.
 
@@ -1353,7 +1353,8 @@ C<&AddReturn> returns a list of four items:
 
 C<$doreturn> is true iff the return succeeded.
 
-C<$messages> is a reference-to-hash giving the reason for failure:
+C<$messages> is a reference-to-hash giving feedback on the operation.
+The keys of the hash are:
 
 =over 4
 
@@ -1375,6 +1376,12 @@ the book's home branch.
 
 This book has been withdrawn/cancelled. The value should be ignored.
 
+=item C<Wrongbranch>
+
+This book has was returned to the wrong branch.  The value is a hashref
+so that C<$messages->{Wrongbranch}->{Wrongbranch}> and C<$messages->{Wrongbranch}->{Rightbranch}>
+contain the branchcode of the incorrect and correct return library, respectively.
+
 =item C<ResFound>
 
 The item was reserved. The value is a reference-to-hash whose keys are
@@ -1384,6 +1391,9 @@ either C<Waiting>, C<Reserved>, or 0.
 
 =back
 
+C<$iteminformation> is a reference-to-hash, giving information about the
+returned item from the issues table.
+
 C<$borrower> is a reference-to-hash, giving information about the
 patron who last borrowed the book.
 
@@ -1391,189 +1401,181 @@ patron who last borrowed the book.
 
 sub AddReturn {
     my ( $barcode, $branch, $exemptfine, $dropbox ) = @_;
-    my $dbh      = C4::Context->dbh;
+    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
     my $messages;
-    my $doreturn = 1;
     my $borrower;
+    my $biblio;
+    my $doreturn       = 1;
     my $validTransfert = 0;
-    my $reserveDone = 0;
     
     # get information on item
-    my $iteminformation = GetItemIssue( GetItemnumberFromBarcode($barcode));
-    my $biblio = GetBiblioItemData($iteminformation->{'biblioitemnumber'});
-#     use Data::Dumper;warn Data::Dumper::Dumper($iteminformation);  
-    unless ($iteminformation->{'itemnumber'} ) {
-        $messages->{'BadBarcode'} = $barcode;
-        $doreturn = 0;
+    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 {
-        # find the borrower
-        if ( ( not $iteminformation->{borrowernumber} ) && $doreturn ) {
-            $messages->{'NotIssued'} = $barcode;
-            # even though item is not on loan, it may still
-            # be transferred; therefore, get current branch information
-            my $curr_iteminfo = GetItem($iteminformation->{'itemnumber'});
-            $iteminformation->{'homebranch'} = $curr_iteminfo->{'homebranch'};
-            $iteminformation->{'holdingbranch'} = $curr_iteminfo->{'holdingbranch'};
-            $iteminformation->{'itemlost'} = $curr_iteminfo->{'itemlost'};
-            $doreturn = 0;
-        }
-    
-        # check if the book is in a permanent collection....
-        my $hbr      = $iteminformation->{C4::Context->preference("HomeOrHoldingBranch")};
-        my $branches = GetBranches();
-               # FIXME -- This 'PE' attribute is largely undocumented.  afaict, there's no user interface that reflects this functionality.
-        if ( $hbr && $branches->{$hbr}->{'PE'} ) {
-            $messages->{'IsPermanent'} = $hbr;
-        }
-               
-                   # if independent branches are on and returning to different branch, refuse the return
-        if ($hbr ne C4::Context->userenv->{'branch'} && C4::Context->preference("IndependantBranches")){
-                         $messages->{'Wrongbranch'} = 1;
-                         $doreturn=0;
-                   }
-                       
-        # check that the book has been cancelled
-        if ( $iteminformation->{'wthdrawn'} ) {
-            $messages->{'wthdrawn'} = 1;
-            $doreturn = 0;
-        }
-    
-    #     new op dev : if the book returned in an other branch update the holding branch
-    
-    # update issues, thereby returning book (should push this out into another subroutine
-        $borrower = C4::Members::GetMemberDetails( $iteminformation->{borrowernumber}, 0 );
-    
-    # case of a return of document (deal with issues and holdingbranch)
-    
-        if ($doreturn) {
-                       my $circControlBranch;
-                       if($dropbox) {
-                               # don't allow dropbox mode to create an invalid entry in issues (issuedate > returndate) FIXME: actually checks eq, not gt
-                               undef($dropbox) if ( $iteminformation->{'issuedate'} eq C4::Dates->today('iso') );
-                               if (C4::Context->preference('CircControl') eq 'ItemHomeBranch' ) {
-                                       $circControlBranch = $iteminformation->{homebranch};
-                               } elsif ( C4::Context->preference('CircControl') eq 'PatronLibrary') {
-                                       $circControlBranch = $borrower->{branchcode};
-                               } else { # CircControl must be PickupLibrary.
-                                       $circControlBranch = $iteminformation->{holdingbranch};
-                                       # FIXME - is this right ? are we sure that the holdingbranch is still the pickup branch?
-                               }
-                       }
-            MarkIssueReturned($borrower->{'borrowernumber'}, $iteminformation->{'itemnumber'},$circControlBranch);
-            $messages->{'WasReturned'} = 1;    # FIXME is the "= 1" right?
+        $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.
+    }
 
-    
-            # continue to deal with returns cases, but not only if we have an issue
-        
-            # the holdingbranch is updated if the document is returned in an other location .
-            if ( $iteminformation->{'holdingbranch'} ne C4::Context->userenv->{'branch'} ) {
-                           UpdateHoldingbranch(C4::Context->userenv->{'branch'},$iteminformation->{'itemnumber'});
-                           #           reload iteminformation holdingbranch with the userenv value
-                           $iteminformation->{'holdingbranch'} = C4::Context->userenv->{'branch'};
-            }
-            ModDateLastSeen( $iteminformation->{'itemnumber'} );
-            ModItem({ onloan => undef }, $biblio->{'biblionumber'}, $iteminformation->{'itemnumber'});
-          
-                       if ($iteminformation->{borrowernumber}){
-                           ($borrower) = C4::Members::GetMemberDetails( $iteminformation->{borrowernumber}, 0 );
-            }
+    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 = $item->{C4::Context->preference("HomeOrHoldingBranch")} || '';
+        # item must be from items table -- issues table has branchcode and issuingbranch, not homebranch nor holdingbranch
+
+    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.
+        $branches->{$hbr}->{PE} and $messages->{'IsPermanent'} = $hbr;
+    }
+
+    # if indy branches and returning to different branch, refuse the return
+    if ($hbr ne $branch && C4::Context->preference("IndependantBranches")){
+        $messages->{'Wrongbranch'} = {
+            Wrongbranch => $branch,
+            Rightbranch => $hbr,
+        };
+        $doreturn = 0;
+        # bailing out here - in this case, current desired behavior
+        # is to act as if no return ever happened at all.
+        # FIXME - even in an indy branches situation, there should
+        # still be an option for the library to accept the item
+        # and transfer it to its owning library.
+        return ( $doreturn, $messages, $issue, $borrower );
+    }
+
+    if ( $item->{'wthdrawn'} ) { # book has been cancelled
+        $messages->{'wthdrawn'} = 1;
+        $doreturn = 0;
+    }
+
+    # case of a return of document (deal with issues and holdingbranch)
+    if ($doreturn) {
+        $borrower or warn "AddReturn without current borrower";
+               my $circControlBranch = _GetCircControlBranch($item,$borrower);
+        if ($dropbox) {
+            # don't allow dropbox mode to create an invalid entry in issues (issuedate > returndate) FIXME: actually checks eq, not gt
+            undef($dropbox) if ( $item->{'issuedate'} eq C4::Dates->today('iso') );
         }
-        # fix up the accounts.....
-        if ( $iteminformation->{'itemlost'} ) {
-            $messages->{'WasLost'} = 1;
+
+        if ($borrowernumber) {
+            MarkIssueReturned($borrowernumber, $item->{'itemnumber'}, $circControlBranch);
+            $messages->{'WasReturned'} = 1;    # FIXME is the "= 1" right?  This could be the borrower hash.
         }
-    
-    # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #
-    #     check if we have a transfer for this document
-        my ($datesent,$frombranch,$tobranch) = GetTransfers( $iteminformation->{'itemnumber'} );
-    
-    #     if we have a transfer to do, we update the line of transfers with the datearrived
-        if ($datesent) {
-            if ( $tobranch eq C4::Context->userenv->{'branch'} ) {
-                    my $sth =
-                    $dbh->prepare(
-                            "UPDATE branchtransfers SET datearrived = now() WHERE itemnumber= ? AND datearrived IS NULL"
-                    );
-                    $sth->execute( $iteminformation->{'itemnumber'} );
-                    $sth->finish;
-    #         now we check if there is a reservation with the validate of transfer if we have one, we can         set it with the status 'W'
-            C4::Reserves::ModReserveStatus( $iteminformation->{'itemnumber'},'W' );
-            }
-        else {
-            $messages->{'WrongTransfer'} = $tobranch;
-            $messages->{'WrongTransferItem'} = $iteminformation->{'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'});
+        $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'} );
+
+    # 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"
+            );
+            $sth->execute( $item->{'itemnumber'} );
+            # if we have a reservation with valid transfer, we can set it's status to 'W'
+            C4::Reserves::ModReserveStatus($item->{'itemnumber'}, 'W');
+        } else {
+            $messages->{'WrongTransfer'}     = $tobranch;
+            $messages->{'WrongTransferItem'} = $item->{'itemnumber'};
         }
         $validTransfert = 1;
-        }
-    
-    # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # 
-        # fix up the accounts.....
-        if ($iteminformation->{'itemlost'}) {
-                FixAccountForLostAndReturned($iteminformation, $borrower);
-                $messages->{'WasLost'} = 1;
-        }
-        # fix up the overdues in accounts...
-        FixOverduesOnReturn( $borrower->{'borrowernumber'},
-            $iteminformation->{'itemnumber'}, $exemptfine, $dropbox );
-    
+    }
+
+    # fix up the accounts.....
+    if ($item->{'itemlost'}) {
+        _FixAccountForLostAndReturned($item->{'itemnumber'}, $borrowernumber, $barcode);    # can tolerate undef $borrowernumber
+        $messages->{'WasLost'} = 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
+    }
+
     # find reserves.....
-    #     if we don't have a reserve with the status W, we launch the Checkreserves routine
-        my ( $resfound, $resrec ) =
-        C4::Reserves::CheckReserves( $iteminformation->{'itemnumber'} );
-        if ($resfound) {
-            $resrec->{'ResFound'}   = $resfound;
-            $messages->{'ResFound'} = $resrec;
-            $reserveDone = 1;
-        }
-    
-        # update stats?
-        # Record the fact that this book was returned.
-        UpdateStats(
-            $branch, 'return', '0', '',
-            $iteminformation->{'itemnumber'},
-            $biblio->{'itemtype'},
-            $borrower->{'borrowernumber'}
-        );
+    # if we don't have a reserve with the status W, we launch the Checkreserves routine
+    my ($resfound, $resrec) = C4::Reserves::CheckReserves( $item->{'itemnumber'} );
+    if ($resfound) {
+          $resrec->{'ResFound'} = $resfound;
+        $messages->{'ResFound'} = $resrec;
+    }
 
-        # Send a check-in slip.
-        my $circulation_alert = 'C4::ItemCirculationAlertPreference';
-        my %conditions = (
-            branchcode   => $branch,
-            categorycode => $borrower->{categorycode},
-            item_type    => $iteminformation->{itype},
-            notification => 'CHECKIN',
-        );
-        if ($doreturn && $circulation_alert->is_enabled_for(\%conditions)) {
-            SendCirculationAlert({
-                type     => 'CHECKIN',
-                item     => $iteminformation,
-                borrower => $borrower,
-                branch   => $branch,
-            });
-        }
-        
-        logaction("CIRCULATION", "RETURN", $iteminformation->{borrowernumber}, $iteminformation->{'biblionumber'}) 
-            if C4::Context->preference("ReturnLog");
-        
-        #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 and ($branch ne $iteminformation->{'homebranch'}) and not $messages->{'WrongTransfer'} and ($validTransfert ne 1) and ($reserveDone ne 1) ){
-                       if (C4::Context->preference("AutomaticItemReturn") == 1) {
-                               ModItemTransfer($iteminformation->{'itemnumber'}, C4::Context->userenv->{'branch'}, $iteminformation->{'homebranch'});
-                               $messages->{'WasTransfered'} = 1;
-                       } elsif ( C4::Context->preference("UseBranchTransferLimits") == 1 
-                                       && ! IsBranchTransferAllowed( $branch, $iteminformation->{'homebranch'}, $iteminformation->{ C4::Context->preference("BranchTransferLimitsType") } )
-                               ) {
-                               ModItemTransfer($iteminformation->{'itemnumber'}, C4::Context->userenv->{'branch'}, $iteminformation->{'homebranch'});
-                                $messages->{'WasTransfered'} = 1;
-                       }
-                       else {
-                               $messages->{'NeedsTransfer'} = 1;
-                       }
+    # update stats?
+    # Record the fact that this book was returned.
+    UpdateStats(
+        $branch, 'return', '0', '',
+        $item->{'itemnumber'},
+        $biblio->{'itemtype'},
+        $borrowernumber
+    );
+
+    # 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 = (
+        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,
+        });
+    }
+    
+    logaction("CIRCULATION", "RETURN", $borrowernumber, $item->{'biblionumber'})
+        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 and ($branch ne $hbr) and not $messages->{'WrongTransfer'} and ($validTransfert ne 1) ){
+        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);
+            $messages->{'WasTransfered'} = 1;
+        } else {
+            $messages->{'NeedsTransfer'} = 1;   # TODO: instead of 1, specify branchcode that the transfer SHOULD go to, $item->{homebranch}
         }
     }
-    return ( $doreturn, $messages, $iteminformation, $borrower );
+    return ( $doreturn, $messages, $issue, $borrower );
 }
 
 =head2 MarkIssueReturned
@@ -1631,9 +1633,9 @@ sub MarkIssueReturned {
     $sth_del->execute($borrowernumber, $itemnumber);
 }
 
-=head2 FixOverduesOnReturn
+=head2 _FixOverduesOnReturn
 
-    &FixOverduesOnReturn($brn,$itm, $exemptfine, $dropboxmode);
+    &_FixOverduesOnReturn($brn,$itm, $exemptfine, $dropboxmode);
 
 C<$brn> borrowernumber
 
@@ -1642,193 +1644,214 @@ C<$itm> itemnumber
 C<$exemptfine> BOOL -- remove overdue charge associated with this issue. 
 C<$dropboxmode> BOOL -- remove lastincrement on overdue charge associated with this issue.
 
-internal function, called only by AddReturn
+Internal function, called only by AddReturn
 
 =cut
 
-sub FixOverduesOnReturn {
-    my ( $borrowernumber, $item, $exemptfine, $dropbox ) = @_;
+sub _FixOverduesOnReturn {
+    my ($borrowernumber, $item);
+    unless ($borrowernumber = shift) {
+        warn "_FixOverduesOnReturn() not supplied valid borrowernumber";
+        return;
+    }
+    unless ($item = shift) {
+        warn "_FixOverduesOnReturn() not supplied valid itemnumber";
+        return;
+    }
+    my ($exemptfine, $dropbox) = @_;
     my $dbh = C4::Context->dbh;
 
     # check for overdue fine
-    my $sth =
-      $dbh->prepare(
+    my $sth = $dbh->prepare(
 "SELECT * FROM accountlines WHERE (borrowernumber = ?) AND (itemnumber = ?) AND (accounttype='FU' OR accounttype='O')"
-      );
+    );
     $sth->execute( $borrowernumber, $item );
 
     # alter fine to show that the book has been returned
-   my $data; 
-       if ($data = $sth->fetchrow_hashref) {
-        my $uquery;
-               my @bind = ($borrowernumber,$item ,$data->{'accountno'});
-               if ($exemptfine) {
-                       $uquery = "update accountlines set accounttype='FFOR', amountoutstanding=0";
-                       if (C4::Context->preference("FinesLog")) {
-                       &logaction("FINES", 'MODIFY',$borrowernumber,"Overdue forgiven: item $item");
-                       }
-               } elsif ($dropbox && $data->{lastincrement}) {
-                       my $outstanding = $data->{amountoutstanding} - $data->{lastincrement} ;
-                       my $amt = $data->{amount} - $data->{lastincrement} ;
-                       if (C4::Context->preference("FinesLog")) {
-                       &logaction("FINES", 'MODIFY',$borrowernumber,"Dropbox adjustment $amt, item $item");
-                       }
-                        $uquery = "update accountlines set accounttype='F' ";
-                        if($outstanding  >= 0 && $amt >=0) {
-                               $uquery .= ", amount = ? , amountoutstanding=? ";
-                               unshift @bind, ($amt, $outstanding) ;
-                       }
-               } else {
-                       $uquery = "update accountlines set accounttype='F' ";
-               }
-               $uquery .= " where (borrowernumber = ?) and (itemnumber = ?) and (accountno = ?)";
-        my $usth = $dbh->prepare($uquery);
-        $usth->execute(@bind);
-        $usth->finish();
+    my $data = $sth->fetchrow_hashref;
+    return 0 unless $data;    # no warning, there's just nothing to fix
+
+    my $uquery;
+    my @bind = ($borrowernumber, $item, $data->{'accountno'});
+    if ($exemptfine) {
+        $uquery = "update accountlines set accounttype='FFOR', amountoutstanding=0";
+        if (C4::Context->preference("FinesLog")) {
+            &logaction("FINES", 'MODIFY',$borrowernumber,"Overdue forgiven: item $item");
+        }
+    } elsif ($dropbox && $data->{lastincrement}) {
+        my $outstanding = $data->{amountoutstanding} - $data->{lastincrement} ;
+        my $amt = $data->{amount} - $data->{lastincrement} ;
+        if (C4::Context->preference("FinesLog")) {
+            &logaction("FINES", 'MODIFY',$borrowernumber,"Dropbox adjustment $amt, item $item");
+        }
+         $uquery = "update accountlines set accounttype='F' ";
+         if($outstanding  >= 0 && $amt >=0) {
+            $uquery .= ", amount = ? , amountoutstanding=? ";
+            unshift @bind, ($amt, $outstanding) ;
+        }
+    } else {
+        $uquery = "update accountlines set accounttype='F' ";
     }
+    $uquery .= " where (borrowernumber = ?) and (itemnumber = ?) and (accountno = ?)";
+    my $usth = $dbh->prepare($uquery);
+    return $usth->execute(@bind);
+}
+
+=head2 _FixAccountForLostAndReturned
+
+       &_FixAccountForLostAndReturned($itemnumber, [$borrowernumber, $barcode]);
+
+Calculates the charge for a book lost and returned.
+
+Internal function, not exported, called only by AddReturn.
+
+FIXME: This function reflects how inscrutable fines logic is.  Fix both.
+FIXME: Give a positive return value on success.  It might be the $borrowernumber who received credit, or the amount forgiven.
+
+=cut
 
-    $sth->finish();
+sub _FixAccountForLostAndReturned {
+    my $itemnumber     = shift or return;
+    my $borrowernumber = @_ ? shift : undef;
+    my $item_id        = @_ ? shift : $itemnumber;  # Send the barcode if you want that logged in the description
+    my $dbh = C4::Context->dbh;
+    # check for charge made for lost book
+    my $sth = $dbh->prepare("SELECT * FROM accountlines WHERE (itemnumber = ?) AND (accounttype='L' OR accounttype='Rep') ORDER BY date DESC");
+    $sth->execute($itemnumber);
+    my $data = $sth->fetchrow_hashref;
+    $data or return;    # bail if there is nothing to do
+
+    # writeoff this amount
+    my $offset;
+    my $amount = $data->{'amount'};
+    my $acctno = $data->{'accountno'};
+    my $amountleft;                                             # Starts off undef/zero.
+    if ($data->{'amountoutstanding'} == $amount) {
+        $offset     = $data->{'amount'};
+        $amountleft = 0;                                        # Hey, it's zero here, too.
+    } else {
+        $offset     = $amount - $data->{'amountoutstanding'};   # Um, isn't this the same as ZERO?  We just tested those two things are ==
+        $amountleft = $data->{'amountoutstanding'} - $amount;   # Um, isn't this the same as ZERO?  We just tested those two things are ==
+    }
+    my $usth = $dbh->prepare("UPDATE accountlines SET accounttype = 'LR',amountoutstanding='0'
+        WHERE (borrowernumber = ?)
+        AND (itemnumber = ?) AND (accountno = ?) ");
+    $usth->execute($data->{'borrowernumber'},$itemnumber,$acctno);      # We might be adjusting an account for some OTHER borrowernumber now.  Not the one we passed in.  
+    #check if any credit is left if so writeoff other accounts
+    my $nextaccntno = getnextacctno($data->{'borrowernumber'});
+    $amountleft *= -1 if ($amountleft < 0);
+    if ($amountleft > 0) {
+        my $msth = $dbh->prepare("SELECT * FROM accountlines WHERE (borrowernumber = ?)
+                            AND (amountoutstanding >0) ORDER BY date");     # might want to order by amountoustanding ASC (pay smallest first)
+        $msth->execute($data->{'borrowernumber'});
+        # offset transactions
+        my $newamtos;
+        my $accdata;
+        while (($accdata=$msth->fetchrow_hashref) and ($amountleft>0)){
+            if ($accdata->{'amountoutstanding'} < $amountleft) {
+                $newamtos = 0;
+                $amountleft -= $accdata->{'amountoutstanding'};
+            }  else {
+                $newamtos = $accdata->{'amountoutstanding'} - $amountleft;
+                $amountleft = 0;
+            }
+            my $thisacct = $accdata->{'accountno'};
+            # FIXME: move prepares outside while loop!
+            my $usth = $dbh->prepare("UPDATE accountlines SET amountoutstanding= ?
+                    WHERE (borrowernumber = ?)
+                    AND (accountno=?)");
+            $usth->execute($newamtos,$data->{'borrowernumber'},'$thisacct');    # FIXME: '$thisacct' is a string literal!
+            $usth = $dbh->prepare("INSERT INTO accountoffsets
+                (borrowernumber, accountno, offsetaccount,  offsetamount)
+                VALUES
+                (?,?,?,?)");
+            $usth->execute($data->{'borrowernumber'},$accdata->{'accountno'},$nextaccntno,$newamtos);
+        }
+        $msth->finish;  # $msth might actually have data left
+    }
+    $amountleft *= -1 if ($amountleft > 0);
+    my $desc = "Item Returned " . $item_id;
+    $usth = $dbh->prepare("INSERT INTO accountlines
+        (borrowernumber,accountno,date,amount,description,accounttype,amountoutstanding)
+        VALUES (?,?,now(),?,?,'CR',?)");
+    $usth->execute($data->{'borrowernumber'},$nextaccntno,0-$amount,$desc,$amountleft);
+    if ($borrowernumber) {
+        # FIXME: same as query above.  use 1 sth for both
+        $usth = $dbh->prepare("INSERT INTO accountoffsets
+            (borrowernumber, accountno, offsetaccount,  offsetamount)
+            VALUES (?,?,?,?)");
+        $usth->execute($borrowernumber, $data->{'accountno'}, $nextaccntno, $offset);
+    }
+    ModItem({ paidfor => '' }, undef, $itemnumber);
     return;
 }
 
-=head2 FixAccountForLostAndReturned
+=head2 _GetCircControlBranch
 
-       &FixAccountForLostAndReturned($iteminfo,$borrower);
+   my $circ_control_branch = _GetCircControlBranch($iteminfos, $borrower);
 
-Calculates the charge for a book lost and returned (Not exported & used only once)
+Internal function : 
 
-C<$iteminfo> is a hashref to iteminfo. Only {itemnumber} is used.
+Return the library code to be used to determine which circulation
+policy applies to a transaction.  Looks up the CircControl and
+HomeOrHoldingBranch system preferences.
 
-C<$borrower> is a hashref to borrower. Only {borrowernumber is used.
+C<$iteminfos> is a hashref to iteminfo. Only {homebranch or holdingbranch} is used.
 
-Internal function, called by AddReturn
+C<$borrower> is a hashref to borrower. Only {branchcode} is used.
 
 =cut
 
-sub FixAccountForLostAndReturned {
-       my ($iteminfo, $borrower) = @_;
-       my $dbh = C4::Context->dbh;
-       my $itm = $iteminfo->{'itemnumber'};
-       # check for charge made for lost book
-       my $sth = $dbh->prepare("SELECT * FROM accountlines WHERE (itemnumber = ?) AND (accounttype='L' OR accounttype='Rep') ORDER BY date DESC");
-       $sth->execute($itm);
-       if (my $data = $sth->fetchrow_hashref) {
-       # writeoff this amount
-               my $offset;
-               my $amount = $data->{'amount'};
-               my $acctno = $data->{'accountno'};
-               my $amountleft;
-               if ($data->{'amountoutstanding'} == $amount) {
-               $offset = $data->{'amount'};
-               $amountleft = 0;
-               } else {
-               $offset = $amount - $data->{'amountoutstanding'};
-               $amountleft = $data->{'amountoutstanding'} - $amount;
-               }
-               my $usth = $dbh->prepare("UPDATE accountlines SET accounttype = 'LR',amountoutstanding='0'
-                       WHERE (borrowernumber = ?)
-                       AND (itemnumber = ?) AND (accountno = ?) ");
-               $usth->execute($data->{'borrowernumber'},$itm,$acctno);
-               $usth->finish;
-       #check if any credit is left if so writeoff other accounts
-               my $nextaccntno = getnextacctno($data->{'borrowernumber'});
-               if ($amountleft < 0){
-               $amountleft*=-1;
-               }
-               if ($amountleft > 0){
-               my $msth = $dbh->prepare("SELECT * FROM accountlines WHERE (borrowernumber = ?)
-                                                       AND (amountoutstanding >0) ORDER BY date");
-               $msth->execute($data->{'borrowernumber'});
-       # offset transactions
-               my $newamtos;
-               my $accdata;
-               while (($accdata=$msth->fetchrow_hashref) and ($amountleft>0)){
-                       if ($accdata->{'amountoutstanding'} < $amountleft) {
-                       $newamtos = 0;
-                       $amountleft -= $accdata->{'amountoutstanding'};
-                       }  else {
-                       $newamtos = $accdata->{'amountoutstanding'} - $amountleft;
-                       $amountleft = 0;
-                       }
-                       my $thisacct = $accdata->{'accountno'};
-                       my $usth = $dbh->prepare("UPDATE accountlines SET amountoutstanding= ?
-                                       WHERE (borrowernumber = ?)
-                                       AND (accountno=?)");
-                       $usth->execute($newamtos,$data->{'borrowernumber'},'$thisacct');
-                       $usth->finish;
-                       $usth = $dbh->prepare("INSERT INTO accountoffsets
-                               (borrowernumber, accountno, offsetaccount,  offsetamount)
-                               VALUES
-                               (?,?,?,?)");
-                       $usth->execute($data->{'borrowernumber'},$accdata->{'accountno'},$nextaccntno,$newamtos);
-                       $usth->finish;
-               }
-               $msth->finish;
-               }
-               if ($amountleft > 0){
-                       $amountleft*=-1;
-               }
-               my $desc="Item Returned ".$iteminfo->{'barcode'};
-               $usth = $dbh->prepare("INSERT INTO accountlines
-                       (borrowernumber,accountno,date,amount,description,accounttype,amountoutstanding)
-                       VALUES (?,?,now(),?,?,'CR',?)");
-               $usth->execute($data->{'borrowernumber'},$nextaccntno,0-$amount,$desc,$amountleft);
-               $usth->finish;
-               $usth = $dbh->prepare("INSERT INTO accountoffsets
-                       (borrowernumber, accountno, offsetaccount,  offsetamount)
-                       VALUES (?,?,?,?)");
-               $usth->execute($borrower->{'borrowernumber'},$data->{'accountno'},$nextaccntno,$offset);
-               $usth->finish;
-        ModItem({ paidfor => '' }, undef, $itm);
-       }
-       $sth->finish;
-       return;
+sub _GetCircControlBranch {
+    my ($item, $borrower) = @_;
+    my $circcontrol = C4::Context->preference('CircControl');
+    my $branch;
+
+    if ($circcontrol eq 'PickupLibrary') {
+        $branch= C4::Context->userenv->{'branch'};
+    } elsif ($circcontrol eq 'PatronLibrary') {
+        $branch=$borrower->{branchcode};
+    } else {
+        my $branchfield = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch';
+        $branch = $item->{$branchfield};
+        # default to item home branch if holdingbranch is used
+        # and is not defined
+        if (!defined($branch) && $branchfield eq 'holdingbranch') {
+            $branch = $item->{homebranch};
+        }
+    }
+    return $branch;
 }
 
-=head2 GetItemIssue
 
-$issues = &GetItemIssue($itemnumber);
 
-Returns patrons currently having a book. nothing if item is not issued atm
 
-C<$itemnumber> is the itemnumber
 
-Returns an array of hashes
 
-FIXME: Though the above says that this function returns nothing if the
-item is not issued, this actually returns a hasref that looks like
-this:
-    {
-      itemnumber => 1,
-      overdue    => 1
-    }
+=head2 GetItemIssue
+
+$issue = &GetItemIssue($itemnumber);
 
+Returns patron currently having a book, or undef if not checked out.
+
+C<$itemnumber> is the itemnumber.
+
+C<$issue> is a hashref of the row from the issues table.
 
 =cut
 
 sub GetItemIssue {
-    my ( $itemnumber) = @_;
+    my ($itemnumber) = @_;
     return unless $itemnumber;
-    my $dbh = C4::Context->dbh;
-    my @GetItemIssues;
-    
-    # get today date
-    my $today = POSIX::strftime("%Y%m%d", localtime);
-
-    my $sth = $dbh->prepare(
-        "SELECT * FROM issues 
+    my $sth = C4::Context->dbh->prepare(
+        "SELECT *
+        FROM issues 
         LEFT JOIN items ON issues.itemnumber=items.itemnumber
-    WHERE
-    issues.itemnumber=?");
+        WHERE issues.itemnumber=?");
     $sth->execute($itemnumber);
     my $data = $sth->fetchrow_hashref;
-    my $datedue = $data->{'date_due'};
-    $datedue =~ s/-//g;
-    if ( $datedue < $today ) {
-        $data->{'overdue'} = 1;
-    }
-    $data->{'itemnumber'} = $itemnumber; # fill itemnumber, in case item is not on issue
-    $sth->finish;
+    return unless $data;
+    $data->{'overdue'} = ($data->{'date_due'} lt C4::Dates->today('iso')) ? 1 : 0;
     return ($data);
 }
 
@@ -1861,23 +1884,20 @@ $issues = &GetItemIssues($itemnumber, $history);
 Returns patrons that have issued a book
 
 C<$itemnumber> is the itemnumber
-C<$history> is 0 if you want actuel "issuer" (if it exist) and 1 if you want issues history
+C<$history> is false if you just want the current "issuer" (if any)
+and true if you want issues history from old_issues also.
 
-Returns an array of hashes
+Returns reference to an array of hashes
 
 =cut
 
 sub GetItemIssues {
-    my ( $itemnumber,$history ) = @_;
-    my $dbh = C4::Context->dbh;
-    my @GetItemIssues;
+    my ( $itemnumber, $history ) = @_;
     
-    # get today date
-    my $today = POSIX::strftime("%Y%m%d", localtime);
-
+    my $today = C4::Dates->today('iso');  # get today date
     my $sql = "SELECT * FROM issues 
               JOIN borrowers USING (borrowernumber)
-              JOIN items USING (itemnumber)
+              JOIN items     USING (itemnumber)
               WHERE issues.itemnumber = ? ";
     if ($history) {
         $sql .= "UNION ALL
@@ -1887,23 +1907,17 @@ sub GetItemIssues {
                  WHERE old_issues.itemnumber = ? ";
     }
     $sql .= "ORDER BY date_due DESC";
-    my $sth = $dbh->prepare($sql);
+    my $sth = C4::Context->dbh->prepare($sql);
     if ($history) {
         $sth->execute($itemnumber, $itemnumber);
     } else {
         $sth->execute($itemnumber);
     }
-    while ( my $data = $sth->fetchrow_hashref ) {
-        my $datedue = $data->{'date_due'};
-        $datedue =~ s/-//g;
-        if ( $datedue < $today ) {
-            $data->{'overdue'} = 1;
-        }
-        my $itemnumber = $data->{'itemnumber'};
-        push @GetItemIssues, $data;
+    my $results = $sth->fetchall_arrayref({});
+    foreach (@$results) {
+        $_->{'overdue'} = ($_->{'date_due'} lt $today) ? 1 : 0;
     }
-    $sth->finish;
-    return ( \@GetItemIssues );
+    return $results;
 }
 
 =head2 GetBiblioIssues
@@ -2075,7 +2089,8 @@ has the item.
 
 C<$itemnumber> is the number of the item to renew.
 
-C<$branch> is the library branch.  Defaults to the homebranch of the ITEM.
+C<$branch> is the library where the renewal took place (if any).
+           The library that controls the circ policies for the renewal is retrieved from the issues record.
 
 C<$datedue> can be a C4::Dates object used to set the due date.
 
@@ -2088,33 +2103,13 @@ from the book's item type.
 =cut
 
 sub AddRenewal {
-       my $borrowernumber = shift or return undef;
-       my     $itemnumber = shift or return undef;
+    my $borrowernumber  = shift or return undef;
+    my $itemnumber      = shift or return undef;
+    my $branch          = shift;
+    my $datedue         = shift;
+    my $lastreneweddate = shift || C4::Dates->new()->output('iso');
     my $item   = GetItem($itemnumber) or return undef;
     my $biblio = GetBiblioFromItemNumber($itemnumber) or return undef;
-    my $branch  = (@_) ? shift : $item->{homebranch};  # opac-renew doesn't send branch
-    my $datedue = shift;
-    my $lastreneweddate = shift;
-
-    # If the due date wasn't specified, calculate it by adding the
-    # book's loan length to today's date.
-    unless ($datedue && $datedue->output('iso')) {
-
-        my $borrower = C4::Members::GetMemberDetails( $borrowernumber, 0 ) or return undef;
-        my $loanlength = GetLoanLength(
-            $borrower->{'categorycode'},
-             (C4::Context->preference('item-level_itypes')) ? $biblio->{'itype'} : $biblio->{'itemtype'} ,
-                       $item->{homebranch}                     # item's homebranch determines loanlength OR do we want the branch specified by the AddRenewal argument?
-        );
-               #FIXME -- use circControl?
-               $datedue =  CalcDateDue(C4::Dates->new(),$loanlength,$branch,$borrower);        # this branch is the transactional branch.
-                                                               # The question of whether to use item's homebranch calendar is open.
-    }
-
-    # $lastreneweddate defaults to today.
-    unless (defined $lastreneweddate) {
-        $lastreneweddate = strftime( "%Y-%m-%d", localtime );
-    }
 
     my $dbh = C4::Context->dbh;
     # Find the issues record for this book
@@ -2126,24 +2121,25 @@ sub AddRenewal {
     $sth->execute( $borrowernumber, $itemnumber );
     my $issuedata = $sth->fetchrow_hashref;
     $sth->finish;
-
+    if($datedue && ! $datedue->output('iso')){
+        warn "Invalid date passed to AddRenewal.";
+        return undef;
+    }
     # If the due date wasn't specified, calculate it by adding the
-    # book's loan length to due's date.
-    unless (@_ and $datedue = shift and $datedue->output('iso')) {
+    # book's loan length to today's date or the current due date
+    # based on the value of the RenewalPeriodBase syspref.
+    unless ($datedue) {
 
         my $borrower = C4::Members::GetMemberDetails( $borrowernumber, 0 ) or return undef;
         my $loanlength = GetLoanLength(
-            $borrower->{'categorycode'},
-             (C4::Context->preference('item-level_itypes')) ? $biblio->{'itype'} : $biblio->{'itemtype'} ,
-                       $item->{homebranch}                     # item's homebranch determines loanlength OR do we want the branch specified by the AddRenewal argument?
-        );
+                    $borrower->{'categorycode'},
+                    (C4::Context->preference('item-level_itypes')) ? $biblio->{'itype'} : $biblio->{'itemtype'} ,
+                               $issuedata->{'branchcode'}  );   # that's the circ control branch.
 
         $datedue = (C4::Context->preference('RenewalPeriodBase') eq 'date_due') ?
                                         C4::Dates->new($issuedata->{date_due}, 'iso') :
                                         C4::Dates->new();
-        #FIXME -- use circControl?
-        $datedue =  CalcDateDue($datedue,$loanlength,$branch); # this branch is the transactional branch.
-        # The question of whether to use item's homebranch calendar is open.
+        $datedue =  CalcDateDue($datedue,$loanlength,$issuedata->{'branchcode'},$borrower);
     }
 
     # Update the issues record to have the new due date, and a new count
@@ -2537,9 +2533,11 @@ sub CalcDateDue {
 
        # if ceilingDueDate ON the datedue can't be after the ceiling date
        if ( C4::Context->preference('ceilingDueDate')
-            && ( C4::Context->preference('ceilingDueDate') =~ C4::Dates->regexp('syspref') )
-            && $datedue->output gt C4::Context->preference('ceilingDueDate') ) {
-           $datedue = C4::Dates->new( C4::Context->preference('ceilingDueDate') );
+             && ( C4::Context->preference('ceilingDueDate') =~ C4::Dates->regexp('syspref') ) ) {
+            my $ceilingDate = C4::Dates->new( C4::Context->preference('ceilingDueDate') );
+            if ( $datedue->output( 'iso' ) gt $ceilingDate->output( 'iso' ) ) {
+                $datedue = $ceilingDate;
+            }
        }
 
        return $datedue;