From 76a9fdcc06b18b9c9077c5bd0c75c99d5d283ee9 Mon Sep 17 00:00:00 2001 From: Arthur Suzuki Date: Thu, 11 Apr 2024 14:30:18 +0200 Subject: [PATCH] Bug 25408: (QA follow-up) using perltidy to resolve complaining QA scripts --- C4/Reserves.pm | 990 +++++++++++++++-------------- opac/opac-reserve.pl | 293 ++++----- t/db_dependent/Reserves.t | 1272 +++++++++++++++++++++---------------- 3 files changed, 1396 insertions(+), 1159 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index bc158b31a2e..2e88165cf76 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -20,11 +20,10 @@ package C4::Reserves; # You should have received a copy of the GNU General Public License # along with Koha; if not, see . - use Modern::Perl; use C4::Accounts; -use C4::Biblio qw( GetMarcFromKohaField ); +use C4::Biblio qw( GetMarcFromKohaField ); use C4::Circulation qw( CheckIfIssuedToPatron GetAgeRestriction GetBranchItemRule ); use C4::Context; use C4::Items qw( CartToShelf get_hostitemnumbers_of ); @@ -97,53 +96,54 @@ This modules provides somes functions to deal with reservations. =cut -our (@ISA, @EXPORT_OK); +our ( @ISA, @EXPORT_OK ); + BEGIN { require Exporter; - @ISA = qw(Exporter); + @ISA = qw(Exporter); @EXPORT_OK = qw( - AddReserve + AddReserve - GetReserveStatus + GetReserveStatus - GetOtherReserves - ChargeReserveFee - GetReserveFee + GetOtherReserves + ChargeReserveFee + GetReserveFee - ModReserveAffect - ModReserve - ModReserveStatus - ModReserveCancelAll - ModReserveMinusPriority - MoveReserve + ModReserveAffect + ModReserve + ModReserveStatus + ModReserveCancelAll + ModReserveMinusPriority + MoveReserve - CheckReserves - CanBookBeReserved - CanItemBeReserved - CanReserveBeCanceledFromOpac - CancelExpiredReserves + CheckReserves + CanBookBeReserved + CanItemBeReserved + CanReserveBeCanceledFromOpac + CancelExpiredReserves - AutoUnsuspendReserves + AutoUnsuspendReserves - IsAvailableForItemLevelRequest - ItemsAnyAvailableAndNotRestricted + IsAvailableForItemLevelRequest + ItemsAnyAvailableAndNotRestricted - AlterPriority - ToggleLowestPriority + AlterPriority + ToggleLowestPriority - ReserveSlip - ToggleSuspend - SuspendAll + ReserveSlip + ToggleSuspend + SuspendAll - CalculatePriority + CalculatePriority - IsItemOnHoldAndFound + IsItemOnHoldAndFound - GetMaxPatronHoldsForRecord + GetMaxPatronHoldsForRecord - MergeHolds + MergeHolds - RevertWaitingStatus + RevertWaitingStatus ); } @@ -185,27 +185,27 @@ The following tables are available within the HOLDPLACED_PATRON message: =cut sub AddReserve { - my ($params) = @_; - my $branch = $params->{branchcode}; - my $borrowernumber = $params->{borrowernumber}; - my $biblionumber = $params->{biblionumber}; - my $priority = $params->{priority}; - my $resdate = $params->{reservation_date}; + my ($params) = @_; + my $branch = $params->{branchcode}; + my $borrowernumber = $params->{borrowernumber}; + my $biblionumber = $params->{biblionumber}; + my $priority = $params->{priority}; + my $resdate = $params->{reservation_date}; my $patron_expiration_date = $params->{expiration_date}; - my $notes = $params->{notes}; - my $title = $params->{title}; - my $checkitem = $params->{itemnumber}; - my $found = $params->{found}; - my $itemtype = $params->{itemtype}; - my $non_priority = $params->{non_priority}; - my $item_group_id = $params->{item_group_id}; + my $notes = $params->{notes}; + my $title = $params->{title}; + my $checkitem = $params->{itemnumber}; + my $found = $params->{found}; + my $itemtype = $params->{itemtype}; + my $non_priority = $params->{non_priority}; + my $item_group_id = $params->{item_group_id}; $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 . if ( $checkitem and not C4::Context->preference('ReservesNeedReturns') ) { - my $item = Koha::Items->find( $checkitem ); # FIXME Prevent bad calls + my $item = Koha::Items->find($checkitem); # FIXME Prevent bad calls if ( # If item is already checked out, it cannot be set waiting @@ -221,15 +221,17 @@ sub AddReserve { && ( $item->damaged && C4::Context->preference('AllowHoldsOnDamagedItems') || !$item->damaged ) # Lastly, if this already has holds, we shouldn't make it waiting for the new hold - && !$item->current_holds->count ) + && !$item->current_holds->count + ) { $priority = 0; - $found = 'W'; + $found = 'W'; } } - if ( C4::Context->preference( 'AllowHoldDateInFuture' ) ) { - # Make room in reserves for this if passed a priority - $priority = _ShiftPriority( $biblionumber, $priority ); + if ( C4::Context->preference('AllowHoldDateInFuture') ) { + + # Make room in reserves for this if passed a priority + $priority = _ShiftPriority( $biblionumber, $priority ); } my $waitingdate; @@ -245,20 +247,20 @@ sub AddReserve { # updates take place here my $hold = Koha::Hold->new( { - borrowernumber => $borrowernumber, - biblionumber => $biblionumber, - item_group_id => $item_group_id, - reservedate => $resdate, - branchcode => $branch, - priority => $priority, - reservenotes => $notes, - itemnumber => $checkitem, - found => $found, - waitingdate => $waitingdate, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + item_group_id => $item_group_id, + reservedate => $resdate, + branchcode => $branch, + priority => $priority, + reservenotes => $notes, + itemnumber => $checkitem, + found => $found, + waitingdate => $waitingdate, patron_expiration_date => $patron_expiration_date, - itemtype => $itemtype, - item_level_hold => $checkitem ? 1 : 0, - non_priority => $non_priority ? 1 : 0, + itemtype => $itemtype, + item_level_hold => $checkitem ? 1 : 0, + non_priority => $non_priority ? 1 : 0, } )->store(); $hold->set_waiting() if $found && $found eq 'W'; @@ -277,26 +279,29 @@ sub AddReserve { ChargeReserveFee( $borrowernumber, $reserve_fee, $title ); } - _FixPriority({ biblionumber => $biblionumber}); + _FixPriority( { biblionumber => $biblionumber } ); # Send e-mail to librarian if syspref is active - if(C4::Context->preference("emailLibrarianWhenHoldIsPlaced")){ - my $patron = $hold->patron; + if ( C4::Context->preference("emailLibrarianWhenHoldIsPlaced") ) { + my $patron = $hold->patron; my $library = $patron->library; - if ( my $letter = C4::Letters::GetPreparedLetter ( - module => 'reserves', - letter_code => 'HOLDPLACED', - branchcode => $branch, - lang => $patron->lang, - tables => { - 'branches' => $library->unblessed, - 'borrowers' => $patron->unblessed, - 'biblio' => $biblionumber, - 'biblioitems' => $biblionumber, - 'items' => $checkitem, - 'reserves' => $hold->unblessed, - }, - ) ) { + if ( + my $letter = C4::Letters::GetPreparedLetter( + module => 'reserves', + letter_code => 'HOLDPLACED', + branchcode => $branch, + lang => $patron->lang, + tables => { + 'branches' => $library->unblessed, + 'borrowers' => $patron->unblessed, + 'biblio' => $biblionumber, + 'biblioitems' => $biblionumber, + 'items' => $checkitem, + 'reserves' => $hold->unblessed, + }, + ) + ) + { my $branch_email_address = $library->inbound_email_address; @@ -338,7 +343,7 @@ sub AddReserve { } } - Koha::Plugins->call('after_hold_create', $hold); + Koha::Plugins->call( 'after_hold_create', $hold ); Koha::Plugins->call( 'after_hold_action', { @@ -347,11 +352,8 @@ sub AddReserve { } ); - Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue( - { - biblio_ids => [ $biblionumber ] - } - ) if C4::Context->preference('RealTimeHoldsQueue'); + Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue( { biblio_ids => [$biblionumber] } ) + if C4::Context->preference('RealTimeHoldsQueue'); return $reserve_id; } @@ -367,13 +369,14 @@ See CanItemBeReserved() for possible return values. =cut -sub CanBookBeReserved{ - my ($borrowernumber, $biblionumber, $pickup_branchcode, $params) = @_; +sub CanBookBeReserved { + my ( $borrowernumber, $biblionumber, $pickup_branchcode, $params ) = @_; # Check that patron have not checked out this biblio (if AllowHoldsOnPatronsPossessions set) if ( !C4::Context->preference('AllowHoldsOnPatronsPossessions') - && C4::Circulation::CheckIfIssuedToPatron( $borrowernumber, $biblionumber ) ) { - return { status =>'alreadypossession' }; + && C4::Circulation::CheckIfIssuedToPatron( $borrowernumber, $biblionumber ) ) + { + return { status => 'alreadypossession' }; } if ( $params->{itemtype} ) { @@ -398,38 +401,43 @@ sub CanBookBeReserved{ { 'item.itype' => $params->{itemtype} } ] }, - { - join => ['item'] - } + { join => ['item'] } )->count; return { status => '' } - if defined $reservesallowed and $reservesallowed < $count + 1; + if defined $reservesallowed and $reservesallowed < $count + 1; } my $items; + #get items linked via host records my @hostitemnumbers = get_hostitemnumbers_of($biblionumber); - if (@hostitemnumbers){ - $items = Koha::Items->search({ - -or => [ - biblionumber => $biblionumber, - itemnumber => { -in => @hostitemnumbers } - ] - }); + if (@hostitemnumbers) { + $items = Koha::Items->search( + { + -or => [ + biblionumber => $biblionumber, + itemnumber => { -in => @hostitemnumbers } + ] + } + ); } else { - $items = Koha::Items->search({ biblionumber => $biblionumber}); + $items = Koha::Items->search( { biblionumber => $biblionumber } ); } my $canReserve = { status => '' }; - my $patron = Koha::Patrons->find( $borrowernumber ); + my $patron = Koha::Patrons->find($borrowernumber); while ( my $item = $items->next ) { $canReserve = CanItemBeReserved( $patron, $item, $pickup_branchcode, $params ); if ( C4::Context->interface eq 'opac' ) { - my $opacitemholds = Koha::CirculationRules->get_opacitemholds_policy({ item => $item, - patron => $patron }) // 'Y'; + my $opacitemholds = Koha::CirculationRules->get_opacitemholds_policy( + { + item => $item, + patron => $patron + } + ) // 'Y'; return { status => 'recordHoldNotAllowed' } if $opacitemholds eq 'F'; - return { status => 'OK' } if $canReserve->{status} eq 'recordHoldsOnly'; + return { status => 'OK' } if $canReserve->{status} eq 'recordHoldsOnly'; } return { status => 'OK' } if $canReserve->{status} eq 'OK'; } @@ -461,8 +469,9 @@ sub CanBookBeReserved{ =cut our $CanItemBeReserved_cache_key; + sub _cache { - my ( $return ) = @_; + my ($return) = @_; my $memory_cache = Koha::Cache::Memory::Lite->get_instance(); $memory_cache->set_in_cache( $CanItemBeReserved_cache_key, $return ); return $return; @@ -472,15 +481,16 @@ sub CanItemBeReserved { my ( $patron, $item, $pickup_branchcode, $params ) = @_; my $memory_cache = Koha::Cache::Memory::Lite->get_instance(); - $CanItemBeReserved_cache_key = sprintf "Hold_CanItemBeReserved:%s:%s:%s", $patron->id, $item->itemnumber, $pickup_branchcode || ""; + $CanItemBeReserved_cache_key = sprintf "Hold_CanItemBeReserved:%s:%s:%s", $patron->id, $item->itemnumber, + $pickup_branchcode || ""; if ( $params->{get_from_cache} ) { my $cached = $memory_cache->get_from_cache($CanItemBeReserved_cache_key); return $cached if $cached; } 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 + 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 @@ -497,31 +507,35 @@ sub CanItemBeReserved { my $borrower = $patron->unblessed; # If an item is damaged and we don't allow holds on damaged items, we can stop right here - return _cache { status =>'damaged' } - if ( $item->damaged + return _cache { status => 'damaged' } + if ( $item->damaged && !C4::Context->preference('AllowHoldsOnDamagedItems') ); - if( GetMarcFromKohaField('biblioitems.agerestriction') ){ + if ( GetMarcFromKohaField('biblioitems.agerestriction') ) { my $biblio = $item->biblio; + # Check for the age restriction my ( $ageRestriction, $daysToAgeRestriction ) = - C4::Circulation::GetAgeRestriction( $biblio->biblioitem->agerestriction, $borrower ); + C4::Circulation::GetAgeRestriction( $biblio->biblioitem->agerestriction, $borrower ); return _cache { status => 'ageRestricted' } if $daysToAgeRestriction && $daysToAgeRestriction > 0; } # Check that the patron doesn't have an item level hold on this item already - return _cache { status =>'itemAlreadyOnHold' } - if ( !$params->{ignore_hold_counts} && Koha::Holds->search( { borrowernumber => $patron->borrowernumber, itemnumber => $item->itemnumber } )->count() ); + return _cache { status => 'itemAlreadyOnHold' } + 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, $item->biblionumber ) ) { - return _cache { status =>'alreadypossession' }; + && C4::Circulation::CheckIfIssuedToPatron( $patron->borrowernumber, $item->biblionumber ) ) + { + return _cache { status => 'alreadypossession' }; } # check if a recall exists on this item from this borrower return _cache { status => 'recall' } - if $patron->recalls->filter_by_current->search({ item_id => $item->itemnumber })->count; + if $patron->recalls->filter_by_current->search( { item_id => $item->itemnumber } )->count; my $controlbranch = C4::Context->preference('ReservesControlBranch'); @@ -529,41 +543,44 @@ sub CanItemBeReserved { my $branchfield = "reserves.branchcode"; if ( $controlbranch eq "ItemHomeLibrary" ) { - $branchfield = "items.homebranch"; - $reserves_control_branch = $item->homebranch; - } - elsif ( $controlbranch eq "PatronLibrary" ) { - $branchfield = "borrowers.branchcode"; - $reserves_control_branch = $borrower->{branchcode}; + $branchfield = "items.homebranch"; + $reserves_control_branch = $item->homebranch; + } elsif ( $controlbranch eq "PatronLibrary" ) { + $branchfield = "borrowers.branchcode"; + $reserves_control_branch = $borrower->{branchcode}; } # we retrieve rights if ( - my $reservesallowed = Koha::CirculationRules->get_effective_rule({ + my $reservesallowed = Koha::CirculationRules->get_effective_rule( + { itemtype => $item->effective_itemtype, categorycode => $borrower->{categorycode}, branchcode => $reserves_control_branch, rule_name => 'reservesallowed', - }) - ) { - $ruleitemtype = $reservesallowed->itemtype; - $allowedreserves = $reservesallowed->rule_value // 0; #undefined is 0, blank is unlimited - } - else { + } + ) + ) + { + $ruleitemtype = $reservesallowed->itemtype; + $allowedreserves = $reservesallowed->rule_value // 0; #undefined is 0, blank is unlimited + } else { $ruleitemtype = undef; } - my $rights = Koha::CirculationRules->get_effective_rules({ - categorycode => $borrower->{'categorycode'}, - itemtype => $item->effective_itemtype, - branchcode => $reserves_control_branch, - rules => ['holds_per_record','holds_per_day','opacitemholds'] - }); + my $rights = Koha::CirculationRules->get_effective_rules( + { + categorycode => $borrower->{'categorycode'}, + itemtype => $item->effective_itemtype, + branchcode => $reserves_control_branch, + rules => [ 'holds_per_record', 'holds_per_day', 'opacitemholds' ] + } + ); my $holds_per_record = $rights->{holds_per_record} // 1; my $holds_per_day = $rights->{holds_per_day}; my $opacitemholds = $rights->{opacitemholds} // 'Y'; - if ( defined $holds_per_record && $holds_per_record ne '' ){ + if ( defined $holds_per_record && $holds_per_record ne '' ) { if ( $holds_per_record == 0 ) { return _cache { status => "noReservesAllowed" }; } @@ -573,25 +590,29 @@ sub CanItemBeReserved { biblionumber => $item->biblionumber, }; my $holds = Koha::Holds->search($search_params); - return _cache { status => "tooManyHoldsForThisRecord", limit => $holds_per_record } if $holds->count() >= $holds_per_record; + return _cache { status => "tooManyHoldsForThisRecord", limit => $holds_per_record } + if $holds->count() >= $holds_per_record; } } - if (!$params->{ignore_hold_counts} && defined $holds_per_day && $holds_per_day ne '') - { - my $today_holds = Koha::Holds->search({ - borrowernumber => $patron->borrowernumber, - reservedate => dt_from_string->date - }); - return _cache { status => 'tooManyReservesToday', limit => $holds_per_day } if $today_holds->count() >= $holds_per_day; + if ( !$params->{ignore_hold_counts} && defined $holds_per_day && $holds_per_day ne '' ) { + my $today_holds = Koha::Holds->search( + { + borrowernumber => $patron->borrowernumber, + reservedate => dt_from_string->date + } + ); + return _cache { status => 'tooManyReservesToday', limit => $holds_per_day } + if $today_holds->count() >= $holds_per_day; } # we check if it's ok or not - if ( defined $allowedreserves && $allowedreserves ne '' ){ - if( $allowedreserves == 0 ){ + if ( defined $allowedreserves && $allowedreserves ne '' ) { + if ( $allowedreserves == 0 ) { return _cache { status => 'noReservesAllowed' }; } if ( !$params->{ignore_hold_counts} ) { + # we retrieve count my $querycount = q{ SELECT count(*) AS count @@ -611,8 +632,7 @@ sub CanItemBeReserved { AND ( COALESCE( items.itype, biblioitems.itemtype ) = ? OR reserves.itemtype = ? ) }; - } - else { + } else { $querycount .= q{ AND ( biblioitems.itemtype = ? OR reserves.itemtype = ? ) @@ -624,8 +644,7 @@ sub CanItemBeReserved { if ( defined $ruleitemtype ) { $sthcount->execute( $patron->borrowernumber, $reserves_control_branch, $ruleitemtype, $ruleitemtype ); - } - else { + } else { $sthcount->execute( $patron->borrowernumber, $reserves_control_branch ); } @@ -634,7 +653,8 @@ sub CanItemBeReserved { $reservecount = $rowcount->{count}; } - return _cache { status => 'tooManyReserves', limit => $allowedreserves } if $reservecount >= $allowedreserves; + return _cache { status => 'tooManyReserves', limit => $allowedreserves } + if $reservecount >= $allowedreserves; } } @@ -646,18 +666,14 @@ sub CanItemBeReserved { rule_name => 'max_holds', } ); - if (!$params->{ignore_hold_counts} && $rule && defined( $rule->rule_value ) && $rule->rule_value ne '' ) { - my $total_holds_count = Koha::Holds->search( - { - borrowernumber => $patron->borrowernumber - } - )->count(); + if ( !$params->{ignore_hold_counts} && $rule && defined( $rule->rule_value ) && $rule->rule_value ne '' ) { + my $total_holds_count = Koha::Holds->search( { borrowernumber => $patron->borrowernumber } )->count(); - return _cache { status => 'tooManyReserves', limit => $rule->rule_value} if $total_holds_count >= $rule->rule_value; + return _cache { status => 'tooManyReserves', limit => $rule->rule_value } + if $total_holds_count >= $rule->rule_value; } - my $branchitemrule = - C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->effective_itemtype ); + my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->effective_itemtype ); if ( $branchitemrule->{holdallowed} eq 'not_allowed' ) { return _cache { status => 'notReservable' }; @@ -669,36 +685,45 @@ sub CanItemBeReserved { return _cache { status => 'cannotReserveFromOtherBranches' }; } - my $item_library = Koha::Libraries->find( {branchcode => $item->homebranch} ); - if ( $branchitemrule->{holdallowed} eq 'from_local_hold_group') { - if($patron->branchcode ne $item->homebranch && !$item_library->validate_hold_sibling( {branchcode => $patron->branchcode} )) { + my $item_library = Koha::Libraries->find( { branchcode => $item->homebranch } ); + if ( $branchitemrule->{holdallowed} eq 'from_local_hold_group' ) { + if ( $patron->branchcode ne $item->homebranch + && !$item_library->validate_hold_sibling( { branchcode => $patron->branchcode } ) ) + { return _cache { status => 'branchNotInHoldGroup' }; } } if ($pickup_branchcode) { - my $destination = Koha::Libraries->find({ - branchcode => $pickup_branchcode, - }); + my $destination = Koha::Libraries->find( + { + branchcode => $pickup_branchcode, + } + ); unless ($destination) { return _cache { status => 'libraryNotFound' }; } - unless ($destination->pickup_location) { + unless ( $destination->pickup_location ) { return _cache { status => 'libraryNotPickupLocation' }; } - unless ($item->can_be_transferred({ to => $destination })) { + unless ( $item->can_be_transferred( { to => $destination } ) ) { return _cache { status => 'cannotBeTransferred' }; } - if ($branchitemrule->{hold_fulfillment_policy} eq 'holdgroup' && !$item_library->validate_hold_sibling( {branchcode => $pickup_branchcode} )) { + if ( $branchitemrule->{hold_fulfillment_policy} eq 'holdgroup' + && !$item_library->validate_hold_sibling( { branchcode => $pickup_branchcode } ) ) + { return _cache { status => 'pickupNotInHoldGroup' }; } - if ($branchitemrule->{hold_fulfillment_policy} eq 'patrongroup' && !Koha::Libraries->find({branchcode => $borrower->{branchcode}})->validate_hold_sibling({branchcode => $pickup_branchcode})) { + if ( $branchitemrule->{hold_fulfillment_policy} eq 'patrongroup' + && !Koha::Libraries->find( { branchcode => $borrower->{branchcode} } ) + ->validate_hold_sibling( { branchcode => $pickup_branchcode } ) ) + { return _cache { status => 'pickupNotInHoldGroup' }; } } - if ( $opacitemholds eq "N" && C4::Context->interface eq 'opac') { + if ( $opacitemholds eq "N" && C4::Context->interface eq 'opac' ) { return _cache { status => "recordHoldsOnly" }; } @@ -716,7 +741,7 @@ sub CanItemBeReserved { =cut sub CanReserveBeCanceledFromOpac { - my ($reserve_id, $borrowernumber) = @_; + my ( $reserve_id, $borrowernumber ) = @_; return unless $reserve_id and $borrowernumber; my $reserve = Koha::Holds->find($reserve_id) or return; @@ -742,6 +767,7 @@ sub GetOtherReserves { if ($checkreserves) { if ( $item->holdingbranch ne $checkreserves->{'branchcode'} ) { $messages->{'transfert'} = $checkreserves->{'branchcode'}; + #minus priorities of others reservs ModReserveMinusPriority( $itemnumber, @@ -754,18 +780,18 @@ sub GetOtherReserves { $item->holdingbranch, $checkreserves->{'branchcode'}, 'Reserve' - ), - ; + ), + ; } - #step 2b : case of a reservation on the same branch, set the waiting status + #step 2b : case of a reservation on the same branch, set the waiting status else { $messages->{'waiting'} = 1; ModReserveMinusPriority( $itemnumber, $checkreserves->{'reserve_id'}, ); - ModReserveStatus($itemnumber,'W'); + ModReserveStatus( $itemnumber, 'W' ); } $nextreservinfo = $checkreserves; @@ -823,21 +849,27 @@ SELECT COUNT(*) FROM reserves WHERE biblionumber=? AND borrowernumber<>? }; my $dbh = C4::Context->dbh; - my ( $fee ) = $dbh->selectrow_array( $borquery, undef, ($borrowernumber) ); + my ($fee) = $dbh->selectrow_array( $borquery, undef, ($borrowernumber) ); $fee += 0; my $hold_fee_mode = C4::Context->preference('HoldFeeMode') || 'not_always'; - if( $fee and $fee > 0 and $hold_fee_mode eq 'not_always' ) { + if ( $fee and $fee > 0 and $hold_fee_mode eq 'not_always' ) { + # This is a reconstruction of the old code: # Compare number of items with items issued, and optionally check holds # If not all items are issued and there are no holds: charge no fee # NOTE: Lost, damaged, not-for-loan, etc. are just ignored here my ( $notissued, $reserved ); - ( $notissued ) = $dbh->selectrow_array( $issue_qry, undef, - ( $biblionumber ) ); - if( $notissued == 0 ) { + ($notissued) = $dbh->selectrow_array( + $issue_qry, undef, + ($biblionumber) + ); + if ( $notissued == 0 ) { + # all items are issued - ( $reserved ) = $dbh->selectrow_array( $holds_qry, undef, - ( $biblionumber, $borrowernumber ) ); + ($reserved) = $dbh->selectrow_array( + $holds_qry, undef, + ( $biblionumber, $borrowernumber ) + ); $fee = 0 if $reserved == 0; } else { $fee = 0; @@ -864,22 +896,22 @@ sub GetReserveStatus { my $dbh = C4::Context->dbh; - my ($sth, $found, $priority); - if ( $itemnumber ) { + my ( $sth, $found, $priority ); + if ($itemnumber) { $sth = $dbh->prepare("SELECT found, priority FROM reserves WHERE itemnumber = ? order by priority LIMIT 1"); $sth->execute($itemnumber); - ($found, $priority) = $sth->fetchrow_array; + ( $found, $priority ) = $sth->fetchrow_array; } - if(defined $found) { - return 'Waiting' if $found eq 'W' and $priority == 0; - return 'Processing' if $found eq 'P'; - return 'Finished' if $found eq 'F'; + if ( defined $found ) { + return 'Waiting' if $found eq 'W' and $priority == 0; + return 'Processing' if $found eq 'P'; + return 'Finished' if $found eq 'F'; } return 'Reserved' if defined $priority && $priority > 0; - return ''; # empty string here will remove need for checking undef, or less log lines + return ''; # empty string here will remove need for checking undef, or less log lines } =head2 CheckReserves @@ -912,9 +944,10 @@ table in the Koha database. sub CheckReserves { my ( $item, $lookahead_days, $ignore_borrowers ) = @_; + # note: we get the itemnumber because we might have started w/ just the barcode. Now we know for sure we have it. - return unless $item; # bail if we got nothing. + return unless $item; # bail if we got nothing. return if ( $item->damaged && !C4::Context->preference('AllowHoldsOnDamagedItems') ); @@ -927,16 +960,15 @@ sub CheckReserves { my $dont_trap = C4::Context->preference('TrapHoldsOnOrder') ? $item->notforloan > 0 : $item->notforloan; if ( !$dont_trap ) { my $item_type = $item->effective_itemtype; - if ( $item_type ) { - return if Koha::ItemTypes->find( $item_type )->notforloan; + if ($item_type) { + return if Koha::ItemTypes->find($item_type)->notforloan; } - } - else { + } else { return; } # Find this item in the reserves - my @reserves = _Findgroupreserve( $item->biblionumber, $item->itemnumber, $lookahead_days, $ignore_borrowers); + my @reserves = _Findgroupreserve( $item->biblionumber, $item->itemnumber, $lookahead_days, $ignore_borrowers ); # $priority and $highest are used to find the most important item # in the list returned by &_Findgroupreserve. (The lower $priority, @@ -944,19 +976,19 @@ sub CheckReserves { # $highest is the most important item we've seen so far. my $highest; - if (scalar @reserves) { - my $LocalHoldsPriority = C4::Context->preference('LocalHoldsPriority'); + if ( scalar @reserves ) { + my $LocalHoldsPriority = C4::Context->preference('LocalHoldsPriority'); my $LocalHoldsPriorityPatronControl = C4::Context->preference('LocalHoldsPriorityPatronControl'); - my $LocalHoldsPriorityItemControl = C4::Context->preference('LocalHoldsPriorityItemControl'); - my $priority = 10000000; + my $LocalHoldsPriorityItemControl = C4::Context->preference('LocalHoldsPriorityItemControl'); + my $priority = 10000000; foreach my $res (@reserves) { - if ($res->{'found'} && $res->{'found'} eq 'W') { - return ( "Waiting", $res, \@reserves ); # Found it, it is waiting - } elsif ($res->{'found'} && $res->{'found'} eq 'P') { - return ( "Processing", $res, \@reserves ); # Found determinated hold, e. g. the transferred one - } elsif ($res->{'found'} && $res->{'found'} eq 'T') { - return ( "Transferred", $res, \@reserves ); # Found determinated hold, e. g. the transferred one + if ( $res->{'found'} && $res->{'found'} eq 'W' ) { + return ( "Waiting", $res, \@reserves ); # Found it, it is waiting + } elsif ( $res->{'found'} && $res->{'found'} eq 'P' ) { + return ( "Processing", $res, \@reserves ); # Found determinated hold, e. g. the transferred one + } elsif ( $res->{'found'} && $res->{'found'} eq 'T' ) { + return ( "Transferred", $res, \@reserves ); # Found determinated hold, e. g. the transferred one } else { my $patron; my $local_hold_match; @@ -964,36 +996,46 @@ sub CheckReserves { if ($LocalHoldsPriority) { $patron = Koha::Patrons->find( $res->{borrowernumber} ); - unless ($item->exclude_from_local_holds_priority || $patron->category->exclude_from_local_holds_priority) { - my $local_holds_priority_item_branchcode = - $item->$LocalHoldsPriorityItemControl; + unless ( $item->exclude_from_local_holds_priority + || $patron->category->exclude_from_local_holds_priority ) + { + my $local_holds_priority_item_branchcode = $item->$LocalHoldsPriorityItemControl; my $local_holds_priority_patron_branchcode = - ( $LocalHoldsPriorityPatronControl eq 'PickupLibrary' ) - ? $res->{branchcode} - : ( $LocalHoldsPriorityPatronControl eq 'HomeLibrary' ) - ? $patron->branchcode - : undef; + ( $LocalHoldsPriorityPatronControl eq 'PickupLibrary' ) ? $res->{branchcode} + : ( $LocalHoldsPriorityPatronControl eq 'HomeLibrary' ) ? $patron->branchcode + : undef; $local_hold_match = - $local_holds_priority_item_branchcode eq - $local_holds_priority_patron_branchcode; + $local_holds_priority_item_branchcode eq $local_holds_priority_patron_branchcode; } } # See if this item is more important than what we've got so far if ( ( $res->{'priority'} && $res->{'priority'} < $priority ) || $local_hold_match ) { - next if $res->{item_group_id} && ( !$item->item_group || $item->item_group->id != $res->{item_group_id} ); + next + if $res->{item_group_id} + && ( !$item->item_group || $item->item_group->id != $res->{item_group_id} ); next if $res->{itemtype} && $res->{itemtype} ne $item->effective_itemtype; $patron //= Koha::Patrons->find( $res->{borrowernumber} ); - my $branch = Koha::Policy::Holds->holds_control_library( $item, $patron ); - my $branchitemrule = C4::Circulation::GetBranchItemRule($branch,$item->effective_itemtype); - next if ($branchitemrule->{'holdallowed'} eq 'not_allowed'); - 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 $branch = Koha::Policy::Holds->holds_control_library( $item, $patron ); + my $branchitemrule = C4::Circulation::GetBranchItemRule( $branch, $item->effective_itemtype ); + next if ( $branchitemrule->{'holdallowed'} eq 'not_allowed' ); + 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}; - next if ( ($hold_fulfillment_policy eq 'holdgroup') && (!$library->validate_hold_sibling({branchcode => $res->{branchcode}})) ); - next if ( ($hold_fulfillment_policy eq 'homebranch') && ($res->{branchcode} ne $item->$hold_fulfillment_policy) ); - next if ( ($hold_fulfillment_policy eq 'holdingbranch') && ($res->{branchcode} ne $item->$hold_fulfillment_policy) ); + next + if ( ( $hold_fulfillment_policy eq 'holdgroup' ) + && ( !$library->validate_hold_sibling( { branchcode => $res->{branchcode} } ) ) ); + next + if ( ( $hold_fulfillment_policy eq 'homebranch' ) + && ( $res->{branchcode} ne $item->$hold_fulfillment_policy ) ); + next + if ( ( $hold_fulfillment_policy eq 'holdingbranch' ) + && ( $res->{branchcode} ne $item->$hold_fulfillment_policy ) ); next unless $item->can_be_transferred( { to => Koha::Libraries->find( $res->{branchcode} ) } ); $priority = $res->{'priority'}; $highest = $res; @@ -1010,7 +1052,7 @@ sub CheckReserves { return ( "Reserved", $highest, \@reserves ); } - return ( '' ); + return (''); } =head2 CancelExpiredReserves @@ -1023,35 +1065,35 @@ Cancels all reserves with an expiration date from before today. sub CancelExpiredReserves { my $cancellation_reason = shift; - my $today = dt_from_string(); - my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays'); - my $expireWaiting = C4::Context->preference('ExpireReservesMaxPickUpDelay'); + my $today = dt_from_string(); + my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays'); + my $expireWaiting = C4::Context->preference('ExpireReservesMaxPickUpDelay'); - my $dtf = Koha::Database->new->schema->storage->datetime_parser; + my $dtf = Koha::Database->new->schema->storage->datetime_parser; my $params = { -or => [ - { expirationdate => { '<', $dtf->format_date($today) } }, + { expirationdate => { '<', $dtf->format_date($today) } }, { patron_expiration_date => { '<' => $dtf->format_date($today) } } ] }; - $params->{found} = [ { '!=', 'W' }, undef ] unless $expireWaiting; + $params->{found} = [ { '!=', 'W' }, undef ] unless $expireWaiting; # FIXME To move to Koha::Holds->search_expired (?) - my $holds = Koha::Holds->search( $params ); + my $holds = Koha::Holds->search($params); while ( my $hold = $holds->next ) { my $calendar = Koha::Calendar->new( branchcode => $hold->branchcode ); - next if !$cancel_on_holidays && $calendar->is_holiday( $today ); + next if !$cancel_on_holidays && $calendar->is_holiday($today); my $cancel_params = {}; $cancel_params->{cancellation_reason} = $cancellation_reason if defined($cancellation_reason); - if ( defined($hold->found) && $hold->found eq 'W' ) { + 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 ); + $hold->cancel($cancel_params); } } @@ -1111,27 +1153,28 @@ itemnumber and supplying itemnumber. =cut sub ModReserve { - my ( $params ) = @_; - - my $rank = $params->{'rank'}; - my $reserve_id = $params->{'reserve_id'}; - my $branchcode = $params->{'branchcode'}; - my $itemnumber = $params->{'itemnumber'}; - my $suspend_until = $params->{'suspend_until'}; - my $borrowernumber = $params->{'borrowernumber'}; - my $biblionumber = $params->{'biblionumber'}; + my ($params) = @_; + + my $rank = $params->{'rank'}; + my $reserve_id = $params->{'reserve_id'}; + my $branchcode = $params->{'branchcode'}; + my $itemnumber = $params->{'itemnumber'}; + my $suspend_until = $params->{'suspend_until'}; + my $borrowernumber = $params->{'borrowernumber'}; + my $biblionumber = $params->{'biblionumber'}; my $cancellation_reason = $params->{'cancellation_reason'}; - my $date = $params->{expirationdate}; + my $date = $params->{expirationdate}; return if defined $rank && $rank eq "n"; return unless ( $reserve_id || ( $borrowernumber && ( $biblionumber || $itemnumber ) ) ); my $hold; - unless ( $reserve_id ) { - my $holds = Koha::Holds->search({ biblionumber => $biblionumber, borrowernumber => $borrowernumber, itemnumber => $itemnumber }); - return unless $holds->count; # FIXME Should raise an exception - $hold = $holds->next; + unless ($reserve_id) { + my $holds = Koha::Holds->search( + { biblionumber => $biblionumber, borrowernumber => $borrowernumber, itemnumber => $itemnumber } ); + return unless $holds->count; # FIXME Should raise an exception + $hold = $holds->next; $reserve_id = $hold->reserve_id; } @@ -1141,16 +1184,14 @@ sub ModReserve { 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) { + $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) { + } elsif ( $rank =~ /^\d+/ and $rank > 0 ) { logaction( 'HOLDS', 'MODIFY', $hold->reserve_id, $hold ) if C4::Context->preference('HoldsLog'); @@ -1161,26 +1202,27 @@ sub ModReserve { found => undef, waitingdate => undef }; - if (exists $params->{reservedate}) { + if ( exists $params->{reservedate} ) { $properties->{reservedate} = $params->{reservedate} || undef; } - if (exists $params->{expirationdate}) { + if ( exists $params->{expirationdate} ) { $properties->{expirationdate} = $params->{expirationdate} || undef; } $hold->set($properties)->store(); - if ( defined( $suspend_until ) ) { - if ( $suspend_until ) { - $hold->suspend_hold( $suspend_until ); + if ( defined($suspend_until) ) { + if ($suspend_until) { + $hold->suspend_hold($suspend_until); } else { + # If the hold is suspended leave the hold suspended, but convert it to an indefinite hold. # If the hold is not suspended, this does nothing. $hold->set( { suspend_until => undef } )->store(); } } - _FixPriority({ reserve_id => $reserve_id, rank =>$rank }); + _FixPriority( { reserve_id => $reserve_id, rank => $rank } ); } } @@ -1199,18 +1241,21 @@ $newstatus is the new status. sub ModReserveStatus { #first : check if we have a reservation for this item . - my ($itemnumber, $newstatus) = @_; + my ( $itemnumber, $newstatus ) = @_; my $dbh = C4::Context->dbh; - my $query = "UPDATE reserves SET found = ?, waitingdate = NOW() WHERE itemnumber = ? AND found IS NULL AND priority = 0"; + my $query = + "UPDATE reserves SET found = ?, waitingdate = NOW() WHERE itemnumber = ? AND found IS NULL AND priority = 0"; my $sth_set = $dbh->prepare($query); $sth_set->execute( $newstatus, $itemnumber ); my $item = Koha::Items->find($itemnumber); - if ( $item->location && $item->location eq 'CART' + if ( $item->location + && $item->location eq 'CART' && ( !$item->permanent_location || $item->permanent_location ne 'CART' ) - && $newstatus ) { - CartToShelf( $itemnumber ); + && $newstatus ) + { + CartToShelf($itemnumber); } } @@ -1245,10 +1290,13 @@ sub ModReserveAffect { # waiting in order to not send duplicate hold filled notifications my $hold; + # Find hold by id if we have it - $hold = Koha::Holds->find( $reserve_id ) if $reserve_id; + $hold = Koha::Holds->find($reserve_id) if $reserve_id; + # Find item level hold for this item if there is one $hold ||= Koha::Holds->search( { borrowernumber => $borrowernumber, itemnumber => $itemnumber } )->next(); + # Find record level hold if there is no item level hold $hold ||= Koha::Holds->search( { borrowernumber => $borrowernumber, biblionumber => $biblionumber } )->next(); @@ -1260,28 +1308,33 @@ sub ModReserveAffect { if ($transferToDo) { $hold->set_transfer(); - } elsif (C4::Context->preference('HoldsNeedProcessingSIP') - && C4::Context->interface eq 'sip' - && !$already_on_shelf) { + } elsif ( C4::Context->preference('HoldsNeedProcessingSIP') + && C4::Context->interface eq 'sip' + && !$already_on_shelf ) + { $hold->set_processing(); } else { $hold->set_waiting($desk_id); _koha_notify_reserve( $hold->reserve_id ) unless $already_on_shelf; + # Complete transfer if one exists my $transfer = $hold->item->get_transfer; $transfer->receive if $transfer; } - _koha_notify_hold_changed( $hold ) if $notify_library; + _koha_notify_hold_changed($hold) if $notify_library; _FixPriority( { biblionumber => $biblionumber } ); my $item = Koha::Items->find($itemnumber); - if ( $item->location && $item->location eq 'CART' - && ( !$item->permanent_location || $item->permanent_location ne 'CART' ) ) { - CartToShelf( $itemnumber ); + if ( $item->location + && $item->location eq 'CART' + && ( !$item->permanent_location || $item->permanent_location ne 'CART' ) ) + { + CartToShelf($itemnumber); } - my $std = $dbh->prepare(q{ + my $std = $dbh->prepare( + q{ DELETE q, t FROM tmp_holdsqueue q INNER JOIN hold_fill_targets t @@ -1291,8 +1344,9 @@ sub ModReserveAffect { AND q.item_level_request = t.item_level_request AND q.holdingbranch = t.source_branchcode WHERE t.reserve_id = ? - }); - $std->execute($hold->reserve_id); + } + ); + $std->execute( $hold->reserve_id ); logaction( 'HOLDS', 'MODIFY', $hold->reserve_id, $hold ) if C4::Context->preference('HoldsLog'); @@ -1314,9 +1368,9 @@ sub ModReserveCancelAll { my ( $itemnumber, $borrowernumber, $cancellation_reason ) = @_; #step 1 : cancel the reservation - my $holds = Koha::Holds->search({ itemnumber => $itemnumber, borrowernumber => $borrowernumber }); + my $holds = Koha::Holds->search( { itemnumber => $itemnumber, borrowernumber => $borrowernumber } ); return unless $holds->count; - $holds->next->cancel({ cancellation_reason => $cancellation_reason }); + $holds->next->cancel( { cancellation_reason => $cancellation_reason } ); #step 2 launch the subroutine of the others reserves ( $messages, $nextreservinfo ) = GetOtherReserves($itemnumber); @@ -1344,8 +1398,9 @@ sub ModReserveMinusPriority { "; my $sth_upd = $dbh->prepare($query); $sth_upd->execute( $itemnumber, $reserve_id ); + # second step update all others reserves - _FixPriority({ reserve_id => $reserve_id, rank => '0' }); + _FixPriority( { reserve_id => $reserve_id, rank => '0' } ); } =head2 IsAvailableForItemLevelRequest @@ -1378,26 +1433,28 @@ checks with CanItemBeReserved or CanBookBeReserved. =cut sub IsAvailableForItemLevelRequest { - my $item = shift; - my $patron = shift; - my $pickup_branchcode = shift; + my $item = shift; + my $patron = shift; + my $pickup_branchcode = shift; my $dbh = C4::Context->dbh; + # must check the notforloan setting of the itemtype # FIXME - a lot of places in the code do this # or something similar - need to be # consolidated my $itemtype = $item->effective_itemtype; return 0 - unless defined $itemtype; + unless defined $itemtype; my $notforloan_per_itemtype = Koha::ItemTypes->find($itemtype)->notforloan; - return 0 if - $notforloan_per_itemtype || - $item->itemlost || - $item->notforloan > 0 || # item with negative or zero notforloan value is holdable - $item->withdrawn || - ($item->damaged && !C4::Context->preference('AllowHoldsOnDamagedItems')); + return 0 + if $notforloan_per_itemtype + || $item->itemlost + || $item->notforloan > 0 + || # item with negative or zero notforloan value is holdable + $item->withdrawn + || ( $item->damaged && !C4::Context->preference('AllowHoldsOnDamagedItems') ); if ($pickup_branchcode) { my $destination = Koha::Libraries->find($pickup_branchcode); @@ -1405,10 +1462,11 @@ sub IsAvailableForItemLevelRequest { return 0 unless $destination->pickup_location; return 0 unless $item->can_be_transferred( { to => $destination } ); my $reserves_control_branch = Koha::Policy::Holds->holds_control_library( $item, $patron ); - my $branchitemrule = - C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->itype ); - my $home_library = Koha::Libraries->find( {branchcode => $item->homebranch} ); - return 0 unless $branchitemrule->{hold_fulfillment_policy} ne 'holdgroup' || $home_library->validate_hold_sibling( {branchcode => $pickup_branchcode} ); + my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->itype ); + my $home_library = Koha::Libraries->find( { branchcode => $item->homebranch } ); + return 0 + unless $branchitemrule->{hold_fulfillment_policy} ne 'holdgroup' + || $home_library->validate_hold_sibling( { branchcode => $pickup_branchcode } ); } my $on_shelf_holds = Koha::CirculationRules->get_onshelfholds_policy( { item => $item, patron => $patron } ); @@ -1454,22 +1512,22 @@ sub ItemsAnyAvailableAndNotRestricted { foreach my $i (@items) { my $reserves_control_branch = Koha::Policy::Holds->holds_control_library( $i, $param->{patron} ); - my $branchitemrule = - C4::Circulation::GetBranchItemRule( $reserves_control_branch, $i->itype ); - my $item_library = Koha::Libraries->find( { branchcode => $i->homebranch } ); + my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $i->itype ); + my $item_library = Koha::Libraries->find( { branchcode => $i->homebranch } ); # we can return (end the loop) when first one found: return 1 unless $i->itemlost - || $i->notforloan # items with non-zero notforloan cannot be checked out + || $i->notforloan # items with non-zero notforloan cannot be checked out || $i->withdrawn || $i->onloan || IsItemOnHoldAndFound( $i->id ) || ( $i->damaged - && ! C4::Context->preference('AllowHoldsOnDamagedItems') ) + && !C4::Context->preference('AllowHoldsOnDamagedItems') ) || 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 } ) + || $branchitemrule->{holdallowed} eq 'from_local_hold_group' + && !$item_library->validate_hold_sibling( { branchcode => $param->{patron}->branchcode } ) || CanItemBeReserved( $param->{patron}, $i )->{status} ne 'OK'; } @@ -1488,31 +1546,30 @@ Input: $where is 'up', 'down', 'top' or 'bottom'. Biblionumber, Date reserve was sub AlterPriority { my ( $where, $reserve_id, $prev_priority, $next_priority, $first_priority, $last_priority ) = @_; - my $hold = Koha::Holds->find( $reserve_id ); + my $hold = Koha::Holds->find($reserve_id); return unless $hold; if ( $hold->cancellationdate ) { - warn "I cannot alter the priority for reserve_id $reserve_id, the reserve has been cancelled (" . $hold->cancellationdate . ')'; + warn "I cannot alter the priority for reserve_id $reserve_id, the reserve has been cancelled (" + . $hold->cancellationdate . ')'; return; } if ( $where eq 'up' ) { - return unless $prev_priority; - _FixPriority({ reserve_id => $reserve_id, rank => $prev_priority }) + return unless $prev_priority; + _FixPriority( { reserve_id => $reserve_id, rank => $prev_priority } ); } elsif ( $where eq 'down' ) { - return unless $next_priority; - _FixPriority({ reserve_id => $reserve_id, rank => $next_priority }) + return unless $next_priority; + _FixPriority( { reserve_id => $reserve_id, rank => $next_priority } ); } elsif ( $where eq 'top' ) { - _FixPriority({ reserve_id => $reserve_id, rank => $first_priority }) + _FixPriority( { reserve_id => $reserve_id, rank => $first_priority } ); } elsif ( $where eq 'bottom' ) { - _FixPriority({ reserve_id => $reserve_id, rank => $last_priority }); + _FixPriority( { reserve_id => $reserve_id, rank => $last_priority } ); } - Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue( - { - biblio_ids => [ $hold->biblionumber ] - } - ) if C4::Context->preference('RealTimeHoldsQueue'); + Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue( { biblio_ids => [ $hold->biblionumber ] } ) + if C4::Context->preference('RealTimeHoldsQueue'); + # FIXME Should return the new priority } @@ -1525,14 +1582,14 @@ This function sets the lowestPriority field to true if is false, and false if it =cut sub ToggleLowestPriority { - my ( $reserve_id ) = @_; + my ($reserve_id) = @_; my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare( "UPDATE reserves SET lowestPriority = NOT lowestPriority WHERE reserve_id = ?"); - $sth->execute( $reserve_id ); + my $sth = $dbh->prepare("UPDATE reserves SET lowestPriority = NOT lowestPriority WHERE reserve_id = ?"); + $sth->execute($reserve_id); - _FixPriority({ reserve_id => $reserve_id, rank => '999999' }); + _FixPriority( { reserve_id => $reserve_id, rank => '999999' } ); } =head2 ToggleSuspend @@ -1548,12 +1605,12 @@ be cleared when it is unsuspended. sub ToggleSuspend { my ( $reserve_id, $suspend_until ) = @_; - my $hold = Koha::Holds->find( $reserve_id ); + my $hold = Koha::Holds->find($reserve_id); if ( $hold->is_suspended ) { - $hold->resume() + $hold->resume(); } else { - $hold->suspend_hold( $suspend_until ); + $hold->suspend_hold($suspend_until); } } @@ -1579,26 +1636,24 @@ sub SuspendAll { my $borrowernumber = $params{'borrowernumber'} || undef; my $biblionumber = $params{'biblionumber'} || undef; my $suspend_until = $params{'suspend_until'} || undef; - my $suspend = defined( $params{'suspend'} ) ? $params{'suspend'} : 1; + my $suspend = defined( $params{'suspend'} ) ? $params{'suspend'} : 1; return unless ( $borrowernumber || $biblionumber ); my $params; $params->{found} = undef; $params->{borrowernumber} = $borrowernumber if $borrowernumber; - $params->{biblionumber} = $biblionumber if $biblionumber; + $params->{biblionumber} = $biblionumber if $biblionumber; my @holds = Koha::Holds->search($params)->as_list; if ($suspend) { map { $_->suspend_hold($suspend_until) } @holds; - } - else { + } else { map { $_->resume() } @holds; } } - =head2 _FixPriority _FixPriority({ @@ -1635,32 +1690,32 @@ when _FixPriority calls itself. =cut sub _FixPriority { - my ( $params ) = @_; - my $reserve_id = $params->{reserve_id}; - my $rank = $params->{rank} // ''; + my ($params) = @_; + my $reserve_id = $params->{reserve_id}; + my $rank = $params->{rank} // ''; my $ignoreSetLowestRank = $params->{ignoreSetLowestRank}; - my $biblionumber = $params->{biblionumber}; + my $biblionumber = $params->{biblionumber}; my $dbh = C4::Context->dbh; my $hold; - if ( $reserve_id ) { - $hold = Koha::Holds->find( $reserve_id ); - if (!defined $hold){ + if ($reserve_id) { + $hold = Koha::Holds->find($reserve_id); + if ( !defined $hold ) { + # may have already been checked out and hold fulfilled - $hold = Koha::Old::Holds->find( $reserve_id ); + $hold = Koha::Old::Holds->find($reserve_id); } return unless $hold; } - unless ( $biblionumber ) { # FIXME This is a very weird API + unless ($biblionumber) { # FIXME This is a very weird API $biblionumber = $hold->biblionumber; } - if ( $rank eq "del" ) { # FIXME will crash if called without $hold + if ( $rank eq "del" ) { # FIXME will crash if called without $hold $hold->cancel; - } - elsif ( $reserve_id && ( $rank eq "W" || $rank eq "0" ) ) { + } elsif ( $reserve_id && ( $rank eq "W" || $rank eq "0" ) ) { # make sure priority for waiting or in-transit items is 0 my $query = " @@ -1670,7 +1725,7 @@ sub _FixPriority { AND found IN ('W', 'T', 'P') "; my $sth = $dbh->prepare($query); - $sth->execute( $reserve_id ); + $sth->execute($reserve_id); } my @priority; @@ -1683,9 +1738,9 @@ sub _FixPriority { ORDER BY priority ASC "; my $sth = $dbh->prepare($query); - $sth->execute( $biblionumber ); + $sth->execute($biblionumber); while ( my $line = $sth->fetchrow_hashref ) { - push( @priority, $line ); + push( @priority, $line ); } # FIXME This whole sub must be rewritten, especially to highlight what is done when reserve_id is not given @@ -1701,7 +1756,7 @@ 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 ); @@ -1721,16 +1776,19 @@ sub _FixPriority { ); } - unless ( $ignoreSetLowestRank ) { - $sth = $dbh->prepare( "SELECT reserve_id FROM reserves WHERE lowestPriority = 1 AND biblionumber = ? ORDER BY priority" ); + 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'}, - rank => '999999', - ignoreSetLowestRank => 1 - }); - } + while ( my $res = $sth->fetchrow_hashref() ) { + _FixPriority( + { + reserve_id => $res->{'reserve_id'}, + rank => '999999', + ignoreSetLowestRank => 1 + } + ); + } } } @@ -1757,8 +1815,8 @@ All return values will respect any borrowernumbers passed as arrayref in $ignore =cut sub _Findgroupreserve { - my ( $biblionumber, $itemnumber, $lookahead, $ignore_borrowers) = @_; - my $dbh = C4::Context->dbh; + my ( $biblionumber, $itemnumber, $lookahead, $ignore_borrowers ) = @_; + my $dbh = C4::Context->dbh; # check for targeted match form the holds queue my $hold_target_query = qq{ @@ -1788,11 +1846,11 @@ sub _Findgroupreserve { ORDER BY priority }; my $sth = $dbh->prepare($hold_target_query); - $sth->execute($itemnumber, $lookahead||0); + $sth->execute( $itemnumber, $lookahead || 0 ); my @results; if ( my $data = $sth->fetchrow_hashref ) { push( @results, $data ) - unless any{ $data->{borrowernumber} eq $_ } @$ignore_borrowers ; + unless any { $data->{borrowernumber} eq $_ } @$ignore_borrowers; } return @results if @results; @@ -1820,11 +1878,11 @@ sub _Findgroupreserve { ORDER BY priority }; $sth = $dbh->prepare($query); - $sth->execute( $biblionumber, $itemnumber, $lookahead||0); + $sth->execute( $biblionumber, $itemnumber, $lookahead || 0 ); @results = (); while ( my $data = $sth->fetchrow_hashref ) { push( @results, $data ) - unless any{ $data->{borrowernumber} eq $_ } @$ignore_borrowers ; + unless any { $data->{borrowernumber} eq $_ } @$ignore_borrowers; } return @results; } @@ -1860,38 +1918,41 @@ The following tables are availalbe witin the notice: sub _koha_notify_reserve { my $reserve_id = shift; - my $hold = Koha::Holds->find($reserve_id); + my $hold = Koha::Holds->find($reserve_id); my $borrowernumber = $hold->borrowernumber; - my $patron = Koha::Patrons->find( $borrowernumber ); + my $patron = Koha::Patrons->find($borrowernumber); # Try to get the borrower's email address my $to_address = $patron->notice_email_address; - my $messagingprefs = C4::Members::Messaging::GetMessagingPreferences( { + my $messagingprefs = C4::Members::Messaging::GetMessagingPreferences( + { borrowernumber => $borrowernumber, - message_name => 'Hold_Filled' - } ); + message_name => 'Hold_Filled' + } + ); - my $library = Koha::Libraries->find( $hold->branchcode ); + my $library = Koha::Libraries->find( $hold->branchcode ); my $from_email_address = $library->from_email_address; my %letter_params = ( - module => 'reserves', + module => 'reserves', branchcode => $hold->branchcode, - lang => $patron->lang, - tables => { - 'branches' => $library->unblessed, - 'borrowers' => $patron->unblessed, - 'biblio' => $hold->biblionumber, - 'biblioitems' => $hold->biblionumber, - 'reserves' => $hold->unblessed, - 'items' => $hold->itemnumber, + lang => $patron->lang, + tables => { + 'branches' => $library->unblessed, + 'borrowers' => $patron->unblessed, + 'biblio' => $hold->biblionumber, + 'biblioitems' => $hold->biblionumber, + 'reserves' => $hold->unblessed, + 'items' => $hold->itemnumber, }, ); - my $notification_sent = 0; #Keeping track if a Hold_filled message is sent. If no message can be sent, then default to a print message. - my $do_not_lock = ( exists $ENV{_} && $ENV{_} =~ m|prove| ) || $ENV{KOHA_TESTING}; + my $notification_sent = + 0; #Keeping track if a Hold_filled message is sent. If no message can be sent, then default to a print message. + my $do_not_lock = ( exists $ENV{_} && $ENV{_} =~ m|prove| ) || $ENV{KOHA_TESTING}; my $send_notification = sub { my ( $mtt, $letter_code, $wants_digest ) = (@_); return unless defined $letter_code; @@ -1928,19 +1989,23 @@ sub _koha_notify_reserve { }; while ( my ( $mtt, $letter_code ) = each %{ $messagingprefs->{transports} } ) { - next if ( - ( $mtt eq 'email' and not $to_address ) # No email address - or ( $mtt eq 'sms' and not $patron->smsalertnumber ) # No SMS number - or ( $mtt eq 'itiva' and C4::Context->preference('TalkingTechItivaPhoneNotification') ) # Notice is handled by TalkingTech_itiva_outbound.pl - or ( $mtt eq 'phone' and not $patron->phone ) # No phone number to call - ); + next + if ( + ( $mtt eq 'email' and not $to_address ) # No email address + or ( $mtt eq 'sms' and not $patron->smsalertnumber ) # No SMS number + or ( $mtt eq 'itiva' + and C4::Context->preference('TalkingTechItivaPhoneNotification') + ) # Notice is handled by TalkingTech_itiva_outbound.pl + or ( $mtt eq 'phone' and not $patron->phone ) # No phone number to call + ); - &$send_notification($mtt, $letter_code, $messagingprefs->{wants_digest}); + &$send_notification( $mtt, $letter_code, $messagingprefs->{wants_digest} ); $notification_sent++; } + #Making sure that a print notification is sent if no other transport types can be utilized. - if (! $notification_sent) { - &$send_notification('print', 'HOLD'); + if ( !$notification_sent ) { + &$send_notification( 'print', 'HOLD' ); } } @@ -1954,14 +2019,14 @@ sub _koha_notify_reserve { sub _koha_notify_hold_changed { my $hold = shift; - my $patron = $hold->patron; + 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 ) }, + substitute => { today => output_pref(dt_from_string) }, tables => { 'branches' => $library->unblessed, 'borrowers' => $patron->unblessed, @@ -1974,9 +2039,8 @@ sub _koha_notify_hold_changed { return unless $letter; - my $email = - C4::Context->preference('ExpireReservesAutoFillEmail') - || $library->inbound_email_address; + my $email = C4::Context->preference('ExpireReservesAutoFillEmail') + || $library->inbound_email_address; C4::Letters::EnqueueLetter( { @@ -2010,13 +2074,14 @@ the sub accounts for that too. sub _ShiftPriority { my ( $biblio, $new_priority ) = @_; - my $dbh = C4::Context->dbh; + my $dbh = C4::Context->dbh; my $query = "SELECT priority FROM reserves WHERE biblionumber = ? AND priority > ? ORDER BY priority ASC LIMIT 1"; - my $sth = $dbh->prepare( $query ); + my $sth = $dbh->prepare($query); $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 ); + $new_priority = $min_priority if ($min_priority); # Shift the priority up by one; works in conjunction with the next SQL statement $query = "UPDATE reserves @@ -2025,17 +2090,18 @@ sub _ShiftPriority { AND borrowernumber = ? AND reservedate = ? AND found IS NULL"; - my $sth_update = $dbh->prepare( $query ); + my $sth_update = $dbh->prepare($query); # Select all reserves for the biblio with priority greater than $new_priority, and order greatest to least - $query = "SELECT borrowernumber, reservedate FROM reserves WHERE priority >= ? AND biblionumber = ? ORDER BY priority DESC"; - $sth = $dbh->prepare( $query ); + $query = + "SELECT borrowernumber, reservedate FROM reserves WHERE priority >= ? AND biblionumber = ? ORDER BY priority DESC"; + $sth = $dbh->prepare($query); $sth->execute( $new_priority, $biblio ); while ( my $row = $sth->fetchrow_hashref ) { $sth_update->execute( $biblio, $row->{borrowernumber}, $row->{reservedate} ); } - return $new_priority; # so the caller knows what priority they wind up receiving + return $new_priority; # so the caller knows what priority they wind up receiving } =head2 MoveReserve @@ -2052,38 +2118,39 @@ sub MoveReserve { $cancelreserve //= 0; - my $lookahead = C4::Context->preference('ConfirmFutureHolds'); #number of days to look for future holds - my $item = Koha::Items->find($itemnumber); + my $lookahead = C4::Context->preference('ConfirmFutureHolds'); #number of days to look for future holds + my $item = Koha::Items->find($itemnumber); my ( $restype, $res, undef ) = CheckReserves( $item, $lookahead ); return unless $res; my $biblionumber = $res->{biblionumber}; - if ($res->{borrowernumber} == $borrowernumber) { + if ( $res->{borrowernumber} == $borrowernumber ) { my $hold = Koha::Holds->find( $res->{reserve_id} ); - $hold->fill({ item_id => $itemnumber }); - } - else { + $hold->fill( { item_id => $itemnumber } ); + } else { + # warn "Reserved"; # The item is reserved by someone else. # Find this item in the reserves - my $borr_res = Koha::Holds->search({ - borrowernumber => $borrowernumber, - biblionumber => $biblionumber, - },{ - order_by => 'priority' - })->next(); + my $borr_res = Koha::Holds->search( + { + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + }, + { order_by => 'priority' } + )->next(); + + if ($borr_res) { - if ( $borr_res ) { # The item is reserved by the current patron - $borr_res->fill({ item_id => $itemnumber }); + $borr_res->fill( { item_id => $itemnumber } ); } - if ( $cancelreserve eq 'revert' ) { ## Revert waiting reserve to priority 1 - RevertWaitingStatus({ itemnumber => $itemnumber }); - } - elsif ( $cancelreserve eq 'cancel' || $cancelreserve ) { # cancel reserves on this item + if ( $cancelreserve eq 'revert' ) { ## Revert waiting reserve to priority 1 + RevertWaitingStatus( { itemnumber => $itemnumber } ); + } elsif ( $cancelreserve eq 'cancel' || $cancelreserve ) { # cancel reserves on this item my $hold = Koha::Holds->find( $res->{reserve_id} ); $hold->cancel; } @@ -2100,28 +2167,25 @@ This shifts the holds from C<$from_biblio> to C<$to_biblio> and reorders them by sub MergeHolds { my ( $dbh, $to_biblio, $from_biblio ) = @_; - my $sth = $dbh->prepare( - "SELECT count(*) as reserve_count FROM reserves WHERE biblionumber = ?" - ); + my $sth = $dbh->prepare("SELECT count(*) as reserve_count FROM reserves WHERE biblionumber = ?"); $sth->execute($from_biblio); if ( my $data = $sth->fetchrow_hashref() ) { # holds exist on old record, if not we don't need to do anything - $sth = $dbh->prepare( - "UPDATE reserves SET biblionumber = ? WHERE biblionumber = ?"); + $sth = $dbh->prepare("UPDATE reserves SET biblionumber = ? WHERE biblionumber = ?"); $sth->execute( $to_biblio, $from_biblio ); # Reorder by date # don't reorder those already waiting $sth = $dbh->prepare( -"SELECT * FROM reserves WHERE biblionumber = ? AND (found NOT IN ('W', 'T', 'P') OR found is NULL) ORDER BY reservedate ASC" + "SELECT * FROM reserves WHERE biblionumber = ? AND (found NOT IN ('W', 'T', 'P') OR found is NULL) ORDER BY reservedate ASC" ); my $upd_sth = $dbh->prepare( -"UPDATE reserves SET priority = ? WHERE biblionumber = ? AND borrowernumber = ? + "UPDATE reserves SET priority = ? WHERE biblionumber = ? AND borrowernumber = ? AND reservedate = ? AND (itemnumber = ? or itemnumber is NULL) " ); - $sth->execute( $to_biblio ); + $sth->execute($to_biblio); my $priority = 1; while ( my $reserve = $sth->fetchrow_hashref() ) { $upd_sth->execute( @@ -2149,10 +2213,10 @@ sub MergeHolds { =cut sub RevertWaitingStatus { - my ( $params ) = @_; + my ($params) = @_; my $itemnumber = $params->{'itemnumber'}; - return unless ( $itemnumber ); + return unless ($itemnumber); my $dbh = C4::Context->dbh; @@ -2160,23 +2224,23 @@ sub RevertWaitingStatus { my $hold = Koha::Holds->search( { itemnumber => $itemnumber, - found => { not => undef }, + found => { not => undef }, } )->next; ## Increment the priority of all other non-waiting ## reserves for this bib record - my $holds = Koha::Holds->search({ biblionumber => $hold->biblionumber, priority => { '>' => 0 } }) - ->update({ priority => \'priority + 1' }, { no_triggers => 1 }); + my $holds = Koha::Holds->search( { biblionumber => $hold->biblionumber, priority => { '>' => 0 } } ) + ->update( { priority => \'priority + 1' }, { no_triggers => 1 } ); ## Fix up the currently waiting reserve $hold->set( { - priority => 1, - found => undef, - waitingdate => undef, + priority => 1, + found => undef, + waitingdate => undef, expirationdate => $hold->patron_expiration_date, - itemnumber => $hold->item_level_hold ? $hold->itemnumber : undef, + itemnumber => $hold->item_level_hold ? $hold->itemnumber : undef, } )->store(); @@ -2185,12 +2249,8 @@ sub RevertWaitingStatus { _FixPriority( { biblionumber => $hold->biblionumber } ); - Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue( - { - biblio_ids => [ $hold->biblionumber ] - } - ) if C4::Context->preference('RealTimeHoldsQueue'); - + Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue->new->enqueue( { biblio_ids => [ $hold->biblionumber ] } ) + if C4::Context->preference('RealTimeHoldsQueue'); return $hold; } @@ -2222,23 +2282,23 @@ available within the slip: =cut sub ReserveSlip { - my ($args) = @_; - my $branchcode = $args->{branchcode}; + my ($args) = @_; + my $branchcode = $args->{branchcode}; my $reserve_id = $args->{reserve_id}; my $itemnumber = $args->{itemnumber}; my $hold = Koha::Holds->find($reserve_id); return unless $hold; - my $patron = $hold->borrower; + my $patron = $hold->borrower; my $reserve = $hold->unblessed; - return C4::Letters::GetPreparedLetter ( - module => 'circulation', + return C4::Letters::GetPreparedLetter( + module => 'circulation', letter_code => 'HOLD_SLIP', - branchcode => $branchcode, - lang => $patron->lang, - tables => { + branchcode => $branchcode, + lang => $patron->lang, + tables => { 'reserves' => $reserve, 'branches' => $reserve->{branchcode}, 'borrowers' => $reserve->{borrowernumber}, @@ -2277,21 +2337,21 @@ sub CalculatePriority { AND priority > 0 AND (found IS NULL OR found = '') }; + #skip found==W or found==T or found==P (waiting, transit or processing holds) - if( $resdate ) { - $sql.= ' AND ( reservedate <= ? )'; - } - else { - $sql.= ' AND ( reservedate < NOW() )'; + if ($resdate) { + $sql .= ' AND ( reservedate <= ? )'; + } else { + $sql .= ' AND ( reservedate < NOW() )'; } my $dbh = C4::Context->dbh(); my @row = $dbh->selectrow_array( $sql, undef, - $resdate ? ($biblionumber, $resdate) : ($biblionumber) + $resdate ? ( $biblionumber, $resdate ) : ($biblionumber) ); - return @row ? $row[0]+1 : 1; + return @row ? $row[0] + 1 : 1; } =head2 IsItemOnHoldAndFound @@ -2334,7 +2394,7 @@ sub GetMaxPatronHoldsForRecord { my ( $borrowernumber, $biblionumber ) = @_; my $patron = Koha::Patrons->find($borrowernumber); - my @items = Koha::Items->search( { biblionumber => $biblionumber } )->as_list; + my @items = Koha::Items->search( { biblionumber => $biblionumber } )->as_list; my $controlbranch = C4::Context->preference('ReservesControlBranch'); @@ -2348,12 +2408,14 @@ sub GetMaxPatronHoldsForRecord { $branchcode = $item->homebranch if ( $controlbranch eq "ItemHomeLibrary" ); - my $rule = Koha::CirculationRules->get_effective_rule({ - categorycode => $categorycode, - itemtype => $itemtype, - branchcode => $branchcode, - rule_name => 'holds_per_record' - }); + my $rule = Koha::CirculationRules->get_effective_rule( + { + categorycode => $categorycode, + itemtype => $itemtype, + branchcode => $branchcode, + rule_name => 'holds_per_record' + } + ); my $holds_per_record = $rule ? $rule->rule_value : 0; $max = $holds_per_record if $holds_per_record > $max; } diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index f340a1441ae..4423b83df8e 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -1,6 +1,5 @@ #!/usr/bin/perl - # Copyright Katipo Communications 2002 # Copyright Koha Development team 2012 # @@ -21,13 +20,13 @@ use Modern::Perl; -use CGI qw ( -utf8 ); -use C4::Auth qw( get_template_and_user ); -use C4::Koha qw( getitemtypeimagelocation getitemtypeimagesrc ); +use CGI qw ( -utf8 ); +use C4::Auth qw( get_template_and_user ); +use C4::Koha qw( getitemtypeimagelocation getitemtypeimagesrc ); use C4::Circulation qw( GetBranchItemRule ); -use C4::Reserves qw( CanItemBeReserved CanBookBeReserved AddReserve IsAvailableForItemLevelRequest GetReserveFee ); -use C4::Biblio qw( GetBiblioData GetFrameworkCode ); -use C4::Output qw( output_html_with_http_headers ); +use C4::Reserves qw( CanItemBeReserved CanBookBeReserved AddReserve IsAvailableForItemLevelRequest GetReserveFee ); +use C4::Biblio qw( GetBiblioData GetFrameworkCode ); +use C4::Output qw( output_html_with_http_headers ); use C4::Context; use C4::Members; use C4::Overdues; @@ -48,26 +47,26 @@ my $query = CGI->new; my $op = $query->param('op') // q{}; # if OPACHoldRequests (for placing holds) is disabled, leave immediately -if ( ! C4::Context->preference('OPACHoldRequests') ) { +if ( !C4::Context->preference('OPACHoldRequests') ) { print $query->redirect("/cgi-bin/koha/errors/404.pl"); exit; } my ( $template, $borrowernumber, $cookie ) = get_template_and_user( { - template_name => "opac-reserve.tt", - query => $query, - type => "opac", + template_name => "opac-reserve.tt", + query => $query, + type => "opac", } ); -my $patron = Koha::Patrons->find( $borrowernumber, { prefetch => ['categorycode'] } ); +my $patron = Koha::Patrons->find( $borrowernumber, { prefetch => ['categorycode'] } ); my $category = $patron->category; my $can_place_hold_if_available_at_pickup = C4::Context->preference('OPACHoldsIfAvailableAtPickup'); -unless ( $can_place_hold_if_available_at_pickup ) { +unless ($can_place_hold_if_available_at_pickup) { my @patron_categories = split ',', C4::Context->preference('OPACHoldsIfAvailableAtPickupExceptions'); - if ( @patron_categories ) { + if (@patron_categories) { my $categorycode = $patron->categorycode; $can_place_hold_if_available_at_pickup = grep { $_ eq $categorycode } @patron_categories; } @@ -78,12 +77,12 @@ my $itemtypes = { map { $_->{itemtype} => $_ } @{ Koha::ItemTypes->search_with_l # There are two ways of calling this script, with a single biblio num # or multiple biblio nums. my $biblionumbers = $query->param('biblionumbers'); -my $reserveMode = $query->param('reserve_mode'); -if ($reserveMode && ($reserveMode eq 'single')) { +my $reserveMode = $query->param('reserve_mode'); +if ( $reserveMode && ( $reserveMode eq 'single' ) ) { my $bib = $query->param('single_bib'); $biblionumbers = "$bib/"; } -if (! $biblionumbers) { +if ( !$biblionumbers ) { $biblionumbers = $query->param('biblionumber'); } @@ -100,8 +99,9 @@ $template->param( biblionumbers => $biblionumbers ); # Each biblio number is suffixed with '/', e.g. "1/2/3/" my @biblionumbers = split /\//, $biblionumbers; if ( $#biblionumbers < 0 && $op ne 'cud-place_reserve' ) { + # TODO: New message? - $template->param(message=>1, no_biblionumber=>1); + $template->param( message => 1, no_biblionumber => 1 ); output_html_with_http_headers $query, $cookie, $template->output, undef, { force_no_caching => 1 }; exit; } @@ -111,25 +111,26 @@ if ( $#biblionumbers < 0 && $op ne 'cud-place_reserve' ) { # Here we check that the borrower can actually make reserves Stage 1. # # -my $noreserves = 0; +my $noreserves = 0; if ( $category->effective_BlockExpiredPatronOpacActions ) { if ( $patron->is_expired ) { + # cannot reserve, their card has expired and the rules set mean this is not allowed $noreserves = 1; $template->param( message => 1, expired_patron => 1 ); } } -my $maxoutstanding = C4::Context->preference("maxoutstanding"); +my $maxoutstanding = C4::Context->preference("maxoutstanding"); my $amountoutstanding = $patron->account->balance; -if ( $amountoutstanding && ($amountoutstanding > $maxoutstanding) ) { +if ( $amountoutstanding && ( $amountoutstanding > $maxoutstanding ) ) { my $amount = sprintf "%.02f", $amountoutstanding; $template->param( message => 1 ); $noreserves = 1; $template->param( too_much_oweing => $amount ); } -if ( $patron->gonenoaddress && ($patron->gonenoaddress == 1) ) { +if ( $patron->gonenoaddress && ( $patron->gonenoaddress == 1 ) ) { $noreserves = 1; $template->param( message => 1, @@ -137,7 +138,7 @@ if ( $patron->gonenoaddress && ($patron->gonenoaddress == 1) ) { ); } -if ( $patron->lost && ($patron->lost == 1) ) { +if ( $patron->lost && ( $patron->lost == 1 ) ) { $noreserves = 1; $template->param( message => 1, @@ -155,7 +156,7 @@ if ( $patron->is_debarred ) { ); } -my $holds = $patron->holds; +my $holds = $patron->holds; my $reserves_count = $holds->count; $template->param( RESERVES => $holds->unblessed ); if ( $maxreserves && ( $reserves_count >= $maxreserves ) ) { @@ -164,14 +165,13 @@ if ( $maxreserves && ( $reserves_count >= $maxreserves ) ) { $template->param( too_many_reserves => $holds->count ); } -if( $noreserves ){ +if ($noreserves) { output_html_with_http_headers $query, $cookie, $template->output, undef, { force_no_caching => 1 }; exit; } - # pass the pickup branch along.... -my $branch = $query->param('branch') || $patron->branchcode || C4::Context->userenv->{branch} || '' ; +my $branch = $query->param('branch') || $patron->branchcode || C4::Context->userenv->{branch} || ''; $template->param( branch => $branch ); # @@ -189,12 +189,13 @@ if ( $op eq 'cud-place_reserve' ) { # List is composed of alternating biblio/item/branch my $selectedItems = $query->param('selecteditems'); - if ($query->param('reserve_mode') eq 'single') { + if ( $query->param('reserve_mode') eq 'single' ) { + # This indicates non-JavaScript mode, so there was # only a single biblio number selected. - my $bib = $query->param('single_bib'); + my $bib = $query->param('single_bib'); my $item = $query->param("checkitem_$bib"); - if ($item eq 'any') { + if ( $item eq 'any' ) { $item = ''; } my $branch = $query->param('branch'); @@ -207,8 +208,8 @@ if ( $op eq 'cud-place_reserve' ) { # Make sure there is a biblionum/itemnum/branch triplet for each item. # The itemnum can be 'any', meaning next available. my $selectionCount = @selectedItems; - if (($selectionCount == 0) || (($selectionCount % 3) != 0)) { - $template->param(message=>1, bad_data=>1); + if ( ( $selectionCount == 0 ) || ( ( $selectionCount % 3 ) != 0 ) ) { + $template->param( message => 1, bad_data => 1 ); output_html_with_http_headers $query, $cookie, $template->output, undef, { force_no_caching => 1 }; exit; } @@ -222,22 +223,23 @@ if ( $op eq 'cud-place_reserve' ) { my $canreserve = 0; my $singleBranchMode = Koha::Libraries->search->count == 1; - if ( $singleBranchMode || ! C4::Context->preference("OPACAllowUserToChooseBranch") ) - { # single branch mode or disabled user choosing + if ( $singleBranchMode || !C4::Context->preference("OPACAllowUserToChooseBranch") ) + { # single branch mode or disabled user choosing $branch = $patron->branchcode; } # FIXME We shouldn't need to fetch the item here - my $item = $itemNum ? Koha::Items->find( $itemNum ) : undef; + my $item = $itemNum ? Koha::Items->find($itemNum) : undef; + # When choosing a specific item, the default pickup library should be dictated by the default hold policy - if ( ! C4::Context->preference("OPACAllowUserToChooseBranch") && $item ) { + if ( !C4::Context->preference("OPACAllowUserToChooseBranch") && $item ) { my $type = $item->effective_itemtype; my $reserves_control_branch = Koha::Policy::Holds->holds_control_library( $item, $patron ); my $rule = GetBranchItemRule( $reserves_control_branch, $type ); if ( $rule->{hold_fulfillment_policy} eq 'any' || $rule->{hold_fulfillment_policy} eq 'patrongroup' ) { $branch = $patron->branchcode; - } elsif ( $rule->{hold_fulfillment_policy} eq 'holdgroup' ){ + } elsif ( $rule->{hold_fulfillment_policy} eq 'holdgroup' ) { $branch = $item->homebranch; } else { my $policy = $rule->{hold_fulfillment_policy}; @@ -246,7 +248,7 @@ if ( $op eq 'cud-place_reserve' ) { } # if we have an item, we are placing the hold on the item's bib, in case of analytics - if ( $item ) { + if ($item) { $biblioNum = $item->biblionumber; } @@ -264,19 +266,18 @@ if ( $op eq 'cud-place_reserve' ) { $itemtype = undef if $itemNum; my $biblio = Koha::Biblios->find($biblioNum); - my $rank = $biblio->holds->search( { found => [ { "!=" => "W" }, undef ] } )->count + 1; - if ( $item ) { + my $rank = $biblio->holds->search( { found => [ { "!=" => "W" }, undef ] } )->count + 1; + if ($item) { my $status = CanItemBeReserved( $patron, $item, $branch, { get_from_cache => 1 } )->{status}; - if( $status eq 'OK' ){ + if ( $status eq 'OK' ) { $canreserve = 1; } else { push @failed_holds, $status; } - } - else { + } else { my $status = CanBookBeReserved( $borrowernumber, $biblioNum, $branch, { itemtype => $itemtype } )->{status}; - if( $status eq 'OK'){ + if ( $status eq 'OK' ) { $canreserve = 1; } else { push @failed_holds, $status; @@ -285,7 +286,7 @@ if ( $op eq 'cud-place_reserve' ) { # Inserts a null into the 'itemnumber' field of 'reserves' table. $itemNum = undef; } - my $notes = $query->param('notes_'.$biblioNum)||''; + my $notes = $query->param( 'notes_' . $biblioNum ) || ''; my $item_group_id = $query->param("item_group_id_$biblioNum") || undef; if ( $maxreserves @@ -295,10 +296,13 @@ if ( $op eq 'cud-place_reserve' ) { $canreserve = 0; } - unless ( $can_place_hold_if_available_at_pickup ) { - my $items_in_this_library = Koha::Items->search({ biblionumber => $biblioNum, holdingbranch => $branch }); - my $nb_of_items_issued = $items_in_this_library->search({ 'issue.itemnumber' => { not => undef }}, { join => 'issue' })->count; - my $nb_of_items_unavailable = $items_in_this_library->search({ -or => { lost => { '!=' => 0 }, damaged => { '!=' => 0 }, } }); + unless ($can_place_hold_if_available_at_pickup) { + my $items_in_this_library = Koha::Items->search( { biblionumber => $biblioNum, holdingbranch => $branch } ); + my $nb_of_items_issued = + $items_in_this_library->search( { 'issue.itemnumber' => { not => undef } }, { join => 'issue' } ) + ->count; + my $nb_of_items_unavailable = + $items_in_this_library->search( { -or => { lost => { '!=' => 0 }, damaged => { '!=' => 0 }, } } ); if ( $items_in_this_library->count > $nb_of_items_issued + $nb_of_items_unavailable ) { $canreserve = 0; push @failed_holds, 'items_available'; @@ -323,7 +327,7 @@ if ( $op eq 'cud-place_reserve' ) { item_group_id => $item_group_id, } ); - if( $reserve_id ){ + if ($reserve_id) { ++$reserve_cnt; } else { push @failed_holds, 'not_placed'; @@ -331,7 +335,9 @@ if ( $op eq 'cud-place_reserve' ) { } } - print $query->redirect("/cgi-bin/koha/opac-user.pl?" . ( @failed_holds ? "failed_holds=" . join('|',@failed_holds) : q|| ) . "&opac-user-holds=1"); + print $query->redirect( "/cgi-bin/koha/opac-user.pl?" + . ( @failed_holds ? "failed_holds=" . join( '|', @failed_holds ) : q|| ) + . "&opac-user-holds=1" ); exit; } @@ -341,45 +347,39 @@ if ( $op eq 'cud-place_reserve' ) { # # -my %biblioDataHash; # Hash of biblionumber to biblio/biblioitems record. +my %biblioDataHash; # Hash of biblionumber to biblio/biblioitems record. foreach my $biblioNumber (@biblionumbers) { my $biblioData = GetBiblioData($biblioNumber); $biblioDataHash{$biblioNumber} = $biblioData; - my $biblio = Koha::Biblios->find( $biblioNumber ); + my $biblio = Koha::Biblios->find($biblioNumber); next unless $biblio; my $items = Koha::Items->search_ordered( [ 'me.biblionumber' => $biblioNumber, - 'me.itemnumber' => { - -in => [ - $biblio->host_items->get_column('itemnumber') - ] - } + 'me.itemnumber' => { -in => [ $biblio->host_items->get_column('itemnumber') ] } ], { prefetch => [ 'issue', 'homebranch', 'holdingbranch' ] } - )->filter_by_visible_in_opac({ patron => $patron }); + )->filter_by_visible_in_opac( { patron => $patron } ); - $biblioData->{items} = [$items->as_list]; # FIXME Potentially a lot in memory here! + $biblioData->{items} = [ $items->as_list ]; # FIXME Potentially a lot in memory here! # Compute the priority rank. $biblioData->{object} = $biblio; - my $reservecount = $biblio->holds->search({ found => [ {"!=" => "W"},undef] })->count; + my $reservecount = $biblio->holds->search( { found => [ { "!=" => "W" }, undef ] } )->count; $biblioData->{reservecount} = $reservecount; - $biblioData->{rank} = $reservecount + 1; + $biblioData->{rank} = $reservecount + 1; } - -my $requested_reserves_count = scalar( @biblionumbers ); +my $requested_reserves_count = scalar(@biblionumbers); if ( $maxreserves && ( $reserves_count + $requested_reserves_count > $maxreserves ) ) { $template->param( new_reserves_allowed => $maxreserves - $reserves_count ); } $template->param( select_item_types => 1 ); - # # # Build the template parameters that will show the info @@ -387,14 +387,14 @@ $template->param( select_item_types => 1 ); # # -my $biblioLoop = []; -my $numBibsAvailable = 0; +my $biblioLoop = []; +my $numBibsAvailable = 0; my $itemdata_enumchron = 0; -my $itemdata_ccode = 0; -my $anyholdable = 0; -my $itemLevelTypes = C4::Context->preference('item-level_itypes'); -my $pickup_locations = Koha::Libraries->search({ pickup_location => 1 }); -$template->param('item_level_itypes' => $itemLevelTypes); +my $itemdata_ccode = 0; +my $anyholdable = 0; +my $itemLevelTypes = C4::Context->preference('item-level_itypes'); +my $pickup_locations = Koha::Libraries->search( { pickup_location => 1 } ); +$template->param( 'item_level_itypes' => $itemLevelTypes ); my $patron_unblessed = $patron->unblessed; foreach my $biblioNum (@biblionumbers) { @@ -404,56 +404,57 @@ foreach my $biblioNum (@biblionumbers) { # Get relevant biblio data. my $biblioData = $biblioDataHash{$biblioNum}; - if (! $biblioData) { - $template->param(message=>1, bad_biblionumber=>$biblioNum); + if ( !$biblioData ) { + $template->param( message => 1, bad_biblionumber => $biblioNum ); output_html_with_http_headers $query, $cookie, $template->output, undef, { force_no_caching => 1 }; exit; } my @not_available_at = (); - my $biblio = $biblioData->{object}; + my $biblio = $biblioData->{object}; foreach my $library ( $pickup_locations->as_list ) { - push( @not_available_at, $library->branchcode ) unless $biblio->can_be_transferred({ to => $library }); + push( @not_available_at, $library->branchcode ) unless $biblio->can_be_transferred( { to => $library } ); } my $frameworkcode = GetFrameworkCode( $biblioData->{biblionumber} ); - $biblioLoopIter{biblionumber} = $biblioData->{biblionumber}; - $biblioLoopIter{title} = $biblioData->{title}; - $biblioLoopIter{subtitle} = $biblioData->{'subtitle'}; - $biblioLoopIter{medium} = $biblioData->{medium}; - $biblioLoopIter{part_number} = $biblioData->{part_number}; - $biblioLoopIter{part_name} = $biblioData->{part_name}; - $biblioLoopIter{author} = $biblioData->{author}; - $biblioLoopIter{rank} = $biblioData->{rank}; - $biblioLoopIter{reservecount} = $biblioData->{reservecount}; + $biblioLoopIter{biblionumber} = $biblioData->{biblionumber}; + $biblioLoopIter{title} = $biblioData->{title}; + $biblioLoopIter{subtitle} = $biblioData->{'subtitle'}; + $biblioLoopIter{medium} = $biblioData->{medium}; + $biblioLoopIter{part_number} = $biblioData->{part_number}; + $biblioLoopIter{part_name} = $biblioData->{part_name}; + $biblioLoopIter{author} = $biblioData->{author}; + $biblioLoopIter{rank} = $biblioData->{rank}; + $biblioLoopIter{reservecount} = $biblioData->{reservecount}; $biblioLoopIter{already_reserved} = $biblioData->{already_reserved}; - $biblioLoopIter{object} = $biblio; + $biblioLoopIter{object} = $biblio; - if (!$itemLevelTypes && $biblioData->{itemtype}) { - $biblioLoopIter{translated_description} = $itemtypes->{$biblioData->{itemtype}}{translated_description}; - $biblioLoopIter{imageurl} = getitemtypeimagesrc() . "/". $itemtypes->{$biblioData->{itemtype}}{imageurl}; + if ( !$itemLevelTypes && $biblioData->{itemtype} ) { + $biblioLoopIter{translated_description} = $itemtypes->{ $biblioData->{itemtype} }{translated_description}; + $biblioLoopIter{imageurl} = getitemtypeimagesrc() . "/" . $itemtypes->{ $biblioData->{itemtype} }{imageurl}; } - - $biblioLoopIter{itemLoop} = []; - my $numCopiesAvailable = 0; + my $numCopiesAvailable = 0; my $numCopiesOPACAvailable = 0; + # iterating through all items first to check if any of them available # to pass this value further inside down to IsAvailableForItemLevelRequest to # it's complicated logic to analyse. # (before this loop was inside that sub loop so it was O(n^2) ) - foreach my $item (@{$biblioData->{items}}) { + foreach my $item ( @{ $biblioData->{items} } ) { my $item_info = $item->unblessed; $item_info->{holding_branch} = $item->holding_branch; $item_info->{home_branch} = $item->home_branch; if ($itemLevelTypes) { my $itemtype = $item->itemtype; - $item_info->{'imageurl'} = getitemtypeimagelocation( 'opac', - $itemtypes->{ $itemtype->itemtype }->{'imageurl'} ); + $item_info->{'imageurl'} = getitemtypeimagelocation( + 'opac', + $itemtypes->{ $itemtype->itemtype }->{'imageurl'} + ); $item_info->{'translated_description'} = - $itemtypes->{ $itemtype->itemtype }->{translated_description}; + $itemtypes->{ $itemtype->itemtype }->{translated_description}; } # checking for holds @@ -469,8 +470,8 @@ foreach my $biblioNum (@biblionumbers) { if ( $transfer && $transfer->in_transit ) { $item_info->{transfertwhen} = $transfer->datesent; $item_info->{transfertfrom} = $transfer->frombranch; - $item_info->{transfertto} = $transfer->tobranch; - $item_info->{nocancel} = 1; + $item_info->{transfertto} = $transfer->tobranch; + $item_info->{nocancel} = 1; } # if the items belongs to a host record, show link to host record @@ -483,14 +484,14 @@ foreach my $biblioNum (@biblionumbers) { my $item_status = CanItemBeReserved( $patron, $item, undef, { get_from_cache => 1 } )->{status}; - if ($item_status eq 'recordHoldsOnly') { - $biblioLoopIter{force_hold} = 1; - $biblioLoopIter{itemholdable} = 0; + if ( $item_status eq 'recordHoldsOnly' ) { + $biblioLoopIter{force_hold} = 1; + $biblioLoopIter{itemholdable} = 0; $biblioLoopIter{forced_hold_level} = 'record'; } - my $policy_holdallowed = IsAvailableForItemLevelRequest($item, $patron, undef) && - ($item_status eq 'OK' or $item_status eq 'recordHoldsOnly'); + my $policy_holdallowed = IsAvailableForItemLevelRequest( $item, $patron, undef ) + && ( $item_status eq 'OK' or $item_status eq 'recordHoldsOnly' ); if ($policy_holdallowed) { $numCopiesAvailable++; @@ -498,9 +499,12 @@ foreach my $biblioNum (@biblionumbers) { $numCopiesOPACAvailable++; $item_info->{available} = 1; } - unless ( $can_place_hold_if_available_at_pickup ) { - my $items_in_this_library = Koha::Items->search({ biblionumber => $item->biblionumber, holdingbranch => $item->holdingbranch, notforloan => 0 }); - my $nb_of_items_issued = $items_in_this_library->search({ 'issue.itemnumber' => { not => undef }}, { join => 'issue' })->count; + unless ($can_place_hold_if_available_at_pickup) { + my $items_in_this_library = Koha::Items->search( + { biblionumber => $item->biblionumber, holdingbranch => $item->holdingbranch, notforloan => 0 } ); + my $nb_of_items_issued = + $items_in_this_library->search( { 'issue.itemnumber' => { not => undef } }, { join => 'issue' } ) + ->count; if ( $items_in_this_library->count > $nb_of_items_issued ) { push @not_available_at, $item->holdingbranch; } @@ -508,39 +512,41 @@ foreach my $biblioNum (@biblionumbers) { } # Show serial enumeration when needed - if ($item_info->{enumchron}) { + if ( $item_info->{enumchron} ) { $itemdata_enumchron = 1; } + # Show collection when needed - if ($item_info->{ccode}) { + if ( $item_info->{ccode} ) { $itemdata_ccode = 1; } - push @{$biblioLoopIter{itemLoop}}, $item_info; + push @{ $biblioLoopIter{itemLoop} }, $item_info; } $template->param( itemdata_enumchron => $itemdata_enumchron, itemdata_ccode => $itemdata_ccode, ); - if ($numCopiesAvailable > 0) { + if ( $numCopiesAvailable > 0 ) { $numBibsAvailable++; $biblioLoopIter{bib_available} = 1; - $biblioLoopIter{holdable} = 1; - $biblioLoopIter{itemholdable} = 1 if $numCopiesOPACAvailable; + $biblioLoopIter{holdable} = 1; + $biblioLoopIter{itemholdable} = 1 if $numCopiesOPACAvailable; } - if ($biblioLoopIter{already_reserved}) { - $biblioLoopIter{holdable} = undef; + if ( $biblioLoopIter{already_reserved} ) { + $biblioLoopIter{holdable} = undef; $biblioLoopIter{itemholdable} = undef; } if ( $biblioLoopIter{holdable} ) { @not_available_at = uniq @not_available_at; - $biblioLoopIter{not_available_at} = \@not_available_at ; + $biblioLoopIter{not_available_at} = \@not_available_at; } - unless ( $can_place_hold_if_available_at_pickup ) { + unless ($can_place_hold_if_available_at_pickup) { @not_available_at = uniq @not_available_at; - $biblioLoopIter{not_available_at} = \@not_available_at ; + $biblioLoopIter{not_available_at} = \@not_available_at; + # The record is not holdable is not available at any of the libraries if ( Koha::Libraries->search->count == @not_available_at ) { $biblioLoopIter{holdable} = 0; @@ -548,28 +554,29 @@ foreach my $biblioNum (@biblionumbers) { } my $record_status = CanBookBeReserved( $borrowernumber, $biblioNum )->{status}; - if ($record_status eq 'recordHoldNotAllowed') { - $biblioLoopIter{force_hold} = 1; - $biblioLoopIter{itemholdable} = 1; + if ( $record_status eq 'recordHoldNotAllowed' ) { + $biblioLoopIter{force_hold} = 1; + $biblioLoopIter{itemholdable} = 1; $biblioLoopIter{forced_hold_level} = 'item'; $biblioLoopIter{holdable} &&= 1; - } - else { + } else { $biblioLoopIter{holdable} &&= $record_status eq 'OK'; } if ( $biblioLoopIter{holdable} and C4::Context->preference('AllowHoldItemTypeSelection') ) { + # build the allowed item types loop my $rs = $biblio->items->search_ordered( undef, - { select => [ { distinct => 'itype' } ], + { + select => [ { distinct => 'itype' } ], as => 'item_type' } ); my @item_types = - grep { CanBookBeReserved( $borrowernumber, $biblioNum, $branch, { itemtype => $_ } )->{status} eq 'OK' } - $rs->get_column('item_type'); + grep { CanBookBeReserved( $borrowernumber, $biblioNum, $branch, { itemtype => $_ } )->{status} eq 'OK' } + $rs->get_column('item_type'); $biblioLoopIter{allowed_item_types} = \@item_types; } @@ -591,10 +598,10 @@ foreach my $biblioNum (@biblionumbers) { } )->forced_hold_level(); if ($forced_hold_level) { - $biblioLoopIter{force_hold} = 1 if $forced_hold_level eq 'item'; - $biblioLoopIter{force_hold} = 0 if $forced_hold_level eq 'item_group'; - $biblioLoopIter{itemholdable} = 0 if $forced_hold_level eq 'record'; - $biblioLoopIter{itemholdable} = 0 if $forced_hold_level eq 'item_group'; + $biblioLoopIter{force_hold} = 1 if $forced_hold_level eq 'item'; + $biblioLoopIter{force_hold} = 0 if $forced_hold_level eq 'item_group'; + $biblioLoopIter{itemholdable} = 0 if $forced_hold_level eq 'record'; + $biblioLoopIter{itemholdable} = 0 if $forced_hold_level eq 'item_group'; $biblioLoopIter{forced_hold_level} = $forced_hold_level; } @@ -606,35 +613,35 @@ foreach my $biblioNum (@biblionumbers) { $anyholdable = 1 if $biblioLoopIter{holdable}; } -unless ($pickup_locations->count) { +unless ( $pickup_locations->count ) { $numBibsAvailable = 0; - $anyholdable = 0; + $anyholdable = 0; $template->param( - message => 1, + message => 1, no_pickup_locations => 1 ); } -if ( $numBibsAvailable == 0 || $anyholdable == 0) { +if ( $numBibsAvailable == 0 || $anyholdable == 0 ) { $template->param( none_available => 1 ); } -if (scalar @biblionumbers > 1) { - $template->param( multi_hold => 1); +if ( scalar @biblionumbers > 1 ) { + $template->param( multi_hold => 1 ); } -my $show_notes=C4::Context->preference('OpacHoldNotes'); -$template->param(OpacHoldNotes=>$show_notes); +my $show_notes = C4::Context->preference('OpacHoldNotes'); +$template->param( OpacHoldNotes => $show_notes ); # display infos -$template->param(bibitemloop => $biblioLoop); +$template->param( bibitemloop => $biblioLoop ); + # can set reserve date in future -if ( - C4::Context->preference( 'AllowHoldDateInFuture' ) && - C4::Context->preference( 'OPACAllowHoldDateInFuture' ) - ) { +if ( C4::Context->preference('AllowHoldDateInFuture') + && C4::Context->preference('OPACAllowHoldDateInFuture') ) +{ $template->param( - reserve_in_future => 1, + reserve_in_future => 1, ); } diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index ed4b64db465..923d608a326 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -32,7 +32,8 @@ use C4::Items; use C4::Biblio qw( GetMarcFromKohaField ModBiblio ); use C4::HoldsQueue; use C4::Members; -use C4::Reserves qw( AddReserve AlterPriority CheckReserves ModReserve ModReserveAffect ReserveSlip CalculatePriority CanReserveBeCanceledFromOpac CanBookBeReserved IsAvailableForItemLevelRequest MoveReserve ChargeReserveFee RevertWaitingStatus CanItemBeReserved MergeHolds ); +use C4::Reserves + qw( AddReserve AlterPriority CheckReserves ModReserve ModReserveAffect ReserveSlip CalculatePriority CanReserveBeCanceledFromOpac CanBookBeReserved IsAvailableForItemLevelRequest MoveReserve ChargeReserveFee RevertWaitingStatus CanItemBeReserved MergeHolds ); use Koha::ActionLogs; use Koha::Biblios; use Koha::Caches; @@ -51,7 +52,7 @@ BEGIN { # Start transaction my $database = Koha::Database->new(); -my $schema = $database->schema(); +my $schema = $database->schema(); $schema->storage->txn_begin(); my $dbh = C4::Context->dbh; $dbh->do('DELETE FROM circulation_rules'); @@ -60,11 +61,13 @@ my $builder = t::lib::TestBuilder->new; my $frameworkcode = q//; - -t::lib::Mocks::mock_preference('ReservesNeedReturns', 1); +t::lib::Mocks::mock_preference( 'ReservesNeedReturns', 1 ); # Somewhat arbitrary field chosen for age restriction unit tests. Must be added to db before the framework is cached -$dbh->do("update marc_subfield_structure set kohafield='biblioitems.agerestriction' where tagfield='521' and tagsubfield='a' and frameworkcode=?", undef, $frameworkcode); +$dbh->do( + "update marc_subfield_structure set kohafield='biblioitems.agerestriction' where tagfield='521' and tagsubfield='a' and frameworkcode=?", + undef, $frameworkcode +); my $cache = Koha::Caches->get_instance; $cache->clear_from_cache("MarcStructure-0-$frameworkcode"); $cache->clear_from_cache("MarcStructure-1-$frameworkcode"); @@ -72,41 +75,41 @@ $cache->clear_from_cache("MarcSubfieldStructure-$frameworkcode"); ## Setup Test # Add branches -my $branch_1 = $builder->build({ source => 'Branch' })->{ branchcode }; -my $branch_2 = $builder->build({ source => 'Branch' })->{ branchcode }; -my $branch_3 = $builder->build({ source => 'Branch' })->{ branchcode }; +my $branch_1 = $builder->build( { source => 'Branch' } )->{branchcode}; +my $branch_2 = $builder->build( { source => 'Branch' } )->{branchcode}; +my $branch_3 = $builder->build( { source => 'Branch' } )->{branchcode}; + # Add categories -my $category_1 = $builder->build({ source => 'Category' })->{ categorycode }; -my $category_2 = $builder->build({ source => 'Category' })->{ categorycode }; +my $category_1 = $builder->build( { source => 'Category' } )->{categorycode}; +my $category_2 = $builder->build( { source => 'Category' } )->{categorycode}; + # Add an item type -my $itemtype = $builder->build( - { source => 'Itemtype', value => { notforloan => undef } } )->{itemtype}; +my $itemtype = $builder->build( { source => 'Itemtype', value => { notforloan => undef } } )->{itemtype}; -t::lib::Mocks::mock_userenv({ branchcode => $branch_1 }); +t::lib::Mocks::mock_userenv( { branchcode => $branch_1 } ); -my $bibnum = $builder->build_sample_biblio({frameworkcode => $frameworkcode})->biblionumber; +my $bibnum = $builder->build_sample_biblio( { frameworkcode => $frameworkcode } )->biblionumber; # Create a helper item instance for testing -my $item = $builder->build_sample_item({ biblionumber => $bibnum, library => $branch_1, itype => $itemtype }); +my $item = $builder->build_sample_item( { biblionumber => $bibnum, library => $branch_1, itype => $itemtype } ); my $biblio_with_no_item = $builder->build_sample_biblio; # Modify item; setting barcode. my $testbarcode = '97531'; -$item->barcode($testbarcode)->store; # FIXME We should not hardcode a barcode! Also, what's the purpose of this? - +$item->barcode($testbarcode)->store; # FIXME We should not hardcode a barcode! Also, what's the purpose of this? # Create a borrower my %data = ( - firstname => 'my firstname', - surname => 'my surname', + firstname => 'my firstname', + surname => 'my surname', categorycode => $category_1, - branchcode => $branch_1, + branchcode => $branch_1, ); -Koha::Patron::Categories->find($category_1)->set({ enrolmentfee => 0})->store; -my $borrowernumber = Koha::Patron->new(\%data)->store->borrowernumber; -my $patron = Koha::Patrons->find( $borrowernumber ); -my $borrower = $patron->unblessed; +Koha::Patron::Categories->find($category_1)->set( { enrolmentfee => 0 } )->store; +my $borrowernumber = Koha::Patron->new( \%data )->store->borrowernumber; +my $patron = Koha::Patrons->find($borrowernumber); +my $borrower = $patron->unblessed; my $biblionumber = $bibnum; my $branchcode = Koha::Libraries->search->next->branchcode; @@ -120,41 +123,49 @@ AddReserve( } ); -my ($status, $reserve, $all_reserves) = CheckReserves( $item ); +my ( $status, $reserve, $all_reserves ) = CheckReserves($item); -is($status, "Reserved", "CheckReserves Test 1"); +is( $status, "Reserved", "CheckReserves Test 1" ); -ok(exists($reserve->{reserve_id}), 'CheckReserves() include reserve_id in its response'); +ok( exists( $reserve->{reserve_id} ), 'CheckReserves() include reserve_id in its response' ); -($status, $reserve, $all_reserves) = CheckReserves( $item ); -is($status, "Reserved", "CheckReserves Test 2"); +( $status, $reserve, $all_reserves ) = CheckReserves($item); +is( $status, "Reserved", "CheckReserves Test 2" ); ### ### Regression test for bug 10272 ### my %requesters = (); -$requesters{$branch_1} = Koha::Patron->new({ - branchcode => $branch_1, - categorycode => $category_2, - surname => "borrower from $branch_1", -})->store->borrowernumber; -for my $i ( 2 .. 5 ) { - $requesters{"CPL$i"} = Koha::Patron->new({ +$requesters{$branch_1} = Koha::Patron->new( + { branchcode => $branch_1, categorycode => $category_2, - surname => "borrower $i from $branch_1", - })->store->borrowernumber; + surname => "borrower from $branch_1", + } +)->store->borrowernumber; +for my $i ( 2 .. 5 ) { + $requesters{"CPL$i"} = Koha::Patron->new( + { + branchcode => $branch_1, + categorycode => $category_2, + surname => "borrower $i from $branch_1", + } + )->store->borrowernumber; } -$requesters{$branch_2} = Koha::Patron->new({ - branchcode => $branch_2, - categorycode => $category_2, - surname => "borrower from $branch_2", -})->store->borrowernumber; -$requesters{$branch_3} = Koha::Patron->new({ - branchcode => $branch_3, - categorycode => $category_2, - surname => "borrower from $branch_3", -})->store->borrowernumber; +$requesters{$branch_2} = Koha::Patron->new( + { + branchcode => $branch_2, + categorycode => $category_2, + surname => "borrower from $branch_2", + } +)->store->borrowernumber; +$requesters{$branch_3} = Koha::Patron->new( + { + branchcode => $branch_3, + categorycode => $category_2, + surname => "borrower from $branch_3", + } +)->store->borrowernumber; # Configure rules so that $branch_1 allows only $branch_1 patrons # to request its items, while $branch_2 will allow its items @@ -167,7 +178,7 @@ Koha::CirculationRules->set_rules( categorycode => undef, itemtype => undef, rules => { - reservesallowed => 25, + reservesallowed => 25, holds_per_record => 1, } } @@ -176,9 +187,9 @@ Koha::CirculationRules->set_rules( # CPL allows only its own patrons to request its items Koha::CirculationRules->set_rules( { - branchcode => $branch_1, - itemtype => undef, - rules => { + branchcode => $branch_1, + itemtype => undef, + rules => { holdallowed => 'from_home_library', returnbranch => 'homebranch', } @@ -188,18 +199,18 @@ Koha::CirculationRules->set_rules( # ... while FPL allows anybody to request its items Koha::CirculationRules->set_rules( { - branchcode => $branch_2, - itemtype => undef, - rules => { + branchcode => $branch_2, + itemtype => undef, + rules => { holdallowed => 'from_any_library', returnbranch => 'homebranch', } } ); -my $bibnum2 = $builder->build_sample_biblio({frameworkcode => $frameworkcode})->biblionumber; +my $bibnum2 = $builder->build_sample_biblio( { frameworkcode => $frameworkcode } )->biblionumber; -my ($itemnum_cpl, $itemnum_fpl); +my ( $itemnum_cpl, $itemnum_fpl ); $itemnum_cpl = $builder->build_sample_item( { biblionumber => $bibnum2, @@ -219,7 +230,7 @@ $itemnum_fpl = $builder->build_sample_item( # Ensure that priorities are numbered correcly when a hold is moved to waiting # (bug 11947) -$dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum2)); +$dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, ($bibnum2) ); AddReserve( { branchcode => $branch_3, @@ -244,21 +255,20 @@ AddReserve( priority => 3, } ); -ModReserveAffect($itemnum_cpl, $requesters{$branch_3}, 0); +ModReserveAffect( $itemnum_cpl, $requesters{$branch_3}, 0 ); # Now it should have different priorities. -my $biblio = Koha::Biblios->find( $bibnum2 ); -my $holds = $biblio->holds({}, { order_by => 'reserve_id' });; -is($holds->next->priority, 0, 'Item is correctly waiting'); -is($holds->next->priority, 1, 'Item is correctly priority 1'); -is($holds->next->priority, 2, 'Item is correctly priority 2'); - -my @reserves = Koha::Holds->search({ borrowernumber => $requesters{$branch_3} })->waiting->as_list; -is( @reserves, 1, 'GetWaiting got only the waiting reserve' ); +my $biblio = Koha::Biblios->find($bibnum2); +my $holds = $biblio->holds( {}, { order_by => 'reserve_id' } ); +is( $holds->next->priority, 0, 'Item is correctly waiting' ); +is( $holds->next->priority, 1, 'Item is correctly priority 1' ); +is( $holds->next->priority, 2, 'Item is correctly priority 2' ); + +my @reserves = Koha::Holds->search( { borrowernumber => $requesters{$branch_3} } )->waiting->as_list; +is( @reserves, 1, 'GetWaiting got only the waiting reserve' ); is( $reserves[0]->borrowernumber(), $requesters{$branch_3}, 'GetWaiting got the reserve for the correct borrower' ); - -$dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum2)); +$dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, ($bibnum2) ); AddReserve( { branchcode => $branch_3, @@ -289,13 +299,16 @@ AddReserve( t::lib::Mocks::mock_preference( 'ReservesControlBranch', 'ItemHomeLibrary' ); my $messages; + # Return the CPL item at FPL. The hold that should be triggered is # the one placed by the CPL patron, as the other two patron's hold # requests cannot be filled by that item per policy. -(undef, $messages, undef, undef) = AddReturn('bug10272_CPL', $branch_2); -is( $messages->{ResFound}->{borrowernumber}, +( undef, $messages, undef, undef ) = AddReturn( 'bug10272_CPL', $branch_2 ); +is( + $messages->{ResFound}->{borrowernumber}, $requesters{$branch_1}, - 'restrictive library\'s items only fill requests by own patrons (bug 10272)'); + 'restrictive library\'s items only fill requests by own patrons (bug 10272)' +); # Return the FPL item at FPL. The hold that should be triggered is # the one placed by the RPL patron, as that patron is first in line @@ -304,20 +317,22 @@ is( $messages->{ResFound}->{borrowernumber}, # Ensure that the preference 'LocalHoldsPriority' is not set (Bug 15244): t::lib::Mocks::mock_preference( 'LocalHoldsPriority', '' ); -(undef, $messages, undef, undef) = AddReturn('bug10272_FPL', $branch_2); -is( $messages->{ResFound}->{borrowernumber}, +( undef, $messages, undef, undef ) = AddReturn( 'bug10272_FPL', $branch_2 ); +is( + $messages->{ResFound}->{borrowernumber}, $requesters{$branch_3}, - 'for generous library, its items fill first hold request in line (bug 10272)'); + 'for generous library, its items fill first hold request in line (bug 10272)' +); -$biblio = Koha::Biblios->find( $biblionumber ); -$holds = $biblio->holds; -is($holds->count, 1, "Only one reserves for this biblio"); +$biblio = Koha::Biblios->find($biblionumber); +$holds = $biblio->holds; +is( $holds->count, 1, "Only one reserves for this biblio" ); $holds->next->reserve_id; # Tests for bug 9761 (ConfirmFutureHolds): new CheckReserves lookahead parameter, and corresponding change in AddReturn # Note that CheckReserve uses its lookahead parameter and does not check ConfirmFutureHolds pref (it should be passed if needed like AddReturn does) # Test 9761a: Add a reserve without date, CheckReserve should return it -$dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); +$dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, ($bibnum) ); AddReserve( { branchcode => $branch_1, @@ -326,16 +341,16 @@ AddReserve( priority => 1, } ); -($status)=CheckReserves( $item ); -is( $status, 'Reserved', 'CheckReserves returns reserve without lookahead'); -($status)=CheckReserves( $item, 7 ); -is( $status, 'Reserved', 'CheckReserves also returns reserve with lookahead'); +($status) = CheckReserves($item); +is( $status, 'Reserved', 'CheckReserves returns reserve without lookahead' ); +($status) = CheckReserves( $item, 7 ); +is( $status, 'Reserved', 'CheckReserves also returns reserve with lookahead' ); # Test 9761b: Add a reserve with future date, CheckReserve should not return it -$dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); -t::lib::Mocks::mock_preference('AllowHoldDateInFuture', 1); -my $resdate= dt_from_string(); -$resdate->add_duration(DateTime::Duration->new(days => 4)); +$dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, ($bibnum) ); +t::lib::Mocks::mock_preference( 'AllowHoldDateInFuture', 1 ); +my $resdate = dt_from_string(); +$resdate->add_duration( DateTime::Duration->new( days => 4 ) ); my $reserve_id = AddReserve( { branchcode => $branch_1, @@ -345,31 +360,36 @@ my $reserve_id = AddReserve( reservation_date => $resdate, } ); -($status)=CheckReserves( $item ); -is( $status, '', 'CheckReserves returns no future reserve without lookahead'); +($status) = CheckReserves($item); +is( $status, '', 'CheckReserves returns no future reserve without lookahead' ); # Test 9761c: Add a reserve with future date, CheckReserve should return it if lookahead is high enough -($status)=CheckReserves( $item, 3 ); -is( $status, '', 'CheckReserves returns no future reserve with insufficient lookahead'); -($status)=CheckReserves( $item, 4 ); -is( $status, 'Reserved', 'CheckReserves returns future reserve with sufficient lookahead'); +($status) = CheckReserves( $item, 3 ); +is( $status, '', 'CheckReserves returns no future reserve with insufficient lookahead' ); +($status) = CheckReserves( $item, 4 ); +is( $status, 'Reserved', 'CheckReserves returns future reserve with sufficient lookahead' ); # Test 9761d: Check ResFound message of AddReturn for future hold # Note that AddReturn is in Circulation.pm, but this test really pertains to reserves; AddReturn uses the ConfirmFutureHolds pref when calling CheckReserves # In this test we do not need an issued item; it is just a 'checkin' -t::lib::Mocks::mock_preference('ConfirmFutureHolds', 0); -(my $doreturn, $messages)= AddReturn($testbarcode,$branch_1); -is($messages->{ResFound}//'', '', 'AddReturn does not care about future reserve when ConfirmFutureHolds is off'); -t::lib::Mocks::mock_preference('ConfirmFutureHolds', 3); -($doreturn, $messages)= AddReturn($testbarcode,$branch_1); -is(exists $messages->{ResFound}?1:0, 0, 'AddReturn ignores future reserve beyond ConfirmFutureHolds days'); -t::lib::Mocks::mock_preference('ConfirmFutureHolds', 7); -($doreturn, $messages)= AddReturn($testbarcode,$branch_1); -is(exists $messages->{ResFound}?1:0, 1, 'AddReturn considers future reserve within ConfirmFutureHolds days'); - -my $now_holder = $builder->build_object({ class => 'Koha::Patrons', value => { - branchcode => $branch_1, -}}); +t::lib::Mocks::mock_preference( 'ConfirmFutureHolds', 0 ); +( my $doreturn, $messages ) = AddReturn( $testbarcode, $branch_1 ); +is( $messages->{ResFound} // '', '', 'AddReturn does not care about future reserve when ConfirmFutureHolds is off' ); +t::lib::Mocks::mock_preference( 'ConfirmFutureHolds', 3 ); +( $doreturn, $messages ) = AddReturn( $testbarcode, $branch_1 ); +is( exists $messages->{ResFound} ? 1 : 0, 0, 'AddReturn ignores future reserve beyond ConfirmFutureHolds days' ); +t::lib::Mocks::mock_preference( 'ConfirmFutureHolds', 7 ); +( $doreturn, $messages ) = AddReturn( $testbarcode, $branch_1 ); +is( exists $messages->{ResFound} ? 1 : 0, 1, 'AddReturn considers future reserve within ConfirmFutureHolds days' ); + +my $now_holder = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + branchcode => $branch_1, + } + } +); my $now_reserve_id = AddReserve( { branchcode => $branch_1, @@ -380,46 +400,50 @@ my $now_reserve_id = AddReserve( } ); my $which_highest; -($status,$which_highest)=CheckReserves( $item, 3 ); -is( $which_highest->{reserve_id}, $now_reserve_id, 'CheckReserves returns lower priority current reserve with insufficient lookahead'); -($status, $which_highest)=CheckReserves( $item, 4 ); -is( $which_highest->{reserve_id}, $reserve_id, 'CheckReserves returns higher priority future reserve with sufficient lookahead'); -ModReserve({ reserve_id => $now_reserve_id, rank => 'del', cancellation_reason => 'test reserve' }); - +( $status, $which_highest ) = CheckReserves( $item, 3 ); +is( + $which_highest->{reserve_id}, $now_reserve_id, + 'CheckReserves returns lower priority current reserve with insufficient lookahead' +); +( $status, $which_highest ) = CheckReserves( $item, 4 ); +is( + $which_highest->{reserve_id}, $reserve_id, + 'CheckReserves returns higher priority future reserve with sufficient lookahead' +); +ModReserve( { reserve_id => $now_reserve_id, rank => 'del', cancellation_reason => 'test reserve' } ); # End of tests for bug 9761 (ConfirmFutureHolds) - # test marking a hold as captured my $hold_notice_count = count_hold_print_messages(); -ModReserveAffect($item->itemnumber, $requesters{$branch_1}, 0); +ModReserveAffect( $item->itemnumber, $requesters{$branch_1}, 0 ); my $new_count = count_hold_print_messages(); -is($new_count, $hold_notice_count + 1, 'patron notified when item set to waiting'); +is( $new_count, $hold_notice_count + 1, 'patron notified when item set to waiting' ); # test that duplicate notices aren't generated -ModReserveAffect($item->itemnumber, $requesters{$branch_1}, 0); +ModReserveAffect( $item->itemnumber, $requesters{$branch_1}, 0 ); $new_count = count_hold_print_messages(); -is($new_count, $hold_notice_count + 1, 'patron not notified a second time (bug 11445)'); +is( $new_count, $hold_notice_count + 1, 'patron not notified a second time (bug 11445)' ); # avoiding the not_same_branch error -t::lib::Mocks::mock_preference('IndependentBranches', 0); -$item = Koha::Items->find($item->itemnumber); +t::lib::Mocks::mock_preference( 'IndependentBranches', 0 ); +$item = Koha::Items->find( $item->itemnumber ); is( - @{$item->safe_delete->messages}[0]->message, + @{ $item->safe_delete->messages }[0]->message, 'book_reserved', 'item that is captured to fill a hold cannot be deleted', ); my $letter = ReserveSlip( { branchcode => $branch_1, reserve_id => $reserve_id } ); -ok(defined($letter), 'can successfully generate hold slip (bug 10949)'); +ok( defined($letter), 'can successfully generate hold slip (bug 10949)' ); # Tests for bug 9788: Does Koha::Item->current_holds return a future wait? # 9788a: current_holds does not return future next available hold -$dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); -t::lib::Mocks::mock_preference('ConfirmFutureHolds', 2); -t::lib::Mocks::mock_preference('AllowHoldDateInFuture', 1); -$resdate= dt_from_string(); -$resdate->add_duration(DateTime::Duration->new(days => 2)); +$dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, ($bibnum) ); +t::lib::Mocks::mock_preference( 'ConfirmFutureHolds', 2 ); +t::lib::Mocks::mock_preference( 'AllowHoldDateInFuture', 1 ); +$resdate = dt_from_string(); +$resdate->add_duration( DateTime::Duration->new( days => 2 ) ); AddReserve( { branchcode => $branch_1, @@ -431,11 +455,12 @@ AddReserve( ); $holds = $item->current_holds; -my $dtf = Koha::Database->new->schema->storage->datetime_parser; -my $future_holds = $holds->search({ reservedate => { '>' => $dtf->format_date( dt_from_string ) } } ); -is( $future_holds->count, 0, 'current_holds does not return a future next available hold'); +my $dtf = Koha::Database->new->schema->storage->datetime_parser; +my $future_holds = $holds->search( { reservedate => { '>' => $dtf->format_date(dt_from_string) } } ); +is( $future_holds->count, 0, 'current_holds does not return a future next available hold' ); + # 9788b: current_holds does not return future item level hold -$dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); +$dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, ($bibnum) ); AddReserve( { branchcode => $branch_1, @@ -445,19 +470,22 @@ AddReserve( reservation_date => $resdate, itemnumber => $item->itemnumber, } -); #item level hold -$future_holds = $holds->search({ reservedate => { '>' => $dtf->format_date( dt_from_string ) } } ); +); #item level hold +$future_holds = $holds->search( { reservedate => { '>' => $dtf->format_date(dt_from_string) } } ); is( $future_holds->count, 0, 'current_holds does not return a future item level hold' ); + # 9788c: current_holds returns future wait (confirmed future hold) -ModReserveAffect( $item->itemnumber, $requesters{$branch_1} , 0); #confirm hold -$future_holds = $holds->search({ reservedate => { '>' => $dtf->format_date( dt_from_string ) } } ); +ModReserveAffect( $item->itemnumber, $requesters{$branch_1}, 0 ); #confirm hold +$future_holds = $holds->search( { reservedate => { '>' => $dtf->format_date(dt_from_string) } } ); is( $future_holds->count, 1, 'current_holds returns a future wait (confirmed future hold)' ); + # End of tests for bug 9788 -$dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); +$dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, ($bibnum) ); + # Tests for CalculatePriority (bug 8918) my $p = C4::Reserves::CalculatePriority($bibnum2); -is($p, 4, 'CalculatePriority should now return priority 4'); +is( $p, 4, 'CalculatePriority should now return priority 4' ); AddReserve( { branchcode => $branch_1, @@ -467,11 +495,13 @@ AddReserve( } ); $p = C4::Reserves::CalculatePriority($bibnum2); -is($p, 5, 'CalculatePriority should now return priority 5'); +is( $p, 5, 'CalculatePriority should now return priority 5' ); + #some tests on bibnum -$dbh->do("DELETE FROM reserves WHERE biblionumber=?",undef,($bibnum)); +$dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, ($bibnum) ); $p = C4::Reserves::CalculatePriority($bibnum); -is($p, 1, 'CalculatePriority should now return priority 1'); +is( $p, 1, 'CalculatePriority should now return priority 1' ); + #add a new reserve and confirm it to waiting AddReserve( { @@ -483,10 +513,11 @@ AddReserve( } ); $p = C4::Reserves::CalculatePriority($bibnum); -is($p, 2, 'CalculatePriority should now return priority 2'); -ModReserveAffect( $item->itemnumber, $requesters{$branch_1} , 0); +is( $p, 2, 'CalculatePriority should now return priority 2' ); +ModReserveAffect( $item->itemnumber, $requesters{$branch_1}, 0 ); $p = C4::Reserves::CalculatePriority($bibnum); -is($p, 1, 'CalculatePriority should now return priority 1'); +is( $p, 1, 'CalculatePriority should now return priority 1' ); + #add another biblio hold, no resdate AddReserve( { @@ -497,68 +528,86 @@ AddReserve( } ); $p = C4::Reserves::CalculatePriority($bibnum); -is($p, 2, 'CalculatePriority should now return priority 2'); +is( $p, 2, 'CalculatePriority should now return priority 2' ); + #add another future hold -t::lib::Mocks::mock_preference('AllowHoldDateInFuture', 1); -$resdate= dt_from_string(); -$resdate->add_duration(DateTime::Duration->new(days => 1)); +t::lib::Mocks::mock_preference( 'AllowHoldDateInFuture', 1 ); +$resdate = dt_from_string(); +$resdate->add_duration( DateTime::Duration->new( days => 1 ) ); AddReserve( { - branchcode => $branch_1, - borrowernumber => $requesters{'CPL2'}, - biblionumber => $bibnum, - priority => $p, + branchcode => $branch_1, + borrowernumber => $requesters{'CPL2'}, + biblionumber => $bibnum, + priority => $p, reservation_date => $resdate, } ); $p = C4::Reserves::CalculatePriority($bibnum); -is($p, 2, 'CalculatePriority should now still return priority 2'); +is( $p, 2, 'CalculatePriority should now still return priority 2' ); + #calc priority with future resdate -$p = C4::Reserves::CalculatePriority($bibnum, $resdate); -is($p, 3, 'CalculatePriority should now return priority 3'); +$p = C4::Reserves::CalculatePriority( $bibnum, $resdate ); +is( $p, 3, 'CalculatePriority should now return priority 3' ); + # End of tests for bug 8918 # regression test for bug 12630 # Now there are 2 reserves on $bibnum -t::lib::Mocks::mock_preference('AllowHoldDateInFuture', 1); -my $bor_tmp_1 = $builder->build_object({ class => 'Koha::Patrons',value =>{ - firstname => 'my firstname tmp 1', - surname => 'my surname tmp 1', - categorycode => 'S', - branchcode => 'CPL', -}}); -my $bor_tmp_2 = $builder->build_object({ class => 'Koha::Patrons',value =>{ - firstname => 'my firstname tmp 2', - surname => 'my surname tmp 2', - categorycode => 'S', - branchcode => 'CPL', -}}); +t::lib::Mocks::mock_preference( 'AllowHoldDateInFuture', 1 ); +my $bor_tmp_1 = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + firstname => 'my firstname tmp 1', + surname => 'my surname tmp 1', + categorycode => 'S', + branchcode => 'CPL', + } + } +); +my $bor_tmp_2 = $builder->build_object( + { + class => 'Koha::Patrons', + value => { + firstname => 'my firstname tmp 2', + surname => 'my surname tmp 2', + categorycode => 'S', + branchcode => 'CPL', + } + } +); my $borrowernumber_tmp_1 = $bor_tmp_1->borrowernumber; my $borrowernumber_tmp_2 = $bor_tmp_2->borrowernumber; -my $date_in_future = dt_from_string(); -$date_in_future = $date_in_future->add_duration(DateTime::Duration->new(days => 1)); -AddReserve({ - branchcode => 'CPL', - borrowernumber => $borrowernumber_tmp_1, - biblionumber => $bibnum, - priority => 3, - reservation_date => $date_in_future -}); -AddReserve({ - branchcode => 'CPL', - borrowernumber => $borrowernumber_tmp_2, - biblionumber => $bibnum, - priority => 4, - reservation_date => $date_in_future -}); -my @r1 = Koha::Holds->search({ borrowernumber => $borrowernumber_tmp_1 })->as_list; -my @r2 = Koha::Holds->search({ borrowernumber => $borrowernumber_tmp_2 })->as_list; -is( $r1[0]->priority, 3, 'priority for hold in future should be correct'); -is( $r2[0]->priority, 4, 'priority for hold not in future should be correct'); +my $date_in_future = dt_from_string(); +$date_in_future = $date_in_future->add_duration( DateTime::Duration->new( days => 1 ) ); +AddReserve( + { + branchcode => 'CPL', + borrowernumber => $borrowernumber_tmp_1, + biblionumber => $bibnum, + priority => 3, + reservation_date => $date_in_future + } +); +AddReserve( + { + branchcode => 'CPL', + borrowernumber => $borrowernumber_tmp_2, + biblionumber => $bibnum, + priority => 4, + reservation_date => $date_in_future + } +); +my @r1 = Koha::Holds->search( { borrowernumber => $borrowernumber_tmp_1 } )->as_list; +my @r2 = Koha::Holds->search( { borrowernumber => $borrowernumber_tmp_2 } )->as_list; +is( $r1[0]->priority, 3, 'priority for hold in future should be correct' ); +is( $r2[0]->priority, 4, 'priority for hold not in future should be correct' ); + # end of tests for bug 12630 # Tests for cancel reserves by users from OPAC. -$dbh->do('DELETE FROM reserves', undef, ($bibnum)); +$dbh->do( 'DELETE FROM reserves', undef, ($bibnum) ); AddReserve( { branchcode => $branch_1, @@ -567,9 +616,10 @@ AddReserve( priority => 1, } ); -my (undef, $canres, undef) = CheckReserves( $item ); +my ( undef, $canres, undef ) = CheckReserves($item); -is( CanReserveBeCanceledFromOpac(), undef, +is( + CanReserveBeCanceledFromOpac(), undef, 'CanReserveBeCanceledFromOpac should return undef if called without any parameter' ); is( @@ -583,19 +633,21 @@ is( 'CanReserveBeCanceledFromOpac should return undef if called without borrowernumber' ); -my $cancancel = CanReserveBeCanceledFromOpac($canres->{reserve_id}, $requesters{$branch_1}); -is($cancancel, 1, 'Can user cancel its own reserve'); +my $cancancel = CanReserveBeCanceledFromOpac( $canres->{reserve_id}, $requesters{$branch_1} ); +is( $cancancel, 1, 'Can user cancel its own reserve' ); -$cancancel = CanReserveBeCanceledFromOpac($canres->{reserve_id}, $requesters{$branch_2}); -is($cancancel, 0, 'Other user cant cancel reserve'); +$cancancel = CanReserveBeCanceledFromOpac( $canres->{reserve_id}, $requesters{$branch_2} ); +is( $cancancel, 0, 'Other user cant cancel reserve' ); -ModReserveAffect($item->itemnumber, $requesters{$branch_1}, 1); -$cancancel = CanReserveBeCanceledFromOpac($canres->{reserve_id}, $requesters{$branch_1}); -is($cancancel, 0, 'Reserve in transfer status cant be canceled'); +ModReserveAffect( $item->itemnumber, $requesters{$branch_1}, 1 ); +$cancancel = CanReserveBeCanceledFromOpac( $canres->{reserve_id}, $requesters{$branch_1} ); +is( $cancancel, 0, 'Reserve in transfer status cant be canceled' ); -$dbh->do('DELETE FROM reserves', undef, ($bibnum)); -is( CanReserveBeCanceledFromOpac($canres->{resserve_id}, $requesters{$branch_1}), undef, - 'Cannot cancel a deleted hold' ); +$dbh->do( 'DELETE FROM reserves', undef, ($bibnum) ); +is( + CanReserveBeCanceledFromOpac( $canres->{resserve_id}, $requesters{$branch_1} ), undef, + 'Cannot cancel a deleted hold' +); AddReserve( { @@ -605,17 +657,17 @@ AddReserve( priority => 1, } ); -(undef, $canres, undef) = CheckReserves( $item ); +( undef, $canres, undef ) = CheckReserves($item); -ModReserveAffect($item->itemnumber, $requesters{$branch_1}, 0); -$cancancel = CanReserveBeCanceledFromOpac($canres->{reserve_id}, $requesters{$branch_1}); -is($cancancel, 0, 'Reserve in waiting status cant be canceled'); +ModReserveAffect( $item->itemnumber, $requesters{$branch_1}, 0 ); +$cancancel = CanReserveBeCanceledFromOpac( $canres->{reserve_id}, $requesters{$branch_1} ); +is( $cancancel, 0, 'Reserve in waiting status cant be canceled' ); # End of tests for bug 12876 - #### +#### ####### Testing Bug 13113 - Prevent juvenile/children from reserving ageRestricted material >>> - #### +#### t::lib::Mocks::mock_preference( 'AgeRestrictionMarker', 'FSK|PEGI|Age|K' ); @@ -624,32 +676,44 @@ t::lib::Mocks::mock_preference( 'AgeRestrictionMarker', 'FSK|PEGI|Age|K' ); #Set the ageRestriction for the Biblio $biblio = Koha::Biblios->find($bibnum); my $record = $biblio->metadata->record; -my ( $ageres_tagid, $ageres_subfieldid ) = GetMarcFromKohaField( "biblioitems.agerestriction" ); -$record->append_fields( MARC::Field->new($ageres_tagid, '', '', $ageres_subfieldid => 'PEGI 16') ); +my ( $ageres_tagid, $ageres_subfieldid ) = GetMarcFromKohaField("biblioitems.agerestriction"); +$record->append_fields( MARC::Field->new( $ageres_tagid, '', '', $ageres_subfieldid => 'PEGI 16' ) ); C4::Biblio::ModBiblio( $record, $bibnum, $frameworkcode ); -is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber)->{status} , 'OK', "Reserving an ageRestricted Biblio without a borrower dateofbirth succeeds" ); +is( + C4::Reserves::CanBookBeReserved( $borrowernumber, $biblionumber )->{status}, 'OK', + "Reserving an ageRestricted Biblio without a borrower dateofbirth succeeds" +); #Set the dateofbirth for the Borrower making them "too young". $borrower->{dateofbirth} = DateTime->now->add( years => -15 ); -Koha::Patrons->find( $borrowernumber )->set({ dateofbirth => $borrower->{dateofbirth} })->store; +Koha::Patrons->find($borrowernumber)->set( { dateofbirth => $borrower->{dateofbirth} } )->store; -is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber)->{status} , 'ageRestricted', "Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails"); +is( + C4::Reserves::CanBookBeReserved( $borrowernumber, $biblionumber )->{status}, 'ageRestricted', + "Reserving a 'PEGI 16' Biblio by a 15 year old borrower fails" +); #Set the dateofbirth for the Borrower making them "too old". $borrower->{dateofbirth} = DateTime->now->add( years => -30 ); -Koha::Patrons->find( $borrowernumber )->set({ dateofbirth => $borrower->{dateofbirth} })->store; +Koha::Patrons->find($borrowernumber)->set( { dateofbirth => $borrower->{dateofbirth} } )->store; -is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber)->{status} , 'OK', "Reserving a 'PEGI 16' Biblio by a 30 year old borrower succeeds"); +is( + C4::Reserves::CanBookBeReserved( $borrowernumber, $biblionumber )->{status}, 'OK', + "Reserving a 'PEGI 16' Biblio by a 30 year old borrower succeeds" +); -is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblio_with_no_item->biblionumber)->{status} , '', "Biblio with no item. Status is empty"); - #### +is( + C4::Reserves::CanBookBeReserved( $borrowernumber, $biblio_with_no_item->biblionumber )->{status}, '', + "Biblio with no item. Status is empty" +); +#### ####### EO Bug 13113 <<< - #### +#### -ok( C4::Reserves::IsAvailableForItemLevelRequest($item, $patron), "Reserving a book on item level" ); +ok( C4::Reserves::IsAvailableForItemLevelRequest( $item, $patron ), "Reserving a book on item level" ); -my $pickup_branch = $builder->build({ source => 'Branch' })->{ branchcode }; +my $pickup_branch = $builder->build( { source => 'Branch' } )->{branchcode}; t::lib::Mocks::mock_preference( 'UseBranchTransferLimits', '1' ); t::lib::Mocks::mock_preference( 'BranchTransferLimitsType', 'itemtype' ); my $limit = Koha::Item::Transfer::Limit->new( @@ -659,17 +723,20 @@ my $limit = Koha::Item::Transfer::Limit->new( itemtype => $item->effective_itemtype, } )->store(); -is( C4::Reserves::IsAvailableForItemLevelRequest($item, $patron, $pickup_branch), 0, "Item level request not available due to transfer limit" ); -t::lib::Mocks::mock_preference( 'UseBranchTransferLimits', '0' ); +is( + C4::Reserves::IsAvailableForItemLevelRequest( $item, $patron, $pickup_branch ), 0, + "Item level request not available due to transfer limit" +); +t::lib::Mocks::mock_preference( 'UseBranchTransferLimits', '0' ); -my $categorycode = $borrower->{categorycode}; +my $categorycode = $borrower->{categorycode}; my $holdingbranch = $item->{holdingbranch}; Koha::CirculationRules->set_rules( { categorycode => $categorycode, itemtype => $item->effective_itemtype, branchcode => $holdingbranch, - rules => { + rules => { onshelfholds => 1, } } @@ -677,9 +744,9 @@ Koha::CirculationRules->set_rules( # tests for MoveReserve in relation to ConfirmFutureHolds (BZ 14526) # hold from A pos 1, today, no fut holds: MoveReserve should fill it -$dbh->do('DELETE FROM reserves', undef, ($bibnum)); -t::lib::Mocks::mock_preference('ConfirmFutureHolds', 0); -t::lib::Mocks::mock_preference('AllowHoldDateInFuture', 1); +$dbh->do( 'DELETE FROM reserves', undef, ($bibnum) ); +t::lib::Mocks::mock_preference( 'ConfirmFutureHolds', 0 ); +t::lib::Mocks::mock_preference( 'AllowHoldDateInFuture', 1 ); AddReserve( { branchcode => $branch_1, @@ -689,10 +756,11 @@ AddReserve( } ); MoveReserve( $item->itemnumber, $borrowernumber ); -($status)=CheckReserves( $item ); -is( $status, '', 'MoveReserve filled hold'); +($status) = CheckReserves($item); +is( $status, '', 'MoveReserve filled hold' ); + # hold from A waiting, today, no fut holds: MoveReserve should fill it -my $other_item = $builder->build_sample_item({ biblionumber => $biblio->id }); +my $other_item = $builder->build_sample_item( { biblionumber => $biblio->id } ); AddReserve( { branchcode => $branch_1, @@ -704,67 +772,71 @@ AddReserve( } ); MoveReserve( $item->itemnumber, $borrowernumber ); -($status)=CheckReserves( $item ); -is( $status, '', 'MoveReserve filled waiting hold'); +($status) = CheckReserves($item); +is( $status, '', 'MoveReserve filled waiting hold' ); + # hold from A pos 1, tomorrow, no fut holds: not filled -$resdate= dt_from_string(); -$resdate->add_duration(DateTime::Duration->new(days => 1)); +$resdate = dt_from_string(); +$resdate->add_duration( DateTime::Duration->new( days => 1 ) ); AddReserve( { - branchcode => $branch_1, - borrowernumber => $borrowernumber, - biblionumber => $bibnum, - priority => 1, + branchcode => $branch_1, + borrowernumber => $borrowernumber, + biblionumber => $bibnum, + priority => 1, reservation_date => $resdate, } ); MoveReserve( $item->itemnumber, $borrowernumber ); -($status)=CheckReserves( $item, 1 ); -is( $status, 'Reserved', 'MoveReserve did not fill future hold'); -$dbh->do('DELETE FROM reserves', undef, ($bibnum)); +($status) = CheckReserves( $item, 1 ); +is( $status, 'Reserved', 'MoveReserve did not fill future hold' ); +$dbh->do( 'DELETE FROM reserves', undef, ($bibnum) ); + # hold from A pos 1, tomorrow, fut holds=2: MoveReserve should fill it -t::lib::Mocks::mock_preference('ConfirmFutureHolds', 2); +t::lib::Mocks::mock_preference( 'ConfirmFutureHolds', 2 ); AddReserve( { - branchcode => $branch_1, - borrowernumber => $borrowernumber, - biblionumber => $bibnum, - priority => 1, + branchcode => $branch_1, + borrowernumber => $borrowernumber, + biblionumber => $bibnum, + priority => 1, reservation_date => $resdate, } ); MoveReserve( $item->itemnumber, $borrowernumber ); -($status)=CheckReserves( $item, undef, 2 ); -is( $status, '', 'MoveReserve filled future hold now'); +($status) = CheckReserves( $item, undef, 2 ); +is( $status, '', 'MoveReserve filled future hold now' ); + # hold from A waiting, tomorrow, fut holds=2: MoveReserve should fill it AddReserve( { - branchcode => $branch_1, - borrowernumber => $borrowernumber, - biblionumber => $bibnum, - priority => 1, + branchcode => $branch_1, + borrowernumber => $borrowernumber, + biblionumber => $bibnum, + priority => 1, reservation_date => $resdate, } ); MoveReserve( $item->itemnumber, $borrowernumber ); -($status)=CheckReserves( $item, undef, 2 ); -is( $status, '', 'MoveReserve filled future waiting hold now'); +($status) = CheckReserves( $item, undef, 2 ); +is( $status, '', 'MoveReserve filled future waiting hold now' ); + # hold from A pos 1, today+3, fut holds=2: MoveReserve should not fill it -$resdate= dt_from_string(); -$resdate->add_duration(DateTime::Duration->new(days => 3)); +$resdate = dt_from_string(); +$resdate->add_duration( DateTime::Duration->new( days => 3 ) ); AddReserve( { - branchcode => $branch_1, - borrowernumber => $borrowernumber, - biblionumber => $bibnum, - priority => 1, + branchcode => $branch_1, + borrowernumber => $borrowernumber, + biblionumber => $bibnum, + priority => 1, reservation_date => $resdate, } ); MoveReserve( $item->itemnumber, $borrowernumber ); -($status)=CheckReserves( $item, 3 ); -is( $status, 'Reserved', 'MoveReserve did not fill future hold of 3 days'); -$dbh->do('DELETE FROM reserves', undef, ($bibnum)); +($status) = CheckReserves( $item, 3 ); +is( $status, 'Reserved', 'MoveReserve did not fill future hold of 3 days' ); +$dbh->do( 'DELETE FROM reserves', undef, ($bibnum) ); $cache->clear_from_cache("MarcStructure-0-$frameworkcode"); $cache->clear_from_cache("MarcStructure-1-$frameworkcode"); @@ -774,63 +846,73 @@ subtest '_koha_notify_reserve() tests' => sub { plan tests => 3; - my $branch = $builder->build_object({ - class => 'Koha::Libraries', - value => { - branchemail => 'branch@e.mail', - branchreplyto => 'branch@reply.to', - pickup_location => 1 + my $branch = $builder->build_object( + { + class => 'Koha::Libraries', + value => { + branchemail => 'branch@e.mail', + branchreplyto => 'branch@reply.to', + pickup_location => 1 + } } - }); - my $item = $builder->build_sample_item({ - homebranch => $branch->branchcode, - holdingbranch => $branch->branchcode - }); + ); + my $item = $builder->build_sample_item( + { + homebranch => $branch->branchcode, + holdingbranch => $branch->branchcode + } + ); my $wants_hold_and_email = { wants_digest => '0', - transports => { - sms => 'HOLD', + transports => { + sms => 'HOLD', email => 'HOLD', - }, + }, letter_code => 'HOLD' }; - my $mp = Test::MockModule->new( 'C4::Members::Messaging' ); + my $mp = Test::MockModule->new('C4::Members::Messaging'); - $mp->mock("GetMessagingPreferences",$wants_hold_and_email); + $mp->mock( "GetMessagingPreferences", $wants_hold_and_email ); $dbh->do('DELETE FROM letter'); - my $email_hold_notice = $builder->build({ + my $email_hold_notice = $builder->build( + { source => 'Letter', - value => { + value => { message_transport_type => 'email', - branchcode => '', - code => 'HOLD', - module => 'reserves', - lang => 'default', + branchcode => '', + code => 'HOLD', + module => 'reserves', + lang => 'default', } - }); + } + ); - my $sms_hold_notice = $builder->build({ + my $sms_hold_notice = $builder->build( + { source => 'Letter', - value => { + value => { message_transport_type => 'sms', - branchcode => '', - code => 'HOLD', - module => 'reserves', - lang=>'default', + branchcode => '', + code => 'HOLD', + module => 'reserves', + lang => 'default', } - }); + } + ); - my $hold_borrower = $builder->build({ + my $hold_borrower = $builder->build( + { source => 'Borrower', - value => { - smsalertnumber=>'5555555555', - email=>'a@b.com', + value => { + smsalertnumber => '5555555555', + email => 'a@b.com', } - })->{borrowernumber}; + } + )->{borrowernumber}; C4::Reserves::AddReserve( { @@ -840,42 +922,49 @@ subtest '_koha_notify_reserve() tests' => sub { } ); - ModReserveAffect($item->itemnumber, $hold_borrower, 0); - my $sms_message_address = $schema->resultset('MessageQueue')->search({ - letter_code => 'HOLD', + ModReserveAffect( $item->itemnumber, $hold_borrower, 0 ); + my $sms_message_address = $schema->resultset('MessageQueue')->search( + { + letter_code => 'HOLD', message_transport_type => 'sms', - borrowernumber => $hold_borrower, - })->next()->to_address(); - is($sms_message_address, undef ,"We should not populate the sms message with the sms number, sending will do so"); + borrowernumber => $hold_borrower, + } + )->next()->to_address(); + is( $sms_message_address, undef, "We should not populate the sms message with the sms number, sending will do so" ); - my $email = $schema->resultset('MessageQueue')->search({ - letter_code => 'HOLD', + my $email = $schema->resultset('MessageQueue')->search( + { + letter_code => 'HOLD', message_transport_type => 'email', - borrowernumber => $hold_borrower, - })->next(); + borrowernumber => $hold_borrower, + } + )->next(); my $email_to_address = $email->to_address(); - is($email_to_address, undef ,"We should not populate the hold message with the email address, sending will do so"); + is( + $email_to_address, undef, + "We should not populate the hold message with the email address, sending will do so" + ); my $email_from_address = $email->from_address(); - is($email_from_address,'branch@e.mail',"Library's from address is used for sending"); + is( $email_from_address, 'branch@e.mail', "Library's from address is used for sending" ); }; subtest 'ReservesNeedReturns' => sub { plan tests => 18; - my $library = $builder->build_object( { class => 'Koha::Libraries' } ); - my $item_info = { - homebranch => $library->branchcode, - holdingbranch => $library->branchcode, + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + my $item_info = { + homebranch => $library->branchcode, + holdingbranch => $library->branchcode, }; - my $item = $builder->build_sample_item($item_info); - my $patron = $builder->build_object( + my $item = $builder->build_sample_item($item_info); + my $patron = $builder->build_object( { class => 'Koha::Patrons', value => { branchcode => $library->branchcode, } } ); - my $patron_2 = $builder->build_object( + my $patron_2 = $builder->build_object( { class => 'Koha::Patrons', value => { branchcode => $library->branchcode, } @@ -884,16 +973,17 @@ subtest 'ReservesNeedReturns' => sub { my $priority = 1; - t::lib::Mocks::mock_preference('ReservesNeedReturns', 1); # Test with feature disabled + t::lib::Mocks::mock_preference( 'ReservesNeedReturns', 1 ); # Test with feature disabled my $hold = place_item_hold( $patron, $item, $library, $priority ); is( $hold->priority, $priority, 'If ReservesNeedReturns is 1, priority must not have been set to changed' ); - is( $hold->found, undef, 'If ReservesNeedReturns is 1, found must not have been set waiting' ); + is( $hold->found, undef, 'If ReservesNeedReturns is 1, found must not have been set waiting' ); $hold->delete; - t::lib::Mocks::mock_preference('ReservesNeedReturns', 0); # '0' means 'Automatically mark a hold as found and waiting' + t::lib::Mocks::mock_preference( 'ReservesNeedReturns', 0 ) + ; # '0' means 'Automatically mark a hold as found and waiting' $hold = place_item_hold( $patron, $item, $library, $priority ); - is( $hold->priority, 0, 'If ReservesNeedReturns is 0 and no other status, priority must have been set to 0' ); - is( $hold->found, 'W', 'If ReservesNeedReturns is 0 and no other status, found must have been set waiting' ); + is( $hold->priority, 0, 'If ReservesNeedReturns is 0 and no other status, priority must have been set to 0' ); + is( $hold->found, 'W', 'If ReservesNeedReturns is 0 and no other status, found must have been set waiting' ); $hold->delete; $item->onloan('2010-01-01')->store; @@ -901,63 +991,71 @@ subtest 'ReservesNeedReturns' => sub { is( $hold->priority, 1, 'If ReservesNeedReturns is 0 but item onloan priority must be set to 1' ); $hold->delete; - t::lib::Mocks::mock_preference('AllowHoldsOnDamagedItems', 0); # '0' means damaged holds not allowed + t::lib::Mocks::mock_preference( 'AllowHoldsOnDamagedItems', 0 ); # '0' means damaged holds not allowed $item->onloan(undef)->damaged(1)->store; $hold = place_item_hold( $patron, $item, $library, $priority ); - is( $hold->priority, 1, 'If ReservesNeedReturns is 0 but item damaged and not allowed holds on damaged items priority must be set to 1' ); + is( + $hold->priority, 1, + 'If ReservesNeedReturns is 0 but item damaged and not allowed holds on damaged items priority must be set to 1' + ); $hold->delete; - t::lib::Mocks::mock_preference('AllowHoldsOnDamagedItems', 1); # '0' means damaged holds not allowed + t::lib::Mocks::mock_preference( 'AllowHoldsOnDamagedItems', 1 ); # '0' means damaged holds not allowed $hold = place_item_hold( $patron, $item, $library, $priority ); is( $hold->priority, 0, 'If ReservesNeedReturns is 0 and damaged holds allowed, priority must have been set to 0' ); - is( $hold->found, 'W', 'If ReservesNeedReturns is 0 and damaged holds allowed, found must have been set waiting' ); + is( $hold->found, 'W', 'If ReservesNeedReturns is 0 and damaged holds allowed, found must have been set waiting' ); $hold->delete; my $hold_1 = place_item_hold( $patron, $item, $library, $priority ); - is( $hold_1->found, 'W', 'First hold on item is set to waiting with ReservesNeedReturns set to 0' ); - is( $hold_1->priority, 0, 'First hold on item is set to waiting with ReservesNeedReturns set to 0' ); + is( $hold_1->found, 'W', 'First hold on item is set to waiting with ReservesNeedReturns set to 0' ); + is( $hold_1->priority, 0, 'First hold on item is set to waiting with ReservesNeedReturns set to 0' ); $hold = place_item_hold( $patron_2, $item, $library, $priority ); is( $hold->priority, 1, 'If ReservesNeedReturns is 0 but item already on hold priority must be set to 1' ); $hold->delete; $hold_1->delete; - my $transfer = $builder->build_object({ - class => "Koha::Item::Transfers", - value => { - itemnumber => $item->itemnumber, - datearrived => undef, - datecancelled => undef + my $transfer = $builder->build_object( + { + class => "Koha::Item::Transfers", + value => { + itemnumber => $item->itemnumber, + datearrived => undef, + datecancelled => undef + } } - }); + ); $item->damaged(0)->store; $hold = place_item_hold( $patron, $item, $library, $priority ); - is( $hold->found, undef, 'If ReservesNeedReturns is 0 but item in transit the hold must not be set to waiting' ); - is( $hold->priority, 1, 'If ReservesNeedReturns is 0 but item in transit the hold must not be set to waiting' ); + is( $hold->found, undef, 'If ReservesNeedReturns is 0 but item in transit the hold must not be set to waiting' ); + is( $hold->priority, 1, 'If ReservesNeedReturns is 0 but item in transit the hold must not be set to waiting' ); $hold->delete; $transfer->delete; $hold = place_item_hold( $patron, $item, $library, $priority ); - is( $hold->priority, 0, 'If ReservesNeedReturns is 0 and no other status, priority must have been set to 0' ); - is( $hold->found, 'W', 'If ReservesNeedReturns is 0 and no other status, found must have been set waiting' ); + is( $hold->priority, 0, 'If ReservesNeedReturns is 0 and no other status, priority must have been set to 0' ); + is( $hold->found, 'W', 'If ReservesNeedReturns is 0 and no other status, found must have been set waiting' ); $hold_1 = place_item_hold( $patron, $item, $library, $priority ); is( $hold_1->priority, 1, 'If ReservesNeedReturns is 0 but item has a hold priority is 1' ); - $hold_1->suspend(1)->store; # We suspend the hold - $hold->delete; # Delete the waiting hold + $hold_1->suspend(1)->store; # We suspend the hold + $hold->delete; # Delete the waiting hold $hold = place_item_hold( $patron, $item, $library, $priority ); - is( $hold->priority, 0, 'If ReservesNeedReturns is 0 and other hold(s) suspended, priority must have been set to 0' ); - is( $hold->found, 'W', 'If ReservesNeedReturns is 0 and other hold(s) suspended, found must have been set waiting' ); - - - + is( + $hold->priority, 0, + 'If ReservesNeedReturns is 0 and other hold(s) suspended, priority must have been set to 0' + ); + is( + $hold->found, 'W', + 'If ReservesNeedReturns is 0 and other hold(s) suspended, found must have been set waiting' + ); - t::lib::Mocks::mock_preference('ReservesNeedReturns', 1); # Don't affect other tests + t::lib::Mocks::mock_preference( 'ReservesNeedReturns', 1 ); # Don't affect other tests }; subtest 'ChargeReserveFee tests' => sub { plan tests => 8; - my $library = $builder->build_object({ class => 'Koha::Libraries' }); - my $patron = $builder->build_object({ class => 'Koha::Patrons' }); + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); my $fee = 20; my $title = 'A title'; @@ -967,21 +1065,21 @@ subtest 'ChargeReserveFee tests' => sub { my $line = C4::Reserves::ChargeReserveFee( $patron->id, $fee, $title ); - is( ref($line), 'Koha::Account::Line' , 'Returns a Koha::Account::Line object'); + is( ref($line), 'Koha::Account::Line', 'Returns a Koha::Account::Line object' ); ok( $line->is_debit, 'Generates a debit line' ); - is( $line->debit_type_code, 'RESERVE' , 'generates RESERVE debit_type'); - is( $line->borrowernumber, $patron->id , 'generated line belongs to the passed patron'); - is( $line->amount, $fee , 'amount set correctly'); - is( $line->amountoutstanding, $fee , 'amountoutstanding set correctly'); - is( $line->description, "$title" , 'description is title of reserved item'); - is( $line->branchcode, $library->id , "Library id is picked from userenv and stored correctly" ); + is( $line->debit_type_code, 'RESERVE', 'generates RESERVE debit_type' ); + is( $line->borrowernumber, $patron->id, 'generated line belongs to the passed patron' ); + is( $line->amount, $fee, 'amount set correctly' ); + is( $line->amountoutstanding, $fee, 'amountoutstanding set correctly' ); + is( $line->description, "$title", 'description is title of reserved item' ); + is( $line->branchcode, $library->id, "Library id is picked from userenv and stored correctly" ); }; subtest 'reserves.item_level_hold' => sub { plan tests => 4; my $item = $builder->build_sample_item; - my $patron = $builder->build_object({ class => "Koha::Patrons" }); + my $patron = $builder->build_object( { class => "Koha::Patrons" } ); $patron->branchcode( $item->homebranch ); subtest 'item level hold' => sub { @@ -1003,14 +1101,17 @@ subtest 'reserves.item_level_hold' => sub { ModReserveAffect( $item->itemnumber, $patron->id, 1 ); my $mock = Test::MockModule->new('Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue'); - $mock->mock( 'enqueue', sub { - my ( $self, $args ) = @_; - is_deeply( - $args->{biblio_ids}, - [ $hold->biblionumber ], - "AlterPriority triggers a holds queue update for the related biblio" - ); - } ); + $mock->mock( + 'enqueue', + sub { + my ( $self, $args ) = @_; + is_deeply( + $args->{biblio_ids}, + [ $hold->biblionumber ], + "AlterPriority triggers a holds queue update for the related biblio" + ); + } + ); t::lib::Mocks::mock_preference( 'RealTimeHoldsQueue', 1 ); t::lib::Mocks::mock_preference( 'HoldsLog', 1 ); @@ -1060,7 +1161,10 @@ subtest 'reserves.item_level_hold' => sub { ); my $hold = Koha::Holds->find($reserve_id); - is( $hold->item_level_hold, 0, 'item_level_hold should not be set when AddReserve is called without a specific item' ); + is( + $hold->item_level_hold, 0, + 'item_level_hold should not be set when AddReserve is called without a specific item' + ); # Mark it waiting ModReserveAffect( $item->itemnumber, $patron->borrowernumber, 1 ); @@ -1087,21 +1191,25 @@ subtest 'reserves.item_level_hold' => sub { itemtype => undef, rules => { reservesallowed => 25, - opacitemholds => 'F', + opacitemholds => 'F', } } ); my $canreserve = C4::Reserves::CanBookBeReserved( $patron->id, $item->biblionumber ); - is( $canreserve->{status}, + is( + $canreserve->{status}, 'recordHoldNotAllowed', - 'record-level holds should not be possible with opacitemholds set to "Force"' ); + 'record-level holds should not be possible with opacitemholds set to "Force"' + ); $canreserve = C4::Reserves::CanItemBeReserved( $patron, $item ); - is( $canreserve->{status}, 'OK', - 'item-level holds should be possible with opacitemholds set to "Force"' ); + is( + $canreserve->{status}, 'OK', + 'item-level holds should be possible with opacitemholds set to "Force"' + ); Koha::CirculationRules->set_rules( { @@ -1110,20 +1218,24 @@ subtest 'reserves.item_level_hold' => sub { itemtype => undef, rules => { reservesallowed => 25, - opacitemholds => 'N', + opacitemholds => 'N', } } - ); + ); $canreserve = C4::Reserves::CanBookBeReserved( $patron->id, $item->biblionumber ); - is( $canreserve->{status}, 'OK', - 'record-level holds should be possible with opacitemholds set to "No"' ); + is( + $canreserve->{status}, 'OK', + 'record-level holds should be possible with opacitemholds set to "No"' + ); $canreserve = C4::Reserves::CanItemBeReserved( $patron, $item ); - is( $canreserve->{status}, 'recordHoldsOnly', - 'item-level holds should not be possible with opacitemholds set to "No"' ); + is( + $canreserve->{status}, 'recordHoldsOnly', + 'item-level holds should not be possible with opacitemholds set to "No"' + ); Koha::CirculationRules->set_rules( { @@ -1132,20 +1244,24 @@ subtest 'reserves.item_level_hold' => sub { itemtype => undef, rules => { reservesallowed => 25, - opacitemholds => 'Y', + opacitemholds => 'Y', } } - ); + ); $canreserve = C4::Reserves::CanBookBeReserved( $patron->id, $item->biblionumber ); - is( $canreserve->{status}, 'OK', - 'record-level holds should be possible with opacitemholds set to "Yes"' ); + is( + $canreserve->{status}, 'OK', + 'record-level holds should be possible with opacitemholds set to "Yes"' + ); $canreserve = C4::Reserves::CanItemBeReserved( $patron, $item ); - is( $canreserve->{status}, 'OK', - 'item-level holds should be possible with opacitemholds set to "Yes"' ); + is( + $canreserve->{status}, 'OK', + 'item-level holds should be possible with opacitemholds set to "Yes"' + ); }; subtest 'test opacitemholds rules in staff context' => sub { @@ -1159,30 +1275,34 @@ subtest 'reserves.item_level_hold' => sub { itemtype => undef, rules => { reservesallowed => 25, - opacitemholds => 'N', + opacitemholds => 'N', } } ); my $canreserve = C4::Reserves::CanBookBeReserved( $patron->id, $item->biblionumber ); - is( $canreserve->{status}, 'OK', - 'record-level holds should be possible with opacitemholds set to "No"' ); + is( + $canreserve->{status}, 'OK', + 'record-level holds should be possible with opacitemholds set to "No"' + ); $canreserve = C4::Reserves::CanItemBeReserved( $patron, $item ); - is( $canreserve->{status}, 'OK', - 'item-level holds should still be possible in staff context, even with opacitemholds set to "No"' ); + is( + $canreserve->{status}, 'OK', + 'item-level holds should still be possible in staff context, even with opacitemholds set to "No"' + ); }; Koha::CirculationRules->set_rules( { - branchcode => undef, - categorycode => undef, - itemtype => undef, - rules => { - opacitemholds => undef, - } + branchcode => undef, + categorycode => undef, + itemtype => undef, + rules => { + opacitemholds => undef, + } } ); }; @@ -1193,11 +1313,13 @@ subtest 'MoveReserve additional test' => sub { # Create the items and patrons we need my $biblio = $builder->build_sample_biblio(); - my $itype = $builder->build_object({ class => "Koha::ItemTypes", value => { notforloan => 0 } }); - my $item_1 = $builder->build_sample_item({ biblionumber => $biblio->biblionumber,notforloan => 0, itype => $itype->itemtype }); - my $item_2 = $builder->build_sample_item({ biblionumber => $biblio->biblionumber, notforloan => 0, itype => $itype->itemtype }); - my $patron_1 = $builder->build_object({ class => "Koha::Patrons" }); - my $patron_2 = $builder->build_object({ class => "Koha::Patrons" }); + my $itype = $builder->build_object( { class => "Koha::ItemTypes", value => { notforloan => 0 } } ); + my $item_1 = $builder->build_sample_item( + { biblionumber => $biblio->biblionumber, notforloan => 0, itype => $itype->itemtype } ); + my $item_2 = $builder->build_sample_item( + { biblionumber => $biblio->biblionumber, notforloan => 0, itype => $itype->itemtype } ); + my $patron_1 = $builder->build_object( { class => "Koha::Patrons" } ); + my $patron_2 = $builder->build_object( { class => "Koha::Patrons" } ); # Place a hold on the title for both patrons my $reserve_1 = AddReserve( @@ -1218,16 +1340,22 @@ subtest 'MoveReserve additional test' => sub { itemnumber => $item_1->itemnumber, } ); - is($patron_1->holds->next()->reserve_id, $reserve_1, "The 1st patron has a hold"); - is($patron_2->holds->next()->reserve_id, $reserve_2, "The 2nd patron has a hold"); + is( $patron_1->holds->next()->reserve_id, $reserve_1, "The 1st patron has a hold" ); + is( $patron_2->holds->next()->reserve_id, $reserve_2, "The 2nd patron has a hold" ); # Fake the holds queue - $dbh->do(q{INSERT INTO hold_fill_targets VALUES (?, ?, ?, ?, ?,?)},undef,($patron_1->borrowernumber,$biblio->biblionumber,$item_1->itemnumber,$item_1->homebranch,0,$reserve_1)); + $dbh->do( + q{INSERT INTO hold_fill_targets VALUES (?, ?, ?, ?, ?,?)}, undef, + ( $patron_1->borrowernumber, $biblio->biblionumber, $item_1->itemnumber, $item_1->homebranch, 0, $reserve_1 ) + ); # The 2nd hold should be filed even if the item is preselected for the first hold - MoveReserve($item_1->itemnumber,$patron_2->borrowernumber); - is($patron_2->holds->count, 0, "The 2nd patrons no longer has a hold"); - is($patron_2->old_holds->next()->reserve_id, $reserve_2, "The 2nd patrons hold was filled and moved to old holds"); + MoveReserve( $item_1->itemnumber, $patron_2->borrowernumber ); + is( $patron_2->holds->count, 0, "The 2nd patrons no longer has a hold" ); + is( + $patron_2->old_holds->next()->reserve_id, $reserve_2, + "The 2nd patrons hold was filled and moved to old holds" + ); }; @@ -1238,9 +1366,8 @@ subtest 'RevertWaitingStatus' => sub { # Create the items and patrons we need my $biblio = $builder->build_sample_biblio(); my $library = $builder->build_object( { class => 'Koha::Libraries' } ); - my $itype = $builder->build_object( - { class => "Koha::ItemTypes", value => { notforloan => 0 } } ); - my $item_1 = $builder->build_sample_item( + my $itype = $builder->build_object( { class => "Koha::ItemTypes", value => { notforloan => 0 } } ); + my $item_1 = $builder->build_sample_item( { biblionumber => $biblio->biblionumber, itype => $itype->itemtype, @@ -1254,10 +1381,10 @@ subtest 'RevertWaitingStatus' => sub { # Place a hold on the title for both patrons my $priority = 1; - my $hold_1 = place_item_hold( $patron_1, $item_1, $library, $priority ); - my $hold_2 = place_item_hold( $patron_2, $item_1, $library, $priority ); - my $hold_3 = place_item_hold( $patron_3, $item_1, $library, $priority ); - my $hold_4 = place_item_hold( $patron_4, $item_1, $library, $priority ); + my $hold_1 = place_item_hold( $patron_1, $item_1, $library, $priority ); + my $hold_2 = place_item_hold( $patron_2, $item_1, $library, $priority ); + my $hold_3 = place_item_hold( $patron_3, $item_1, $library, $priority ); + my $hold_4 = place_item_hold( $patron_4, $item_1, $library, $priority ); $hold_1->set_waiting; AddIssue( $patron_3, $item_1->barcode, undef, 'revert' ); @@ -1278,7 +1405,7 @@ subtest 'CheckReserves additional tests' => sub { plan tests => 8; - my $item = $builder->build_sample_item; + my $item = $builder->build_sample_item; my $reserve1 = $builder->build_object( { class => "Koha::Holds", @@ -1340,40 +1467,47 @@ subtest 'CheckReserves additional tests' => sub { } ); - ModReserveAffect( $item->itemnumber, $reserve1->borrowernumber, 1, - $reserve1->reserve_id ); - my ( $status, $matched_reserve, $possible_reserves ) = - CheckReserves( $item ); + ModReserveAffect( + $item->itemnumber, $reserve1->borrowernumber, 1, + $reserve1->reserve_id + ); + my ( $status, $matched_reserve, $possible_reserves ) = CheckReserves($item); is( $status, 'Transferred', "We found a reserve" ); - is( $matched_reserve->{reserve_id}, - $reserve1->reserve_id, "We got the Transit reserve" ); + is( + $matched_reserve->{reserve_id}, + $reserve1->reserve_id, "We got the Transit reserve" + ); is( scalar @$possible_reserves, 2, 'We do get both reserves' ); - my $patron_B = $builder->build_object({ class => "Koha::Patrons" }); - my $item_A = $builder->build_sample_item; - my $item_B = $builder->build_sample_item({ - homebranch => $patron_B->branchcode, - biblionumber => $item_A->biblionumber, - itype => $item_A->itype - }); + my $patron_B = $builder->build_object( { class => "Koha::Patrons" } ); + my $item_A = $builder->build_sample_item; + my $item_B = $builder->build_sample_item( + { + homebranch => $patron_B->branchcode, + biblionumber => $item_A->biblionumber, + itype => $item_A->itype + } + ); Koha::CirculationRules->set_rules( { branchcode => undef, categorycode => undef, itemtype => $item_A->itype, rules => { - reservesallowed => 25, + reservesallowed => 25, holds_per_record => 1, } } ); - Koha::CirculationRules->set_rule({ - branchcode => undef, - itemtype => $item_A->itype, - rule_name => 'holdallowed', - rule_value => 'from_home_library' - }); + Koha::CirculationRules->set_rule( + { + branchcode => undef, + itemtype => $item_A->itype, + rule_name => 'holdallowed', + rule_value => 'from_home_library' + } + ); my $reserve_id = AddReserve( { branchcode => $patron_B->branchcode, @@ -1384,32 +1518,35 @@ subtest 'CheckReserves additional tests' => sub { } ); - ok( $reserve_id, "We can place a record level hold because one item is owned by patron's home library"); - t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); - ( $status, $matched_reserve, $possible_reserves ) = CheckReserves( $item_A ); - is( $status, "", "We do not fill the hold with item A because it is not from the patron's homebranch"); - Koha::CirculationRules->set_rule({ - branchcode => $item_A->homebranch, - itemtype => $item_A->itype, - rule_name => 'holdallowed', - rule_value => 'from_any_library' - }); - ( $status, $matched_reserve, $possible_reserves ) = CheckReserves( $item_A ); - is( $status, "Reserved", "We fill the hold with item A because item's branch rule says allow any"); - + ok( $reserve_id, "We can place a record level hold because one item is owned by patron's home library" ); + t::lib::Mocks::mock_preference( 'ReservesControlBranch', 'ItemHomeLibrary' ); + ( $status, $matched_reserve, $possible_reserves ) = CheckReserves($item_A); + is( $status, "", "We do not fill the hold with item A because it is not from the patron's homebranch" ); + Koha::CirculationRules->set_rule( + { + branchcode => $item_A->homebranch, + itemtype => $item_A->itype, + rule_name => 'holdallowed', + rule_value => 'from_any_library' + } + ); + ( $status, $matched_reserve, $possible_reserves ) = CheckReserves($item_A); + is( $status, "Reserved", "We fill the hold with item A because item's branch rule says allow any" ); # Changing the control branch should change only the rule we get - t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary'); - ( $status, $matched_reserve, $possible_reserves ) = CheckReserves( $item_A ); - is( $status, "", "We do not fill the hold with item A because it is not from the patron's homebranch"); - Koha::CirculationRules->set_rule({ - branchcode => $patron_B->branchcode, - itemtype => $item_A->itype, - rule_name => 'holdallowed', - rule_value => 'from_any_library' - }); - ( $status, $matched_reserve, $possible_reserves ) = CheckReserves( $item_A ); - is( $status, "Reserved", "We fill the hold with item A because patron's branch rule says allow any"); + t::lib::Mocks::mock_preference( 'ReservesControlBranch', 'PatronLibrary' ); + ( $status, $matched_reserve, $possible_reserves ) = CheckReserves($item_A); + is( $status, "", "We do not fill the hold with item A because it is not from the patron's homebranch" ); + Koha::CirculationRules->set_rule( + { + branchcode => $patron_B->branchcode, + itemtype => $item_A->itype, + rule_name => 'holdallowed', + rule_value => 'from_any_library' + } + ); + ( $status, $matched_reserve, $possible_reserves ) = CheckReserves($item_A); + is( $status, "Reserved", "We fill the hold with item A because patron's branch rule says allow any" ); }; @@ -1419,46 +1556,64 @@ subtest 'AllowHoldOnPatronPossession test' => sub { # Create the items and patrons we need my $biblio = $builder->build_sample_biblio(); - my $itype = $builder->build_object({ class => "Koha::ItemTypes", value => { notforloan => 0 } }); - my $item = $builder->build_sample_item({ biblionumber => $biblio->biblionumber,notforloan => 0, itype => $itype->itemtype }); - my $patron = $builder->build_object({ class => "Koha::Patrons", - value => { branchcode => $item->homebranch }}); - - C4::Circulation::AddIssue($patron, - $item->barcode); - t::lib::Mocks::mock_preference('AllowHoldsOnPatronsPossessions', 0); - - is(C4::Reserves::CanBookBeReserved($patron->borrowernumber, - $item->biblionumber)->{status}, - 'alreadypossession', - 'Patron cannot place hold on a book loaned to itself'); - - is(C4::Reserves::CanItemBeReserved( $patron, $item )->{status}, - 'alreadypossession', - 'Patron cannot place hold on an item loaned to itself'); - - t::lib::Mocks::mock_preference('AllowHoldsOnPatronsPossessions', 1); - - is(C4::Reserves::CanBookBeReserved($patron->borrowernumber, - $item->biblionumber)->{status}, - 'OK', - 'Patron can place hold on a book loaned to itself'); - - is(C4::Reserves::CanItemBeReserved( $patron, $item )->{status}, - 'OK', - 'Patron can place hold on an item loaned to itself'); + my $itype = $builder->build_object( { class => "Koha::ItemTypes", value => { notforloan => 0 } } ); + my $item = $builder->build_sample_item( + { biblionumber => $biblio->biblionumber, notforloan => 0, itype => $itype->itemtype } ); + my $patron = $builder->build_object( + { + class => "Koha::Patrons", + value => { branchcode => $item->homebranch } + } + ); + + C4::Circulation::AddIssue( + $patron, + $item->barcode + ); + t::lib::Mocks::mock_preference( 'AllowHoldsOnPatronsPossessions', 0 ); + + is( + C4::Reserves::CanBookBeReserved( + $patron->borrowernumber, + $item->biblionumber + )->{status}, + 'alreadypossession', + 'Patron cannot place hold on a book loaned to itself' + ); + + is( + C4::Reserves::CanItemBeReserved( $patron, $item )->{status}, + 'alreadypossession', + 'Patron cannot place hold on an item loaned to itself' + ); + + t::lib::Mocks::mock_preference( 'AllowHoldsOnPatronsPossessions', 1 ); + + is( + C4::Reserves::CanBookBeReserved( + $patron->borrowernumber, + $item->biblionumber + )->{status}, + 'OK', + 'Patron can place hold on a book loaned to itself' + ); + + is( + C4::Reserves::CanItemBeReserved( $patron, $item )->{status}, + 'OK', + 'Patron can place hold on an item loaned to itself' + ); }; subtest 'MergeHolds' => sub { plan tests => 1; - my $biblio_1 = $builder->build_sample_biblio(); - my $biblio_2 = $builder->build_sample_biblio(); - my $library = $builder->build_object( { class => 'Koha::Libraries' } ); - my $itype = $builder->build_object( - { class => "Koha::ItemTypes", value => { notforloan => 0 } } ); - my $item_1 = $builder->build_sample_item( + my $biblio_1 = $builder->build_sample_biblio(); + my $biblio_2 = $builder->build_sample_biblio(); + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + my $itype = $builder->build_object( { class => "Koha::ItemTypes", value => { notforloan => 0 } } ); + my $item_1 = $builder->build_sample_item( { biblionumber => $biblio_1->biblionumber, itype => $itype->itemtype, @@ -1472,7 +1627,7 @@ subtest 'MergeHolds' => sub { place_item_hold( $patron_1, $item_1, $library, $priority ); # Move and make sure hold is now on $biblio_2 - C4::Reserves::MergeHolds($dbh, $biblio_2->biblionumber, $biblio_1->biblionumber); + C4::Reserves::MergeHolds( $dbh, $biblio_2->biblionumber, $biblio_1->biblionumber ); is( $biblio_2->holds->count, 1, 'Hold has been transferred' ); }; @@ -1480,7 +1635,7 @@ subtest 'ModReserveAffect logging' => sub { plan tests => 4; - my $item = $builder->build_sample_item; + my $item = $builder->build_sample_item; my $patron = $builder->build_object( { class => "Koha::Patrons", @@ -1488,8 +1643,8 @@ subtest 'ModReserveAffect logging' => sub { } ); - t::lib::Mocks::mock_userenv({ patron => $patron }); - t::lib::Mocks::mock_preference('HoldsLog', 1); + t::lib::Mocks::mock_userenv( { patron => $patron } ); + t::lib::Mocks::mock_preference( 'HoldsLog', 1 ); my $reserve_id = AddReserve( { @@ -1501,7 +1656,7 @@ subtest 'ModReserveAffect logging' => sub { } ); - my $hold = Koha::Holds->find($reserve_id); + my $hold = Koha::Holds->find($reserve_id); my $previous_timestamp = '1970-01-01 12:34:56'; $hold->timestamp($previous_timestamp)->store; @@ -1519,23 +1674,25 @@ subtest 'ModReserveAffect logging' => sub { ok( $hold->is_waiting, 'Hold has been set waiting' ); isnt( $hold->timestamp, $previous_timestamp, 'The timestamp has been modified' ); - my $log = Koha::ActionLogs->search({ module => 'HOLDS', action => 'MODIFY', object => $hold->reserve_id })->next; + my $log = Koha::ActionLogs->search( { module => 'HOLDS', action => 'MODIFY', object => $hold->reserve_id } )->next; my $expected = sprintf q{'timestamp' => '%s'}, $hold->timestamp; like( $log->info, qr{$expected}, 'Timestamp logged is the current one' ); }; sub count_hold_print_messages { - my $message_count = $dbh->selectall_arrayref(q{ + my $message_count = $dbh->selectall_arrayref( + q{ SELECT COUNT(*) FROM message_queue WHERE letter_code = 'HOLD' AND message_transport_type = 'print' - }); + } + ); return $message_count->[0]->[0]; } sub place_item_hold { - my ($patron,$item,$library,$priority) = @_; + my ( $patron, $item, $library, $priority ) = @_; my $hold_id = C4::Reserves::AddReserve( { @@ -1620,14 +1777,17 @@ subtest 'AddReserve() tests' => sub { my $biblio = $builder->build_sample_biblio; my $mock = Test::MockModule->new('Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue'); - $mock->mock( 'enqueue', sub { - my ( $self, $args ) = @_; - is_deeply( - $args->{biblio_ids}, - [ $biblio->id ], - "AddReserve triggers a holds queue update for the related biblio" - ); - } ); + $mock->mock( + 'enqueue', + sub { + my ( $self, $args ) = @_; + is_deeply( + $args->{biblio_ids}, + [ $biblio->id ], + "AddReserve triggers a holds queue update for the related biblio" + ); + } + ); t::lib::Mocks::mock_preference( 'RealTimeHoldsQueue', 1 ); @@ -1661,11 +1821,11 @@ subtest 'AlterPriorty() tests' => sub { $schema->storage->txn_begin; - my $library = $builder->build_object({ class => 'Koha::Libraries' }); - my $patron_1 = $builder->build_object({ class => 'Koha::Patrons' }); - my $patron_2 = $builder->build_object({ class => 'Koha::Patrons' }); - my $patron_3 = $builder->build_object({ class => 'Koha::Patrons' }); - my $biblio = $builder->build_sample_biblio; + my $library = $builder->build_object( { class => 'Koha::Libraries' } ); + my $patron_1 = $builder->build_object( { class => 'Koha::Patrons' } ); + my $patron_2 = $builder->build_object( { class => 'Koha::Patrons' } ); + my $patron_3 = $builder->build_object( { class => 'Koha::Patrons' } ); + my $biblio = $builder->build_sample_biblio; my $reserve_id = AddReserve( { @@ -1690,14 +1850,17 @@ subtest 'AlterPriorty() tests' => sub { ); my $mock = Test::MockModule->new('Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue'); - $mock->mock( 'enqueue', sub { - my ( $self, $args ) = @_; - is_deeply( - $args->{biblio_ids}, - [ $biblio->id ], - "AlterPriority triggers a holds queue update for the related biblio" - ); - } ); + $mock->mock( + 'enqueue', + sub { + my ( $self, $args ) = @_; + is_deeply( + $args->{biblio_ids}, + [ $biblio->id ], + "AlterPriority triggers a holds queue update for the related biblio" + ); + } + ); t::lib::Mocks::mock_preference( 'RealTimeHoldsQueue', 1 ); @@ -1705,7 +1868,7 @@ subtest 'AlterPriorty() tests' => sub { my $hold = Koha::Holds->find($reserve_id); - is($hold->priority,3,'Successfully altered priority to bottom'); + is( $hold->priority, 3, 'Successfully altered priority to bottom' ); t::lib::Mocks::mock_preference( 'RealTimeHoldsQueue', 0 ); @@ -1720,16 +1883,13 @@ subtest 'CanBookBeReserved() tests' => sub { $schema->storage->txn_begin; - my $library = $builder->build_object( - { class => 'Koha::Libraries', value => { pickup_location => 1 } } ); - my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); - my $itype = $builder->build_object( { class => 'Koha::ItemTypes' } ); + my $library = $builder->build_object( { class => 'Koha::Libraries', value => { pickup_location => 1 } } ); + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $itype = $builder->build_object( { class => 'Koha::ItemTypes' } ); my $biblio = $builder->build_sample_biblio(); - my $item_1 = $builder->build_sample_item( - { biblionumber => $biblio->id, itype => $itype->id } ); - my $item_2 = $builder->build_sample_item( - { biblionumber => $biblio->id, itype => $itype->id } ); + my $item_1 = $builder->build_sample_item( { biblionumber => $biblio->id, itype => $itype->id } ); + my $item_2 = $builder->build_sample_item( { biblionumber => $biblio->id, itype => $itype->id } ); Koha::CirculationRules->delete; Koha::CirculationRules->set_rules( @@ -1765,10 +1925,14 @@ subtest 'CanBookBeReserved() tests' => sub { ## Limit on item type is 2, only one hold, success tests - my $res = CanBookBeReserved( $patron->id, $biblio->id, $library->id, - { itemtype => $itype->id } ); - is_deeply( $res, { status => 'OK' }, - 'Holds on itemtype limit not reached' ); + my $res = CanBookBeReserved( + $patron->id, $biblio->id, $library->id, + { itemtype => $itype->id } + ); + is_deeply( + $res, { status => 'OK' }, + 'Holds on itemtype limit not reached' + ); # Add a second hold, biblio-level and item type-constrained C4::Reserves::AddReserve( @@ -1783,8 +1947,10 @@ subtest 'CanBookBeReserved() tests' => sub { ## Limit on item type is 2, two holds, one of them biblio-level/item type-constrained - $res = CanBookBeReserved( $patron->id, $biblio->id, $library->id, - { itemtype => $itype->id } ); + $res = CanBookBeReserved( + $patron->id, $biblio->id, $library->id, + { itemtype => $itype->id } + ); is_deeply( $res, { status => '' }, 'Holds on itemtype limit reached' ); $schema->storage->txn_rollback; @@ -1801,12 +1967,13 @@ subtest 'CanItemBeReserved() tests' => sub { my $itype = $builder->build_object( { class => 'Koha::ItemTypes' } ); my $biblio = $builder->build_sample_biblio(); - my $item_1 = $builder->build_sample_item({ biblionumber => $biblio->id, itype => $itype->id }); - my $item_2 = $builder->build_sample_item({ biblionumber => $biblio->id, itype => $itype->id }); + my $item_1 = $builder->build_sample_item( { biblionumber => $biblio->id, itype => $itype->id } ); + my $item_2 = $builder->build_sample_item( { biblionumber => $biblio->id, itype => $itype->id } ); Koha::CirculationRules->delete; Koha::CirculationRules->set_rules( - { branchcode => undef, + { + branchcode => undef, categorycode => undef, itemtype => undef, rules => { @@ -1815,7 +1982,8 @@ subtest 'CanItemBeReserved() tests' => sub { } ); Koha::CirculationRules->set_rules( - { branchcode => undef, + { + branchcode => undef, categorycode => undef, itemtype => $itype->id, rules => { @@ -1862,24 +2030,26 @@ subtest 'DefaultHoldExpiration tests' => sub { plan tests => 2; $schema->storage->txn_begin; - t::lib::Mocks::mock_preference( 'DefaultHoldExpirationdate', 1 ); + t::lib::Mocks::mock_preference( 'DefaultHoldExpirationdate', 1 ); t::lib::Mocks::mock_preference( 'DefaultHoldExpirationdatePeriod', 365 ); t::lib::Mocks::mock_preference( 'DefaultHoldExpirationUnitOfTime', 'days;' ); - my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); - my $item = $builder->build_sample_item(); + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $item = $builder->build_sample_item(); - my $reserve_id = AddReserve({ - branchcode => $item->homebranch, - borrowernumber => $patron->id, - biblionumber => $item->biblionumber, - }); + my $reserve_id = AddReserve( + { + branchcode => $item->homebranch, + borrowernumber => $patron->id, + biblionumber => $item->biblionumber, + } + ); my $today = dt_from_string(); - my $hold = Koha::Holds->find( $reserve_id ); + my $hold = Koha::Holds->find($reserve_id); - is( $hold->reservedate, $today->ymd, "Hold created today" ); - is( $hold->expirationdate, $today->add( days => 365)->ymd, "Reserve date set 1 year from today" ); + is( $hold->reservedate, $today->ymd, "Hold created today" ); + is( $hold->expirationdate, $today->add( days => 365 )->ymd, "Reserve date set 1 year from today" ); $schema->txn_rollback; }; @@ -1912,8 +2082,8 @@ subtest '_Findgroupreserves' => sub { C4::HoldsQueue::AddToHoldTargetMap( { $item->id => { - borrowernumber => $patron_1->id, biblionumber => $item->biblionumber, - holdingbranch => $item->holdingbranch, item_level => 0, reserve_id => $reserve_id_1 + borrowernumber => $patron_1->id, biblionumber => $item->biblionumber, + holdingbranch => $item->holdingbranch, item_level => 0, reserve_id => $reserve_id_1 } } ); @@ -1951,8 +2121,6 @@ subtest '_Findgroupreserves' => sub { is( scalar @reserves, 1, "We should still only get the item level hold that is in the map" ); is( $reserves[0]->{reserve_id}, $reserve_id_1, "We got the expected reserve which has been updated" ); - - $schema->txn_rollback; }; -- 2.30.2