34764 – sip_cli_emulator -fa/--fee_acknowledge does not act as expected

Bug 34764 - sip_cli_emulator -fa/--fee_acknowledge does not act as expected

Summary: sip_cli_emulator -fa/--fee_acknowledge does not act as expected

Status:	CLOSED FIXED

Alias:	None

Product:	Koha
Classification:	Unclassified
Component:	Command-line Utilities (show other bugs)
Version:	unspecified
Hardware:	All All

Importance:	P5 - low major
Assignee:	Martin Renvoize (ashimema)
QA Contact:	Kyle M Hall (khall)

URL:
Keywords:

Depends on:
Blocks:

Reported:	2023-09-12 13:10 UTC by Martin Renvoize (ashimema)
Modified:	2024-07-04 20:40 UTC (History)
CC List:	4 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
Circulation function:

Attachments
Bug 34764: Set fee_acknowledged to expect a string (4.74 KB, patch) 2023-09-12 14:33 UTC, Martin Renvoize (ashimema)	Details \| Diff \| Splinter Review
Bug 34764: Set fee_acknowledged to expect a string (4.86 KB, patch) 2023-09-12 21:55 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 34764: Set fee_acknowledged to expect a string (4.92 KB, patch) 2023-09-13 18:40 UTC, Kyle M Hall (khall)	Details \| Diff \| Splinter Review
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Martin Renvoize (ashimema) 2023-09-12 13:10:48 UTC

The sip_cli_emulator documents a `fee_acknoledge`/`fa` option.  In the code it's treated as a boolean in some places and a string in others which results in incorrect messages being generated.

Comment 1 Martin Renvoize (ashimema) 2023-09-12 14:33:11 UTC

Created attachment 155553 [details] [review]
Bug 34764: Set fee_acknowledged to expect a string

For consistency with other options I opted to have the fee acknowledged
parameter to expect a string and as it's an optional parameter I've
dropped the default value of 'N' too.

Test plan
1) Prior to this patch
1a) Attempt a checkout without passing -fa/--fee_acknowledged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756`
    The final field of the SIP request will be '|BON', the default
1b) Attempt a checkout passing -fa/--fee_acknowledged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged --patron 23529000035676 --item 39999000007756`
    The final field of the SIP request will still be '|BON', failure
1c) Attempt a checkout passing a string for fee_acknoewledeged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756`
    The final field of the SIP request will still be '|BON', failure
2) Apply the patch
2a) Attempt a checkout without passing -fa/--fee_acknowledged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756`
    The optional `|BO` element should not be present
2b) Attempt a checkout passing -fa/--fee_acknowledged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged N --patron 23529000035676 --item 39999000007756`
    The final field of the SIP request will now be '|BON', success
2c) Attempt a checkout passing a string for fee_acknoewledeged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756`
    The final field of the SIP request will now be '|BOY', success

Comment 2 David Nind 2023-09-12 18:26:46 UTC

When I issue any of the commands, either before or after the patch is applied, I get no response back. For example:

./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756
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

Comment 3 Martin Renvoize (ashimema) 2023-09-12 19:26:50 UTC

Darn.. I missed the '-m checkout' on the end of all those commands.  Must have got lost in the copy/paste from my own testing.

Without that part, SIP doesn't know which message to send..oops

Comment 4 David Nind 2023-09-12 21:55:17 UTC

Created attachment 155578 [details] [review]
Bug 34764: Set fee_acknowledged to expect a string

For consistency with other options I opted to have the fee acknowledged
parameter to expect a string and as it's an optional parameter I've
dropped the default value of 'N' too.

Test plan
1) Prior to this patch
1a) Attempt a checkout without passing -fa/--fee_acknowledged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756 -m checkout`
    The final field of the SIP request will be '|BON', the default
1b) Attempt a checkout passing -fa/--fee_acknowledged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged --patron 23529000035676 --item 39999000007756 -m checkout`
    The final field of the SIP request will still be '|BON', failure
1c) Attempt a checkout passing a string for fee_acknoewledeged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m checkout`
    The final field of the SIP request will still be '|BON', failure
2) Apply the patch
2a) Attempt a checkout without passing -fa/--fee_acknowledged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756 -m checkout`
    The optional `|BO` element should not be present
2b) Attempt a checkout passing -fa/--fee_acknowledged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged N --patron 23529000035676 --item 39999000007756 -m checkout`
    The final field of the SIP request will now be '|BON', success
2c) Attempt a checkout passing a string for fee_acknoewledeged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m checkout`
    The final field of the SIP request will now be '|BOY', success

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

Comment 5 David Nind 2023-09-13 00:14:38 UTC

Testing notes (using KTD):

1. After each command, I checked the item back in using the staff interface when required. I tested without and with a rental fee for the item type - not sure whether I needed to do that or not, or if that was going overboard!

2. I edited the commit message to include the '-m checkout' at the end of each command (see comment #3).

3. (1a) Result from step 1a:

without a rental charge for the item type - item checked out, no charges
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756 -m checkout
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 'checkout'
SEND: 11YN20230912    21260620230912    212606AOCPL|AA23529000035676|AB39999000007756|ACterm1|BON|BIN|
READ: 121NNY20230912    212606AOCPL|AA23529000035676|AB39999000007756|AJScarlet and black /|AH20230917    235900|

with a rental charge for the item type - item not checked out, no charges
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756 -m checkout
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 'checkout'
SEND: 11YN20230912    23592420230912    235924AOCPL|AA23529000035676|AB39999000007756|ACterm1|BON|BIN|
READ: 120NUN20230912    235924AOCPL|AA23529000035676|AB39999000007756|AJScarlet and black /|AH|BLY|BV1.00|BHUSD|BT06|


4. (1b) Result from step 1b:

without a rental charge for the item type - item checked out, no charges
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged --patron 23529000035676 --item 39999000007756 -m checkout
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 'checkout'
SEND: 11YN20230912    21295620230912    212956AOCPL|AA23529000035676|AB39999000007756|ACterm1|BON|BIN|
READ: 121NNY20230912    212956AOCPL|AA23529000035676|AB39999000007756|AJScarlet and black /|AH20230917    235900|

with a rental charge for the item type - item not checked out, no charges
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged --patron 23529000035676 --item 39999000007756 -m checkout
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 'checkout'
SEND: 11YN20230913    00002820230913    000028AOCPL|AA23529000035676|AB39999000007756|ACterm1|BON|BIN|
READ: 120NUN20230913    000028AOCPL|AA23529000035676|AB39999000007756|AJScarlet and black /|AH|BLY|BV1.00|BHUSD|BT06|


5. (1c) Result from step 1c:

without a rental charge for the item type - item checked out, no charges
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m checkout
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 'checkout'
SEND: 11YN20230912    21312020230912    213120AOCPL|AA23529000035676|AB39999000007756|ACterm1|BON|BIN|
READ: 121NNY20230912    213120AOCPL|AA23529000035676|AB39999000007756|AJScarlet and black /|AH20230917    235900|

with a rental charge for the item type - item not checked out, no charges
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m checkout
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 'checkout'
SEND: 11YN20230913    00013520230913    000135AOCPL|AA23529000035676|AB39999000007756|ACterm1|BON|BIN|
READ: 120NUN20230913    000135AOCPL|AA23529000035676|AB39999000007756|AJScarlet and black /|AH|BLY|BV1.00|BHUSD|BT06|


6. (2a) Result from step 2a - item not checked out:

without a rental charge for the item type - item checked out, no charges
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756 -m checkout
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 'checkout'
SEND: 11YN20230912    21324520230912    213245AOCPL|AA23529000035676|AB39999000007756|ACterm1|BIN|
READ: 121NNY20230912    213245AOCPL|AA23529000035676|AB39999000007756|AJScarlet and black /|AH20230917    235900|

with a rental charge for the item type - item not checked out, no charges
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756 -m checkout
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 'checkout'
SEND: 11YN20230912    23555020230912    235550AOCPL|AA23529000035676|AB39999000007756|ACterm1|BIN|
READ: 120NUN20230912    235550AOCPL|AA23529000035676|AB39999000007756|AJScarlet and black /|AH|BLY|BV1.00|BHUSD|BT06|


7. (2b) Result from step 2b - item not checked out:

without a rental charge for the item type - item checked out, no charges
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged N --patron 23529000035676 --item 39999000007756 -m checkout
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 'checkout'
SEND: 11YN20230912    21434320230912    214343AOCPL|AA23529000035676|AB39999000007756|ACterm1|BON|BIN|
READ: 121NNY20230912    214343AOCPL|AA23529000035676|AB39999000007756|AJScarlet and black /|AH20230917    235900|

with a rental charge for the item type - item not checked out, no charges
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged N --patron 23529000035676 --item 39999000007756 -m checkout
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 'checkout'
SEND: 11YN20230912    23565620230912    235656AOCPL|AA23529000035676|AB39999000007756|ACterm1|BON|BIN|
READ: 120NUN20230912    235656AOCPL|AA23529000035676|AB39999000007756|AJScarlet and black /|AH|BLY|BV1.00|BHUSD|BT06|


8. 2(c) Result from step 2c - item checked out:

without a rental charge for the item type - item checked out, no charges
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m checkout
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 'checkout'
SEND: 11YN20230912    21451820230912    214518AOCPL|AA23529000035676|AB39999000007756|ACterm1|BOY|BIN|
READ: 121NNY20230912    214518AOCPL|AA23529000035676|AB39999000007756|AJScarlet and black /|AH20230917    235900|

with a rental charge for the item type - item checked out + $1.00 charge
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m checkout
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 'checkout'
SEND: 11YN20230912    23533120230912    235331AOCPL|AA23529000035676|AB39999000007756|ACterm1|BOY|BIN|
READ: 121NNY20230912    235332AOCPL|AA23529000035676|AB39999000007756|AJScarlet and black /|AH20230917    235900|BV1.00|BHUSD|BT06|

Comment 6 Kyle M Hall (khall) 2023-09-13 18:40:05 UTC

Created attachment 155597 [details] [review]
Bug 34764: Set fee_acknowledged to expect a string

For consistency with other options I opted to have the fee acknowledged
parameter to expect a string and as it's an optional parameter I've
dropped the default value of 'N' too.

Test plan
1) Prior to this patch
1a) Attempt a checkout without passing -fa/--fee_acknowledged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756 -m checkout`
    The final field of the SIP request will be '|BON', the default
1b) Attempt a checkout passing -fa/--fee_acknowledged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged --patron 23529000035676 --item 39999000007756 -m checkout`
    The final field of the SIP request will still be '|BON', failure
1c) Attempt a checkout passing a string for fee_acknoewledeged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m checkout`
    The final field of the SIP request will still be '|BON', failure
2) Apply the patch
2a) Attempt a checkout without passing -fa/--fee_acknowledged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --patron 23529000035676 --item 39999000007756 -m checkout`
    The optional `|BO` element should not be present
2b) Attempt a checkout passing -fa/--fee_acknowledged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged N --patron 23529000035676 --item 39999000007756 -m checkout`
    The final field of the SIP request will now be '|BON', success
2c) Attempt a checkout passing a string for fee_acknoewledeged flag
    `./misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 -l CPL --fee-acknowledged Y --patron 23529000035676 --item 39999000007756 -m checkout`
    The final field of the SIP request will now be '|BOY', success

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Comment 7 Tomás Cohen Arazi (tcohen) 2023-09-15 14:59:09 UTC

Pushed to master for 23.11.

Nice work everyone, thanks!

Comment 8 Fridolin Somers 2023-09-18 07:40:12 UTC

Pushed to 23.05.x for 23.05.04

Comment 9 Matt Blenkinsop 2023-10-12 15:32:27 UTC

Nice work everyone!

Pushed to oldstable for 22.11.x