Bug 28529

Summary: Item type-constrained biblio-level holds should honour max_holds as item-level do
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Hold requestsAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: andrewfh, arthur.suzuki, david, dcook, fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer, kyle, lucas, martin.renvoize, nick, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24683
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.01, 21.11.11
Attachments: Bug 28529: Add BiblioHoldItemTypeUseForRules system preference
Bug 28529: Unit tests
Bug 28529: Make biblio-level hold itemtype count against max rules
Bug 28529: Add BiblioHoldItemTypeUseForRules system preference
Bug 28529: Unit tests
Bug 28529: Make biblio-level hold itemtype count against max rules
Screenshots - behaviour after patches applied
Bug 28529: Add BiblioHoldItemTypeUseForRules system preference
Bug 28529: Unit tests
Bug 28529: Make biblio-level hold itemtype count against max rules
Bug 28529: Add BiblioHoldItemTypeUseForRules system preference
Bug 28529: Unit tests
Bug 28529: Make biblio-level hold itemtype count against max rules
Bug 28529: Add BiblioHoldItemTypeUseForRules system preference
Bug 28529: Unit tests
Bug 28529: Make biblio-level hold itemtype count against max rules
Bug 28529: Add BiblioHoldItemTypeUseForRules system preference
Bug 28529: Unit tests
Bug 28529: Make biblio-level hold itemtype count against max rules
Bug 28529: Regression tests
Bug 28529: Make biblio-level hold itemtype count against max rules
[21.11.x] Bug 28529: Regression tests
[21.11.x] Bug 28529: Make biblio-level hold itemtype count against max rules

Description Tomás Cohen Arazi 2021-06-08 11:15:53 UTC
Bug 15533 introduced a way to constraint biblio-level holds to a specific item type. This is controlled by the AllowHoldItemTypeSelection syspref.

On the circulation rules, you are able to set max_holds for an item type.

This max_holds setting, seems to be honoured for item-level holds only, and it is not quite correct.

To reproduce:
1. Have a patron of category 'Staff' (S)
2. Have 3 records with items with the 'BK' item type, and maybe others
3. Enable AllowHoldItemTypeSelection
4. Set a limit of 2 max holds for that category+item type
5. In the OPAC. Place bibio-level holds, with item type contraint to 'BK' on those 3 records
=> FAIL: You can place the 3 holds
Comment 1 Tomás Cohen Arazi 2021-06-08 11:30:42 UTC
I consider this a bug, but this should be an opt-in change, because the feature never considered circ rules in its conception, and libraries are using them as-is and might see their workflows affected.
Comment 2 Katrin Fischer 2021-06-08 12:56:01 UTC
Hi Tomas,
I seem to remember that the biblio-level itemtype played into the max holds setting as well (which poses a problem to us, as we usually don't set it).

In your test, was there a biblio-level itemtype (942$c) set?
Comment 3 Tomás Cohen Arazi 2021-06-08 13:07:24 UTC
(In reply to Katrin Fischer from comment #2)
> Hi Tomas,
> I seem to remember that the biblio-level itemtype played into the max holds
> setting as well (which poses a problem to us, as we usually don't set it).
> 
> In your test, was there a biblio-level itemtype (942$c) set?

There wasn't a 942$c set. If you look at bug 15533 you will notice the purpose of the feature is to define which items on the biblio can fill the hold.
Comment 4 Tomás Cohen Arazi 2021-09-03 19:51:43 UTC
Created attachment 124505 [details] [review]
Bug 28529: Add BiblioHoldItemTypeUseForRules system preference
Comment 5 Tomás Cohen Arazi 2021-09-03 19:51:47 UTC
Created attachment 124506 [details] [review]
Bug 28529: Unit tests
Comment 6 Tomás Cohen Arazi 2021-09-03 19:51:51 UTC
Created attachment 124507 [details] [review]
Bug 28529: Make biblio-level hold itemtype count against max rules

The current situation is that biblio-level holds can be assigned an item
type, so they can only be fulfilled by items matching that specified
item type (be it item-level itype or the fallback to biblio-level).

But there's the situation in which max holds limits for a specific item
type can be overridden by using biblio-level holds with item type
selection (AllowHoldItemTypeSelection) enabled.

To test:
1. Have a patron of category 'Staff' (S)
2. Have 3 records with items with the 'BK' item type, and maybe others
3. Enable AllowHoldItemTypeSelection
4. Set a limit of 2 max holds for that category+item type
5. In the OPAC. Place bibio-level holds, with item type contraint to 'BK' on those 3 records
=> FAIL: You can place the 3 holds
6. Cancel the holds
7. Apply this patch and restart all
8. Repeat 5
=> SUCCESS: You can only place 2 holds
9. Run:
   $ kshell t/db_dependent/Reserves.t
=> SUCCESS: Tests pass!
10. Sign off :-D
Comment 7 Tomás Cohen Arazi 2021-09-03 19:54:36 UTC
(In reply to Katrin Fischer from comment #2)
> Hi Tomas,
> I seem to remember that the biblio-level itemtype played into the max holds
> setting as well (which poses a problem to us, as we usually don't set it).
> 
> In your test, was there a biblio-level itemtype (942$c) set?

I did some digging about this. The biblio-level itype is used for item-level holds, when item-level_itypes is disabled. So it doesn't seem to be the same thing.
Comment 8 David Nind 2021-09-03 22:36:41 UTC
I couldn't reproduce the issue before the patch is applied (koha-testing-docker):

- For step 4 I setup a circulation and fines rule for patron category=Staff and item type = Books, with holds allowed (total) = 2
- Using the OPAC, I placed holds with "Request specific item type" set to Books
- I can't place a third hold (message was "There are no items that can be placed on hold.")
- AllowHoldItemTypeSelection is enabled
- The records I placed holds on all have 942$c set to Books and each item is also set to Books and have multiple items

Is there something else or other system preference setting needed?

I'm guessing it is something with the circulation and fines rules that I'm not getting right.
Comment 9 Tomás Cohen Arazi 2022-05-03 18:42:24 UTC
(In reply to David Nind from comment #8)
> I couldn't reproduce the issue before the patch is applied
> (koha-testing-docker):
> 
> I'm guessing it is something with the circulation and fines rules that I'm
> not getting right.

David, thanks for taking the time. I have just set a fresh KTD and reproduced the issue fairly easily. This were my steps:

1. Fresh KTD (master branch of Koha)
2. Pick 'Henry Acevedo' patron, and set a known user/pass (henry/Henry123!)
3. Enable AllowHoldItemTypeSelection
4. Circ rules: there's only one global rule, for all categories and types. I generated a new one, for 'Staff' and 'Books' with everything similar to the existing rule, except holds. I put 2 in the holds rules (max, daily, per record)
5. Search for 'a'
6. Make sure the first three titles contain items of type 'Books', I used Edit > Edit items and added 'multiple' items of type 'Books'. On each record
7. Logged in as henry/Henry123! in the OPAC
8. Searched for 'a'
9. Placed record-level holds on them, choosing the 'Books' in the More options section.
=> FAIL: I was able to set 4 holds
Comment 10 Tomás Cohen Arazi 2022-05-03 21:02:24 UTC
Created attachment 134557 [details] [review]
Bug 28529: Add BiblioHoldItemTypeUseForRules system preference
Comment 11 Tomás Cohen Arazi 2022-05-03 21:02:32 UTC
Created attachment 134558 [details] [review]
Bug 28529: Unit tests
Comment 12 Tomás Cohen Arazi 2022-05-03 21:02:39 UTC
Created attachment 134559 [details] [review]
Bug 28529: Make biblio-level hold itemtype count against max rules

The current situation is that biblio-level holds can be assigned an item
type, so they can only be fulfilled by items matching that specified
item type (be it item-level itype or the fallback to biblio-level).

But there's the situation in which max holds limits for a specific item
type can be overridden by using biblio-level holds with item type
selection (AllowHoldItemTypeSelection) enabled.

To test:
1. Have a patron of category 'Staff' (S)
2. Have 3 records with items with the 'BK' item type, and maybe others
3. Enable AllowHoldItemTypeSelection
4. Set a limit of 2 max holds for that category+item type
5. In the OPAC. Place bibio-level holds, with item type contraint to 'BK' on those 3 records
=> FAIL: You can place the 3 holds
6. Cancel the holds
7. Apply this patch and restart all
8. Repeat 5
=> SUCCESS: You can only place 2 holds
9. Run:
   $ kshell t/db_dependent/Reserves.t
=> SUCCESS: Tests pass!
10. Sign off :-D
Comment 13 Tomás Cohen Arazi 2022-05-03 21:04:50 UTC
Rebased. I still think this could've gone without the syspref and enabled by default. Yet, as-is it is good for inclusion.
Comment 14 David Nind 2022-05-04 02:06:57 UTC
Created attachment 134562 [details]
Screenshots - behaviour after patches applied

Hi Tomás.

I finally managed to get this to work (I think)!

Things I didn't do:

- Make sure the 3 records had multiple books and some other type of items.

- I didn't update the database after the patches were applied.

- I didn't test with BiblioHoldItemTypeUseForRules set to 'Don't consider' (retains current behaviour - the total number of holds is not taken into account when multiple item types) and 'Consider' (limits number of holds to holds allowed (total)) when AllowHoldItemTypeSelection set to allow.

However:

1. When I went to place a hold for record 3 (searching for 'a' after applying the patches) I didn't get any message - it just took me to the patron holds summary page and didn't place a hold.

2. Placing a hold for another record where there were only items with one item type showed the message "There are no items that can be placed on hold.", which seems like the correct behaviour to me. However, if I add another item to that record with a different item type, I get the same behaviour as in 1 - that is, I don't get any message and am taken to the patron holds summary screen.

I've attached a PDF with some screenshots to illustrate what I mean.

Also, for the new/updated system preference, should it be 'BiblioHold...' and 'biblio-level hold...' or 'RecordHold...' and 'record-level hold...'? I'm not sure of the correct terminology to use, considering we have several system preference names that include biblio, but all the descriptions now use bibliographic record.

David
Comment 15 Tomás Cohen Arazi 2022-05-04 02:21:06 UTC
Yes, there's no feedback on problems with selected options. I felt like it was out of the scope of the bug because any error on placing the hold is actually silently skipped, but 99.9% of the time the checks have been done first and the UI doesn't let you choose the problematic options.

One way to trigger that is to choose to place some allowed hold, but before confirming, use another browser tab to add new holds that would invalidate the one we are about to place.
In that case you will reproduce the silently skipped hold when you try to confirm it.

I'll think of a proper solution tomorrow at my desk.

Thanks!
Comment 16 Tomás Cohen Arazi 2022-05-09 18:06:46 UTC
Created attachment 134790 [details] [review]
Bug 28529: Add BiblioHoldItemTypeUseForRules system preference
Comment 17 Tomás Cohen Arazi 2022-05-09 18:06:52 UTC
Created attachment 134791 [details] [review]
Bug 28529: Unit tests
Comment 18 Tomás Cohen Arazi 2022-05-09 18:06:59 UTC
Created attachment 134792 [details] [review]
Bug 28529: Make biblio-level hold itemtype count against max rules

The current situation is that biblio-level holds can be assigned an item
type, so they can only be fulfilled by items matching that specified
item type (be it item-level itype or the fallback to biblio-level).

But there's the situation in which max holds limits for a specific item
type can be overridden by using biblio-level holds with item type
selection (AllowHoldItemTypeSelection) enabled.

To test:
1. Have a patron of category 'Staff' (S)
2. Have 3 records with items with the 'BK' item type, and maybe others
3. Enable AllowHoldItemTypeSelection
4. Set a limit of 2 max holds for that category+item type
5. In the OPAC. Place bibio-level holds, with item type contraint to 'BK' on those 3 records
=> FAIL: You can place the 3 holds
6. Cancel the holds
7. Apply this patch and restart all
8. Repeat 5
=> SUCCESS: You can only place 2 holds
9. Run:
   $ kshell t/db_dependent/Reserves.t
=> SUCCESS: Tests pass!
10. Sign off :-D
Comment 19 Tomás Cohen Arazi 2022-05-09 18:08:18 UTC
(In reply to David Nind from comment #14)
> However:
> 
> 1. When I went to place a hold for record 3 (searching for 'a' after
> applying the patches) I didn't get any message - it just took me to the
> patron holds summary page and didn't place a hold.

I took the current approach of checking holdability of each option so it doesn't offer the end user the forbidden item types.

It should work as expected now.
Comment 20 David Nind 2022-05-09 23:08:29 UTC
Patch no longer applies 8-(..

git bz apply 28529

Bug 28529 - Item type-constrained biblio-level holds should honour max_holds as item-level do

134790 - Bug 28529: Add BiblioHoldItemTypeUseForRules system preference
134791 - Bug 28529: Unit tests
134792 - Bug 28529: Make biblio-level hold itemtype count against max rules

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 28529: Add BiblioHoldItemTypeUseForRules system preference
Applying: Bug 28529: Unit tests
Using index info to reconstruct a base tree...
M	t/db_dependent/Reserves.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Reserves.t
CONFLICT (content): Merge conflict in t/db_dependent/Reserves.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 28529: Unit tests
Comment 21 Tomás Cohen Arazi 2022-05-10 11:17:47 UTC
Created attachment 134809 [details] [review]
Bug 28529: Add BiblioHoldItemTypeUseForRules system preference
Comment 22 Tomás Cohen Arazi 2022-05-10 11:17:55 UTC
Created attachment 134810 [details] [review]
Bug 28529: Unit tests
Comment 23 Tomás Cohen Arazi 2022-05-10 11:18:02 UTC
Created attachment 134811 [details] [review]
Bug 28529: Make biblio-level hold itemtype count against max rules

The current situation is that biblio-level holds can be assigned an item
type, so they can only be fulfilled by items matching that specified
item type (be it item-level itype or the fallback to biblio-level).

But there's the situation in which max holds limits for a specific item
type can be overridden by using biblio-level holds with item type
selection (AllowHoldItemTypeSelection) enabled.

To test:
1. Have a patron of category 'Staff' (S)
2. Have 3 records with items with the 'BK' item type, and maybe others
3. Enable AllowHoldItemTypeSelection
4. Set a limit of 2 max holds for that category+item type
5. In the OPAC. Place bibio-level holds, with item type contraint to 'BK' on those 3 records
=> FAIL: You can place the 3 holds
6. Cancel the holds
7. Apply this patch and restart all
8. Repeat 5
=> SUCCESS: You can only place 2 holds
9. Run:
   $ kshell t/db_dependent/Reserves.t
=> SUCCESS: Tests pass!
10. Sign off :-D
Comment 24 David Nind 2022-05-10 22:24:05 UTC
Created attachment 134837 [details] [review]
Bug 28529: Add BiblioHoldItemTypeUseForRules system preference

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2022-05-10 22:24:11 UTC
Created attachment 134838 [details] [review]
Bug 28529: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 David Nind 2022-05-10 22:24:16 UTC
Created attachment 134839 [details] [review]
Bug 28529: Make biblio-level hold itemtype count against max rules

The current situation is that biblio-level holds can be assigned an item
type, so they can only be fulfilled by items matching that specified
item type (be it item-level itype or the fallback to biblio-level).

But there's the situation in which max holds limits for a specific item
type can be overridden by using biblio-level holds with item type
selection (AllowHoldItemTypeSelection) enabled.

To test:
1. Have a patron of category 'Staff' (S)
2. Have 3 records with items with the 'BK' item type, and maybe others
3. Enable AllowHoldItemTypeSelection
4. Set a limit of 2 max holds for that category+item type
5. In the OPAC. Place bibio-level holds, with item type contraint to 'BK' on those 3 records
=> FAIL: You can place the 3 holds
6. Cancel the holds
7. Apply this patch and restart all
8. Repeat 5
=> SUCCESS: You can only place 2 holds
9. Run:
   $ kshell t/db_dependent/Reserves.t
=> SUCCESS: Tests pass!
10. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 27 David Nind 2022-05-10 22:29:28 UTC
I think I have tested this correctly.

Another sign-off would be very welcome!
Comment 28 Martin Renvoize 2022-05-14 07:42:13 UTC
Created attachment 134991 [details] [review]
Bug 28529: Add BiblioHoldItemTypeUseForRules system preference

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2022-05-14 07:42:17 UTC
Created attachment 134992 [details] [review]
Bug 28529: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2022-05-14 07:42:22 UTC
Created attachment 134993 [details] [review]
Bug 28529: Make biblio-level hold itemtype count against max rules

The current situation is that biblio-level holds can be assigned an item
type, so they can only be fulfilled by items matching that specified
item type (be it item-level itype or the fallback to biblio-level).

But there's the situation in which max holds limits for a specific item
type can be overridden by using biblio-level holds with item type
selection (AllowHoldItemTypeSelection) enabled.

To test:
1. Have a patron of category 'Staff' (S)
2. Have 3 records with items with the 'BK' item type, and maybe others
3. Enable AllowHoldItemTypeSelection
4. Set a limit of 2 max holds for that category+item type
5. In the OPAC. Place bibio-level holds, with item type contraint to 'BK' on those 3 records
=> FAIL: You can place the 3 holds
6. Cancel the holds
7. Apply this patch and restart all
8. Repeat 5
=> SUCCESS: You can only place 2 holds
9. Run:
   $ kshell t/db_dependent/Reserves.t
=> SUCCESS: Tests pass!
10. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2022-05-14 07:44:24 UTC
All works as described and resolves the bug.. Unit tests passing and QA script happy.

I would argue we don't really need the system preference for this.. in my opinion, it's fixing an inconsistency/bug and thus would be a welcome change and consistency improvement so doesn't really need the option to enable/disable..  But I'll leave the final decision to the RM on that one.

Passing QA
Comment 32 Tomás Cohen Arazi 2022-05-16 17:26:41 UTC
(In reply to Martin Renvoize from comment #31)
> All works as described and resolves the bug.. Unit tests passing and QA
> script happy.
> 
> I would argue we don't really need the system preference for this.. in my
> opinion, it's fixing an inconsistency/bug and thus would be a welcome change
> and consistency improvement so doesn't really need the option to
> enable/disable..  But I'll leave the final decision to the RM on that one.
> 
> Passing QA

I think you're absolutely right. This is really a behavior inconsistency introduced by bug 15533 a lot of time ago!

I agree 100% we shouldn't make it depend on that syspref. But I hoped to hear others 'complain about the fix' and overstepped. I will provide a follow-up removing it, just in case the RM prefers to pick it.
Comment 33 Tomás Cohen Arazi 2022-05-16 17:55:58 UTC
Created attachment 135044 [details] [review]
Bug 28529: Regression tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Tomás Cohen Arazi 2022-05-16 17:56:05 UTC
Created attachment 135045 [details] [review]
Bug 28529: Make biblio-level hold itemtype count against max rules

The current situation is that biblio-level holds can be assigned an item
type, so they can only be fulfilled by items matching that specified
item type (be it item-level itype or the fallback to biblio-level).

But there's the situation in which max holds limits for a specific item
type can be overridden by using biblio-level holds with item type
selection (AllowHoldItemTypeSelection) enabled.

To test:
1. Have a patron of category 'Staff' (S)
2. Have 3 records with items with the 'BK' item type, and maybe others
3. Enable AllowHoldItemTypeSelection
4. Set a limit of 2 max holds for that category+item type
5. In the OPAC. Place bibio-level holds, with item type contraint to 'BK' on those 3 records
=> FAIL: You can place the 3 holds
6. Cancel the holds
7. Apply this patch and restart all
8. Repeat 5
=> SUCCESS: You can only place 2 holds
9. Run:
   $ kshell t/db_dependent/Reserves.t
=> SUCCESS: Tests pass!
10. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Fridolin Somers 2022-05-16 21:18:19 UTC
I propose to push this in next release, too risky at this end-of-cycle.
Comment 36 Tomás Cohen Arazi 2022-05-16 22:23:51 UTC
(In reply to Fridolin Somers from comment #35)
> I propose to push this in next release, too risky at this end-of-cycle.

+1 we've got enough to do. This bugfix can be easily backported anyway.
Comment 37 Tomás Cohen Arazi 2022-06-01 19:30:51 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 38 Lucas Gass 2022-06-10 15:24:52 UTC
Pushed to 22.05.x for 22.05.01
Comment 39 Tomás Cohen Arazi 2022-06-22 21:03:51 UTC
@RMaints: can we get this down to 21.05?
Comment 40 Arthur Suzuki 2022-07-21 13:00:30 UTC
Hi, tried to apply to 21.11.x but have conflicts.
Didn't manage to fix them on the main patch.
Comment 41 Tomás Cohen Arazi 2022-07-21 14:00:48 UTC
Created attachment 137964 [details] [review]
[21.11.x] Bug 28529: Regression tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 42 Tomás Cohen Arazi 2022-07-21 14:00:56 UTC
Created attachment 137965 [details] [review]
[21.11.x] Bug 28529: Make biblio-level hold itemtype count against max rules

The current situation is that biblio-level holds can be assigned an item
type, so they can only be fulfilled by items matching that specified
item type (be it item-level itype or the fallback to biblio-level).

But there's the situation in which max holds limits for a specific item
type can be overridden by using biblio-level holds with item type
selection (AllowHoldItemTypeSelection) enabled.

To test:
1. Have a patron of category 'Staff' (S)
2. Have 3 records with items with the 'BK' item type, and maybe others
3. Enable AllowHoldItemTypeSelection
4. Set a limit of 2 max holds for that category+item type
5. In the OPAC. Place bibio-level holds, with item type contraint to 'BK' on those 3 records
=> FAIL: You can place the 3 holds
6. Cancel the holds
7. Apply this patch and restart all
8. Repeat 5
=> SUCCESS: You can only place 2 holds
9. Run:
   $ kshell t/db_dependent/Reserves.t
=> SUCCESS: Tests pass!
10. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 43 Arthur Suzuki 2022-07-21 14:37:27 UTC
backported to 21.11.x for 21.11.11.
Thx!
Comment 44 Victor Grousset/tuxayo 2022-07-24 22:08:43 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document, marking resolved.