Bug 30730 - Holds to Pull should not list items with a notforloan status
Summary: Holds to Pull should not list items with a notforloan status
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Shi Yao Wang
QA Contact: Martin Renvoize
URL:
Keywords: regression, rel_21_11_candidate
: 30322 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-05-11 13:34 UTC by Andrew Fuerste-Henry
Modified: 2022-11-16 15:13 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00


Attachments
Bug 30730: Holds to Pull should not list items with a notforloan status (2.02 KB, patch)
2022-05-17 15:49 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30730: Unit tests (2.13 KB, patch)
2022-05-17 15:49 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30730: Holds to Pull should not list items with a notforloan status (2.07 KB, patch)
2022-05-17 23:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 30730: Unit tests (2.18 KB, patch)
2022-05-17 23:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 30730: Holds to Pull should not list items with a notforloan status (2.13 KB, patch)
2022-05-19 15:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30730: Unit tests (2.24 KB, patch)
2022-05-19 15:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30730: (follow-up) Simplify query (1.43 KB, patch)
2022-05-19 15:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30730: Holds to Pull should not list items with a notforloan status (2.13 KB, patch)
2022-05-19 15:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30730: Unit tests (2.24 KB, patch)
2022-05-19 15:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30730: (follow-up) Simplify query (1.43 KB, patch)
2022-05-19 15:33 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2022-05-11 13:34:44 UTC
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.
Comment 1 Caroline Cyr La Rose 2022-05-16 14:04:34 UTC
*** Bug 30322 has been marked as a duplicate of this bug. ***
Comment 2 Caroline Cyr La Rose 2022-05-16 15:08:50 UTC
I think this bug may be due to Bug 29786 which removed the notforloan check in Koha/Holds.pm ?
Comment 3 Shi Yao Wang 2022-05-17 15:49:51 UTC
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
Comment 4 Shi Yao Wang 2022-05-17 15:49:55 UTC
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.
Comment 5 David Nind 2022-05-17 23:30:32 UTC
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>
Comment 6 David Nind 2022-05-17 23:30:37 UTC
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>
Comment 7 David Nind 2022-05-18 01:52:48 UTC
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!
Comment 8 Shi Yao Wang 2022-05-18 12:47:49 UTC
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?
Comment 9 Martin Renvoize 2022-05-19 14:48:02 UTC
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.
Comment 10 Martin Renvoize 2022-05-19 15:28:35 UTC
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.
Comment 11 Martin Renvoize 2022-05-19 15:29:18 UTC
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>
Comment 12 Martin Renvoize 2022-05-19 15:29:23 UTC
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>
Comment 13 Martin Renvoize 2022-05-19 15:29:27 UTC
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>
Comment 14 Martin Renvoize 2022-05-19 15:33:49 UTC
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>
Comment 15 Martin Renvoize 2022-05-19 15:33:54 UTC
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>
Comment 16 Martin Renvoize 2022-05-19 15:33:59 UTC
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>
Comment 17 Martin Renvoize 2022-05-19 15:35:42 UTC
OK.. I'm happy this restores the previous functionality.. we have Unit tests and the code is simplified.

QA scripts are happy

Passing QA
Comment 18 David Nind 2022-05-19 18:12:10 UTC
Thanks Martin, apologies for confusing things....
Comment 19 Martin Renvoize 2022-05-21 00:09:37 UTC
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.
Comment 20 Fridolin Somers 2022-05-23 18:53:43 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 21 Martin Renvoize 2022-09-23 14:36:43 UTC
Can we get this onto 21.11?
Comment 22 Steve Butzel 2022-11-16 15:13:20 UTC
Getting this into 21.11 would FANTASTIC!!! Please and thank you!