Bug 26459 - Allow sip_cli_emulator to handle cancelling holds
Summary: Allow sip_cli_emulator to handle cancelling holds
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-09-14 17:02 UTC by Nick Clemens
Modified: 2021-04-07 14:38 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00


Attachments
Bug 26459: Add hold-mode option to sip_cli_emulator (1.99 KB, patch)
2020-09-14 17:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26459: Add hold-mode option to sip_cli_emulator (2.04 KB, patch)
2020-11-05 10:25 UTC, David Nind
Details | Diff | Splinter Review
Bug 26459: (follow-up) Clarify language and remove duplicated code (1.31 KB, patch)
2021-03-29 18:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26459: Add hold-mode option to sip_cli_emulator (2.10 KB, patch)
2021-03-30 22:29 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 26459: (follow-up) Clarify language and remove duplicated code (1.37 KB, patch)
2021-03-30 22:29 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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!