From aa78dd0253bd4fcc11a9b9c0dd471f60adcee3ca 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 --- 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 1bf3568321..f2096fbd2f 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 a7f06dd876..3ed1ce71aa 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.34.1