Bugzilla – Attachment 193157 Details for
Bug 41404
No need to check related guarantor/guarantee charges when the limits are not set
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 41404: [25.05.x] Use looks_like_a_number to verify if prefs are set
Bug-41404-2505x-Use-lookslikeanumber-to-verify-if-.patch (text/plain), 9.09 KB, created by
Nick Clemens (kidclamp)
on 2026-02-13 21:15:25 UTC
(
hide
)
Description:
Bug 41404: [25.05.x] Use looks_like_a_number to verify if prefs are set
Filename:
MIME Type:
Creator:
Nick Clemens (kidclamp)
Created:
2026-02-13 21:15:25 UTC
Size:
9.09 KB
patch
obsolete
>From 64cc9017f229f5ae43829dbf974fb2d3db0122a8 Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >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 <david@davidnind.com> >Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> > >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 <martin.renvoize@openfifth.co.uk> >--- > 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
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 41404
:
190307
|
190313
|
190322
|
190699
|
190700
| 193157