From c19152d2dc7897b5ed6efbdffb4b17937ba6ad93 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Wed, 4 Mar 2026 17:21:05 +0000 Subject: [PATCH] Bug 41514: Enforce lead/trail periods server-side in booking API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, lead and trail period validation for bookings was only enforced in the UI (place_booking.js), allowing third-party API consumers to create bookings that violated the configured buffer zones. This patch adds server-side enforcement in the two check_booking() methods and the Booking->store() call: - Koha::Item::check_booking(): accepts an optional branchcode param, fetches bookings_lead_period and bookings_trail_period circ rules for the item's effective itemtype, expands the overlap query so that effective periods (core ± lead/trail) are checked rather than just core date ranges. Also expands the checkout-due-date check so that items must be returned at least lead_days before the booking start. - Koha::Biblio::check_booking(): same expansion using the global (itemtype-agnostic) rule as an approximation for biblios that have items of mixed types. The item-level check provides the definitive per-item validation. - Koha::Booking::store(): passes pickup_library_id as branchcode to both check_booking() calls so that branch-specific rules are used. Tests added to t/db_dependent/Koha/Booking.t covering: - booking ending in existing booking's lead period → clash - booking ending before effective period → OK - booking starting in existing booking's trail period → clash - new booking's lead period overlapping existing trail period → clash - booking starting after full effective period ends → OK - checkout due within lead period → clash - checkout due before lead period → OK Signed-off-by: David Nind --- Koha/Biblio.pm | 42 ++++++-- Koha/Booking.pm | 20 +++- Koha/Item.pm | 43 ++++++++- t/db_dependent/Koha/Booking.t | 177 +++++++++++++++++++++++++++++++++- 4 files changed, 266 insertions(+), 16 deletions(-) diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm index 1f3f0b45e1..26b3c72240 100644 --- a/Koha/Biblio.pm +++ b/Koha/Biblio.pm @@ -342,12 +342,16 @@ sub can_be_edited { =head3 check_booking my $bookable = - $biblio->check_booking( { start_date => $datetime, end_date => $datetime, [ booking_id => $booking_id ] } ); + $biblio->check_booking( { start_date => $datetime, end_date => $datetime, [ booking_id => $booking_id, branchcode => $branchcode, itemtype => $itemtype ] } ); Returns a boolean denoting whether the passed booking can be made without clashing. Optionally, you may pass a booking id to exclude from the checks; This is helpful when you are updating an existing booking. +Optionally, you may pass a branchcode to use when looking up lead/trail circulation rules. + +Optionally, you may pass an itemtype to use when looking up lead/trail circulation rules; falls back to a global rule if not provided. + =cut sub check_booking { @@ -356,11 +360,36 @@ sub check_booking { my $start_date = dt_from_string( $params->{start_date} ); my $end_date = dt_from_string( $params->{end_date} ); my $booking_id = $params->{booking_id}; + my $branchcode = $params->{branchcode}; + my $itemtype = $params->{itemtype}; + + my $lead_rule = Koha::CirculationRules->get_effective_rule( + { + rule_name => 'bookings_lead_period', + itemtype => $itemtype, + branchcode => $branchcode, + } + ); + my $lead_days = $lead_rule && defined $lead_rule->rule_value ? $lead_rule->rule_value : 0; + + my $trail_rule = Koha::CirculationRules->get_effective_rule( + { + rule_name => 'bookings_trail_period', + itemtype => $itemtype, + branchcode => $branchcode, + } + ); + my $trail_days = $trail_rule && defined $trail_rule->rule_value ? $trail_rule->rule_value : 0; my $bookable_items = $self->bookable_items; my $total_bookable = $bookable_items->count; - my $dtf = Koha::Database->new->schema->storage->datetime_parser; + my $dtf = Koha::Database->new->schema->storage->datetime_parser; + + # Expand bounds to account for lead/trail buffer zones on both sides + my $expanded_start = $start_date->clone->subtract( days => $lead_days + $trail_days ); + my $expanded_end = $end_date->clone->add( days => $trail_days + $lead_days ); + my $existing_bookings = $self->bookings( { '-and' => [ @@ -369,13 +398,13 @@ sub check_booking { start_date => { '-between' => [ $dtf->format_datetime($start_date), - $dtf->format_datetime($end_date) + $dtf->format_datetime($expanded_end), ] }, end_date => { '-between' => [ - $dtf->format_datetime($start_date), - $dtf->format_datetime($end_date) + $dtf->format_datetime($expanded_start), + $dtf->format_datetime($end_date), ] }, { @@ -400,7 +429,8 @@ sub check_booking { my $checkouts = $self->current_checkouts->search( { - date_due => { '>=' => $dtf->format_datetime($start_date) }, + # Item must be returned at least lead_days before booking starts + date_due => { '>=' => $dtf->format_datetime( $start_date->clone->subtract( days => $lead_days ) ) }, "me.itemnumber" => { '-not_in' => $existing_bookings->_resultset->get_column('item_id')->as_query, '-in' => $bookable_item_ids, diff --git a/Koha/Booking.pm b/Koha/Booking.pm index 2bd609b4e6..55fb3b423a 100644 --- a/Koha/Booking.pm +++ b/Koha/Booking.pm @@ -186,7 +186,8 @@ sub store { { start_date => $self->start_date, end_date => $self->end_date, - booking_id => $self->in_storage ? $self->booking_id : undef + booking_id => $self->in_storage ? $self->booking_id : undef, + branchcode => $self->pickup_library_id, } ); @@ -196,7 +197,9 @@ sub store { { start_date => $self->start_date, end_date => $self->end_date, - booking_id => $self->in_storage ? $self->booking_id : undef + booking_id => $self->in_storage ? $self->booking_id : undef, + branchcode => $self->pickup_library_id, + itemtype => $self->item_id ? $self->item->effective_itemtype : undef, } ); } @@ -282,8 +285,17 @@ sub _assign_item_for_booking { } ); - my $checkouts = - $biblio->current_checkouts->search( { date_due => { '>=' => $dtf->format_datetime($start_date) } } ); + my $lead_rule = Koha::CirculationRules->get_effective_rule( + { + rule_name => 'bookings_lead_period', + itemtype => $biblio->biblioitem->itemtype, + branchcode => $self->pickup_library_id, + } + ); + my $lead_days = $lead_rule && defined $lead_rule->rule_value ? $lead_rule->rule_value : 0; + + my $checkouts = $biblio->current_checkouts->search( + { date_due => { '>=' => $dtf->format_datetime( $start_date->clone->subtract( days => $lead_days ) ) } } ); # Build search conditions for bookable items my $item_search_conditions = { diff --git a/Koha/Item.pm b/Koha/Item.pm index ed175d3ced..786e40d283 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -726,12 +726,14 @@ sub find_booking { =head3 check_booking my $bookable = - $item->check_booking( { start_date => $datetime, end_date => $datetime, [ booking_id => $booking_id ] } ); + $item->check_booking( { start_date => $datetime, end_date => $datetime, [ booking_id => $booking_id, branchcode => $branchcode ] } ); Returns a boolean denoting whether the passed booking can be made without clashing. Optionally, you may pass a booking id to exclude from the checks; This is helpful when you are updating an existing booking. +Optionally, you may pass a branchcode to use when looking up lead/trail circulation rules. + =cut sub check_booking { @@ -740,13 +742,44 @@ sub check_booking { my $start_date = dt_from_string( $params->{start_date} ); my $end_date = dt_from_string( $params->{end_date} ); my $booking_id = $params->{booking_id}; + my $branchcode = $params->{branchcode}; + + # Get lead and trail periods for this item type to enforce buffer zones + my $lead_rule = Koha::CirculationRules->get_effective_rule( + { + rule_name => 'bookings_lead_period', + itemtype => $self->effective_itemtype, + branchcode => $branchcode, + } + ); + my $lead_days = $lead_rule && defined $lead_rule->rule_value ? $lead_rule->rule_value : 0; + + my $trail_rule = Koha::CirculationRules->get_effective_rule( + { + rule_name => 'bookings_trail_period', + itemtype => $self->effective_itemtype, + branchcode => $branchcode, + } + ); + my $trail_days = $trail_rule && defined $trail_rule->rule_value ? $trail_rule->rule_value : 0; if ( my $checkout = $self->checkout ) { - return 0 if ( $start_date <= dt_from_string( $checkout->date_due ) ); + + # Item must be returned at least lead_days before booking starts + return 0 + if ( dt_from_string( $checkout->date_due ) >= $start_date->clone->subtract( days => $lead_days ) ); } my $dtf = Koha::Database->new->schema->storage->datetime_parser; + # To check whether effective periods (core period ± lead/trail) overlap, we expand + # the query bounds. For items of the same effective itemtype the lead/trail values + # are identical for both the new and existing bookings, so the overlap condition + # becomes: existing_start <= new_end + trail + lead + # AND existing_end >= new_start - lead - trail + my $expanded_start = $start_date->clone->subtract( days => $lead_days + $trail_days ); + my $expanded_end = $end_date->clone->add( days => $trail_days + $lead_days ); + my $existing_bookings = $self->bookings( { '-and' => [ @@ -755,13 +788,13 @@ sub check_booking { start_date => { '-between' => [ $dtf->format_datetime($start_date), - $dtf->format_datetime($end_date) + $dtf->format_datetime($expanded_end), ] }, end_date => { '-between' => [ - $dtf->format_datetime($start_date), - $dtf->format_datetime($end_date) + $dtf->format_datetime($expanded_start), + $dtf->format_datetime($end_date), ] }, { diff --git a/t/db_dependent/Koha/Booking.t b/t/db_dependent/Koha/Booking.t index 77358ae5e5..df337771d3 100755 --- a/t/db_dependent/Koha/Booking.t +++ b/t/db_dependent/Koha/Booking.t @@ -20,13 +20,14 @@ use Modern::Perl; use utf8; -use Test::More tests => 7; +use Test::More tests => 8; use Test::NoWarnings; use Test::Warn; use Test::Exception; +use Koha::CirculationRules; use Koha::DateUtils qw( dt_from_string ); use Koha::Notice::Template; use Koha::Notice::Templates; @@ -1227,3 +1228,177 @@ subtest 'store() skips clash detection on terminal status transition' => sub { $schema->storage->txn_rollback; }; + +subtest 'lead/trail period enforcement tests' => sub { + plan tests => 8; + $schema->storage->txn_begin; + + my $patron = $builder->build_object( { class => "Koha::Patrons" } ); + t::lib::Mocks::mock_userenv( { patron => $patron } ); + + my $item = $builder->build_sample_item( { bookable => 1 } ); + my $branchcode = $item->homebranch; + my $itype = $item->effective_itemtype; + + # Set lead=3 days, trail=2 days + Koha::CirculationRules->set_rule( + { + branchcode => $branchcode, + itemtype => $itype, + rule_name => 'bookings_lead_period', + rule_value => 3, + } + ); + Koha::CirculationRules->set_rule( + { + branchcode => $branchcode, + itemtype => $itype, + rule_name => 'bookings_trail_period', + rule_value => 2, + } + ); + + # Existing booking B1: today+10 to today+20 + # B1 effective period (with lead=3, trail=2): today+7 to today+22 + my $b1_start = dt_from_string->add( days => 10 )->truncate( to => 'day' ); + my $b1_end = dt_from_string->add( days => 20 )->truncate( to => 'day' ); + my $booking1 = Koha::Booking->new( + { + patron_id => $patron->borrowernumber, + biblio_id => $item->biblio->biblionumber, + item_id => $item->itemnumber, + pickup_library_id => $branchcode, + start_date => $b1_start, + end_date => $b1_end, + } + )->store; + ok( $booking1->in_storage, 'First booking stored OK' ); + + # FAIL: B2 ends inside B1's lead period (B2: today+5 to today+8, B1 lead starts today+7) + throws_ok { + Koha::Booking->new( + { + patron_id => $patron->borrowernumber, + biblio_id => $item->biblio->biblionumber, + item_id => $item->itemnumber, + pickup_library_id => $branchcode, + start_date => dt_from_string->add( days => 5 )->truncate( to => 'day' ), + end_date => dt_from_string->add( days => 8 )->truncate( to => 'day' ), + } + )->store; + } + 'Koha::Exceptions::Booking::Clash', + 'Throws when new booking ends inside an existing booking lead period'; + + # SUCCESS: B2 ends before B1's effective period (B2: today+3 to today+4) + # B2 effective end = today+4+trail=today+6 < B1 effective start today+7 + my $booking2 = Koha::Booking->new( + { + patron_id => $patron->borrowernumber, + biblio_id => $item->biblio->biblionumber, + item_id => $item->itemnumber, + pickup_library_id => $branchcode, + start_date => dt_from_string->add( days => 3 )->truncate( to => 'day' ), + end_date => dt_from_string->add( days => 4 )->truncate( to => 'day' ), + } + )->store; + ok( $booking2->in_storage, 'Booking OK when it ends before existing booking effective period' ); + + # FAIL: B3 starts inside B1's trail period (B3: today+22 to today+25, B1 trail ends today+22) + throws_ok { + Koha::Booking->new( + { + patron_id => $patron->borrowernumber, + biblio_id => $item->biblio->biblionumber, + item_id => $item->itemnumber, + pickup_library_id => $branchcode, + start_date => dt_from_string->add( days => 22 )->truncate( to => 'day' ), + end_date => dt_from_string->add( days => 25 )->truncate( to => 'day' ), + } + )->store; + } + 'Koha::Exceptions::Booking::Clash', + 'Throws when new booking starts inside an existing booking trail period'; + + # FAIL: B3 lead period overlaps B1's trail period + # B3: today+23 to today+25, lead=3 → B3 eff start=today+20, which overlaps B1 end at today+20 + throws_ok { + Koha::Booking->new( + { + patron_id => $patron->borrowernumber, + biblio_id => $item->biblio->biblionumber, + item_id => $item->itemnumber, + pickup_library_id => $branchcode, + start_date => dt_from_string->add( days => 23 )->truncate( to => 'day' ), + end_date => dt_from_string->add( days => 25 )->truncate( to => 'day' ), + } + )->store; + } + 'Koha::Exceptions::Booking::Clash', + "Throws when new booking's lead period overlaps an existing booking's trail period"; + + # SUCCESS: B3 starts after B1's full effective period ends + # B3: today+26 to today+30, B3 eff start = today+26-3=today+23 > B1 eff end today+22 + my $booking3 = Koha::Booking->new( + { + patron_id => $patron->borrowernumber, + biblio_id => $item->biblio->biblionumber, + item_id => $item->itemnumber, + pickup_library_id => $branchcode, + start_date => dt_from_string->add( days => 26 )->truncate( to => 'day' ), + end_date => dt_from_string->add( days => 30 )->truncate( to => 'day' ), + } + )->store; + ok( + $booking3->in_storage, + 'Booking OK when new effective period starts after existing effective period ends' + ); + + # Checkout tests: item must be returned at least lead_days before booking start. + # Use dates outside all existing bookings' effective periods (B3 effective end = today+32, + # so B4 effective start must be > today+32 → B4 start >= today+36). + # FAIL: checkout due today+34, booking start today+36 (lead=3 → must return by today+33) + # today+34 >= today+33 → FAIL + $builder->build_object( + { + class => 'Koha::Checkouts', + value => { + itemnumber => $item->itemnumber, + date_due => dt_from_string->add( days => 34 )->truncate( to => 'day' )->datetime(q{ }), + } + } + ); + throws_ok { + Koha::Booking->new( + { + patron_id => $patron->borrowernumber, + biblio_id => $item->biblio->biblionumber, + item_id => $item->itemnumber, + pickup_library_id => $branchcode, + start_date => dt_from_string->add( days => 36 )->truncate( to => 'day' ), + end_date => dt_from_string->add( days => 40 )->truncate( to => 'day' ), + } + )->store; + } + 'Koha::Exceptions::Booking::Clash', + 'Throws when checked-out item is due within the booking lead period'; + + # SUCCESS: checkout due today+32, booking start today+36 (lead=3 → must return by today+33) + # today+32 < today+33 → OK + # Update the checkout to an earlier due date + Koha::Checkouts->search( { itemnumber => $item->itemnumber } ) + ->update( { date_due => dt_from_string->add( days => 32 )->truncate( to => 'day' )->datetime(q{ }) } ); + my $booking4 = Koha::Booking->new( + { + patron_id => $patron->borrowernumber, + biblio_id => $item->biblio->biblionumber, + item_id => $item->itemnumber, + pickup_library_id => $branchcode, + start_date => dt_from_string->add( days => 36 )->truncate( to => 'day' ), + end_date => dt_from_string->add( days => 40 )->truncate( to => 'day' ), + } + )->store; + ok( $booking4->in_storage, 'Booking OK when checkout is due before the lead period starts' ); + + $schema->storage->txn_rollback; +}; -- 2.39.5