Browse Source

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>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
rmain2205
Tomás Cohen Arazi 5 months ago
committed by Fridolin Somers
parent
commit
763d73fbf0
  1. 11
      C4/Biblio.pm
  2. 27
      C4/Circulation.pm
  3. 2
      C4/Items.pm
  4. 2
      Koha/Item.pm

11
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

27
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 );

2
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

2
Koha/Item.pm

@ -217,7 +217,7 @@ sub store {
{
biblio_ids => [ $self->biblionumber ]
}
);
) unless $params->{skip_holds_queue};
return $result;
}

Loading…
Cancel
Save