|
Description
Tomás Cohen Arazi (tcohen)
2021-06-08 11:15:53 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. 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? (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. Created attachment 124505 [details] [review] Bug 28529: Add BiblioHoldItemTypeUseForRules system preference Created attachment 124506 [details] [review] Bug 28529: Unit tests 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 (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. 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. (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 Created attachment 134557 [details] [review] Bug 28529: Add BiblioHoldItemTypeUseForRules system preference Created attachment 134558 [details] [review] Bug 28529: Unit tests 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 Rebased. I still think this could've gone without the syspref and enabled by default. Yet, as-is it is good for inclusion. 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
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! Created attachment 134790 [details] [review] Bug 28529: Add BiblioHoldItemTypeUseForRules system preference Created attachment 134791 [details] [review] Bug 28529: Unit tests 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 (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. 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 Created attachment 134809 [details] [review] Bug 28529: Add BiblioHoldItemTypeUseForRules system preference Created attachment 134810 [details] [review] Bug 28529: Unit tests 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 Created attachment 134837 [details] [review] Bug 28529: Add BiblioHoldItemTypeUseForRules system preference Signed-off-by: David Nind <david@davidnind.com> Created attachment 134838 [details] [review] Bug 28529: Unit tests Signed-off-by: David Nind <david@davidnind.com> 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> I think I have tested this correctly. Another sign-off would be very welcome! 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> 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> 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> 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 (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. 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> 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> I propose to push this in next release, too risky at this end-of-cycle. (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. Pushed to master for 22.11. Nice work everyone, thanks! Pushed to 22.05.x for 22.05.01 @RMaints: can we get this down to 21.05? Hi, tried to apply to 21.11.x but have conflicts. Didn't manage to fix them on the main patch. 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> 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> backported to 21.11.x for 21.11.11. Thx! Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed. Nothing to document, marking resolved. |