Bugzilla – Attachment 191258 Details for
Bug 37966
When overriding a hold to renew a book the due date becomes "now" if not specified
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 37966: (follow-up) Make empty string handling more explicit in Circulation.pm
b717907.patch (text/plain), 5.52 KB, created by
Martin Renvoize (ashimema)
on 2026-01-12 14:07:25 UTC
(
hide
)
Description:
Bug 37966: (follow-up) Make empty string handling more explicit in Circulation.pm
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2026-01-12 14:07:25 UTC
Size:
5.52 KB
patch
obsolete
>From b7179075e482a3941a2b1a873909707d13c852c4 Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@openfifth.co.uk> >Date: Mon, 12 Jan 2026 14:02:58 +0000 >Subject: [PATCH] Bug 37966: (follow-up) Make empty string handling more > explicit in Circulation.pm >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >This commit improves upon the original fix by making the intent more >explicit when handling empty string date parameters in AddRenewal and >AddIssue functions. > >WHY THIS IS BETTER: > >The original fix (changing `defined $datedue` to just `$datedue`) >works correctly but relies on Perl's implicit truthy/falsy semantics: > > if ( $datedue && ref $datedue ne 'DateTime' ) > >This is subtle because: >- Empty string "" is falsy, so parsing is skipped รข >- But it's not immediately obvious that empty strings are being > handled specially >- The code doesn't explicitly show its intent > >THIS APPROACH: > > if ( defined $datedue && $datedue ne '' && ref $datedue ne 'DateTime' ) > >Benefits: >1. EXPLICIT: Clearly states "only parse if defined AND not empty > AND not already a DateTime" >2. SELF-DOCUMENTING: Intent is obvious to future developers >3. NO RELIANCE on truthy/falsy semantics >4. CONSISTENT: Same pattern applied across all date parameter handling > >COMPREHENSIVE CHANGES: > >1. AddRenewal (C4/Circulation.pm:3449) > - Made datedue empty string check explicit > >2. AddIssue (C4/Circulation.pm:1611) > - Made datedue empty string check explicit > - Also updated issuedate handling (line 1616) to explicitly handle > empty strings > >3. Test Coverage (t/db_dependent/Circulation/issue.t) > - Added test for AddIssue with empty string datedue > - Added test for AddIssue with empty string issuedate > - Ensures both functions properly calculate dates from circulation > rules instead of using "now" > >The existing test for AddRenewal already covered empty string datedue >(added in the original Bug 37966 commit). These new tests ensure the >same correct behavior for AddIssue. >--- > C4/Circulation.pm | 6 +-- > t/db_dependent/Circulation/issue.t | 69 +++++++++++++++++++++++++++++- > 2 files changed, 71 insertions(+), 4 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index aeeb730801d..9bc5343c02f 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -1608,12 +1608,12 @@ sub AddIssue { > > my $issue; > >- if ( $datedue && ref $datedue ne 'DateTime' ) { >+ if ( defined $datedue && $datedue ne '' && ref $datedue ne 'DateTime' ) { > $datedue = dt_from_string($datedue); > } > > # $issuedate defaults to today. >- if ( !defined $issuedate ) { >+ if ( !defined $issuedate || $issuedate eq '' ) { > $issuedate = dt_from_string(); > } else { > if ( ref $issuedate ne 'DateTime' ) { >@@ -3446,7 +3446,7 @@ sub AddRenewal { > > $borrowernumber ||= $issue->borrowernumber; > >- if ( $datedue && ref $datedue ne 'DateTime' ) { >+ if ( defined $datedue && $datedue ne '' && ref $datedue ne 'DateTime' ) { > $datedue = dt_from_string( $datedue, 'sql' ); > } > >diff --git a/t/db_dependent/Circulation/issue.t b/t/db_dependent/Circulation/issue.t >index d04248e6ad9..67f62451f31 100755 >--- a/t/db_dependent/Circulation/issue.t >+++ b/t/db_dependent/Circulation/issue.t >@@ -18,7 +18,7 @@ > use Modern::Perl; > > use Test::NoWarnings; >-use Test::More tests => 69; >+use Test::More tests => 71; > use DateTime::Duration; > > use t::lib::Mocks; >@@ -454,6 +454,73 @@ ok( > q{AddRenewal with a defined but empty due date does not set the due date to today} > ); > >+# Bug 37966: Test AddIssue with empty string parameters >+# Update the default circulation rule to have a non-zero issuelength >+Koha::CirculationRules->set_rules( >+ { >+ itemtype => '*', >+ categorycode => '*', >+ branchcode => '*', >+ rules => { >+ lengthunit => 'days', >+ issuelength => 7, >+ } >+ } >+); >+$dbh->do("DELETE FROM old_issues"); >+AddReturn($barcode_1); >+my $issue_empty_datedue = C4::Circulation::AddIssue( $patron_1, $barcode_1, "", 0, undef ); >+my $expected_datedue = dt_from_string()->add( days => 7 ); >+my $expected_datedue_date = output_pref( >+ { >+ dt => $expected_datedue, >+ dateformat => 'iso', >+ timeformat => '24hr', >+ dateonly => 1 >+ } >+); >+my $issue_empty_datedue_date = output_pref( >+ { >+ dt => dt_from_string( $issue_empty_datedue->date_due ), >+ dateformat => 'iso', >+ timeformat => '24hr', >+ dateonly => 1 >+ } >+); >+is( >+ $issue_empty_datedue_date, $expected_datedue_date, >+ q{AddIssue with empty string datedue calculates due date based on circulation rules} >+); >+ >+AddReturn($barcode_1); >+my $issue_empty_issuedate = C4::Circulation::AddIssue( $patron_1, $barcode_1, undef, 0, "" ); >+my $issue_check = Koha::Checkouts->find( { itemnumber => $item_id1 } ); >+my $issue_issuedate = output_pref( >+ { >+ dt => dt_from_string( $issue_check->issuedate ), >+ dateformat => 'iso', >+ timeformat => '24hr', >+ dateonly => 1 >+ } >+); >+ok( >+ $issue_issuedate eq $today, >+ q{AddIssue with empty string issuedate defaults to today} >+); >+ >+# Restore default circulation rule >+Koha::CirculationRules->set_rules( >+ { >+ itemtype => '*', >+ categorycode => '*', >+ branchcode => '*', >+ rules => { >+ lengthunit => 'days', >+ issuelength => 0, >+ } >+ } >+); >+ > $dbh->do("DELETE FROM old_issues"); > AddReturn($barcode_1); > my $return = >-- >2.52.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 37966
:
171790
|
179490
|
180478
|
188524
|
190590
|
190591
|
191256
|
191257
| 191258 |
191259