Bug 21658 - Renew requests fail if a fee attached
Summary: Renew requests fail if a fee attached
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P3 normal (vote)
Assignee: Colin Campbell
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-25 09:39 UTC by Colin Campbell
Modified: 2021-12-13 21:08 UTC (History)
7 users (show)

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


Attachments
patch to stop sipserver crash handling fees (1.50 KB, patch)
2018-10-25 10:18 UTC, Colin Campbell
Details | Diff | Splinter Review
patch to suppress runtime warning (1.08 KB, patch)
2018-10-25 10:20 UTC, Colin Campbell
Details | Diff | Splinter Review
Fix error in value not passed to transaction (1.10 KB, patch)
2018-10-25 10:24 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 21658 Define string to avoid run time warnings (1.16 KB, patch)
2019-05-08 23:13 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 21658 Pass fee_ack value to Renew transaction (1.19 KB, patch)
2019-05-08 23:13 UTC, Liz Rea
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2018-10-25 09:39:47 UTC
A Sip Renew request (29) for an item with an associated fee should behave similarly to a checkout request i.e. the initial request has fee acknowledged set to N - server rejects the transaction an responds indicating  the fee amount. Self check unit has the responsibility of collecting that fee and can then proceed repeating the request but with fee acknowledged set to Y, this request should then succeed.

Currently this is not happening on older versions renew requests are always rejected if there is a fee irrespective of the value of the fee acknowledged field. On master there is an additional problem that the server is crashing due to a runtime error when processing the fee.

This bug has not manifested for many users as 3m and most other self-check units actually send a checkout request not a renewal request when renewing items. Newer biblioteca units do send a renew request and expect this functionality
Comment 1 Colin Campbell 2018-10-25 10:11:12 UTC
Example of correct responses:
Item has a rental fee of 2.00
renew request sent with field BO set to N
300NUN20181025    110552AA23529001223675|AB502326000833|AJAlien nation|AH|BV2.00|BHUSD|BT06|AOCPL|
same request sent with field BO now set to Y
301YNN20181025    110552AA23529001223675|AB502326000833|AJAlien nation|AH20181030    235900|CK|BV2.00|BHUSD|BT06|AOCPL|

Key difference is first field after the response id '30' 0 == renew rejected, 1 == renew succeeded
Comment 2 Colin Campbell 2018-10-25 10:18:24 UTC
Created attachment 81126 [details] [review]
patch to stop sipserver crash handling fees

This patch is required on master otherwise runtime error occurs in handling fees as C4::Circulation::getnextacctno is not found (i.e. import of routine in package namespace has been overwritten)
(bug 21486 was also causing problems but is now in master)
Comment 3 Colin Campbell 2018-10-25 10:20:11 UTC
Created attachment 81127 [details] [review]
patch to suppress runtime warning

Fix runtime error caused by assumption that only errors (with accompanying message) block renewal
Comment 4 Colin Campbell 2018-10-25 10:24:18 UTC
Created attachment 81128 [details] [review]
Fix error in value not passed to transaction

Renewal transaction blocks/allows based on fee_acknowledge field as value was not passed to the renewal transaction code it was always N blocking the transaction. Pass the value similarly to ILS.pm's checkout code for the same purpose
Comment 5 Josef Moravec 2018-10-26 11:28:40 UTC
see also bug 21683
Comment 6 Colin Campbell 2018-10-26 15:30:17 UTC
(In reply to Josef Moravec from comment #5)
> see also bug 21683

Yes if bug 21683 is applied the first if these 3 patches is no longer required
Comment 7 Hayley Pelham 2019-03-10 21:50:00 UTC
Hi, is there a test plan for this?
Comment 8 Liz Rea 2019-05-08 23:11:23 UTC
I tested as follows, with a set up running SIP in my testing setup (docker, this time):

sent this:
kohadev-koha@b46acddf5bb7:/kohadevbox/koha$ perl misc/sip_cli_emulator.pl --sip_user=term1 --sip_pass=term1 --patron=2 --item='502326000048' -m renew -a 127.0.0.1 -p 6001 -l MAIN

Trying 'renew'
SEND: 29NN20190508    23053520190508    230535AOMAIN|AA2|AB502326000048|AJ|ACterm1|BON|
READ: 300NUN20190508    230535AA2|AB502326000048|AJWorking in health care :|AH|BV3.00|BHUSD|BT06|AOCPL|

Item was not renewed.

I couldn't get the emulator to do something sensible, so I used telnet for the next part: 

root@b46acddf5bb7:koha(bug21658-qa)$ telnet localhost 6001
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
9300CNterm1|COterm1|CPMAIN|
941
29NN20190508    23055020190508    230550AOMAIN|AA2|AB502326000048|AJ|ACterm1|BOY|    
301YNN20190508    230653AA2|AB502326000048|AJWorking in health care :|AH20190523    235900|CK|BV3.00|BHUSD|BT06|AOCPL|

The item was renewed, and the fee was accepted (evidence in the interface and in the string), like I think I was expecting. The SIP server also didn't crash at any point here, so I think that's winning.

Liz
Comment 9 Liz Rea 2019-05-08 23:13:20 UTC
Created attachment 89487 [details] [review]
Bug 21658 Define string to avoid run time warnings

In addition to an explicit error a renewal may be
rejected because a fee is due and has not yet been flagged
as paid. In this case define the error message as empty string
to avoid runtime errors in the regexes performed
on the standard error messages

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 10 Liz Rea 2019-05-08 23:13:29 UTC
Created attachment 89488 [details] [review]
Bug 21658 Pass fee_ack value to Renew transaction

Response to a renew request should mirror that of checkout
However fee_ack was not being passed to the renewal
transaction so that it was always 'N' not allowing
the renewal to proceed even although the fee was paid

This bug was not seen with many units as they use
checkout requests both for an initial issue and
renewal. Code now passes fee_ack as is done for checkout

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 11 Martin Renvoize 2021-03-09 16:14:54 UTC
Fixed by bug 21683