Bug 41404 - No need to check related guarantor/guarantee charges when the limits are not set
Summary: No need to check related guarantor/guarantee charges when the limits are not set
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords: release-notes-needed
Depends on:
Blocks:
 
Reported: 2025-12-08 18:33 UTC by Nick Clemens (kidclamp)
Modified: 2026-01-16 17:19 UTC (History)
2 users (show)

See Also:
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 (1.94 KB, patch)
2025-12-08 18:36 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 41404: Use looks_like_a_number to verify if prefs are set (1.98 KB, patch)
2025-12-08 21:58 UTC, David Nind
Details | Diff | Splinter Review
Bug 41404: Use looks_like_a_number to verify if prefs are set (1.99 KB, patch)
2025-12-09 00:54 UTC, David Nind
Details | Diff | Splinter Review
Bug 41404: Use looks_like_a_number to verify if prefs are set (2.06 KB, patch)
2025-12-23 14:24 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 41404: (QA follow-up) Add tests for empty string and zero edge cases (6.79 KB, patch)
2025-12-23 14:24 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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