Created attachment 99257 [details] An example of a biblio with mixed itemtype items. When having different rules for placing holds for each item type, having both item types on one biblio "breaks" these rules. As shown in the attachment: Smart rules for item type "Course literature" allows patrons to place holds on items. The smart rules for the item type "Dayloan" does not allow patrons to place holds on the items. When having both item types on one biblio, it should in theory be possible to place a hold on biblio level even if the dayloan item type is available. Setting the option "AllowHoldItemTypeSelection" to allow does not solve the problem. Various other options have been tried, but as long as there is an available item that is not "not for loan", then placing holds is not possible. This is troublesome for us, since holds must be placed manually by staff. The Dayloan item type is for having a copy available for many students and should not be placed on hold.
The problem can be reproduced by doing the following; The basic assumption is that no on shelf holds are allowed ("if all unavailable"), regardless of item type. No holds on Item level. Setting a Koha item type on the biblio (942 c) has no effect on the issue. In smart rules define 1 category of items that allow holds on biblio level if all unavailable ("course literature" in our catalog), and one category that do not allow holds whatsoever ("dayloan"). Lend the "course literature" item specimen that is eligeble for holds to a patron. Leave the other copy that do not allow holds on the shelf. As a patron that has no extra powers than a normal user, place a hold on biblio level. Koha will report that _there are no items that can be placed on hold_. It seems that smart rules that should be applied on item levels are not if there is a mix of rules for items on one biblio.
Created attachment 100966 [details] [review] Bug 24683: Add description of QueryFuzzy support in ES to syspref description To test: Apply patch Find QueryFuzzy in sysprefs Verify the new text makes sense
Nick: Wrong bug?
(In reply to Nick Clemens from comment #2) > Created attachment 100966 [details] [review] [review] > Bug 24683: Add description of QueryFuzzy support in ES to syspref description > > To test: > Apply patch > Find QueryFuzzy in sysprefs > Verify the new text makes sense Nick: this is not the bug you are looking for.
Changing to NEW as there is no patch and no assignee. Please update if you plan to work on this!
Thank you Katrin. Yes, we need a solution in some way, will continue working on this.
In version 18.11 we successfully tried adding the following section of code to lines 1191 to 1193 in Reserves.pm, however the same code is not used in the current installation. It might be of some use. if($i->itype() eq "DL") { next; }
Created attachment 105651 [details] [review] Bug 24683: Subroutine name changed (fix), no code logic changed This is the intermediate refactor: renamed subroutine only. Naming mistake came because this sub is used to detect if anything available for hold, but it used in "if ANY UNAVAILABLE rule", so actually results of this sub negated (see below "return" in the code). In details: when previous refactor was done, name for subroutine was chosen wrongly in "opposite" direction from what it actually does: it was named "ItemsAnyAvailableForHold", but this subroutine gave truth (1) if at least one of the items available on shelf, not lost, not on loan, not held, and not restricted by smart rules and damaged status. So, if this sub says that item is still "available", this actually PREVENTS item from hold in parent sub (see negated return): sub IsAvailableForItemLevelRequest { ... my $any_available = ItemsAnyAvailableAndNotRestricted... return $any_available ? 0 : 1; # ^^^ if any available and not restricted - we don't allow # on-shelf holds ... I.e. like it named now: "ItemsAnyAvailableAndNotRestricted". Small aside fix: white space for '&&' inside brackets added to join operation by priority visually. Testing plan not needed: all places where sub used it just renamed. More: all this places/code was introduced in one older commit so there is also no overlaps or other calls/uses for this subroutine.
Created attachment 105652 [details] [review] Bug 24683: Fix for take smart rules into account in "if all unavailable" Inside of ItemsAnyAvailableAndNotRestricted was no effect from main set of smart rules (per record and other limits): i.e. call to "CanItemBeReserved" was absent totally. Because of this there was a bug: for example none of two items were allowed to be held when first was allowed by one smart rule, BUT on loan, and second was disallowed by another smart rule (for example, 0 "Holds per record"), i.e. in this case both items unavailable: so on-shelf holds setting "allow hold if all unavailable" should allow to hold first one, and not the second one. But it was that both wasn't allowed to be held. Solution: call to sub "CanItemBeReserved" added so it checked for "...->{status} ne 'OK'" so now if item restricted by smart rule it also accounted as "unavailable" and "AnyAvailavble" not counts it. How to reproduce: 1. Add 2 smart rules (/cgi-bin/koha/admin/smart-rules.pl) with "on shelf holds": "if all unavailable" for all rules, no "item level holds", and set "holds per record" to 2 for "books" and "0" for "computer files". 2. Create only 2 items for one biblio, but different types, "book" and "computer file". For example in misc4dev env: /cgi-bin/koha/cataloguing/additem.pl?biblionumber=1#additem 3. Check out that item of type "book" to some person, for example, in misc4dev: /cgi-bin/koha/circ/circulation.pl?borrowernumber=2&barcode=3999900000001 4. Open reserve/request, for example, for item 1 and patron 1 in misc4dev env (/cgi-bin/koha/reserve/request.pl?biblionumber=1&borrowernumber=1) 5. It does not allow to hold, both red crossed, but computer file says "Exceeded max holds per record" because of "0" limit set on step 1. 6. Apply the patch. 7. Reload page on step 5 and see that "book" will be available for hold, but "computer file" still will be red-crossed "Exceeded max holds per record", now that's correct because both items unavailable: one because on load, another because of "0" limit for computer files. 8. Check-in book from step 3 so it will be returned to the library, 9. Reload page on step 5 and see that again no any holds available, but it's now also correct: "book" now returned but "on shelf holds" set to "if all unavailable".
I confirm this patch fixes another bug: If there is a biblio with 2 items having different itemtypes. First item is not available for holds, the other one is checked out, and available "if all unavailable". At the OPAC there was a blocking message "There are no items that can be placed on hold." With this patch I am able to place a hold on the second item. Technical comments: It seems ok, only minor changes: * perltidy a bit the tests * add tests in a subtest * AddReturn statement is not needed * it would be great to have IsAvailableForItemLevelRequest tested as well (just in case, especially because we are lacking test coverage for this circ rule! This comment does not block the signoff process.
Why "in discussion"?
I thought to hold sign off for a few days and group tests and fix those tech comments Joubu requested... Or not needed?
s/not needed?/not blocks or not that priority?/
I was just wondering, better to use "Failed QA" if you want to fix those things first - In Discussion can be a bit of a dead end at times as it indicated an unresolved disagreement. You could also leave as "needs sign-off" and add the tests as follow-up - judging from the last line of Joubu's comment.
Created attachment 107072 [details] [review] Bug 24683: Subroutine name changed (fix), no code logic changed This is the intermediate refactor: renamed subroutine only. Naming mistake came because this sub is used to detect if anything available for hold, but it used in "if ANY UNAVAILABLE rule", so actually results of this sub negated (see below "return" in the code). In details: when previous refactor was done, name for subroutine was chosen wrongly in "opposite" direction from what it actually does: it was named "ItemsAnyAvailableForHold", but this subroutine gave truth (1) if at least one of the items available on shelf, not lost, not on loan, not held, and not restricted by smart rules and damaged status. So, if this sub says that item is still "available", this actually PREVENTS item from hold in parent sub (see negated return): sub IsAvailableForItemLevelRequest { ... my $any_available = ItemsAnyAvailableAndNotRestricted... return $any_available ? 0 : 1; # ^^^ if any available and not restricted - we don't allow # on-shelf holds ... I.e. like it named now: "ItemsAnyAvailableAndNotRestricted". Small aside fix: white space for '&&' inside brackets added to join operation by priority visually. Testing plan not needed: all places where sub used it just renamed. More: all this places/code was introduced in one older commit so there is also no overlaps or other calls/uses for this subroutine.
Created attachment 107073 [details] [review] Bug 24683: Optimize loop in ItemsAnyAvailableAndNotRestricted Add cut-off shortcut (return from inside the loop) when first "Any Available And Not Restricted" item found, because one is enough for "Any". Testing: no change visible for code behavior/results, it is just faster because won't loop over the whole set.
Created attachment 107074 [details] [review] Bug 24683: Fix for take smart rules into account in "if all unavailable" Inside of ItemsAnyAvailableAndNotRestricted was no effect from main set of smart rules (per record and other limits): i.e. call to "CanItemBeReserved" was absent totally. Because of this there was a bug: for example none of two items were allowed to be held when first was allowed by one smart rule, BUT on loan, and second was disallowed by another smart rule (for example, 0 "Holds per record"), i.e. in this case both items unavailable: so on-shelf holds setting "allow hold if all unavailable" should allow to hold first one, and not the second one. But it was that both wasn't allowed to be held. Solution: call to sub "CanItemBeReserved" added so it checked for "...->{status} ne 'OK'" so now if item restricted by smart rule it also accounted as "unavailable" and "AnyAvailavble" not counts it. How to reproduce: 1. Add 2 smart rules (/cgi-bin/koha/admin/smart-rules.pl) with "on shelf holds": "if all unavailable" for all rules, no "item level holds", and set "holds per record" to 2 for "books" and "0" for "computer files". 2. Create only 2 items for one biblio, but different types, "book" and "computer file". For example in misc4dev env: /cgi-bin/koha/cataloguing/additem.pl?biblionumber=1#additem 3. Check out that item of type "book" to some person, for example, in misc4dev: /cgi-bin/koha/circ/circulation.pl?borrowernumber=2&barcode=3999900000001 4. Open reserve/request, for example, for item 1 and patron 1 in misc4dev env (/cgi-bin/koha/reserve/request.pl?biblionumber=1&borrowernumber=1) 5. It does not allow to hold, both red crossed, but computer file says "Exceeded max holds per record" because of "0" limit set on step 1. 6. Apply the patch. 7. Reload page on step 5 and see that "book" will be available for hold, but "computer file" still will be red-crossed "Exceeded max holds per record", now that's correct because both items unavailable: one because on load, another because of "0" limit for computer files. 8. Check-in book from step 3 so it will be returned to the library, 9. Reload page on step 5 and see that again no any holds available, but it's now also correct: "book" now returned but "on shelf holds" set to "if all unavailable".
Created attachment 107075 [details] [review] Bug 24683: whole test formatted by 'perltidy' This is complementary patch using styling from bundled /xt/perltidyrc file Almost no code change except a few long constant strings broken to parts by concatenation.
Created attachment 107076 [details] [review] Bug 24683: IsAvailableForItemLevelRequest sub description expanded
(In reply to Jonathan Druart from comment #10) > Technical comments: > * perltidy a bit the tests done > * add tests in a subtest done > * AddReturn statement is not needed done > * it would be great to have IsAvailableForItemLevelRequest tested as well > (just in case, especially because we are lacking test coverage for this circ > rule! done + also because this I propose (last 107076 patch) some clarification in subroutine description (I agree that's tricky logic but I reversed up by calls through code and that how it made and looks expected this sub behavior, so I just decided to note this in sub desc). Also, Jonathan: this have one more integral update: optimization added to cut of loop if first available found: thanks for Joonas for the idea.
Also, we don't need the weird "Koha::Exceptions::UnknownProgramState" exception, C4::Context->interface() is always returning something.
Created attachment 107116 [details] [review] Bug 24683: Subroutine name changed (fix), no code logic changed This is the intermediate refactor: renamed subroutine only. Naming mistake came because this sub is used to detect if anything available for hold, but it used in "if ANY UNAVAILABLE rule", so actually results of this sub negated (see below "return" in the code). In details: when previous refactor was done, name for subroutine was chosen wrongly in "opposite" direction from what it actually does: it was named "ItemsAnyAvailableForHold", but this subroutine gave truth (1) if at least one of the items available on shelf, not lost, not on loan, not held, and not restricted by smart rules and damaged status. So, if this sub says that item is still "available", this actually PREVENTS item from hold in parent sub (see negated return): sub IsAvailableForItemLevelRequest { ... my $any_available = ItemsAnyAvailableAndNotRestricted... return $any_available ? 0 : 1; # ^^^ if any available and not restricted - we don't allow # on-shelf holds ... I.e. like it named now: "ItemsAnyAvailableAndNotRestricted". Small aside fix: white space for '&&' inside brackets added to join operation by priority visually. Testing plan not needed: all places where sub used it just renamed. More: all this places/code was introduced in one older commit so there is also no overlaps or other calls/uses for this subroutine. Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Created attachment 107117 [details] [review] Bug 24683: Optimize loop in ItemsAnyAvailableAndNotRestricted Add cut-off shortcut (return from inside the loop) when first "Any Available And Not Restricted" item found, because one is enough for "Any". Testing: no change visible for code behavior/results, it is just faster because won't loop over the whole set. Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Created attachment 107118 [details] [review] Bug 24683: Fix for take smart rules into account in "if all unavailable" Inside of ItemsAnyAvailableAndNotRestricted was no effect from main set of smart rules (per record and other limits): i.e. call to "CanItemBeReserved" was absent totally. Because of this there was a bug: for example none of two items were allowed to be held when first was allowed by one smart rule, BUT on loan, and second was disallowed by another smart rule (for example, 0 "Holds per record"), i.e. in this case both items unavailable: so on-shelf holds setting "allow hold if all unavailable" should allow to hold first one, and not the second one. But it was that both wasn't allowed to be held. Solution: call to sub "CanItemBeReserved" added so it checked for "...->{status} ne 'OK'" so now if item restricted by smart rule it also accounted as "unavailable" and "AnyAvailavble" not counts it. How to reproduce: 1. Add 2 smart rules (/cgi-bin/koha/admin/smart-rules.pl) with "on shelf holds": "if all unavailable" for all rules, no "item level holds", and set "holds per record" to 2 for "books" and "0" for "computer files". 2. Create only 2 items for one biblio, but different types, "book" and "computer file". For example in misc4dev env: /cgi-bin/koha/cataloguing/additem.pl?biblionumber=1#additem 3. Check out that item of type "book" to some person, for example, in misc4dev: /cgi-bin/koha/circ/circulation.pl?borrowernumber=2&barcode=3999900000001 4. Open reserve/request, for example, for item 1 and patron 1 in misc4dev env (/cgi-bin/koha/reserve/request.pl?biblionumber=1&borrowernumber=1) 5. It does not allow to hold, both red crossed, but computer file says "Exceeded max holds per record" because of "0" limit set on step 1. 6. Apply the patch. 7. Reload page on step 5 and see that "book" will be available for hold, but "computer file" still will be red-crossed "Exceeded max holds per record", now that's correct because both items unavailable: one because on load, another because of "0" limit for computer files. 8. Check-in book from step 3 so it will be returned to the library, 9. Reload page on step 5 and see that again no any holds available, but it's now also correct: "book" now returned but "on shelf holds" set to "if all unavailable". Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Created attachment 107119 [details] [review] Bug 24683: whole test formatted by 'perltidy' This is complementary patch using styling from bundled /xt/perltidyrc file Almost no code change except a few long constant strings broken to parts by concatenation. Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Created attachment 107120 [details] [review] Bug 24683: IsAvailableForItemLevelRequest sub description expanded Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Great work Andrew!
(In reply to Jonathan Druart from comment #21) > Also, we don't need the weird "Koha::Exceptions::UnknownProgramState" > exception, C4::Context->interface() is always returning something. UnknownProgramState? Where is it please advise, grep didn't get me to any results
(In reply to Agustín Moyano from comment #27) > Great work Andrew! Agustín: :safe_handshake: thanks!
(In reply to Andrew Nugged from comment #28) > (In reply to Jonathan Druart from comment #21) > > Also, we don't need the weird "Koha::Exceptions::UnknownProgramState" > > exception, C4::Context->interface() is always returning something. > > UnknownProgramState? Where is it please advise, grep didn't get me to any > results Forget that, wrong bug report!
Created attachment 108905 [details] [review] Bug 24683: Subroutine name changed (fix), no code logic changed This is the intermediate refactor: renamed subroutine only. Naming mistake came because this sub is used to detect if anything available for hold, but it used in "if ANY UNAVAILABLE rule", so actually results of this sub negated (see below "return" in the code). In details: when previous refactor was done, name for subroutine was chosen wrongly in "opposite" direction from what it actually does: it was named "ItemsAnyAvailableForHold", but this subroutine gave truth (1) if at least one of the items available on shelf, not lost, not on loan, not held, and not restricted by smart rules and damaged status. So, if this sub says that item is still "available", this actually PREVENTS item from hold in parent sub (see negated return): sub IsAvailableForItemLevelRequest { ... my $any_available = ItemsAnyAvailableAndNotRestricted... return $any_available ? 0 : 1; # ^^^ if any available and not restricted - we don't allow # on-shelf holds ... I.e. like it named now: "ItemsAnyAvailableAndNotRestricted". Small aside fix: white space for '&&' inside brackets added to join operation by priority visually. Testing plan not needed: all places where sub used it just renamed. More: all this places/code was introduced in one older commit so there is also no overlaps or other calls/uses for this subroutine. Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 108906 [details] [review] Bug 24683: Optimize loop in ItemsAnyAvailableAndNotRestricted Add cut-off shortcut (return from inside the loop) when first "Any Available And Not Restricted" item found, because one is enough for "Any". Testing: no change visible for code behavior/results, it is just faster because won't loop over the whole set. Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 108907 [details] [review] Bug 24683: Fix for take smart rules into account in "if all unavailable" Inside of ItemsAnyAvailableAndNotRestricted was no effect from main set of smart rules (per record and other limits): i.e. call to "CanItemBeReserved" was absent totally. Because of this there was a bug: for example none of two items were allowed to be held when first was allowed by one smart rule, BUT on loan, and second was disallowed by another smart rule (for example, 0 "Holds per record"), i.e. in this case both items unavailable: so on-shelf holds setting "allow hold if all unavailable" should allow to hold first one, and not the second one. But it was that both wasn't allowed to be held. Solution: call to sub "CanItemBeReserved" added so it checked for "...->{status} ne 'OK'" so now if item restricted by smart rule it also accounted as "unavailable" and "AnyAvailavble" not counts it. How to reproduce: 1. Add 2 smart rules (/cgi-bin/koha/admin/smart-rules.pl) with "on shelf holds": "if all unavailable" for all rules, no "item level holds", and set "holds per record" to 2 for "books" and "0" for "computer files". 2. Create only 2 items for one biblio, but different types, "book" and "computer file". For example in misc4dev env: /cgi-bin/koha/cataloguing/additem.pl?biblionumber=1#additem 3. Check out that item of type "book" to some person, for example, in misc4dev: /cgi-bin/koha/circ/circulation.pl?borrowernumber=2&barcode=3999900000001 4. Open reserve/request, for example, for item 1 and patron 1 in misc4dev env (/cgi-bin/koha/reserve/request.pl?biblionumber=1&borrowernumber=1) 5. It does not allow to hold, both red crossed, but computer file says "Exceeded max holds per record" because of "0" limit set on step 1. 6. Apply the patch. 7. Reload page on step 5 and see that "book" will be available for hold, but "computer file" still will be red-crossed "Exceeded max holds per record", now that's correct because both items unavailable: one because on load, another because of "0" limit for computer files. 8. Check-in book from step 3 so it will be returned to the library, 9. Reload page on step 5 and see that again no any holds available, but it's now also correct: "book" now returned but "on shelf holds" set to "if all unavailable". Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 108908 [details] [review] Bug 24683: whole test formatted by 'perltidy' This is complementary patch using styling from bundled /xt/perltidyrc file Almost no code change except a few long constant strings broken to parts by concatenation. Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 108909 [details] [review] Bug 24683: IsAvailableForItemLevelRequest sub description expanded Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Pushed to master for 20.11, thanks to everybody involved!
Awesome! Hope this can be backported to the stable versions as well.
I will port (also becase Hans Pålsson requested to have it on older version and I already did pre-test port for Hans earlier).
I can only give my humble thanks to all involved!
backported to 20.05.x for 20.05.04
missing dependencies, not backported to 19.11.x