From 64cc9017f229f5ae43829dbf974fb2d3db0122a8 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Mon, 8 Dec 2025 18:34:36 +0000 Subject: [PATCH] Bug 41404: [25.05.x] Use looks_like_a_number to verify if prefs are set Bug 41404: Use looks_like_a_number to verify if prefs are set This check adds a check to ensure we have number, not just defined To test: 1 - Apply only unit tests patch 2 - Run the tests prove -v t/db_dependent/Koha/Patron.t 3 - Apply this patch 4 - Run the tests 5 - They pass! Signed-off-by: David Nind Signed-off-by: Martin Renvoize Bug 41404: (QA follow-up) Add tests for empty string and zero edge cases This QA follow-up adds comprehensive test coverage for edge cases that the original patch handles: 1. Empty string values ('') - the default value in sysprefs - Verifies that empty strings are treated as "not set" - Confirms charge calculations are skipped (performance optimization) - Ensures patrons are not blocked 2. Zero values (0) - valid numeric limit - Verifies that zero triggers charge calculation (valid number) - Confirms patrons are not blocked (0 means "no limit") - Documents intended behavior that 0 disables blocking 3. Positive numeric values - normal case - Confirms charges are calculated and blocking works correctly These tests validate that looks_like_number() is the correct approach rather than simple truthiness checking, as it properly distinguishes between: - Empty string '' (not a number, skip calculation) - Zero 0 (is a number, calculate but don't block) Test plan: 1. Apply this patch 2. Run the tests: prove t/db_dependent/Koha/Patron.t 3. Verify all tests pass Signed-off-by: Martin Renvoize --- Koha/Patron.pm | 9 ++- t/db_dependent/Koha/Patron.t | 127 ++++++++++++++++++++++++++++++++++- 2 files changed, 132 insertions(+), 4 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 339b990a9e3..b06341ded54 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -22,6 +22,7 @@ use Modern::Perl; use List::MoreUtils qw( any none uniq notall zip6); use JSON qw( to_json ); +use Scalar::Util qw(looks_like_number); use Unicode::Normalize qw( NFKD ); use Try::Tiny; use DateTime (); @@ -1113,7 +1114,7 @@ sub set_password { module => 'members', letter_code => 'PASSWORD_CHANGE', branchcode => $self_from_storage->branchcode, - , + lang => $self_from_storage->lang || 'default', tables => { 'branches' => $self_from_storage->branchcode, @@ -3379,7 +3380,7 @@ sub is_patron_inside_charge_limits { my $guarantors_non_issues_charges = 0; # Check the debt of this patrons guarantees - if ( defined $no_issues_charge_guarantees ) { + if ( defined $no_issues_charge_guarantees && looks_like_number($no_issues_charge_guarantees) ) { my @guarantees = map { $_->guarantee } $patron->guarantee_relationships->as_list; foreach my $g (@guarantees) { $guarantees_non_issues_charges += $g->account->non_issues_charges; @@ -3387,7 +3388,9 @@ sub is_patron_inside_charge_limits { } # Check the debt of this patrons guarantors *and* the guarantees of those guarantors - if ( defined $no_issues_charge_guarantors_with_guarantees ) { + if ( defined $no_issues_charge_guarantors_with_guarantees + && looks_like_number($no_issues_charge_guarantors_with_guarantees) ) + { $guarantors_non_issues_charges = $patron->relationships_debt( { include_guarantors => 1, only_this_guarantor => 0, include_this_patron => 1 } ); } diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t index ae9e999c954..fdfd8431c8a 100755 --- a/t/db_dependent/Koha/Patron.t +++ b/t/db_dependent/Koha/Patron.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 42; +use Test::More tests => 43; use Test::NoWarnings; use Test::Exception; use Test::Warn; @@ -2952,6 +2952,131 @@ subtest 'is_patron_inside_charge_limits() tests' => sub { $schema->storage->txn_rollback; }; +subtest 'is_patron_inside_charge_limits - edge cases for empty string and zero' => sub { + plan tests => 14; + + $schema->storage->txn_begin; + + my $patron_category = $builder->build( + { + source => 'Category', + value => { + categorycode => 'TEST_CAT', + category_type => 'P', + enrolmentfee => 0, + noissuescharge => undef, + noissueschargeguarantees => undef, + noissueschargeguarantorswithguarantees => undef + } + } + ); + + my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + value => { categorycode => $patron_category->{categorycode} } + } + ); + + my $child = $builder->build_object( { class => 'Koha::Patrons' } ); + $child->add_guarantor( { guarantor_id => $patron->borrowernumber, relationship => 'parent' } ); + + # Add some charges to the child + my $fee = $builder->build_object( + { + class => 'Koha::Account::Lines', + value => { + borrowernumber => $child->borrowernumber, + amountoutstanding => 5.50, + debit_type_code => 'OVERDUE', + } + } + )->store; + + # Test 1: Empty string values should be treated as "not set" + # Empty string is the default value in sysprefs for these preferences + t::lib::Mocks::mock_preference( 'NoIssuesChargeGuarantees', '' ); + t::lib::Mocks::mock_preference( 'NoIssuesChargeGuarantorsWithGuarantees', '' ); + + my $patron_borrowing_status = $patron->is_patron_inside_charge_limits(); + + is( + $patron_borrowing_status->{NoIssuesChargeGuarantees}->{limit}, '', + "Empty string limit is preserved in return value" + ); + is( + $patron_borrowing_status->{NoIssuesChargeGuarantees}->{charge}, 0, + "Charges should be 0 when limit is empty string (calculation skipped)" + ); + is( + $patron_borrowing_status->{NoIssuesChargeGuarantees}->{overlimit}, 0, + "Patron should not be overlimit when preference is empty string" + ); + + is( + $patron_borrowing_status->{NoIssuesChargeGuarantorsWithGuarantees}->{limit}, '', + "Empty string limit is preserved for guarantors preference" + ); + is( + $patron_borrowing_status->{NoIssuesChargeGuarantorsWithGuarantees}->{charge}, 0, + "Guarantors charges should be 0 when limit is empty string (calculation skipped)" + ); + is( + $patron_borrowing_status->{NoIssuesChargeGuarantorsWithGuarantees}->{overlimit}, 0, + "Patron should not be overlimit when guarantors preference is empty string" + ); + + # Test 2: Zero values should trigger calculation but not block + t::lib::Mocks::mock_preference( 'NoIssuesChargeGuarantees', 0 ); + t::lib::Mocks::mock_preference( 'NoIssuesChargeGuarantorsWithGuarantees', 0 ); + + $patron_borrowing_status = $patron->is_patron_inside_charge_limits(); + + is( + $patron_borrowing_status->{NoIssuesChargeGuarantees}->{limit}, 0, + "Zero limit is preserved in return value" + ); + is( + $patron_borrowing_status->{NoIssuesChargeGuarantees}->{charge}, 5.50, + "Charges should be calculated when limit is 0 (valid number)" + ); + is( + $patron_borrowing_status->{NoIssuesChargeGuarantees}->{overlimit}, 0, + "Patron should not be overlimit when limit is 0 (0 means no blocking)" + ); + + is( + $patron_borrowing_status->{NoIssuesChargeGuarantorsWithGuarantees}->{limit}, 0, + "Zero limit is preserved for guarantors preference" + ); + + # Note: This will be 5.50 because the child is included in guarantor debt calculation + ok( + $patron_borrowing_status->{NoIssuesChargeGuarantorsWithGuarantees}->{charge} > 0, + "Guarantors charges should be calculated when limit is 0 (valid number)" + ); + is( + $patron_borrowing_status->{NoIssuesChargeGuarantorsWithGuarantees}->{overlimit}, 0, + "Patron should not be overlimit when guarantors limit is 0 (0 means no blocking)" + ); + + # Test 3: Verify that a small positive number triggers calculation and can block + t::lib::Mocks::mock_preference( 'NoIssuesChargeGuarantees', 5.00 ); + + $patron_borrowing_status = $patron->is_patron_inside_charge_limits(); + + is( + $patron_borrowing_status->{NoIssuesChargeGuarantees}->{charge}, 5.50, + "Charges are calculated with positive limit" + ); + is( + $patron_borrowing_status->{NoIssuesChargeGuarantees}->{overlimit}, 1, + "Patron is correctly marked overlimit when charges exceed limit" + ); + + $schema->storage->txn_rollback; +}; + subtest 'Scrub the note fields' => sub { plan tests => 4; -- 2.39.5