From 2da3248b41c3aeae814b1392ead13c7821d0a144 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Mon, 6 Jul 2026 10:14:04 +0100 Subject: [PATCH] Bug 42997: Prevent renewals running into the next booking CanBookBeRenewed checked bookings by asking Koha::Item->find_booking for the first booking that clashes with the renewal period. When the renewing patron's own booking satisfies this checkout, that booking is returned first and renewal is allowed - subsequent bookings for the item were never considered. A patron could therefore renew a booked item straight through the start of the next patron's booking. Additionally, when staff renew with an explicit due date (the renewal due date input on the checkouts screen, or the hard due date on circ/renew.pl), CanBookBeRenewed validated a *calculated* due date whilst AddRenewal then applied the *requested* one, so the requested date was never checked against bookings at all. This patch: * repeats the booking lookup with the patron's own booking excluded, refusing the renewal ('booked') when it would run into the next booking for the item * adds an optional $date_due parameter to CanBookBeRenewed, used in place of the calculated due date for the bookings check, and threads the requested date through from svc/renew and circ/renew.pl Test plan: 1) Run t/db_dependent/Circulation.t - the extended 'CanBookBeRenewed | bookings' subtest covers both the calculated and requested due date cases 2) Check out a booked item to its patron, add a subsequent booking for another patron, and attempt to renew past the second booking's start date from the checkouts screen; confirm the renewal is refused as 'booked' Sponsored-by: OpenFifth Sponsored-by: Westminster City Council Signed-off-by: Hannah Dunne-Howrie Signed-off-by: Paul Derscheid Signed-off-by: Pedro Amorim --- C4/Circulation.pm | 27 +++++++++++++++-- circ/renew.pl | 7 ++++- svc/renew | 2 +- t/db_dependent/Circulation.t | 57 +++++++++++++++++++++++++++++++++++- 4 files changed, 87 insertions(+), 6 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 211de86d21..d7b1294f02 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -3331,7 +3331,7 @@ sub GetUpcomingDueIssues { =head2 CanBookBeRenewed - ($ok,$error,$info) = &CanBookBeRenewed($patron, $issue, $override_limit, $cron); + ($ok,$error,$info) = &CanBookBeRenewed($patron, $issue, $override_limit, $cron, $date_due); Find out whether a borrowed item may be renewed. @@ -3348,6 +3348,11 @@ the limit on the number of times that the loan can be renewed to renew sooner than "No renewal before" and to manually renew loans that are automatically renewed. +C<$date_due>, optional, is the requested due date for the renewal when +the caller intends to renew to a specific date rather than let the +circulation rules calculate one. It is used in place of the calculated +due date when checking the renewal against bookings for the item. + C<$CanBookBeRenewed> returns a true value if the item may be renewed. The item must currently be on loan to the specified borrower; renewals must be allowed for the item's type; and the borrower must not have @@ -3359,7 +3364,7 @@ already renewed the loan. =cut sub CanBookBeRenewed { - my ( $patron, $issue, $override_limit, $cron ) = @_; + my ( $patron, $issue, $override_limit, $cron, $date_due ) = @_; my $auto_renew = "no"; my $soonest; @@ -3524,7 +3529,10 @@ sub CanBookBeRenewed { ( C4::Context->preference('ManualRenewalPeriodBase') eq 'date_due' ) ? dt_from_string( $issue->date_due, 'sql' ) : dt_from_string(); - my $datedue = CalcDateDue( $startdate, $item->effective_itemtype, $branchcode, $patron, 'is a renewal' ); + my $datedue = + $date_due + ? dt_from_string($date_due) + : CalcDateDue( $startdate, $item->effective_itemtype, $branchcode, $patron, 'is a renewal' ); if ( my $booking = $item->find_booking( { checkout_date => $startdate, due_date => $datedue, patron_id => $patron->borrowernumber } @@ -3532,6 +3540,19 @@ sub CanBookBeRenewed { ) { return ( 0, 'booked' ) unless ( $booking->patron_id == $patron->borrowernumber ); + + # The renewal must not run into the next booking for this item, + # regardless of whose booking that is; the patron's own booking + # hides any subsequent ones from the lookup above, so repeat it + # with their booking excluded + return ( 0, 'booked' ) + if $item->find_booking( + { + checkout_date => $startdate, + due_date => $datedue, + exclude_booking_id => $booking->booking_id, + } + ); } } diff --git a/circ/renew.pl b/circ/renew.pl index 67df976745..e15f59c63f 100755 --- a/circ/renew.pl +++ b/circ/renew.pl @@ -72,7 +72,12 @@ if ( $op eq 'cud-renew' && $barcode ) { my $confirmations; my $can_renew; my $info; - ( $can_renew, $error, $info ) = CanBookBeRenewed( $patron, $checkout, $override_limit ); + my $requested_due_date = + ( C4::Context->preference('SpecifyDueDate') && $hard_due_date ) + ? $hard_due_date + : undef; + ( $can_renew, $error, $info ) = + CanBookBeRenewed( $patron, $checkout, $override_limit, undef, $requested_due_date ); push @{$confirmations}, 'RENEWAL_LIMIT' if $override_limit; if ( $error && ( $error eq 'on_reserve' ) ) { diff --git a/svc/renew b/svc/renew index faff4c822a..be2e51d1cf 100755 --- a/svc/renew +++ b/svc/renew @@ -66,7 +66,7 @@ my $item = Koha::Items->find($itemnumber); my $confirmations; ( $data->{renew_okay}, $data->{error} ) = - CanBookBeRenewed( $patron, $item->checkout, $override_limit ); + CanBookBeRenewed( $patron, $item->checkout, $override_limit, undef, $date_due ); # If we're allowing reserved items to be renewed... if ( $data->{error} && $data->{error} eq 'on_reserve' && C4::Context->preference('AllowRenewalOnHoldOverride') ) { diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 7564d18d90..ed09de6a97 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -2009,7 +2009,7 @@ subtest "CanBookBeRenewed tests" => sub { }; subtest "CanBookBeRenewed | bookings" => sub { - plan tests => 3; + plan tests => 8; my $schema = Koha::Database->schema; $schema->storage->txn_begin; @@ -2044,6 +2044,61 @@ subtest "CanBookBeRenewed | bookings" => sub { is( $renewok, 0, "Renewal not allowed as it would mean the item was not returned before the next booking" ); is( $error, 'booked', "Error is 'booked'" ); + # Renewing patron has their own booking satisfied by this checkout; + # the renewal still must not run into the next booking for the item + $booking->delete(); + + my $own_booking = $builder->build_object( + { + class => 'Koha::Bookings', + value => { + patron_id => $renewing_patron->borrowernumber, + pickup_library_id => $pickup_library->branchcode, + item_id => $item->itemnumber, + biblio_id => $item->biblio->biblionumber, + start_date => dt_from_string()->subtract( days => 1 ), + end_date => $datedue->clone(), + status => 'issued', + } + } + ); + $issue->booking_id( $own_booking->booking_id )->store(); + $issue->discard_changes; + + my $next_booking = $builder->build_object( + { + class => 'Koha::Bookings', + value => { + patron_id => $booked_patron->borrowernumber, + pickup_library_id => $pickup_library->branchcode, + item_id => $item->itemnumber, + biblio_id => $item->biblio->biblionumber, + start_date => $datedue->clone()->add( days => 2 ), + end_date => $datedue->clone()->add( days => 10 ), + status => 'new', + } + } + ); + + # Calculated renewal due date (date_due + renewal period) runs into the next booking + ( $renewok, $error ) = CanBookBeRenewed( $renewing_patron, $issue, 0 ); + is( $renewok, 0, "Renewal with own booking not allowed when it would run into the next booking" ); + is( $error, 'booked', "Error is 'booked'" ); + + # An explicitly requested due date is checked instead of the calculated one + ( $renewok, $error ) = CanBookBeRenewed( + $renewing_patron, $issue, 0, undef, + $datedue->clone()->add( days => 3 ) + ); + is( $renewok, 0, "Requested due date running into the next booking is refused" ); + is( $error, 'booked', "Error is 'booked'" ); + + ( $renewok, $error ) = CanBookBeRenewed( + $renewing_patron, $issue, 0, undef, + $datedue->clone()->add( days => 1 ) + ); + is( $renewok, 1, "Requested due date clear of the next booking is allowed" ); + $schema->storage->txn_rollback; };