Bug 6918 - Can't place holds on 'on order' items with AllowOnShelfHolds off
Summary: Can't place holds on 'on order' items with AllowOnShelfHolds off
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P3 normal (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact: Bugs List
URL:
Whiteboard:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2011-09-26 12:33 UTC by Nicole C. Engard
Modified: 2018-05-15 12:26 UTC (History)
20 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this: 2013-10-25 00:00:00
Who signed the patch off:
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 (1.11 KB, patch)
2012-12-11 16:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off (2.60 KB, patch)
2012-12-14 19:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off (2.67 KB, patch)
2012-12-15 09:22 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off (2.73 KB, patch)
2013-01-03 13:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off (5.20 KB, patch)
2013-01-07 14:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off (5.37 KB, patch)
2013-02-05 20:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off (5.46 KB, patch)
2013-06-17 18:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off (5.13 KB, patch)
2017-11-02 17:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off (5.13 KB, patch)
2017-11-03 19:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off (5.11 KB, patch)
2018-01-02 18:30 UTC, Kyle M Hall
Details | Diff | Splinter Review

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