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.