| Summary: | Allow sip_cli_emulator to handle cancelling holds | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Nick Clemens (kidclamp) <nick> |
| Component: | Command-line Utilities | Assignee: | Nick Clemens (kidclamp) <nick> |
| Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
| Severity: | enhancement | ||
| Priority: | P5 - low | CC: | andrew, 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
|
|
| Circulation function: | |||
| 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 (kidclamp)
2020-09-14 17:02:59 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 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> 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)
Created attachment 118980 [details] [review] Bug 26459: (follow-up) Clarify language and remove duplicated code (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 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. 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> Created attachment 119027 [details] [review] Bug 26459: (follow-up) Clarify language and remove duplicated code Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Still work, QA tools happy, code ok :) Pushed to master for 21.05, thanks to everybody involved! Applies on 20.11.x, I choose to backport. Pushed to 20.11.x for 20.11.05 enhancement, not backporting to 20.05. |