Bug 4319 - waiting and in transit items cannot be reserved
Summary: waiting and in transit items cannot be reserved
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P2 major (vote)
Assignee: Alex Arnaud
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 3012 11321 14935 (view as bug list)
Depends on: 19319
Blocks: 19301
  Show dependency treegraph
 
Reported: 2010-03-16 04:10 UTC by Nahuel Angelinetti
Modified: 2018-06-04 20:15 UTC (History)
18 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Proposed patch (1.44 KB, patch)
2011-04-06 12:37 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Updated patch (1.64 KB, patch)
2011-05-13 08:04 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Screenshot of the problem in master (9.11 KB, image/png)
2011-05-13 20:09 UTC, Owen Leonard
Details
Proposed patch (with syspref) (1.60 KB, patch)
2011-06-01 08:46 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Signed-off patch (1.71 KB, patch)
2011-06-01 13:11 UTC, Owen Leonard
Details | Diff | Splinter Review
can't place a hold on a waiting item (121.19 KB, image/jpeg)
2013-03-28 12:49 UTC, Nicole C. Engard
Details
Bug 4319 - [OPAC] allow to holds waiting items (2.27 KB, patch)
2017-10-03 14:56 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 4319 - [OPAC] allow to holds waiting items (2.68 KB, patch)
2017-10-04 03:30 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 4319 - [OPAC] allow to holds items in transit (1.93 KB, patch)
2017-10-05 14:06 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 4319 followup - [OPAC] allow to holds items in transit (3.40 KB, patch)
2017-10-06 13:03 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 4319 followup - [OPAC] allow to holds items in transit (3.45 KB, patch)
2017-10-11 18:47 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 4319 followup - [OPAC] allow to holds items in transit (4.32 KB, patch)
2017-10-17 07:07 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 4319 - [OPAC] allow to holds waiting items (2.73 KB, patch)
2017-10-17 08:21 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 4319 - [OPAC] allow to holds items in transit (1.99 KB, patch)
2017-10-17 08:21 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 4319 followup - [OPAC] allow to holds items in transit (4.37 KB, patch)
2017-10-17 08:21 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 4319 - [OPAC] allow to holds waiting items (5.50 KB, patch)
2017-10-25 12:38 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 4319 - [OPAC] allow to holds waiting items (5.42 KB, patch)
2017-10-27 18:24 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 4319 - [OPAC] allow to holds waiting items (5.47 KB, patch)
2017-10-27 18:40 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 4319 - [OPAC] allow to holds waiting items (5.29 KB, patch)
2017-12-12 09:16 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 4319 - [QA fix] Create Koha::Biblio->hasItemswaitingOrInTransit (5.52 KB, patch)
2017-12-12 09:16 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 4319 - [QA fix] Create Koha::Biblio->hasItemswaitingOrInTransit (5.57 KB, patch)
2017-12-12 10:35 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 4319: [OPAC] Allow holds on waiting/transit items (8.59 KB, patch)
2017-12-15 10:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 4319: (QA follow-up) Consistency in IsAvailableForItemLevelRequest (2.40 KB, patch)
2017-12-15 10:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 4319: (QA follow-up) Use ReservableItems in all scripts (2.94 KB, patch)
2017-12-15 10:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 4319 - [QA follow-up] Rename hasItemswaitingOrInTransit to has_items_waiting_or_intransit and update pod (3.93 KB, patch)
2018-01-12 09:52 UTC, Alex Arnaud
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:25:46 UTC


---- 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.

Comment 1 Nicole C. Engard 2010-06-29 15:12:48 UTC
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.
Comment 2 Owen Leonard 2010-07-09 20:00:13 UTC
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.
Comment 3 Owen Leonard 2010-07-09 20:01:48 UTC
See also Bug 3012
Comment 4 Fridolin SOMERS 2011-04-06 12:34:11 UTC
*** Bug 3012 has been marked as a duplicate of this bug. ***
Comment 5 Fridolin SOMERS 2011-04-06 12:37:01 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2011-05-13 02:15:10 UTC
 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".
Comment 7 Fridolin SOMERS 2011-05-13 08:04:03 UTC Comment hidden (obsolete)
Comment 8 Nicole C. Engard 2011-05-13 19:56:56 UTC
Is this still an issue? I can't seem to replicate this problem.
Comment 9 Owen Leonard 2011-05-13 20:09:53 UTC
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."
Comment 10 Nicole C. Engard 2011-05-13 20:23:37 UTC
Oh! The search results, okay, signed off.
Comment 11 Owen Leonard 2011-05-31 19:50:38 UTC
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.
Comment 12 Fridolin SOMERS 2011-06-01 08:46:40 UTC Comment hidden (obsolete)
Comment 13 Owen Leonard 2011-06-01 13:11:02 UTC Comment hidden (obsolete)
Comment 14 Chris Cormack 2011-06-12 23:49:37 UTC
I have run into this problem at a client site, this patch solved their problem, pushing to master
Comment 15 Owen Leonard 2011-10-13 20:20:35 UTC
A waiting item can be placed on hold both in the OPAC and staff client. Marking resolved.
Comment 16 Nicole C. Engard 2013-03-28 12:49:40 UTC
Created attachment 16965 [details]
can't place a hold on a waiting item

This problem is back in Koha 3.10.3.
Comment 17 Fridolin SOMERS 2013-08-16 11:00:23 UTC
I think this is solved by Bug 8975
Comment 18 Katrin Fischer 2014-08-06 14:04:20 UTC
We have a report for this bug from a library on 3.12.9 currently, I will retest on master.
Comment 19 Katrin Fischer 2014-08-06 14:05:15 UTC
*** Bug 11321 has been marked as a duplicate of this bug. ***
Comment 20 Katrin Fischer 2014-08-06 14:08:48 UTC
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.
Comment 21 Katrin Fischer 2014-08-11 09:59:06 UTC
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?
Comment 22 Nicole C. Engard 2014-10-06 14:23:01 UTC
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
Comment 23 Katrin Fischer 2014-10-09 21:23:50 UTC
Thx for the comment Nicole!
Comment 24 Koha Team Lyon 3 2015-09-22 15:35:17 UTC
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
Comment 25 Koha Team Lyon 3 2015-10-01 14:22:48 UTC

*** This bug has been marked as a duplicate of bug 14935 ***
Comment 26 Katrin Fischer 2016-01-15 15:09:48 UTC
*** Bug 14935 has been marked as a duplicate of this bug. ***
Comment 27 Marjorie Barry-Vila 2017-04-21 14:44:46 UTC
Still valid in 16.11?
anyone tested it?

Marjorie
Comment 28 Alex Arnaud 2017-10-03 14:56:06 UTC
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
Comment 29 Alex Buckley 2017-10-04 03:30:12 UTC
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>
Comment 30 Katrin Fischer 2017-10-04 05:49:21 UTC
Glad to see this one moving!
Comment 31 Koha Team Lyon 3 2017-10-05 09:17:33 UTC
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
Comment 32 Katrin Fischer 2017-10-05 09:41:55 UTC
Good catch, I agree that "In transit" should also be treated as unavailable.
Comment 33 Alex Arnaud 2017-10-05 14:06:19 UTC
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
Comment 34 Koha Team Lyon 3 2017-10-05 15:05:53 UTC
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
Comment 35 Alex Arnaud 2017-10-06 13:03:00 UTC
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
Comment 36 Jon Knight 2017-10-11 18:47:38 UTC
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>
Comment 37 Koha Team Lyon 3 2017-10-12 12:23:18 UTC
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"
Comment 38 Alex Arnaud 2017-10-13 09:10:21 UTC
(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?
Comment 39 Koha Team Lyon 3 2017-10-13 15:25:02 UTC
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
Comment 40 Alex Arnaud 2017-10-17 07:07:37 UTC
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
Comment 41 Alex Arnaud 2017-10-17 07:09:36 UTC
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.
Comment 42 sandboxes@biblibre.com 2017-10-17 08:20:10 UTC
Patch tested with a sandbox, by Sonia BOUIS <sonia.bouis@univl-yon3.fr>
Comment 43 sandboxes@biblibre.com 2017-10-17 08:21:00 UTC
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>
Comment 44 sandboxes@biblibre.com 2017-10-17 08:21:04 UTC
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>
Comment 45 sandboxes@biblibre.com 2017-10-17 08:21:08 UTC
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>
Comment 46 Koha Team Lyon 3 2017-10-17 08:36:54 UTC
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
Comment 47 Marcel de Rooy 2017-10-20 08:10:37 UTC
QA: Looking here now
Comment 48 Marcel de Rooy 2017-10-20 08:22:42 UTC
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
Comment 49 Alex Arnaud 2017-10-25 12:38:18 UTC
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
Comment 50 Alex Arnaud 2017-10-25 12:41:45 UTC
(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
Comment 51 Charles Farmer 2017-10-27 18:23:04 UTC
Patch didn't apply smoothly: the template variable OPACShowBarcode disappeared from opac/opac-detail.pl.

Resubmitting a corrected version.
Comment 52 Charles Farmer 2017-10-27 18:24:00 UTC Comment hidden (obsolete)
Comment 53 Charles Farmer 2017-10-27 18:40:38 UTC
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>
Comment 54 Marcel de Rooy 2017-11-24 10:03:37 UTC
QA: Looking here now
Comment 55 Marcel de Rooy 2017-11-24 10:35:13 UTC
+    OPACShowBarcode => C4::Context->preference("OPACShowBarcode")

Regression from a rebase ?
Comment 56 Marcel de Rooy 2017-11-24 11:38:04 UTC
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.
Comment 57 Marcel de Rooy 2017-11-27 10:59:47 UTC
Conflicts now with just pushed 18547
Comment 58 Marcel de Rooy 2017-11-27 11:43:45 UTC
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.
Comment 59 Katrin Fischer 2017-11-27 11:46:28 UTC
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.
Comment 60 Alex Arnaud 2017-12-12 09:04:18 UTC
(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
Comment 61 Alex Arnaud 2017-12-12 09:16:11 UTC
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
Comment 62 Alex Arnaud 2017-12-12 09:16:23 UTC
Created attachment 69725 [details] [review]
Bug 4319 - [QA fix] Create Koha::Biblio->hasItemswaitingOrInTransit
Comment 63 Jon Knight 2017-12-12 10:35:36 UTC
Created attachment 69728 [details] [review]
Bug 4319 - [QA fix] Create Koha::Biblio->hasItemswaitingOrInTransit

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 64 Marcel de Rooy 2017-12-12 12:53:34 UTC
(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
Comment 65 Jon Knight 2017-12-12 13:53:24 UTC
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.
Comment 66 Jon Knight 2017-12-12 13:53:56 UTC
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.
Comment 67 Marcel de Rooy 2017-12-12 13:56:27 UTC
(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.
Comment 68 Alex Arnaud 2017-12-12 14:03:17 UTC
@Marcel: did you read my question on Comment 60 ?
Comment 69 Marcel de Rooy 2017-12-15 08:17:06 UTC
QA: Looking here again
Comment 70 Marcel de Rooy 2017-12-15 10:26:02 UTC
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>
Comment 71 Marcel de Rooy 2017-12-15 10:26:08 UTC
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>
Comment 72 Marcel de Rooy 2017-12-15 10:26:14 UTC
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.
Comment 73 Marcel de Rooy 2017-12-15 10:26:39 UTC
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.
Comment 74 Marcel de Rooy 2017-12-15 10:28:17 UTC
(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 75 Jonathan Druart 2018-01-09 19:15:33 UTC
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?
Comment 76 Jonathan Druart 2018-01-09 19:17:42 UTC
Additionally, do you think adding camelCase method's name in a module containing only snake_case is a good idea?
Comment 77 Jonathan Druart 2018-01-09 19:19:33 UTC
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.
Comment 78 Marcel de Rooy 2018-01-10 12:13:05 UTC
(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.
Comment 79 Alex Arnaud 2018-01-12 09:52:30 UTC
Created attachment 70457 [details] [review]
Bug 4319 - [QA follow-up] Rename hasItemswaitingOrInTransit to has_items_waiting_or_intransit and update pod
Comment 80 Alex Arnaud 2018-01-12 09:55:31 UTC
(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.
Comment 81 Jonathan Druart 2018-01-15 15:13:34 UTC
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.
Comment 82 Jonathan Druart 2018-02-09 19:45:52 UTC
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!
Comment 83 Marcel de Rooy 2018-02-12 07:39:29 UTC
(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.
Comment 84 Marcel de Rooy 2018-02-12 20:50:00 UTC
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.
Comment 85 Jonathan Druart 2018-02-13 17:18:08 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 86 Nick Clemens 2018-02-26 11:41:17 UTC
Awesome work all! Pushed to stable for 17.11.03