Merge remote branch 'kc/new/bug_3326' into kcmaster
[koha.git] / C4 / Circulation.pm
index 117ef83..516a111 100644 (file)
@@ -1,6 +1,7 @@
 package C4::Circulation;
 
 # Copyright 2000-2002 Katipo Communications
+# copyright 2010 BibLibre
 #
 # This file is part of Koha.
 #
@@ -13,13 +14,13 @@ package C4::Circulation;
 # WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
 # A PARTICULAR PURPOSE.  See the GNU General Public License for more details.
 #
-# You should have received a copy of the GNU General Public License along with
-# Koha; if not, write to the Free Software Foundation, Inc., 59 Temple Place,
-# Suite 330, Boston, MA  02111-1307 USA
+# You should have received a copy of the GNU General Public License along
+# with Koha; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 
 
 use strict;
-#use warnings;  # soon!
+#use warnings; FIXME - Bug 2505
 use C4::Context;
 use C4::Stats;
 use C4::Reserves;
@@ -31,7 +32,9 @@ use C4::Dates;
 use C4::Calendar;
 use C4::Accounts;
 use C4::ItemCirculationAlertPreference;
+use C4::Dates qw(format_date);
 use C4::Message;
+use C4::Debug;
 use Date::Calc qw(
   Today
   Today_and_Now
@@ -68,7 +71,6 @@ BEGIN {
                &AddRenewal
                &GetRenewCount
                &GetItemIssue
-                &GetOpenIssue
                &GetItemIssues
                &GetBorrowerIssues
                &GetIssuingCharges
@@ -76,6 +78,7 @@ BEGIN {
         &GetBranchBorrowerCircRule
         &GetBranchItemRule
                &GetBiblioIssues
+               &GetOpenIssue
                &AnonymiseIssueHistory
        );
 
@@ -116,11 +119,9 @@ Also deals with stocktaking.
 
 =head2 barcodedecode
 
-=head3 $str = &barcodedecode($barcode, [$filter]);
+  $str = &barcodedecode($barcode, [$filter]);
 
-=over 4
-
-=item Generic filter function for barcode string.
+Generic filter function for barcode string.
 Called on every circ if the System Pref itemBarcodeInputFilter is set.
 Will do some manipulation of the barcode for systems that deliver a barcode
 to circulation.pl that differs from the barcode stored for the item.
@@ -131,8 +132,6 @@ The optional $filter argument is to allow for testing or explicit
 behavior that ignores the System Pref.  Valid values are the same as the 
 System Pref options.
 
-=back
-
 =cut
 
 # FIXME -- the &decode fcn below should be wrapped into this one.
@@ -140,6 +139,7 @@ System Pref options.
 #
 sub barcodedecode {
     my ($barcode, $filter) = @_;
+    my $branch = C4::Branch::mybranch();
     $filter = C4::Context->preference('itemBarcodeInputFilter') unless $filter;
     $filter or return $barcode;     # ensure filter is defined, else return untouched barcode
        if ($filter eq 'whitespace') {
@@ -158,24 +158,28 @@ sub barcodedecode {
         # FIXME: $barcode could be "T1", causing warning: substr outside of string
         # Why drop the nonzero digit after the T?
         # Why pass non-digits (or empty string) to "T%07d"?
+       } elsif ($filter eq 'libsuite8') {
+               unless($barcode =~ m/^($branch)-/i){    #if barcode starts with branch code its in Koha style. Skip it.
+                       if($barcode =~ m/^(\d)/i){      #Some barcodes even start with 0's & numbers and are assumed to have b as the item type in the libsuite8 software
+                                $barcode =~ s/^[0]*(\d+)$/$branch-b-$1/i;
+                        }else{
+                               $barcode =~ s/^(\D+)[0]*(\d+)$/$branch-$1-$2/i;
+                       }
+               }
        }
     return $barcode;    # return barcode, modified or not
 }
 
 =head2 decode
 
-=head3 $str = &decode($chunk);
-
-=over 4
+  $str = &decode($chunk);
 
-=item Decodes a segment of a string emitted by a CueCat barcode scanner and
+Decodes a segment of a string emitted by a CueCat barcode scanner and
 returns it.
 
 FIXME: Should be replaced with Barcode::Cuecat from CPAN
 or Javascript based decoding on the client side.
 
-=back
-
 =cut
 
 sub decode {
@@ -207,7 +211,8 @@ sub decode {
 
 =head2 transferbook
 
-($dotransfer, $messages, $iteminformation) = &transferbook($newbranch, $barcode, $ignore_reserves);
+  ($dotransfer, $messages, $iteminformation) = &transferbook($newbranch, 
+                                            $barcode, $ignore_reserves);
 
 Transfers an item to a new branch. If the item is currently on loan, it is automatically returned before the actual transfer.
 
@@ -220,15 +225,17 @@ Otherwise, if an item is reserved, the transfer fails.
 
 Returns three values:
 
-=head3 $dotransfer 
+=over
+
+=item $dotransfer 
 
 is true if the transfer was successful.
 
-=head3 $messages
+=item $messages
 
 is a reference-to-hash which may have any of the following keys:
 
-=over 4
+=over
 
 =item C<BadBarcode>
 
@@ -256,6 +263,8 @@ The item was eligible to be transferred. Barring problems communicating with the
 
 =back
 
+=back
+
 =cut
 
 sub transferbook {
@@ -325,8 +334,9 @@ sub transferbook {
 
         # don't need to update MARC anymore, we do it in batch now
         $messages->{'WasTransfered'} = 1;
-               ModDateLastSeen( $itemnumber );
+
     }
+    ModDateLastSeen( $itemnumber );
     return ( $dotransfer, $messages, $biblio );
 }
 
@@ -339,7 +349,7 @@ sub TooMany {
     my $dbh             = C4::Context->dbh;
        my $branch;
        # Get which branchcode we need
-       $branch=_GetCirculationBranch($item,$borrower);
+       $branch = _GetCircControlBranch($item,$borrower);
        my $type = (C4::Context->preference('item-level_itypes')) 
                        ? $item->{'itype'}         # item-level
                        : $item->{'itemtype'};     # biblio-level
@@ -413,7 +423,7 @@ sub TooMany {
 
         my $max_loans_allowed = $issuing_rule->{'maxissueqty'};
         if ($current_loan_count >= $max_loans_allowed) {
-            return "$current_loan_count / $max_loans_allowed";
+            return ($current_loan_count, $max_loans_allowed);
         }
     }
 
@@ -441,7 +451,7 @@ sub TooMany {
 
         my $max_loans_allowed = $branch_borrower_circ_rule->{maxissueqty};
         if ($current_loan_count >= $max_loans_allowed) {
-            return "$current_loan_count / $max_loans_allowed";
+            return ($current_loan_count, $max_loans_allowed);
         }
     }
 
@@ -531,7 +541,6 @@ sub itemissues {
             $data->{'date_due'} = ($data->{'wthdrawn'} eq '1') ? 'Cancelled' : 'Available';
         }
 
-        $sth2->finish;
 
         # Find the last 3 people who borrowed this item.
         $sth2 = $dbh->prepare(
@@ -551,20 +560,19 @@ sub itemissues {
             }    # if
         }    # for
 
-        $sth2->finish;
         $results[$i] = $data;
         $i++;
     }
 
-    $sth->finish;
     return (@results);
 }
 
 =head2 CanBookBeIssued
 
-Check if a book can be issued.
+  ( $issuingimpossible, $needsconfirmation ) =  CanBookBeIssued( $borrower, 
+                                      $barcode, $duedatespec, $inprocess );
 
-( $issuingimpossible, $needsconfirmation ) =  CanBookBeIssued( $borrower, $barcode, $duedatespec, $inprocess );
+Check if a book can be issued.
 
 C<$issuingimpossible> and C<$needsconfirmation> are some hashref.
 
@@ -621,7 +629,9 @@ item withdrawn.
 
 item is restricted (set by ??)
 
-C<$issuingimpossible> a reference to a hash. It contains reasons why issuing is impossible.
+C<$needsconfirmation> a reference to a hash. It contains reasons why the loan 
+could be prevented, but ones that can be overriden by the operator.
+
 Possible values are :
 
 =head3 DEBT
@@ -642,7 +652,7 @@ reserved for someone else.
 
 =head3 INVALID_DATE
 
-sticky due date is invalid
+sticky due date is invalid or due date in the past
 
 =head3 TOO_MANY
 
@@ -672,7 +682,7 @@ sub CanBookBeIssued {
     unless ( $duedate ) {
         my $issuedate = strftime( "%Y-%m-%d", localtime );
 
-        my $branch = _GetCirculationBranch($item,$borrower);
+        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 );
@@ -680,7 +690,12 @@ sub CanBookBeIssued {
         # 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') );
+    if ($duedate) {
+        $needsconfirmation{INVALID_DATE} = $duedate->output('syspref')
+          unless $duedate->output('iso') ge C4::Dates->today('iso');
+    } else {
+        $issuingimpossible{INVALID_DATE} = $duedate->output('syspref');
+    }
 
     #
     # BORROWER STATUS
@@ -688,6 +703,7 @@ sub CanBookBeIssued {
     if ( $borrower->{'category_type'} eq 'X' && (  $item->{barcode}  )) { 
        # stats only borrower -- add entry to statistics table, and return issuingimpossible{STATS} = 1  .
         &UpdateStats(C4::Context->userenv->{'branch'},'localuse','','',$item->{'itemnumber'},$item->{'itemtype'},$borrower->{'borrowernumber'});
+        ModDateLastSeen( $item->{'itemnumber'} );
         return( { STATS => 1 }, {});
     }
     if ( $borrower->{flags}->{GNA} ) {
@@ -715,30 +731,55 @@ sub CanBookBeIssued {
     # DEBTS
     my ($amount) =
       C4::Members::GetMemberAccountRecords( $borrower->{'borrowernumber'}, '' && $duedate->output('iso') );
+    my $amountlimit = C4::Context->preference("noissuescharge");
+    my $allowfineoverride = C4::Context->preference("AllowFineOverride");
+    my $allfinesneedoverride = C4::Context->preference("AllFinesNeedOverride");
     if ( C4::Context->preference("IssuingInProcess") ) {
-        my $amountlimit = C4::Context->preference("noissuescharge");
-        if ( $amount > $amountlimit && !$inprocess ) {
+        if ( $amount > $amountlimit && !$inprocess && !$allowfineoverride) {
             $issuingimpossible{DEBT} = sprintf( "%.2f", $amount );
-        }
-        elsif ( $amount > 0 && $amount <= $amountlimit && !$inprocess ) {
+        } elsif ( $amount > $amountlimit && !$inprocess && $allowfineoverride) {
+            $needsconfirmation{DEBT} = sprintf( "%.2f", $amount );
+        } elsif ( $allfinesneedoverride && $amount > 0 && $amount <= $amountlimit && !$inprocess ) {
             $needsconfirmation{DEBT} = sprintf( "%.2f", $amount );
         }
     }
     else {
-        if ( $amount > 0 ) {
+        if ( $amount > $amountlimit && $allowfineoverride ) {
+            $needsconfirmation{DEBT} = sprintf( "%.2f", $amount );
+        } elsif ( $amount > $amountlimit && !$allowfineoverride) {
+            $issuingimpossible{DEBT} = sprintf( "%.2f", $amount );
+        } elsif ( $amount > 0 && $allfinesneedoverride ) {
             $needsconfirmation{DEBT} = sprintf( "%.2f", $amount );
         }
     }
 
-    #
+    my ($blocktype, $count) = C4::Members::IsMemberBlocked($borrower->{'borrowernumber'});
+    if ($blocktype == -1) {
+        ## patron has outstanding overdue loans
+           if ( C4::Context->preference("OverduesBlockCirc") eq 'block'){
+               $issuingimpossible{USERBLOCKEDOVERDUE} = $count;
+           }
+           elsif ( C4::Context->preference("OverduesBlockCirc") eq 'confirmation'){
+               $needsconfirmation{USERBLOCKEDOVERDUE} = $count;
+           }
+    } elsif($blocktype == 1) {
+        # patron has accrued fine days
+        $issuingimpossible{USERBLOCKEDREMAINING} = $count;
+    }
+
+#
     # JB34 CHECKS IF BORROWERS DONT HAVE ISSUE TOO MANY BOOKS
     #
-       my $toomany = TooMany( $borrower, $item->{biblionumber}, $item );
-    # if TooMany return / 0, then the user has no permission to check out this book
-    if ($toomany =~ /\/ 0/) {
+       my ($current_loan_count, $max_loans_allowed) = TooMany( $borrower, $item->{biblionumber}, $item );
+    # if TooMany max_loans_allowed returns 0 the user doesn't have permission to check out this book
+    if ($max_loans_allowed eq 0) {
         $needsconfirmation{PATRON_CANT} = 1;
     } else {
-        $needsconfirmation{TOO_MANY} = $toomany if $toomany;
+        if($max_loans_allowed){
+            $needsconfirmation{TOO_MANY} = 1;
+            $needsconfirmation{current_loan_count} = $current_loan_count;
+            $needsconfirmation{max_loans_allowed} = $max_loans_allowed;
+        }
     }
 
     #
@@ -789,8 +830,10 @@ sub CanBookBeIssued {
     if ( C4::Context->preference("IndependantBranches") ) {
         my $userenv = C4::Context->userenv;
         if ( ($userenv) && ( $userenv->{flags} % 2 != 1 ) ) {
-            $issuingimpossible{NOTSAMEBRANCH} = 1
+            $issuingimpossible{ITEMNOTSAMEBRANCH} = 1
               if ( $item->{C4::Context->preference("HomeOrHoldingBranch")} ne $userenv->{branch} );
+            $needsconfirmation{BORRNOTSAMEBRANCH} = GetBranchName( $borrower->{'branchcode'} )
+              if ( $borrower->{'branchcode'} ne $userenv->{branch} );
         }
     }
 
@@ -819,8 +862,11 @@ sub CanBookBeIssued {
         my $currborinfo =    C4::Members::GetMemberDetails( $issue->{borrowernumber} );
 
 #        warn "=>.$currborinfo->{'firstname'} $currborinfo->{'surname'} ($currborinfo->{'cardnumber'})";
-        $needsconfirmation{ISSUED_TO_ANOTHER} =
-"$currborinfo->{'reservedate'} : $currborinfo->{'firstname'} $currborinfo->{'surname'} ($currborinfo->{'cardnumber'})";
+        $needsconfirmation{ISSUED_TO_ANOTHER} = 1;
+        $needsconfirmation{issued_firstname} = $currborinfo->{'firstname'};
+        $needsconfirmation{issued_surname} = $currborinfo->{'surname'};
+        $needsconfirmation{issued_cardnumber} = $currborinfo->{'cardnumber'};
+        $needsconfirmation{issued_borrowernumber} = $currborinfo->{'borrowernumber'};
     }
 
     # See if the item is on reserve.
@@ -834,13 +880,23 @@ sub CanBookBeIssued {
         {
             # The item is on reserve and waiting, but has been
             # reserved by some other patron.
-            $needsconfirmation{RESERVE_WAITING} =
-"$resborrower->{'firstname'} $resborrower->{'surname'} ($resborrower->{'cardnumber'}, $branchname)";
+            $needsconfirmation{RESERVE_WAITING} = 1;
+            $needsconfirmation{'resfirstname'} = $resborrower->{'firstname'};
+            $needsconfirmation{'ressurname'} = $resborrower->{'surname'};
+            $needsconfirmation{'rescardnumber'} = $resborrower->{'cardnumber'};
+            $needsconfirmation{'resborrowernumber'} = $resborrower->{'borrowernumber'};
+            $needsconfirmation{'resbranchname'} = $branchname;
+            $needsconfirmation{'reswaitingdate'} = format_date($res->{'waitingdate'});
         }
         elsif ( $restype eq "Reserved" ) {
             # The item is on reserve for someone else.
-            $needsconfirmation{RESERVED} =
-"$res->{'reservedate'} : $resborrower->{'firstname'} $resborrower->{'surname'} ($resborrower->{'cardnumber'})";
+            $needsconfirmation{RESERVED} = 1;
+            $needsconfirmation{'resfirstname'} = $resborrower->{'firstname'};
+            $needsconfirmation{'ressurname'} = $resborrower->{'surname'};
+            $needsconfirmation{'rescardnumber'} = $resborrower->{'cardnumber'};
+            $needsconfirmation{'resborrowernumber'} = $resborrower->{'borrowernumber'};
+            $needsconfirmation{'resbranchname'} = $branchname;
+            $needsconfirmation{'resreservedate'} = format_date($res->{'reservedate'});
         }
     }
        return ( \%issuingimpossible, \%needsconfirmation );
@@ -848,9 +904,9 @@ sub CanBookBeIssued {
 
 =head2 AddIssue
 
-Issue a book. Does no check, they are done in CanBookBeIssued. If we reach this sub, it means the user confirmed if needed.
+  &AddIssue($borrower, $barcode, [$datedue], [$cancelreserve], [$issuedate])
 
-&AddIssue($borrower, $barcode, [$datedue], [$cancelreserve], [$issuedate])
+Issue a book. Does no check, they are done in CanBookBeIssued. If we reach this sub, it means the user confirmed if needed.
 
 =over 4
 
@@ -888,7 +944,6 @@ sub AddIssue {
     my ( $borrower, $barcode, $datedue, $cancelreserve, $issuedate, $sipmode) = @_;
     my $dbh = C4::Context->dbh;
        my $barcodecheck=CheckValidBarcode($barcode);
-
     # $issuedate defaults to today.
     if ( ! defined $issuedate ) {
         $issuedate = strftime( "%Y-%m-%d", localtime );
@@ -898,7 +953,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 = _GetCirculationBranch($item,$borrower);
+               my $branch = _GetCircControlBranch($item,$borrower);
                
                # get actual issuing if there is one
                my $actualissue = GetItemIssue( $item->{itemnumber});
@@ -999,6 +1054,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'},
@@ -1055,9 +1113,9 @@ sub AddIssue {
 
 =head2 GetLoanLength
 
-Get loan length for an itemtype, a borrower type and a branch
+  my $loanlength = &GetLoanLength($borrowertype,$itemtype,branchcode)
 
-my $loanlength = &GetLoanLength($borrowertype,$itemtype,branchcode)
+Get loan length for an itemtype, a borrower type and a branch
 
 =cut
 
@@ -1117,6 +1175,8 @@ sub GetLoanLength {
 
 =head2 GetIssuingRule
 
+  my $irule = &GetIssuingRule($borrowertype,$itemtype,branchcode)
+
 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.
@@ -1124,8 +1184,6 @@ this close to release, however, Overdues::GetIssuingRules is broken.
 Get the issuing rule for an itemtype, a borrower type and a branch
 Returns a hashref from the issuingrules table.
 
-my $irule = &GetIssuingRule($borrowertype,$itemtype,branchcode)
-
 =cut
 
 sub GetIssuingRule {
@@ -1172,11 +1230,7 @@ sub GetIssuingRule {
 
 =head2 GetBranchBorrowerCircRule
 
-=over 4
-
-my $branch_cat_rule = GetBranchBorrowerCircRule($branchcode, $categorycode);
-
-=back
+  my $branch_cat_rule = GetBranchBorrowerCircRule($branchcode, $categorycode);
 
 Retrieves circulation rule attributes that apply to the given
 branch and patron category, regardless of item type.  
@@ -1259,11 +1313,7 @@ sub GetBranchBorrowerCircRule {
 
 =head2 GetBranchItemRule
 
-=over 4
-
-my $branch_item_rule = GetBranchItemRule($branchcode, $itemtype);
-
-=back
+  my $branch_item_rule = GetBranchItemRule($branchcode, $itemtype);
 
 Retrieves circulation rule attributes that apply to the given
 branch and item type, regardless of patron category.
@@ -1323,8 +1373,8 @@ sub GetBranchItemRule {
 
 =head2 AddReturn
 
-($doreturn, $messages, $iteminformation, $borrower) =
-    &AddReturn($barcode, $branch, $exemptfine, $dropbox);
+  ($doreturn, $messages, $iteminformation, $borrower) =
+      &AddReturn($barcode, $branch, $exemptfine, $dropbox);
 
 Returns a book.
 
@@ -1429,7 +1479,8 @@ sub AddReturn {
     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")} || '';
+    my $hbr      = C4::Context->preference("HomeOrHoldingBranchReturn") || "homebranch";
+    $hbr = $item->{$hbr} || '';
         # 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
@@ -1447,7 +1498,13 @@ sub AddReturn {
             Wrongbranch => $branch,
             Rightbranch => $hbr,
         };
-        $doreturn = 0;  # Could we bail here?
+        $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
@@ -1458,26 +1515,30 @@ sub AddReturn {
     # case of a return of document (deal with issues and holdingbranch)
     if ($doreturn) {
         $borrower or warn "AddReturn without current borrower";
-               my $circControlBranch = _GetCirculationBranch($item,$borrower);
+               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 ( $item->{'issuedate'} eq C4::Dates->today('iso') );
+            # define circControlBranch only if dropbox mode is set
+            # don't allow dropbox mode to create an invalid entry in issues (issuedate > today)
+            # FIXME: check issuedate > returndate, factoring in holidays
+            $circControlBranch = _GetCircControlBranch($item,$borrower) unless ( $item->{'issuedate'} eq C4::Dates->today('iso') );;
         }
 
         if ($borrowernumber) {
-            MarkIssueReturned($borrowernumber, $item->{'itemnumber'}, $circControlBranch);
+            MarkIssueReturned($borrowernumber, $item->{'itemnumber'}, $circControlBranch, '', $borrower->{'privacy'});
             $messages->{'WasReturned'} = 1;    # FIXME is the "= 1" right?  This could be the borrower hash.
         }
 
-        # the holdingbranch is updated if the document is returned to another location.
-        if ($item->{'holdingbranch'} ne $branch) {
-            UpdateHoldingbranch($branch, $item->{'itemnumber'});
-            $item->{'holdingbranch'} = $branch; # update item data holdingbranch too
-        }
-        ModDateLastSeen( $item->{'itemnumber'} );
         ModItem({ onloan => undef }, $issue->{'biblionumber'}, $item->{'itemnumber'});
     }
 
+    # the holdingbranch is updated if the document is returned to another location.
+    # this is always done regardless of whether the item was on loan or not
+    if ($item->{'holdingbranch'} ne $branch) {
+        UpdateHoldingbranch($branch, $item->{'itemnumber'});
+        $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'} );
 
@@ -1550,14 +1611,14 @@ 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 ($branch ne $item->{$hbr}) and not $messages->{'WrongTransfer'} and ($validTransfert ne 1) ){
+    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, $item->{$hbr}, $item->{C4::Context->preference("BranchTransferLimitsType")} )
+             ! IsBranchTransferAllowed($branch, $hbr, $item->{C4::Context->preference("BranchTransferLimitsType")} )
            )) {
-            warn sprintf "about to call ModItemTransfer(%s, %s, %s)", $item->{'itemnumber'},$branch, $item->{$hbr};
-            warn "item: " . Dumper($item);
-            ModItemTransfer($item->{'itemnumber'}, $branch, $item->{$hbr});
+            $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}
@@ -1568,11 +1629,7 @@ sub AddReturn {
 
 =head2 MarkIssueReturned
 
-=over 4
-
-MarkIssueReturned($borrowernumber, $itemnumber, $dropbox_branch, $returndate);
-
-=back
+  MarkIssueReturned($borrowernumber, $itemnumber, $dropbox_branch, $returndate, $privacy);
 
 Unconditionally marks an issue as being returned by
 moving the C<issues> row to C<old_issues> and
@@ -1584,6 +1641,9 @@ it's safe to do this, i.e. last non-holiday > issuedate.
 if C<$returndate> is specified (in iso format), it is used as the date
 of the return. It is ignored when a dropbox_branch is passed in.
 
+C<$privacy> contains the privacy parameter. If the patron has set privacy to 2,
+the old_issue is immediately anonymised
+
 Ideally, this function would be internal to C<C4::Circulation>,
 not exported, but it is currently needed by one 
 routine in C<C4::Accounts>.
@@ -1591,7 +1651,7 @@ routine in C<C4::Accounts>.
 =cut
 
 sub MarkIssueReturned {
-    my ( $borrowernumber, $itemnumber, $dropbox_branch, $returndate ) = @_;
+    my ( $borrowernumber, $itemnumber, $dropbox_branch, $returndate, $privacy ) = @_;
     my $dbh   = C4::Context->dbh;
     my $query = "UPDATE issues SET returndate=";
     my @bind;
@@ -1615,6 +1675,16 @@ sub MarkIssueReturned {
                                   WHERE borrowernumber = ?
                                   AND itemnumber = ?");
     $sth_copy->execute($borrowernumber, $itemnumber);
+    # anonymise patron checkout immediately if $privacy set to 2 and AnonymousPatron is set to a valid borrowernumber
+    if ( $privacy == 2) {
+        # The default of 0 does not work due to foreign key constraints
+        # The anonymisation will fail quietly if AnonymousPatron is not a valid entry
+        my $anonymouspatron = (C4::Context->preference('AnonymousPatron')) ? C4::Context->preference('AnonymousPatron') : 0;
+        my $sth_ano = $dbh->prepare("UPDATE old_issues SET borrowernumber=?
+                                  WHERE borrowernumber = ?
+                                  AND itemnumber = ?");
+       $sth_ano->execute($anonymouspatron, $borrowernumber, $itemnumber);
+    }
     my $sth_del  = $dbh->prepare("DELETE FROM issues
                                   WHERE borrowernumber = ?
                                   AND itemnumber = ?");
@@ -1623,7 +1693,7 @@ sub MarkIssueReturned {
 
 =head2 _FixOverduesOnReturn
 
-    &_FixOverduesOnReturn($brn,$itm, $exemptfine, $dropboxmode);
+   &_FixOverduesOnReturn($brn,$itm, $exemptfine, $dropboxmode);
 
 C<$brn> borrowernumber
 
@@ -1687,7 +1757,7 @@ sub _FixOverduesOnReturn {
 
 =head2 _FixAccountForLostAndReturned
 
-       &_FixAccountForLostAndReturned($itemnumber, [$borrowernumber, $barcode]);
+  &_FixAccountForLostAndReturned($itemnumber, [$borrowernumber, $barcode]);
 
 Calculates the charge for a book lost and returned.
 
@@ -1774,37 +1844,41 @@ sub _FixAccountForLostAndReturned {
     return;
 }
 
-=head2 _GetCirculationBranch
+=head2 _GetCircControlBranch
 
-   _GetCirculationBranch($iteminfos,$borrower);
+   my $circ_control_branch = _GetCircControlBranch($iteminfos, $borrower);
 
 Internal function : 
-Return the branchcode that must be used for an item circulation
+
+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<$iteminfos> is a hashref to iteminfo. Only {homebranch or holdingbranch} is used.
 
 C<$borrower> is a hashref to borrower. Only {branchcode} is used.
 
-Internal function, called by AddReturn
-
 =cut
 
-sub _GetCirculationBranch {
-    my ($iteminfos, $borrower) = @_;
+sub _GetCircControlBranch {
+    my ($item, $borrower) = @_;
     my $circcontrol = C4::Context->preference('CircControl');
-       my $branch;
+    my $branch;
 
-    if ($circcontrol eq 'PickupLibrary'){
-       $branch= C4::Context->userenv->{'branch'};
-    }
-       elsif($circcontrol eq 'PatronLibrary'){
+    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};
+        }
     }
-       else {
-               my $branchfield=C4::Context->preference("HomeOrHoldingBranch")||"homebranch";
-        $branch= $iteminfos->{$branchfield};
-       }
-       return $branch;
+    return $branch;
 }
 
 
@@ -1814,7 +1888,7 @@ sub _GetCirculationBranch {
 
 =head2 GetItemIssue
 
-$issue = &GetItemIssue($itemnumber);
+  $issue = &GetItemIssue($itemnumber);
 
 Returns patron currently having a book, or undef if not checked out.
 
@@ -1841,7 +1915,7 @@ sub GetItemIssue {
 
 =head2 GetOpenIssue
 
-$issue = GetOpenIssue( $itemnumber );
+  $issue = GetOpenIssue( $itemnumber );
 
 Returns the row from the issues table if the item is currently issued, undef if the item is not currently issued
 
@@ -1863,7 +1937,7 @@ sub GetOpenIssue {
 
 =head2 GetItemIssues
 
-$issues = &GetItemIssues($itemnumber, $history);
+  $issues = &GetItemIssues($itemnumber, $history);
 
 Returns patrons that have issued a book
 
@@ -1906,7 +1980,7 @@ sub GetItemIssues {
 
 =head2 GetBiblioIssues
 
-$issues = GetBiblioIssues($biblionumber);
+  $issues = GetBiblioIssues($biblionumber);
 
 this function get all issues from a biblionumber.
 
@@ -1950,11 +2024,7 @@ sub GetBiblioIssues {
 
 =head2 GetUpcomingDueIssues
 
-=over 4
-my $upcoming_dues = GetUpcomingDueIssues( { days_in_advance => 4 } );
-
-=back
+  my $upcoming_dues = GetUpcomingDueIssues( { days_in_advance => 4 } );
 
 =cut
 
@@ -1984,7 +2054,7 @@ END_SQL
 
 =head2 CanBookBeRenewed
 
-($ok,$error) = &CanBookBeRenewed($borrowernumber, $itemnumber[, $override_limit]);
+  ($ok,$error) = &CanBookBeRenewed($borrowernumber, $itemnumber[, $override_limit]);
 
 Find out whether a borrowed item may be renewed.
 
@@ -2018,39 +2088,52 @@ sub CanBookBeRenewed {
     # Look in the issues table for this item, lent to this borrower,
     # and not yet returned.
 
-    # FIXME - I think this function could be redone to use only one SQL call.
-    my $sth1 = $dbh->prepare(
-        "SELECT * FROM issues
-            WHERE borrowernumber = ?
-            AND itemnumber = ?"
-    );
-    $sth1->execute( $borrowernumber, $itemnumber );
-    if ( my $data1 = $sth1->fetchrow_hashref ) {
-
-        # Found a matching item
-
-        # See if this item may be renewed. This query is convoluted
-        # because it's a bit messy: given the item number, we need to find
-        # the biblioitem, which gives us the itemtype, which tells us
-        # whether it may be renewed.
-        my $query = "SELECT renewalsallowed FROM items ";
-        $query .= (C4::Context->preference('item-level_itypes'))
-                    ? "LEFT JOIN itemtypes ON items.itype = itemtypes.itemtype "
-                    : "LEFT JOIN biblioitems on items.biblioitemnumber = biblioitems.biblioitemnumber
-                       LEFT JOIN itemtypes ON biblioitems.itemtype = itemtypes.itemtype ";
-        $query .= "WHERE items.itemnumber = ?";
-        my $sth2 = $dbh->prepare($query);
-        $sth2->execute($itemnumber);
-        if ( my $data2 = $sth2->fetchrow_hashref ) {
-            $renews = $data2->{'renewalsallowed'};
-        }
-        if ( ( $renews && $renews > $data1->{'renewals'} ) || $override_limit ) {
+    # Look in the issues table for this item, lent to this borrower,
+    # and not yet returned.
+    my %branch = (
+            'ItemHomeLibrary' => 'items.homebranch',
+            'PickupLibrary'   => 'items.holdingbranch',
+            'PatronLibrary'   => 'borrowers.branchcode'
+            );
+    my $controlbranch = $branch{C4::Context->preference('CircControl')};
+    my $itype         = C4::Context->preference('item-level_itypes') ? 'items.itype' : 'biblioitems.itemtype';
+    
+    my $sthcount = $dbh->prepare("
+                   SELECT 
+                    borrowers.categorycode, biblioitems.itemtype, issues.renewals, renewalsallowed, $controlbranch
+                   FROM  issuingrules, 
+                   issues 
+                   LEFT JOIN items USING (itemnumber) 
+                   LEFT JOIN borrowers USING (borrowernumber) 
+                   LEFT JOIN biblioitems USING (biblioitemnumber)
+                   
+                   WHERE
+                    (issuingrules.categorycode = borrowers.categorycode OR issuingrules.categorycode = '*')
+                   AND
+                    (issuingrules.itemtype = $itype OR issuingrules.itemtype = '*')
+                   AND
+                    (issuingrules.branchcode = $controlbranch OR issuingrules.branchcode = '*') 
+                   AND 
+                    borrowernumber = ? 
+                   AND
+                    itemnumber = ?
+                   ORDER BY
+                    issuingrules.categorycode desc,
+                    issuingrules.itemtype desc,
+                    issuingrules.branchcode desc
+                   LIMIT 1;
+                  ");
+
+    $sthcount->execute( $borrowernumber, $itemnumber );
+    if ( my $data1 = $sthcount->fetchrow_hashref ) {
+        
+        if ( ( $data1->{renewalsallowed} && $data1->{renewalsallowed} > $data1->{renewals} ) || $override_limit ) {
             $renewokay = 1;
         }
         else {
                        $error="too_many";
                }
-        $sth2->finish;
+               
         my ( $resfound, $resrec ) = C4::Reserves::CheckReserves($itemnumber);
         if ($resfound) {
             $renewokay = 0;
@@ -2058,13 +2141,12 @@ sub CanBookBeRenewed {
         }
 
     }
-    $sth1->finish;
     return ($renewokay,$error);
 }
 
 =head2 AddRenewal
 
-&AddRenewal($borrowernumber, $itemnumber, $branch, [$datedue], [$lastreneweddate]);
+  &AddRenewal($borrowernumber, $itemnumber, $branch, [$datedue], [$lastreneweddate]);
 
 Renews a loan.
 
@@ -2166,39 +2248,42 @@ sub AddRenewal {
 
 sub GetRenewCount {
     # check renewal status
-    my ($bornum,$itemno)=@_;
-    my $dbh = C4::Context->dbh;
-    my $renewcount = 0;
-        my $renewsallowed = 0;
-        my $renewsleft = 0;
+    my ( $bornum, $itemno ) = @_;
+    my $dbh           = C4::Context->dbh;
+    my $renewcount    = 0;
+    my $renewsallowed = 0;
+    my $renewsleft    = 0;
+
+    my $borrower = C4::Members::GetMemberDetails($bornum);
+    my $item     = GetItem($itemno); 
+
     # Look in the issues table for this item, lent to this borrower,
     # and not yet returned.
 
     # FIXME - I think this function could be redone to use only one SQL call.
-    my $sth = $dbh->prepare("select * from issues
+    my $sth = $dbh->prepare(
+        "select * from issues
                                 where (borrowernumber = ?)
-                                and (itemnumber = ?)");
-    $sth->execute($bornum,$itemno);
+                                and (itemnumber = ?)"
+    );
+    $sth->execute( $bornum, $itemno );
     my $data = $sth->fetchrow_hashref;
     $renewcount = $data->{'renewals'} if $data->{'renewals'};
     $sth->finish;
-    my $query = "SELECT renewalsallowed FROM items ";
-    $query .= (C4::Context->preference('item-level_itypes'))
-                ? "LEFT JOIN itemtypes ON items.itype = itemtypes.itemtype "
-                : "LEFT JOIN biblioitems on items.biblioitemnumber = biblioitems.biblioitemnumber
-                   LEFT JOIN itemtypes ON biblioitems.itemtype = itemtypes.itemtype ";
-    $query .= "WHERE items.itemnumber = ?";
-    my $sth2 = $dbh->prepare($query);
-    $sth2->execute($itemno);
-    my $data2 = $sth2->fetchrow_hashref();
-    $renewsallowed = $data2->{'renewalsallowed'};
-    $renewsleft = $renewsallowed - $renewcount;
-    return ($renewcount,$renewsallowed,$renewsleft);
+    # $item and $borrower should be calculated
+    my $branchcode = _GetCircControlBranch($item, $borrower);
+    
+    my $issuingrule = GetIssuingRule($borrower->{categorycode}, $item->{itype}, $branchcode);
+    
+    $renewsallowed = $issuingrule->{'renewalsallowed'};
+    $renewsleft    = $renewsallowed - $renewcount;
+    if($renewsleft < 0){ $renewsleft = 0; }
+    return ( $renewcount, $renewsallowed, $renewsleft );
 }
 
 =head2 GetIssuingCharges
 
-($charge, $item_type) = &GetIssuingCharges($itemnumber, $borrowernumber);
+  ($charge, $item_type) = &GetIssuingCharges($itemnumber, $borrowernumber);
 
 Calculate how much it would cost for a given patron to borrow a given
 item, including any applicable discounts.
@@ -2238,7 +2323,7 @@ sub GetIssuingCharges {
         my $q2 = "SELECT rentaldiscount FROM borrowers
             LEFT JOIN issuingrules ON borrowers.categorycode = issuingrules.categorycode
             WHERE borrowers.borrowernumber = ?
-            AND issuingrules.itemtype = ?";
+            AND (issuingrules.itemtype = ? OR issuingrules.itemtype = '*')";
         my $sth2 = $dbh->prepare($q2);
         $sth2->execute( $borrowernumber, $item_type );
         if ( my $data2 = $sth2->fetchrow_hashref ) {
@@ -2257,7 +2342,7 @@ sub GetIssuingCharges {
 
 =head2 AddIssuingCharge
 
-&AddIssuingCharge( $itemno, $borrowernumber, $charge )
+  &AddIssuingCharge( $itemno, $borrowernumber, $charge )
 
 =cut
 
@@ -2279,7 +2364,7 @@ sub AddIssuingCharge {
 
 =head2 GetTransfers
 
-GetTransfers($itemnumber);
+  GetTransfers($itemnumber);
 
 =cut
 
@@ -2305,7 +2390,7 @@ sub GetTransfers {
 
 =head2 GetTransfersFromTo
 
-@results = GetTransfersFromTo($frombranch,$tobranch);
+  @results = GetTransfersFromTo($frombranch,$tobranch);
 
 Returns the list of pending transfers between $from and $to branch
 
@@ -2335,7 +2420,7 @@ sub GetTransfersFromTo {
 
 =head2 DeleteTransfer
 
-&DeleteTransfer($itemnumber);
+  &DeleteTransfer($itemnumber);
 
 =cut
 
@@ -2353,11 +2438,14 @@ sub DeleteTransfer {
 
 =head2 AnonymiseIssueHistory
 
-$rows = AnonymiseIssueHistory($borrowernumber,$date)
+  $rows = AnonymiseIssueHistory($date,$borrowernumber)
 
 This function write NULL instead of C<$borrowernumber> given on input arg into the table issues.
 if C<$borrowernumber> is not set, it will delete the issue history for all borrower older than C<$date>.
 
+If c<$borrowernumber> is set, it will delete issue history for only that borrower, regardless of their opac privacy
+setting (force delete).
+
 return the number of affected rows.
 
 =cut
@@ -2368,12 +2456,24 @@ sub AnonymiseIssueHistory {
     my $dbh            = C4::Context->dbh;
     my $query          = "
         UPDATE old_issues
-        SET    borrowernumber = NULL
-        WHERE  returndate < '".$date."'
+        SET    borrowernumber = ?
+        WHERE  returndate < ?
           AND borrowernumber IS NOT NULL
     ";
-    $query .= " AND borrowernumber = '".$borrowernumber."'" if defined $borrowernumber;
-    my $rows_affected = $dbh->do($query);
+
+    # The default of 0 does not work due to foreign key constraints
+    # The anonymisation will fail quietly if AnonymousPatron is not a valid entry
+    my $anonymouspatron = (C4::Context->preference('AnonymousPatron')) ? C4::Context->preference('AnonymousPatron') : 0;
+    my @bind_params = ($anonymouspatron, $date);
+    if (defined $borrowernumber) {
+       $query .= " AND borrowernumber = ?";
+       push @bind_params, $borrowernumber;
+    } else {
+       $query .= " AND (SELECT privacy FROM borrowers WHERE borrowers.borrowernumber=old_issues.borrowernumber) <> 0";
+    }
+    my $sth = $dbh->prepare($query);
+    $sth->execute(@bind_params);
+    my $rows_affected = $sth->rows;  ### doublecheck row count return function
     return $rows_affected;
 }
 
@@ -2450,7 +2550,7 @@ sub SendCirculationAlert {
 
 =head2 updateWrongTransfer
 
-$items = updateWrongTransfer($itemNumber,$borrowernumber,$waitingAtLibrary,$FromLibrary);
+  $items = updateWrongTransfer($itemNumber,$borrowernumber,$waitingAtLibrary,$FromLibrary);
 
 This function validate the line of brachtransfer but with the wrong destination (mistake from a librarian ...), and create a new line in branchtransfer from the actual library to the original library of reservation 
 
@@ -2476,7 +2576,8 @@ sub updateWrongTransfer {
 
 =head2 UpdateHoldingbranch
 
-$items = UpdateHoldingbranch($branch,$itmenumber);
+  $items = UpdateHoldingbranch($branch,$itmenumber);
+
 Simple methode for updating hodlingbranch in items BDD line
 
 =cut
@@ -2528,15 +2629,18 @@ sub CalcDateDue {
 }
 
 =head2 CheckValidDatedue
-       This function does not account for holiday exceptions nor does it handle the 'useDaysMode' syspref .
-       To be replaced by CalcDateDue() once C4::Calendar use is tested.
 
-$newdatedue = CheckValidDatedue($date_due,$itemnumber,$branchcode);
+  $newdatedue = CheckValidDatedue($date_due,$itemnumber,$branchcode);
+
+This function does not account for holiday exceptions nor does it handle the 'useDaysMode' syspref .
+To be replaced by CalcDateDue() once C4::Calendar use is tested.
+
 this function validates the loan length against the holidays calendar, and adjusts the due date as per the 'useDaysMode' syspref.
 C<$date_due>   = returndate calculate with no day check
 C<$itemnumber>  = itemnumber
 C<$branchcode>  = location of issue (affected by 'CircControl' syspref)
 C<$loanlength>  = loan length prior to adjustment
+
 =cut
 
 sub CheckValidDatedue {
@@ -2565,8 +2669,10 @@ return $newdatedue;
 
 =head2 CheckRepeatableHolidays
 
-$countrepeatable = CheckRepeatableHoliday($itemnumber,$week_day,$branchcode);
-this function checks if the date due is a repeatable holiday
+  $countrepeatable = CheckRepeatableHoliday($itemnumber,$week_day,$branchcode);
+
+This function checks if the date due is a repeatable holiday
+
 C<$date_due>   = returndate calculate with no day check
 C<$itemnumber>  = itemnumber
 C<$branchcode>  = localisation of issue 
@@ -2590,8 +2696,10 @@ return $result;
 
 =head2 CheckSpecialHolidays
 
-$countspecial = CheckSpecialHolidays($years,$month,$day,$itemnumber,$branchcode);
-this function check if the date is a special holiday
+  $countspecial = CheckSpecialHolidays($years,$month,$day,$itemnumber,$branchcode);
+
+This function check if the date is a special holiday
+
 C<$years>   = the years of datedue
 C<$month>   = the month of datedue
 C<$day>     = the day of datedue
@@ -2619,8 +2727,10 @@ return $countspecial;
 
 =head2 CheckRepeatableSpecialHolidays
 
-$countspecial = CheckRepeatableSpecialHolidays($month,$day,$itemnumber,$branchcode);
-this function check if the date is a repeatble special holidays
+  $countspecial = CheckRepeatableSpecialHolidays($month,$day,$itemnumber,$branchcode);
+
+This function check if the date is a repeatble special holidays
+
 C<$month>   = the month of datedue
 C<$day>     = the day of datedue
 C<$itemnumber>  = itemnumber
@@ -2662,7 +2772,7 @@ return $exist;
 
 =head2 IsBranchTransferAllowed
 
-$allowed = IsBranchTransferAllowed( $toBranch, $fromBranch, $code );
+  $allowed = IsBranchTransferAllowed( $toBranch, $fromBranch, $code );
 
 Code is either an itemtype or collection doe depending on the pref BranchTransferLimitsType
 
@@ -2690,7 +2800,7 @@ sub IsBranchTransferAllowed {
 
 =head2 CreateBranchTransferLimit
 
-CreateBranchTransferLimit( $toBranch, $fromBranch, $code );
+  CreateBranchTransferLimit( $toBranch, $fromBranch, $code );
 
 $code is either itemtype or collection code depending on what the pref BranchTransferLimitsType is set to.
 
@@ -2709,7 +2819,7 @@ sub CreateBranchTransferLimit {
 
 =head2 DeleteBranchTransferLimits
 
-DeleteBranchTransferLimits();
+  DeleteBranchTransferLimits();
 
 =cut
 
@@ -2726,7 +2836,7 @@ __END__
 
 =head1 AUTHOR
 
-Koha Developement team <info@koha.org>
+Koha Development Team <http://koha-community.org/>
 
 =cut