From 763d73fbf0a774da3dfece55304a973b988c2089 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 Signed-off-by: Tomas Cohen Arazi Signed-off-by: Nick Clemens Signed-off-by: Fridolin Somers --- 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 d438688e2d..da10581ede 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 813e9f0b00..46f538dd8d 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 65bf8bc971..ddd2fff096 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 c989a8df37..e4e71c5beb 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.39.2