From b7179075e482a3941a2b1a873909707d13c852c4 Mon Sep 17 00:00:00 2001 From: Martin Renvoize 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