Bug 21478 - Koha::Hold->suspend_hold allows suspending in transit holds
Summary: Koha::Hold->suspend_hold allows suspending in transit holds
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 22206
  Show dependency treegraph
 
Reported: 2018-10-02 19:07 UTC by Tomás Cohen Arazi
Modified: 2019-02-25 09:38 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21478: Add Koha::Exceptions::Hold (3.45 KB, patch)
2019-01-25 16:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21478: Unit tests (5.03 KB, patch)
2019-01-25 16:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21478: Make Koha::Hold->suspend reject found holds (1.81 KB, patch)
2019-01-25 16:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21478: Add Koha::Exceptions::Hold (3.52 KB, patch)
2019-01-25 20:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21478: Unit tests (5.10 KB, patch)
2019-01-25 20:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21478: Make Koha::Hold->suspend reject found holds (1.88 KB, patch)
2019-01-25 20:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21478: Add Koha::Exceptions::Hold (3.57 KB, patch)
2019-01-27 10:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21478: Unit tests (5.15 KB, patch)
2019-01-27 10:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21478: Make Koha::Hold->suspend reject found holds (1.92 KB, patch)
2019-01-27 10:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21478: (QA follow-up) Make sure tests don't fail randomly (874 bytes, patch)
2019-01-28 15:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.