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: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-09-12 13:10 UTC by Martin Renvoize
Modified: 2023-10-12 15:32 UTC (History)
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


Attachments
Bug 34764: Set fee_acknowledged to expect a string (4.74 KB, patch)
2023-09-12 14:33 UTC, Martin Renvoize
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
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 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 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 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 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 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