From b6385330c730619adffd200c4f1054c771ed3674 Mon Sep 17 00:00:00 2001
From: Kyle M Hall <kyle@bywatersolutions.com>
Date: Mon, 24 Nov 2014 09:53:00 -0500
Subject: [PATCH] Bug 13330 - All overdue items with fines returned via checkouts table will have fines forgiven!

Due to a disconnect between how booleans are handled in Perl vs JSON,
all checkins done from the checkouts table will have fines forgiven!
This is because the parameters sent to svc/checkin are coded in JSON
and the JSON false is treated in Perl as the string 'false' which
evaluates to true!

Test Plan:
1) Find a patron with an overdue item that has a fine
2) Return the item via the checkouts table on circulation.pl or moremember.pl
3) Note the fine is forgiven
4) Apply this patch
5) Repeat steps 1-2
6) Note the fine is no longer forgiven
---
 C4/Circulation.pm |  129 +++++++++++++++++++++++++++--------------------------
 svc/checkin       |    4 ++
 2 files changed, 69 insertions(+), 64 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index f24f2f7..b8c5ce6 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -149,11 +149,11 @@ 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.
-For proper functioning of this filter, calling the function on the 
+For proper functioning of this filter, calling the function on the
 correct barcode string (items.barcode) should return an unaltered barcode.
 
-The optional $filter argument is to allow for testing or explicit 
-behavior that ignores the System Pref.  Valid values are the same as the 
+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.
 
 =cut
@@ -243,7 +243,7 @@ sub decode {
 
 =head2 transferbook
 
-  ($dotransfer, $messages, $iteminformation) = &transferbook($newbranch, 
+  ($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.
@@ -259,7 +259,7 @@ Returns three values:
 
 =over
 
-=item $dotransfer 
+=item $dotransfer
 
 is true if the transfer was successful.
 
@@ -382,10 +382,10 @@ sub TooMany {
 	my $branch;
 	# Get which branchcode we need
 	$branch = _GetCircControlBranch($item,$borrower);
-	my $type = (C4::Context->preference('item-level_itypes')) 
+	my $type = (C4::Context->preference('item-level_itypes'))
   			? $item->{'itype'}         # item-level
 			: $item->{'itemtype'};     # biblio-level
- 
+
     # given branch, patron category, and item type, determine
     # applicable issuing rule
     my $issuing_rule = GetIssuingRule($cat_borrower, $type, $branch);
@@ -410,8 +410,8 @@ sub TooMany {
                                     AND   (categorycode = ? OR categorycode = ?)
                                     AND   itemtype <> '*'
                                   ) ";
-            } else { 
-                $count_query .= " JOIN  biblioitems USING (biblionumber) 
+            } else {
+                $count_query .= " JOIN  biblioitems USING (biblionumber)
                                   WHERE biblioitems.itemtype NOT IN (
                                     SELECT itemtype FROM issuingrules
                                     WHERE branchcode = ?
@@ -427,8 +427,8 @@ sub TooMany {
             # specific item type
             if (C4::Context->preference('item-level_itypes')) {
                 $count_query .= " WHERE items.itype = ? ";
-            } else { 
-                $count_query .= " JOIN  biblioitems USING (biblionumber) 
+            } else {
+                $count_query .= " JOIN  biblioitems USING (biblionumber)
                                   WHERE biblioitems.itemtype= ? ";
             }
             push @bind_params, $type;
@@ -601,7 +601,7 @@ sub itemissues {
 
 =head2 CanBookBeIssued
 
-  ( $issuingimpossible, $needsconfirmation ) =  CanBookBeIssued( $borrower, 
+  ( $issuingimpossible, $needsconfirmation ) =  CanBookBeIssued( $borrower,
                       $barcode, $duedatespec, $inprocess, $ignore_reserves );
 
 Check if a book can be issued.
@@ -630,7 +630,7 @@ Possible values are :
 
 =back
 
-=head3 INVALID_DATE 
+=head3 INVALID_DATE
 
 sticky due date is invalid
 
@@ -662,7 +662,7 @@ item withdrawn.
 
 item is restricted (set by ??)
 
-C<$needsconfirmation> a reference to a hash. It contains reasons why the loan 
+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 :
@@ -702,7 +702,7 @@ sub CanBookBeIssued {
     my $item = GetItem(GetItemnumberFromBarcode( $barcode ));
     my $issue = GetItemIssue($item->{itemnumber});
 	my $biblioitem = GetBiblioItemData($item->{biblioitemnumber});
-	$item->{'itemtype'}=$item->{'itype'}; 
+	$item->{'itemtype'}=$item->{'itype'};
     my $dbh             = C4::Context->dbh;
 
     # MANDATORY CHECKS - unless item exists, nothing else matters
@@ -741,7 +741,7 @@ sub CanBookBeIssued {
     #
     # BORROWER STATUS
     #
-    if ( $borrower->{'category_type'} eq 'X' && (  $item->{barcode}  )) { 
+    if ( $borrower->{'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'},
@@ -1210,13 +1210,13 @@ sub AddIssue {
 		# find which item we issue
 		my $item = GetItem('', $barcode) or return;	# if we don't get an Item, abort.
 		my $branch = _GetCircControlBranch($item,$borrower);
-		
+
 		# get actual issuing if there is one
 		my $actualissue = GetItemIssue( $item->{itemnumber});
-		
+
 		# get biblioinformation for this item
 		my $biblio = GetBiblioFromItemNumber($item->{itemnumber});
-		
+
 		#
 		# check if we just renew the issue.
 		#
@@ -1247,7 +1247,7 @@ sub AddIssue {
         # 	updating line of branchtranfert to finish it, and changing the to branch value, implement a comment for visibility of this case (maybe for stats ....)
                 my $sth =
                     $dbh->prepare(
-                    "UPDATE branchtransfers 
+                    "UPDATE branchtransfers
                         SET datearrived = now(),
                         tobranch = ?,
                         comments = 'Forced branchtransfer'
@@ -1457,7 +1457,7 @@ sub GetHardDueDate {
   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 
+as a stop-gap.  Do not wish to change API for GetLoanLength
 this close to release.
 
 Get the issuing rule for an itemtype, a borrower type and a branch
@@ -1512,7 +1512,7 @@ sub GetIssuingRule {
   my $branch_cat_rule = GetBranchBorrowerCircRule($branchcode, $categorycode);
 
 Retrieves circulation rule attributes that apply to the given
-branch and patron category, regardless of item type.  
+branch and patron category, regardless of item type.
 The return value is a hashref containing the following key:
 
 maxissueqty - maximum number of loans that a
@@ -1520,11 +1520,11 @@ patron of the given category can have at the given
 branch.  If the value is undef, no limit.
 
 This will first check for a specific branch and
-category match from branch_borrower_circ_rules. 
+category match from branch_borrower_circ_rules.
 
 If no rule is found, it will then check default_branch_circ_rules
 (same branch, default category).  If no rule is found,
-it will then check default_borrower_circ_rules (default 
+it will then check default_borrower_circ_rules (default
 branch, same category), then failing that, default_circ_rules
 (default branch, default category).
 
@@ -1574,7 +1574,7 @@ sub GetBranchBorrowerCircRule {
     if ($result = $sth->fetchrow_hashref()) {
         return $result;
     }
-  
+
     # try default branch, default borrower category
     my $default_query = "SELECT maxissueqty
                           FROM default_circ_rules";
@@ -1583,7 +1583,7 @@ sub GetBranchBorrowerCircRule {
     if ($result = $sth->fetchrow_hashref()) {
         return $result;
     }
-    
+
     # built-in default circulation rule
     return {
         maxissueqty => undef,
@@ -1650,7 +1650,7 @@ sub GetBranchItemRule {
         $result->{'holdallowed'}  = $search_result->{'holdallowed'}  unless ( defined $result->{'holdallowed'} );
         $result->{'returnbranch'} = $search_result->{'returnbranch'} unless ( defined $result->{'returnbranch'} );
     }
-    
+
     # built-in default circulation rule
     $result->{'holdallowed'} = 2 unless ( defined $result->{'holdallowed'} );
     $result->{'returnbranch'} = 'homebranch' unless ( defined $result->{'returnbranch'} );
@@ -1737,6 +1737,7 @@ patron who last borrowed the book.
 
 sub AddReturn {
     my ( $barcode, $branch, $exemptfine, $dropbox, $return_date ) = @_;
+    warn "AddReturn( $barcode, $branch, $exemptfine, $dropbox, $return_date )";
 
     if ($branch and not GetBranchDetail($branch)) {
         warn "AddReturn error: branch '$branch' not found.  Reverting to " . C4::Context->userenv->{'branch'};
@@ -1947,7 +1948,7 @@ sub AddReturn {
     if ($borrowernumber) {
         my $fix = _FixOverduesOnReturn($borrowernumber, $item->{itemnumber}, $exemptfine, $dropbox);
         defined($fix) or warn "_FixOverduesOnReturn($borrowernumber, $item->{itemnumber}...) failed!";  # zero is OK, check defined
-        
+
         if ( $issue->{overdue} && $issue->{date_due} ) {
         # fix fine days
             my ($debardate,$reminder) = _debar_user_on_return( $borrower, $item, $issue->{date_due}, $today );
@@ -2008,10 +2009,10 @@ sub AddReturn {
             branch   => $branch,
         });
     }
-    
+
     logaction("CIRCULATION", "RETURN", $borrowernumber, $item->{'itemnumber'})
         if C4::Context->preference("ReturnLog");
-    
+
     # Remove any OVERDUES related debarment if the borrower has no overdues
     if ( $borrowernumber
       && $borrower->{'debarred'}
@@ -2051,7 +2052,7 @@ Unconditionally marks an issue as being returned by
 moving the C<issues> row to C<old_issues> and
 setting C<returndate> to the current date, or
 the last non-holiday date of the branccode specified in
-C<dropbox_branch> .  Assumes you've already checked that 
+C<dropbox_branch> .  Assumes you've already checked that
 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
@@ -2061,7 +2062,7 @@ 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 
+not exported, but it is currently needed by one
 routine in C<C4::Accounts>.
 
 =cut
@@ -2193,7 +2194,7 @@ C<$brn> borrowernumber
 
 C<$itm> itemnumber
 
-C<$exemptfine> BOOL -- remove overdue charge associated with this issue. 
+C<$exemptfine> BOOL -- remove overdue charge associated with this issue.
 C<$dropboxmode> BOOL -- remove lastincrement on overdue charge associated with this issue.
 
 Internal function, called only by AddReturn
@@ -2340,7 +2341,7 @@ sub _FixAccountForLostAndReturned {
 
    my $circ_control_branch = _GetCircControlBranch($iteminfos, $borrower);
 
-Internal function : 
+Internal function :
 
 Return the library code to be used to determine which circulation
 policy applies to a transaction.  Looks up the CircControl and
@@ -2425,7 +2426,7 @@ Returns a hashref
 sub GetOpenIssue {
   my ( $itemnumber ) = @_;
   return unless $itemnumber;
-  my $dbh = C4::Context->dbh;  
+  my $dbh = C4::Context->dbh;
   my $sth = $dbh->prepare( "SELECT * FROM issues WHERE itemnumber = ? AND returndate IS NULL" );
   $sth->execute( $itemnumber );
   return $sth->fetchrow_hashref();
@@ -2515,7 +2516,7 @@ Returns reference to an array of hashes
 
 sub GetItemIssues {
     my ( $itemnumber, $history ) = @_;
-    
+
     my $today = DateTime->now( time_zome => C4::Context->tz);  # get today date
     $today->truncate( to => 'minute' );
     my $sql = "SELECT * FROM issues
@@ -2604,7 +2605,7 @@ sub GetUpcomingDueIssues {
 
     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 
+FROM issues
 LEFT JOIN items USING (itemnumber)
 LEFT OUTER JOIN branches USING (branchcode)
 WHERE returndate is NULL
@@ -2612,7 +2613,7 @@ HAVING days_until_due >= 0 AND days_until_due <= ?
 END_SQL
 
     my @bind_parameters = ( $params->{'days_in_advance'} );
-    
+
     my $sth = $dbh->prepare( $statement );
     $sth->execute( @bind_parameters );
     my $upcoming_dues = $sth->fetchall_arrayref({});
@@ -2813,7 +2814,7 @@ sub AddRenewal {
     # of how many times it has been renewed.
     my $renews = $issuedata->{'renewals'} + 1;
     $sth = $dbh->prepare("UPDATE issues SET date_due = ?, renewals = ?, lastreneweddate = ?
-                            WHERE borrowernumber=? 
+                            WHERE borrowernumber=?
                             AND itemnumber=?"
     );
 
@@ -2829,7 +2830,7 @@ sub AddRenewal {
         my $accountno = getnextacctno( $borrowernumber );
         my $item = GetBiblioFromItemNumber($itemnumber);
         my $manager_id = 0;
-        $manager_id = C4::Context->userenv->{'number'} if C4::Context->userenv; 
+        $manager_id = C4::Context->userenv->{'number'} if C4::Context->userenv;
         $sth = $dbh->prepare(
                 "INSERT INTO accountlines
                     (date, borrowernumber, accountno, amount, manager_id,
@@ -2892,7 +2893,7 @@ sub GetRenewCount {
     my $renewsleft    = 0;
 
     my $borrower = C4::Members::GetMember( borrowernumber => $bornum);
-    my $item     = GetItem($itemno); 
+    my $item     = GetItem($itemno);
 
     # Look in the issues table for this item, lent to this borrower,
     # and not yet returned.
@@ -2908,9 +2909,9 @@ sub GetRenewCount {
     $renewcount = $data->{'renewals'} if $data->{'renewals'};
     # $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; }
@@ -3286,13 +3287,13 @@ sub SendCirculationAlert {
 
   $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 
+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
 
 =cut
 
 sub updateWrongTransfer {
 	my ( $itemNumber,$waitingAtLibrary,$FromLibrary ) = @_;
-	my $dbh = C4::Context->dbh;	
+	my $dbh = C4::Context->dbh;
 # first step validate the actual line of transfert .
 	my $sth =
         	$dbh->prepare(
@@ -3428,15 +3429,15 @@ 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 
+C<$branchcode>  = localisation of issue
 
 =cut
 
 sub CheckRepeatableHolidays{
 my($itemnumber,$week_day,$branchcode)=@_;
 my $dbh = C4::Context->dbh;
-my $query = qq|SELECT count(*)  
-	FROM repeatable_holidays 
+my $query = qq|SELECT count(*)
+	FROM repeatable_holidays
 	WHERE branchcode=?
 	AND weekday=?|;
 my $sth = $dbh->prepare($query);
@@ -3456,14 +3457,14 @@ C<$years>   = the years of datedue
 C<$month>   = the month of datedue
 C<$day>     = the day of datedue
 C<$itemnumber>  = itemnumber
-C<$branchcode>  = localisation of issue 
+C<$branchcode>  = localisation of issue
 
 =cut
 
 sub CheckSpecialHolidays{
 my ($years,$month,$day,$itemnumber,$branchcode) = @_;
 my $dbh = C4::Context->dbh;
-my $query=qq|SELECT count(*) 
+my $query=qq|SELECT count(*)
 	     FROM `special_holidays`
 	     WHERE year=?
 	     AND month=?
@@ -3485,14 +3486,14 @@ 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
-C<$branchcode>  = localisation of issue 
+C<$branchcode>  = localisation of issue
 
 =cut
 
 sub CheckRepeatableSpecialHolidays{
 my ($month,$day,$itemnumber,$branchcode) = @_;
 my $dbh = C4::Context->dbh;
-my $query=qq|SELECT count(*) 
+my $query=qq|SELECT count(*)
 	     FROM `repeatable_holidays`
 	     WHERE month=?
 	     AND day=?
@@ -3509,8 +3510,8 @@ return $countspecial;
 sub CheckValidBarcode{
 my ($barcode) = @_;
 my $dbh = C4::Context->dbh;
-my $query=qq|SELECT count(*) 
-	     FROM items 
+my $query=qq|SELECT count(*)
+	     FROM items
              WHERE barcode=?
 	    |;
 my $sth = $dbh->prepare($query);
@@ -3531,21 +3532,21 @@ sub IsBranchTransferAllowed {
 	my ( $toBranch, $fromBranch, $code ) = @_;
 
 	if ( $toBranch eq $fromBranch ) { return 1; } ## Short circuit for speed.
-        
-	my $limitType = C4::Context->preference("BranchTransferLimitsType");   
+
+	my $limitType = C4::Context->preference("BranchTransferLimitsType");
 	my $dbh = C4::Context->dbh;
-            
+
 	my $sth = $dbh->prepare("SELECT * FROM branch_transfer_limits WHERE toBranch = ? AND fromBranch = ? AND $limitType = ?");
 	$sth->execute( $toBranch, $fromBranch, $code );
 	my $limit = $sth->fetchrow_hashref();
-                        
+
 	## If a row is found, then that combination is not allowed, if no matching row is found, then the combination *is allowed*
 	if ( $limit->{'limitId'} ) {
 		return 0;
 	} else {
 		return 1;
 	}
-}                                                        
+}
 
 =head2 CreateBranchTransferLimit
 
@@ -3559,9 +3560,9 @@ sub CreateBranchTransferLimit {
    my ( $toBranch, $fromBranch, $code ) = @_;
    return unless defined($toBranch) && defined($fromBranch);
    my $limitType = C4::Context->preference("BranchTransferLimitsType");
-   
+
    my $dbh = C4::Context->dbh;
-   
+
    my $sth = $dbh->prepare("INSERT INTO branch_transfer_limits ( $limitType, toBranch, fromBranch ) VALUES ( ?, ?, ? )");
    return $sth->execute( $code, $toBranch, $fromBranch );
 }
@@ -3602,9 +3603,9 @@ sub LostItem{
     my ($itemnumber, $mark_returned) = @_;
 
     my $dbh = C4::Context->dbh();
-    my $sth=$dbh->prepare("SELECT issues.*,items.*,biblio.title 
-                           FROM issues 
-                           JOIN items USING (itemnumber) 
+    my $sth=$dbh->prepare("SELECT issues.*,items.*,biblio.title
+                           FROM issues
+                           JOIN items USING (itemnumber)
                            JOIN biblio USING (biblionumber)
                            WHERE issues.itemnumber=?");
     $sth->execute($itemnumber);
diff --git a/svc/checkin b/svc/checkin
index 2ca539c..c10a1b8 100755
--- a/svc/checkin
+++ b/svc/checkin
@@ -47,6 +47,10 @@ my $exempt_fine    = $input->param('exempt_fine');
 my $branchcode     = $input->param('branchcode')
   || C4::Context->userenv->{'branch'};
 
+# Expect these inputs to come in as JSON boolean values
+$override_limit = $override_limit ? $override_limit eq 'true' : undef;
+$exempt_fine    = $exempt_fine    ? $exempt_fine eq 'true'    : undef;
+
 my $barcode = GetBarcodeFromItemnumber($itemnumber);
 
 my $data;
-- 
1.7.2.5