Bug 26459

Summary: Allow sip_cli_emulator to handle cancelling holds
Product: Koha Reporter: Nick Clemens <nick>
Component: Command-line UtilitiesAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: andrewfh, fridolin.somers, kyle, robin, victor
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.05
Attachments: Bug 26459: Add hold-mode option to sip_cli_emulator
Bug 26459: Add hold-mode option to sip_cli_emulator
Bug 26459: (follow-up) Clarify language and remove duplicated code
Bug 26459: Add hold-mode option to sip_cli_emulator
Bug 26459: (follow-up) Clarify language and remove duplicated code

Description Nick Clemens 2020-09-14 17:02:59 UTC

    
Comment 1 Nick Clemens 2020-09-14 17:06:04 UTC
Created attachment 110071 [details] [review]
Bug 26459: Add hold-mode option to sip_cli_emulator

To test:
1 - Apply patch
2 - Restart all
3 - Place an item level hold for a patron via the staff client
4 - Cancel the hold using the sip_cli_emulator with hold-mode: -
perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 --patron {cardnumber} --item {BARCODE}  -m hold -l CPL --hold-mode -
5 - Run the sip_cli_emulator with no params
6 - Verify help text is understandable
Comment 2 David Nind 2020-11-05 10:25:05 UTC
Created attachment 113066 [details] [review]
Bug 26459: Add hold-mode option to sip_cli_emulator

To test:
1 - Apply patch
2 - Restart all
3 - Place an item level hold for a patron via the staff client
4 - Cancel the hold using the sip_cli_emulator with hold-mode: -
perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 --patron {cardnumber} --item {BARCODE}  -m hold -l CPL --hold-mode -
5 - Run the sip_cli_emulator with no params
6 - Verify help text is understandable

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Victor Grousset/tuxayo 2021-03-27 16:43:53 UTC
It works and QA tools are happy :)

1.
It seems these two lines of code do the same thing: default to + if not - passed

            hold_mode           => $hold_mode eq '-' ? '-' : '+',
    my $hold_mode         = $params->{hold_mode} || '+';

2.
IIUC we are using a separated flag as opposed to having something like "-m hold+"/"-m hold" and "-m hold-" (not sure if that would be more clear to use) because that reflects better the underlying SIP protocol. (direct mapping to the flag) A bit verbose the separate flag but that should make sense.

3.
Accepts +: Add hold or -: Cancel hold, defaults to +
Is ↓that↓ more clear?
Accepts "+" to add hold or "-" to cancel hold, defaults to +

4.
Not really related thing, shouldn't the following place an item level hold?

sip_cli_emulator.pl  -a localhost -p 6001 -su term1 -sp term1 --patron  23529001000463 --item 39999000000511 -m hold -l CPL

It places a record level one. (which can be piled up and bypasses Holds per record circ rule)
Comment 4 Nick Clemens 2021-03-29 18:52:40 UTC
Created attachment 118980 [details] [review]
Bug 26459: (follow-up) Clarify language and remove duplicated code
Comment 5 Nick Clemens 2021-03-29 18:55:40 UTC
(In reply to Victor Grousset/tuxayo from comment #3)
> It works and QA tools are happy :)
> 
> 1.
> It seems these two lines of code do the same thing: default to + if not -
> passed
> 
>             hold_mode           => $hold_mode eq '-' ? '-' : '+',
>     my $hold_mode         = $params->{hold_mode} || '+';

OK, removed

> 2.
> IIUC we are using a separated flag as opposed to having something like "-m
> hold+"/"-m hold" and "-m hold-" (not sure if that would be more clear to
> use) because that reflects better the underlying SIP protocol. (direct
> mapping to the flag) A bit verbose the separate flag but that should make
> sense.

I can rebuild if there is strong preference, I just wanted a quikc way to do this

> 3.
> Accepts +: Add hold or -: Cancel hold, defaults to +
> Is ↓that↓ more clear?
> Accepts "+" to add hold or "-" to cancel hold, defaults to +

Yes, updated

> 4.
> Not really related thing, shouldn't the following place an item level hold?
> 
> sip_cli_emulator.pl  -a localhost -p 6001 -su term1 -sp term1 --patron 
> 23529001000463 --item 39999000000511 -m hold -l CPL
> 
> It places a record level one. (which can be piled up and bypasses Holds per
> record circ rule)

See bug 26461
Comment 6 Victor Grousset/tuxayo 2021-03-30 22:20:34 UTC
Thanks for the fixes :)


> 2. 

> I can rebuild if there is strong preference, I just wanted a quikc way to do this

It's ok, I was just surprised by the params but they also make sense.
Comment 7 Victor Grousset/tuxayo 2021-03-30 22:29:41 UTC
Created attachment 119026 [details] [review]
Bug 26459: Add hold-mode option to sip_cli_emulator

To test:
1 - Apply patch
2 - Restart all
3 - Place an item level hold for a patron via the staff client
4 - Cancel the hold using the sip_cli_emulator with hold-mode: -
perl misc/sip_cli_emulator.pl -a localhost -p 6001 -su term1 -sp term1 --patron {cardnumber} --item {BARCODE}  -m hold -l CPL --hold-mode -
5 - Run the sip_cli_emulator with no params
6 - Verify help text is understandable

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 8 Victor Grousset/tuxayo 2021-03-30 22:29:45 UTC
Created attachment 119027 [details] [review]
Bug 26459: (follow-up) Clarify language and remove duplicated code

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 9 Victor Grousset/tuxayo 2021-03-30 22:30:32 UTC
Still work, QA tools happy, code ok :)
Comment 10 Jonathan Druart 2021-04-07 14:38:09 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 11 Fridolin Somers 2021-04-19 10:01:22 UTC
Applies on 20.11.x, I choose to backport.

Pushed to 20.11.x for 20.11.05
Comment 12 Andrew Fuerste-Henry 2021-04-25 14:15:07 UTC
enhancement, not backporting to 20.05.