Bugzilla – Attachment 134337 Details for
Bug 29346
Real-time holds queue update
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 29346: Add more fine-grained control of holds queue updates
Bug-29346-Add-more-fine-grained-control-of-holds-q.patch (text/plain), 9.64 KB, created by
Nick Clemens
on 2022-04-29 01:03:20 UTC
(
hide
)
Description:
Bug 29346: Add more fine-grained control of holds queue updates
Filename:
MIME Type:
Creator:
Nick Clemens
Created:
2022-04-29 01:03:20 UTC
Size:
9.64 KB
patch
obsolete
>From 58419ddce70d1be74d78220cddce1b60337e11a1 Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >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 <tomascohen@theke.io> > >Signed-off-by: Nick Clemens <nick@bywatersolutions.com> >--- > 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<disable_autolink> 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<skip_holds_queue> >+ >+Unless C<skip_holds_queue> 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.30.2
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 29346
:
131088
|
131089
|
131811
|
131812
|
131813
|
131815
|
132078
|
132079
|
132080
|
132081
|
132082
|
132083
|
133183
|
133184
|
133185
|
133186
|
133187
|
133188
|
133189
|
133190
|
133191
|
133205
|
133991
|
133992
|
133993
|
133994
|
133995
|
133996
|
133997
|
133998
|
133999
|
134000
|
134001
|
134010
|
134225
|
134227
|
134309
|
134310
|
134311
|
134312
|
134313
|
134314
|
134315
|
134316
|
134317
|
134318
|
134319
|
134320
|
134321
|
134322
|
134328
|
134329
|
134330
|
134331
|
134332
|
134333
|
134334
|
134335
|
134336
| 134337 |
134338
|
134339
|
134340
|
134341
|
134342
|
134343