Description
Lucas Gass (lukeg)
2024-12-03 21:29:17 UTC
Created attachment 175097 [details] [review] Bug 38615: Add ability to set hold cancellation request via SIP I think this should be SIP option instead of a system preference since some SIP accounts might prefer to outright cancel vs request to cancel. Created attachment 178046 [details] [review] Bug 38615: Add ability to set hold cancellation request via SIP Created attachment 178047 [details] [review] Bug 38615: Atomic update Created attachment 178055 [details] [review] Bug 38615: Add ability to set hold cancellation request via SIP To test: 1. APPLY PATCH, updatedatabase, restart_all 2. Search for the new system preference HoldCancellationRequestSIP, it should be set to 'Don't'. Leave it as is. 3. Make some holds, either in the staff client of via SIP. -/kohadevbox/koha/misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m hold --patron 23529000035676 --item 39999000001310 --hold-mode + 4. Mark the hold as waiting. 5. Cancel it via SIP: -/kohadevbox/koha/misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m hold --patron 23529000035676 --item 39999000001310 --hold-mode - 6. The hold is cancelled like normal, the screen msg should be "Hold Cancelled." 7. Change HoldCancellationRequestSIP to 'Do'. 8. Do steps 3-5 again. 9. Now the screen message is 'Hold Cancellation Requested.' 10. Now check Holds awaiting pickup. In the "Holds with cancellation requests" tab you should now see the hold from step 8. Created attachment 178056 [details] [review] Bug 38615: Atomic update Created attachment 178098 [details] [review] Bug 38615: Add ability to set hold cancellation request via SIP To test: 1. APPLY PATCH, updatedatabase, restart_all 2. Search for the new system preference HoldCancellationRequestSIP, it should be set to 'Don't'. Leave it as is. 3. Make some holds, either in the staff client of via SIP. -/kohadevbox/koha/misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m hold --patron 23529000035676 --item 39999000001310 --hold-mode + 4. Mark the hold as waiting. 5. Cancel it via SIP: -/kohadevbox/koha/misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m hold --patron 23529000035676 --item 39999000001310 --hold-mode - 6. The hold is cancelled like normal, the screen msg should be "Hold Cancelled." 7. Change HoldCancellationRequestSIP to 'Do'. 8. Do steps 3-5 again. 9. Now the screen message is 'Hold Cancellation Requested.' 10. Now check Holds awaiting pickup. In the "Holds with cancellation requests" tab you should now see the hold from step 8. Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Created attachment 178099 [details] [review] Bug 38615: Atomic update Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Thanks for the test plan with the example SIP commands. With the new system preference set to HoldCancellationRequestSIP set to Do, cancelling a waiting hold via SIP returned |AFHold Cancellation Requested.| and the item appeared in the Holds with cancellation requests tab on the Holds awaiting pickup page. With HoldCancellationRequestSIP set to don't the SIP request returns |AFHold Cancelled.| When testing I noticed something both with and without the patch (so this is a general SIP question) If you place a hold and then try to cancel the hold with this command before checking the item into the hold shelf the SIP cancel request returns |AFError with transaction drop_hold:| I'm just curious why that is, or if there's a different SIP command that would be able to cancel a hold that was just placed but no other action was taken on the item? Please provide feedback on last question raised. Changing status. (In reply to Marcel de Rooy from comment #10) > Please provide feedback on last question raised. Changing status. This is unrelated and outside the scope of this bug report. > I'm just curious why that is, or if there's a different SIP command that
> would be able to cancel a hold that was just placed but no other action was
> taken on the item?
This sounds like an unrelated bug. If you can file a bug report for that I can have a look at it!
(In reply to Kyle M Hall (khall) from comment #12) > > I'm just curious why that is, or if there's a different SIP command that > > would be able to cancel a hold that was just placed but no other action was > > taken on the item? > > This sounds like an unrelated bug. If you can file a bug report for that I > can have a look at it! I filed Bug 39204. Sorry dude.. can we get a unit test to cover this change pretty please.. you know it'll help prevent future regressions ;) Created attachment 179288 [details] [review] Bug 38615: Add ability to set hold cancellation request via SIP To test: 1. APPLY PATCH, updatedatabase, restart_all 2. Search for the new system preference HoldCancellationRequestSIP, it should be set to 'Don't'. Leave it as is. 3. Make some holds, either in the staff client of via SIP. -/kohadevbox/koha/misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m hold --patron 23529000035676 --item 39999000001310 --hold-mode + 4. Mark the hold as waiting. 5. Cancel it via SIP: -/kohadevbox/koha/misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m hold --patron 23529000035676 --item 39999000001310 --hold-mode - 6. The hold is cancelled like normal, the screen msg should be "Hold Cancelled." 7. Change HoldCancellationRequestSIP to 'Do'. 8. Do steps 3-5 again. 9. Now the screen message is 'Hold Cancellation Requested.' 10. Now check Holds awaiting pickup. In the "Holds with cancellation requests" tab you should now see the hold from step 8. Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Created attachment 179289 [details] [review] Bug 38615: (follow-up) Unit tests Adds tests for the new HoldCancellationRequestSIP preference Test plan: 1. prove t/db_dependent/SIP/ILS.t 2. prove t/db_dependent/SIP/Transaction.t Thanks for looking at this Martin! I'm just learning to write unit tests so please let me know if I missed anything (In reply to Brendan Lawlor from comment #17) > Thanks for looking at this Martin! > > I'm just learning to write unit tests so please let me know if I missed > anything Nice Brendan! Thank you for the unit tests! Created attachment 179309 [details] [review] Bug 38615: Atomic update Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 179310 [details] [review] Bug 38615: Add ability to set hold cancellation request via SIP To test: 1. APPLY PATCH, updatedatabase, restart_all 2. Search for the new system preference HoldCancellationRequestSIP, it should be set to 'Don't'. Leave it as is. 3. Make some holds, either in the staff client of via SIP. -/kohadevbox/koha/misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m hold --patron 23529000035676 --item 39999000001310 --hold-mode + 4. Mark the hold as waiting. 5. Cancel it via SIP: -/kohadevbox/koha/misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -su term1 -sp term1 -m hold --patron 23529000035676 --item 39999000001310 --hold-mode - 6. The hold is cancelled like normal, the screen msg should be "Hold Cancelled." 7. Change HoldCancellationRequestSIP to 'Do'. 8. Do steps 3-5 again. 9. Now the screen message is 'Hold Cancellation Requested.' 10. Now check Holds awaiting pickup. In the "Holds with cancellation requests" tab you should now see the hold from step 8. Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 179311 [details] [review] Bug 38615: (follow-up) Unit tests Adds tests for the new HoldCancellationRequestSIP preference Test plan: 1. prove t/db_dependent/SIP/ILS.t 2. prove t/db_dependent/SIP/Transaction.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Excellent work, thanks for the unit tests Brendan. All working great now and covered. Passing QA + if ( C4::Context->preference('HoldCancellationRequestSIP') ) { + $trans->screen_msg("Hold Cancellation Requested."); I sighed a little at capitalization here, but realize it's just following the style of the other messages. Maybe something to fix sometime (later). Fixed when moving the database update: "Added new system preference '38615'" Please also use say_success. Pushed for 25.05! Well done everyone, thank you! |