Bug 12001: Move GetMemberAccountBalance to Koha::Account->non_issues_charges
[srvgit] / C4 / Circulation.pm
index 0c5585a..9bd4e06 100644 (file)
@@ -41,6 +41,7 @@ use Algorithm::CheckDigits;
 use Data::Dumper;
 use Koha::Account;
 use Koha::AuthorisedValues;
+use Koha::Biblioitems;
 use Koha::DateUtils;
 use Koha::Calendar;
 use Koha::Checkouts;
@@ -53,6 +54,8 @@ use Koha::Libraries;
 use Koha::Holds;
 use Koha::RefundLostItemFeeRule;
 use Koha::RefundLostItemFeeRules;
+use Koha::Account::Lines;
+use Koha::Account::Offsets;
 use Carp;
 use List::MoreUtils qw( uniq );
 use Scalar::Util qw( looks_like_number );
@@ -275,10 +278,6 @@ is a reference-to-hash which may have any of the following keys:
 
 There is no item in the catalog with the given barcode. The value is C<$barcode>.
 
-=item C<IsPermanent>
-
-The item's home branch is permanent. This doesn't prevent the item from being transferred, though. The value is the code of the item's home branch.
-
 =item C<DestinationEqualsHolding>
 
 The item is already at the branch to which it is being transferred. The transfer is nonetheless considered to have failed. The value should be ignored.
@@ -333,15 +332,6 @@ sub transferbook {
         }
     }
 
-    # if is permanent...
-    # FIXME Is this still used by someone?
-    # See other FIXME in AddReturn
-    my $library = Koha::Libraries->find($hbr);
-    if ( $library and $library->get_categories->search({'me.categorycode' => 'PE'})->count ) {
-        $messages->{'IsPermanent'} = $hbr;
-        $dotransfer = 0;
-    }
-
     # can't transfer book if is already there....
     if ( $fbr eq $tbr ) {
         $messages->{'DestinationEqualsHolding'} = 1;
@@ -471,7 +461,7 @@ sub TooMany {
         my $max_checkouts_allowed = $issuing_rule->maxissueqty;
         my $max_onsite_checkouts_allowed = $issuing_rule->maxonsiteissueqty;
 
-        if ( $onsite_checkout ) {
+        if ( $onsite_checkout and defined $max_onsite_checkouts_allowed ) {
             if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed )  {
                 return {
                     reason => 'TOO_MANY_ONSITE_CHECKOUTS',
@@ -525,7 +515,7 @@ sub TooMany {
         my $max_checkouts_allowed = $branch_borrower_circ_rule->{maxissueqty};
         my $max_onsite_checkouts_allowed = $branch_borrower_circ_rule->{maxonsiteissueqty};
 
-        if ( $onsite_checkout ) {
+        if ( $onsite_checkout and defined $max_onsite_checkouts_allowed ) {
             if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed )  {
                 return {
                     reason => 'TOO_MANY_ONSITE_CHECKOUTS',
@@ -564,16 +554,20 @@ sub TooMany {
 
 =head2 CanBookBeIssued
 
-  ( $issuingimpossible, $needsconfirmation ) =  CanBookBeIssued( $borrower, 
+  ( $issuingimpossible, $needsconfirmation, [ $alerts ] ) =  CanBookBeIssued( $patron,
                       $barcode, $duedate, $inprocess, $ignore_reserves, $params );
 
 Check if a book can be issued.
 
-C<$issuingimpossible> and C<$needsconfirmation> are some hashref.
+C<$issuingimpossible> and C<$needsconfirmation> are hashrefs.
+
+IMPORTANT: The assumption by users of this routine is that causes blocking
+the issue are keyed by uppercase labels and other returned
+data is keyed in lower case!
 
 =over 4
 
-=item C<$borrower> hash with borrower informations (from Koha::Patron->unblessed)
+=item C<$patron> is a Koha::Patron
 
 =item C<$barcode> is the bar code of the book being issued.
 
@@ -664,7 +658,7 @@ if the borrower borrows to much things
 =cut
 
 sub CanBookBeIssued {
-    my ( $borrower, $barcode, $duedate, $inprocess, $ignore_reserves, $params ) = @_;
+    my ( $patron, $barcode, $duedate, $inprocess, $ignore_reserves, $params ) = @_;
     my %needsconfirmation;    # filled with problems that needs confirmations
     my %issuingimpossible;    # filled with problems that causes the issue to be IMPOSSIBLE
     my %alerts;               # filled with messages that shouldn't stop issuing, but the librarian should be aware of.
@@ -678,6 +672,7 @@ sub CanBookBeIssued {
        my $biblioitem = GetBiblioItemData($item->{biblioitemnumber});
        $item->{'itemtype'}=$item->{'itype'}; 
     my $dbh             = C4::Context->dbh;
+    my $patron_unblessed = $patron->unblessed;
 
     # MANDATORY CHECKS - unless item exists, nothing else matters
     unless ( $item->{barcode} ) {
@@ -695,9 +690,9 @@ sub CanBookBeIssued {
     unless ( $duedate ) {
         my $issuedate = $now->clone();
 
-        my $branch = _GetCircControlBranch($item,$borrower);
+        my $branch = _GetCircControlBranch($item, $patron_unblessed);
         my $itype = ( C4::Context->preference('item-level_itypes') ) ? $item->{'itype'} : $biblioitem->{'itemtype'};
-        $duedate = CalcDateDue( $issuedate, $itype, $branch, $borrower );
+        $duedate = CalcDateDue( $issuedate, $itype, $branch, $patron_unblessed );
 
         # Offline circ calls AddIssue directly, doesn't run through here
         #  So issuingimpossible should be ok.
@@ -715,21 +710,21 @@ sub CanBookBeIssued {
     #
     # BORROWER STATUS
     #
-    if ( $borrower->{'category_type'} eq 'X' && (  $item->{barcode}  )) { 
+    if ( $patron->category->category_type eq 'X' && (  $item->{barcode}  )) {
        # stats only borrower -- add entry to statistics table, and return issuingimpossible{STATS} = 1  .
         &UpdateStats({
                      branch => C4::Context->userenv->{'branch'},
                      type => 'localuse',
                      itemnumber => $item->{'itemnumber'},
                      itemtype => $item->{'itype'},
-                     borrowernumber => $borrower->{'borrowernumber'},
+                     borrowernumber => $patron->borrowernumber,
                      ccode => $item->{'ccode'}}
                     );
         ModDateLastSeen( $item->{'itemnumber'} );
         return( { STATS => 1 }, {});
     }
 
-    my $flags = C4::Members::patronflags( $borrower );
+    my $flags = C4::Members::patronflags( $patron_unblessed );
     if ( ref $flags ) {
         if ( $flags->{GNA} ) {
             $issuingimpossible{GNA} = 1;
@@ -741,16 +736,9 @@ sub CanBookBeIssued {
             $issuingimpossible{DEBARRED} = 1;
         }
     }
-    if ( !defined $borrower->{dateexpiry} || $borrower->{'dateexpiry'} eq '0000-00-00') {
+
+    if ( $patron->is_expired ) {
         $issuingimpossible{EXPIRED} = 1;
-    } else {
-        my $expiry_dt = dt_from_string( $borrower->{dateexpiry}, 'sql', 'floating' );
-        $expiry_dt->truncate( to => 'day');
-        my $today = $now->clone()->truncate(to => 'day');
-        $today->set_time_zone( 'floating' );
-        if ( DateTime->compare($today, $expiry_dt) == 1 ) {
-            $issuingimpossible{EXPIRED} = 1;
-        }
     }
 
     #
@@ -758,8 +746,10 @@ sub CanBookBeIssued {
     #
 
     # DEBTS
-    my ($balance, $non_issue_charges, $other_charges) =
-      C4::Members::GetMemberAccountBalance( $borrower->{'borrowernumber'} );
+    my $account = $patron->account;
+    my $balance = $account->balance;
+    my $non_issues_charges = $account->non_issues_charges;
+    my $other_charges = $balance - $non_issues_charges;
 
     my $amountlimit = C4::Context->preference("noissuescharge");
     my $allowfineoverride = C4::Context->preference("AllowFineOverride");
@@ -769,12 +759,10 @@ sub CanBookBeIssued {
     my $no_issues_charge_guarantees = C4::Context->preference("NoIssuesChargeGuarantees");
     $no_issues_charge_guarantees = undef unless looks_like_number( $no_issues_charge_guarantees );
     if ( defined $no_issues_charge_guarantees ) {
-        my $p = Koha::Patrons->find( $borrower->{borrowernumber} );
-        my @guarantees = $p->guarantees();
+        my @guarantees = $patron->guarantees();
         my $guarantees_non_issues_charges;
         foreach my $g ( @guarantees ) {
-            my ( $b, $n, $o ) = C4::Members::GetMemberAccountBalance( $g->id );
-            $guarantees_non_issues_charges += $n;
+            $guarantees_non_issues_charges += $g->account->non_issues_charges;
         }
 
         if ( $guarantees_non_issues_charges > $no_issues_charge_guarantees && !$inprocess && !$allowfineoverride) {
@@ -787,21 +775,21 @@ sub CanBookBeIssued {
     }
 
     if ( C4::Context->preference("IssuingInProcess") ) {
-        if ( $non_issue_charges > $amountlimit && !$inprocess && !$allowfineoverride) {
-            $issuingimpossible{DEBT} = sprintf( "%.2f", $non_issue_charges );
-        } elsif ( $non_issue_charges > $amountlimit && !$inprocess && $allowfineoverride) {
-            $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issue_charges );
-        } elsif ( $allfinesneedoverride && $non_issue_charges > 0 && $non_issue_charges <= $amountlimit && !$inprocess ) {
-            $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issue_charges );
+        if ( $non_issues_charges > $amountlimit && !$inprocess && !$allowfineoverride) {
+            $issuingimpossible{DEBT} = sprintf( "%.2f", $non_issues_charges );
+        } elsif ( $non_issues_charges > $amountlimit && !$inprocess && $allowfineoverride) {
+            $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issues_charges );
+        } elsif ( $allfinesneedoverride && $non_issues_charges > 0 && $non_issues_charges <= $amountlimit && !$inprocess ) {
+            $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issues_charges );
         }
     }
     else {
-        if ( $non_issue_charges > $amountlimit && $allowfineoverride ) {
-            $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issue_charges );
-        } elsif ( $non_issue_charges > $amountlimit && !$allowfineoverride) {
-            $issuingimpossible{DEBT} = sprintf( "%.2f", $non_issue_charges );
-        } elsif ( $non_issue_charges > 0 && $allfinesneedoverride ) {
-            $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issue_charges );
+        if ( $non_issues_charges > $amountlimit && $allowfineoverride ) {
+            $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issues_charges );
+        } elsif ( $non_issues_charges > $amountlimit && !$allowfineoverride) {
+            $issuingimpossible{DEBT} = sprintf( "%.2f", $non_issues_charges );
+        } elsif ( $non_issues_charges > 0 && $allfinesneedoverride ) {
+            $needsconfirmation{DEBT} = sprintf( "%.2f", $non_issues_charges );
         }
     }
 
@@ -809,7 +797,9 @@ sub CanBookBeIssued {
         $alerts{OTHER_CHARGES} = sprintf( "%.2f", $other_charges );
     }
 
-    my $patron = Koha::Patrons->find( $borrower->{borrowernumber} );
+    $patron = Koha::Patrons->find( $patron->borrowernumber ); # FIXME Refetch just in case, to avoid regressions. But must not be needed
+    $patron_unblessed = $patron->unblessed;
+
     if ( my $debarred_date = $patron->is_debarred ) {
          # patron has accrued fine days or has a restriction. $count is a date
         if ($debarred_date eq '9999-12-31') {
@@ -828,16 +818,66 @@ sub CanBookBeIssued {
         }
     }
 
+    #
+    # CHECK IF BOOK ALREADY ISSUED TO THIS BORROWER
+    #
+    if ( $issue && $issue->borrowernumber eq $patron->borrowernumber ){
+
+        # Already issued to current borrower.
+        # If it is an on-site checkout if it can be switched to a normal checkout
+        # or ask whether the loan should be renewed
+
+        if ( $issue->onsite_checkout
+                and C4::Context->preference('SwitchOnSiteCheckouts') ) {
+            $messages{ONSITE_CHECKOUT_WILL_BE_SWITCHED} = 1;
+        } else {
+            my ($CanBookBeRenewed,$renewerror) = CanBookBeRenewed(
+                $patron->borrowernumber,
+                $item->{'itemnumber'},
+            );
+            if ( $CanBookBeRenewed == 0 ) {    # no more renewals allowed
+                if ( $renewerror eq 'onsite_checkout' ) {
+                    $issuingimpossible{NO_RENEWAL_FOR_ONSITE_CHECKOUTS} = 1;
+                }
+                else {
+                    $issuingimpossible{NO_MORE_RENEWALS} = 1;
+                }
+            }
+            else {
+                $needsconfirmation{RENEW_ISSUE} = 1;
+            }
+        }
+    }
+    elsif ( $issue ) {
+
+        # issued to someone else
+
+        my $patron = Koha::Patrons->find( $issue->borrowernumber );
+
+        my ( $can_be_returned, $message ) = CanBookBeReturned( $item, C4::Context->userenv->{branch} );
+
+        unless ( $can_be_returned ) {
+            $issuingimpossible{RETURN_IMPOSSIBLE} = 1;
+            $issuingimpossible{branch_to_return} = $message;
+        } else {
+            $needsconfirmation{ISSUED_TO_ANOTHER} = 1;
+            $needsconfirmation{issued_firstname} = $patron->firstname;
+            $needsconfirmation{issued_surname} = $patron->surname;
+            $needsconfirmation{issued_cardnumber} = $patron->cardnumber;
+            $needsconfirmation{issued_borrowernumber} = $patron->borrowernumber;
+        }
+    }
+
     # JB34 CHECKS IF BORROWERS DON'T HAVE ISSUE TOO MANY BOOKS
     #
     my $switch_onsite_checkout = (
           C4::Context->preference('SwitchOnSiteCheckouts')
       and $issue
       and $issue->onsite_checkout
-      and $issue->borrowernumber == $borrower->{'borrowernumber'} ? 1 : 0 );
-    my $toomany = TooMany( $borrower, $item->{biblionumber}, $item, { onsite_checkout => $onsite_checkout, switch_onsite_checkout => $switch_onsite_checkout, } );
+      and $issue->borrowernumber == $patron->borrowernumber ? 1 : 0 );
+    my $toomany = TooMany( $patron_unblessed, $item->{biblionumber}, $item, { onsite_checkout => $onsite_checkout, switch_onsite_checkout => $switch_onsite_checkout, } );
     # if TooMany max_allowed returns 0 the user doesn't have permission to check out this book
-    if ( $toomany ) {
+    if ( $toomany && not exists $needsconfirmation{RENEW_ISSUE} ) {
         if ( $toomany->{max_allowed} == 0 ) {
             $needsconfirmation{PATRON_CANT} = 1;
         }
@@ -855,7 +895,7 @@ sub CanBookBeIssued {
     #
     # CHECKPREVCHECKOUT: CHECK IF ITEM HAS EVER BEEN LENT TO PATRON
     #
-    $patron = Koha::Patrons->find($borrower->{borrowernumber});
+    $patron = Koha::Patrons->find( $patron->borrowernumber ); # FIXME Refetch just in case, to avoid regressions. But must not be needed
     my $wants_check = $patron->wants_check_for_previous_checkout;
     $needsconfirmation{PREVISSUE} = 1
         if ($wants_check and $patron->do_check_for_previous_checkout($item));
@@ -922,8 +962,8 @@ sub CanBookBeIssued {
                 $issuingimpossible{ITEMNOTSAMEBRANCH} = 1;
                 $issuingimpossible{'itemhomebranch'} = $item->{C4::Context->preference("HomeOrHoldingBranch")};
             }
-            $needsconfirmation{BORRNOTSAMEBRANCH} = $borrower->{'branchcode'}
-              if ( $borrower->{'branchcode'} ne $userenv->{branch} );
+            $needsconfirmation{BORRNOTSAMEBRANCH} = $patron->branchcode
+              if ( $patron->branchcode ne $userenv->{branch} );
         }
     }
     #
@@ -932,68 +972,18 @@ sub CanBookBeIssued {
     my $rentalConfirmation = C4::Context->preference("RentalFeesCheckoutConfirmation");
 
     if ( $rentalConfirmation ){
-        my ($rentalCharge) = GetIssuingCharges( $item->{'itemnumber'}, $borrower->{'borrowernumber'} );
+        my ($rentalCharge) = GetIssuingCharges( $item->{'itemnumber'}, $patron->borrowernumber );
         if ( $rentalCharge > 0 ){
             $needsconfirmation{RENTALCHARGE} = $rentalCharge;
         }
     }
 
-    #
-    # CHECK IF BOOK ALREADY ISSUED TO THIS BORROWER
-    #
-    if ( $issue && $issue->borrowernumber eq $borrower->{'borrowernumber'} ){
-
-        # Already issued to current borrower.
-        # If it is an on-site checkout if it can be switched to a normal checkout
-        # or ask whether the loan should be renewed
-
-        if ( $issue->onsite_checkout
-                and C4::Context->preference('SwitchOnSiteCheckouts') ) {
-            $messages{ONSITE_CHECKOUT_WILL_BE_SWITCHED} = 1;
-        } else {
-            my ($CanBookBeRenewed,$renewerror) = CanBookBeRenewed(
-                $borrower->{'borrowernumber'},
-                $item->{'itemnumber'},
-            );
-            if ( $CanBookBeRenewed == 0 ) {    # no more renewals allowed
-                if ( $renewerror eq 'onsite_checkout' ) {
-                    $issuingimpossible{NO_RENEWAL_FOR_ONSITE_CHECKOUTS} = 1;
-                }
-                else {
-                    $issuingimpossible{NO_MORE_RENEWALS} = 1;
-                }
-            }
-            else {
-                $needsconfirmation{RENEW_ISSUE} = 1;
-            }
-        }
-    }
-    elsif ( $issue ) {
-
-        # issued to someone else
-
-        my $patron = Koha::Patrons->find( $issue->borrowernumber );
-
-        my ( $can_be_returned, $message ) = CanBookBeReturned( $item, C4::Context->userenv->{branch} );
-
-        unless ( $can_be_returned ) {
-            $issuingimpossible{RETURN_IMPOSSIBLE} = 1;
-            $issuingimpossible{branch_to_return} = $message;
-        } else {
-            $needsconfirmation{ISSUED_TO_ANOTHER} = 1;
-            $needsconfirmation{issued_firstname} = $patron->firstname;
-            $needsconfirmation{issued_surname} = $patron->surname;
-            $needsconfirmation{issued_cardnumber} = $patron->cardnumber;
-            $needsconfirmation{issued_borrowernumber} = $patron->borrowernumber;
-        }
-    }
-
     unless ( $ignore_reserves ) {
         # See if the item is on reserve.
         my ( $restype, $res ) = C4::Reserves::CheckReserves( $item->{'itemnumber'} );
         if ($restype) {
             my $resbor = $res->{'borrowernumber'};
-            if ( $resbor ne $borrower->{'borrowernumber'} ) {
+            if ( $resbor ne $patron->borrowernumber ) {
                 my $patron = Koha::Patrons->find( $resbor );
                 if ( $restype eq "Waiting" )
                 {
@@ -1023,7 +1013,7 @@ sub CanBookBeIssued {
 
     ## CHECK AGE RESTRICTION
     my $agerestriction  = $biblioitem->{'agerestriction'};
-    my ($restriction_age, $daysToAgeRestriction) = GetAgeRestriction( $agerestriction, $borrower );
+    my ($restriction_age, $daysToAgeRestriction) = GetAgeRestriction( $agerestriction, $patron->unblessed );
     if ( $daysToAgeRestriction && $daysToAgeRestriction > 0 ) {
         if ( C4::Context->preference('AgeRestrictionOverride') ) {
             $needsconfirmation{AGE_RESTRICTION} = "$agerestriction";
@@ -1035,7 +1025,7 @@ sub CanBookBeIssued {
 
     ## check for high holds decreasing loan period
     if ( C4::Context->preference('decreaseLoanHighHolds') ) {
-        my $check = checkHighHolds( $item, $borrower );
+        my $check = checkHighHolds( $item, $patron_unblessed );
 
         if ( $check->{exceeded} ) {
             if ($override_high_holds) {
@@ -1068,9 +1058,10 @@ sub CanBookBeIssued {
         require C4::Serials;
         my $is_a_subscription = C4::Serials::CountSubscriptionFromBiblionumber($biblionumber);
         unless ($is_a_subscription) {
+            # FIXME Should be $patron->checkouts($args);
             my $checkouts = Koha::Checkouts->search(
                 {
-                    borrowernumber => $borrower->{borrowernumber},
+                    borrowernumber => $patron->borrowernumber,
                     biblionumber   => $biblionumber,
                 },
                 {
@@ -1428,6 +1419,7 @@ sub AddIssue {
                     other          => ( $sipmode ? "SIP-$sipmode" : '' ),
                     itemnumber     => $item->{'itemnumber'},
                     itemtype       => $item->{'itype'},
+                    location       => $item->{location},
                     borrowernumber => $borrower->{'borrowernumber'},
                     ccode          => $item->{'ccode'}
                 }
@@ -1451,13 +1443,12 @@ sub AddIssue {
                     }
                 );
             }
+            logaction(
+                "CIRCULATION", "ISSUE",
+                $borrower->{'borrowernumber'},
+                $item->{'itemnumber'}
+            ) if C4::Context->preference("IssueLog");
         }
-
-        logaction(
-            "CIRCULATION", "ISSUE",
-            $borrower->{'borrowernumber'},
-            $item->{'itemnumber'}
-        ) if C4::Context->preference("IssueLog");
     }
     return $issue;
 }
@@ -1758,12 +1749,6 @@ No item with this barcode exists. The value is C<$barcode>.
 
 The book is not currently on loan. The value is C<$barcode>.
 
-=item C<IsPermanent>
-
-The book's home branch is a permanent collection. If you have borrowed
-this book, you are not allowed to return it. The value is the code for
-the book's home branch.
-
 =item C<withdrawn>
 
 This book has been withdrawn/cancelled. The value should be ignored.
@@ -1820,10 +1805,7 @@ sub AddReturn {
     }
 
     my $itemnumber = $item->{ itemnumber };
-
-    my $item_level_itypes = C4::Context->preference("item-level_itypes");
-    my $biblio   = $item_level_itypes ? undef : GetBiblioData( $item->{ biblionumber } ); # don't get bib data unless we need it
-    my $itemtype = $item_level_itypes ? $item->{itype} : $biblio->{itemtype};
+    my $itemtype = $item->{itype}; # GetItem called effective_itemtype
 
     my $issue  = Koha::Checkouts->find( { itemnumber => $itemnumber } );
     if ( $issue ) {
@@ -1881,16 +1863,6 @@ sub AddReturn {
         }
     }
 
-
-    # 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 ( $returnbranch ) {
-        my $library = Koha::Libraries->find($returnbranch);
-        if ( $library and $library->get_categories->search({'me.categorycode' => 'PE'})->count ) {
-            $messages->{'IsPermanent'} = $returnbranch;
-        }
-    }
-
     # check if the return is allowed at this branch
     my ($returnallowed, $message) = CanBookBeReturned($item, $branch);
     unless ($returnallowed){
@@ -1907,6 +1879,10 @@ sub AddReturn {
         $doreturn = 0 if C4::Context->preference("BlockReturnOfWithdrawnItems");
     }
 
+    if ( $item->{itemlost} and C4::Context->preference("BlockReturnOfLostItems") ) {
+        $doreturn = 0;
+    }
+
     # case of a return of document (deal with issues and holdingbranch)
     my $today = DateTime->now( time_zone => C4::Context->tz() );
 
@@ -1928,21 +1904,17 @@ sub AddReturn {
 
         if ($patron) {
             eval {
-                my $issue_id = MarkIssueReturned( $borrowernumber, $item->{'itemnumber'},
+                MarkIssueReturned( $borrowernumber, $item->{'itemnumber'},
                     $circControlBranch, $return_date, $patron->privacy );
-                $issue->issue_id($issue_id);
             };
             unless ( $@ ) {
                 if ( ( C4::Context->preference('CalculateFinesOnReturn') && $is_overdue ) || $return_date ) {
                     _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $patron_unblessed, return_date => $return_date } );
                 }
             } else {
-                $messages->{'Wrongbranch'} = {
-                    Wrongbranch => $branch,
-                    Rightbranch => $message
-                };
-                carp $@;
-                return ( 0, { WasReturned => 0 }, $issue, $patron_unblessed );
+                carp "The checkin for the following issue failed, Please go to the about page, section 'data corrupted' to know how to fix this problem ($@)" . Dumper( $issue->unblessed );
+
+                return ( 0, { WasReturned => 0, DataCorrupted => 1 }, $issue, $patron_unblessed );
             }
 
             # FIXME is the "= 1" right?  This could be the borrower hash.
@@ -1988,8 +1960,7 @@ sub AddReturn {
     # fix up the accounts.....
     if ( $item->{'itemlost'} ) {
         $messages->{'WasLost'} = 1;
-
-        if ( $item->{'itemlost'} ) {
+        unless ( C4::Context->preference("BlockReturnOfLostItems") ) {
             if (
                 Koha::RefundLostItemFeeRules->should_refund(
                     {
@@ -2000,7 +1971,8 @@ sub AddReturn {
                 )
               )
             {
-                _FixAccountForLostAndReturned( $item->{'itemnumber'}, $borrowernumber, $barcode );
+                _FixAccountForLostAndReturned( $item->{'itemnumber'},
+                    $borrowernumber, $barcode );
                 $messages->{'LostItemFeeRefunded'} = 1;
             }
         }
@@ -2131,6 +2103,11 @@ routine in C<C4::Accounts>.
 sub MarkIssueReturned {
     my ( $borrowernumber, $itemnumber, $dropbox_branch, $returndate, $privacy ) = @_;
 
+
+    # Retrieve the issue
+    my $issue = Koha::Checkouts->find( { itemnumber => $itemnumber } ) or return;
+    my $issue_id = $issue->issue_id;
+
     my $anonymouspatron;
     if ( $privacy == 2 ) {
         # The default of 0 will not work due to foreign key constraints
@@ -2145,11 +2122,6 @@ sub MarkIssueReturned {
     my $schema   = $database->schema;
     my $dbh   = C4::Context->dbh;
 
-    my $issue_id = $dbh->selectrow_array(
-        q|SELECT issue_id FROM issues WHERE itemnumber = ?|,
-        undef, $itemnumber
-    );
-
     my $query = 'UPDATE issues SET returndate=';
     my @bind;
     if ($dropbox_branch) {
@@ -2169,33 +2141,21 @@ sub MarkIssueReturned {
     # FIXME Improve the return value and handle it from callers
     $schema->txn_do(sub {
 
+        # Update the returndate
         $dbh->do( $query, undef, @bind );
 
-        my $issue = Koha::Checkouts->find( $issue_id ); # FIXME should be fetched earlier
+        # We just updated the returndate, so we need to refetch $issue
+        $issue->discard_changes;
 
         # Create the old_issues entry
-        my $old_checkout_data = $issue->unblessed;
-
-        if ( Koha::Old::Checkouts->find( $issue_id ) ) {
-            my $new_issue_id = ( Koha::Old::Checkouts->search(
-                {},
-                { columns => [ { max_issue_id => { max => 'issue_id' } } ] }
-            )->get_column('max_issue_id') )[0];
-            $new_issue_id++;
-            $issue_id = $new_issue_id;
-        }
-        $old_checkout_data->{issue_id} = $issue_id;
-        my $old_checkout = Koha::Old::Checkout->new($old_checkout_data)->store;
-
-        # Update the fines
-        $dbh->do(q|UPDATE accountlines SET issue_id = ? WHERE issue_id = ?|, undef, $old_checkout->issue_id, $issue->issue_id);
+        my $old_checkout = Koha::Old::Checkout->new($issue->unblessed)->store;
 
         # anonymise patron checkout immediately if $privacy set to 2 and AnonymousPatron is set to a valid borrowernumber
         if ( $privacy == 2) {
             $dbh->do(q|UPDATE old_issues SET borrowernumber=? WHERE issue_id = ?|, undef, $anonymouspatron, $old_checkout->issue_id);
         }
 
-        # Delete the issue
+        # And finally delete the issue
         $issue->delete;
 
         ModItem( { 'onloan' => undef }, undef, $itemnumber );
@@ -2332,39 +2292,65 @@ sub _FixOverduesOnReturn {
     my $dbh = C4::Context->dbh;
 
     # check for overdue fine
-    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 = $sth->fetchrow_hashref;
-    return 0 unless $data;    # no warning, there's just nothing to fix
+    my $accountline = Koha::Account::Lines->search(
+        {
+            borrowernumber => $borrowernumber,
+            itemnumber     => $item,
+            -or            => [
+                accounttype => 'FU',
+                accounttype => 'O',
+            ],
+        }
+    )->next();
+    return 0 unless $accountline;    # no warning, there's just nothing to fix
 
     my $uquery;
-    my @bind = ($data->{'accountlines_id'});
     if ($exemptfine) {
-        $uquery = "update accountlines set accounttype='FFOR', amountoutstanding=0";
+        my $amountoutstanding = $accountline->amountoutstanding;
+
+        $accountline->accounttype('FFOR');
+        $accountline->amountoutstanding(0);
+
+        Koha::Account::Offset->new(
+            {
+                debit_id => $accountline->id,
+                type => 'Forgiven',
+                amount => $amountoutstanding * -1,
+            }
+        );
+
         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");
+    } elsif ($dropbox && $accountline->lastincrement) {
+        my $outstanding = $accountline->amountoutstanding - $accountline->lastincrement;
+        my $amt = $accountline->amount - $accountline->lastincrement;
+
+        Koha::Account::Offset->new(
+            {
+                debit_id => $accountline->id,
+                type => 'Dropbox',
+                amount => $accountline->lastincrement * -1,
+            }
+        );
+
+        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) ;
+
+        $accountline->accounttype('F');
+
+        if ( $outstanding >= 0 && $amt >= 0 ) {
+            $accountline->amount($amt);
+            $accountline->amountoutstanding($outstanding);
         }
+
     } else {
-        $uquery = "update accountlines set accounttype='F' ";
+        $accountline->accounttype('F');
     }
-    $uquery .= " where (accountlines_id = ?)";
-    my $usth = $dbh->prepare($uquery);
-    return $usth->execute(@bind);
+
+    return $accountline->store();
 }
 
 =head2 _FixAccountForLostAndReturned
@@ -2375,83 +2361,45 @@ 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
 
 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 IN ('L', 'Rep', 'W') ORDER BY date DESC, accountno DESC");
-    $sth->execute($itemnumber);
-    my $data = $sth->fetchrow_hashref;
-    $data or return;    # bail if there is nothing to do
-    $data->{accounttype} eq 'W' and return;    # Written off
-
-    # 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 (accountlines_id = ?)");
-    $usth->execute($data->{'accountlines_id'});      # 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->{'accountlines_id'};
-            # FIXME: move prepares outside while loop!
-            my $usth = $dbh->prepare("UPDATE accountlines SET amountoutstanding= ?
-                    WHERE (accountlines_id = ?)");
-            $usth->execute($newamtos,$thisacct);
-            $usth = $dbh->prepare("INSERT INTO accountoffsets
-                (borrowernumber, accountno, offsetaccount,  offsetamount)
-                VALUES
-                (?,?,?,?)");
-            $usth->execute($data->{'borrowernumber'},$accdata->{'accountno'},$nextaccntno,$newamtos);
-        }
-    }
-    $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;
+    my $accountline = Koha::Account::Lines->search(
+        {
+            itemnumber  => $itemnumber,
+            accounttype => { -in => [ 'L', 'Rep', 'W' ] },
+        },
+        {
+            order_by => { -desc => [ 'date', 'accountno' ] }
+        }
+    )->next();
+
+    return unless $accountline;
+    return if $accountline->accounttype eq 'W';    # Written off
+
+    $accountline->accounttype('LR');
+    $accountline->store();
+
+    my $account = Koha::Account->new( { patron_id => $accountline->borrowernumber } );
+    my $credit_id = $account->pay(
+        {
+            amount       => $accountline->amount,
+            description  => "Item Returned " . $item_id,
+            account_type => 'CR',
+            offset_type  => 'Lost Item Return',
+            accounlines  => [$accountline],
+
+        }
+    );
+
+    ModItem( { paidfor => '' }, undef, $itemnumber );
+
+    return $credit_id;
 }
 
 =head2 _GetCircControlBranch
@@ -2520,7 +2468,7 @@ sub GetOpenIssue {
 this function get all issues from a biblionumber.
 
 Return:
-C<$issues> is a reference to array which each value is ref-to-hash. This ref-to-hash containts all column from
+C<$issues> is a reference to array which each value is ref-to-hash. This ref-to-hash contains all column from
 tables issues and the firstname,surname & cardnumber from borrowers.
 
 =cut
@@ -2570,12 +2518,15 @@ sub GetUpcomingDueIssues {
     my $dbh = C4::Context->dbh;
 
     my $statement = <<END_SQL;
-SELECT issues.*, items.itype as itemtype, items.homebranch, TO_DAYS( date_due )-TO_DAYS( NOW() ) as days_until_due, branches.branchemail
-FROM issues 
-LEFT JOIN items USING (itemnumber)
-LEFT OUTER JOIN branches USING (branchcode)
-WHERE returndate is NULL
-HAVING days_until_due >= 0 AND days_until_due <= ?
+SELECT *
+FROM (
+    SELECT issues.*, items.itype as itemtype, items.homebranch, TO_DAYS( date_due )-TO_DAYS( NOW() ) as days_until_due, branches.branchemail
+    FROM issues
+    LEFT JOIN items USING (itemnumber)
+    LEFT OUTER JOIN branches USING (branchcode)
+    WHERE returndate is NULL
+) tmp
+WHERE days_until_due >= 0 AND days_until_due <= ?
 END_SQL
 
     my @bind_parameters = ( $params->{'days_in_advance'} );
@@ -2718,6 +2669,11 @@ sub CanBookBeRenewed {
     }
 
     if ( $issue->auto_renew ) {
+
+        if ( $patron->category->effective_BlockExpiredPatronOpacActions and $patron->is_expired ) {
+            return ( 0, 'auto_account_expired' );
+        }
+
         if ( defined $issuing_rule->no_auto_renewal_after
                 and $issuing_rule->no_auto_renewal_after ne "" ) {
             # Get issue_date and add no_auto_renewal_after
@@ -2741,7 +2697,7 @@ sub CanBookBeRenewed {
 
         if ( C4::Context->preference('OPACFineNoRenewalsBlockAutoRenew') ) {
             my $fine_no_renewals = C4::Context->preference("OPACFineNoRenewals");
-            my ( $amountoutstanding ) = C4::Members::GetMemberAccountRecords($patron->borrowernumber);
+            my $amountoutstanding = $patron->account->balance;
             if ( $amountoutstanding and $amountoutstanding > $fine_no_renewals ) {
                 return ( 0, "auto_too_much_oweing" );
             }
@@ -2915,14 +2871,19 @@ sub AddRenewal {
         DelUniqueDebarment({ borrowernumber => $borrowernumber, type => 'OVERDUES' });
     }
 
+    unless ( C4::Context->interface eq 'opac' ) { #if from opac we are obeying OpacRenewalBranch as calculated in opac-renew.pl
+        $branch = C4::Context->userenv ? C4::Context->userenv->{branch} : $branch;
+    }
+
     # Add the renewal to stats
     UpdateStats(
         {
-            branch => C4::Context->userenv ? C4::Context->userenv->{branch} : $branch,
+            branch         => $branch,
             type           => 'renew',
             amount         => $charge,
             itemnumber     => $itemnumber,
             itemtype       => $item->{itype},
+            location       => $item->{location},
             borrowernumber => $borrowernumber,
             ccode          => $item->{'ccode'}
         }
@@ -3203,19 +3164,33 @@ sub _get_discount_from_rule {
 
 sub AddIssuingCharge {
     my ( $itemnumber, $borrowernumber, $charge ) = @_;
-    my $dbh = C4::Context->dbh;
-    my $nextaccntno = getnextacctno( $borrowernumber );
-    my $manager_id = 0;
+
+    my $nextaccntno = getnextacctno($borrowernumber);
+
+    my $manager_id  = 0;
     $manager_id = C4::Context->userenv->{'number'} if C4::Context->userenv;
-    my $query ="
-        INSERT INTO accountlines
-            (borrowernumber, itemnumber, accountno,
-            date, amount, description, accounttype,
-            amountoutstanding, manager_id)
-        VALUES (?, ?, ?,now(), ?, 'Rental', 'Rent',?,?)
-    ";
-    my $sth = $dbh->prepare($query);
-    $sth->execute( $borrowernumber, $itemnumber, $nextaccntno, $charge, $charge, $manager_id );
+
+    my $accountline = Koha::Account::Line->new(
+        {
+            borrowernumber    => $borrowernumber,
+            itemnumber        => $itemnumber,
+            accountno         => $nextaccntno,
+            amount            => $charge,
+            amountoutstanding => $charge,
+            manager_id        => $manager_id,
+            description       => 'Rental',
+            accounttype       => 'Rent',
+            date              => \'NOW()',
+        }
+    )->store();
+
+    Koha::Account::Offset->new(
+        {
+            debit_id => $accountline->id,
+            type     => 'Rental Fee',
+            amount   => $charge,
+        }
+    )->store();
 }
 
 =head2 GetTransfers
@@ -3522,6 +3497,13 @@ sub CalcDateDue {
                 $datedue = $expiry_dt->clone->set_time_zone( C4::Context->tz );
             }
         }
+        if ( C4::Context->preference('useDaysMode') ne 'Days' ) {
+          my $calendar = Koha::Calendar->new( branchcode => $branch );
+          if ( $calendar->is_holiday($datedue) ) {
+              # Don't return on a closed day
+              $datedue = $calendar->prev_open_day( $datedue );
+          }
+        }
     }
 
     return $datedue;
@@ -3623,6 +3605,8 @@ sub ReturnLostItem{
 sub LostItem{
     my ($itemnumber, $mark_returned) = @_;
 
+    $mark_returned //= C4::Context->preference('MarkLostItemsAsReturned');
+
     my $dbh = C4::Context->dbh();
     my $sth=$dbh->prepare("SELECT issues.*,items.*,biblio.title 
                            FROM issues 
@@ -3956,6 +3940,7 @@ sub GetTopIssues {
 
     my $dbh = C4::Context->dbh;
     my $query = q{
+        SELECT * FROM (
         SELECT b.biblionumber, b.title, b.author, bi.itemtype, bi.publishercode,
           bi.place, bi.publicationyear, b.copyrightdate, bi.pages, bi.size,
           i.ccode, SUM(i.issues) AS count
@@ -3993,11 +3978,13 @@ sub GetTopIssues {
     }
 
     $query .= q{
-        GROUP BY b.biblionumber
-        HAVING count > 0
+        GROUP BY b.biblionumber, b.title, b.author, bi.itemtype, bi.publishercode,
+          bi.place, bi.publicationyear, b.copyrightdate, bi.pages, bi.size,
+          i.ccode
         ORDER BY count DESC
     };
 
+    $query .= q{ ) xxx WHERE count > 0 };
     $count = int($count);
     if ($count > 0) {
         $query .= "LIMIT $count";