Bug 41404

Summary: No need to check related guarantor/guarantee charges when the limits are not set
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens (kidclamp) <nick>
Status: Pushed to stable --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: david, martin.renvoize
Version: MainKeywords: release-notes-needed
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41405
GIT URL: Initiative type: ---
Sponsorship status: --- Comma delimited list of Sponsors:
Crowdfunding goal: 0 Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
26.05.00,25.11.01
Circulation function:
Attachments: Bug 41404: Use looks_like_a_number to verify if prefs are set
Bug 41404: Use looks_like_a_number to verify if prefs are set
Bug 41404: Use looks_like_a_number to verify if prefs are set
Bug 41404: Use looks_like_a_number to verify if prefs are set
Bug 41404: (QA follow-up) Add tests for empty string and zero edge cases

Description Nick Clemens (kidclamp) 2025-12-08 18:33:53 UTC
In the code:
# Check the debt of this patrons guarantees
    if ( defined $no_issues_charge_guarantees ) {

     # Check the debt of this patrons guarantors *and* the guarantees of those guarantors
    if ( defined $no_issues_charge_guarantors_with_guarantees ) {

It is not possible to undef the system preferences, they are set as blank for no value - we need to check if they are numbers
Comment 1 Nick Clemens (kidclamp) 2025-12-08 18:36:52 UTC
Created attachment 190307 [details] [review]
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!
Comment 2 David Nind 2025-12-08 21:58:59 UTC Comment hidden (obsolete)
Comment 3 David Nind 2025-12-08 21:59:53 UTC
There was no unit tests patch (maybe it is missing), but the tests passed after applying the patch.
Comment 4 David Nind 2025-12-09 00:54:02 UTC
Created attachment 190322 [details] [review]
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>
Comment 5 David Nind 2025-12-09 00:54:26 UTC
My sign-off line was incorrect, so have updated.
Comment 6 Martin Renvoize (ashimema) 2025-12-23 14:22:16 UTC
1) Should we also make the same change for noissuescharge?
2) Remind me.. is '0' disabled or 'block all' in this case?
Comment 7 Martin Renvoize (ashimema) 2025-12-23 14:24:41 UTC
Created attachment 190699 [details] [review]
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>
Comment 8 Martin Renvoize (ashimema) 2025-12-23 14:24:42 UTC
Created attachment 190700 [details] [review]
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>
Comment 9 Lucas Gass (lukeg) 2025-12-29 17:00:40 UTC
Nice work everyone!

Pushed to main for 26.05
Comment 10 Jacob O'Mara 2026-01-16 17:19:30 UTC
Thanks all, pushed to 25.11.x