Bug 21478

Summary: Koha::Hold->suspend_hold allows suspending in transit holds
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, kyle.m.hall, lucas, martin.renvoize, nick
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 22206    
Attachments: Bug 21478: Add Koha::Exceptions::Hold
Bug 21478: Unit tests
Bug 21478: Make Koha::Hold->suspend reject found holds
Bug 21478: Add Koha::Exceptions::Hold
Bug 21478: Unit tests
Bug 21478: Make Koha::Hold->suspend reject found holds
Bug 21478: Add Koha::Exceptions::Hold
Bug 21478: Unit tests
Bug 21478: Make Koha::Hold->suspend reject found holds
Bug 21478: (QA follow-up) Make sure tests don't fail randomly

Description Tomás Cohen Arazi 2018-10-02 19:07:50 UTC
My understanding is that both 'waiting' and 'in transit' holds are 'found', and in both cases suspending them should be disallowed. I haven't looked at the places in which this could happen, but the code implementing the checks in Koha::Hold clearly misses the use case.

The method should be renamed to the new guidelines (->suspend) and it should raise exceptions in both cases.
Comment 1 Jonathan Druart 2018-10-22 21:27:44 UTC
Indeed, the condition exist template-side only.

* in holds_table.inc (for request.pl)
177             [% IF SuspendHoldsIntranet %]
178                 <td>
179                     [% UNLESS ( hold.found ) %]


* in holds.js (for "Check out", "Details" from the patron's module)
152                             if ( oObj.found ) {
153                                 return "";
154                             } else if ( oObj.suspend == 1 ) {
155                                 return "<a class='hold-resume btn btn-default btn-xs' id='resume" + oObj.reserve_id + "'>"
156                                      + "<i class='fa fa-play'></i> " + RESUME + "</a>";
157                             } else {
158                                 return "<a class='hold-suspend btn btn-default btn-xs' id='suspend" + oObj.reserve_id + "'>"
159                                      + "<i class='fa fa-pause'></i> " + SUSPEND + "</a>";
160                             }

Other calls to ModReserve do not suspend/resume, except the one from Koha/REST/V1/Hold.pm.
Comment 2 Tomás Cohen Arazi 2019-01-25 16:52:59 UTC
Created attachment 84409 [details] [review]
Bug 21478: Add Koha::Exceptions::Hold
Comment 3 Tomás Cohen Arazi 2019-01-25 16:53:03 UTC
Created attachment 84410 [details] [review]
Bug 21478: Unit tests
Comment 4 Tomás Cohen Arazi 2019-01-25 16:53:07 UTC
Created attachment 84411 [details] [review]
Bug 21478: Make Koha::Hold->suspend reject found holds
Comment 5 Kyle M Hall 2019-01-25 20:12:21 UTC
Created attachment 84432 [details] [review]
Bug 21478: Add Koha::Exceptions::Hold

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Kyle M Hall 2019-01-25 20:12:30 UTC
Created attachment 84433 [details] [review]
Bug 21478: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall 2019-01-25 20:12:33 UTC
Created attachment 84434 [details] [review]
Bug 21478: Make Koha::Hold->suspend reject found holds

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Katrin Fischer 2019-01-27 10:45:15 UTC
Created attachment 84456 [details] [review]
Bug 21478: Add Koha::Exceptions::Hold

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2019-01-27 10:45:20 UTC
Created attachment 84457 [details] [review]
Bug 21478: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2019-01-27 10:45:24 UTC
Created attachment 84458 [details] [review]
Bug 21478: Make Koha::Hold->suspend reject found holds

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Nick Clemens 2019-01-28 14:13:58 UTC
Awesome work all!

Pushed to master for 19.05
Comment 12 Tomás Cohen Arazi 2019-01-28 15:30:44 UTC
Created attachment 84487 [details] [review]
Bug 21478: (QA follow-up) Make sure tests don't fail randomly

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Martin Renvoize 2019-02-01 12:02:29 UTC
Pushed to 18.11.x for 18.11.03
Comment 14 Lucas Gass 2019-02-05 23:21:35 UTC
pushed to 18.05.x for 18.05.09
Comment 15 Fridolin Somers 2019-02-12 15:19:36 UTC
Needs some work for 17.11.x.
I'm not sure it is worth doing it.
Comment 16 Tomás Cohen Arazi 2019-02-12 15:20:10 UTC
(In reply to Fridolin SOMERS from comment #15)
> Needs some work for 17.11.x.
> I'm not sure it is worth doing it.

No worries about it.