Bugzilla – Attachment 194462 Details for
Bug 41514
The API should fully respect lead and trail periods for bookings
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 41514: Enforce lead/trail periods server-side in booking API
039bbd6.patch (text/plain), 19.76 KB, created by
Martin Renvoize (ashimema)
on 2026-03-04 18:17:00 UTC
(
hide
)
Description:
Bug 41514: Enforce lead/trail periods server-side in booking API
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2026-03-04 18:17:00 UTC
Size:
19.76 KB
patch
obsolete
>From 039bbd6a2d11de39b21df7a0a8b9d6685c75e580 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >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 >--- > 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 1f3f0b45e13..26b3c722400 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 2bd609b4e66..55fb3b423ad 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 ed175d3ced5..786e40d2831 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 77358ae5e50..df337771d3e 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.53.0 >
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 41514
:
194462
|
194476