Bug 34764

Summary:	sip_cli_emulator -fa/--fee_acknowledge does not act as expected
Product:	Koha	Reporter:	Martin Renvoize (ashimema) <martin.renvoize>
Component:	Command-line Utilities	Assignee:	Martin Renvoize (ashimema) <martin.renvoize>
Status:	CLOSED FIXED	QA Contact:	Kyle M Hall (khall) <kyle>
Severity:	major
Priority:	P5 - low	CC:	david, fridolin.somers, kyle, robin
Version:	unspecified
Hardware:	All
OS:	All
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 Bug 34764: Set fee_acknowledged to expect a string Bug 34764: Set fee_acknowledged to expect a string

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