Bug 28290: Don't send subfields to 'as_string' if none to send
[koha-ffzg.git] / C4 / Circulation.pm
index e809e3d..58ceeae 100644 (file)
@@ -24,7 +24,6 @@ use POSIX qw( floor );
 use YAML::XS;
 use Encode;
 
-use Koha::DateUtils qw( dt_from_string output_pref );
 use C4::Context;
 use C4::Stats qw( UpdateStats );
 use C4::Reserves qw( CheckReserves CanItemBeReserved MoveReserve ModReserve ModReserveMinusPriority RevertWaitingStatus IsItemOnHoldAndFound IsAvailableForItemLevelRequest ItemsAnyAvailableAndNotRestricted );
@@ -41,8 +40,9 @@ use Algorithm::CheckDigits qw( CheckDigits );
 use Data::Dumper qw( Dumper );
 use Koha::Account;
 use Koha::AuthorisedValues;
+use Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue;
 use Koha::Biblioitems;
-use Koha::DateUtils qw( dt_from_string output_pref );
+use Koha::DateUtils qw( dt_from_string );
 use Koha::Calendar;
 use Koha::Checkouts;
 use Koha::Illrequests;
@@ -95,7 +95,6 @@ BEGIN {
       GetBranchBorrowerCircRule
       GetBranchItemRule
       GetBiblioIssues
-      GetOpenIssue
       GetUpcomingDueIssues
       CheckIfIssuedToPatron
       IsItemIssued
@@ -107,7 +106,6 @@ BEGIN {
 
       transferbook
       TooMany
-      GetTransfers
       GetTransfersFromTo
       updateWrongTransfer
       CalcDateDue
@@ -123,6 +121,7 @@ BEGIN {
       DeleteOfflineOperation
       ProcessOfflineOperation
       ProcessOfflinePayment
+      ProcessOfflineIssue
     );
     push @EXPORT_OK, '_GetCircControlBranch';    # This is wrong!
 }
@@ -153,6 +152,7 @@ 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 
 correct barcode string (items.barcode) should return an unaltered barcode.
+Barcode is going to be automatically trimmed of leading/trailing whitespaces.
 
 The optional $filter argument is to allow for testing or explicit 
 behavior that ignores the System Pref.  Valid values are the same as the 
@@ -165,7 +165,11 @@ System Pref options.
 #
 sub barcodedecode {
     my ($barcode, $filter) = @_;
+
+    return unless defined $barcode;
+
     my $branch = C4::Context::mybranch();
+    $barcode =~ s/^\s+|\s+$//g;
     $filter = C4::Context->preference('itemBarcodeInputFilter') unless $filter;
     Koha::Plugins->call('item_barcode_transform',  \$barcode );
     $filter or return $barcode;     # ensure filter is defined, else return untouched barcode
@@ -383,10 +387,10 @@ sub transferbook {
 
     # find recall
     if ( C4::Context->preference('UseRecalls') ) {
-        my $recall = Koha::Recalls->find({ itemnumber => $itemnumber, status => 'in_transit' });
+        my $recall = Koha::Recalls->find({ item_id => $itemnumber, status => 'in_transit' });
         if ( defined $recall ) {
             # do a transfer if the recall branch is different to the item holding branch
-            if ( $recall->branchcode eq $fbr ) {
+            if ( $recall->pickup_library_id eq $fbr ) {
                 $dotransfer = 0;
                 $messages->{'RecallPlacedAtHoldingBranch'} = 1;
             } else {
@@ -788,20 +792,11 @@ sub CanBookBeIssued {
     my $patron_unblessed = $patron->unblessed;
 
     my $circ_library = Koha::Libraries->find( _GetCircControlBranch($item_unblessed, $patron_unblessed) );
-    #
-    # DUE DATE is OK ? -- should already have checked.
-    #
-    if ($duedate && ref $duedate ne 'DateTime') {
-        $duedate = dt_from_string($duedate);
-    }
-    my $now = dt_from_string();
-    unless ( $duedate ) {
-        my $issuedate = $now->clone();
-
-        $duedate = CalcDateDue( $issuedate, $effective_itemtype, $circ_library->branchcode, $patron_unblessed );
 
-        # Offline circ calls AddIssue directly, doesn't run through here
-        #  So issuingimpossible should be ok.
+    my $now = dt_from_string();
+    $duedate ||= CalcDateDue( $now, $effective_itemtype, $circ_library->branchcode, $patron_unblessed );
+    if (DateTime->compare($duedate,$now) == -1 ) { # duedate cannot be before now
+         $needsconfirmation{INVALID_DATE} = $duedate;
     }
 
     my $fees = Koha::Charges::Fees->new(
@@ -813,16 +808,6 @@ sub CanBookBeIssued {
         }
     );
 
-    if ($duedate) {
-        my $today = $now->clone();
-        $today->truncate( to => 'minute');
-        if (DateTime->compare($duedate,$today) == -1 ) { # duedate cannot be before now
-            $needsconfirmation{INVALID_DATE} = output_pref($duedate);
-        }
-    } else {
-            $issuingimpossible{INVALID_DATE} = output_pref($duedate);
-    }
-
     #
     # BORROWER STATUS
     #
@@ -1136,35 +1121,35 @@ sub CanBookBeIssued {
     # Only bother doing this if UseRecalls is enabled and the item is recallable
     # Don't look at recalls that are in transit
     if ( C4::Context->preference('UseRecalls') and $item_object->can_be_waiting_recall ) {
-        my @recalls = $biblio->recalls({},{ order_by => { -asc => 'recalldate' } })->filter_by_current->as_list;
+        my @recalls = $biblio->recalls({},{ order_by => { -asc => 'created_date' } })->filter_by_current->as_list;
 
         foreach my $r ( @recalls ) {
-            if ( $r->itemnumber and
-                $r->itemnumber == $item_object->itemnumber and
-                $r->borrowernumber == $patron->borrowernumber and
+            if ( $r->item_id and
+                $r->item_id == $item_object->itemnumber and
+                $r->patron_id == $patron->borrowernumber and
                 ( $r->waiting or $r->requested ) ) {
-                $messages{RECALLED} = $r->recall_id;
+                $messages{RECALLED} = $r->id;
                 $recall = $r;
                 # this item is recalled by or already waiting for this borrower and the recall can be fulfilled
                 last;
             }
-            elsif ( $r->itemnumber and
-                $r->itemnumber == $item_object->itemnumber and
+            elsif ( $r->item_id and
+                $r->item_id == $item_object->itemnumber and
                 $r->in_transit ) {
                 # recalled item is in transit
-                $issuingimpossible{RECALLED_INTRANSIT} = $r->branchcode;
+                $issuingimpossible{RECALLED_INTRANSIT} = $r->pickup_library_id;
             }
-            elsif ( $r->item_level_recall and
-                $r->itemnumber == $item_object->itemnumber and
-                $r->borrowernumber != $patron->borrowernumber and
+            elsif ( $r->item_level and
+                $r->item_id == $item_object->itemnumber and
+                $r->patron_id != $patron->borrowernumber and
                 !$r->in_transit ) {
                 # this specific item has been recalled by a different patron
                 $needsconfirmation{RECALLED} = $r;
                 $recall = $r;
                 last;
             }
-            elsif ( !$r->item_level_recall and
-                $r->borrowernumber != $patron->borrowernumber and
+            elsif ( !$r->item_level and
+                $r->patron_id != $patron->borrowernumber and
                 !$r->in_transit ) {
                 # a different patron has placed a biblio-level recall and this item is eligible to fill it
                 $needsconfirmation{RECALLED} = $r;
@@ -1248,19 +1233,16 @@ sub CanBookBeIssued {
         my $check = checkHighHolds( $item_object, $patron );
 
         if ( $check->{exceeded} ) {
+            my $highholds = {
+                num_holds  => $check->{outstanding},
+                duration   => $check->{duration},
+                returndate => $check->{due_date},
+            };
             if ($override_high_holds) {
-                $alerts{HIGHHOLDS} = {
-                    num_holds  => $check->{outstanding},
-                    duration   => $check->{duration},
-                    returndate => output_pref( { dt => dt_from_string($check->{due_date}), dateformat => 'iso', timeformat => '24hr' }),
-                };
+                $alerts{HIGHHOLDS} = $highholds;
             }
             else {
-                $needsconfirmation{HIGHHOLDS} = {
-                    num_holds  => $check->{outstanding},
-                    duration   => $check->{duration},
-                    returndate => output_pref( { dt => dt_from_string($check->{due_date}), dateformat => 'iso', timeformat => '24hr' }),
-                };
+                $needsconfirmation{HIGHHOLDS} = $highholds;
             }
         }
     }
@@ -1427,7 +1409,7 @@ sub checkHighHolds {
 
         my $orig_due = C4::Circulation::CalcDateDue( $issuedate, $itype, $branchcode, $patron->unblessed );
 
-        my $rule = Koha::CirculationRules->get_effective_rule(
+        my $rule = Koha::CirculationRules->get_effective_rule_value(
             {
                 categorycode => $patron->categorycode,
                 itemtype     => $item->effective_itemtype,
@@ -1437,9 +1419,9 @@ sub checkHighHolds {
         );
 
         my $duration;
-        if ( defined($rule) && $rule->rule_value ne '' ){
+        if ( defined($rule) && $rule ne '' ){
             # overrides decreaseLoanHighHoldsDuration syspref
-            $duration = $rule->rule_value;
+            $duration = $rule;
         } else {
             $duration = C4::Context->preference('decreaseLoanHighHoldsDuration');
         }
@@ -1475,8 +1457,8 @@ Calculated if empty.
 
 =item C<$cancelreserve> is 1 to override and cancel any pending reserves for the item (optional).
 
-=item C<$issuedate> is the date to issue the item in iso (YYYY-MM-DD) format (optional).
-Defaults to today.  Unlike C<$datedue>, NOT a DateTime object, unfortunately.
+=item C<$issuedate> is a DateTime object for the date to issue the item (optional).
+Defaults to today.
 
 AddIssue does the following things :
 
@@ -1557,6 +1539,25 @@ sub AddIssue {
                 $datedue = CalcDateDue( $issuedate, $itype, $branchcode, $borrower );
 
             }
+
+            # Check if we need to use an exact due date set by the ILL module
+            if ( C4::Context->preference('ILLModule') ) {
+                # Check if there is an ILL connected with the biblio of the item we are issuing
+                my $ill_request = Koha::Illrequests->search({
+                    biblio_id => $item_object->biblionumber,
+                    borrowernumber => $borrower->{'borrowernumber'},
+                    completed => undef,
+                    due_date => { '!=', undef },
+                })->next;
+
+                if ( $ill_request and length( $ill_request->due_date ) > 0 ) {
+                    my $ill_dt = dt_from_string( $ill_request->due_date );
+                    $ill_dt->set_hour(23);
+                    $ill_dt->set_minute(59);
+                    $datedue = $ill_dt;
+                }
+            }
+
             $datedue->truncate( to => 'minute' );
 
             my $patron = Koha::Patrons->find( $borrower );
@@ -1615,7 +1616,7 @@ sub AddIssue {
 
             # If automatic renewal wasn't selected while issuing, set the value according to the issuing rule.
             unless ($auto_renew) {
-                my $rule = Koha::CirculationRules->get_effective_rule(
+                my $rule = Koha::CirculationRules->get_effective_rule_value(
                     {
                         categorycode => $borrower->{categorycode},
                         itemtype     => $item_object->effective_itemtype,
@@ -1624,13 +1625,13 @@ sub AddIssue {
                     }
                 );
 
-                $auto_renew = $rule->rule_value if $rule;
+                $auto_renew = $rule if defined $rule && $rule ne '';
             }
 
             my $issue_attributes = {
                 borrowernumber  => $borrower->{'borrowernumber'},
-                issuedate       => $issuedate->strftime('%Y-%m-%d %H:%M:%S'),
-                date_due        => $datedue->strftime('%Y-%m-%d %H:%M:%S'),
+                issuedate       => $issuedate,
+                date_due        => $datedue,
                 branchcode      => C4::Context->userenv->{'branch'},
                 onsite_checkout => $onsite_checkout,
                 auto_renew      => $auto_renew ? 1 : 0,
@@ -1670,7 +1671,7 @@ sub AddIssue {
             }
 
             if ( C4::Context->preference('UpdateTotalIssuesOnCirc') ) {
-                UpdateTotalIssues( $item_object->biblionumber, 1 );
+                UpdateTotalIssues( $item_object->biblionumber, 1, undef, { skip_holds_queue => 1 } );
             }
 
             # Record if item was lost
@@ -1682,7 +1683,7 @@ sub AddIssue {
             $item_object->onloan($datedue->ymd());
             $item_object->datelastborrowed( dt_from_string()->ymd() );
             $item_object->datelastseen( dt_from_string()->ymd() );
-            $item_object->store({log_action => 0});
+            $item_object->store( { log_action => 0, skip_holds_queue => 1 } );
 
             # If the item was lost, it has now been found, charge the overdue if necessary
             if ($was_lost) {
@@ -1773,6 +1774,12 @@ sub AddIssue {
                     checkout => $issue->get_from_storage
                 }
             });
+
+            Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue(
+                {
+                    biblio_ids => [ $item_object->biblionumber ]
+                }
+            ) if C4::Context->preference('RealTimeHoldsQueue');
         }
     }
     return $issue;
@@ -2070,7 +2077,7 @@ sub AddReturn {
                 . Dumper($issue->unblessed) . "\n";
     } else {
         $messages->{'NotIssued'} = $barcode;
-        $item->onloan(undef)->store({skip_record_index=>1}) if defined $item->onloan;
+        $item->onloan(undef)->store( { skip_record_index => 1, skip_holds_queue => 1 } ) if defined $item->onloan;
 
         # even though item is not on loan, it may still be transferred;  therefore, get current branch info
         $doreturn = 0;
@@ -2103,7 +2110,7 @@ sub AddReturn {
                 (!defined $item->location && $update_loc_rules->{_ALL_} ne "")
                ) {
                 $messages->{'ItemLocationUpdated'} = { from => $item->location, to => $update_loc_rules->{_ALL_} };
-                $item->location($update_loc_rules->{_ALL_})->store({skip_record_index=>1});
+                $item->location($update_loc_rules->{_ALL_})->store({ log_action => 0, skip_record_index => 1, skip_holds_queue => 1});
             }
         }
         else {
@@ -2112,7 +2119,7 @@ sub AddReturn {
                 if ( $update_loc_rules->{$key} eq '_BLANK_') { $update_loc_rules->{$key} = '' ;}
                 if ( ($item->location eq $key && $item->location ne $update_loc_rules->{$key}) || ($key eq '_BLANK_' && $item->location eq '' && $update_loc_rules->{$key} ne '') ) {
                     $messages->{'ItemLocationUpdated'} = { from => $item->location, to => $update_loc_rules->{$key} };
-                    $item->location($update_loc_rules->{$key})->store({skip_record_index=>1});
+                    $item->location($update_loc_rules->{$key})->store({ log_action => 0, skip_record_index => 1, skip_holds_queue => 1});
                     last;
                 }
             }
@@ -2131,7 +2138,7 @@ sub AddReturn {
             foreach my $key ( keys %$rules ) {
                 if ( $item->notforloan eq $key ) {
                     $messages->{'NotForLoanStatusUpdated'} = { from => $item->notforloan, to => $rules->{$key} };
-                    $item->notforloan($rules->{$key})->store({ log_action => 0, skip_record_index => 1 });
+                    $item->notforloan($rules->{$key})->store({ log_action => 0, skip_record_index => 1, skip_holds_queue => 1 });
                     last;
                 }
             }
@@ -2167,7 +2174,7 @@ sub AddReturn {
 
         if ($patron) {
             eval {
-                MarkIssueReturned( $borrowernumber, $item->itemnumber, $return_date, $patron->privacy, { skip_record_index => 1} );
+                MarkIssueReturned( $borrowernumber, $item->itemnumber, $return_date, $patron->privacy, { skip_record_index => 1, skip_holds_queue => 1} );
             };
             unless ( $@ ) {
                 if (
@@ -2193,19 +2200,19 @@ sub AddReturn {
             $messages->{'WasReturned'} = 1;
 
         } else {
-            $item->onloan(undef)->store({ log_action => 0 , skip_record_index => 1 });
+            $item->onloan(undef)->store({ log_action => 0 , skip_record_index => 1, skip_holds_queue => 1 });
         }
     }
 
     # 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) {
-        $item->holdingbranch($branch)->store({ skip_record_index => 1 });
+        $item->holdingbranch($branch)->store({ log_action => 0, skip_record_index => 1, skip_holds_queue => 1 });
     }
 
     my $item_was_lost = $item->itemlost;
     my $leave_item_lost = C4::Context->preference("BlockReturnOfLostItems") ? 1 : 0;
-    my $updated_item = ModDateLastSeen( $item->itemnumber, $leave_item_lost, { skip_record_index => 1 } ); # will unset itemlost if needed
+    my $updated_item = ModDateLastSeen( $item->itemnumber, $leave_item_lost, { skip_record_index => 1, skip_holds_queue => 1 } ); # will unset itemlost if needed
 
     # fix up the accounts.....
     if ($item_was_lost) {
@@ -2311,7 +2318,7 @@ sub AddReturn {
         $recall = $item->check_recalls if $item->can_be_waiting_recall;
         if ( defined $recall ) {
             $messages->{RecallFound} = $recall;
-            if ( $recall->branchcode ne $branch ) {
+            if ( $recall->pickup_library_id ne $branch ) {
                 $messages->{RecallNeedsTransfer} = $branch;
             }
         }
@@ -2378,8 +2385,8 @@ sub AddReturn {
 
     if ( C4::Context->preference('UseRecalls') ) {
         # all recalls that have triggered a transfer will have an allocated itemnumber
-        my $transfer_recall = Koha::Recalls->find({ itemnumber => $item->itemnumber, status => 'in_transit' });
-        if ( $transfer_recall and $transfer_recall->branchcode eq $branch ) {
+        my $transfer_recall = Koha::Recalls->find({ item_id => $item->itemnumber, status => 'in_transit' });
+        if ( $transfer_recall and $transfer_recall->pickup_library_id eq $branch ) {
             $messages->{TransferredRecall} = $transfer_recall;
         }
     }
@@ -2421,6 +2428,12 @@ sub AddReturn {
         }
     }
 
+    # Check for bundle status
+    if ( $item->in_bundle ) {
+        my $host = $item->bundle_host;
+        $messages->{InBundle} = $host;
+    }
+
     my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX });
     $indexer->index_records( $item->biblionumber, "specialUpdate", "biblioserver" );
 
@@ -2433,6 +2446,12 @@ sub AddReturn {
                 checkout=> $checkin
             }
         });
+
+        Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue(
+            {
+                biblio_ids => [ $item->biblionumber ]
+            }
+        ) if C4::Context->preference('RealTimeHoldsQueue');
     }
 
     return ( $doreturn, $messages, $issue, ( $patron ? $patron->unblessed : {} ));
@@ -2496,7 +2515,12 @@ sub MarkIssueReturned {
         # And finally delete the issue
         $issue->delete;
 
-        $issue->item->onloan(undef)->store({ log_action => 0, skip_record_index => $params->{skip_record_index} });
+        $issue->item->onloan(undef)->store(
+            {   log_action        => 0,
+                skip_record_index => $params->{skip_record_index},
+                skip_holds_queue  => $params->{skip_holds_queue}
+            }
+        );
 
         if ( C4::Context->preference('StoreLastBorrower') ) {
             my $item = Koha::Items->find( $itemnumber );
@@ -2765,28 +2789,6 @@ sub _GetCircControlBranch {
     return $branch;
 }
 
-=head2 GetOpenIssue
-
-  $issue = GetOpenIssue( $itemnumber );
-
-Returns the row from the issues table if the item is currently issued, undef if the item is not currently issued
-
-C<$itemnumber> is the item's itemnumber
-
-Returns a hashref
-
-=cut
-
-sub GetOpenIssue {
-  my ( $itemnumber ) = @_;
-  return unless $itemnumber;
-  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();
-
-}
-
 =head2 GetUpcomingDueIssues
 
   my $upcoming_dues = GetUpcomingDueIssues( { days_in_advance => 4 } );
@@ -2873,11 +2875,11 @@ sub CanBookBeRenewed {
         );
 
         return ( 0, "too_many" )
-          if not $issuing_rule->{renewalsallowed} or $issuing_rule->{renewalsallowed} <= $issue->renewals;
+          if not $issuing_rule->{renewalsallowed} or $issuing_rule->{renewalsallowed} <= $issue->renewals_count;
 
         return ( 0, "too_unseen" )
           if C4::Context->preference('UnseenRenewals') &&
-            $issuing_rule->{unseen_renewals_allowed} &&
+            looks_like_number($issuing_rule->{unseen_renewals_allowed}) &&
             $issuing_rule->{unseen_renewals_allowed} <= $issue->unseen_renewals;
 
         my $overduesblockrenewing = C4::Context->preference('OverduesBlockRenewing');
@@ -2910,9 +2912,9 @@ sub CanBookBeRenewed {
         my $recall = undef;
         $recall = $item->check_recalls if $item->can_be_waiting_recall;
         if ( defined $recall ) {
-            if ( $recall->item_level_recall ) {
+            if ( $recall->item_level ) {
                 # item-level recall. check if this item is the recalled item, otherwise renewal will be allowed
-                return ( 0, 'recalled' ) if ( $recall->itemnumber == $item->itemnumber );
+                return ( 0, 'recalled' ) if ( $recall->item_id == $item->itemnumber );
             } else {
                 # biblio-level recall, so only disallow renewal if the biblio-level recall has been fulfilled by a different item
                 return ( 0, 'recalled' ) unless ( $recall->waiting );
@@ -2920,87 +2922,59 @@ sub CanBookBeRenewed {
         }
     }
 
-    # Note: possible_reserves will contain all title level holds on this bib and item level
-    # holds on the checked out item
-    my ( $resfound, $resrec, $possible_reserves ) = C4::Reserves::CheckReserves($itemnumber);
-
-    # If next hold is non priority, then check if any hold with priority (non_priority = 0) exists for the same biblionumber.
-    if ( $resfound && $resrec->{non_priority} ) {
-        $resfound = Koha::Holds->search(
-            { biblionumber => $resrec->{biblionumber}, non_priority => 0 } )
-          ->count > 0;
-    }
-
-
-
-    # This item can fill one or more unfilled reserve, can those unfilled reserves
-    # all be filled by other available items?
-    if ( $resfound
-        && C4::Context->preference('AllowRenewalIfOtherItemsAvailable') )
-    {
-        my $item_holds = Koha::Holds->search( { itemnumber => $itemnumber, found => undef } )->count();
-        if ($item_holds) {
-            # There is an item level hold on this item, no other item can fill the hold
-            $resfound = 1;
-        }
-        else {
+    my $fillable_holds = Koha::Holds->search(
+        {
+            biblionumber => $item->biblionumber,
+            non_priority => 0,
+            found        => undef,
+            reservedate  => { '<=' => \'NOW()' },
+            suspend      => 0,
+        },
+        { prefetch => 'patron' }
+    );
+    if ( $fillable_holds->count ) {
+        if ( C4::Context->preference('AllowRenewalIfOtherItemsAvailable') ) {
+            my @possible_holds = $fillable_holds->as_list;
 
             # Get all other items that could possibly fill reserves
             # FIXME We could join reserves (or more tables) here to eliminate some checks later
-            my $items = Koha::Items->search({
-                biblionumber => $resrec->{biblionumber},
+            my @other_items = Koha::Items->search({
+                biblionumber => $item->biblionumber,
                 onloan       => undef,
                 notforloan   => 0,
-                -not         => { itemnumber => $itemnumber }
-            });
-            my $item_count = $items->count();
-
-            # Get all other reserves that could have been filled by this item
-            my @borrowernumbers = map { $_->{borrowernumber} } @$possible_reserves;
-            # Note: fetching the patrons in this manner means that a patron with 2 holds will
-            # not block renewal if one reserve can be satisfied i.e. each patron is checked once
-            my $patrons = Koha::Patrons->search({
-                borrowernumber => { -in => \@borrowernumbers }
-            });
-            my $patron_count = $patrons->count();
+                -not         => { itemnumber => $itemnumber } })->as_list;
 
-            return ( 0, "on_reserve" ) if ($patron_count > $item_count);
-            # We cannot possibly fill all reserves if we don't have enough items
+            return ( 0, "on_reserve" ) if @possible_holds && (scalar @other_items < scalar @possible_holds);
 
-            # If we can fill each hold that has been found with the available items on the record
-            # then the patron can renew. If we cannot, they cannot renew.
-            # FIXME This code does not check whether the item we are renewing can fill
-            # any of the existing reserves.
-            my $reservable = 0;
             my %matched_items;
-            my $seen = 0;
-            PATRON: while ( my $patron = $patrons->next ) {
-                # If there is a reserve that cannot be filled we are done
-                return ( 0, "on_reserve" ) if ( $seen > $reservable );
-                my $items_any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $item->biblionumber, patron => $patron });
-                while ( my $other_item = $items->next ) {
-                    next if defined $matched_items{$other_item->itemnumber};
-                    next if IsItemOnHoldAndFound( $other_item->itemnumber );
-                    next unless IsAvailableForItemLevelRequest($other_item, $patron, undef, $items_any_available);
-                    next unless CanItemBeReserved($patron,$other_item,undef,{ignore_hold_counts=>1})->{status} eq 'OK';
-                    # NOTE: At checkin we call 'CheckReserves' which checks hold 'policy'
-                    # CanItemBeReserved checks 'rules' and 'policies' which means
-                    # items will fill holds at checkin that are rejected here
-                    $reservable++;
-                    if ($reservable >= $patron_count) {
-                        $resfound = 0;
-                        last PATRON;
-                    }
-                    $matched_items{$other_item->itemnumber} = 1;
-                    last;
+            foreach my $possible_hold (@possible_holds) {
+                my $fillable = 0;
+                my $patron_with_reserve = Koha::Patrons->find($possible_hold->borrowernumber);
+                my $items_any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $item->biblionumber, patron => $patron_with_reserve });
+
+                # FIXME: We are not checking whether the item we are renewing can fill the hold
+
+                foreach my $other_item (@other_items) {
+                  next if defined $matched_items{$other_item->itemnumber};
+                  next if IsItemOnHoldAndFound( $other_item->itemnumber );
+                  next unless IsAvailableForItemLevelRequest($other_item, $patron_with_reserve, undef, $items_any_available);
+                  next unless CanItemBeReserved($patron_with_reserve,$other_item,undef,{ignore_hold_counts=>1})->{status} eq 'OK';
+                  # NOTE: At checkin we call 'CheckReserves' which checks hold 'policy'
+                  # CanItemBeReserved checks 'rules' and 'policies' which means
+                  # items will fill holds at checkin that are rejected here
+                  $fillable = 1;
+                  $matched_items{$other_item->itemnumber} = 1;
+                  last;
                 }
-                $items->reset;
-                $seen++;
+                return ( 0, "on_reserve" ) unless $fillable;
             }
+
+        } else {
+            my ($status, $matched_reserve, $possible_reserves) = CheckReserves($itemnumber);
+            return ( 0, "on_reserve" ) if $status;
         }
     }
 
-    return ( 0, "on_reserve" ) if $resfound;    # '' when no hold was found
     return ( 0, $auto_renew, { soonest_renew_date => $soonest } ) if $auto_renew =~ 'too_soon';#$auto_renew ne "no" && $auto_renew ne "ok";
     $soonest = GetSoonestRenewDate($borrowernumber, $itemnumber);
     if ( $soonest > dt_from_string() ){
@@ -3069,8 +3043,7 @@ sub AddRenewal {
     $borrowernumber ||= $issue->borrowernumber;
 
     if ( defined $datedue && ref $datedue ne 'DateTime' ) {
-        carp 'Invalid date passed to AddRenewal.';
-        return;
+        $datedue = dt_from_string($datedue, 'sql');
     }
 
     my $patron = Koha::Patrons->find( $borrowernumber ) or return; # FIXME Should do more than just return
@@ -3120,7 +3093,7 @@ sub AddRenewal {
                     rule_name    => 'unseen_renewals_allowed'
                 }
             );
-            if (!$seen && $rule && $rule->rule_value) {
+            if (!$seen && $rule && looks_like_number($rule->rule_value)) {
                 $unseen_renewals++;
             } else {
                 # If the renewal is seen, unseen should revert to 0
@@ -3130,8 +3103,8 @@ sub AddRenewal {
 
         # Update the issues record to have the new due date, and a new count
         # of how many times it has been renewed.
-        my $renews = ( $issue->renewals || 0 ) + 1;
-        my $sth = $dbh->prepare("UPDATE issues SET date_due = ?, renewals = ?, unseen_renewals = ?, lastreneweddate = ? WHERE issue_id = ?");
+        my $renews = ( $issue->renewals_count || 0 ) + 1;
+        my $sth = $dbh->prepare("UPDATE issues SET date_due = ?, renewals_count = ?, unseen_renewals = ?, lastreneweddate = ? WHERE issue_id = ?");
 
         eval{
             $sth->execute( $datedue->strftime('%Y-%m-%d %H:%M'), $renews, $unseen_renewals, $lastreneweddate, $issue->issue_id );
@@ -3194,6 +3167,16 @@ sub AddRenewal {
             DelUniqueDebarment({ borrowernumber => $borrowernumber, type => 'OVERDUES' });
         }
 
+        # Add renewal record
+        my $renewal = Koha::Checkouts::Renewal->new(
+            {
+                checkout_id => $issue->issue_id,
+                renewer_id  => C4::Context->userenv ? C4::Context->userenv->{'number'} : undef,
+                seen        => $seen,
+                interface   => C4::Context->interface
+            }
+        )->store();
+
         # Add the renewal to stats
         C4::Stats::UpdateStats(
             {
@@ -3249,7 +3232,7 @@ sub GetRenewCount {
     );
     $sth->execute( $bornum, $itemno );
     my $data = $sth->fetchrow_hashref;
-    $renewcount = $data->{'renewals'} if $data->{'renewals'};
+    $renewcount = $data->{'renewals_count'} if $data->{'renewals_count'};
     $unseencount = $data->{'unseen_renewals'} if $data->{'unseen_renewals'};
     # $item and $borrower should be calculated
     my $branchcode = _GetCircControlBranch($item->unblessed, $patron->unblessed);
@@ -3504,35 +3487,6 @@ sub AddIssuingCharge {
     );
 }
 
-=head2 GetTransfers
-
-  GetTransfers($itemnumber);
-
-=cut
-
-sub GetTransfers {
-    my ($itemnumber) = @_;
-
-    my $dbh = C4::Context->dbh;
-
-    my $query = '
-        SELECT datesent,
-               frombranch,
-               tobranch,
-               branchtransfer_id,
-               daterequested,
-               reason
-        FROM branchtransfers
-        WHERE itemnumber = ?
-          AND datearrived IS NULL
-          AND datecancelled IS NULL
-        ';
-    my $sth = $dbh->prepare($query);
-    $sth->execute($itemnumber);
-    my @row = $sth->fetchrow_array();
-    return @row;
-}
-
 =head2 GetTransfersFromTo
 
   @results = GetTransfersFromTo($frombranch,$tobranch);
@@ -3720,7 +3674,7 @@ sub CalcDateDue {
     my $loanlength =
             GetLoanLength( $borrower->{'categorycode'}, $itemtype, $branch );
 
-    my $length_key = ( $isrenewal and defined $loanlength->{renewalperiod} )
+    my $length_key = ( $isrenewal and defined $loanlength->{renewalperiod} and $loanlength->{renewalperiod} ne q{} )
             ? qq{renewalperiod}
             : qq{issuelength};
 
@@ -4034,16 +3988,16 @@ sub ProcessOfflineReturn {
 
     if ( $item ) {
         my $itemnumber = $item->itemnumber;
-        my $issue = GetOpenIssue( $itemnumber );
+        my $issue = $item->checkout;
         if ( $issue ) {
             my $leave_item_lost = C4::Context->preference("BlockReturnOfLostItems") ? 1 : 0;
             ModDateLastSeen( $itemnumber, $leave_item_lost );
             MarkIssueReturned(
-                $issue->{borrowernumber},
+                $issue->borrowernumber,
                 $itemnumber,
                 $operation->{timestamp},
             );
-            $item->renewals(0);
+            $item->renewals_count(0);
             $item->onloan(undef);
             $item->store({ log_action => 0 });
             return "Success.";
@@ -4066,11 +4020,11 @@ sub ProcessOfflineIssue {
             return "Barcode not found.";
         }
         my $itemnumber = $item->itemnumber;
-        my $issue = GetOpenIssue( $itemnumber );
+        my $issue = $item->checkout;
 
-        if ( $issue and ( $issue->{borrowernumber} ne $patron->borrowernumber ) ) { # Item already issued to another patron mark it returned
+        if ( $issue and ( $issue->borrowernumber ne $patron->borrowernumber ) ) { # Item already issued to another patron mark it returned
             MarkIssueReturned(
-                $issue->{borrowernumber},
+                $issue->borrowernumber,
                 $itemnumber,
                 $operation->{timestamp},
             );
@@ -4384,7 +4338,7 @@ sub _CalculateAndUpdateFine {
                 itemnumber     => $issue->itemnumber,
                 borrowernumber => $issue->borrowernumber,
                 amount         => $amount,
-                due            => output_pref($datedue),
+                due            => $datedue,
             });
         }
         elsif ($return_date) {
@@ -4397,7 +4351,7 @@ sub _CalculateAndUpdateFine {
                 itemnumber     => $issue->itemnumber,
                 borrowernumber => $issue->borrowernumber,
                 amount         => 0,
-                due            => output_pref($datedue),
+                due            => $datedue,
             });
         }
     }