From 73a8c20731e86e4cf00d96e046c34d3b6cabad72 Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date: Fri, 25 Nov 2016 13:53:03 +0100
Subject: [PATCH] Bug 17680: C4::Circulation - Remove GetItemIssue, complex
 calls
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

There are a few calls to GetItemIssue where it's not as easy to make
sure everything will be fine just replacing the calls with a
Koha::Issues->find
- In AddReturn the overdue flag is used (that's why this patch depends
on bug 17689)
- In CanBookBeRenewed, as well as the overdue flag the dates converted
to DateTime were used. It's now our job to convert them when we need
them.
- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we
need to update the variables in this subroutine.

Note that, prior to this patch, AddReturn returned the GetItemIssue
hashref in the $iteminformation. Most of the time this variable is not
used, I have found only 1 place where it's used: circ/returns.pl
TODO: In this script we should call ->is_overdue instead of the
DateTime->compare calls

Test plan:
All the circulation tests must pass (it's how I have caught the specific
cases).
Do some checkins/checkouts/renewal and focus on the due date

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
---
 C4/Circulation.pm | 83 +++++++++++++++++++++++++++----------------------------
 circ/returns.pl   | 11 ++++----
 2 files changed, 47 insertions(+), 47 deletions(-)

diff --git a/C4/Circulation.pm b/C4/Circulation.pm
index 2155bbf..4a27c3b 100644
--- a/C4/Circulation.pm
+++ b/C4/Circulation.pm
@@ -1833,11 +1833,11 @@ sub AddReturn {
     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 $issue  = GetItemIssue($itemnumber);
-    if ($issue and $issue->{borrowernumber}) {
-        $borrower = C4::Members::GetMember( borrowernumber => $issue->{borrowernumber} )
-            or die "Data inconsistency: barcode $barcode (itemnumber:$itemnumber) claims to be issued to non-existent borrowernumber '$issue->{borrowernumber}'\n"
-                . Dumper($issue) . "\n";
+    my $issue  = Koha::Checkouts->find( { itemnumber => $itemnumber } );
+    if ( $issue ) {
+        $borrower = C4::Members::GetMember( borrowernumber => $issue->borrowernumber )
+            or die "Data inconsistency: barcode $barcode (itemnumber:$itemnumber) claims to be issued to non-existent borrowernumber '" . $issue->borrowernumber . "'\n"
+                . Dumper($issue->unblessed) . "\n";
     } else {
         $messages->{'NotIssued'} = $barcode;
         # even though item is not on loan, it may still be transferred;  therefore, get current branch info
@@ -1919,7 +1919,8 @@ sub AddReturn {
     my $today = DateTime->now( time_zone => C4::Context->tz() );
 
     if ($doreturn) {
-        my $datedue = $issue->{date_due};
+        my $is_overdue;
+        die "The item is not issed and cannot be returned" unless $issue; # Just in case...
         $borrower or warn "AddReturn without current borrower";
 		my $circControlBranch;
         if ($dropbox) {
@@ -1928,7 +1929,7 @@ sub AddReturn {
             # FIXME: check issuedate > returndate, factoring in holidays
 
             $circControlBranch = _GetCircControlBranch($item,$borrower);
-            $issue->{'overdue'} = DateTime->compare($issue->{'date_due'}, $dropboxdate ) == -1 ? 1 : 0;
+            $is_overdue = $issue->is_overdue( $dropboxdate );
         }
 
         if ($borrowernumber) {
@@ -1938,7 +1939,7 @@ sub AddReturn {
                 $issue->{issue_id} = $issue_id;
             };
             unless ( $@ ) {
-                if ( ( C4::Context->preference('CalculateFinesOnReturn') && $issue->{'overdue'} ) || $return_date ) {
+                if ( ( C4::Context->preference('CalculateFinesOnReturn') && $is_overdue ) || $return_date ) {
                     _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $borrower, return_date => $return_date } );
                 }
             } else {
@@ -1955,7 +1956,7 @@ sub AddReturn {
 
         }
 
-        ModItem({ onloan => undef }, $issue->{'biblionumber'}, $item->{'itemnumber'});
+        ModItem({ onloan => undef }, $item->{biblionumber}, $item->{'itemnumber'});
     }
 
     # the holdingbranch is updated if the document is returned to another location.
@@ -2015,18 +2016,18 @@ 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} ) {
+
+        if ( $issue and $issue->is_overdue ) {
         # fix fine days
             $today = $dropboxdate if $dropbox;
-            my ($debardate,$reminder) = _debar_user_on_return( $borrower, $item, $issue->{date_due}, $today );
+            my ($debardate,$reminder) = _debar_user_on_return( $borrower, $item, dt_from_string($issue->date_due), $today );
             if ($reminder){
                 $messages->{'PrevDebarred'} = $debardate;
             } else {
                 $messages->{'Debarred'} = $debardate if $debardate;
             }
         # there's no overdue on the item but borrower had been previously debarred
-        } elsif ( $issue->{date_due} and $borrower->{'debarred'} ) {
+        } elsif ( $issue->date_due and $borrower->{'debarred'} ) {
              if ( $borrower->{debarred} eq "9999-12-31") {
                 $messages->{'ForeverDebarred'} = $borrower->{'debarred'};
              } else {
@@ -2621,10 +2622,10 @@ sub CanBookBeRenewed {
     my $renews = 1;
 
     my $item      = GetItem($itemnumber)      or return ( 0, 'no_item' );
-    my $itemissue = GetItemIssue($itemnumber) or return ( 0, 'no_checkout' );
-    return ( 0, 'onsite_checkout' ) if $itemissue->{onsite_checkout};
+    my $issue = Koha::Checkouts->find( { itemnumber => $itemnumber } ) or return ( 0, 'no_checkout' );
+    return ( 0, 'onsite_checkout' ) if $issue->onsite_checkout;
 
-    $borrowernumber ||= $itemissue->{borrowernumber};
+    $borrowernumber ||= $issue->borrowernumber;
     my $borrower = C4::Members::GetMember( borrowernumber => $borrowernumber )
       or return;
 
@@ -2706,7 +2707,7 @@ sub CanBookBeRenewed {
     );
 
     return ( 0, "too_many" )
-      if not $issuing_rule or $issuing_rule->renewalsallowed <= $itemissue->{renewals};
+      if not $issuing_rule or $issuing_rule->renewalsallowed <= $issue->renewals;
 
     my $overduesblockrenewing = C4::Context->preference('OverduesBlockRenewing');
     my $restrictionblockrenewing = C4::Context->preference('RestrictionBlockRenewing');
@@ -2716,16 +2717,16 @@ sub CanBookBeRenewed {
 
     if ( $restricted and $restrictionblockrenewing ) {
         return ( 0, 'restriction');
-    } elsif ( ($hasoverdues and $overduesblockrenewing eq 'block') || ($itemissue->{overdue} and $overduesblockrenewing eq 'blockitem') ) {
+    } elsif ( ($hasoverdues and $overduesblockrenewing eq 'block') || ($issue->is_overdue and $overduesblockrenewing eq 'blockitem') ) {
         return ( 0, 'overdue');
     }
 
-    if ( $itemissue->{auto_renew} ) {
+    if ( $issue->auto_renew ) {
         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
             # If this is greater than today, it's too late for renewal.
-            my $maximum_renewal_date = dt_from_string($itemissue->{issuedate});
+            my $maximum_renewal_date = dt_from_string($issue->issuedate, 'sql');
             $maximum_renewal_date->add(
                 $issuing_rule->lengthunit => $issuing_rule->no_auto_renewal_after
             );
@@ -2756,9 +2757,7 @@ sub CanBookBeRenewed {
     {
 
         # Calculate soonest renewal by subtracting 'No renewal before' from due date
-        my $soonestrenewal =
-          $itemissue->{date_due}->clone()
-          ->subtract(
+        my $soonestrenewal = dt_from_string( $issue->date_due, 'sql' )->subtract(
             $issuing_rule->lengthunit => $issuing_rule->norenewalbefore );
 
         # Depending on syspref reset the exact time, only check the date
@@ -2770,20 +2769,20 @@ sub CanBookBeRenewed {
 
         if ( $soonestrenewal > DateTime->now( time_zone => C4::Context->tz() ) )
         {
-            return ( 0, "auto_too_soon" ) if $itemissue->{auto_renew};
+            return ( 0, "auto_too_soon" ) if $issue->auto_renew;
             return ( 0, "too_soon" );
         }
-        elsif ( $itemissue->{auto_renew} ) {
+        elsif ( $issue->auto_renew ) {
             return ( 0, "auto_renew" );
         }
     }
 
     # Fallback for automatic renewals:
     # If norenewalbefore is undef, don't renew before due date.
-    if ( $itemissue->{auto_renew} ) {
+    if ( $issue->auto_renew ) {
         my $now = dt_from_string;
         return ( 0, "auto_renew" )
-          if $now >= $itemissue->{date_due};
+          if $now >= dt_from_string( $issue->date_due, 'sql' );
         return ( 0, "auto_too_soon" );
     }
 
@@ -2827,11 +2826,11 @@ sub AddRenewal {
     my $dbh = C4::Context->dbh;
 
     # Find the issues record for this book
-    my $issuedata  = GetItemIssue($itemnumber);
+    my $issue = Koha::Checkouts->find( { itemnumber => $itemnumber } );
 
-    return unless ( $issuedata );
+    return unless $issue;
 
-    $borrowernumber ||= $issuedata->{borrowernumber};
+    $borrowernumber ||= $issue->borrowernumber;
 
     if ( defined $datedue && ref $datedue ne 'DateTime' ) {
         carp 'Invalid date passed to AddRenewal.';
@@ -2840,8 +2839,8 @@ sub AddRenewal {
 
     my $borrower = C4::Members::GetMember( borrowernumber => $borrowernumber ) or return;
 
-    if ( C4::Context->preference('CalculateFinesOnReturn') && $issuedata->{overdue} ) {
-        _CalculateAndUpdateFine( { issue => $issuedata, item => $item, borrower => $borrower } );
+    if ( C4::Context->preference('CalculateFinesOnReturn') && $issue->is_overdue ) {
+        _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $borrower } );
     }
     _FixOverduesOnReturn( $borrowernumber, $itemnumber );
 
@@ -2853,14 +2852,14 @@ sub AddRenewal {
         my $itemtype = (C4::Context->preference('item-level_itypes')) ? $biblio->{'itype'} : $biblio->{'itemtype'};
 
         $datedue = (C4::Context->preference('RenewalPeriodBase') eq 'date_due') ?
-                                        dt_from_string( $issuedata->{date_due} ) :
+                                        dt_from_string( $issue->date_due, 'sql' ) :
                                         DateTime->now( time_zone => C4::Context->tz());
         $datedue =  CalcDateDue($datedue, $itemtype, _GetCircControlBranch($item, $borrower), $borrower, 'is a renewal');
     }
 
     # Update the issues record to have the new due date, and a new count
     # of how many times it has been renewed.
-    my $renews = $issuedata->{'renewals'} + 1;
+    my $renews = $issue->renewals + 1;
     my $sth = $dbh->prepare("UPDATE issues SET date_due = ?, renewals = ?, lastreneweddate = ?
                             WHERE borrowernumber=? 
                             AND itemnumber=?"
@@ -4023,7 +4022,7 @@ sub _CalculateAndUpdateFine {
     unless ($item)     { carp "No item passed in!"     && return; }
     unless ($issue)    { carp "No issue passed in!"    && return; }
 
-    my $datedue = $issue->{date_due};
+    my $datedue = dt_from_string( $issue->date_due );
 
     # we only need to calculate and change the fines if we want to do that on return
     # Should be on for hourly loans
@@ -4031,7 +4030,7 @@ sub _CalculateAndUpdateFine {
     my $control_branchcode =
         ( $control eq 'ItemHomeLibrary' ) ? $item->{homebranch}
       : ( $control eq 'PatronLibrary' )   ? $borrower->{branchcode}
-      :                                     $issue->{branchcode};
+      :                                     $issue->branchcode;
 
     my $date_returned = $return_date ? dt_from_string($return_date) : dt_from_string();
 
@@ -4043,9 +4042,9 @@ sub _CalculateAndUpdateFine {
     if ( C4::Context->preference('finesMode') eq 'production' ) {
         if ( $amount > 0 ) {
             C4::Overdues::UpdateFine({
-                issue_id       => $issue->{issue_id},
-                itemnumber     => $issue->{itemnumber},
-                borrowernumber => $issue->{borrowernumber},
+                issue_id       => $issue->issue_id,
+                itemnumber     => $issue->itemnumber,
+                borrowernumber => $issue->borrowernumber,
                 amount         => $amount,
                 type           => $type,
                 due            => output_pref($datedue),
@@ -4057,9 +4056,9 @@ sub _CalculateAndUpdateFine {
             # so in this case, we need to drop those fines to 0
 
             C4::Overdues::UpdateFine({
-                issue_id       => $issue->{issue_id},
-                itemnumber     => $issue->{itemnumber},
-                borrowernumber => $issue->{borrowernumber},
+                issue_id       => $issue->issue_id,
+                itemnumber     => $issue->itemnumber,
+                borrowernumber => $issue->borrowernumber,
                 amount         => 0,
                 type           => $type,
                 due            => output_pref($datedue),
diff --git a/circ/returns.pl b/circ/returns.pl
index a6041d1..991bf68 100755
--- a/circ/returns.pl
+++ b/circ/returns.pl
@@ -188,7 +188,7 @@ if ( $query->param('reserve_id') ) {
 my $borrower;
 my $returned = 0;
 my $messages;
-my $issueinformation;
+my $issue;
 my $itemnumber;
 my $barcode     = $query->param('barcode');
 my $exemptfine  = $query->param('exemptfine');
@@ -310,21 +310,22 @@ if ($barcode) {
     );
 
     # do the return
-    ( $returned, $messages, $issueinformation, $borrower ) =
+    ( $returned, $messages, $issue, $borrower ) =
       AddReturn( $barcode, $userenv_branch, $exemptfine, $dropboxmode, $return_date_override, $dropboxdate );
 
     if ($returned) {
         my $time_now = DateTime->now( time_zone => C4::Context->tz )->truncate( to => 'minute');
-        my $duedate = $issueinformation->{date_due}->strftime('%Y-%m-%d %H:%M');
+        my $date_due_dt = dt_from_string( $issue->date_due, 'sql' );
+        my $duedate = $date_due_dt->strftime('%Y-%m-%d %H:%M');
         $returneditems{0}      = $barcode;
         $riborrowernumber{0}   = $borrower->{'borrowernumber'};
         $riduedate{0}          = $duedate;
         $input{borrowernumber} = $borrower->{'borrowernumber'};
         $input{duedate}        = $duedate;
         unless ( $dropboxmode ) {
-            $input{return_overdue} = 1 if (DateTime->compare($issueinformation->{date_due}, DateTime->now()) == -1);
+            $input{return_overdue} = 1 if (DateTime->compare($date_due_dt, DateTime->now()) == -1);
         } else {
-            $input{return_overdue} = 1 if (DateTime->compare($issueinformation->{date_due}, $dropboxdate) == -1);
+            $input{return_overdue} = 1 if (DateTime->compare($date_due_dt, $dropboxdate) == -1);
         }
         push( @inputloop, \%input );
 
-- 
2.7.4