Bug 6918

Summary: Can't place holds on 'on order' items with AllowOnShelfHolds off
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: CirculationAssignee: Kyle M Hall <kyle.m.hall>
Status: RESOLVED DUPLICATE QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P3 CC: aleisha, alexbuckley, andrewfh, cbrannon, dominic.pichette, gitbot, gmcharlt, janet.mcgowan, jean-manuel.broust, jonathan.druart+koha, joonas.kylmala, jzairo, katrin.fischer, kyle, m.de.rooy, marjorie.barry-vila, martin.renvoize, mirko, niamh.walker-headon, patrick.robitaille, simon.pouchol, srdjan, veron, zoebennett1308
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27058
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918: Can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918: Can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918: Can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918: Can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918: Can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918: Add syspref
Bug 6918: Can't place holds on 'on order' items with AllowOnShelfHolds off
Bug 6918: Add syspref

Description Nicole C. Engard 2011-09-26 12:33:55 UTC
I don't think this was always the case (but I might be wrong).  If AllowOnShelfHolds is set to 'don't allow' then patrons cannot place holds on items that are marked 'on order' (not for loan=-1).  Technically items on order are not on the shelf and should be allowed to be placed on hold.  

This may require a preference for people to say whether they want to allow holds on 'on order' items and/or what the not for loan value is for 'on order' since there is nothing stopping the user from changing the value from -1 to something else.

The easy fix is just to make the system allow holds on items with -1 for notforloan like I think it was before.
Comment 1 Kyle M Hall 2012-12-11 16:12:16 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-12-14 19:11:54 UTC Comment hidden (obsolete)
Comment 3 Mirko Tietgen 2012-12-15 09:22:53 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2012-12-26 11:34:48 UTC
QA Comments:

I am not sure at all but the check on the notforloan value should not be made in the CanItemBeReserved routine ? Is not it more relevant ?

If not, your patch has to be resubmitted for a little modification.

The test in IsAvailableForItemLevelRequest is:

  return ($available_per_item and ($item->{onloan} or GetReserveStatus($itemnumber) eq "W" or $item->{notforloan} < 0));

it will be optimized if you change the order, as:

  return ($available_per_item and ($item->{onloan} or $item->{notforloan} < 0 or GetReserveStatus($itemnumber) eq "W" ));

Marked as Failed QA.
Comment 5 Kyle M Hall 2013-01-03 13:06:51 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2013-01-03 13:12:07 UTC
That was my first instinct as well, but the way the scripts work, it didn't 'fit' in that function. In fact, that function doesn't deal with items statuses at all! IsAvailableForItemLevelRequest actually deals with the status so that was where the changes needed to be made.

I have implemented your optimization for the return value.

Kyle

(In reply to comment #4)
> QA Comments:
> 
> I am not sure at all but the check on the notforloan value should not be
> made in the CanItemBeReserved routine ? Is not it more relevant ?
> 
> If not, your patch has to be resubmitted for a little modification.
> 
> The test in IsAvailableForItemLevelRequest is:
> 
>   return ($available_per_item and ($item->{onloan} or
> GetReserveStatus($itemnumber) eq "W" or $item->{notforloan} < 0));
> 
> it will be optimized if you change the order, as:
> 
>   return ($available_per_item and ($item->{onloan} or $item->{notforloan} <
> 0 or GetReserveStatus($itemnumber) eq "W" ));
> 
> Marked as Failed QA.
Comment 7 Jared Camins-Esakov 2013-01-04 13:23:43 UTC
The revised patch needs to be QAed.
Comment 8 Jonathan Druart 2013-01-04 14:52:34 UTC
QA Comment:

If I correctly understand your patch, the increment of the count items issued have to be made in opac/opac-ISBDdetail.pl and opac/opac-MARCdetail.pl too.
Comment 9 Kyle M Hall 2013-01-07 14:35:47 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2013-01-07 14:39:24 UTC
Jonathan, here is a variation that keeps the code from being repeated multiple times. It also takes care of your issues in comment 8.
Comment 11 Owen Leonard 2013-02-05 20:26:53 UTC
Needs a test plan.
Comment 12 Kyle M Hall 2013-02-05 20:45:34 UTC Comment hidden (obsolete)
Comment 13 Owen Leonard 2013-02-05 21:17:08 UTC
(In reply to comment #12)
> 4) Attempt to place a hold on the item, you should now be able to do so

I see that this patch affects several scripts. Should one test placing a hold from multiple pages or in multiple ways?
Comment 14 Kyle M Hall 2013-02-05 21:22:54 UTC
I changed the prototype for CountItemsIssued, so I had to update all calls to it. Still, it wouldn't hurt to test in multiple ways.

Kyle

(In reply to comment #13)
> (In reply to comment #12)
> > 4) Attempt to place a hold on the item, you should now be able to do so
> 
> I see that this patch affects several scripts. Should one test placing a
> hold from multiple pages or in multiple ways?
Comment 15 Owen Leonard 2013-02-07 20:15:12 UTC
I'm guessing from the diff that I need to test these pages:

opac-ISBDdetail.pl
opac-MARCdetail.pl
opac-detail.pl
opac-search.pl
opac-shelves.pl

It seems to be working correctly on opac-ISBDdetail.pl, opac-MARCdetail.pl, and opac-detail.pl. I see problems with opac-search.pl and opac-shelves.pl: the "place hold" link which should appear within the search results/list contents does not appear--that is, within the table of results, on the line labeled "Actions."
Comment 16 Kyle M Hall 2013-06-17 18:12:11 UTC
Created attachment 19087 [details] [review]
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off

Test Plan:
1) Apply patch
2) Turn off AllowOnShelfHolds
3) Create a bib with one item, mark the item as 'on order'
4) Attempt to place a hold on the item, you should now be able to do so
Comment 17 Srdjan Jankovic 2013-07-17 03:18:52 UTC
I don't think adding a flag warrants changing CountItemsIssued() interface that much, ie I believe in this case CountItemsIssued($biblionumbe, $include_on_order) is a better solution.
A test would be nice to have too, but not that fussed.
Comment 18 I'm just a bot 2013-09-29 05:28:54 UTC
Patch applied cleanly, go forth and signoff
Comment 19 I'm just a bot 2013-10-24 19:35:46 UTC
Patch applied cleanly, go forth and signoff
Comment 20 Christopher Brannon 2013-12-11 00:05:45 UTC
Kyle,
This patch seems to fix more than just the ability to place a hold on an On Order item.  It appears that prior to the patch, one could not place a hold any any available item if at least the first item was setup as On Order and theAllowOnShelfHolds is set to Don't Allow.  So, it seems you fixed more than you stated.  Good work.

Christopher
Comment 21 Katrin Fischer 2013-12-29 22:08:47 UTC
Hi Kyle, could you take a look at Srdjan's comment?
Comment 22 Christopher Brannon 2014-06-19 21:52:50 UTC
(In reply to Katrin Fischer from comment #21)
> Hi Kyle, could you take a look at Srdjan's comment?

I'm moving this to discussion until Srdjan's comment is addressed.
Comment 23 Katrin Fischer 2017-08-26 21:53:02 UTC
This is still a desirable feature, but the existing patch is quite old and possibly needs a more or less complete rewrite:

Falling back to patching base and 3-way merge...
Auto-merging opac/opac-search.pl
Auto-merging opac/opac-detail.pl
CONFLICT (content): Merge conflict in opac/opac-detail.pl
Auto-merging opac/opac-MARCdetail.pl
CONFLICT (content): Merge conflict in opac/opac-MARCdetail.pl
Auto-merging opac/opac-ISBDdetail.pl
CONFLICT (content): Merge conflict in opac/opac-ISBDdetail.pl
CONFLICT (modify/delete): C4/VirtualShelves/Page.pm deleted in HEAD and modified in Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off. Version Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off of C4/VirtualShelves/Page.pm left in tree.
Auto-merging C4/Reserves.pm
CONFLICT (content): Merge conflict in C4/Reserves.pm
Auto-merging C4/Biblio.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
...

Can we move this back to NEW?
Comment 24 Jessie Zairo 2017-08-28 14:49:46 UTC
If a library sets their On Shelf Holds to "If all unavailable" or "If any unavailable" patrons cannot place holds on items that are marked 'on order' (not for loan=-1). 

Libraries would like to have the option to make the system allow holds on items with -1 for notforloan so patrons can place holds on "ordered" items.
Comment 25 Katrin Fischer 2017-09-05 06:20:47 UTC
I seem to remember that I had tested it at some point and the system had allowed holds on the negative notforloan values. Wondering now if it something changed.
Comment 26 Jonathan Druart 2017-11-02 17:15:01 UTC
Created attachment 68898 [details] [review]
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off

Test Plan:
1) Apply patch
2) Turn off AllowOnShelfHolds
3) Create a bib with one item, mark the item as 'on order'
4) Attempt to place a hold on the item, you should now be able to do so
Comment 27 Jonathan Druart 2017-11-02 17:16:32 UTC
Patch rebased and rewritten, please test.

Srdjan's concern is not valid IMO.
Comment 28 Simon Pouchol 2017-11-03 16:13:36 UTC
Hey Jonathan,
It seems to me the patch isn't applying :
Applying: Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
fatal: sha1 information is lacking or useless (C4/Biblio.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
Comment 29 Jonathan Druart 2017-11-03 16:45:21 UTC
Simon, make sure your branch is up-to-date with origin/master
Comment 30 Dominic Pichette 2017-11-03 18:55:32 UTC
I did a reset --hard on origin/master and still have the same error than Simon..
Comment 31 Jonathan Druart 2017-11-03 19:26:22 UTC
Created attachment 68934 [details] [review]
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off

Test Plan:
1) Apply patch
2) Turn off AllowOnShelfHolds
3) Create a bib with one item, mark the item as 'on order'
4) Attempt to place a hold on the item, you should now be able to do so
Comment 32 Alex Buckley 2017-11-05 07:03:59 UTC
There's a unnecessary bracket in line 677 of opac-search.pl:

syntax error at /home/vagrant/kohaclone/opac/opac-search.pl line 677, near ") )"
Comment 33 Kyle M Hall 2018-01-02 18:30:12 UTC
Created attachment 70232 [details] [review]
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off

Test Plan:
1) Apply patch
2) Turn off AllowOnShelfHolds
3) Create a bib with one item, mark the item as 'on order'
4) Attempt to place a hold on the item, you should now be able to do so
Comment 34 Zoe Bennett 2018-01-16 03:45:31 UTC
I tested the patch by setting On Shelf Holds to "If all unavailable" and then "If any unavailable". I set 7-Not for Loan to 'ordered' and was still unable to place a hold on the order.
Comment 35 Kyle M Hall 2018-01-16 12:30:13 UTC
(In reply to Zoe Bennett from comment #34)
> I tested the patch by setting On Shelf Holds to "If all unavailable" and
> then "If any unavailable". I set 7-Not for Loan to 'ordered' and was still
> unable to place a hold on the order.

What is the value of your "On order" status? It needs to be a negative number. In Koha this indicates that the item will be available for loan in the future.
Comment 36 Katrin Fischer 2018-01-16 12:50:19 UTC
Ordered should be -1 if the sample data was used.
Comment 37 Kyle M Hall 2020-04-15 12:50:48 UTC
Created attachment 102998 [details] [review]
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off

Test Plan:
1) Apply patch
2) Turn off AllowOnShelfHolds
3) Create a bib with one item, mark the item as 'on order'
4) Attempt to place a hold on the item, you should now be able to do so
Comment 38 Andrew Fuerste-Henry 2020-04-15 14:52:47 UTC
Just gave this a try and the hold was still blocked. Elaborating a bit on the test plan:
- apply patch
- set On Shelf Holds to If Any Unavailable
- verify you have a NotForLoan authorised value with a negative number for Ordered. The default data set uses -1
- grab a bib with two items, neither checked out, one set to NotForLoan=0 the other Notforloan=-1
- attempt to place hold
- should be blocked from holding item 1, allowed to hold item 2
Comment 39 Kyle M Hall 2020-04-15 15:25:45 UTC
Created attachment 103016 [details] [review]
Bug 6918: Can't place holds on 'on order' items with AllowOnShelfHolds off

Test Plan:
1) Apply patch
2) Turn off AllowOnShelfHolds
3) Create a bib with one item, mark the item as 'on order'
4) Attempt to place a hold on the item, you should now be able to do so
Comment 40 Andrew Fuerste-Henry 2020-04-15 15:44:22 UTC
With On Shelf Holds set to If Any Unavailable, I'm seeing the correct behavior. I can place holds on items with a negative NotForLoan value but not on items with positive or zero NotForLoan values. Yay!

However, if I change my rule to If All Unavailable and set all of the items on my bib to NotForLoan=-1, I am not allowed to place a hold. I should be allowed to do so.
Comment 41 Kyle M Hall 2020-04-15 16:25:54 UTC
(In reply to Andrew Fuerste-Henry from comment #40)
> With On Shelf Holds set to If Any Unavailable, I'm seeing the correct
> behavior. I can place holds on items with a negative NotForLoan value but
> not on items with positive or zero NotForLoan values. Yay!
> 
> However, if I change my rule to If All Unavailable and set all of the items
> on my bib to NotForLoan=-1, I am not allowed to place a hold. I should be
> allowed to do so.

I wasn't sure about that scenario. Thanks for the clarity!
Comment 42 Kyle M Hall 2020-04-15 16:26:16 UTC
Created attachment 103020 [details] [review]
Bug 6918: Can't place holds on 'on order' items with AllowOnShelfHolds off

Test Plan:
1) Apply patch
2) Turn off AllowOnShelfHolds
3) Create a bib with one item, mark the item as 'on order'
4) Attempt to place a hold on the item, you should now be able to do so
Comment 43 Kyle M Hall 2020-04-15 16:29:53 UTC
Created attachment 103021 [details] [review]
Bug 6918: Can't place holds on 'on order' items with AllowOnShelfHolds off

Test Plan:
1) Apply patch
2) Turn off AllowOnShelfHolds
3) Create a bib with one item, mark the item as 'on order'
4) Attempt to place a hold on the item, you should now be able to do so
Comment 44 Andrew Fuerste-Henry 2020-04-15 21:32:12 UTC
If All Unavailable still isn't quite right. Backing up:
If On Shelf Holds is set to Yes, you should be able to place holds on all items, regardless of whehter or not they're on order.

If On Shelf Holds is set to If Any Unavailable, you should be able to place holds on items marked On Order, but not on items where notforloan=0.

If On Shelf Holds is set to If All Unavailable, you should be able to place holds on items marked On Order only if *all* items on the bib have notforloan!=0, are checked out, or are otherwise not available for checkout. So if I have 1 item that's currently checked out and one that marked On Order, I should be able to place a hold on either.
Comment 45 Kyle M Hall 2020-04-17 17:22:03 UTC
Created attachment 103175 [details] [review]
Bug 6918: Can't place holds on 'on order' items with AllowOnShelfHolds off

Test Plan:
1) Apply patch
2) Turn off AllowOnShelfHolds
3) Create a bib with one item, mark the item as 'on order'
4) Attempt to place a hold on the item, you should now be able to do so
Comment 46 Andrew Fuerste-Henry 2020-04-17 18:22:11 UTC
Test plan:
1 find a record with two items
2 set one item to On Order (or any other negative Not For Loan value)
3 set circ rule to On Shelf Holds = Yes
4 confirm bib and either item can be held
5 set circ rule to On Shelf Holds = In Any Unavailable
6 confirm bib and on order item can be held, available item cannot be held
7 set circ rule to On Shelf Holds = If All Unavailable
8 confirm bib and both items cannot be held
9 check out the item with notforloan=0
10 confirm bib and all items now able to be held
Comment 47 Andrew Fuerste-Henry 2020-04-17 18:23:04 UTC
Created attachment 103179 [details] [review]
Bug 6918: Can't place holds on 'on order' items with AllowOnShelfHolds off

Test Plan:
1) Apply patch
2) Turn off AllowOnShelfHolds
3) Create a bib with one item, mark the item as 'on order'
4) Attempt to place a hold on the item, you should now be able to do so

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 48 Marcel de Rooy 2020-05-01 10:00:19 UTC
Not sure this is a In discussion or a Failed QA.
But the report dates from 2011 and the patch from 2013, so there must be a reason why this took so long.
The first comments says "This may require a pref" too.
Since we are used now to the current behavior since 2011/2013, I think we cannot revert behavior without a pref.

Changing status
Comment 49 Katrin Fischer 2020-05-22 12:28:40 UTC
Personally I think this is a bug that hasn't been fixed in an awfully long time - maybe because use of "AllowOnShelfHolds = yes" is the more common choice.

But I'd be ok with a preference if this means we can move this forward.
Comment 50 Kyle M Hall 2020-05-27 11:56:44 UTC
Created attachment 105390 [details] [review]
Bug 6918: Add syspref
Comment 51 Andrew Fuerste-Henry 2020-06-01 12:25:00 UTC
Test plan:

1: Apply patch, updatedatabase, restart all
2: Set AllowOnOrderOnShelfHolds to Allow
3: Set relevant circ rule to allow onshelf holds If Any Unavailable
4: Find a bib with 2 items, one available and one on-order
5: Confirm that you can place hold on bib or on-order item
5: set circ rule to On Shelf Holds = If All Unavailable
6: confirm no holds can be placed
7: set circ rule to On Shelf Holds = Yes
8: confirm you can place hold on bib or either item
9 Set AllowOnOrderOnShelfHolds to Don't Allow
10: confirm circ rule still set to On Shelf Holds = Yes
11: confirm you can place hold on bib or either item
12: Set relevant circ rule to allow onshelf holds If Any Unavailable
13: Confirm no holds can be placed
14: set circ rule to On Shelf Holds = If All Unavailable
15: confirm no holds can be placed
Comment 52 Andrew Fuerste-Henry 2020-06-01 12:25:49 UTC
Created attachment 105473 [details] [review]
Bug 6918: Can't place holds on 'on order' items with AllowOnShelfHolds off

Test Plan:
1) Apply patch
2) Turn off AllowOnShelfHolds
3) Create a bib with one item, mark the item as 'on order'
4) Attempt to place a hold on the item, you should now be able to do so

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 53 Andrew Fuerste-Henry 2020-06-01 12:25:54 UTC
Created attachment 105474 [details] [review]
Bug 6918: Add syspref

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 54 Martin Renvoize 2020-06-08 12:06:48 UTC
Sorry.. but I'm not seeing any Unit Tests... so failing QA for that.

As a side note.. I think I would have preferred to see a new method 'CountItemsOrdered' as opposed to adding logic into 'CountItemsIssued' as it makes the methods purpose less well defined/clear.  It could, of course, be entirely differently named or in the Koha::Object world perhaps be a chainable filter on an Koha::Items's set?
Comment 55 Joonas Kylmälä 2020-11-26 09:00:27 UTC
Hi,

could you please see bug 27058. I had worked on the same issue before and didn't notice this bug report until now. I have a unit test there too but the bug report is only about on shelf holds = 2 / If all unavailable. There is no syspref for it because the syspref is needed only for the "If any unavailable" case which is more complex. For If all unavailable case the current behaviour is *definitely buggy* because it breaks holds when there is a ordered item, leading to a situation where there is no item available for the patron to checkout and no way for the library staff or patron themselves to place a hold to the book. This leads to a situation where all Koha libraries using If all unavailable need to keep a separate list of holds in "paper notes" or just say to the patron "sorry, come back later" which could be years for popular books. If you don't have any items available for checkout it is clear that a hold must be able to be placed if it it normally would be the case without ordered items (because you can't checkout ordered items because they don't exists yet).

I have spend multiple hours thinking the scenarios carefully through, understanding the incorrectly grouped and named functions in C4/Reserves.pm, discussed this problem with 5 different libraries so I ask you also to read the code carefully when reviewing bug 27058. One important thing is that ItemsAnyAvailableAndNotRestricted() is actually two different functions: 1) Do basic checkout availability checks that are not specific to the patron 2) Do basic reservability checks to accomodate hold groups or rules regarding from which branch patron can reserve book (CanItemBeReserved()). So please don't let the incorrectly named function names to stop fixing this bug.

There are a lot of other issues around IsAvailableForItemLevelRequest and CanItemBeReserved functions but please let's keep the scope on this particular issue, and on If all unavailable case only in bug 27058. In Bug 27032 I start to refactor these hard-to-understand pieces of code (and yes CanBookBeRenewed has ties to ItemsAnyAvailableAndNotRestricted...).
Comment 56 Andrew Fuerste-Henry 2021-03-14 20:04:29 UTC

*** This bug has been marked as a duplicate of bug 27058 ***