Summary: | SIPserver rejects renewals for patrons with alphanumeric cardnumbers | ||
---|---|---|---|
Product: | Koha | Reporter: | Colin Campbell <colin.campbell> |
Component: | SIP2 | Assignee: | Colin Campbell <colin.campbell> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | normal | ||
Priority: | P5 - low | CC: | brendan, colin.campbell, f.demians, julian.maurice, mtompset |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Attachments: |
Proposed Patch
Add test for routine [SIGNED-OFF] Bug 15479 Make ILS cardnumber comparison case insensitive [SIGNED-OFF] Bug 15479 Add tests for ILS.pm Bug 15479 Make ILS cardnumber comparison case insensitive Bug 15479 Add tests for ILS.pm Bug 15479 [QA Followup] - Tidy sub to remove tabs causing qa script to fail |
Description
Colin Campbell
2016-01-06 12:54:52 UTC
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. |