From 5f485e476b2c5521d59bda6d48b56ed36cff3f86 Mon Sep 17 00:00:00 2001 From: Kyle M Hall Date: Fri, 10 Aug 2018 10:36:36 -0400 Subject: [PATCH] Bug 15524: (QA follow-up) Change Can[Book|Item]BeReserved to return hashref, pass limit to template Signed-off-by: Nick Clemens --- C4/ILSDI/Services.pm | 6 ++-- C4/Reserves.pm | 36 +++++++++---------- Koha/REST/V1/Hold.pm | 2 +- opac/opac-reserve.pl | 8 ++--- reserve/request.pl | 21 +++++------ t/db_dependent/Holds.t | 40 ++++++++++----------- t/db_dependent/Reserves.t | 6 ++-- t/db_dependent/Reserves/MultiplePerRecord.t | 6 ++-- 8 files changed, 63 insertions(+), 62 deletions(-) diff --git a/C4/ILSDI/Services.pm b/C4/ILSDI/Services.pm index 9498a77db0..53292a55b1 100644 --- a/C4/ILSDI/Services.pm +++ b/C4/ILSDI/Services.pm @@ -542,7 +542,7 @@ sub GetServices { # Reserve level management my $biblionumber = $item->{'biblionumber'}; my $canbookbereserved = CanBookBeReserved( $borrower, $biblionumber ); - if ($canbookbereserved eq 'OK') { + if ($canbookbereserved->{status} eq 'OK') { push @availablefor, 'title level hold'; my $canitembereserved = IsAvailableForItemLevelRequest($item, $borrower); if ($canitembereserved) { @@ -662,7 +662,7 @@ sub HoldTitle { my $title = $biblio ? $biblio->title : ''; # Check if the biblio can be reserved - return { code => 'NotHoldable' } unless CanBookBeReserved( $borrowernumber, $biblionumber ) eq 'OK'; + return { code => 'NotHoldable' } unless CanBookBeReserved( $borrowernumber, $biblionumber )->{status} eq 'OK'; my $branch; @@ -740,7 +740,7 @@ sub HoldItem { # Check for item disponibility my $canitembereserved = C4::Reserves::CanItemBeReserved( $borrowernumber, $itemnumber ); my $canbookbereserved = C4::Reserves::CanBookBeReserved( $borrowernumber, $biblionumber ); - return { code => 'NotHoldable' } unless $canbookbereserved eq 'OK' and $canitembereserved eq 'OK'; + return { code => 'NotHoldable' } unless $canbookbereserved->{status} eq 'OK' and $canitembereserved->{status} eq 'OK'; # Pickup branch management my $branch; diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 16ad20df77..38f9953c09 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -285,7 +285,7 @@ sub CanBookBeReserved{ my $canReserve; foreach my $item (@$items) { $canReserve = CanItemBeReserved( $borrowernumber, $item ); - return 'OK' if $canReserve eq 'OK'; + return $canReserve if $canReserve->{status} eq 'OK'; } return $canReserve; } @@ -293,14 +293,14 @@ sub CanBookBeReserved{ =head2 CanItemBeReserved $canReserve = &CanItemBeReserved($borrowernumber, $itemnumber) - if ($canReserve eq 'OK') { #We can reserve this Item! } + if ($canReserve->{status} eq 'OK') { #We can reserve this Item! } -@RETURNS OK, if the Item can be reserved. - ageRestricted, if the Item is age restricted for this borrower. - damaged, if the Item is damaged. - cannotReserveFromOtherBranches, if syspref 'canreservefromotherbranches' is OK. - tooManyReserves, if the borrower has exceeded his maximum reserve amount. - notReservable, if holds on this item are not allowed +@RETURNS { status => OK }, if the Item can be reserved. + { status => ageRestricted }, if the Item is age restricted for this borrower. + { status => damaged }, if the Item is damaged. + { status => cannotReserveFromOtherBranches }, if syspref 'canreservefromotherbranches' is OK. + { status => tooManyReserves, limit => $limit }, if the borrower has exceeded his maximum reserve amount. + { status => notReservable }, if holds on this item are not allowed =cut @@ -320,17 +320,17 @@ 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 'damaged' + return { status =>'damaged' } if ( $item->{damaged} && !C4::Context->preference('AllowHoldsOnDamagedItems') ); # Check for the age restriction my ( $ageRestriction, $daysToAgeRestriction ) = C4::Circulation::GetAgeRestriction( $biblio->biblioitem->agerestriction, $borrower ); - return 'ageRestricted' if $daysToAgeRestriction && $daysToAgeRestriction > 0; + return { status => 'ageRestricted' } if $daysToAgeRestriction && $daysToAgeRestriction > 0; # Check that the patron doesn't have an item level hold on this item already - return 'itemAlreadyOnHold' + return { status =>'itemAlreadyOnHold' } if Koha::Holds->search( { borrowernumber => $borrowernumber, itemnumber => $itemnumber } )->count(); my $controlbranch = C4::Context->preference('ReservesControlBranch'); @@ -375,7 +375,7 @@ sub CanItemBeReserved { } ); if ( $holds->count() >= $holds_per_record ) { - return "tooManyHoldsForThisRecord"; + return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record }; } # we retrieve count @@ -406,7 +406,7 @@ sub CanItemBeReserved { # we check if it's ok or not if ( $reservecount >= $allowedreserves ) { - return 'tooManyReserves'; + return { status => 'tooManyReserves', limit => $allowedreserves }; } # Now we need to check hold limits by patron category @@ -429,7 +429,7 @@ sub CanItemBeReserved { } )->count(); - return 'tooManyReserves' if $total_holds_count >= $rule->max_holds; + return { status => 'tooManyReserves', limit => $rule->max_holds } if $total_holds_count >= $rule->max_holds; } my $circ_control_branch = @@ -438,13 +438,13 @@ sub CanItemBeReserved { C4::Circulation::GetBranchItemRule( $circ_control_branch, $item->itype ); if ( $branchitemrule->{holdallowed} == 0 ) { - return 'notReservable'; + return { status => 'notReservable' }; } if ( $branchitemrule->{holdallowed} == 1 && $borrower->{branchcode} ne $item->homebranch ) { - return 'cannotReserveFromOtherBranches'; + return { status => 'cannotReserveFromOtherBranches' }; } # If reservecount is ok, we check item branch if IndependentBranches is ON @@ -454,11 +454,11 @@ sub CanItemBeReserved { { my $itembranch = $item->homebranch; if ( $itembranch ne $borrower->{branchcode} ) { - return 'cannotReserveFromOtherBranches'; + return { status => 'cannotReserveFromOtherBranches' }; } } - return 'OK'; + return { status => 'OK' }; } =head2 CanReserveBeCanceledFromOpac diff --git a/Koha/REST/V1/Hold.pm b/Koha/REST/V1/Hold.pm index 2833f39f19..c152fb71bb 100644 --- a/Koha/REST/V1/Hold.pm +++ b/Koha/REST/V1/Hold.pm @@ -89,7 +89,7 @@ sub add { ? CanItemBeReserved( $borrowernumber, $itemnumber ) : CanBookBeReserved( $borrowernumber, $biblionumber ); - unless ($can_reserve eq 'OK') { + unless ($can_reserve->{status} eq 'OK') { return $c->render( status => 403, openapi => { error => "Reserve cannot be placed. Reason: $can_reserve" } ); diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index a9af8d168a..92989a219d 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -269,10 +269,10 @@ if ( $query->param('place_reserve') ) { my $rank = $biblioData->{rank}; if ( $itemNum ne '' ) { - $canreserve = 1 if CanItemBeReserved( $borrowernumber, $itemNum ) eq 'OK'; + $canreserve = 1 if CanItemBeReserved( $borrowernumber, $itemNum )->{status} eq 'OK'; } else { - $canreserve = 1 if CanBookBeReserved( $borrowernumber, $biblioNum ) eq 'OK'; + $canreserve = 1 if CanBookBeReserved( $borrowernumber, $biblioNum )->{status} eq 'OK'; # Inserts a null into the 'itemnumber' field of 'reserves' table. $itemNum = undef; @@ -525,7 +525,7 @@ foreach my $biblioNum (@biblionumbers) { my $policy_holdallowed = !$itemLoopIter->{already_reserved}; $policy_holdallowed &&= IsAvailableForItemLevelRequest($itemInfo,$patron_unblessed) && - CanItemBeReserved($borrowernumber,$itemNum) eq 'OK'; + CanItemBeReserved($borrowernumber,$itemNum)->{status} eq 'OK'; if ($policy_holdallowed) { my $opac_hold_policy = Koha::IssuingRules->get_opacitemholds_policy( { item => $item, patron => $patron } ); @@ -585,7 +585,7 @@ foreach my $biblioNum (@biblionumbers) { } } - $biblioLoopIter{holdable} &&= CanBookBeReserved($borrowernumber,$biblioNum) eq 'OK'; + $biblioLoopIter{holdable} &&= CanBookBeReserved($borrowernumber,$biblioNum)->{status} eq 'OK'; # For multiple holds per record, if a patron has previously placed a hold, # the patron can only place more holds of the same type. That is, if the diff --git a/reserve/request.pl b/reserve/request.pl index 2902de09ae..2fd36e8e75 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -211,24 +211,25 @@ foreach my $biblionumber (@biblionumbers) { if ( $patron ) { { # CanBookBeReserved my $canReserve = CanBookBeReserved( $patron->borrowernumber, $biblionumber ); - $canReserve //= ''; - if ( $canReserve eq 'OK' ) { + $canReserve->{status} //= ''; + if ( $canReserve->{status} eq 'OK' ) { #All is OK and we can continue } - elsif ( $canReserve eq 'tooManyReserves' ) { + elsif ( $canReserve->{status} eq 'tooManyReserves' ) { $exceeded_maxreserves = 1; + $template->param( maxreserves => $canReserve->{limit} ); } - elsif ( $canReserve eq 'tooManyHoldsForThisRecord' ) { + elsif ( $canReserve->{status} eq 'tooManyHoldsForThisRecord' ) { $exceeded_holds_per_record = 1; - $biblioloopiter{$canReserve} = 1; + $biblioloopiter{ $canReserve->{status} } = 1; } - elsif ( $canReserve eq 'ageRestricted' ) { - $template->param( $canReserve => 1 ); - $biblioloopiter{$canReserve} = 1; + elsif ( $canReserve->{status} eq 'ageRestricted' ) { + $template->param( $canReserve->{status} => 1 ); + $biblioloopiter{ $canReserve->{status} } = 1; } else { - $biblioloopiter{$canReserve} = 1; + $biblioloopiter{ $canReserve->{status} } = 1; } } @@ -462,7 +463,7 @@ foreach my $biblionumber (@biblionumbers) { $item->{'holdallowed'} = $branchitemrule->{'holdallowed'}; my $can_item_be_reserved = CanItemBeReserved( $patron->borrowernumber, $itemnumber ); - $item->{not_holdable} = $can_item_be_reserved unless ( $can_item_be_reserved eq 'OK' ); + $item->{not_holdable} = $can_item_be_reserved->{status} unless ( $can_item_be_reserved->{status} eq 'OK' ); $item->{item_level_holds} = Koha::IssuingRules->get_opacitemholds_policy( { item => $item_object, patron => $patron } ); diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 8f1eaf2a0a..27151ae84f 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -7,7 +7,7 @@ use t::lib::TestBuilder; use C4::Context; -use Test::More tests => 56; +use Test::More tests => 55; use MARC::Record; use Koha::Patrons; use C4::Items; @@ -261,7 +261,7 @@ t::lib::Mocks::mock_preference('item-level_itypes', 1); # if IndependentBranches is OFF, a $branch_1 patron can reserve an $branch_2 item t::lib::Mocks::mock_preference('IndependentBranches', 0); ok( - CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber) eq 'OK', + CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber)->{status} eq 'OK', '$branch_1 patron allowed to reserve $branch_2 item with IndependentBranches OFF (bug 2394)' ); @@ -269,14 +269,14 @@ ok( t::lib::Mocks::mock_preference('IndependentBranches', 1); t::lib::Mocks::mock_preference('canreservefromotherbranches', 0); ok( - CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber) eq 'cannotReserveFromOtherBranches', + CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber)->{status} eq 'cannotReserveFromOtherBranches', '$branch_1 patron NOT allowed to reserve $branch_2 item with IndependentBranches ON ... (bug 2394)' ); # ... unless canreservefromotherbranches is ON t::lib::Mocks::mock_preference('canreservefromotherbranches', 1); ok( - CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber) eq 'OK', + CanItemBeReserved($borrowernumbers[0], $foreign_itemnumber)->{status} eq 'OK', '... unless canreservefromotherbranches is ON (bug 2394)' ); @@ -299,7 +299,7 @@ ok( ModItem({ damaged => 1 }, $item_bibnum, $itemnumber); t::lib::Mocks::mock_preference( 'AllowHoldsOnDamagedItems', 1 ); -is( CanItemBeReserved( $borrowernumbers[0], $itemnumber), 'OK', "Patron can reserve damaged item with AllowHoldsOnDamagedItems enabled" ); +is( CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status}, 'OK', "Patron can reserve damaged item with AllowHoldsOnDamagedItems enabled" ); ok( defined( ( CheckReserves($itemnumber) )[1] ), "Hold can be trapped for damaged item with AllowHoldsOnDamagedItems enabled" ); $hold = Koha::Hold->new( @@ -309,13 +309,13 @@ $hold = Koha::Hold->new( biblionumber => $item_bibnum, } )->store(); -is( CanItemBeReserved( $borrowernumbers[0], $itemnumber ), +is( CanItemBeReserved( $borrowernumbers[0], $itemnumber )->{status}, 'itemAlreadyOnHold', "Patron cannot place a second item level hold for a given item" ); $hold->delete(); t::lib::Mocks::mock_preference( 'AllowHoldsOnDamagedItems', 0 ); -ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'damaged', "Patron cannot reserve damaged item with AllowHoldsOnDamagedItems disabled" ); +ok( CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} eq 'damaged', "Patron cannot reserve damaged item with AllowHoldsOnDamagedItems disabled" ); ok( !defined( ( CheckReserves($itemnumber) )[1] ), "Hold cannot be trapped for damaged item with AllowHoldsOnDamagedItems disabled" ); # Regression test for bug 9532 @@ -329,19 +329,19 @@ AddReserve( 1, ); is( - CanItemBeReserved( $borrowernumbers[0], $itemnumber), 'tooManyReserves', + CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status}, 'tooManyReserves', "cannot request item if policy that matches on item-level item type forbids it" ); ModItem({ itype => 'CAN' }, $item_bibnum, $itemnumber); ok( - CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'OK', + CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} eq 'OK', "can request item if policy that matches on item type allows it" ); t::lib::Mocks::mock_preference('item-level_itypes', 0); ModItem({ itype => undef }, $item_bibnum, $itemnumber); ok( - CanItemBeReserved( $borrowernumbers[0], $itemnumber) eq 'tooManyReserves', + CanItemBeReserved( $borrowernumbers[0], $itemnumber)->{status} eq 'tooManyReserves', "cannot request item if policy that matches on bib-level item type forbids it (bug 9532)" ); @@ -370,18 +370,18 @@ $dbh->do(q{ ($bibnum, $title, $bibitemnum) = create_helper_biblio('CANNOT'); ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem( { homebranch => $branch_1, holdingbranch => $branch_1, itype => 'CANNOT' } , $bibnum); -is(CanItemBeReserved($borrowernumbers[0], $itemnumber), 'notReservable', +is(CanItemBeReserved($borrowernumbers[0], $itemnumber)->{status}, 'notReservable', "CanItemBeReserved should return 'notReservable'"); ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem( { homebranch => $branch_2, holdingbranch => $branch_1, itype => 'CAN' } , $bibnum); -is(CanItemBeReserved($borrowernumbers[0], $itemnumber), +is(CanItemBeReserved($borrowernumbers[0], $itemnumber)->{status}, 'cannotReserveFromOtherBranches', "CanItemBeReserved should return 'cannotReserveFromOtherBranches'"); ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem( { homebranch => $branch_1, holdingbranch => $branch_1, itype => 'CAN' } , $bibnum); -is(CanItemBeReserved($borrowernumbers[0], $itemnumber), 'OK', +is(CanItemBeReserved($borrowernumbers[0], $itemnumber)->{status}, 'OK', "CanItemBeReserved should return 'OK'"); # Bug 12632 @@ -403,12 +403,12 @@ $dbh->do( {}, '*', '*', 'ONLY1', 1, 99 ); -is( CanItemBeReserved( $borrowernumbers[0], $itemnumber ), +is( CanItemBeReserved( $borrowernumbers[0], $itemnumber )->{status}, 'OK', 'Patron can reserve item with hold limit of 1, no holds placed' ); my $res_id = AddReserve( $branch_1, $borrowernumbers[0], $bibnum, '', 1, ); -is( CanItemBeReserved( $borrowernumbers[0], $itemnumber ), +is( CanItemBeReserved( $borrowernumbers[0], $itemnumber )->{status}, 'tooManyReserves', 'Patron cannot reserve item with hold limit of 1, 1 bib level hold placed' ); subtest 'Test max_holds per library/patron category' => sub { @@ -438,7 +438,7 @@ subtest 'Test max_holds per library/patron category' => sub { is( $count, 3, 'Patron now has 3 holds' ); my $ret = CanItemBeReserved( $borrowernumbers[0], $itemnumber ); - is( $ret, 'OK', 'Patron can place hold with no borrower circ rules' ); + is( $ret->{status}, 'OK', 'Patron can place hold with no borrower circ rules' ); my $rule_all = $schema->resultset('DefaultBorrowerCircRule')->new( { @@ -456,19 +456,19 @@ subtest 'Test max_holds per library/patron category' => sub { )->insert(); $ret = CanItemBeReserved( $borrowernumbers[0], $itemnumber ); - is( $ret, 'OK', 'Patron can place hold with branch/category rule of 5, category rule of 3' ); + is( $ret->{status}, 'OK', 'Patron can place hold with branch/category rule of 5, category rule of 3' ); $rule_branch->delete(); $ret = CanItemBeReserved( $borrowernumbers[0], $itemnumber ); - is( $ret, 'tooManyReserves', 'Patron cannot place hold with only a category rule of 3' ); + is( $ret->{status}, 'tooManyReserves', 'Patron cannot place hold with only a category rule of 3' ); $rule_all->delete(); $rule_branch->max_holds(3); $rule_branch->insert(); $ret = CanItemBeReserved( $borrowernumbers[0], $itemnumber ); - is( $ret, 'tooManyReserves', 'Patron cannot place hold with only a branch/category rule of 3' ); + is( $ret->{status}, 'tooManyReserves', 'Patron cannot place hold with only a branch/category rule of 3' ); $rule_branch->max_holds(5); $rule_branch->update(); @@ -476,7 +476,7 @@ subtest 'Test max_holds per library/patron category' => sub { $rule_all->insert(); $ret = CanItemBeReserved( $borrowernumbers[0], $itemnumber ); - is( $ret, 'OK', 'Patron can place hold with branch/category rule of 5, category rule of 5' ); + is( $ret->{status}, 'OK', 'Patron can place hold with branch/category rule of 5, category rule of 5' ); }; # Helper method to set up a Biblio. diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 406e45036e..9fe293df34 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -522,19 +522,19 @@ my ( $ageres_tagid, $ageres_subfieldid ) = GetMarcFromKohaField( "biblioitems.ag $record->append_fields( MARC::Field->new($ageres_tagid, '', '', $ageres_subfieldid => 'PEGI 16') ); C4::Biblio::ModBiblio( $record, $bibnum, $frameworkcode ); -is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , '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; -is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , '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; -is( C4::Reserves::CanBookBeReserved($borrowernumber, $biblionumber) , '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"); #### ####### EO Bug 13113 <<< #### diff --git a/t/db_dependent/Reserves/MultiplePerRecord.t b/t/db_dependent/Reserves/MultiplePerRecord.t index 70f26001dc..7f639ce459 100755 --- a/t/db_dependent/Reserves/MultiplePerRecord.t +++ b/t/db_dependent/Reserves/MultiplePerRecord.t @@ -277,17 +277,17 @@ $rule = $rules_rs->new( )->insert(); my $can = CanBookBeReserved($patron->{borrowernumber}, $biblio->{biblionumber}); -is( $can, 'OK', 'Hold can be placed with 0 holds' ); +is( $can->{status}, 'OK', 'Hold can be placed with 0 holds' ); my $hold_id = AddReserve( $library->{branchcode}, $patron->{borrowernumber}, $biblio->{biblionumber}, '', 1 ); ok( $hold_id, 'First hold was placed' ); $can = CanBookBeReserved($patron->{borrowernumber}, $biblio->{biblionumber}); -is( $can, 'OK', 'Hold can be placed with 1 hold' ); +is( $can->{status}, 'OK', 'Hold can be placed with 1 hold' ); $hold_id = AddReserve( $library->{branchcode}, $patron->{borrowernumber}, $biblio->{biblionumber}, '', 1 ); ok( $hold_id, 'Second hold was placed' ); $can = CanBookBeReserved($patron->{borrowernumber}, $biblio->{biblionumber}); -is( $can, 'tooManyHoldsForThisRecord', 'Third hold exceeds limit of holds per record' ); +is( $can->{status}, 'tooManyHoldsForThisRecord', 'Third hold exceeds limit of holds per record' ); $schema->storage->txn_rollback; -- 2.39.5