BugFixing : 3306
[koha.git] / C4 / Circulation.pm
index e490e6f..7c9fe89 100644 (file)
@@ -318,16 +318,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
@@ -648,10 +639,27 @@ sub CanBookBeIssued {
        $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;
+
     #
     # DUE DATE is OK ? -- should already have checked.
     #
-    #$issuingimpossible{INVALID_DATE} = 1 unless ($duedate);
+    unless ( $duedate ) {
+        my $issuedate = strftime( "%Y-%m-%d", localtime );
+
+        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 );
+
+        # Offline circ calls AddIssue directly, doesn't run through here
+        #  So issuingimpossible should be ok.
+    }
+    $issuingimpossible{INVALID_DATE} = $duedate->output('syspref') unless ( $duedate && $duedate->output('iso') ge C4::Dates->today('iso') );
 
     #
     # BORROWER STATUS
@@ -704,7 +712,7 @@ sub CanBookBeIssued {
     my ($blocktype, $count) = C4::Members::IsMemberBlocked($borrower->{'borrowernumber'});
     if($blocktype == -1){
         ## remaining overdue documents
-        $issuingimpossible{USERBLOCKEDREMAINING} = $count;
+        $needsconfirmation{USERBLOCKEDREMAINING} = $count;
     }elsif($blocktype == 1){
         ## blocked because of overdue return
         $issuingimpossible{USERBLOCKEDOVERDUE} = $count;
@@ -714,7 +722,12 @@ sub CanBookBeIssued {
     # JB34 CHECKS IF BORROWERS DONT HAVE ISSUE TOO MANY BOOKS
     #
        my $toomany = TooMany( $borrower, $item->{biblionumber}, $item );
-    $needsconfirmation{TOO_MANY} = $toomany if $toomany;
+    # if TooMany return / 0, then the user has no permission to check out this book
+    if ($toomany =~ /\/ 0/) {
+        $needsconfirmation{PATRON_CANT} = 1;
+    } else {
+        $needsconfirmation{TOO_MANY} = $toomany if $toomany;
+    }
 
     #
     # ITEM CHECKING
@@ -726,27 +739,35 @@ sub CanBookBeIssued {
         && $item->{'notforloan'} > 0 )
     {
         if(C4::Context->preference("AllowNotForLoanOverride")){
-            $issuingimpossible{NOT_FOR_LOAN_CAN_FORCE} = 1;
+           $needsconfirmation{NOT_FOR_LOAN_FORCING} = 1;
         }else{
             $issuingimpossible{NOT_FOR_LOAN} = 1;
         }
     }
-       elsif ( !$item->{'notforloan'} ){
-               # we have to check itemtypes.notforloan also
-               if (C4::Context->preference('item-level_itypes')){
-                       # this should probably be a subroutine
-                       my $sth = $dbh->prepare("SELECT notforloan FROM itemtypes WHERE itemtype = ?");
-                       $sth->execute($item->{'itemtype'});
-                       my $notforloan=$sth->fetchrow_hashref();
-                       $sth->finish();
-                       if ($notforloan->{'notforloan'} == 1){
-                               $issuingimpossible{NOT_FOR_LOAN} = 1;                           
-                       }
-               }
-               elsif ($biblioitem->{'notforloan'} == 1){
-                       $issuingimpossible{NOT_FOR_LOAN} = 1;
-               }
-       }
+    elsif ( !$item->{'notforloan'} ){
+        # we have to check itemtypes.notforloan also
+        if (C4::Context->preference('item-level_itypes')){
+            # this should probably be a subroutine
+            my $sth = $dbh->prepare("SELECT notforloan FROM itemtypes WHERE itemtype = ?");
+            $sth->execute($item->{'itemtype'});
+            my $notforloan=$sth->fetchrow_hashref();
+            $sth->finish();
+            if ($notforloan->{'notforloan'}) {
+                if (!C4::Context->preference("AllowNotForLoanOverride")) {
+                    $issuingimpossible{NOT_FOR_LOAN} = 1;
+                } else {
+                    $needsconfirmation{NOT_FOR_LOAN_FORCING} = 1;
+                }
+            }
+        }
+        elsif ($biblioitem->{'notforloan'} == 1){
+            if (!C4::Context->preference("AllowNotForLoanOverride")) {
+                $issuingimpossible{NOT_FOR_LOAN} = 1;
+            } else {
+                $needsconfirmation{NOT_FOR_LOAN_FORCING} = 1;
+            }
+        }
+    }
     if ( $item->{'wthdrawn'} && $item->{'wthdrawn'} == 1 )
     {
         $issuingimpossible{WTHDRAWN} = 1;
@@ -786,7 +807,7 @@ sub CanBookBeIssued {
     elsif ($issue->{borrowernumber}) {
 
         # issued to someone else
-        my $currborinfo = GetMemberDetails( $issue->{borrowernumber} );
+        my $currborinfo =    C4::Members::GetMemberDetails( $issue->{borrowernumber} );
 
 #        warn "=>.$currborinfo->{'firstname'} $currborinfo->{'surname'} ($currborinfo->{'cardnumber'})";
         $needsconfirmation{ISSUED_TO_ANOTHER} =
@@ -813,12 +834,6 @@ sub CanBookBeIssued {
 "$res->{'reservedate'} : $resborrower->{'firstname'} $resborrower->{'surname'} ($resborrower->{'cardnumber'})";
         }
     }
-    if ( C4::Context->preference("LibraryName") eq "Horowhenua Library Trust" ) {
-        if ( $borrower->{'categorycode'} eq 'W' ) {
-            my %emptyhash;
-            return ( \%emptyhash, \%needsconfirmation );
-        }
-       }
        return ( \%issuingimpossible, \%needsconfirmation );
 }
 
@@ -873,9 +888,8 @@ 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 $hbr = C4::Context->preference("HomeOrHoldingBranch")||"homebranch";
+               my $branch = _GetCircControlBranch($item,$borrower);
                
                # get actual issuing if there is one
                my $actualissue = GetItemIssue( $item->{itemnumber});
@@ -965,7 +979,7 @@ sub AddIssue {
         unless ($datedue) {
             my $itype = ( C4::Context->preference('item-level_itypes') ) ? $biblio->{'itype'} : $biblio->{'itemtype'};
             my $loanlength = GetLoanLength( $borrower->{'categorycode'}, $itype, $branch );
-            $datedue = CalcDateDue( C4::Dates->new( $issuedate, 'iso' ), $loanlength, $branch );
+            $datedue = CalcDateDue( C4::Dates->new( $issuedate, 'iso' ), $loanlength, $branch, $borrower );
 
             # if ReturnBeforeExpiry ON the datedue can't be after borrower expirydate
             if ( C4::Context->preference('ReturnBeforeExpiry') && $datedue->output('iso') gt $borrower->{dateexpiry} ) {
@@ -977,12 +991,11 @@ sub AddIssue {
             $item->{'itemnumber'},              # itemnumber
             $issuedate,                         # issuedate
             $datedue->output('iso'),            # date_due
-            C4::Context->userenv->{'branch'}    # branchcode
+            $branch                                                            # branchcode
         );
-        $sth->finish;
         $item->{'issues'}++;
         ModItem({ issues           => $item->{'issues'},
-                  holdingbranch    => C4::Context->userenv->{'branch'},
+                  holdingbranch    => C4::Context->userenv->{branch},
                   itemlost         => 0,
                   datelastborrowed => C4::Dates->new()->output('iso'),
                   onloan           => $datedue->output('iso'),
@@ -1280,6 +1293,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.
 
@@ -1293,26 +1309,30 @@ sub AddReturn {
     my $borrower;
     my $validTransfert = 0;
     my $reserveDone = 0;
+       $branch ||=C4::Context->userenv->{'branch'};
     
     # get information on item
-    my $iteminformation = GetItemIssue( GetItemnumberFromBarcode($barcode));
+    my $itemnumber = GetItemnumberFromBarcode($barcode);
+    my $iteminformation = GetItemIssue( $itemnumber );
     my $biblio = GetBiblioItemData($iteminformation->{'biblioitemnumber'});
 #     use Data::Dumper;warn Data::Dumper::Dumper($iteminformation);  
-    unless ($iteminformation->{'itemnumber'} ) {
+    unless ( $iteminformation->{'itemnumber'} or $itemnumber) {
         $messages->{'BadBarcode'} = $barcode;
         $doreturn = 0;
     } else {
         # find the borrower
-        if ( ( not $iteminformation->{borrowernumber} ) && $doreturn ) {
+        if ( not $iteminformation->{borrowernumber} ) {
             $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'};
             $doreturn = 0;
         }
-    
+        
+        # even though item is not on loan, it may still
+        # be transferred; therefore, get current branch information
+        my $curr_iteminfo = GetItem($itemnumber);
+        $iteminformation->{'homebranch'} = $curr_iteminfo->{'homebranch'};
+        $iteminformation->{'holdingbranch'} = $curr_iteminfo->{'holdingbranch'};
+        $iteminformation->{'itemlost'} = $curr_iteminfo->{'itemlost'};
+        
         # check if the book is in a permanent collection....
         my $hbr      = $iteminformation->{C4::Context->preference("HomeOrHoldingBranch")};
         my $branches = GetBranches();
@@ -1322,7 +1342,7 @@ sub AddReturn {
         }
                
                    # if independent branches are on and returning to different branch, refuse the return
-        if ($hbr ne C4::Context->userenv->{'branch'} && C4::Context->preference("IndependantBranches")){
+        if ($hbr ne $branch && C4::Context->preference("IndependantBranches") && $iteminformation->{borrowernumber}){
                          $messages->{'Wrongbranch'} = 1;
                          $doreturn=0;
                    }
@@ -1333,6 +1353,7 @@ sub AddReturn {
             $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
@@ -1341,39 +1362,27 @@ sub AddReturn {
     # case of a return of document (deal with issues and holdingbranch)
     
         if ($doreturn) {
-                       my $circControlBranch;
+                       my $circControlBranch = _GetCircControlBranch($iteminformation,$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 ( $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?  
             # 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'};
-            }
+            
+            
+            # We update the holdingbranch from circControlBranch variable
+            UpdateHoldingbranch($branch,$iteminformation->{'itemnumber'});
+            $iteminformation->{'holdingbranch'} = $branch;
+        
+            
             ModDateLastSeen( $iteminformation->{'itemnumber'} );
             ModItem({ onloan => undef }, $biblio->{'biblionumber'}, $iteminformation->{'itemnumber'});
 
             if ($iteminformation->{borrowernumber}){
               ($borrower) = C4::Members::GetMemberDetails( $iteminformation->{borrowernumber}, 0 );
             }
-        }       
-        # fix up the accounts.....
-        if ( $iteminformation->{'itemlost'} ) {
-            $messages->{'WasLost'} = 1;
         }
     
     # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #
@@ -1382,7 +1391,7 @@ sub AddReturn {
     
     #     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'} ) {
+            if ( $tobranch eq $branch ) {
                     my $sth =
                     $dbh->prepare(
                             "UPDATE branchtransfers SET datearrived = now() WHERE itemnumber= ? AND datearrived IS NULL"
@@ -1403,6 +1412,7 @@ sub AddReturn {
         # fix up the accounts.....
         if ($iteminformation->{'itemlost'}) {
                 FixAccountForLostAndReturned($iteminformation, $borrower);
+                ModItem({ itemlost => '0' }, $biblio->{'biblionumber'}, $iteminformation->{'itemnumber'});
                 $messages->{'WasLost'} = 1;
         }
         # fix up the overdues in accounts...
@@ -1411,8 +1421,8 @@ sub AddReturn {
     
     # 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'} );
+        my ( $resfound, $resrec ) = 
+        C4::Reserves::CheckReserves( $itemnumber, $barcode );
         if ($resfound) {
             $resrec->{'ResFound'}   = $resfound;
             $messages->{'ResFound'} = $resrec;
@@ -1434,9 +1444,9 @@ sub AddReturn {
         #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 ($iteminformation->{'holdingbranch'} ne $iteminformation->{'homebranch'}) and not $messages->{'WrongTransfer'} and ($validTransfert ne 1) and ($reserveDone ne 1) ){
+        if (($doreturn or $messages->{'NotIssued'}) and ($branch ne $iteminformation->{$hbr}) 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'});
+                               ModItemTransfer($iteminformation->{'itemnumber'}, $branch, $iteminformation->{$hbr});
                                $messages->{'WasTransfered'} = 1;
                        }
                        else {
@@ -1600,7 +1610,6 @@ sub FixAccountForLostAndReturned {
                        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){
@@ -1632,9 +1641,8 @@ sub FixAccountForLostAndReturned {
                                VALUES
                                (?,?,?,?)");
                        $usth->execute($data->{'borrowernumber'},$accdata->{'accountno'},$nextaccntno,$newamtos);
-                       $usth->finish;
                }
-               $msth->finish;
+               $msth->finish;  # $msth might actually have data left
                }
                if ($amountleft > 0){
                        $amountleft*=-1;
@@ -1644,62 +1652,74 @@ sub FixAccountForLostAndReturned {
                        (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;
 }
 
-=head2 GetItemIssue
+=head2 _GetCircControlBranch
 
-$issues = &GetItemIssue($itemnumber);
+   my $circ_control_branch = _GetCircControlBranch($iteminfos, $borrower);
 
-Returns patrons currently having a book. nothing if item is not issued atm
+Internal function : 
 
-C<$itemnumber> is the itemnumber
+Return the library code to be used to determine which circulation
+policy applies to a transaction.  Looks up the CircControl and
+HomeOrHoldingBranch system preferences.
 
-Returns an array of hashes
+C<$iteminfos> is a hashref to iteminfo. Only {itemnumber} is used.
 
-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
+C<$borrower> is a hashref to borrower. Only {borrowernumber is used.
+
+=cut
+
+sub _GetCircControlBranch {
+    my ($iteminfos, $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 = $iteminfos->{$branchfield};
     }
+    return $branch;
+}
 
+=head2 GetItemIssue
+
+$issues = &GetItemIssue($itemnumber);
+
+Returns patron currently having a book, or undef if not checked out.
+
+C<$itemnumber> is the itemnumber
+
+C<$issues> is an array of hashes.
 
 =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;
+    $data->{'itemnumber'} = $itemnumber; # fill itemnumber, in case item is not on issue.
+    # FIXME: that would mean issues.itemnumber IS NULL and we didn't really match it.
     return ($data);
 }
 
@@ -1710,23 +1730,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
@@ -1736,23 +1753,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
@@ -1924,7 +1935,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.
 
@@ -1937,13 +1949,16 @@ 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 $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;
+    my $datedue         = shift;
+    my $lastreneweddate = shift || C4::Dates->new()->output('iso');
+
+
+    my $biblio = GetBiblioFromItemNumber($itemnumber) or return undef;
 
     # If the due date wasn't specified, calculate it by adding the
     # book's loan length to today's date.
@@ -1956,7 +1971,7 @@ sub AddRenewal {
                        $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);  # this branch is the transactional branch.
+               $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.
     }
 
@@ -1975,24 +1990,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
@@ -2296,17 +2312,32 @@ C<$loanlength>  = loan length prior to adjustment
 =cut
 
 sub CalcDateDue { 
-       my ($startdate,$loanlength,$branch) = @_;
+       my ($startdate,$loanlength,$branch,$borrower) = @_;
+       my $datedue;
+
        if(C4::Context->preference('useDaysMode') eq 'Days') {  # ignoring calendar
-               my $datedue = time + ($loanlength) * 86400;
+               my $timedue = time + ($loanlength) * 86400;
        #FIXME - assumes now even though we take a startdate 
-               my @datearr  = localtime($datedue);
-               return C4::Dates->new( sprintf("%04d-%02d-%02d", 1900 + $datearr[5], $datearr[4] + 1, $datearr[3]), 'iso');
+               my @datearr  = localtime($timedue);
+               $datedue = C4::Dates->new( sprintf("%04d-%02d-%02d", 1900 + $datearr[5], $datearr[4] + 1, $datearr[3]), 'iso');
        } else {
                my $calendar = C4::Calendar->new(  branchcode => $branch );
-               my $datedue = $calendar->addDate($startdate, $loanlength);
-               return $datedue;
+               $datedue = $calendar->addDate($startdate, $loanlength);
+       }
+
+       # if ReturnBeforeExpiry ON the datedue can't be after borrower expirydate
+       if ( C4::Context->preference('ReturnBeforeExpiry') && $datedue->output('iso') gt $borrower->{dateexpiry} ) {
+           $datedue = C4::Dates->new( $borrower->{dateexpiry}, 'iso' );
+       }
+
+       # 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') );
        }
+
+       return $datedue;
 }
 
 =head2 CheckValidDatedue