---- Reported by nahuel.angelinetti@biblibre.com 2010-03-16 16:10:32 ---- In opac and intranet, items that are waiting to be checked out are considered as "on shelf", so the librarian cannot set any other reserve ---- Additional Comments From gmcharlt@gmail.com 2010-05-01 14:27:55 ---- Patch pushed. Please test and close. --- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:25 UTC --- This bug was previously known as _bug_ 4319 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4319 Actual time not defined. Setting to 0.0 Setting qa contact to the default for this product. This bug either had no qa contact or an invalid one.
This is fine in the opac, in the staff client though it says that this item isn't usually allowed to be put on hold - I can still put it on hold cause I have overrides on.
Starting at line 1609 of Search.pm I see this: # item is withdrawn, lost or damaged if ( $item->{wthdrawn} || $item->{itemlost} || $item->{damaged} || $item->{notforloan} || $reservestatus eq 'Waiting' || ($transfertwhen ne '')) { Why is "$reservestatus eq 'Waiting'" grouped with these statuses which block holds? For that matter, it looks like items which are in transit are dealt with in the same section. Why should holds on these items be blocked? I wonder also if part of the problem is that we're trying to add up the "Available" count at the same time we're trying to decide if something can be put on hold. A waiting hold isn't available, but it can be put on hold.
See also Bug 3012
*** Bug 3012 has been marked as a duplicate of this bug. ***
Created attachment 3740 [details] [review] Proposed patch
git am -ius3 attachment.cgi\?id=3740 Commit Body is: -------------------------- BUG4319 Display hold link when item in transit or reserved Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> -------------------------- Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y Applying: BUG4319 Display hold link when item in transit or reserved fatal: sha1 information is lacking or useless (C4/Search.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 BUG4319 Display hold link when item in transit or reserved When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort".
Created attachment 4099 [details] [review] Updated patch Patch updated with revision 3.05.00.001.
Is this still an issue? I can't seem to replicate this problem.
Created attachment 4110 [details] Screenshot of the problem in master I can confirm the problem still exists. In this example there is only one item attached to the record and it is "waiting" for a patron. The search results screen says "No holds allowed."
Oh! The search results, okay, signed off.
I think this signoff needs to be reconsidered. The patch adds these lines: + # only withdrawn, lost and damaged avoid placing hold on item + $can_place_holds = 1 unless ($item->{withdrawn} || $item->{itemlost} || $item->{damaged}); A damaged status does not always block a hold. It depends on the AllowHoldsOnDamagedItems preference.
Created attachment 4327 [details] [review] Proposed patch (with syspref) Added "AllowHoldsOnDamagedItems" syspref support.
Created attachment 4329 [details] [review] Signed-off patch Thanks for the quick update. This fix now works properly with AllowHoldsOnDamagedItems on or off.
I have run into this problem at a client site, this patch solved their problem, pushing to master
A waiting item can be placed on hold both in the OPAC and staff client. Marking resolved.
Created attachment 16965 [details] can't place a hold on a waiting item This problem is back in Koha 3.10.3.
I think this is solved by Bug 8975
We have a report for this bug from a library on 3.12.9 currently, I will retest on master.
*** Bug 11321 has been marked as a duplicate of this bug. ***
Copying the test plan from the duplicate bug report: When holds on items available items are not allowed (allowonshelfholds = don't allow), it's impossible to place holds on items that are 'on hold' (awaiting pickup). The item is not checked out yet, but unavailable to the patron. I think holds should be allowed in this case. To test: Set allowonshelfholds to don't allow 0) Search for a record with 1 available item Opac status: available No hold can be placed. 1) Check out item to user A Opac status: checked out 'Place hold' button is shown and holds can be placed. 2) Place a title level hold for user B Opac status: checked out 'Place hold' button is shown and holds can be placed. 3) Return item and confirm hold Opac status: on hold No hold can be placed.
I have tested this on current master: 1) Place a hold on an item 2) Return the item, status of the item is "on hold" or "waiting" 3) Make sure AllowOnShelfHolds is set to Allow 4) Place a new hold for another patron - OK 5) Cancel the hold 6) Switch AllowOnShelfHolds to "Don't allow" 7) Verify that placing a hold is not possible. I'd like to try and fix this, but I am not sure if we can agree it's a bug or if this should be optional. There have been different bug reports about this is in the past, so I tend to say it's a bug. Opinions?
My opinion is that it is in fact a bug - because it bugs a lot of people :) No, seriously, it's confusing many users and should be consistent. Nicole
Thx for the comment Nicole!
Hello, I met the same issue with items waiting to be pick-up and items in transit : there are not available but we can't place a new hold on it. Should we open a new bug for that or continue with this one ? Thanks, Sonia
*** This bug has been marked as a duplicate of bug 14935 ***
*** Bug 14935 has been marked as a duplicate of this bug. ***
Still valid in 16.11? anyone tested it? Marjorie
Created attachment 67558 [details] [review] Bug 4319 - [OPAC] allow to holds waiting items Test plan: - Checkout an item - Place hold on this item, - Return the item - Make sure the hold is waiting (found W) and AllowOnShelfHolds is not to 'Allow' - Check that the button "Place hold" appears in opac detail page of the biblio
Created attachment 67584 [details] [review] Bug 4319 - [OPAC] allow to holds waiting items Test plan: - Checkout an item - Place hold on this item, - Return the item - Make sure the hold is waiting (found W) and AllowOnShelfHolds is not to 'Allow' - Check that the button "Place hold" appears in opac detail page of the biblio Followed test plan (see below note), patch worked as described. Also ran QA test tool and both altered files passed. Just a note about the test plan: AllowOnShelfHolds is no longer a syspref, but it exists in the circulation matrix, i.e. it is available on the Administrator->Circulation and fine rules, to test this patch I set it to 'If any unavailable'. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Glad to see this one moving!
Hello, sorry but I've done few tests and for me there's still a problem. When items is in transit, "place hold" doesn't appears on the details page. Items are note avalaible at this moment, so it should be possible to place an hold on it. On the result page, we can see a "Place hold" button but when we click on it, we can't reserve it : "Sorry, none of these items can be placed on hold. Petites histoires à raconter , by Sara Cone Bryant, Natha Caputo No available items." Sonia
Good catch, I agree that "In transit" should also be treated as unavailable.
Created attachment 67637 [details] [review] Bug 4319 - [OPAC] allow to holds items in transit Test plan is the same that for waiting items but with items in transit
Hello, we are making progress : the button "place hold" is diplayed on result and detail pages when an item is on transit. But, when I click on it, I always have the message : " "Sorry, none of these items can be placed on hold. ..; No available items" Sonia
Created attachment 67704 [details] [review] Bug 4319 followup - [OPAC] allow to holds items in transit Changes on C4::Reserves::IsAvailableForItemLevelRequest Make sure this tests pass: - t/db_dependent/Reserves.t - t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
Created attachment 67970 [details] [review] Bug 4319 followup - [OPAC] allow to holds items in transit Changes on C4::Reserves::IsAvailableForItemLevelRequest Make sure this tests pass: - t/db_dependent/Reserves.t - t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Hello, I found the same problem as in comment 31 while testing today : "When items is in transit, "place hold" doesn't appears on the details page"
(In reply to Koha Team Lyon 3 from comment #37) > Hello, > I found the same problem as in comment 31 while testing today : "When items > is in transit, "place hold" doesn't appears on the details page" Can't reproduce. Can you provide a test plan?
Hello Alex, I've used the circulation transfer menu (/cgi-bin/koha/circ/branchtransfers.pl) to put an item on transfer. When you put an item in transfer like that you don't have the 'place hold' link on the opac detail page. A tricky way to do that, perhaps... It seems to be the last case, my other tests are ok... keep trying :) Sonia
Created attachment 68194 [details] [review] Bug 4319 followup - [OPAC] allow to holds items in transit Changes on C4::Reserves::IsAvailableForItemLevelRequest Make sure this tests pass: - t/db_dependent/Reserves.t - t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
Hello Sonia, I've changed the last patch to take into account the items in transit (and not only reserves in transit). Please try again.
Patch tested with a sandbox, by Sonia BOUIS <sonia.bouis@univl-yon3.fr>
Created attachment 68197 [details] [review] Bug 4319 - [OPAC] allow to holds waiting items Test plan: - Checkout an item - Place hold on this item, - Return the item - Make sure the hold is waiting (found W) and AllowOnShelfHolds is not to 'Allow' - Check that the button "Place hold" appears in opac detail page of the biblio Followed test plan (see below note), patch worked as described. Also ran QA test tool and both altered files passed. Just a note about the test plan: AllowOnShelfHolds is no longer a syspref, but it exists in the circulation matrix, i.e. it is available on the Administrator->Circulation and fine rules, to test this patch I set it to 'If any unavailable'. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Sonia BOUIS <sonia.bouis@univl-yon3.fr>
Created attachment 68198 [details] [review] Bug 4319 - [OPAC] allow to holds items in transit Test plan is the same that for waiting items but with items in transit Signed-off-by: Sonia BOUIS <sonia.bouis@univl-yon3.fr>
Created attachment 68199 [details] [review] Bug 4319 followup - [OPAC] allow to holds items in transit Changes on C4::Reserves::IsAvailableForItemLevelRequest Make sure this tests pass: - t/db_dependent/Reserves.t - t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t Signed-off-by: Sonia BOUIS <sonia.bouis@univl-yon3.fr>
Hello, This time, all my tests were OK. I,'ve tried 2 configurations in issuing rules : On shelf holds allowed "if all unvailable"(onshelfholds=2 in issuingrules) and then "if any unavailable" (onshelfholds=0 in issuingrules). By the way, I found the wording is not very clear. I've tried to place an hold on waiting item, reserved item on transit, item in transit without reserve. At each time, the link to reserve appears on results page and on detail page. Thank you Alex
QA: Looking here now
Please squash the patches. I am not sure why you change IsAvailableForItemLevelRequest in the third patch. Please explain. Where is it called in the process? You add: + my $flag = shift; This is not really a clear name ;) Where do you use this parameter ? + if (Koha::Holds->search({itemnumber => $item->{itemnumber}, + found => ['W', 'T']}, + {order_by => {-asc => 'priority'}})) { Why do you order_by if you are interested only in the number ? + return 0; + return GetReserveStatus($item->{itemnumber}) eq "Waiting"; When will you reach this second return ? +my $itemsWaitingOrInTransit = Koha::Holds->search( + { + biblionumber => $biblionumber, + found => ['W', 'T'] + })->count(); + +foreach my $item ( Koha::Items->search(biblionumber => $biblionumber) ) { + $itemsWaitingOrInTransit = 1 if $item->get_transfer; +} If the flag is set, there is no need to start traversing all items or keep doing so. Changing status
Created attachment 68531 [details] [review] Bug 4319 - [OPAC] allow to holds waiting items Test plan: - Checkout an item - Place hold on this item, - Return the item - Make sure the hold is waiting (found W) and AllowOnShelfHolds is not to 'Allow' - Check that the button "Place hold" appears in opac detail page of the biblio - do the samewith items/reserves in transit Changes on C4::Reserves::IsAvailableForItemLevelRequest Make sure this tests pass: - t/db_dependent/Reserves.t - t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
(In reply to Marcel de Rooy from comment #48) > Please squash the patches. > I am not sure why you change IsAvailableForItemLevelRequest in the third > patch. Please explain. Where is it called in the process? It's called in opac/opac-reserve.pl. If you remove this changes, for an item on hold in transit, you have the link "hold" in detail page but you get the message: Sorry, none of these items can be placed on hold. > > You add: > + my $flag = shift; > This is not really a clear name ;) > Where do you use this parameter ? No, it's an ugly remaining debug thing :) > > + if (Koha::Holds->search({itemnumber => $item->{itemnumber}, > + found => ['W', 'T']}, > + {order_by => {-asc => 'priority'}})) { > Why do you order_by if you are interested only in the number ? No reason. And i need a ->count at the end because a resultset is always true. I added a test to cover that. > > + return 0; > + return GetReserveStatus($item->{itemnumber}) eq "Waiting"; > When will you reach this second return ? Oops! A mistake. The last is no more needed. > > +my $itemsWaitingOrInTransit = Koha::Holds->search( > + { > + biblionumber => $biblionumber, > + found => ['W', 'T'] > + })->count(); > + > +foreach my $item ( Koha::Items->search(biblionumber => $biblionumber) ) { > + $itemsWaitingOrInTransit = 1 if $item->get_transfer; > +} > If the flag is set, there is no need to start traversing all items or keep > doing so. I change the code to avoid this I've also squashed patch
Patch didn't apply smoothly: the template variable OPACShowBarcode disappeared from opac/opac-detail.pl. Resubmitting a corrected version.
Created attachment 68782 [details] [review] Bug 4319 - [OPAC] allow to holds waiting items Test plan: - Checkout an item - Place hold on this item, - Return the item - Make sure the hold is waiting (found W) and AllowOnShelfHolds is not to 'Allow' - Check that the button "Place hold" appears in opac detail page of the biblio - do the samewith items/reserves in transit Changes on C4::Reserves::IsAvailableForItemLevelRequest Make sure this tests pass: - t/db_dependent/Reserves.t - t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
Created attachment 68786 [details] [review] Bug 4319 - [OPAC] allow to holds waiting items Test plan: - Checkout an item - Place hold on this item, - Return the item - Make sure the hold is waiting (found W) and AllowOnShelfHolds is not to 'Allow' - Check that the button "Place hold" appears in opac detail page of the biblio - do the samewith items/reserves in transit Changes on C4::Reserves::IsAvailableForItemLevelRequest Make sure this tests pass: - t/db_dependent/Reserves.t - t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
+ OPACShowBarcode => C4::Context->preference("OPACShowBarcode") Regression from a rebase ?
It would be more consistent if we should use IsItemOnHoldAndFound in IsAvailableForItemLevelRequest instead of a Koha::Holds->search. We actually should adjust that routine. It does the same. The formerly used GetReserveStatus call returns false for transit holds. Now we check the count on W and T. This actually makes me think that the report name should reflect that; the real change is on the transit side. Cannot finish this session now. Will try to finish monday. Parking it in Blocked.
Conflicts now with just pushed 18547
Are you fixing a missing status (Transit) from GetReserveStatus by working around it in IsAvailableForItemLevelRequest ? (See above too) What about other calls of GetReserveStatus ? - [% IF ( AllowOnShelfHolds OR ItemsIssued ) %] + [% IF ( AllowOnShelfHolds OR ItemsIssued OR ItemsWaitingOrInTransit ) %] Would it be easier to just have one template var for this? +my $itemsWaitingOrInTransit = Koha::Holds->search( + { + biblionumber => $biblionumber, + found => ['W', 'T'] + })->count(); +unless ($itemsWaitingOrInTransit) { + foreach my $item ( Koha::Items->search(biblionumber => $biblionumber) ) { + $itemsWaitingOrInTransit = 1 if $item->get_transfer; + } +} Move this code into a sub ? And test it ? I think we are too late anyway to get this into the new release. Some of the points mentioned are no blockers, but imo the code still needs some attention.
I know quite a few libraries of ours would be happy to see this fixed. As it is classified as a bugfix (correctly) it could still go into 17.11.x later on.
(In reply to Marcel de Rooy from comment #56) > It would be more consistent if we should use IsItemOnHoldAndFound in > IsAvailableForItemLevelRequest instead of a Koha::Holds->search. We actually > should adjust that routine. It does the same. Are we sure IsItemOnHoldAndFound does the same than Koha::Holds->search on W and T? What about a F status? Is it already used? Patch is coming for the others QA things
Created attachment 69724 [details] [review] Bug 4319 - [OPAC] allow to holds waiting items Test plan: - Checkout an item - Place hold on this item, - Return the item - Make sure the hold is waiting (found W) and AllowOnShelfHolds is not to 'Allow' - Check that the button "Place hold" appears in opac detail page of the biblio - do the samewith items/reserves in transit Changes on C4::Reserves::IsAvailableForItemLevelRequest Make sure this tests pass: - t/db_dependent/Reserves.t - t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t Rebased - 2017-12-12 - Alex Arnaud
Created attachment 69725 [details] [review] Bug 4319 - [QA fix] Create Koha::Biblio->hasItemswaitingOrInTransit
Created attachment 69728 [details] [review] Bug 4319 - [QA fix] Create Koha::Biblio->hasItemswaitingOrInTransit Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
(In reply to Jon Knight from comment #63) > Created attachment 69728 [details] [review] [review] > Bug 4319 - [QA fix] Create Koha::Biblio->hasItemswaitingOrInTransit > > Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk> Jon, For completeness: Did you intentionally sign off only the latest patch? Or is it meant as a complete signoff and did you forget to change status? Marcel
Ah, sorry it was a sign off from applying the bugs using git bz as normal, so I'd not noticed it only included one. Should be a complete sign off.
(In reply to Jon Knight from comment #66) > Ah, sorry it was a sign off from applying the bugs using git bz as normal, > so I'd not noticed it only included one. Should be a complete sign off. Thx. @Alex: I plan to resume QA on this one soon.
@Marcel: did you read my question on Comment 60 ?
QA: Looking here again
Created attachment 69815 [details] [review] Bug 4319: [OPAC] Allow holds on waiting/transit items Test plan: - Checkout an item - Place hold on this item, - Return the item - Make sure the hold is waiting (found W) and AllowOnShelfHolds is not to 'Allow' - Check that the button "Place hold" appears in opac detail page of the biblio - do the samewith items/reserves in transit Changes on C4::Reserves::IsAvailableForItemLevelRequest Make sure this tests pass: - t/db_dependent/Reserves.t - t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t Rebased - 2017-12-12 - Alex Arnaud Bug 4319 - [QA fix] Create Koha::Biblio->hasItemswaitingOrInTransit Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 69816 [details] [review] Bug 4319: (QA follow-up) Consistency in IsAvailableForItemLevelRequest [1] For consistency going back to IsItemOnHoldAndFound in this sub. This call is used in the on_shelf_holds == 2 case too. The routine will be refactored quite soon. Adding the else branch for on_shelf_holds == 0 for more clarity. [2] Removing the test for found==F in reserves. In Koha F is only used when the hold is filled and moved to oldreserves. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 69817 [details] [review] Bug 4319: (QA follow-up) Use ReservableItems in all scripts [1] Call CountItemsIssued or hasItemswaitingOrInTransit when needed only. [2] Add this logic to ISBD and MARC detail too, since they also use this include. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Confirming that Place hold now comes up if you have a waiting item and circulation rule == If any unavailable.
QA Comment: Thanks Alex for your perseverance. I am addressing your question from comment60 here too. Putting this on top of sec report 19319 which does some changes to Koha::Biblios->find in detail scripts. IsItemOnHoldAndFound is only used in CanBookBeRenewed and IsAvailableForItemLevelRequest; unfortunately it is not tested specifically. I think it should be moved to Koha::Item. And it should not call DBIx directly but use your search. The condition != undef is in practice the same as [W,T] since F is only used when the reserve is filled (finished) and moved to oldreserves. For the same reason I remove this test (using F in the reserves table does not reflect current use). Koha::Holds->find($hold->{reserve_id})->found('F')->store; $is = IsAvailableForItemLevelRequest( $item3, $borrower1); is( $is, 0, "Item is neither waiting nor in transit." ); See the followup patches. Rename hasItemswaitingOrInTransit => hasItemsWaitingOrInTransit ? No blocker. The POD statement is adjusted. Imo the POD should get some more attention. No blocker.
(In reply to Marcel de Rooy from comment #73) > QA Comment: > The POD statement is adjusted. Imo the POD should get some more attention. > No blocker. This is about the POD of IsAvailableForItemLevelRequest
Comment on attachment 69815 [details] [review] Bug 4319: [OPAC] Allow holds on waiting/transit items Review of attachment 69815 [details] [review]: ----------------------------------------------------------------- ::: Koha/Biblio.pm @@ +319,5 @@ > } > > +=head3 hasItemswaitingOrInTransit > + > +=cut Why are you cheating the QA script?
Additionally, do you think adding camelCase method's name in a module containing only snake_case is a good idea?
Also, when I see: @@ -1141,18 +1141,9 @@ sub IsAvailableForItemLevelRequest { } return $any_available ? 0 : 1; + } else { # on_shelf_holds == 0 "If any unavailable" (the description is rather cryptic and could still be improved) + return $item->{onloan} || IsItemOnHoldAndFound( $item->{itemnumber} ); } - - if ($item->{onloan}) { - return 1; - } - - if ( Koha::Holds->search({itemnumber => $item->{itemnumber}, - found => ['W', 'T']})->count ) { - return 1; - } Then: +sub hasItemswaitingOrInTransit { + my ( $self ) = @_; + + if ( Koha::Holds->search({ biblionumber => $self->id, + found => ['W', 'T'] })->count ) { + return 1; + } I am pretty sure something is wrong.
(In reply to Jonathan Druart from comment #77) > Also, when I see: > @@ -1141,18 +1141,9 @@ sub IsAvailableForItemLevelRequest { > > } > > return $any_available ? 0 : 1; > > + } else { # on_shelf_holds == 0 "If any unavailable" (the description is > rather cryptic and could still be improved) > + return $item->{onloan} || IsItemOnHoldAndFound( $item->{itemnumber} > ); > > } > - > - if ($item->{onloan}) { > > - return 1; > > - } > > - > - if ( Koha::Holds->search({itemnumber => $item->{itemnumber}, > > - found => ['W', 'T']})->count ) { > > - return 1; > > - } > > Then: > +sub hasItemswaitingOrInTransit { > + my ( $self ) = @_; > + > + if ( Koha::Holds->search({ biblionumber => $self->id, > + found => ['W', 'T'] })->count ) { > + return 1; > + } > > I am pretty sure something is wrong. It might be helpful to explain what you think is wrong. You are comparing pieces of code here that have different goals.
Created attachment 70457 [details] [review] Bug 4319 - [QA follow-up] Rename hasItemswaitingOrInTransit to has_items_waiting_or_intransit and update pod
(In reply to Jonathan Druart from comment #77) > > I am pretty sure something is wrong. As already asked by Marcel, can you give me more explanation about what is wrong? Otherwise, i renamed hasItemswaitingOrInTransit to has_items_waiting_or_intransit and and added pod description.
See bug 18856 comment 24. I do not have the time right now to deep into it but I am collecting ideas to keep the code smart. Marked as blocked by 18856.
WARNING: I have rebased bug 19301 on top of this one to avoid a rebase, but if this is not unblocked quickly I am going to rebase bug 19301 against master instead!
(In reply to Jonathan Druart from comment #82) > WARNING: I have rebased bug 19301 on top of this one to avoid a rebase, but > if this is not unblocked quickly I am going to rebase bug 19301 against > master instead! Here you are :) You blocked it yourself and you didnt respond to my question in comment78. Please do. Thanks.
As referred to on IRC by Jonathan: === Looking at bug 4319: We also have C4::Reserves::IsItemOnHoldAndFound that does exactly the same thing. So maybe "is_cancelable" is too specific? What could we do to keep this code simple, understandable and consistent? === I would personally not block this patch for that reason. We use IsItemOnHoldAndFound here to reduce yet more code to check the same; but this report is not about that sub. I would prefer to move the sub to Koha::Holds too, but that could well be done a its own report. If 4319 gets pushed and the other one follows, a new report can merge them. Just my thoughts.
Pushed to master for 18.05, thanks to everybody involved!
Awesome work all! Pushed to stable for 17.11.03