Bugzilla – Attachment 102101 Details for
Bug 23172
Holds queue should check patron category hold policies when mapping items to pending reserves
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests
Bug-23172-Fixes-to-failing-Circulationt-and-Reserv.patch (text/plain), 14.55 KB, created by
Aleisha Amohia
on 2020-03-31 02:56:17 UTC
(
hide
)
Description:
Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests
Filename:
MIME Type:
Creator:
Aleisha Amohia
Created:
2020-03-31 02:56:17 UTC
Size:
14.55 KB
patch
obsolete
>From d2772d454d7cd5727652dfae894544c9b08a74e7 Mon Sep 17 00:00:00 2001 >From: Alex Buckley <alexbuckley@catalyst.net.nz> >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 <instancename> > >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 <hayleymapley@catalyst.net.nz> >--- > C4/Reserves.pm | 3 -- > t/db_dependent/.Reserves.t.swp | Bin 0 -> 16384 bytes > t/db_dependent/Circulation.t | 20 +++++++++ > t/db_dependent/Holds.t | 5 ++- > t/db_dependent/Reserves.t | 95 ++++++++++++++++++++++++++++++++++++++++- > 5 files changed, 118 insertions(+), 5 deletions(-) > create mode 100644 t/db_dependent/.Reserves.t.swp > >diff --git a/C4/Reserves.pm b/C4/Reserves.pm >index 1743303d59..5ebaa7927b 100644 >--- a/C4/Reserves.pm >+++ b/C4/Reserves.pm >@@ -810,7 +810,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'); >@@ -866,14 +865,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<b*j2*+?kKo*4T%pXDoc~wyZC?2bb%!cUWJ!-?H|c9pOZhBu9RB >z<Cf#|JvY93q?+{!W3WsWyfBV<S(bg*_IZa#K96L%&J!;7crtzSMvrbPm*x!Q4BWuL >zG3(UHSvA!s#*edm-n)5&MqZFJkTZ}okTZ}okTZ}okTZ}okTdZA%Yf`0vp$LLz1?&% >zHs5a>`TmFbWH08O5&7SZJTm2f9Ff0i<d2y0x@jP5=M5wOn2~>VME*A;zhvaEj>um( >z^2dz4VLFuAf6d67M*i%G{I5oS*~ssGhyG7i|5YQe8F?}y|BI1t8Tk)J<gXZc=HETH >z4BI#A)|jaujmS-L{*^P3GmtZoGmtZoGmtZoGmtZoGmtZoGmtazA2Hzg`rZH6^rDOV >z|Lpz$rMoQaI`9kN=fD%d!@#Y;AKqhGzXzTIJYW)d<(Or?3_J^50dTBfT?Q@zRp5ib >z81O#eHsH^9TGoreZ-AcxKLwrvz6Lx2TmyE2ZD1KV1&jm7fS2BFS-%5*1bh?t2Jj^C >zIB*r{0v<3Ai~(=l0iS{I0nY=U13n7~;0n+I9so`QKfc|v?gd_amt{Q%q`(>A_1i4# >zHQ-OcZ-FlWtH3S5%ePwAw}35R8JGlq^-jzBCGch7Nnjp01KbUK9q;x}0TQ?bYy*!1 >zTfl?BeZY5cAn<MAi@>LW9pGW$1aK#CJMev+VI;r;Fb2GdF;wHIBB8y{CS6B(vCo<L >z%(F?!6KP84ETs~-VGu`Y+vNgkP;mnnk472`5qBX$dB>3=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<+3N<IU}tNOuC1^w&XQJ~hCYj8$y$!^mD8}Ns~>08Fo{d7+j3+f >zVY0<pCl#GI;Vf?AF-RU#;V}>|@T~CR2Tz}xEzH5ewb&1u`;2^=W1v?C(N1Q38Uc9C >z5SqlP@H8WfD%1)CVP|&(*|MY9CNmUcZW@F>7w9GZr+8o~<-?_R;<P&<S9LMC2-|)j >ztFcpYTw-B>Q55?$;QjH0<NF=umZDWXkjn$iw|Lk=<g3PmD3J&Z&YH1c#0h$<ebck$ >zM&C2<v|Gi4ra)kHW3k%{o}m(o#K9iSAw5(oaJ^Dli@n{XQfcGMPVxIF9_>+g4-ZsE >zolwDgzy_$VfnVxlp~Uu_FjXBtpwK{n!q|z3oD{#oYO-ti2VV}T+f?J4p0L^DjUA<E >zt@=KL5k(bMQlr+J948N{H00fqnW_bNkEU5xQ&D9c$ja>5|JIsz*VseC(DA6~v`^j4 >z_@dIw932=0<EAdLZ`$R5(audfdz2mBQSA}BIWfUDxJ)~&f%ru_@f@#(yy}JA5q49= >zZCjNT$JdY|Hd414@X+6oBK2g71Uxwuuj2@_x3+*la?()RD%NbonB9(jyA8WjM^{u$ >z_nMsjw;CO;J99`~TJKaJG0J-RSRxLvWbANhXLB4K^WE0ef*%*h(+(CP&d75+F_hRz >z<{;%gMn7g$RLvLk1g2S#OglT6sn85OiRHC`M2lx8ogJfl`s8c@bH=Bl%<_D}(Pcwa >z&p$L?(v!2qh6h3AtBn|W1zDUq!VRR#bgBvFwUEstm9gLq8}J6!0;$qiGR@Y~L^2mk >zjqh`K6Eok%BgPEgVpZt5!h!@76Lk59r>2hJO-fE4FBY=jajtXj*Aa6fN|1SGDK)9t >z)-$S;A-hs8Lqx-4761%Tmi{PO7qevn8v`iz#;35nvbxe(VQ1?X)|eLvFAW`{+ZHLd >zC<U}fHZhv8w{$;t32`L(Rf(~6TNvg9y=gmohXIPF7Ub2)cKJ>a6-`yGK}mr&Bd(J` >zNtb0BS$Qv{mV1h{i_t?Q9Ra?PIub{P1C~dXqXoZ`27=r1Zm~fAWxSE+i!0}9>+Aqq >zP^`%&TKhe$QwQe;WP0B<xDmQhI#gjjmVw7Bl?KNuiXa~nuBtT?$#Kv~jRJeH;CvkV >zLj~wN-;_Z(RH)~et~c1pe#siA-KVKUDvN3@?<)+ntcJ1A1EPWWp@thl8{Jt>g$e;( >zd97MsB3ivzV7sbR4~Z+Sn+eo<Ym_PYDwQoqL|R8blLq!b7=qzanlh-6&}#a?X=4J4 >zTtwIya}lCwf;sr*^A4k(j~doF7Bp!kE+Lz6cBUm|r&2C=yWQ!XD4mYQPC3LlMF~$9 >zrcrx4PP3g|%h{u8?eSm_GuL5W+}YQ=H_pzjZ?bbd;=&2p#S}XukF9Dm))wOHkg >zHqCyYO9w+qjjpZKm(Ew$8`Z_yYOS%&un9U_YpkzqY_PKz>a5BxR_l%0(&lQl&Mt1& >zFJ9PKnZ{zHG6<?~)f(q7Y&KYReVc7n>vdS#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<xVa35~4CwY985E3E8NMV`w#{v0 >zcK@ZjHd0L;#;Y?#a*C>%%N|^&^kn@y2NtPibc0lLESMd;RVuj1Fb_pdH<XL!=Ch>u >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_@j<V >LGmBSJo^tD-QJGuB > >literal 0 >HcmV?d00001 > >diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t >index 104fc8fc85..09593f3031 100755 >--- a/t/db_dependent/Circulation.t >+++ b/t/db_dependent/Circulation.t >@@ -265,6 +265,8 @@ Koha::CirculationRules->set_rules( > auto_renew => 0, > fine => .10, > chargeperiod => 1, >+ holds_per_day => 12, >+ holds_per_record => 1, > } > } > ); >@@ -1458,6 +1460,7 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { > branchcode => undef, > rules => { > onshelfholds => 0, >+ holds_per_day => 1 > } > } > ); >@@ -2940,6 +2943,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}, >@@ -2964,6 +2983,7 @@ subtest 'Set waiting flag' => sub { > my $hold = Koha::Holds->find( $reserve_id ); > is( $hold->found, 'T', 'Hold is in transit' ); > >+ > my ( $status ) = CheckReserves($item->{itemnumber}); > is( $status, 'Reserved', 'Hold is not waiting yet'); > >diff --git a/t/db_dependent/Holds.t b/t/db_dependent/Holds.t >index 69b2071284..fe5a80e04b 100755 >--- a/t/db_dependent/Holds.t >+++ b/t/db_dependent/Holds.t >@@ -240,6 +240,7 @@ Koha::CirculationRules->set_rules( > rules => { > reservesallowed => 25, > holds_per_record => 99, >+ holds_per_day => 99, > } > } > ); >@@ -249,8 +250,9 @@ Koha::CirculationRules->set_rules( > branchcode => undef, > itemtype => 'CANNOT', > rules => { >- reservesallowed => 0, >+ reservesallowed => 1, > holds_per_record => 99, >+ holds_per_day => 99, > } > } > ); >@@ -355,6 +357,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 c75a69ad95..4a3f6c4c21 100755 >--- a/t/db_dependent/Reserves.t >+++ b/t/db_dependent/Reserves.t >@@ -17,7 +17,7 @@ > > use Modern::Perl; > >-use Test::More tests => 62; >+use Test::More tests => 65; > use Test::MockModule; > use Test::Warn; > >@@ -190,6 +190,7 @@ Koha::CirculationRules->set_rules( > rules => { > reservesallowed => 25, > holds_per_record => 1, >+ holds_per_day => 1, > } > } > ); >@@ -314,6 +315,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)'); >@@ -725,6 +727,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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 23172
:
90868
|
91133
|
91301
|
92250
|
92251
|
92252
|
102099
|
102100
|
102101
|
102169
|
118930
|
118931
|
118932
|
118933
|
128817
|
128857