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: Needs documenting --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: david, Laura.escamilla, 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 Crowdfunding committed: 0
Crowdfunding contact: 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
Bug 41404: [25.05.x] Use looks_like_a_number to verify if prefs are set

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
Comment 11 Laura Escamilla 2026-02-02 18:41:41 UTC
This patch doesn’t apply cleanly to 25.05, so I’m not planning to backport it at this time. If you’re able to update it for 25.05 compatibility, I’d be happy to take another look.
Comment 12 Nick Clemens (kidclamp) 2026-02-13 21:15:25 UTC
Created attachment 193157 [details] [review]
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>
Comment 13 Nick Clemens (kidclamp) 2026-02-13 21:16:04 UTC
(In reply to Laura Escamilla from comment #11)
> This patch doesn’t apply cleanly to 25.05, so I’m not planning to backport
> it at this time. If you’re able to update it for 25.05 compatibility, I’d be
> happy to take another look.

Rebased for 25.05.x