From 49b465bd0041f0d8e7c95a0334b819f265c38609 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Tue, 26 Apr 2022 16:28:22 -0300 Subject: [PATCH] Bug 29346: Add more fine-grained control of holds queue updates This patch deals with the fact that high-level circualtion methods like `AddIssue`, `AddReturn` and `ModDateLastSeen` all eventually call lower-level methods like ModBiblio, Koha::Item->store of UpdateTotalIssues which are expected to trigger holds queue updates (for the object CRUD operations use cases). As the circulation methods need to trigger holds queue update as well, duplicate updates were being requested which is suboptimal, of course. In order to prevent this, and because circulation methdos could trigger holds queue updates several times, actually, I added a new parameter *skip_holds_queue* to the low-level methods, so when they are called from circulation, the trigger is skipped and we have greater control on when and how holds queue updates are scheduled. This patch introduces the `skip_holds_queue` parameter to the following methods: * C4::Biblio::ModBiblio * C4::Biblio::UpdateTotalIssues * Koha::Item->store Calls to those methods from the following methods will include the new parameter, and thus duplicated holds queue updates avoided: * C4::Circulation::AddIssue * C4::Circulation::AddReturn * C4::Items::ModDateLastSeen Tests are added, to verify that the (mocked) BatchUpdateBiblioHoldsQueue task is only scheduled once when they are called. To test: 1. Apply up to the previous patch 2. Run: $ kshell k$ prove t/db_dependent/Biblio.t \ t/db_dependent/Biblio_holdsqueue.t \ t/db_dependent/Circulation_holdsqueue.t => FAIL: Tests fail! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D --- C4/Biblio.pm | 11 ++++++++--- C4/Circulation.pm | 27 ++++++++++++++++----------- C4/Items.pm | 2 +- Koha/Item.pm | 2 +- 4 files changed, 26 insertions(+), 16 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index d438688e2d4..da10581ede1 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -344,6 +344,11 @@ Unless C is passed ModBiblio will relink record headings to authorities based on settings in the system preferences. This flag allows us to not relink records when the authority linker is saving modifications. +=item C + +Unless C is passed, ModBiblio will trigger the BatchUpdateBiblioHoldsQueue +task to rebuild the holds queue for the biblio. + =back Returns 1 on success 0 on failure @@ -433,7 +438,7 @@ sub ModBiblio { { biblio_ids => [ $biblionumber ] } - ); + ) unless $options->{skip_holds_queue}; return 1; } @@ -3082,7 +3087,7 @@ Update the total issue count for a particular bib record. =cut sub UpdateTotalIssues { - my ($biblionumber, $increase, $value) = @_; + my ($biblionumber, $increase, $value, $skip_holds_queue) = @_; my $totalissues; my $record = GetMarcBiblio({ biblionumber => $biblionumber }); @@ -3116,7 +3121,7 @@ sub UpdateTotalIssues { $record->insert_grouped_field($field); } - return ModBiblio($record, $biblionumber, $biblio->frameworkcode); + return ModBiblio($record, $biblionumber, $biblio->frameworkcode, { skip_holds_queue => $skip_holds_queue }); } =head2 RemoveAllNsb diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 1bf35683218..f2096fbd2f9 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1671,7 +1671,7 @@ sub AddIssue { } if ( C4::Context->preference('UpdateTotalIssuesOnCirc') ) { - UpdateTotalIssues( $item_object->biblionumber, 1 ); + UpdateTotalIssues( $item_object->biblionumber, 1, undef, { skip_holds_queue => 1 } ); } # Record if item was lost @@ -1683,7 +1683,7 @@ sub AddIssue { $item_object->onloan($datedue->ymd()); $item_object->datelastborrowed( dt_from_string()->ymd() ); $item_object->datelastseen( dt_from_string()->ymd() ); - $item_object->store({log_action => 0}); + $item_object->store( { log_action => 0, skip_holds_queue => 1 } ); # If the item was lost, it has now been found, charge the overdue if necessary if ($was_lost) { @@ -2077,7 +2077,7 @@ sub AddReturn { . Dumper($issue->unblessed) . "\n"; } else { $messages->{'NotIssued'} = $barcode; - $item->onloan(undef)->store({skip_record_index=>1}) if defined $item->onloan; + $item->onloan(undef)->store( { skip_record_index => 1, skip_holds_queue => 1 } ) if defined $item->onloan; # even though item is not on loan, it may still be transferred; therefore, get current branch info $doreturn = 0; @@ -2110,7 +2110,7 @@ sub AddReturn { (!defined $item->location && $update_loc_rules->{_ALL_} ne "") ) { $messages->{'ItemLocationUpdated'} = { from => $item->location, to => $update_loc_rules->{_ALL_} }; - $item->location($update_loc_rules->{_ALL_})->store({skip_record_index=>1}); + $item->location($update_loc_rules->{_ALL_})->store({ skip_record_index => 1, skip_holds_queue => 1}); } } else { @@ -2119,7 +2119,7 @@ sub AddReturn { if ( $update_loc_rules->{$key} eq '_BLANK_') { $update_loc_rules->{$key} = '' ;} if ( ($item->location eq $key && $item->location ne $update_loc_rules->{$key}) || ($key eq '_BLANK_' && $item->location eq '' && $update_loc_rules->{$key} ne '') ) { $messages->{'ItemLocationUpdated'} = { from => $item->location, to => $update_loc_rules->{$key} }; - $item->location($update_loc_rules->{$key})->store({skip_record_index=>1}); + $item->location($update_loc_rules->{$key})->store({ skip_record_index => 1, skip_holds_queue => 1}); last; } } @@ -2138,7 +2138,7 @@ sub AddReturn { foreach my $key ( keys %$rules ) { if ( $item->notforloan eq $key ) { $messages->{'NotForLoanStatusUpdated'} = { from => $item->notforloan, to => $rules->{$key} }; - $item->notforloan($rules->{$key})->store({ log_action => 0, skip_record_index => 1 }); + $item->notforloan($rules->{$key})->store({ log_action => 0, skip_record_index => 1, skip_holds_queue => 1 }); last; } } @@ -2174,7 +2174,7 @@ sub AddReturn { if ($patron) { eval { - MarkIssueReturned( $borrowernumber, $item->itemnumber, $return_date, $patron->privacy, { skip_record_index => 1} ); + MarkIssueReturned( $borrowernumber, $item->itemnumber, $return_date, $patron->privacy, { skip_record_index => 1, skip_holds_queue => 1} ); }; unless ( $@ ) { if ( @@ -2200,19 +2200,19 @@ sub AddReturn { $messages->{'WasReturned'} = 1; } else { - $item->onloan(undef)->store({ log_action => 0 , skip_record_index => 1 }); + $item->onloan(undef)->store({ log_action => 0 , skip_record_index => 1, skip_holds_queue => 1 }); } } # the holdingbranch is updated if the document is returned to another location. # this is always done regardless of whether the item was on loan or not if ($item->holdingbranch ne $branch) { - $item->holdingbranch($branch)->store({ skip_record_index => 1 }); + $item->holdingbranch($branch)->store({ skip_record_index => 1, skip_holds_queue => 1 }); } my $item_was_lost = $item->itemlost; my $leave_item_lost = C4::Context->preference("BlockReturnOfLostItems") ? 1 : 0; - my $updated_item = ModDateLastSeen( $item->itemnumber, $leave_item_lost, { skip_record_index => 1 } ); # will unset itemlost if needed + my $updated_item = ModDateLastSeen( $item->itemnumber, $leave_item_lost, { skip_record_index => 1, skip_holds_queue => 1 } ); # will unset itemlost if needed # fix up the accounts..... if ($item_was_lost) { @@ -2509,7 +2509,12 @@ sub MarkIssueReturned { # And finally delete the issue $issue->delete; - $issue->item->onloan(undef)->store({ log_action => 0, skip_record_index => $params->{skip_record_index} }); + $issue->item->onloan(undef)->store( + { log_action => 0, + skip_record_index => $params->{skip_record_index}, + skip_holds_queue => $params->{skip_holds_queue} + } + ); if ( C4::Context->preference('StoreLastBorrower') ) { my $item = Koha::Items->find( $itemnumber ); diff --git a/C4/Items.pm b/C4/Items.pm index a7f06dd8769..3ed1ce71aac 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -400,7 +400,7 @@ sub ModDateLastSeen { my $item = Koha::Items->find($itemnumber); $item->datelastseen($today); $item->itemlost(0) unless $leave_item_lost; - $item->store({ log_action => 0, skip_record_index => $params->{skip_record_index} }); + $item->store({ log_action => 0, skip_record_index => $params->{skip_record_index}, skip_holds_queue => $params->{skip_holds_queue} }); } =head2 CheckItemPreSave diff --git a/Koha/Item.pm b/Koha/Item.pm index c989a8df37e..e4e71c5beb3 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -217,7 +217,7 @@ sub store { { biblio_ids => [ $self->biblionumber ] } - ); + ) unless $params->{skip_holds_queue}; return $result; } -- 2.36.0