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.
Created attachment 14004 [details] [review] Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off Koha documenation suggests that NOT_LOAN values less than 0 should be hold-able. This patch enables that. From the manual: Negative number values will still allow holds (use for on order statuses for example) where as positive numbers will not allow holds or checkouts
Created attachment 14152 [details] [review] Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off Koha documenation suggests that NOT_LOAN values less than 0 should be hold-able. This patch enables that. From the manual: Negative number values will still allow holds (use for on order statuses for example) where as positive numbers will not allow holds or checkouts
Created attachment 14158 [details] [review] Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off Koha documenation suggests that NOT_LOAN values less than 0 should be hold-able. This patch enables that. From the manual: Negative number values will still allow holds (use for on order statuses for example) where as positive numbers will not allow holds or checkouts Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Works as expected
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.
Created attachment 14390 [details] [review] Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off Koha documenation suggests that NOT_LOAN values less than 0 should be hold-able. This patch enables that. From the manual: Negative number values will still allow holds (use for on order statuses for example) where as positive numbers will not allow holds or checkouts Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Works as expected
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.
The revised patch needs to be QAed.
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.
Created attachment 14451 [details] [review] Bug 6918 - can't place holds on 'on order' items with AllowOnShelfHolds off
Jonathan, here is a variation that keeps the code from being repeated multiple times. It also takes care of your issues in comment 8.
Needs a test plan.
Created attachment 15077 [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
(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?
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?
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."
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
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.
Patch applied cleanly, go forth and signoff
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
Hi Kyle, could you take a look at Srdjan's comment?
(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.
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?
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.
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.
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
Patch rebased and rewritten, please test. Srdjan's concern is not valid IMO.
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
Simon, make sure your branch is up-to-date with origin/master
I did a reset --hard on origin/master and still have the same error than Simon..
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
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 ") )"
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
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.
(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.
Ordered should be -1 if the sample data was used.
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
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
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
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.
(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!
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
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
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.
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
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
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>
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
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.
Created attachment 105390 [details] [review] Bug 6918: Add syspref
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
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>
Created attachment 105474 [details] [review] Bug 6918: Add syspref Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
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?
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...).
*** This bug has been marked as a duplicate of bug 27058 ***