Attachment 92252 Details for Bug 23172 – Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests

[patch] Bug 23172: Fixes to failing Circulation.t and Reserves.t unit tests

Bug-23172-Fixes-to-failing-Circulationt-and-Reserv.patch (text/plain), 13.10 KB, created by Hayley Pelham on 2019-08-16 03:51:25 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Hayley Pelham

Created: 2019-08-16 03:51:25 UTC

Size: 13.10 KB

patch

obsolete

>From 6d5a0a7fef034b9745f698e97c1f6567f4f8b8a1 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/Circulation.pm            |   1 +
> C4/Reserves.pm               |   5 +--
> t/db_dependent/Circulation.t |  15 +++++--
> t/db_dependent/Holds.t       |  13 +++---
> t/db_dependent/Reserves.t    | 100 +++++++++++++++++++++++++++++++++++++++++--
> 5 files changed, 116 insertions(+), 18 deletions(-)
>
>diff --git a/C4/Circulation.pm b/C4/Circulation.pm
>index 9961d380778..658756b499a 100644
>--- a/C4/Circulation.pm
>+++ b/C4/Circulation.pm
>@@ -2060,6 +2060,7 @@ sub AddReturn {
>     my ($resfound, $resrec);
>     my $lookahead= C4::Context->preference('ConfirmFutureHolds'); #number of days to look for future holds
>     ($resfound, $resrec, undef) = C4::Reserves::CheckReserves( $item->itemnumber, undef, $lookahead ) unless ( $item->withdrawn );
>+
>     if ($resfound) {
>           $resrec->{'ResFound'} = $resfound;
>         $messages->{'ResFound'} = $resrec;
>diff --git a/C4/Reserves.pm b/C4/Reserves.pm
>index 987022ec34b..556bb961b84 100644
>--- a/C4/Reserves.pm
>+++ b/C4/Reserves.pm
>@@ -214,7 +214,6 @@ sub AddReserve {
>         }
>     )->store();
>     $hold->set_waiting() if $found eq 'W';
>-
>     logaction( 'HOLDS', 'CREATE', $hold->id, Dumper($hold->unblessed) )
>         if C4::Context->preference('HoldsLog');
> 
>@@ -378,6 +377,7 @@ sub CanItemBeReserved {
>             found          => undef, # Found holds don't count against a patron's holds limit
>         }
>     );
>+
>     if ( $holds->count() >= $holds_per_record ) {
>         return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record };
>     }
>@@ -758,7 +758,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');
>@@ -810,14 +809,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/Circulation.t b/t/db_dependent/Circulation.t
>index b7acf2a6179..a242a9ed961 100755
>--- a/t/db_dependent/Circulation.t
>+++ b/t/db_dependent/Circulation.t
>@@ -259,12 +259,15 @@ $dbh->do(
>                                 issuelength, lengthunit,
>                                 renewalsallowed, renewalperiod,
>                                 norenewalbefore, auto_renew,
>-                                fine, chargeperiod)
>+                                fine, chargeperiod,
>+                                holds_per_day, holds_per_record
>+                                )
>       VALUES (?, ?, ?, ?,
>               ?, ?,
>               ?, ?,
>               ?, ?,
>-              ?, ?
>+              ?, ?,
>+              ?,?
>              )
>     },
>     {},
>@@ -272,7 +275,8 @@ $dbh->do(
>     14, 'days',
>     1, 7,
>     undef, 0,
>-    .10, 1
>+    .10, 1,
>+    12,1
> );
> 
> my ( $reused_itemnumber_1, $reused_itemnumber_2 );
>@@ -1193,7 +1197,7 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub {
>         undef, undef, undef
>     );
> 
>-    C4::Context->dbh->do("UPDATE issuingrules SET onshelfholds = 0");
>+    C4::Context->dbh->do("UPDATE issuingrules SET onshelfholds = 0, holds_per_day = 1");
>     t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 0 );
>     ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber );
>     is( $renewokay, 0, 'Bug 14337 - Verify the borrower cannot renew with a hold on the record if AllowRenewalIfOtherItemsAvailable and onshelfholds are disabled' );
>@@ -2661,6 +2665,8 @@ subtest 'Set waiting flag' => sub {
>     my $biblio = $builder->build( { source => 'Biblio' } );
>     my $biblioitem = $builder->build( { source => 'Biblioitem', value => { biblionumber => $biblio->{biblionumber} } } );
> 
>+    C4::Context->dbh->do("UPDATE issuingrules SET reservesallowed = 1, holds_per_day = 1, holds_per_record = 1");
>+
>     my $item = $builder->build(
>         {
>             source => 'Item',
>@@ -2688,6 +2694,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 f7919b3827a..19507d188fd 100755
>--- a/t/db_dependent/Holds.t
>+++ b/t/db_dependent/Holds.t
>@@ -245,16 +245,16 @@ my ($foreign_item_bibnum, $foreign_item_bibitemnum, $foreign_itemnumber)
>   = AddItem({ homebranch => $branch_2, holdingbranch => $branch_2 } , $foreign_biblio->biblionumber);
> $dbh->do('DELETE FROM issuingrules');
> $dbh->do(
>-    q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record)
>-      VALUES (?, ?, ?, ?, ?)},
>+    q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record, holds_per_day)
>+      VALUES (?, ?, ?, ?, ?, ?)},
>     {},
>-    '*', '*', '*', 25, 99
>+    '*', '*', '*', 25, 99, 99
> );
> $dbh->do(
>-    q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record)
>-      VALUES (?, ?, ?, ?, ?)},
>+    q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record, holds_per_day)
>+      VALUES (?, ?, ?, ?, ?, ?)},
>     {},
>-    '*', '*', 'CANNOT', 0, 99
>+    '*', '*', 'CANNOT', 1, 99, 99
> );
> 
> # make sure some basic sysprefs are set
>@@ -331,6 +331,7 @@ AddReserve(
>     '',
>     1,
> );
>+
> is(
>     CanItemBeReserved( $borrowernumbers[0], $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 33aa3d9f76a..93a0feefc9a 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;
> 
>@@ -192,10 +192,10 @@ $requesters{$branch_3} = Koha::Patron->new({
> 
> $dbh->do('DELETE FROM issuingrules');
> $dbh->do(
>-    q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed)
>-      VALUES (?, ?, ?, ?)},
>+    q{INSERT INTO issuingrules (categorycode, branchcode, itemtype, reservesallowed, holds_per_record, holds_per_day)
>+      VALUES (?, ?, ?, ?, ?, ?)},
>     {},
>-    '*', '*', '*', 25
>+    '*', '*', '*', 25, 1, 1
> );
> 
> # CPL allows only its own patrons to request its items
>@@ -290,6 +290,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)');
>@@ -596,6 +597,97 @@ MoveReserve( $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

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