Bug 15479 - SIPserver rejects renewals for patrons with alphanumeric cardnumbers
Summary: SIPserver rejects renewals for patrons with alphanumeric cardnumbers
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Colin Campbell
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-06 12:54 UTC by Colin Campbell
Modified: 2017-06-14 22:10 UTC (History)
5 users (show)

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


Attachments
Proposed Patch (2.23 KB, patch)
2016-01-06 13:22 UTC, Colin Campbell
Details | Diff | Splinter Review
Add test for routine (3.02 KB, patch)
2016-01-21 11:45 UTC, Colin Campbell
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15479 Make ILS cardnumber comparison case insensitive (2.30 KB, patch)
2016-02-09 14:42 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15479 Add tests for ILS.pm (3.29 KB, patch)
2016-02-09 14:42 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 15479 Make ILS cardnumber comparison case insensitive (2.34 KB, patch)
2016-02-12 13:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15479 Add tests for ILS.pm (3.33 KB, patch)
2016-02-12 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15479 [QA Followup] - Tidy sub to remove tabs causing qa script to fail (3.70 KB, patch)
2016-02-12 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2016-01-06 12:54:52 UTC
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
Comment 1 Colin Campbell 2016-01-06 13:22:00 UTC Comment hidden (obsolete)
Comment 2 Colin Campbell 2016-01-06 13:24:31 UTC
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]
Comment 3 Frédéric Demians 2016-01-15 18:30:34 UTC
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
Comment 4 Colin Campbell 2016-01-21 11:41:11 UTC
(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
Comment 5 Colin Campbell 2016-01-21 11:45:13 UTC Comment hidden (obsolete)
Comment 6 Mark Tompsett 2016-02-09 14:42:23 UTC Comment hidden (obsolete)
Comment 7 Mark Tompsett 2016-02-09 14:42:32 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2016-02-12 13:08:53 UTC
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>
Comment 9 Kyle M Hall 2016-02-12 13:09:03 UTC
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>
Comment 10 Kyle M Hall 2016-02-12 13:09:06 UTC
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>
Comment 11 Brendan Gallagher 2016-02-23 23:27:58 UTC
Pushed to Master - Should be in the May 2016 Release.  Thanks
Comment 12 Julian Maurice 2016-02-26 13:21:38 UTC
Patches pushed to 3.22.x, will be in 3.22.4
Comment 13 Frédéric Demians 2016-03-01 16:53:09 UTC
This patch has been pushed to 3.20.x, will be in 3.20.9.