Bugzilla – Attachment 134723 Details for
Bug 29483
AllowRenewalIfOtherItemsAvailable has poor performance for records with many items
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 29483: [21.11.x] Check ItemsAnyAvailableAndNotRestricted once per patron
Bug-29483-2111x-Check-ItemsAnyAvailableAndNotRestr.patch (text/plain), 19.79 KB, created by
Nick Clemens (kidclamp)
on 2022-05-06 11:34:58 UTC
(
hide
)
Description:
Bug 29483: [21.11.x] Check ItemsAnyAvailableAndNotRestricted once per patron
Filename:
MIME Type:
Creator:
Nick Clemens (kidclamp)
Created:
2022-05-06 11:34:58 UTC
Size:
19.79 KB
patch
obsolete
>From 489dab1038965a2a741a6c2cf86a886116278c9d Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Mon, 15 Nov 2021 13:54:00 +0000 >Subject: [PATCH] Bug 29483: [21.11.x] Check ItemsAnyAvailableAndNotRestricted > once per patron > >ItemsAnyAvailableAndNotRestricted can take a long time and create nested loops. >We can check it once per patron, however, this requires us to flip the loops. >Since an item can only be used once, we now add a check to see if this item has already >been assigned to a borrower. > >To test: >1 - Find or create a biblio with 100 items >2 - Place ten 'Next available' holds on a biblio >3 - Set preference 'AllowRenewalIfOtherItemsAvailable' to 'Allow' > Set circ rules 'On shelf holds allowed' to 'If any unavailable' >4 - Checkout one of the items to a patron, backdated to be overdue >5 - Note a long loading time for the patron's checkouts >6 - Apply patch, restart_all >7 - Patron loads much faster > >Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> > >Bug 29483: Unit tests > >This patch updates the AllowRenewalIfOtherItemsAvailable tests to >remove deletion of all data, and create specific circ rules for this >test. It adjust several other tests that were relying on the rules from >this test, so thy too create their opwn specific rules. > >Additionally, we add tests to cover the case of mutliple items on the record, >and some items cannot fill some reserves. > >What is uncovered here is that the same patron is checked twice, so two holds can >be filled, but they only satisfy a single patron > >Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> > >Bug 29483: Further improve performance of script > >This patch adds a few tests to cover more cases, and to highlight current functionality. > >The script only allows renewal if all outstanding holds can be filled by available items. >This means we can return as soon as we have determined that not all holds can be filled. > >I add FIXME and some explanatory comments - I will file a follow-up bug for those, but >I feel we can accept these improvements to the performance and deal with the issues >of how it 'should' work versus how it does work on another report. > >To test: >1 - prove -v t/db_dependent/Circulation.t > >Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > C4/Circulation.pm | 51 ++++++--- > t/db_dependent/Circulation.t | 211 +++++++++++++++++++++++++++++------ > 2 files changed, 211 insertions(+), 51 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index a4d6929e31..543524feb4 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -27,7 +27,7 @@ use Encode; > use Koha::DateUtils qw( dt_from_string output_pref ); > use C4::Context; > use C4::Stats qw( UpdateStats ); >-use C4::Reserves qw( CheckReserves CanItemBeReserved MoveReserve ModReserve ModReserveMinusPriority RevertWaitingStatus IsItemOnHoldAndFound IsAvailableForItemLevelRequest ); >+use C4::Reserves qw( CheckReserves CanItemBeReserved MoveReserve ModReserve ModReserveMinusPriority RevertWaitingStatus IsItemOnHoldAndFound IsAvailableForItemLevelRequest ItemsAnyAvailableAndNotRestricted ); > use C4::Biblio qw( UpdateTotalIssues ); > use C4::Items qw( ModItemTransfer ModDateLastSeen CartToShelf ); > use C4::Accounts; >@@ -2835,37 +2835,56 @@ sub CanBookBeRenewed { > else { > > # Get all other items that could possibly fill reserves >+ # FIXME We could join reserves (or more tables) here to eliminate some checks later > my $items = Koha::Items->search({ > biblionumber => $resrec->{biblionumber}, > onloan => undef, > notforloan => 0, > -not => { itemnumber => $itemnumber } > }); >+ my $item_count = $items->count(); > > # Get all other reserves that could have been filled by this item > my @borrowernumbers = map { $_->{borrowernumber} } @$possible_reserves; >+ # Note: fetching the patrons in this manner means that a patron with 2 holds will >+ # not block renewal if one reserve can be satisfied i.e. each patron is checked once > my $patrons = Koha::Patrons->search({ > borrowernumber => { -in => \@borrowernumbers } > }); >- >- # If the count of the union of the lists of reservable items for each borrower >- # is equal or greater than the number of borrowers, we know that all reserves >- # can be filled with available items. We can get the union of the sets simply >- # by pushing all the elements onto an array and removing the duplicates. >- my @reservable; >- ITEM: while ( my $item = $items->next ) { >- next if IsItemOnHoldAndFound( $item->itemnumber ); >- while ( my $patron = $patrons->next ) { >- next unless IsAvailableForItemLevelRequest($item, $patron); >- next unless CanItemBeReserved($patron->borrowernumber,$item->itemnumber,undef,{ignore_hold_counts=>1})->{status} eq 'OK'; >- push @reservable, $item->itemnumber; >- if (@reservable >= @borrowernumbers) { >+ my $patron_count = $patrons->count(); >+ >+ return ( 0, "on_reserve" ) if ($patron_count > $item_count); >+ # We cannot possibly fill all reserves if we don't have enough items >+ >+ # If we can fill each hold that has been found with the available items on the record >+ # then the patron can renew. If we cannot, they cannot renew. >+ # FIXME This code does not check whether the item we are renewing can fill >+ # any of the existing reserves. >+ my $reservable = 0; >+ my %matched_items; >+ my $seen = 0; >+ PATRON: while ( my $patron = $patrons->next ) { >+ # If there is a reserve that cannot be filled we are done >+ return ( 0, "on_reserve" ) if ( $seen > $reservable ); >+ my $items_any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $item->biblionumber, patron => $patron }); >+ while ( my $other_item = $items->next ) { >+ next if defined $matched_items{$other_item->itemnumber}; >+ next if IsItemOnHoldAndFound( $other_item->itemnumber ); >+ next unless IsAvailableForItemLevelRequest($other_item, $patron, undef, $items_any_available); >+ next unless CanItemBeReserved($patron->borrowernumber,$other_item->itemnumber,undef,{ignore_hold_counts=>1})->{status} eq 'OK'; >+ # NOTE: At checkin we call 'CheckReserves' which checks hold 'policy' >+ # CanItemBeReserved checks 'rules' and 'policies' which means >+ # items will fill holds at checkin that are rejected here >+ $reservable++; >+ if ($reservable >= $patron_count) { > $resfound = 0; >- last ITEM; >+ last PATRON; > } >+ $matched_items{$other_item->itemnumber} = 1; > last; > } >- $patrons->reset; >+ $items->reset; >+ $seen++; > } > } > } >diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t >index 249e8aee1c..de4637de9b 100755 >--- a/t/db_dependent/Circulation.t >+++ b/t/db_dependent/Circulation.t >@@ -1556,16 +1556,30 @@ subtest "Bug 13841 - Do not create new 0 amount fines" => sub { > }; > > subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { >- $dbh->do('DELETE FROM issues'); >- $dbh->do('DELETE FROM items'); >- $dbh->do('DELETE FROM circulation_rules'); >+ plan tests => 13; >+ my $biblio = $builder->build_sample_biblio(); >+ my $item_1 = $builder->build_sample_item( >+ { >+ biblionumber => $biblio->biblionumber, >+ library => $library2->{branchcode}, >+ } >+ ); >+ my $item_2= $builder->build_sample_item( >+ { >+ biblionumber => $biblio->biblionumber, >+ library => $library2->{branchcode}, >+ itype => $item_1->effective_itemtype, >+ } >+ ); >+ > Koha::CirculationRules->set_rules( > { > categorycode => undef, >- itemtype => undef, >+ itemtype => $item_1->effective_itemtype, > branchcode => undef, > rules => { > reservesallowed => 25, >+ holds_per_record => 25, > issuelength => 14, > lengthunit => 'days', > renewalsallowed => 1, >@@ -1578,23 +1592,7 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { > } > } > ); >- my $biblio = $builder->build_sample_biblio(); > >- my $item_1 = $builder->build_sample_item( >- { >- biblionumber => $biblio->biblionumber, >- library => $library2->{branchcode}, >- itype => $itemtype, >- } >- ); >- >- my $item_2= $builder->build_sample_item( >- { >- biblionumber => $biblio->biblionumber, >- library => $library2->{branchcode}, >- itype => $itemtype, >- } >- ); > > my $borrowernumber1 = Koha::Patron->new({ > firstname => 'Kyle', >@@ -1608,6 +1606,22 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { > categorycode => $patron_category->{categorycode}, > branchcode => $library2->{branchcode}, > })->store->borrowernumber; >+ my $patron_category_2 = $builder->build( >+ { >+ source => 'Category', >+ value => { >+ category_type => 'P', >+ enrolmentfee => 0, >+ BlockExpiredPatronOpacActions => -1, # Pick the pref value >+ } >+ } >+ ); >+ my $borrowernumber3 = Koha::Patron->new({ >+ firstname => 'Carnegie', >+ surname => 'Hall', >+ categorycode => $patron_category_2->{categorycode}, >+ branchcode => $library2->{branchcode}, >+ })->store->borrowernumber; > > my $borrower1 = Koha::Patrons->find( $borrowernumber1 )->unblessed; > my $borrower2 = Koha::Patrons->find( $borrowernumber2 )->unblessed; >@@ -1629,7 +1643,7 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { > Koha::CirculationRules->set_rules( > { > categorycode => undef, >- itemtype => undef, >+ itemtype => $item_1->effective_itemtype, > branchcode => undef, > rules => { > onshelfholds => 0, >@@ -1640,53 +1654,121 @@ subtest "AllowRenewalIfOtherItemsAvailable tests" => sub { > ( $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' ); > >+ t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 1 ); >+ ( $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 is enabled and onshelfholds is disabled' ); >+ > Koha::CirculationRules->set_rules( > { > categorycode => undef, >- itemtype => undef, >+ itemtype => $item_1->effective_itemtype, > branchcode => undef, > rules => { >- onshelfholds => 0, >+ onshelfholds => 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 is disabled and onshelfhold is enabled' ); >+ > t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 1 ); > ( $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 is enabled and onshelfholds is disabled' ); >+ is( $renewokay, 1, 'Bug 14337 - Verify the borrower can renew with a hold on the record if AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled' ); >+ >+ AddReserve( >+ { >+ branchcode => $library2->{branchcode}, >+ borrowernumber => $borrowernumber3, >+ biblionumber => $biblio->biblionumber, >+ priority => 1, >+ } >+ ); >+ >+ ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); >+ is( $renewokay, 0, 'Verify the borrower cannot renew with 2 holds on the record if AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled and one other item on record' ); >+ >+ my $item_3= $builder->build_sample_item( >+ { >+ biblionumber => $biblio->biblionumber, >+ library => $library2->{branchcode}, >+ itype => $item_1->effective_itemtype, >+ } >+ ); >+ >+ ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); >+ is( $renewokay, 1, 'Verify the borrower cannot renew with 2 holds on the record if AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled and two other items on record' ); > > Koha::CirculationRules->set_rules( > { >- categorycode => undef, >- itemtype => undef, >+ categorycode => $patron_category_2->{categorycode}, >+ itemtype => $item_1->effective_itemtype, > branchcode => undef, > rules => { >- onshelfholds => 1, >+ reservesallowed => 0, > } > } > ); >- 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 is disabled and onshelfhold is enabled' ); >+ is( $renewokay, 0, 'Verify the borrower cannot renew with 2 holds on the record, but only one of those holds can be filled when AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled and two other items on record' ); > > Koha::CirculationRules->set_rules( > { >- categorycode => undef, >- itemtype => undef, >+ categorycode => $patron_category_2->{categorycode}, >+ itemtype => $item_1->effective_itemtype, > branchcode => undef, > rules => { >- onshelfholds => 1, >+ reservesallowed => 25, > } > } > ); >- t::lib::Mocks::mock_preference( 'AllowRenewalIfOtherItemsAvailable', 1 ); >- ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); >- is( $renewokay, 1, 'Bug 14337 - Verify the borrower can renew with a hold on the record if AllowRenewalIfOtherItemsAvailable and onshelfhold are enabled' ); > > # Setting item not checked out to be not for loan but holdable > $item_2->notforloan(-1)->store; > > ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); > is( $renewokay, 0, 'Bug 14337 - Verify the borrower can not renew with a hold on the record if AllowRenewalIfOtherItemsAvailable is enabled but the only available item is notforloan' ); >+ >+ my $mock_circ = Test::MockModule->new("C4::Circulation"); >+ $mock_circ->mock( CanItemBeReserved => sub { >+ warn "Checked"; >+ return { status => 'no' } >+ } ); >+ >+ $item_2->notforloan(0)->store; >+ $item_3->delete(); >+ # Two items total, one item available, one issued, two holds on record >+ >+ warnings_are{ >+ ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); >+ } [], "CanItemBeReserved not called when there are more possible holds than available items"; >+ is( $renewokay, 0, 'Borrower cannot renew when there are more holds than available items' ); >+ >+ $item_3 = $builder->build_sample_item( >+ { >+ biblionumber => $biblio->biblionumber, >+ library => $library2->{branchcode}, >+ itype => $item_1->effective_itemtype, >+ } >+ ); >+ >+ Koha::CirculationRules->set_rules( >+ { >+ categorycode => undef, >+ itemtype => $item_1->effective_itemtype, >+ branchcode => undef, >+ rules => { >+ reservesallowed => 0, >+ } >+ } >+ ); >+ >+ warnings_are{ >+ ( $renewokay, $error ) = CanBookBeRenewed( $borrowernumber1, $item_1->itemnumber ); >+ } ["Checked","Checked"], "CanItemBeReserved only called once per available item if it returns a negative result for all items for a borrower"; >+ is( $renewokay, 0, 'Borrower cannot renew when there are more holds than available items' ); >+ > }; > > { >@@ -1771,6 +1853,25 @@ subtest 'CanBookBeIssued & AllowReturnToBranch' => sub { > holdingbranch => $holdingbranch->{branchcode}, > } > ); >+ Koha::CirculationRules->set_rules( >+ { >+ categorycode => undef, >+ itemtype => $item->effective_itemtype, >+ branchcode => undef, >+ rules => { >+ reservesallowed => 25, >+ issuelength => 14, >+ lengthunit => 'days', >+ renewalsallowed => 1, >+ renewalperiod => 7, >+ norenewalbefore => undef, >+ auto_renew => 0, >+ fine => .10, >+ chargeperiod => 1, >+ maxissueqty => 20 >+ } >+ } >+ ); > > set_userenv($holdingbranch); > >@@ -1924,6 +2025,26 @@ subtest 'CanBookBeIssued + Koha::Patron->is_debarred|has_overdues' => sub { > library => $library->{branchcode}, > } > ); >+ Koha::CirculationRules->set_rules( >+ { >+ categorycode => undef, >+ itemtype => undef, >+ branchcode => $library->{branchcode}, >+ rules => { >+ reservesallowed => 25, >+ issuelength => 14, >+ lengthunit => 'days', >+ renewalsallowed => 1, >+ renewalperiod => 7, >+ norenewalbefore => undef, >+ auto_renew => 0, >+ fine => .10, >+ chargeperiod => 1, >+ maxissueqty => 20 >+ } >+ } >+ ); >+ > > my ( $error, $question, $alerts ); > >@@ -2121,6 +2242,26 @@ subtest 'CanBookBeIssued + AllowMultipleIssuesOnABiblio' => sub { > } > ); > >+ Koha::CirculationRules->set_rules( >+ { >+ categorycode => undef, >+ itemtype => undef, >+ branchcode => $library->{branchcode}, >+ rules => { >+ reservesallowed => 25, >+ issuelength => 14, >+ lengthunit => 'days', >+ renewalsallowed => 1, >+ renewalperiod => 7, >+ norenewalbefore => undef, >+ auto_renew => 0, >+ fine => .10, >+ chargeperiod => 1, >+ maxissueqty => 20 >+ } >+ } >+ ); >+ > my ( $error, $question, $alerts ); > my $issue = AddIssue( $patron->unblessed, $item_1->barcode, dt_from_string->add( days => 1 ) ); > >-- >2.30.2
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=134723">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 29483
:
127656
|
127819
|
127820
|
127883
|
127884
|
127885
|
129450
|
129451
|
129452
|
129453
|
129454
|
129499
|
129500
|
129501
|
132094
|
132095
|
132096
|
132113
|
133389
|
133390
|
133391
| 134723