If you allow non-numeric characters in patron barcodes they may be stored in either lower or uppercase in the borrowers table which is defined with a _ci (case insensitive) collation and therefore treats both forms as equivalent. However when the sipserver processes an issue transaction (used by most devices for both issue and renew) the test to see if the issue is in fact a renew uses a case sensitive comparison of the incoming cardnumber and that in the database causing the renewal to be rejected as 'Item checked out to another patron' e.g. this client sends cardnumber A17 but the database has stored this as 'a17' Request:11YN20160106 12164220160828 093005AO|AAA17|AB502326000767|AC|AD|BOY|AY1AZED86 Response:120NUN20160106 121642AOCPL|AAA17|AB502326000767|AJProzac nation [region 2] [import]|AH|AFItem checked out to another patron|BLY|AY1AZD57E[0d][0a] Request:11YN20160106 12164220160828 093005AO|AAA17|AB502326000755|AC|AD|AY2AZEEEE Response:120NUN20160106 121642AOCPL|AAA17|AB502326000755|AJAdvanced programming in the UNIX environment /|AH|AFItem checked out to another patron|BLY|AY2AZD100[0d][0a] Patron status reveals cardnumber as stored and that the two items are indeed on loan to this patron: Request:6302420160106 121642 Y AOCPL|AAA17|AC|AD|AY3AZF34D Response:64 02420160106 121642000000000002000000000000AOCPL|AAa17|AEFHT Trial|BLY|CQN|BV2.00|CC5|AU502326000767|AU502326000755|PCST|PIY|AFGreetings from Koha. -- Patron owes 2.00|AY3AZCE4B[0d][0a] Comparison in code should use same type of comparison as the database
Created attachment 46326 [details] [review] Proposed Patch Patch uses a case insensitive comparison to get correct result. To test issue a checkout for a borrower whith an item already on loan to them and where the request uses a different case to the db see examples in bug
With patch applied repeating the requests listed in the bug result in issues being correctly identified and processed as renewals: Request:11YN20160106 12224420160828 093005AO|AAA17|AB502326000767|AC|AD|BOY|AY1AZED87 Response:120NUN20160106 122244AOCPL|AAA17|AB502326000767|AJProzac nation [region 2] [import]|AH|AFNO_MORE_RENEWALS: 1|BLY|BV2.00|BHUSD|BT06|AY1AZD77D[0d][0a] Request:11YN20160106 12224420160828 093005AO|AAA17|AB502326000755|AC|AD|AY2AZEEEF Response:121YNY20160106 122245AOCPL|AAA17|AB502326000755|AJAdvanced programming in the UNIX environment /|AH20160108 235900|AFItem already checked out to you: renewing item.|AY2AZCA9E[0d][0a]
I get the reasoning, and agree with it. Do you think you could cook a unit test to check this case? A UT which fails without your patch, and succeed with it? And a minor point of detail: why do you use a function rather than a regex in your test. For example: $item->{patron} && $item->{patron} !~ /^$patron_id$/i
(In reply to Frédéric Demians from comment #3) > I get the reasoning, and agree with it. Do you think you could cook a unit > test to check this case? A UT which fails without your patch, and succeed > with it? > > And a minor point of detail: why do you use a function rather than a regex > in your test. For example: > > $item->{patron} && $item->{patron} !~ /^$patron_id$/i Readability it might be to easy for the eye to skip over the regex, and as the comparison is called twice ensure they both make the same test. Supplementary patch tests the sub's returns as well as adding basic tests for ILS.pm inteface
Created attachment 47105 [details] [review] Add test for routine Add a test for the routine added in bug 15479 Adds a script making basic tests to ILS.pm
Created attachment 47811 [details] [review] [SIGNED-OFF] Bug 15479 Make ILS cardnumber comparison case insensitive The borrowers table is defined COLLATE=utf8_unicode_ci which means that cardnumbers may be saved in either lowercase or uppercase and these are considered equivalent. The server was performing a case sensitive comparison between the incoming patron identifier and that retrieved from the db As a result some renewals were rejected as being on loan to another borrower if the stored cardnumber differed in case. Make code comparison comply with db. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 47812 [details] [review] [SIGNED-OFF] Bug 15479 Add tests for ILS.pm This patch specifically adds a test for the sub added to resolve Bug 15479. As no test script existed to add this two added one which also tests that ILS objects can be instantiated correctly and offer the required interface NOTE: This does not apply without the first patch. Comment #3 suggested a unit test that would fail before and pass after. So while this code is good, it missed the before case. -- mtompset Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 47969 [details] [review] Bug 15479 Make ILS cardnumber comparison case insensitive The borrowers table is defined COLLATE=utf8_unicode_ci which means that cardnumbers may be saved in either lowercase or uppercase and these are considered equivalent. The server was performing a case sensitive comparison between the incoming patron identifier and that retrieved from the db As a result some renewals were rejected as being on loan to another borrower if the stored cardnumber differed in case. Make code comparison comply with db. Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 47970 [details] [review] Bug 15479 Add tests for ILS.pm This patch specifically adds a test for the sub added to resolve Bug 15479. As no test script existed to add this two added one which also tests that ILS objects can be instantiated correctly and offer the required interface NOTE: This does not apply without the first patch. Comment #3 suggested a unit test that would fail before and pass after. So while this code is good, it missed the before case. -- mtompset Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 47972 [details] [review] Bug 15479 [QA Followup] - Tidy sub to remove tabs causing qa script to fail Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to Master - Should be in the May 2016 Release. Thanks
Patches pushed to 3.22.x, will be in 3.22.4
This patch has been pushed to 3.20.x, will be in 3.20.9.