Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a...
[srvgit] / C4 / Reserves.pm
index 357c65d..a03072d 100644 (file)
@@ -24,34 +24,30 @@ package C4::Reserves;
 use Modern::Perl;
 
 use C4::Accounts;
-use C4::Biblio;
-use C4::Circulation;
+use C4::Biblio qw( GetMarcFromKohaField );
+use C4::Circulation qw( CheckIfIssuedToPatron GetAgeRestriction GetBranchItemRule );
 use C4::Context;
-use C4::Items;
+use C4::Items qw( CartToShelf get_hostitemnumbers_of );
 use C4::Letters;
-use C4::Log;
+use C4::Log qw( logaction );
 use C4::Members::Messaging;
 use C4::Members;
 use Koha::Account::Lines;
+use Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue;
 use Koha::Biblios;
 use Koha::Calendar;
 use Koha::CirculationRules;
 use Koha::Database;
-use Koha::DateUtils;
-use Koha::Hold;
+use Koha::DateUtils qw( dt_from_string output_pref );
 use Koha::Holds;
 use Koha::ItemTypes;
 use Koha::Items;
 use Koha::Libraries;
-use Koha::Old::Hold;
+use Koha::Old::Holds;
 use Koha::Patrons;
 use Koha::Plugins;
 
-use Carp;
-use Data::Dumper;
-use List::MoreUtils qw( firstidx any );
-
-use vars qw(@ISA @EXPORT @EXPORT_OK %EXPORT_TAGS);
+use List::MoreUtils qw( any );
 
 =head1 NAME
 
@@ -99,49 +95,56 @@ This modules provides somes functions to deal with reservations.
 
 =cut
 
+our (@ISA, @EXPORT_OK);
 BEGIN {
     require Exporter;
     @ISA = qw(Exporter);
-    @EXPORT = qw(
-        &AddReserve
+    @EXPORT_OK = qw(
+      AddReserve
+
+      GetReserveStatus
+
+      GetOtherReserves
+      ChargeReserveFee
+      GetReserveFee
 
-        &GetReserveStatus
+      ModReserveAffect
+      ModReserve
+      ModReserveStatus
+      ModReserveCancelAll
+      ModReserveMinusPriority
+      MoveReserve
 
-        &GetOtherReserves
+      CheckReserves
+      CanBookBeReserved
+      CanItemBeReserved
+      CanReserveBeCanceledFromOpac
+      CancelExpiredReserves
 
-        &ModReserveFill
-        &ModReserveAffect
-        &ModReserve
-        &ModReserveStatus
-        &ModReserveCancelAll
-        &ModReserveMinusPriority
-        &MoveReserve
+      AutoUnsuspendReserves
 
-        &CheckReserves
-        &CanBookBeReserved
-        &CanItemBeReserved
-        &CanReserveBeCanceledFromOpac
-        &CancelExpiredReserves
+      IsAvailableForItemLevelRequest
+      ItemsAnyAvailableAndNotRestricted
 
-        &AutoUnsuspendReserves
+      AlterPriority
+      ToggleLowestPriority
 
-        &IsAvailableForItemLevelRequest
-        ItemsAnyAvailableAndNotRestricted
+      ReserveSlip
+      ToggleSuspend
+      SuspendAll
 
-        &AlterPriority
-        &ToggleLowestPriority
+      GetReservesControlBranch
 
-        &ReserveSlip
-        &ToggleSuspend
-        &SuspendAll
+      CalculatePriority
 
-        &GetReservesControlBranch
+      IsItemOnHoldAndFound
 
-        IsItemOnHoldAndFound
+      GetMaxPatronHoldsForRecord
 
-        GetMaxPatronHoldsForRecord
+      MergeHolds
+
+      RevertWaitingStatus
     );
-    @EXPORT_OK = qw( MergeHolds );
 }
 
 =head2 AddReserve
@@ -182,7 +185,7 @@ sub AddReserve {
     my $biblionumber   = $params->{biblionumber};
     my $priority       = $params->{priority};
     my $resdate        = $params->{reservation_date};
-    my $expdate        = $params->{expiration_date};
+    my $patron_expiration_date = $params->{expiration_date};
     my $notes          = $params->{notes};
     my $title          = $params->{title};
     my $checkitem      = $params->{itemnumber};
@@ -190,10 +193,7 @@ sub AddReserve {
     my $itemtype       = $params->{itemtype};
     my $non_priority   = $params->{non_priority};
 
-    $resdate = output_pref( { str => dt_from_string( $resdate ), dateonly => 1, dateformat => 'iso' })
-        or output_pref({ dt => dt_from_string, dateonly => 1, dateformat => 'iso' });
-
-    $expdate = output_pref({ str => $expdate, dateonly => 1, dateformat => 'iso' });
+    $resdate ||= dt_from_string;
 
     # if we have an item selectionned, and the pickup branch is the same as the holdingbranch
     # of the document, we force the value $priority and $found .
@@ -220,11 +220,9 @@ sub AddReserve {
             $found = 'W';
         }
     }
-
-    if ( C4::Context->preference('AllowHoldDateInFuture') ) {
-
-        # Make room in reserves for this before those of a later reserve date
-        $priority = _ShiftPriorityByDateAndPriority( $biblionumber, $resdate, $priority );
+    if ( C4::Context->preference( 'AllowHoldDateInFuture' ) ) {
+    # Make room in reserves for this if passed a priority
+    $priority = _ShiftPriority( $biblionumber, $priority );
     }
 
     my $waitingdate;
@@ -249,7 +247,7 @@ sub AddReserve {
             itemnumber     => $checkitem,
             found          => $found,
             waitingdate    => $waitingdate,
-            expirationdate => $expdate,
+            patron_expiration_date => $patron_expiration_date,
             itemtype       => $itemtype,
             item_level_hold => $checkitem ? 1 : 0,
             non_priority   => $non_priority ? 1 : 0,
@@ -257,7 +255,7 @@ sub AddReserve {
     )->store();
     $hold->set_waiting() if $found && $found eq 'W';
 
-    logaction( 'HOLDS', 'CREATE', $hold->id, Dumper($hold->unblessed) )
+    logaction( 'HOLDS', 'CREATE', $hold->id, $hold )
         if C4::Context->preference('HoldsLog');
 
     my $reserve_id = $hold->id();
@@ -303,6 +301,19 @@ sub AddReserve {
     }
 
     Koha::Plugins->call('after_hold_create', $hold);
+    Koha::Plugins->call(
+        'after_hold_action',
+        {
+            action  => 'place',
+            payload => { hold => $hold->get_from_storage }
+        }
+    );
+
+    Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue(
+        {
+            biblio_ids => [ $biblionumber ]
+        }
+    ) if C4::Context->preference('RealTimeHoldsQueue');
 
     return $reserve_id;
 }
@@ -327,16 +338,55 @@ sub CanBookBeReserved{
         return { status =>'alreadypossession' };
     }
 
-    my @itemnumbers = Koha::Items->search({ biblionumber => $biblionumber})->get_column("itemnumber");
+    if ( $params->{itemtype} ) {
+
+        # biblio-level, item type-contrained
+        my $patron          = Koha::Patrons->find($borrowernumber);
+        my $reservesallowed = Koha::CirculationRules->get_effective_rule(
+            {
+                itemtype     => $params->{itemtype},
+                categorycode => $patron->categorycode,
+                branchcode   => $pickup_branchcode,
+                rule_name    => 'reservesallowed',
+            }
+        )->rule_value;
+
+        $reservesallowed = ( $reservesallowed eq '' ) ? undef : $reservesallowed;
+
+        my $count = $patron->holds->search(
+            {
+                '-or' => [
+                    { 'me.itemtype' => $params->{itemtype} },
+                    { 'item.itype'  => $params->{itemtype} }
+                ]
+            },
+            {
+                join => ['item']
+            }
+        )->count;
+
+        return { status => '' }
+          if defined $reservesallowed and $reservesallowed < $count + 1;
+    }
+
+    my $items;
     #get items linked via host records
-    my @hostitems = get_hostitemnumbers_of($biblionumber);
-    if (@hostitems){
-        push (@itemnumbers, @hostitems);
+    my @hostitemnumbers = get_hostitemnumbers_of($biblionumber);
+    if (@hostitemnumbers){
+        $items = Koha::Items->search({
+            -or => [
+                biblionumber => $biblionumber,
+                itemnumber => { -in => @hostitemnumbers }
+            ]
+        });
+    } else {
+        $items = Koha::Items->search({ biblionumber => $biblionumber});
     }
 
     my $canReserve = { status => '' };
-    foreach my $itemnumber (@itemnumbers) {
-        $canReserve = CanItemBeReserved( $borrowernumber, $itemnumber, $pickup_branchcode, $params );
+    my $patron = Koha::Patrons->find( $borrowernumber );
+    while ( my $item = $items->next ) {
+        $canReserve = CanItemBeReserved( $patron, $item, $pickup_branchcode, $params );
         return { status => 'OK' } if $canReserve->{status} eq 'OK';
     }
     return $canReserve;
@@ -344,12 +394,15 @@ sub CanBookBeReserved{
 
 =head2 CanItemBeReserved
 
-  $canReserve = &CanItemBeReserved($borrowernumber, $itemnumber, $branchcode, $params)
+  $canReserve = &CanItemBeReserved($patron, $item, $branchcode, $params)
   if ($canReserve->{status} eq 'OK') { #We can reserve this Item! }
 
-  current params are 'ignore_found_holds' - if true holds that have been trapped are not counted
+  current params are:
+  'ignore_found_holds' - if true holds that have been trapped are not counted
   toward the patron limit, used by checkHighHolds to avoid counting the hold we will fill with the
   current checkout against the high holds threshold
+  'ignore_hold_counts' - we use this routine to check if an item can fill a hold - on this case we
+  should not check if there are too many holds as we only csre about reservability
 
 @RETURNS { status => OK },              if the Item can be reserved.
          { status => ageRestricted },   if the Item is age restricted for this borrower.
@@ -362,21 +415,29 @@ sub CanBookBeReserved{
          { status => libraryNotPickupLocation },   if given branchcode is not configured to be a pickup location
          { status => cannotBeTransferred }, if branch transfer limit applies on given item and branchcode
          { status => pickupNotInHoldGroup }, pickup location is not in hold group, and pickup locations are only allowed from hold groups.
+         { status => recall }, if the borrower has already placed a recall on this item
 
 =cut
 
 sub CanItemBeReserved {
-    my ( $borrowernumber, $itemnumber, $pickup_branchcode, $params ) = @_;
+    my ( $patron, $item, $pickup_branchcode, $params ) = @_;
 
     my $dbh = C4::Context->dbh;
     my $ruleitemtype;    # itemtype of the matching issuing rule
     my $allowedreserves  = 0; # Total number of holds allowed across all records, default to none
 
+    # We check item branch if IndependentBranches is ON
+    # and canreservefromotherbranches is OFF
+    if ( C4::Context->preference('IndependentBranches')
+        and !C4::Context->preference('canreservefromotherbranches') )
+    {
+        if ( $item->homebranch ne $patron->branchcode ) {
+            return { status => 'cannotReserveFromOtherBranches' };
+        }
+    }
+
     # we retrieve borrowers and items informations #
     # item->{itype} will come for biblioitems if necessery
-    my $item       = Koha::Items->find($itemnumber);
-    my $biblio     = $item->biblio;
-    my $patron = Koha::Patrons->find( $borrowernumber );
     my $borrower = $patron->unblessed;
 
     # If an item is damaged and we don't allow holds on damaged items, we can stop right here
@@ -384,42 +445,40 @@ sub CanItemBeReserved {
       if ( $item->damaged
         && !C4::Context->preference('AllowHoldsOnDamagedItems') );
 
-    # Check for the age restriction
-    my ( $ageRestriction, $daysToAgeRestriction ) =
-      C4::Circulation::GetAgeRestriction( $biblio->biblioitem->agerestriction, $borrower );
-    return { status => 'ageRestricted' } if $daysToAgeRestriction && $daysToAgeRestriction > 0;
+    if( GetMarcFromKohaField('biblioitems.agerestriction') ){
+        my $biblio = $item->biblio;
+        # Check for the age restriction
+        my ( $ageRestriction, $daysToAgeRestriction ) =
+          C4::Circulation::GetAgeRestriction( $biblio->biblioitem->agerestriction, $borrower );
+        return { status => 'ageRestricted' } if $daysToAgeRestriction && $daysToAgeRestriction > 0;
+    }
 
     # Check that the patron doesn't have an item level hold on this item already
     return { status =>'itemAlreadyOnHold' }
-      if Koha::Holds->search( { borrowernumber => $borrowernumber, itemnumber => $itemnumber } )->count();
+      if ( !$params->{ignore_hold_counts} && Koha::Holds->search( { borrowernumber => $patron->borrowernumber, itemnumber => $item->itemnumber } )->count() );
 
     # Check that patron have not checked out this biblio (if AllowHoldsOnPatronsPossessions set)
     if ( !C4::Context->preference('AllowHoldsOnPatronsPossessions')
-        && C4::Circulation::CheckIfIssuedToPatron( $patron->borrowernumber, $biblio->biblionumber ) ) {
+        && C4::Circulation::CheckIfIssuedToPatron( $patron->borrowernumber, $item->biblionumber ) ) {
         return { status =>'alreadypossession' };
     }
 
-    my $controlbranch = C4::Context->preference('ReservesControlBranch');
+    # check if a recall exists on this item from this borrower
+    return { status => 'recall' }
+      if $patron->recalls->filter_by_current->search({ item_id => $item->itemnumber })->count;
 
-    my $querycount = q{
-        SELECT count(*) AS count
-          FROM reserves
-     LEFT JOIN items USING (itemnumber)
-     LEFT JOIN biblioitems ON (reserves.biblionumber=biblioitems.biblionumber)
-     LEFT JOIN borrowers USING (borrowernumber)
-         WHERE borrowernumber = ?
-    };
+    my $controlbranch = C4::Context->preference('ReservesControlBranch');
 
-    my $branchcode  = "";
+    my $reserves_control_branch;
     my $branchfield = "reserves.branchcode";
 
     if ( $controlbranch eq "ItemHomeLibrary" ) {
         $branchfield = "items.homebranch";
-        $branchcode  = $item->homebranch;
+        $reserves_control_branch  = $item->homebranch;
     }
     elsif ( $controlbranch eq "PatronLibrary" ) {
         $branchfield = "borrowers.branchcode";
-        $branchcode  = $borrower->{branchcode};
+        $reserves_control_branch  = $borrower->{branchcode};
     }
 
     # we retrieve rights
@@ -427,7 +486,7 @@ sub CanItemBeReserved {
         my $reservesallowed = Koha::CirculationRules->get_effective_rule({
                 itemtype     => $item->effective_itemtype,
                 categorycode => $borrower->{categorycode},
-                branchcode   => $branchcode,
+                branchcode   => $reserves_control_branch,
                 rule_name    => 'reservesallowed',
         })
     ) {
@@ -441,89 +500,108 @@ sub CanItemBeReserved {
     my $rights = Koha::CirculationRules->get_effective_rules({
         categorycode => $borrower->{'categorycode'},
         itemtype     => $item->effective_itemtype,
-        branchcode   => $branchcode,
+        branchcode   => $reserves_control_branch,
         rules        => ['holds_per_record','holds_per_day']
     });
     my $holds_per_record = $rights->{holds_per_record} // 1;
     my $holds_per_day    = $rights->{holds_per_day};
 
-    my $search_params = {
-        borrowernumber => $borrowernumber,
-        biblionumber   => $item->biblionumber,
-    };
-    $search_params->{found} = undef if $params->{ignore_found_holds};
-
-    my $holds = Koha::Holds->search($search_params);
-    if (   defined $holds_per_record && $holds_per_record ne ''
-        && $holds->count() >= $holds_per_record ) {
-        return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record };
+    if (   defined $holds_per_record && $holds_per_record ne '' ){
+        if ( $holds_per_record == 0 ) {
+            return { status => "noReservesAllowed" };
+        }
+        if ( !$params->{ignore_hold_counts} ) {
+            my $search_params = {
+                borrowernumber => $patron->borrowernumber,
+                biblionumber   => $item->biblionumber,
+            };
+            $search_params->{found} = undef if $params->{ignore_found_holds};
+            my $holds = Koha::Holds->search($search_params);
+            return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record } if $holds->count() >= $holds_per_record;
+        }
     }
 
-    my $today_holds = Koha::Holds->search({
-        borrowernumber => $borrowernumber,
-        reservedate    => dt_from_string->date
-    });
-
-    if (   defined $holds_per_day && $holds_per_day ne ''
-        && $today_holds->count() >= $holds_per_day )
+    if (!$params->{ignore_hold_counts} && defined $holds_per_day && $holds_per_day ne '')
     {
-        return { status => 'tooManyReservesToday', limit => $holds_per_day };
+        my $today_holds = Koha::Holds->search({
+            borrowernumber => $patron->borrowernumber,
+            reservedate    => dt_from_string->date
+        });
+        return { status => 'tooManyReservesToday', limit => $holds_per_day } if $today_holds->count() >= $holds_per_day;
     }
 
-    # we retrieve count
-
-    $querycount .= "AND ( $branchfield = ? OR $branchfield IS NULL )";
-
-    # If using item-level itypes, fall back to the record
-    # level itemtype if the hold has no associated item
-    $querycount .=
-      C4::Context->preference('item-level_itypes')
-      ? " AND COALESCE( items.itype, biblioitems.itemtype ) = ?"
-      : " AND biblioitems.itemtype = ?"
-      if defined $ruleitemtype;
+    # we check if it's ok or not
+    if ( defined $allowedreserves && $allowedreserves ne '' ){
+        if( $allowedreserves == 0 ){
+            return { status => 'noReservesAllowed' };
+        }
+        if ( !$params->{ignore_hold_counts} ) {
+            # we retrieve count
+            my $querycount = q{
+                SELECT count(*) AS count
+                  FROM reserves
+             LEFT JOIN items USING (itemnumber)
+             LEFT JOIN biblioitems ON (reserves.biblionumber=biblioitems.biblionumber)
+             LEFT JOIN borrowers USING (borrowernumber)
+                 WHERE borrowernumber = ?
+            };
+            $querycount .= "AND ( $branchfield = ? OR $branchfield IS NULL )";
+
+            # If using item-level itypes, fall back to the record
+            # level itemtype if the hold has no associated item
+            if ( defined $ruleitemtype ) {
+                if ( C4::Context->preference('item-level_itypes') ) {
+                    $querycount .= q{
+                        AND ( COALESCE( items.itype, biblioitems.itemtype ) = ?
+                           OR reserves.itemtype = ? )
+                    };
+                }
+                else {
+                    $querycount .= q{
+                        AND ( biblioitems.itemtype = ?
+                           OR reserves.itemtype = ? )
+                    };
+                }
+            }
 
-    my $sthcount = $dbh->prepare($querycount);
+            my $sthcount = $dbh->prepare($querycount);
 
-    if ( defined $ruleitemtype ) {
-        $sthcount->execute( $borrowernumber, $branchcode, $ruleitemtype );
-    }
-    else {
-        $sthcount->execute( $borrowernumber, $branchcode );
-    }
+            if ( defined $ruleitemtype ) {
+                $sthcount->execute( $patron->borrowernumber, $reserves_control_branch, $ruleitemtype, $ruleitemtype );
+            }
+            else {
+                $sthcount->execute( $patron->borrowernumber, $reserves_control_branch );
+            }
 
-    my $reservecount = "0";
-    if ( my $rowcount = $sthcount->fetchrow_hashref() ) {
-        $reservecount = $rowcount->{count};
-    }
+            my $reservecount = "0";
+            if ( my $rowcount = $sthcount->fetchrow_hashref() ) {
+                $reservecount = $rowcount->{count};
+            }
 
-    # we check if it's ok or not
-    if (   defined  $allowedreserves && $allowedreserves ne ''
-        && $reservecount >= $allowedreserves ) {
-        return { status => 'tooManyReserves', limit => $allowedreserves };
+            return { status => 'tooManyReserves', limit => $allowedreserves } if $reservecount >= $allowedreserves;
+        }
     }
 
     # Now we need to check hold limits by patron category
     my $rule = Koha::CirculationRules->get_effective_rule(
         {
-            categorycode => $borrower->{categorycode},
-            branchcode   => $branchcode,
+            categorycode => $patron->categorycode,
+            branchcode   => $reserves_control_branch,
             rule_name    => 'max_holds',
         }
     );
-    if ( $rule && defined( $rule->rule_value ) && $rule->rule_value ne '' ) {
+    if (!$params->{ignore_hold_counts} && $rule && defined( $rule->rule_value ) && $rule->rule_value ne '' ) {
         my $total_holds_count = Koha::Holds->search(
             {
-                borrowernumber => $borrower->{borrowernumber}
+                borrowernumber => $patron->borrowernumber
             }
         )->count();
 
         return { status => 'tooManyReserves', limit => $rule->rule_value} if $total_holds_count >= $rule->rule_value;
     }
 
-    my $reserves_control_branch =
-      GetReservesControlBranch( $item->unblessed(), $borrower );
     my $branchitemrule =
-      C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->itype ); # FIXME Should not be item->effective_itemtype?
+      C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->effective_itemtype );
 
     if ( $branchitemrule->{holdallowed} eq 'not_allowed' ) {
         return { status => 'notReservable' };
@@ -537,21 +615,11 @@ sub CanItemBeReserved {
 
     my $item_library = Koha::Libraries->find( {branchcode => $item->homebranch} );
     if ( $branchitemrule->{holdallowed} eq 'from_local_hold_group') {
-        if($borrower->{branchcode} ne $item->homebranch && !$item_library->validate_hold_sibling( {branchcode => $borrower->{branchcode}} )) {
+        if($patron->branchcode ne $item->homebranch && !$item_library->validate_hold_sibling( {branchcode => $patron->branchcode} )) {
             return { status => 'branchNotInHoldGroup' };
         }
     }
 
-    # If reservecount is ok, we check item branch if IndependentBranches is ON
-    # and canreservefromotherbranches is OFF
-    if ( C4::Context->preference('IndependentBranches')
-        and !C4::Context->preference('canreservefromotherbranches') )
-    {
-        if ( $item->homebranch ne $borrower->{branchcode} ) {
-            return { status => 'cannotReserveFromOtherBranches' };
-        }
-    }
-
     if ($pickup_branchcode) {
         my $destination = Koha::Libraries->find({
             branchcode => $pickup_branchcode,
@@ -591,7 +659,7 @@ sub CanReserveBeCanceledFromOpac {
     my ($reserve_id, $borrowernumber) = @_;
 
     return unless $reserve_id and $borrowernumber;
-    my $reserve = Koha::Holds->find($reserve_id);
+    my $reserve = Koha::Holds->find($reserve_id) or return;
 
     return 0 unless $reserve->borrowernumber == $borrowernumber;
     return $reserve->is_cancelable_from_opac;
@@ -891,7 +959,7 @@ sub CheckReserves {
                     my $branch = GetReservesControlBranch( $item->unblessed, $patron->unblessed );
                     my $branchitemrule = C4::Circulation::GetBranchItemRule($branch,$item->effective_itemtype);
                     next if ($branchitemrule->{'holdallowed'} eq 'not_allowed');
-                    next if (($branchitemrule->{'holdallowed'} eq 'from_home_library') && ($branch ne $patron->branchcode));
+                    next if (($branchitemrule->{'holdallowed'} eq 'from_home_library') && ($item->homebranch ne $patron->branchcode));
                     my $library = Koha::Libraries->find({branchcode=>$item->homebranch});
                     next if (($branchitemrule->{'holdallowed'} eq 'from_local_hold_group') && (!$library->validate_hold_sibling({branchcode => $patron->branchcode}) ));
                     my $hold_fulfillment_policy = $branchitemrule->{hold_fulfillment_policy};
@@ -932,7 +1000,13 @@ sub CancelExpiredReserves {
     my $expireWaiting = C4::Context->preference('ExpireReservesMaxPickUpDelay');
 
     my $dtf = Koha::Database->new->schema->storage->datetime_parser;
-    my $params = { expirationdate => { '<', $dtf->format_date($today) } };
+    my $params = {
+        -or => [
+            { expirationdate => { '<', $dtf->format_date($today) } },
+            { patron_expiration_date => { '<' => $dtf->format_date($today) } }
+        ]
+    };
+
     $params->{found} = [ { '!=', 'W' }, undef ]  unless $expireWaiting;
 
     # FIXME To move to Koha::Holds->search_expired (?)
@@ -948,6 +1022,7 @@ sub CancelExpiredReserves {
         if ( defined($hold->found) && $hold->found eq 'W' ) {
             $cancel_params->{charge_cancel_fee} = 1;
         }
+        $cancel_params->{autofill} = C4::Context->preference('ExpireReservesAutoFill');
         $hold->cancel( $cancel_params );
     }
 }
@@ -963,7 +1038,7 @@ Unsuspends all suspended reserves with a suspend_until date from before today.
 sub AutoUnsuspendReserves {
     my $today = dt_from_string();
 
-    my @holds = Koha::Holds->search( { suspend_until => { '<=' => $today->ymd() } } );
+    my @holds = Koha::Holds->search( { suspend_until => { '<=' => $today->ymd() } } )->as_list;
 
     map { $_->resume() } @holds;
 }
@@ -980,7 +1055,7 @@ sub AutoUnsuspendReserves {
 Change a hold request's priority or cancel it.
 
 C<$rank> specifies the effect of the change.  If C<$rank>
-is 'W' or 'n', nothing happens.  This corresponds to leaving a
+is 'n', nothing happens.  This corresponds to leaving a
 request alone when changing its priority in the holds queue
 for a bib.
 
@@ -992,6 +1067,9 @@ that the item is not waiting on the hold shelf, setting the
 priority to a non-zero value also sets the request's found
 status and waiting date to NULL.
 
+If the hold is 'found' (waiting, in-transit, processing) the
+only field that can be updated is the expiration date.
+
 The optional C<$itemnumber> parameter is used only when
 C<$rank> is a non-zero integer; if supplied, the itemnumber
 of the hold request is set accordingly; if omitted, the itemnumber
@@ -1015,9 +1093,9 @@ sub ModReserve {
     my $borrowernumber = $params->{'borrowernumber'};
     my $biblionumber = $params->{'biblionumber'};
     my $cancellation_reason = $params->{'cancellation_reason'};
+    my $date = $params->{expirationdate};
 
-    return if $rank eq "W";
-    return if $rank eq "n";
+    return if defined $rank && $rank eq "n";
 
     return unless ( $reserve_id || ( $borrowernumber && ( $biblionumber || $itemnumber ) ) );
 
@@ -1031,11 +1109,21 @@ sub ModReserve {
 
     $hold ||= Koha::Holds->find($reserve_id);
 
+    # FIXME Other calls may fail
+    Koha::Exceptions::ObjectNotFound->throw( 'No hold with id ' . $reserve_id ) unless $hold;
+
     if ( $rank eq "del" ) {
         $hold->cancel({ cancellation_reason => $cancellation_reason });
     }
+    elsif ($hold->found && $hold->priority eq '0' && $date) {
+        logaction( 'HOLDS', 'MODIFY', $hold->reserve_id, $hold )
+            if C4::Context->preference('HoldsLog');
+
+        # The only column that can be updated for a found hold is the expiration date
+        $hold->expirationdate($date)->store();
+    }
     elsif ($rank =~ /^\d+/ and $rank > 0) {
-        logaction( 'HOLDS', 'MODIFY', $hold->reserve_id, Dumper($hold->unblessed) )
+        logaction( 'HOLDS', 'MODIFY', $hold->reserve_id, $hold )
             if C4::Context->preference('HoldsLog');
 
         my $properties = {
@@ -1056,7 +1144,6 @@ sub ModReserve {
 
         if ( defined( $suspend_until ) ) {
             if ( $suspend_until ) {
-                $suspend_until = eval { dt_from_string( $suspend_until ) };
                 $hold->suspend_hold( $suspend_until );
             } else {
                 # If the hold is suspended leave the hold suspended, but convert it to an indefinite hold.
@@ -1069,54 +1156,6 @@ sub ModReserve {
     }
 }
 
-=head2 ModReserveFill
-
-  &ModReserveFill($reserve);
-
-Fill a reserve. If I understand this correctly, this means that the
-reserved book has been found and given to the patron who reserved it.
-
-C<$reserve> specifies the reserve to fill. It is a reference-to-hash
-whose keys are fields from the reserves table in the Koha database.
-
-=cut
-
-sub ModReserveFill {
-    my ($res) = @_;
-    my $reserve_id = $res->{'reserve_id'};
-
-    my $hold = Koha::Holds->find($reserve_id);
-    # get the priority on this record....
-    my $priority = $hold->priority;
-
-    # update the hold statuses, no need to store it though, we will be deleting it anyway
-    $hold->set(
-        {
-            found    => 'F',
-            priority => 0,
-        }
-    );
-
-    logaction( 'HOLDS', 'MODIFY', $hold->reserve_id, Dumper($hold->unblessed) )
-        if C4::Context->preference('HoldsLog');
-
-    # FIXME Must call Koha::Hold->cancel ? => No, should call ->filled and add the correct log
-    Koha::Old::Hold->new( $hold->unblessed() )->store();
-
-    $hold->delete();
-
-    if ( C4::Context->preference('HoldFeeMode') eq 'any_time_is_collected' ) {
-        my $reserve_fee = GetReserveFee( $hold->borrowernumber, $hold->biblionumber );
-        ChargeReserveFee( $hold->borrowernumber, $reserve_fee, $hold->biblio->title );
-    }
-
-    # now fix the priority on the others (if the priority wasn't
-    # already sorted!)....
-    unless ( $priority == 0 ) {
-        _FixPriority( { reserve_id => $reserve_id, biblionumber => $hold->biblionumber } );
-    }
-}
-
 =head2 ModReserveStatus
 
   &ModReserveStatus($itemnumber, $newstatus);
@@ -1149,7 +1188,7 @@ sub ModReserveStatus {
 
 =head2 ModReserveAffect
 
-  &ModReserveAffect($itemnumber,$borrowernumber,$diffBranchSend,$reserve_id, $desk_id);
+  &ModReserveAffect($itemnumber,$borrowernumber,$diffBranchSend,$reserve_id, $desk_id, $notify_library);
 
 This function affect an item and a status for a given reserve, either fetched directly
 by record_id, or by borrowernumber and itemnumber or biblionumber. If only biblionumber
@@ -1165,7 +1204,7 @@ This function also removes any entry of the hold in holds queue table.
 =cut
 
 sub ModReserveAffect {
-    my ( $itemnumber, $borrowernumber, $transferToDo, $reserve_id, $desk_id ) = @_;
+    my ( $itemnumber, $borrowernumber, $transferToDo, $reserve_id, $desk_id, $notify_library ) = @_;
     my $dbh = C4::Context->dbh;
 
     # we want to attach $itemnumber to $borrowernumber, find the biblionumber
@@ -1205,6 +1244,8 @@ sub ModReserveAffect {
         $transfer->receive if $transfer;
     }
 
+    _koha_notify_hold_changed( $hold ) if $notify_library;
+
     _FixPriority( { biblionumber => $biblionumber } );
     my $item = Koha::Items->find($itemnumber);
     if ( $item->location && $item->location eq 'CART'
@@ -1225,7 +1266,7 @@ sub ModReserveAffect {
     });
     $std->execute($hold->reserve_id);
 
-    logaction( 'HOLDS', 'MODIFY', $hold->reserve_id, Dumper($hold->get_from_storage->unblessed) )
+    logaction( 'HOLDS', 'MODIFY', $hold->reserve_id, $hold )
         if C4::Context->preference('HoldsLog');
 
     return;
@@ -1322,6 +1363,8 @@ sub IsAvailableForItemLevelRequest {
     #         or something similar - need to be
     #         consolidated
     my $itemtype = $item->effective_itemtype;
+    return 0
+      unless defined $itemtype;
     my $notforloan_per_itemtype = Koha::ItemTypes->find($itemtype)->notforloan;
 
     return 0 if
@@ -1376,7 +1419,7 @@ AllowHoldsOnDamagedItems or 'holdallowed' own/sibling library)
 sub ItemsAnyAvailableAndNotRestricted {
     my $param = shift;
 
-    my @items = Koha::Items->search( { biblionumber => $param->{biblionumber} } );
+    my @items = Koha::Items->search( { biblionumber => $param->{biblionumber} } )->as_list;
 
     foreach my $i (@items) {
         my $reserves_control_branch =
@@ -1397,7 +1440,7 @@ sub ItemsAnyAvailableAndNotRestricted {
             || Koha::ItemTypes->find( $i->effective_itemtype() )->notforloan
             || $branchitemrule->{holdallowed} eq 'from_home_library' && $param->{patron}->branchcode ne $i->homebranch
             || $branchitemrule->{holdallowed} eq 'from_local_hold_group' && ! $item_library->validate_hold_sibling( { branchcode => $param->{patron}->branchcode } )
-            || CanItemBeReserved( $param->{patron}->borrowernumber, $i->id )->{status} ne 'OK';
+            || CanItemBeReserved( $param->{patron}, $i )->{status} ne 'OK';
     }
 
     return 0;
@@ -1435,6 +1478,11 @@ sub AlterPriority {
       _FixPriority({ reserve_id => $reserve_id, rank => $last_priority });
     }
 
+    Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue(
+        {
+            biblio_ids => [ $hold->biblionumber ]
+        }
+    ) if C4::Context->preference('RealTimeHoldsQueue');
     # FIXME Should return the new priority
 }
 
@@ -1470,8 +1518,6 @@ be cleared when it is unsuspended.
 sub ToggleSuspend {
     my ( $reserve_id, $suspend_until ) = @_;
 
-    $suspend_until = dt_from_string($suspend_until) if ($suspend_until);
-
     my $hold = Koha::Holds->find( $reserve_id );
 
     if ( $hold->is_suspended ) {
@@ -1505,9 +1551,6 @@ sub SuspendAll {
     my $suspend_until  = $params{'suspend_until'}  || undef;
     my $suspend = defined( $params{'suspend'} ) ? $params{'suspend'} : 1;
 
-    $suspend_until = eval { dt_from_string($suspend_until) }
-      if ( defined($suspend_until) );
-
     return unless ( $borrowernumber || $biblionumber );
 
     my $params;
@@ -1515,7 +1558,7 @@ sub SuspendAll {
     $params->{borrowernumber} = $borrowernumber if $borrowernumber;
     $params->{biblionumber}   = $biblionumber if $biblionumber;
 
-    my @holds = Koha::Holds->search($params);
+    my @holds = Koha::Holds->search($params)->as_list;
 
     if ($suspend) {
         map { $_->suspend_hold($suspend_until) } @holds;
@@ -1628,9 +1671,9 @@ sub _FixPriority {
 
     # if index exists in array then move it to new position
     if ( $key > -1 && $rank ne 'del' && $rank > 0 ) {
-        my $new_rank = $rank -
-          1;    # $new_rank is what you want the new index to be in the array
+        my $new_rank = $rank - 1; # $new_rank is what you want the new index to be in the array
         my $moving_item = splice( @priority, $key, 1 );
+        $new_rank = scalar @priority if $new_rank > scalar @priority;
         splice( @priority, $new_rank, 0, $moving_item );
     }
 
@@ -1648,10 +1691,9 @@ sub _FixPriority {
         );
     }
 
-    $sth = $dbh->prepare( "SELECT reserve_id FROM reserves WHERE lowestPriority = 1 ORDER BY priority" );
-    $sth->execute();
-
     unless ( $ignoreSetLowestRank ) {
+        $sth = $dbh->prepare( "SELECT reserve_id FROM reserves WHERE lowestPriority = 1 AND biblionumber = ? ORDER BY priority" );
+        $sth->execute($biblionumber);
       while ( my $res = $sth->fetchrow_hashref() ) {
         _FixPriority({
             reserve_id => $res->{'reserve_id'},
@@ -1798,7 +1840,7 @@ sub _Findgroupreserve {
   _koha_notify_reserve( $hold->reserve_id );
 
 Sends a notification to the patron that their hold has been filled (through
-ModReserveAffect, _not_ ModReserveFill)
+ModReserveAffect)
 
 The letter code for this notice may be found using the following query:
 
@@ -1823,6 +1865,7 @@ The following tables are availalbe witin the notice:
 
 sub _koha_notify_reserve {
     my $reserve_id = shift;
+
     my $hold = Koha::Holds->find($reserve_id);
     my $borrowernumber = $hold->borrowernumber;
 
@@ -1836,16 +1879,15 @@ sub _koha_notify_reserve {
             message_name => 'Hold_Filled'
     } );
 
-    my $library = Koha::Libraries->find( $hold->branchcode )->unblessed;
-
-    my $admin_email_address = $library->{branchemail} || C4::Context->preference('KohaAdminEmailAddress');
+    my $library = Koha::Libraries->find( $hold->branchcode );
+    my $inbound_email_address = $library->inbound_email_address;
 
     my %letter_params = (
         module => 'reserves',
         branchcode => $hold->branchcode,
         lang => $patron->lang,
         tables => {
-            'branches'       => $library,
+            'branches'       => $library->unblessed,
             'borrowers'      => $patron->unblessed,
             'biblio'         => $hold->biblionumber,
             'biblioitems'    => $hold->biblionumber,
@@ -1869,7 +1911,7 @@ sub _koha_notify_reserve {
         C4::Letters::EnqueueLetter( {
             letter => $letter,
             borrowernumber => $borrowernumber,
-            from_address => $admin_email_address,
+            from_address => $inbound_email_address,
             message_transport_type => $mtt,
         } );
     };
@@ -1892,9 +1934,53 @@ sub _koha_notify_reserve {
 
 }
 
-=head2 _ShiftPriorityByDateAndPriority
+=head2 _koha_notify_hold_changed
 
-  $new_priority = _ShiftPriorityByDateAndPriority( $biblionumber, $reservedate, $priority );
+  _koha_notify_hold_changed( $hold_object );
+
+=cut
+
+sub _koha_notify_hold_changed {
+    my $hold = shift;
+
+    my $patron = $hold->patron;
+    my $library = $hold->branch;
+
+    my $letter = C4::Letters::GetPreparedLetter(
+        module      => 'reserves',
+        letter_code => 'HOLD_CHANGED',
+        branchcode  => $hold->branchcode,
+        substitute  => { today => output_pref( dt_from_string ) },
+        tables      => {
+            'branches'    => $library->unblessed,
+            'borrowers'   => $patron->unblessed,
+            'biblio'      => $hold->biblionumber,
+            'biblioitems' => $hold->biblionumber,
+            'reserves'    => $hold->unblessed,
+            'items'       => $hold->itemnumber,
+        },
+    );
+
+    return unless $letter;
+
+    my $email =
+         C4::Context->preference('ExpireReservesAutoFillEmail')
+      || $library->inbound_email_address;
+
+    C4::Letters::EnqueueLetter(
+        {
+            letter                 => $letter,
+            borrowernumber         => $patron->id,
+            message_transport_type => 'email',
+            from_address           => $email,
+            to_address             => $email,
+        }
+    );
+}
+
+=head2 _ShiftPriority
+
+  $new_priority = _ShiftPriority( $biblionumber, $priority );
 
 This increments the priority of all reserves after the one
 with either the lowest date after C<$reservedate>
@@ -1910,13 +1996,13 @@ the sub accounts for that too.
 
 =cut
 
-sub _ShiftPriorityByDateAndPriority {
-    my ( $biblio, $resdate, $new_priority ) = @_;
+sub _ShiftPriority {
+    my ( $biblio, $new_priority ) = @_;
 
     my $dbh = C4::Context->dbh;
-    my $query = "SELECT priority FROM reserves WHERE biblionumber = ? AND ( reservedate > ? OR priority > ? ) ORDER BY priority ASC LIMIT 1";
+    my $query = "SELECT priority FROM reserves WHERE biblionumber = ? AND priority > ? ORDER BY priority ASC LIMIT 1";
     my $sth = $dbh->prepare( $query );
-    $sth->execute( $biblio, $resdate, $new_priority );
+    $sth->execute( $biblio, $new_priority );
     my $min_priority = $sth->fetchrow;
     # if no such matches are found, $new_priority remains as original value
     $new_priority = $min_priority if ( $min_priority );
@@ -1959,10 +2045,11 @@ sub MoveReserve {
     my ( $restype, $res, undef ) = CheckReserves( $itemnumber, undef, $lookahead );
     return unless $res;
 
-    my $biblionumber     =  $res->{biblionumber};
+    my $biblionumber = $res->{biblionumber};
 
     if ($res->{borrowernumber} == $borrowernumber) {
-        ModReserveFill($res);
+        my $hold = Koha::Holds->find( $res->{reserve_id} );
+        $hold->fill;
     }
     else {
         # warn "Reserved";
@@ -1978,7 +2065,7 @@ sub MoveReserve {
 
         if ( $borr_res ) {
             # The item is reserved by the current patron
-            ModReserveFill($borr_res->unblessed);
+            $borr_res->fill;
         }
 
         if ( $cancelreserve eq 'revert' ) { ## Revert waiting reserve to priority 1
@@ -2076,12 +2163,20 @@ sub RevertWaitingStatus {
             priority    => 1,
             found       => undef,
             waitingdate => undef,
+            expirationdate => $hold->patron_expiration_date,
             itemnumber  => $hold->item_level_hold ? $hold->itemnumber : undef,
         }
     )->store();
 
     _FixPriority( { biblionumber => $hold->biblionumber } );
 
+    Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue(
+        {
+            biblio_ids => [ $hold->biblionumber ]
+        }
+    ) if C4::Context->preference('RealTimeHoldsQueue');
+
+
     return $hold;
 }
 
@@ -2178,7 +2273,7 @@ priority is based on the set of holds whose start date falls before
 the parameter value.
 
 After calculation of this priority, it is recommended to call
-_ShiftPriorityByDateAndPriority. Note that this is currently done in
+_ShiftPriority. Note that this is currently done in
 AddReserves.
 
 =cut
@@ -2249,7 +2344,7 @@ sub GetMaxPatronHoldsForRecord {
     my ( $borrowernumber, $biblionumber ) = @_;
 
     my $patron = Koha::Patrons->find($borrowernumber);
-    my @items = Koha::Items->search( { biblionumber => $biblionumber } );
+    my @items = Koha::Items->search( { biblionumber => $biblionumber } )->as_list;
 
     my $controlbranch = C4::Context->preference('ReservesControlBranch');