Bug 26035 - AutoMemberNum fails on very large cardnumbers
Summary: AutoMemberNum fails on very large cardnumbers
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-07-21 21:36 UTC by Andrew Fuerste-Henry
Modified: 2023-09-06 18:55 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 26035: Add tests to highlight the problem (2.08 KB, patch)
2020-07-29 12:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26035: AutoMemberNum should support cardnumbers up to 32 characters (1.36 KB, patch)
2020-07-29 19:16 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26035: AutoMemberNum should support cardnumbers up to 32 characters (1.42 KB, patch)
2020-07-31 17:45 UTC, ByWater Sandboxes
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2020-07-21 21:36:29 UTC
To recreate:
- create a borrower with cardnumber 603628999632182575912
- turn on automembernum
- create a new patron, save without a cardnumber
- see that your new patron has cardnumber 0

As part of the fixup_cardnumber sub in Patron.pm, we CAST(cardnumber AS SIGNED), which tries to make the cardnumber into a 64 bit integer. Way back in bug 13178, we increased the max length of a cardnumber to 32 characters, which is longer than a 64 bit integer can handle. So the CAST spits out a -1, to which Koha adds 1, arriving at cardnumber 0.

Why do we cast it as signed in the first place? I don't understand enough about integers to see what purpose this is serving.
Comment 1 Andrew Fuerste-Henry 2020-07-21 21:38:38 UTC
Previous comment describes the behavior I'm seeing on a partner site on 19.11.
On my test site, on both master and 19.11.x, Koha simply ignores the very large cardnumber, and falls back to the next longest card. I'm not sure what makes the difference there. Either way, it's not working as desired.
Comment 2 Jonathan Druart 2020-07-29 12:33:18 UTC
Created attachment 107492 [details] [review]
Bug 26035: Add tests to highlight the problem
Comment 3 Jonathan Druart 2020-07-29 12:33:53 UTC
I've failed to provide a solution. But I am attaching the tests I wrote in case somebody else wants to give it a try.
Comment 4 Andrew Fuerste-Henry 2020-07-29 15:22:48 UTC
So if we don't cast the cardnumber as an integer, mysql doesn't know how to properly determine the highest cardnumber. It treats them as as strings and oputs 42 higher than 218250005. If we replace "CAST(cardnumber AS SIGNED" with "(cardnumber + 0)," the math forces mysql to treat it as a number, but we end up with a cardnumber in scientific notation if our existing biggest cardnumber is as big as the initial example (603628999632182575912). 
I played around with "use bigint" and "use Math::BigInt" but didn't get quite work. They both went from 603628999632182575912 to 603628999630000000001, suggesting that it's still going to scientific notation and then back again poorly.
Comment 5 Andrew Fuerste-Henry 2020-07-29 19:16:23 UTC
Created attachment 107522 [details] [review]
Bug 26035: AutoMemberNum should support cardnumbers up to 32 characters

To test:
1- create a patron with cardnumber 12345678901234567890123456789012 (or any 32 character number)
2- turn on AutoMemberNum
3- create a new patron, save without a cardnumber
4- confirm that new patron has a cardnumber one more than the number from step 1
Comment 6 ByWater Sandboxes 2020-07-31 17:45:07 UTC
Created attachment 107655 [details] [review]
Bug 26035: AutoMemberNum should support cardnumbers up to 32 characters

To test:
1- create a patron with cardnumber 12345678901234567890123456789012 (or any 32 character number)
2- turn on AutoMemberNum
3- create a new patron, save without a cardnumber
4- confirm that new patron has a cardnumber one more than the number from step 1

Signed-off-by: Donna Bachowski <donna@bywatersolutions.com>
Comment 7 Katrin Fischer 2020-08-15 11:07:53 UTC
There is a problem noted by the QA script - can you please check?

 FAIL	Koha/Patron.pm
   FAIL	  valid
		Useless use of a constant (1) in void context 

I also have some issues with the unit tests failing:

#   Failed test 'No tests run for subtest "fixup_cardnumber"'
#   at t/db_dependent/Koha/Patrons.t line 2237.
Can't call method "cardnumber" on an undefined value at t/db_dependent/Koha/Patrons.t line 2219.
# Looks like your test exited with 2 just after 42.
t/db_dependent/Koha/Patrons.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 1/42 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Patrons.t (Wstat: 512 Tests: 42 Failed: 1)
  Failed test:  42
  Non-zero exit status: 2
Files=1, Tests=42, 29 wallclock secs ( 0.08 usr  0.01 sys + 22.72 cusr  4.31 csys = 27.12 CPU)
Result: FAIL
Comment 8 Fernando 2023-09-06 18:55:39 UTC
I was just checking on the status of this bug. Has anything change on this?