Bug 33216 - SIP fee paid messages explode if payment registers are enabled and the SIP account has no register
Summary: SIP fee paid messages explode if payment registers are enabled and the SIP ac...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-03-13 18:54 UTC by Kyle M Hall
Modified: 2023-12-28 20:44 UTC (History)
3 users (show)

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


Attachments
Bug 33216: Catch and handle RegisterRequired exceptions for SIP fee paid messages (2.63 KB, patch)
2023-03-14 11:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33216: Catch and handle all Koha exceptions (1016 bytes, patch)
2023-03-14 12:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33216: Catch and handle RegisterRequired exceptions for SIP fee paid messages (2.96 KB, patch)
2023-04-17 13:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33216: Catch and handle RegisterRequired exceptions for SIP fee paid messages (3.02 KB, patch)
2023-04-17 13:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33216: Catch and handle RegisterRequired exceptions for SIP fee paid messages (3.25 KB, patch)
2023-04-18 12:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33216: Catch and handle RegisterRequired exceptions for SIP fee paid messages (3.35 KB, patch)
2023-04-18 12:20 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 Kyle M Hall 2023-03-13 18:54:07 UTC
If registers are being used in Koha, they are required. If a SIP account has no register and a fee paid message is sent, the SIP server crashes and the client never gets a response. It would be much better if Koha would response with 38 response where "payment accepted" is N, and an AF field stating that the SIP account needs to be associated with a register.
Comment 1 Martin Renvoize 2023-03-14 07:45:02 UTC
I would say this is somewhat deliberate. If money is changing hands, as it would be in a SIP transaction, we want to record it against a register.
Comment 2 Martin Renvoize 2023-03-14 07:47:32 UTC
The line in question is here: https://git.koha-community.org/Koha-community/Koha/src/branch/master/Koha/Account.pm#L354

We could perhaps drive it with yasp (yet another system preference)
Comment 3 Kyle M Hall 2023-03-14 10:46:08 UTC
(In reply to Martin Renvoize from comment #1)
> I would say this is somewhat deliberate. If money is changing hands, as it
> would be in a SIP transaction, we want to record it against a register.

Yeah, I'm in agreement with that. It's the part where they SIP server crashes instead of sending a "fee payment failed" response that I think is wrong ;)

We should be catching the exception in the SIP code and sending a response back.
Comment 4 Martin Renvoize 2023-03-14 11:00:05 UTC
OH.. good call.. why did my brain not go there...
Comment 5 Kyle M Hall 2023-03-14 11:08:42 UTC
Created attachment 148157 [details] [review]
Bug 33216: Catch and handle RegisterRequired exceptions for SIP fee paid messages

If registers are being used in Koha, they are required. If a SIP account has no register and a fee paid message is sent, the SIP server crashes and the client never gets a response. It would be much better if Koha would response with 38 response where "payment accepted" is N, and an AF field stating that the SIP account needs to be associated with a register.
Comment 6 Kyle M Hall 2023-03-14 12:28:59 UTC
Created attachment 148161 [details] [review]
Bug 33216: Catch and handle all Koha exceptions
Comment 7 Katrin Fischer 2023-04-16 11:03:14 UTC
Hi Kyle, should this be Needs Signoff?
Comment 8 Kyle M Hall 2023-04-17 13:34:10 UTC
Created attachment 149755 [details] [review]
Bug 33216: Catch and handle RegisterRequired exceptions for SIP fee paid messages

If registers are being used in Koha, they are required. If a SIP account has no register and a fee paid message is sent, the SIP server crashes and the client never gets a response. It would be much better if Koha would response with 38 response where "payment accepted" is N, and an AF field stating that the SIP account needs to be associated with a register.

Test Plan:
1) Enable UseCashRegisters, set RequireCashRegister to "always require a
   cash register". Do *not* set a cash register for the SIP account you
   will be testing with.
2) Using the SIP cli tester, send a fee paid message for a patron owing
   fees.
3) Note the lack of a SIP response
4) Apply this patch
5) Restart the SIP server
6) Repeat step 2, you should now get a SIP response with the error
   message in it!
Comment 9 Kyle M Hall 2023-04-17 13:34:39 UTC
(In reply to Katrin Fischer from comment #7)
> Hi Kyle, should this be Needs Signoff?

Was missing a test plan. Fixed!
Comment 10 Martin Renvoize 2023-04-17 13:58:39 UTC
Created attachment 149757 [details] [review]
Bug 33216: Catch and handle RegisterRequired exceptions for SIP fee paid messages

If registers are being used in Koha, they are required. If a SIP account has no register and a fee paid message is sent, the SIP server crashes and the client never gets a response. It would be much better if Koha would response with 38 response where "payment accepted" is N, and an AF field stating that the SIP account needs to be associated with a register.

Test Plan:
1) Enable UseCashRegisters, set RequireCashRegister to "always require a
   cash register". Do *not* set a cash register for the SIP account you
   will be testing with.
2) Using the SIP cli tester, send a fee paid message for a patron owing
   fees.
3) Note the lack of a SIP response
4) Apply this patch
5) Restart the SIP server
6) Repeat step 2, you should now get a SIP response with the error
   message in it!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2023-04-17 13:58:52 UTC
Working well, thanks Kyle.
Comment 12 Martin Renvoize 2023-04-18 08:20:20 UTC
I've decided to just go straight for QA here.. lets pass it.
Comment 13 Kyle M Hall 2023-04-18 12:04:00 UTC
Created attachment 149795 [details] [review]
Bug 33216: Catch and handle RegisterRequired exceptions for SIP fee paid messages

If registers are being used in Koha, they are required. If a SIP account has no register and a fee paid message is sent, the SIP server crashes and the client never gets a response. It would be much better if Koha would response with 38 response where "payment accepted" is N, and an AF field stating that the SIP account needs to be associated with a register.

Test Plan:
1) Enable UseCashRegisters, set RequireCashRegister to "always require a
   cash register". Do *not* set a cash register for the SIP account you
   will be testing with.
2) Using the SIP cli tester, send a fee paid message for a patron owing
   fees.
3) Note the lack of a SIP response
4) Apply this patch
5) Restart the SIP server
6) Repeat step 2, you should now get a SIP response with the error
   message in it!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Kyle M Hall 2023-04-18 12:20:04 UTC
Created attachment 149796 [details] [review]
Bug 33216: Catch and handle RegisterRequired exceptions for SIP fee paid messages

If registers are being used in Koha, they are required. If a SIP account has no register and a fee paid message is sent, the SIP server crashes and the client never gets a response. It would be much better if Koha would response with 38 response where "payment accepted" is N, and an AF field stating that the SIP account needs to be associated with a register.

Test Plan:
1) Enable UseCashRegisters, set RequireCashRegister to "always require a
   cash register". Do *not* set a cash register for the SIP account you
   will be testing with.
2) Using the SIP cli tester, send a fee paid message for a patron owing
   fees.
3) Note the lack of a SIP response
4) Apply this patch
5) Restart the SIP server
6) Repeat step 2, you should now get a SIP response with the error
   message in it!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Tomás Cohen Arazi 2023-04-19 12:22:02 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 16 Jacob O'Mara 2023-05-09 20:52:30 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 17 Lucas Gass 2023-05-15 14:40:07 UTC
Backported to 22.05.x for upcoming 22.05.13
Comment 18 Arthur Suzuki 2023-05-23 14:24:59 UTC
applied to 21.11 for 21.11.21