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
Not pushed to LTS. Marked Resolved. If you feel this should be in LTS please reply with your reason.
(In reply to Jesse Maseto from comment #24) > Not pushed to LTS. Marked Resolved. > > If you feel this should be in LTS please reply with your reason. I believe this is a fixing an issue introduced by bug 36605. I've not tested and may be wrong but as bug 36605 has been pushed to 23.05.x, this should likely also be.