Summary: | handle_patron_status dies if patron not found | ||
---|---|---|---|
Product: | Koha | Reporter: | Pedro Amorim <pedro.amorim> |
Component: | SIP2 | Assignee: | Pedro Amorim <pedro.amorim> |
Status: | Pushed to oldoldstable --- | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
Severity: | normal | ||
Priority: | P5 - low | CC: | aleisha, david, fridolin.somers, jonathan.field, kyle, lmstrand, lucas, martin.renvoize, nick, pedro.amorim |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
This fixes patron status SIP2 request responses if the request has an invalid or empty card number or PIN, and options are set for the TrackLastPatronActivityTriggers system preference. The SIP request was silently dying.
|
Version(s) released in: |
24.11.00,24.05.06,23.11.11
|
Circulation function: | |||
Bug Depends on: | 36605 | ||
Bug Blocks: | |||
Attachments: |
Bug 38284: Add tests
Bug 38284: Add patron check for TrackLastPatronActivityTriggers Bug 38284: Add tests Bug 38284: Add patron check for TrackLastPatronActivityTriggers Bug 38284: Add tests Bug 38284: Add patron check for TrackLastPatronActivityTriggers Bug 38284: Add tests Bug 38284: Add patron check for TrackLastPatronActivityTriggers |
Description
Pedro Amorim
2024-10-29 09:22:13 UTC
Created attachment 173603 [details] [review] Bug 38284: Add tests Created attachment 173604 [details] [review] Bug 38284: Add patron check for TrackLastPatronActivityTriggers If an invalid or empty cardnumber is supplied to patron status request SIP message, SIP dies silently and no 'READ:' exists on the response. Test plan: 1) Apply tests plan, run tests: $ prove t/db_dependent/SIP/Message.t 2) Verify tests fail. Apply 2nd patch. Run tests again. Verify they pass. With just the first patch, the tests still pass for me. Is anything else required to trigger the failure? (In reply to David Nind from comment #3) > With just the first patch, the tests still pass for me. > > Is anything else required to trigger the failure? Hi David, Yes, select some values from the TrackLastPatronActivityTriggers system preference. If none are selected it should work, if you select some, it should fail! Created attachment 173735 [details] [review] Bug 38284: Add tests Signed-off-by: David Nind <david@davidnind.com> Created attachment 173736 [details] [review] Bug 38284: Add patron check for TrackLastPatronActivityTriggers If an invalid or empty cardnumber is supplied to patron status request SIP message, SIP dies silently and no 'READ:' exists on the response. Test plan: 1) Apply tests plan, run tests: $ prove t/db_dependent/SIP/Message.t 2) Verify tests fail. Apply 2nd patch. Run tests again. Verify they pass. Signed-off-by: David Nind <david@davidnind.com> (In reply to Jonathan Field from comment #4) ... > Yes, select some values from the TrackLastPatronActivityTriggers system > preference. If none are selected it should work, if you select some, it > should fail! Thanks Jonathan! Slight QA test fail: OK C4/SIP/Sip/MsgType.pm WARN t/db_dependent/SIP/Message.t WARN tidiness The file is less tidy than before (bad/messy lines before: 531, now: 539) Processing additional checks OK! * Was this bug sponsored by anyone? Running tests (1) * Proving /kohadevbox/koha/t/db_dependent/SIP/Message.t OK! (In reply to Aleisha Amohia from comment #8) > > > * Was this bug sponsored by anyone? > No, just came up as a bug reported by a library. (In reply to David Nind from comment #3) > With just the first patch, the tests still pass for me. > > Is anything else required to trigger the failure? Hi David, thanks! This means I must revisit the test patch. I must've written and tested it with the syspref enabled on my local environment but this needs to be mocked on test instead. Thanks Jonathan for chipping in. Created attachment 173922 [details] [review] Bug 38284: Add tests Signed-off-by: David Nind <david@davidnind.com> Created attachment 173923 [details] [review] Bug 38284: Add patron check for TrackLastPatronActivityTriggers If an invalid or empty cardnumber is supplied to patron status request SIP message, SIP dies silently and no 'READ:' exists on the response. Test plan: 1) Apply tests plan, run tests: $ prove t/db_dependent/SIP/Message.t 2) Verify tests fail. Apply 2nd patch. Run tests again. Verify they pass. Signed-off-by: David Nind <david@davidnind.com> Hi all, thanks for looking here. I've squashed tidy fixes to the patches and revisited/rewritten the tests. They now should consistently fail when applied tests patch only, and consistently pass when fix patch is applied. Created attachment 174284 [details] [review] Bug 38284: Add tests Sponsored-by: PTFS Europe <https://ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 174285 [details] [review] Bug 38284: Add patron check for TrackLastPatronActivityTriggers If an invalid or empty cardnumber is supplied to patron status request SIP message, SIP dies silently and no 'READ:' exists on the response. Test plan: 1) Apply tests plan, run tests: $ prove t/db_dependent/SIP/Message.t 2) Verify tests fail. Apply 2nd patch. Run tests again. Verify they pass. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Pushed for 24.11! Well done everyone, thank you! I noticed this issue too and actually without this patch the SIP server replies with a 96 message Request SC Resend with a checksum field (96AZFEF6 for example) when either cardnumber or pin is wrong. This patch not only fixes the problem with invalid cardnumbers but also when the pin is invalid and I no longer receive 96 messages as replies but 24 messages with this patch. 24 00020241115 140328AEtest test|AA66666666|BLY|CQN|BV0|AFGreetings from Koha. -- Invalid password|AOJOE_JOE| and 24YYYY 00020241115 140438AE|AA66666665|BLN|AFInvalid cardnumber|AOJOE_JOE| ps. Without a checksum in the 23 request nothing was returned without the patch, it seems. Backported to 24.05.x for 24.05.06 I've added a release note, but I'm not sure that I correctly understand what the issue is and what was fixed. Please update the release note if I have not described this correctly! (In reply to David Nind from comment #20) > I've added a release note, but I'm not sure that I correctly understand what > the issue is and what was fixed. > > Please update the release note if I have not described this correctly! I think you have described it perfectly, thanks! (In reply to Jonathan Field from comment #21) > (In reply to David Nind from comment #20) > > I've added a release note, but I'm not sure that I correctly understand what > > the issue is and what was fixed. > > > > Please update the release note if I have not described this correctly! > > I think you have described it perfectly, thanks! Thanks Jonathan! Pushed to 23.11.x for 23.11.11 |