gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Bug 41887: Skip clash detection on terminal status transition 

- Booking::store runs item-level and biblio-level clash
  detection unconditionally on every call
- During checkout, C4::Circulation sets the booking status to
  'completed' and calls ->store(), which triggers clash checks
- Combined with false positives from Biblio::check_booking
  (e.g. checkouts on non-bookable sibling items inflating the
  unavailable count), this throws
  Koha::Exceptions::Booking::Clash resulting in a 500 error
- Running clash detection on a terminal transition is
  unnecessary regardless; skip it for cancelled/completed
- Add _is_final_status_transition() helper that inspects the
  dirty columns to detect completed/cancelled transitions
- Guard both clash detection blocks with the new helper

To test:
1. Have a biblio with two items: one bookable, one not
   (bookable = 0).
2. Create a booking on the bookable item.
3. Check out the non-bookable item to a patron.
4. Check out the bookable item to the booking patron (this
   triggers the completed transition via C4::Circulation).
5. Without the patch: 500 error from
   Koha::Exceptions::Booking::Clash.
6. Apply the patch.
7. Repeat steps 2-4.
8. Verify the checkout succeeds and the booking transitions
   to completed.
9. Run: prove t/db_dependent/Koha/Booking.t
   and confirm the new subtest "store() skips clash
   detection on terminal status transition" passes.

Sponsored-by: Büchereizentrale Schleswig-Holstein <https://www.bz-sh.de/>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
This commit is contained in:
Paul Derscheid 2026-02-19 10:40:07 +01:00 committed by Lucas Gass
parent c39021d1bc
commit ee667d3ddf
No known key found for this signature in database
GPG Key ID: 60B447398B0D36CA
2 changed files with 97 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

67
Koha/Booking.pm
View File

@ -173,25 +173,33 @@ sub store {
value => $self->biblio_id,
) unless ( $self->biblio );
# Throw exception for item level booking clash
Koha::Exceptions::Booking::Clash->throw()
if $self->item_id && !$self->item->check_booking(
{
start_date => $self->start_date,
end_date => $self->end_date,
booking_id => $self->in_storage ? $self->booking_id : undef
}
);
# Skip clash detection when transitioning to a final
# status. During checkout C4::Circulation sets status
# to 'completed' and calls ->store; the clash checks
# are irrelevant at that point and can produce false
# positives.
unless ( $self->_is_final_status_transition ) {
# Throw exception for biblio level booking clash
Koha::Exceptions::Booking::Clash->throw()
if !$self->biblio->check_booking(
{
start_date => $self->start_date,
end_date => $self->end_date,
booking_id => $self->in_storage ? $self->booking_id : undef
}
);
# Throw exception for item level booking clash
Koha::Exceptions::Booking::Clash->throw()
if $self->item_id && !$self->item->check_booking(
{
start_date => $self->start_date,
end_date => $self->end_date,
booking_id => $self->in_storage ? $self->booking_id : undef
}
);
# Throw exception for biblio level booking clash
Koha::Exceptions::Booking::Clash->throw()
if !$self->biblio->check_booking(
{
start_date => $self->start_date,
end_date => $self->end_date,
booking_id => $self->in_storage ? $self->booking_id : undef
}
);
}
# FIXME: We should be able to combine the above two functions into one
@ -336,6 +344,29 @@ sub to_api_mapping {
=head2 Internal methods
=head3 _is_final_status_transition
my $bool = $self->_is_final_status_transition;
Returns true when the booking is being transitioned to a final status
(cancelled or completed). Used to skip clash detection that is only
meaningful for active bookings.
=cut
sub _is_final_status_transition {
my ($self) = @_;
return 0 unless $self->in_storage;
my $updated_columns = { $self->_result->get_dirty_columns };
my $new_status = $updated_columns->{status};
return 0 unless $new_status;
return 1 if any { $_ eq $new_status } qw( cancelled completed );
return 0;
}
=head3 _select_optimal_item
my $item_id = $self->_select_optimal_item($available_items);

49
t/db_dependent/Koha/Booking.t
View File

@ -20,7 +20,7 @@
use Modern::Perl;
use utf8;
use Test::More tests => 6;
use Test::More tests => 7;
use Test::NoWarnings;
use Test::Warn;
@ -1180,3 +1180,50 @@ subtest 'Integration test: Full optimal selection workflow' => sub {
$schema->storage->txn_rollback;
};
subtest 'store() skips clash detection on terminal status transition' => sub {
plan tests => 3;
$schema->storage->txn_begin;
my $patron = $builder->build_object( { class => "Koha::Patrons" } );
t::lib::Mocks::mock_userenv( { patron => $patron } );
my $biblio = $builder->build_sample_biblio();
my $bookable_item = $builder->build_sample_item( { biblionumber => $biblio->biblionumber, bookable => 1 } );
my $non_bookable_item = $builder->build_sample_item( { biblionumber => $biblio->biblionumber, bookable => 0 } );
my $start = dt_from_string()->truncate( to => 'day' );
my $end = $start->clone->add( days => 7 );
# Create booking first, before any checkouts exist so
# store() succeeds without interference from Bug 41886.
my $booking = Koha::Booking->new(
{
patron_id => $patron->borrowernumber,
biblio_id => $biblio->biblionumber,
item_id => $bookable_item->itemnumber,
pickup_library_id => $bookable_item->homebranch,
start_date => $start,
end_date => $end,
}
)->store();
ok( $booking->in_storage, 'Booking on bookable item stored OK' );
# Now check out the non-bookable sibling item. This
# checkout inflates the unavailable count in
# Biblio::check_booking (see Bug 41886) and causes a
# false clash when there is only one bookable item.
C4::Circulation::AddIssue( $patron, $non_bookable_item->barcode );
# Without the fix, transitioning to 'completed' runs clash
# detection which sees the non-bookable checkout and throws
# Koha::Exceptions::Booking::Clash → 500 error.
lives_ok { $booking->status('completed')->store() }
'Transition to completed skips clash detection';
# Cancellation from completed is also a terminal transition
lives_ok { $booking->status('cancelled')->store() }
'Transition to cancelled skips clash detection';
$schema->storage->txn_rollback;
};