Bug 38615 - Cancelling a waiting hold via SIP should include an option to move it to holds with cancellation requests
Summary: Cancelling a waiting hold via SIP should include an option to move it to hold...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Lucas Gass (lukeg)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-12-03 21:29 UTC by Lucas Gass (lukeg)
Modified: 2025-03-17 07:28 UTC (History)
4 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 38615: Add ability to set hold cancellation request via SIP (3.59 KB, patch)
2024-12-03 22:56 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 38615: Add ability to set hold cancellation request via SIP (3.62 KB, patch)
2025-02-13 19:27 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 38615: Atomic update (1.29 KB, patch)
2025-02-13 19:27 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 38615: Add ability to set hold cancellation request via SIP (4.53 KB, patch)
2025-02-13 21:57 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 38615: Atomic update (1.29 KB, patch)
2025-02-13 21:57 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 38615: Add ability to set hold cancellation request via SIP (4.58 KB, patch)
2025-02-14 14:45 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 38615: Atomic update (1.34 KB, patch)
2025-02-14 14:45 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 38615: Add ability to set hold cancellation request via SIP (4.61 KB, patch)
2025-03-13 20:19 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 38615: (follow-up) Unit tests (6.31 KB, patch)
2025-03-13 20:19 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 38615: Atomic update (1.41 KB, patch)
2025-03-14 08:17 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38615: Add ability to set hold cancellation request via SIP (4.68 KB, patch)
2025-03-14 08:17 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38615: (follow-up) Unit tests (6.38 KB, patch)
2025-03-14 08:17 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass (lukeg) 2024-12-03 21:29:17 UTC
You can cancel a hold via SIP, the hold is immediately canceled. There should be an option ( system preference ) to allow cancelled waiting holds from SIP to move to 'Holds with cancellation requests'.
Comment 1 Lucas Gass (lukeg) 2024-12-03 22:56:52 UTC
Created attachment 175097 [details] [review]
Bug 38615: Add ability to set hold cancellation request via SIP
Comment 2 Lucas Gass (lukeg) 2024-12-04 16:19:46 UTC
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.
Comment 3 Lucas Gass (lukeg) 2025-02-13 19:27:26 UTC
Created attachment 178046 [details] [review]
Bug 38615: Add ability to set hold cancellation request via SIP
Comment 4 Lucas Gass (lukeg) 2025-02-13 19:27:29 UTC
Created attachment 178047 [details] [review]
Bug 38615: Atomic update
Comment 5 Lucas Gass (lukeg) 2025-02-13 21:57:04 UTC
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.
Comment 6 Lucas Gass (lukeg) 2025-02-13 21:57:06 UTC
Created attachment 178056 [details] [review]
Bug 38615: Atomic update
Comment 7 Brendan Lawlor 2025-02-14 14:45:40 UTC
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>
Comment 8 Brendan Lawlor 2025-02-14 14:45:42 UTC
Created attachment 178099 [details] [review]
Bug 38615: Atomic update

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 9 Brendan Lawlor 2025-02-14 15:06:10 UTC
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?
Comment 10 Marcel de Rooy 2025-02-21 07:56:13 UTC
Please provide feedback on last question raised. Changing status.
Comment 11 Kyle M Hall (khall) 2025-02-26 13:02:06 UTC
(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.
Comment 12 Kyle M Hall (khall) 2025-02-26 13:02:38 UTC
> 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!
Comment 13 Lucas Gass (lukeg) 2025-02-26 15:40:06 UTC
(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.
Comment 14 Martin Renvoize (ashimema) 2025-03-13 15:27:57 UTC
Sorry dude.. can we get a unit test to cover this change pretty please.. you know it'll help prevent future regressions ;)
Comment 15 Brendan Lawlor 2025-03-13 20:19:45 UTC
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>
Comment 16 Brendan Lawlor 2025-03-13 20:19:48 UTC
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
Comment 17 Brendan Lawlor 2025-03-13 20:21:38 UTC
Thanks for looking at this Martin!

I'm just learning to write unit tests so please let me know if I missed anything
Comment 18 Lucas Gass (lukeg) 2025-03-13 22:04:37 UTC
(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!
Comment 19 Martin Renvoize (ashimema) 2025-03-14 08:17:21 UTC
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>
Comment 20 Martin Renvoize (ashimema) 2025-03-14 08:17:24 UTC
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>
Comment 21 Martin Renvoize (ashimema) 2025-03-14 08:17:26 UTC
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>
Comment 22 Martin Renvoize (ashimema) 2025-03-14 08:18:11 UTC
Excellent work, thanks for the unit tests Brendan.

All working great now and covered. Passing QA
Comment 23 Katrin Fischer 2025-03-17 06:55:20 UTC
+    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).
Comment 24 Katrin Fischer 2025-03-17 06:58:45 UTC
Fixed when moving the database update:

"Added new system preference '38615'"

Please also use say_success.
Comment 25 Katrin Fischer 2025-03-17 07:28:20 UTC
Pushed for 25.05!

Well done everyone, thank you!