The holds to pull list (pendingreserves.pl) is mistakenly showing titles where there are no available items and telling staff to pull items with negative notforloan statuses that should keep them off the list. To recreate: - have a bib with 2 items, both holdable. Give one of these items a notforloan status with a negative value (like Ordered in the testing docker data). - place a title-level hold on your bib - confirm your bib is showing on Holds to Pull as expected, prompting staff to pull the item that does not have a not for loan status (correct!) - take the item that does not have a not for loan status and check it out to a different patron, make sure not to fill or cancel your hold - reload Holds to Pull, see your title is still listed and now Koha is directing staff to the item with a notforloan status In building this list, we start by getting only the bibs that have at least one holdable item, but then we neglect to actually exclude the non-circulating items from the final output.
*** Bug 30322 has been marked as a duplicate of this bug. ***
I think this bug may be due to Bug 29786 which removed the notforloan check in Koha/Holds.pm ?
Created attachment 135074 [details] [review] Bug 30730: Holds to Pull should not list items with a notforloan status Remove items with a notforloan value other than 0 because items that are potentially holdable (with negative notforloan status) are not wanted in holds to pull. Test plan: 1- Have a bib with 2 items, both holdable. Give one of these items a notforloan status with a negative value (like Ordered in the testing docker data). 2- Place a title-level hold on your bib 3- Confirm your bib is showing on Holds to Pull as expected, prompting staff to pull the item that does not have a not for loan status (correct!) 4- Take the item that does not have a not for loan status and check it out to a different patron, make sure not to fill or cancel your hold 5- Reload Holds to Pull, see your title is still listed and now Koha is directing staff to the item with a notforloan status 6- Apply the patch 7- Reload Holds to Pull, see your title with a notforloan status is gone 8- Run `prove t/db_dependent/Koha/Holds.t` and all tests should pass
Created attachment 135075 [details] [review] Bug 30730: Unit tests Added an entry in holds for an item with negative notforloan value to test if get_items_that_can_fill returns the right items.
Created attachment 135081 [details] [review] Bug 30730: Holds to Pull should not list items with a notforloan status Remove items with a notforloan value other than 0 because items that are potentially holdable (with negative notforloan status) are not wanted in holds to pull. Test plan: 1- Have a bib with 2 items, both holdable. Give one of these items a notforloan status with a negative value (like Ordered in the testing docker data). 2- Place a title-level hold on your bib 3- Confirm your bib is showing on Holds to Pull as expected, prompting staff to pull the item that does not have a not for loan status (correct!) 4- Take the item that does not have a not for loan status and check it out to a different patron, make sure not to fill or cancel your hold 5- Reload Holds to Pull, see your title is still listed and now Koha is directing staff to the item with a notforloan status 6- Apply the patch 7- Reload Holds to Pull, see your title with a notforloan status is gone 8- Run `prove t/db_dependent/Koha/Holds.t` and all tests should pass Signed-off-by: David Nind <david@davidnind.com>
Created attachment 135082 [details] [review] Bug 30730: Unit tests Added an entry in holds for an item with negative notforloan value to test if get_items_that_can_fill returns the right items. Signed-off-by: David Nind <david@davidnind.com>
I originally started this an FYI note, but I think someone with a better knowledge of Koha needs to review this and decide what needs to be done, if anything. I'm a little confused with the terminology used. My current understanding: You can place a record-level hold or an item-level hold for items that have a negative not for loan value. This seems to be a feature rather than a bug, whatever the logical part of my brain thinks! The development version of the manual (which is not up-to-date) says for NOT_LOAN (Not for loan) authorised values, that: "- Negative number values will still allow holds (use for ‘on order’ statuses, for example) - Positive numbers will not allow holds or checkouts. - A value of 0 means ‘for loan’." https://koha-community.org/manual/22.05/en/html/administration.html#existing-values-label Reference: https://koha-community.org/manual/22.05/en/html/administration.html#existing-values-label (as the manual isn't up-to-date, this may no longer be the current situation) I also came across the SkipHoldTrapOnNotForLoanValue system preference: "This system preference is used to completely exclude items with ‘not for loan’ values from filling holds." This was added in 20.11 and back ported to 20.05 in bug 25232 - from the description "this .. prevents items with that notforloan value from triggering holds at checkin, allowing for a temporary quarantine or any other circumstance in which an item should be temporarily delayed from circulation. This presents a more customizable alternative to the existing TrapHoldsOnOrder system preference.". Reference (search the page for the system preference name): https://koha-community.org/manual/22.05/en/html/circulationpreferences.html From this, it would seem that: - You should be able to place holds for items with a 'Not for hold' status with a negative value. (Confirmed by testing master) - These should show up on the holds to pull list. (This is the current behaviour before this bug). - If you don't want holds to be placed on 'not for loan' items, then make these values greater than 0. For example, if you don't want holds placed on items that are on order, make the value greater than 0. I originally thought that you should be able to put that value in the SkipHoldTrapOnNotForLoanValue , but as per bug 25232 it wasn't intended for this scenario and doesn't work. Where to from here? Some options/ideas - not exhaustive: 1. Option 1: make 'Not for loan' setting make more sense (minimal effort): - update the documentation for the 'not for loan' authorised value section and system preferences SkipHoldTrapOnNotForLoanValue to clarify that: . use not for loan authorised values greater than 0 if you don't want holds to be placed, or conversely don't allow negative numbers or holds to be placed on items not for loan . use negative not for loan authorised values where items are not for loan (the logic of allowing holds on items not for loan escapes me, and I find it really confusing!) - update the default data for Koha so that the 'Ordered' value is greater than 0: that is, if it is not for loan as it is on order then you can't place holds by default; provide some SQL queries to update any existing 'Ordered' values that are negative - update the description for the SkipHoldTrapOnNotForLoanValue to clarify that this is only for when items are checked in and the scenario it is intended for => end result: holds to pull should work as intended and needs no change 2. Option 2: rethink how to implement this: . To me, if an item is 'Not for loan' then you should not be able to place holds on these items. . If you want to be able to override an item that is 'Not for loan' then maybe have a system preference OverrideNotForLoan or something similar that does what SkipHoldTrapOnNotForLoanValue does, except for placing holds. I think that this a pattern we use elsewhere in Koha. 3. Option 3: identify or ask the Koha Community for the scenarios where you may want to allow holds on items that are not for loan, and then go from there. I hope I haven't confused things even more!
From my understanding/interpretation, when we put "Ordered" in "notforloan" it means the item was ordered but still not in the library, but it would make sense that patrons can place a hold on that because it will be available when it arrives at the library (and thus "notforloan" should be cleared of the "Ordered" status). Is it how it was meant to be used?
Good spot finding 'SkipHoldTrapOnNotForLoanValue' David, to me that pref is slightly mid-described.. it feels like it should affect not just trapping at checking but also this HoldsQueue and pendingreserves too. For history, the notforloan handling was dropped from here by mistake originally in bug 3142 where it looks like we assumed the same logic was already taking place in the filter_by_for_hold. (Bad assumption.. we go from filtering on `!= 0` to filtering on `'<=' => 0` An initial 'fix' would be to restore the `!= 0` found in get_items_that_can_fill I believe.. though I'm not sure how the chaining would treat it.. I'm not entirely a fan of the current implementation in this patch.. your adding a query to fetch all items available for loans instead of just adding the 'notforloan' limit to the existing overall query. I'll have a read of the unit tests now to check they make sense, then I think we can golf this a little further. As for the 'SkipHoldTrapOnNotForLoanValue', that might be a nice thing to add support for whilst we're here.
Wow, the prefs do make this complicated! OK.. I spent some time working through this.. 'Trap' is very much about 'At Checkin' prompts.. in this case we're building lists of items to go and find.. so the preferences don't really come into play.
Created attachment 135207 [details] [review] Bug 30730: Holds to Pull should not list items with a notforloan status Remove items with a notforloan value other than 0 because items that are potentially holdable (with negative notforloan status) are not wanted in holds to pull. Test plan: 1- Have a bib with 2 items, both holdable. Give one of these items a notforloan status with a negative value (like Ordered in the testing docker data). 2- Place a title-level hold on your bib 3- Confirm your bib is showing on Holds to Pull as expected, prompting staff to pull the item that does not have a not for loan status (correct!) 4- Take the item that does not have a not for loan status and check it out to a different patron, make sure not to fill or cancel your hold 5- Reload Holds to Pull, see your title is still listed and now Koha is directing staff to the item with a notforloan status 6- Apply the patch 7- Reload Holds to Pull, see your title with a notforloan status is gone 8- Run `prove t/db_dependent/Koha/Holds.t` and all tests should pass Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135208 [details] [review] Bug 30730: Unit tests Added an entry in holds for an item with negative notforloan value to test if get_items_that_can_fill returns the right items. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135209 [details] [review] Bug 30730: (follow-up) Simplify query This patch drops the 'fetch all non-loanable items' query used in the subsequent query as part of the 'not_in' and instead replaces it with a simple filter on 'notforloan => 0' Test plan Run the included unit tests before and apply applying the followup. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135210 [details] [review] Bug 30730: Holds to Pull should not list items with a notforloan status Remove items with a notforloan value other than 0 because items that are potentially holdable (with negative notforloan status) are not wanted in holds to pull. Test plan: 1- Have a bib with 2 items, both holdable. Give one of these items a notforloan status with a negative value (like Ordered in the testing docker data). 2- Place a title-level hold on your bib 3- Confirm your bib is showing on Holds to Pull as expected, prompting staff to pull the item that does not have a not for loan status (correct!) 4- Take the item that does not have a not for loan status and check it out to a different patron, make sure not to fill or cancel your hold 5- Reload Holds to Pull, see your title is still listed and now Koha is directing staff to the item with a notforloan status 6- Apply the patch 7- Reload Holds to Pull, see your title with a notforloan status is gone 8- Run `prove t/db_dependent/Koha/Holds.t` and all tests should pass Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135211 [details] [review] Bug 30730: Unit tests Added an entry in holds for an item with negative notforloan value to test if get_items_that_can_fill returns the right items. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135212 [details] [review] Bug 30730: (follow-up) Simplify query This patch drops the 'fetch all non-loanable items' query used in the subsequent query as part of the 'not_in' and instead replaces it with a simple filter on 'notforloan => 0' Test plan Run the included unit tests before and apply applying the followup. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
OK.. I'm happy this restores the previous functionality.. we have Unit tests and the code is simplified. QA scripts are happy Passing QA
Thanks Martin, apologies for confusing things....
Not at all David, your insight is always really helpful. It certainly got me digging into QA for this, really glad to have you on the team.
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Can we get this onto 21.11?
Getting this into 21.11 would FANTASTIC!!! Please and thank you!