Description
Katrin Fischer
2020-03-04 14:31:20 UTC
Created attachment 100549 [details] [review] Bug 24800: Add tests Created attachment 100550 [details] [review] Bug 24800: Handle inexistent return date on self checking See bug 24800 comment 0 for a description of the problem. We do not want the SIP server to crash if it receives a checkin request with a return date that is not given. The option this patch chose is to parse it only if provided. Katrin, do you think this patch fix correctly the problem you faced? (In reply to Jonathan Druart from comment #3) > Katrin, do you think this patch fix correctly the problem you faced? We tested the patch and we think it's a clear improvement. It's not a complete fix for us missing an "undo" feature that would allow us to highlight the problematic returns, but it prevents SIP from dying, which I think is the most important for now. I suggest to file a separate enhancement request after this fix is pushed. Created attachment 102452 [details] [review] Bug 24800: Handle inexistent return date on self checking See bug 24800 comment 0 for a description of the problem. We do not want the SIP server to crash if it receives a checkin request with a return date that is not given. The option this patch chose is to parse it only if provided. Signed-off-by: Clemens Elmlinger <clemens.elmlinger@bsz-bw.de> Created attachment 102455 [details] [review] Bug 24800: Add tests Signed-off-by: Clemens Elmlinger <clemens.elmlinger@bsz-bw.de> Created attachment 102456 [details] [review] Bug 24800: Handle inexistent return date on self checking See bug 24800 comment 0 for a description of the problem. We do not want the SIP server to crash if it receives a checkin request with a return date that is not given. The option this patch chose is to parse it only if provided. Signed-off-by: Clemens Elmlinger <clemens.elmlinger@bsz-bw.de> As written before, this worked in our testing, but sadly the tests fail for me: t/db_dependent/SIP/Transaction.t .. 8/8 # Failed test 'Checkin should have been done successfully' # at t/db_dependent/SIP/Transaction.t line 323. # got: '1' # expected: '0' # Looks like you failed 2 tests of 8. t/db_dependent/SIP/Transaction.t .. 8/8 # Failed test 'do_checkin' # at t/db_dependent/SIP/Transaction.t line 324. # Looks like you failed 1 test of 8. t/db_dependent/SIP/Transaction.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/8 subtests Keeping the sign off, but could you take a look please? It passes for me, sample data using koha-testing-docker. Maybe you have a circ rule or a pref that is blocking the checkin? Good question - not sure. Are there candidates? You should check the return of AddReturn in do_checkin C4/SIP/ILS/Transaction/Checkin.pm 75 my ($return, $messages, $issue, $borrower) = AddReturn($barcode, $branch, undef, $return_date); I bet $messages contains the useful info. Hi Joubu, the tests started to pass when I tried to add the debug... I believe it was just Plack/Cache or something? (In reply to Katrin Fischer from comment #12) > Hi Joubu, the tests started to pass when I tried to add the debug... I > believe it was just Plack/Cache or something? I cannot think of something that would cause such behaviour in these tests. (In reply to Jonathan Druart from comment #13) > (In reply to Katrin Fischer from comment #12) > > Hi Joubu, the tests started to pass when I tried to add the debug... I > > believe it was just Plack/Cache or something? > > I cannot think of something that would cause such behaviour in these tests. Don't know, but I think if we have a QA person verify that the tests pass on another system it shoudl be ok. Created attachment 102613 [details] [review] Unit tests pass for me! Created attachment 102614 [details] [review] Bug 24800: Handle inexistent return date on self checking See bug 24800 comment 0 for a description of the problem. We do not want the SIP server to crash if it receives a checkin request with a return date that is not given. The option this patch chose is to parse it only if provided. Signed-off-by: Clemens Elmlinger <clemens.elmlinger@bsz-bw.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 102615 [details] [review] Bug 24800: Add tests Signed-off-by: Clemens Elmlinger <clemens.elmlinger@bsz-bw.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 102616 [details] [review] Bug 24800: Handle inexistent return date on self checking See bug 24800 comment 0 for a description of the problem. We do not want the SIP server to crash if it receives a checkin request with a return date that is not given. The option this patch chose is to parse it only if provided. Signed-off-by: Clemens Elmlinger <clemens.elmlinger@bsz-bw.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Thx, Kyle! Created attachment 102690 [details] [review] Bug 24800: Add tests Signed-off-by: Clemens Elmlinger <clemens.elmlinger@bsz-bw.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 102691 [details] [review] Bug 24800: Handle inexistent return date on self checking See bug 24800 comment 0 for a description of the problem. We do not want the SIP server to crash if it receives a checkin request with a return date that is not given. The option this patch chose is to parse it only if provided. Signed-off-by: Clemens Elmlinger <clemens.elmlinger@bsz-bw.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Nice work everyone! Pushed to master for 20.05 unit test patch does not apply to 19.11.x Please rebase if needed. Created attachment 104348 [details] [review] [19.11.x version] Bug 24800: Add tests Signed-off-by: Clemens Elmlinger <clemens.elmlinger@bsz-bw.de> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Thanks for the rebase. Backported to 19.11.x for 19.11.06 unit tests wont apply to 19.05.x. I tried both the original unit test patch and the 19.11 rebase. No backport to 19.05.x |