From d940c340f27634ef71e42db85ce996735854c4bb Mon Sep 17 00:00:00 2001 From: Alex Buckley Date: Thu, 4 Jul 2019 14:30:21 +0000 Subject: [PATCH] Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests This patch adds to the unit test coverage of the new Koha::Hold->check_if_existing_hold_matches_issuingrules(). In patch two on bug 23172 I added unit test for directly testing the new subroutine Koha::Hold->check_if_existing_hold_matches_issuingrules(). In this patch I fix the test failures in t/db_dependent/Circulation.t and t/db_dependent/Reserves.t and add a test to confirm C4::Circulation->AddReturn() works correctly with Koha::Hold->check_if_existing_hold_matches_issuingrules(). This is because when an item is returned C4::Circulation->AddReturn() calls C4::Reserves->CheckReserves() which in turn calls the aforementioned new subroutine. So the additional unit tests in this patch check all these subroutines work correctly together. Test plan: 1. Apply first two patches and follow their respective test plans 2. Enter Koha shell: sudo koha-shell 3. Run (from your koha home directory): prove t/db_dependent/Circulation.t Confirm it fails 4. Run: prove t/db_dependent/Reserves.t Confirm it fails 5. Apply this patch 6. Repeat steps 3 and 4 and confirm the tests pass 6. Confirm t/db_dependent/Holds.t still passes by repeating step 2 and then running: prove t/db_dependent/Holds.t Sponsored-By: Brimbank Library, Australia Signed-off-by: Hayley Mapley --- C4/Reserves.pm | 3 -- t/db_dependent/.Reserves.t.swp | Bin 0 -> 16384 bytes t/db_dependent/Circulation.t | 22 ++++++++++ t/db_dependent/Holds.t | 5 ++- t/db_dependent/Reserves.t | 93 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 119 insertions(+), 4 deletions(-) create mode 100644 t/db_dependent/.Reserves.t.swp diff --git a/C4/Reserves.pm b/C4/Reserves.pm index c7a93d11454..5af75d967c0 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -853,7 +853,6 @@ sub CheckReserves { my $priority = 10000000; foreach my $res (@reserves) { - #Before allocating to bib-level hold check if reserve respects hold rule, i.e. can patron category/item type combination allow reserves my $checkreserve = Koha::Hold->check_if_existing_hold_matches_issuingrules( $res->{'borrowernumber'}, $itemnumber ); next unless ($checkreserve eq 'OK'); @@ -910,14 +909,12 @@ sub CheckReserves { } } } - # If we get this far, then no exact match was found. # We return the most important (i.e. next) reservation. if ($highest) { $highest->{'itemnumber'} = $item; return ( "Reserved", $highest, \@reserves ); } - return ( '' ); } diff --git a/t/db_dependent/.Reserves.t.swp b/t/db_dependent/.Reserves.t.swp new file mode 100644 index 0000000000000000000000000000000000000000..9f758e243ceee242474197ad82836bae00e9714e GIT binary patch literal 16384 zcmeHNTW=&s6>cCTBxFfMlouSKTH9!5#n@x-W)m2%9mbd4oy7L8#$y~4c3XX^@wD92 zJzHJvS;t!7m5cI#5J=&o0OBD)6h#Uz2yy)b2nimzi3jq8!bXZhNJxP1R8`My9WR#` zgnCw=XS>d+Q>VT`TmFbWH08O5&7SZJTm2f9Ff0iVME*A;zhvaEj>um( z^2dz4VLFuAf6d67M*i%G{I5oS*~ssGhyG7i|5YQe8F?}y|BI1t8Tk)JA_1i4# zHQ-OcZ-FlWtH3S5%ePwAw}35R8JGlq^-jzBCGch7Nnjp01KbUK9q;x}0TQ?bYy*!1 zTfl?BeZY5cAnLW9pGW$1aK#CJMev+VI;r;Fb2GdF;wHIBB8y{CS6B(vCo3=j;0pU$PIata9^wFVkkU< zUv*R}7nN2rwa^SAzX)r_f@$Hzuw7F+TzP6C;@#rMKj1skIWe`6$XM{Hg<(T;)?jm$ z%911bPAmeRwLIzRl6`xYY;ajTU_26W*v1b{nw+!w1vYanGe2j+F=ENZFJ8QsA`J?Y z`q7>(DPd-^C(a5og(MY`(^j?%AlEz=bnRv!5}An^$$KTxz@zFgh%8zYY)NqV&m5L@ z8(|el-UbhPOpAj<+3N08Fo{d7+j3+f zVY0|@T~CR2Tz}xEzH5ewb&1u`;2^=W1v?C(N1Q38Uc9C z5SqlP@H8WfD%1)CVP|&(*|MY9CNmUcZW@F>7w9GZr+8o~<-?_R;Q55?$;QjH0+g4-ZsE zolwDgzy_$VfnVxlp~Uu_FjXBtpwK{n!q|z3oD{#oYO-ti2VV}T+f?J4p0L^DjUA5|JIsz*VseC(DA6~v`^j4 z_@dIw932=02hJO-fE4FBY=jajtXj*Aa6fN|1SGDK)9t z)-$S;A-hs8Lqx-4761%Tmi{PO7qevn8v`iz#;35nvbxe(VQ1?X)|eLvFAW`{+ZHLd zCa6-`yGK}mr&Bd(J` zNtb0BS$Qv{mV1h{i_t?Q9Ra?PIub{P1C~dXqXoZ`27=r1Zm~fAWxSE+i!0}9>+Aqq zP^`%&TKhe$QwQe;WP0Bg$e;( zd97MsB3ivzV7sbR4~Z+Sn+eoa5BxR_l%0(&lQl&Mt1& zFJ9PKnZ{zHG6vdS#o>L6OsbqW)S&C9kfItk;lddB$RV4KV zl_~To_xJ&2Q#X~+2y}&3j4hm`+=q154Z@J}7{Z~Mh9w3~)zu=z``CoCxI?Mo1d{GP zqktD|4?%|_>LIm%R>0$pxGCWPo?>i>bSN>7!RqU?QyI&K(h>-P4cb6qcL+-`-cB;5 zBe-@F?7A^H39>44_Xm30BwrHa%N%t8$v47i_i3O`V*x2+FtEDn&A94u1G{&;TZCW~ z_9^vXOHVXnUp{fXoF<~|22mNC+t8x-|EKXzJ&Sj8djB6&?~a!B0^aj~0(>6$48Q>o zaDi3eK41*Ej(7hTfaie6fpft7f$OmML*RMf8Q{CXr+^3ufn8t&pmt|~{F5_~GmtZo zGmtZoGmtZoGmtZoGmtazKgYn~heo(p3SGzB)#p83dM#H+W$JAZH(_|g>!111c^I2) z;qXM?InfyvJ@t-|)VYeeK~oYt@Npufj*Rfl%txOgWlwbM*t^EVMILTDsAy`TFVOc; zSp^wLydw5Ks4%XJ=#B$7WaGo?9TDJu4R7W8L-w%%N|^&^kn@y2NtPibc0lLESMd;RVuj1Fb_pdHu zEM-lUdfh((Gas?%EF9x99Hn{sO3GG4dN4ZdZCst=tkL#xynutq$7U|6KYCQNOEYq_ zy1q=#8g+Ge*olR-)m0U);y`{R3l^!;XY2e*eT9X%h~omrap?=4^A}c^3(Qgq_@jset_rules( auto_renew => 0, fine => .10, chargeperiod => 1, + holds_per_day => 12, + holds_per_record => 1, } } ); @@ -1559,6 +1561,7 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { branchcode => undef, rules => { onshelfholds => 0, + holds_per_day => 1 } } ); @@ -3339,6 +3342,22 @@ subtest 'Set waiting flag' => sub { my $library_2 = $builder->build( { source => 'Branch' } ); my $patron_2 = $builder->build( { source => 'Borrower', value => { branchcode => $library_2->{branchcode}, categorycode => $patron_category->{categorycode} } } ); + my $biblio = $builder->build( { source => 'Biblio' } ); + my $biblioitem = $builder->build( { source => 'Biblioitem', value => { biblionumber => $biblio->{biblionumber} } } ); + + Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => undef, + rules => { + reservesallowed => 1, + holds_per_day => 1, + holds_per_record => 1, + } + } + ); + my $item = $builder->build_sample_item( { library => $library_1->{branchcode}, @@ -3366,6 +3385,9 @@ subtest 'Set waiting flag' => sub { my ( $status ) = CheckReserves($item->itemnumber); is( $status, 'Transferred', 'Hold is not waiting yet'); + my ( $status ) = CheckReserves($item->{itemnumber}); + is( $status, 'Reserved', 'Hold is not waiting yet'); + set_userenv( $library_2 ); $do_transfer = 0; AddReturn( $item->barcode, $library_2->{branchcode} ); diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t index 3e736f67743..b14b61a4849 100755 --- a/t/db_dependent/Holds.t +++ b/t/db_dependent/Holds.t @@ -242,6 +242,7 @@ Koha::CirculationRules->set_rules( rules => { reservesallowed => 25, holds_per_record => 99, + holds_per_day => 99, } } ); @@ -251,8 +252,9 @@ Koha::CirculationRules->set_rules( branchcode => undef, itemtype => 'CANNOT', rules => { - reservesallowed => 0, + reservesallowed => 1, holds_per_record => 99, + holds_per_day => 99, } } ); @@ -382,6 +384,7 @@ AddReserve( priority => 1, } ); + is( CanItemBeReserved( $borrowernumbers[0], $item->itemnumber)->{status}, 'tooManyReserves', "cannot request item if policy that matches on item-level item type forbids it" diff --git a/t/db_dependent/Reserves.t b/t/db_dependent/Reserves.t index 7422cb98174..a342f59cf42 100755 --- a/t/db_dependent/Reserves.t +++ b/t/db_dependent/Reserves.t @@ -187,6 +187,7 @@ Koha::CirculationRules->set_rules( rules => { reservesallowed => 25, holds_per_record => 1, + holds_per_day => 1, } } ); @@ -311,6 +312,7 @@ my $messages; # 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}, $requesters{$branch_1}, 'restrictive library\'s items only fill requests by own patrons (bug 10272)'); @@ -722,6 +724,97 @@ MoveReserve( $item->itemnumber, $borrowernumber ); is( $status, 'Reserved', 'MoveReserve did not fill future hold of 3 days'); $dbh->do('DELETE FROM reserves', undef, ($bibnum)); + + + + + +#Test bug 23172. Check when returning an item that any patron category/item type combinations that forbid reservation are skipped over +#when allocating the item to a bib-level hold +$dbh->do('DELETE FROM issuingrules'); + +#Create two new patrons +my $patronnumcat1 = Koha::Patron->new({branchcode => $branch_1, categorycode => $category_1})->store->borrowernumber; +my $patronnumcat2 = Koha::Patron->new({branchcode => $branch_1, categorycode => $category_2})->store->borrowernumber; + +#Create a new biblio record +my $bibnum3 = $builder->build_sample_biblio({frameworkcode => $frameworkcode})->biblionumber; + +#Create a second item type +my $itemtype2 = $builder->build({ source => 'Itemtype', value => { notforloan => undef } } )->{itemtype}; + +# Create an item from new itemtype +my ( $item_bibnum2, $item_bibitemnum2, $itemnumber2 ) = AddItem( + { homebranch => $branch_1, + holdingbranch => $branch_1, + itype => $itemtype2, + barcode => 'bug23172_CPL' + }, + $bibnum3 +); + +#Make a general All patron category/all item type rule +$dbh->do( + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_day, holds_per_record) + VALUES (?, ?, ?, ?, ?, ?)}, + {}, + '*', '*', '*', 25, 1, 1 +); + +#When patron category = $category_1 and itemtype = $itemtype1 then reservesallowed, holds_per_day and holds_per_record are 0 +$dbh->do( + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_day, holds_per_record) + VALUES (?, ?, ?, ?, ?, ?)}, + {}, + $category_1, $branch_1, $itemtype2, 0, 1, 1 +); + +#When patron category = $category_2 and itemtype = $itemtype2 then reservesallowed, holds_per_day and holds_per_record are 1 +$dbh->do( + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_day, holds_per_record) + VALUES (?, ?, ?, ?, ?, ?)}, + {}, + $category_2, $branch_1, $itemtype2, 1, 1, 1 +); + +#Remove existing reserves +$dbh->do("DELETE FROM reserves"); + +#Create Bib-level hold for borrower who has a patron category of $category_2 +my $reserveid2 = AddReserve($branch_1, $patronnumcat2, $bibnum3); + +#Bib-level hold for borrower who has a patron category of $category_1 +my $reserveid1 = AddReserve($branch_1, $patronnumcat1, $bibnum3 ); + +#Check bib-level hold priority and confirm hold made by patronnumcat1 (who fits into issuingrule with reserveallowed = 0) has first priority +my $reserve1 = Koha::Holds->find($reserveid1); +is( $reserve1->priority, 1, 'patronnumcat1 reserve is first priority'); + +my $reserve2 = Koha::Holds->find($reserveid2); +is( $reserve2->priority, 2, 'patronnumcat2 reserve is second priority'); + +#Return the item +(undef, $messages, undef, undef) = AddReturn('bug23172_CPL', $branch_1); + +#First priority hold is skipped as issuingrule is checked and the first hold does not respect issuingrules (as one of the three fields: reservesallowed, holds_per_day is 0) therefore not allocating item to this bib-level hold +is( $messages->{ResFound}->{borrowernumber}, $patronnumcat2,'Skip allocating item to first priority hold as it does not respect issuingrules instead allocate to next valid hold (bug 23172)'); + +#Remove and re-add the issuing rule that prevented patronnumcat1 bib-level hold from being allocated upon item return making reservesallowed, holds_per_day and holds_per_record > 0 +$dbh->do(q{DELETE FROM issuingrules WHERE categorycode = ? AND branchcode = ? AND itemtype = ?}, + {}, + $category_1, $branch_1, $itemtype2 +); +$dbh->do( + q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_day, holds_per_record) + VALUES (?, ?, ?, ?, ?, ?)}, + {}, + $category_1, $branch_1, $itemtype2, 1, 1, 1 +); + +#Return the item again and notice this time it is allocated to the bib-level hold made by patronnumcat1 +(undef, $messages, undef, undef) = AddReturn('bug23172_CPL', $branch_1); +is( $messages->{ResFound}->{borrowernumber}, $patronnumcat1,'Skip allocating item to first priority hold as it does not respect issuingrules instead allocate to next valid hold (bug 23172)'); + $cache->clear_from_cache("MarcStructure-0-$frameworkcode"); $cache->clear_from_cache("MarcStructure-1-$frameworkcode"); $cache->clear_from_cache("default_value_for_mod_marc-"); -- 2.11.0