Bug 34767 - SIP2 fee acknowledgement flag on renewals is passed, but not used
Summary: SIP2 fee acknowledgement flag on renewals is passed, but not used
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-09-12 15:17 UTC by Martin Renvoize
Modified: 2023-10-12 15:32 UTC (History)
7 users (show)

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


Attachments
Bug 34767: Pass fee_ack into ::Transaction::Renew(All) (3.35 KB, patch)
2023-09-12 15:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34767: Pass fee_ack into ::Transaction::Renew(All) (3.40 KB, patch)
2023-09-12 17:56 UTC, David Nind
Details | Diff | Splinter Review
Bug 34767: Pass fee_ack into ::Transaction::Renew(All) (3.50 KB, patch)
2023-09-15 06:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2023-09-12 15:17:03 UTC
C4/SIP/ILS.pm
436 sub renew {
437     my ($self, $patron_id, $patron_pwd, $item_id, $title_id,
438     $no_block, $nb_due_date, $third_party,
439     $item_props, $fee_ack) = @_;

The $fee_ack variable is lost in the transition to C4::SIP::ILS::Transaction::Renew.. we need to clone it across as we do for the checkout message transaction.

This is important, as without passing this on, one cannot renew items where charges apply.

To test:
1) Add a rental charge to an itemtype
2) Checkout an item of that itemtype to a user
3) Attempt a renewal of that item via SIP2 and note that it fails
4) Pass the fee_acknowledgement bit in renewal and note the renewal still fails.

This can be achieved using the sip_cli_emulator (after applying bug 34764 to fix it's mishandling of passing the fee_ack bit)
Comment 1 Martin Renvoize 2023-09-12 15:43:18 UTC
Created attachment 155559 [details] [review]
Bug 34767: Pass fee_ack into ::Transaction::Renew(All)

This patch copies the $fee_ack field into the generated
::Transaction::Renew|All objects such that the fee acknowldegement flag
is respected for renewals.

Test plan
To test:
1) Add a rental charge to an itemtype
2) Checkout an item of that itemtype to a user
3) Attempt a renewal of that item via SIP2 and note that it fails
   sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756 -m renew
4) Pass the fee_acknowledgement bit in renewal and note the renewal
   still fails.
   sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m renew
5) Apply patch and note the above now succeeds
   sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m renew
Comment 2 David Nind 2023-09-12 17:56:26 UTC
Created attachment 155567 [details] [review]
Bug 34767: Pass fee_ack into ::Transaction::Renew(All)

This patch copies the $fee_ack field into the generated
::Transaction::Renew|All objects such that the fee acknowldegement flag
is respected for renewals.

Test plan
To test:
1) Add a rental charge to an itemtype
2) Checkout an item of that itemtype to a user
3) Attempt a renewal of that item via SIP2 and note that it fails
   sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756 -m renew
4) Pass the fee_acknowledgement bit in renewal and note the renewal
   still fails.
   sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m renew
5) Apply patch and note the above now succeeds
   sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m renew

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2023-09-12 17:57:59 UTC
Testing notes (using KTD):

1. Applied bug 34764 before starting (not sure whether I should have applied it before starting or at step 5 of the test plan, as not listed as a dependency for this bug).

2. Response from - step 3 (renewal does not work - check details under checkouts for Henry in the staff interface):

./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756 -m renew
Attempting socket connection to localhost:6001...connected!
Use of uninitialized value $hold_mode in string eq at ./misc/sip_cli_emulator.pl line 123.
SEND: 9300CNterm1|COterm1|CPCPL|
READ: 941

Trying 'renew'
SEND: 29NN20230912    17483420230912    174834AOCPL|AA23529000035676|AB39999000007756|AJ|ACterm1|
READ: 300NUN20230912    174834AA23529000035676|AB39999000007756|AJScarlet and black /|AH|BV1.00|BHUSD|BT06|AOCPL|

3. Response - step 4 (renewal does not work - check details under checkouts for Henry in the staff interface):

./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m renew
Attempting socket connection to localhost:6001...connected!
Use of uninitialized value $hold_mode in string eq at ./misc/sip_cli_emulator.pl line 123.
SEND: 9300CNterm1|COterm1|CPCPL|
READ: 941

Trying 'renew'
SEND: 29NN20230912    17493120230912    174931AOCPL|AA23529000035676|AB39999000007756|AJ|ACterm1|BOY|
READ: 300NUN20230912    174931AA23529000035676|AB39999000007756|AJScarlet and black /|AH|BV1.00|BHUSD|BT06|AOCPL|

4. Response from step 5 (renewal now works - showing 1 renewal with 4 of 5 renewals remaining):

./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m renew
Attempting socket connection to localhost:6001...connected!
Use of uninitialized value $hold_mode in string eq at ./misc/sip_cli_emulator.pl line 123.
SEND: 9300CNterm1|COterm1|CPCPL|
READ: 941

Trying 'renew'
SEND: 29NN20230912    17525220230912    175252AOCPL|AA23529000035676|AB39999000007756|AJ|ACterm1|BOY|
READ: 301YNN20230912    175253AA23529000035676|AB39999000007756|AJScarlet and black /|AH20230917    235900|CK|BV1.00|BHUSD|BT06|AOCPL|
Comment 4 Martin Renvoize 2023-09-12 19:19:49 UTC
Thanks so much for testing David, another perfect job.

SIP tends to scare people, so you've reapply mad emy day keeping this marching forward.
Comment 5 Marcel de Rooy 2023-09-15 06:31:50 UTC
Created attachment 155636 [details] [review]
Bug 34767: Pass fee_ack into ::Transaction::Renew(All)

This patch copies the $fee_ack field into the generated
::Transaction::Renew|All objects such that the fee acknowldegement flag
is respected for renewals.

Test plan
To test:
1) Add a rental charge to an itemtype
2) Checkout an item of that itemtype to a user
3) Attempt a renewal of that item via SIP2 and note that it fails
   sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756 -m renew
4) Pass the fee_acknowledgement bit in renewal and note the renewal
   still fails.
   sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m renew
5) Apply patch and note the above now succeeds
   sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m renew

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Tomás Cohen Arazi 2023-09-15 14:59:13 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 7 Fridolin Somers 2023-09-18 08:01:19 UTC
Pushed to 23.05.x for 23.05.04
Comment 8 Matt Blenkinsop 2023-10-12 15:32:31 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x