https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27916 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 118024 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118024&action=edit Bug 27916: Fee paid message can crash SIP server if paying fee that is not "renewable" I'm not entirely sure how this is possible, but I've confirmed my fix works on two separate occasions. In the Koha::Account::pay, if a fine is "renewable" ( as determined by the method "renewable" the response to from the method "renew_item" is stored and returned by by pay(). Later in C4::SIP::Sip::MsgType::handle_fee_paid we assume that each element of the list in $pay_response->{renew_result} is a hashref that had an itemnumber. This is not true. We need to check for the itemnumber and then the item. If either do not exist, the SIP server will crash without responding. My suspicion is that this is caused by Koha::Account::Line::renewable returning true while the corrosponding call to renew_item contains a simple "return;" statement if RenewAccruingItemWhenPaid is not enabled ( or there is not related item or patron ). What happens here, at least for Bibliotheca self checks is that the machine will take payment, and send the fee paid to Koha via SIP. When Koha's SIP crashes and doesn't return a response, the self check machine will refund the money to the patron, *but* the payment still exists in Koha! Considering payments may be in the tens or even hundreds of dollars, this makes for a critical SIP bug. I have been unable to recreate this bug on demand. So the test plan is: 1) Inspect the changes in the patch 2) Note that they make sense and prevent the SIP server from crashing if the itemnumber key has no data or the item is not found -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27916 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=25508 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27916 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118024|0 |1 is obsolete| | --- Comment #3 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 118779 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118779&action=edit Bug 27916: Fee paid message can crash SIP server if paying fee that is not "renewable" I'm not entirely sure how this is possible, but I've confirmed my fix works on two separate occasions. In the Koha::Account::pay, if a fine is "renewable" ( as determined by the method "renewable" the response to from the method "renew_item" is stored and returned by by pay(). Later in C4::SIP::Sip::MsgType::handle_fee_paid we assume that each element of the list in $pay_response->{renew_result} is a hashref that had an itemnumber. This is not true. We need to check for the itemnumber and then the item. If either do not exist, the SIP server will crash without responding. My suspicion is that this is caused by Koha::Account::Line::renewable returning true while the corrosponding call to renew_item contains a simple "return;" statement if RenewAccruingItemWhenPaid is not enabled ( or there is not related item or patron ). What happens here, at least for Bibliotheca self checks is that the machine will take payment, and send the fee paid to Koha via SIP. When Koha's SIP crashes and doesn't return a response, the self check machine will refund the money to the patron, *but* the payment still exists in Koha! Considering payments may be in the tens or even hundreds of dollars, this makes for a critical SIP bug. I have been unable to recreate this bug on demand. So the test plan is: 1) Inspect the changes in the patch 2) Note that they make sense and prevent the SIP server from crashing if the itemnumber key has no data or the item is not found Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27916 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27916 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |14825 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14825 [Bug 14825] Accounts Rewrite Omnibus -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27916 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@helsinki.fi --- Comment #7 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- I hope also that we could get unit test for this, i wasn't able to trigger the error manually. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27916 --- Comment #9 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 119718 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119718&action=edit Bug 27916: Add unit tests -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27916 --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Kyle M Hall from comment #8)

(In reply to Martin Renvoize from comment #6)

...

Any chance of a unit test here Kyle?

I'll see what I can do!

I've spent a good amount of time trying to create a unit test for handle_fee_payment but something is going sideways. I've attached my non-functional unit tests if anyone wants to have a try at fixing them! https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=119718&action=diff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27916 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|critical |blocker --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- I'm upping the severity on this. Just had another partner lose $119 do to this bug. This is real money we are talking about. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27916 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- foreach my $result( @{$pay_response->{renew_result}} ) { next unless $result->{itemnumber}; my $item = Koha::Items->find({ itemnumber => $result->{itemnumber} }); next unless $item; if ($result->{success}) { push @success, '"' . $item->biblio->title . '"'; } else { push @fail, '"' . $item->biblio->title . '" : ' . $failmap->{$result->{error}}; } } my $msg = ""; if (scalar @success > 0) { $msg.="The following items were renewed: " . join(", ", @success) . ". "; } if (scalar @fail > 0) { $msg.="The following items were not renewed: " . join(", ", @fail) . "."; } if (length $msg > 0) { $status->screen_msg($status->screen_msg . " $msg"); } It is clear that SIP should not crash on the lack of an itemnumber or an invalid one. But the current adjustments are just silent ignores. I would at least recommend to not just jump to the next renew outcome but include these cases in the msg. When looking at SIP and Accounts, I noticed that renew_item should already crash on an invalid itemnumber: my $itemnumber = $self->item->itemnumber; So I am wondering if your adjustments are enough? Or should we already do something here too? Is there any more information from syslog or so that you could share here about the actual bug? If the problem is caused by an invalid itemnumber, do we miss some FKs somewhere in Accounts? If the problem is related to the RenewAccruingItem preferences and the resulting empty return, should we touch renew_fine too? Needs some feedback/adjustments -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27916 --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Marcel de Rooy from comment #14)

When looking at SIP and Accounts, I noticed that renew_item should already crash on an invalid itemnumber: my $itemnumber = $self->item->itemnumber; So I am wondering if your adjustments are enough? Or should we already do something here too?

In theory, one should always have called 'is_renewable' prior to calling 'renew_item'.. and 'is_renewable' performs the check for existing item.. so we shouldn't hit the problem you're foreseeing.. in theory. I'm going to investigate the unit tests side again and hopefully at the same time I will work out where things are going. Thanks for reviewing Marcel, this code is all rather complex and all the eyes and perspective we get are useful. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27916 --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Martin Renvoize from comment #16)

Nope I'm stuck again.. with bug 25508 applied, I can't see how this patch has an effect any more at all...

Assuming the constraints are set as Koha expects (Which I've now verified with Kyle) we shouldn't be able to get into this state any more.

I think you are correct, the affected partner will be upgraded to 20.05.11 asap. I'll let you know how it goes! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27916 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |WONTFIX Status|Failed QA |RESOLVED -- You are receiving this mail because: You are watching all bug changes.