Bug 24683

Summary: Holds on biblios with different item types: rules for holds allowed are not applied correctly if any item type is available
Product: Koha Reporter: Hans Pålsson <hans.palsson>
Component: Hold requestsAssignee: Andrii Nugged <nugged>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: agustinmoyano, aleisha, andreas.jonsson, andrewfh, clodagh.kerin, gmcharlt, jonathan.druart, katrin.fischer, lisettepalouse+koha, lucas, magnus, nick, nugged, slavashishkin, stalkernoid
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24185
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24160
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28529
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.04
Attachments: An example of a biblio with mixed itemtype items.
Bug 24683: Add description of QueryFuzzy support in ES to syspref description
Bug 24683: Subroutine name changed (fix), no code logic changed This is the intermediate refactor: renamed subroutine only.
Bug 24683: Fix for take smart rules into account in "if all unavailable"
Bug 24683: Subroutine name changed (fix), no code logic changed This is the intermediate refactor: renamed subroutine only.
Bug 24683: Optimize loop in ItemsAnyAvailableAndNotRestricted
Bug 24683: Fix for take smart rules into account in "if all unavailable"
Bug 24683: whole test formatted by 'perltidy'
Bug 24683: IsAvailableForItemLevelRequest sub description expanded
Bug 24683: Subroutine name changed (fix), no code logic changed This is the intermediate refactor: renamed subroutine only.
Bug 24683: Optimize loop in ItemsAnyAvailableAndNotRestricted
Bug 24683: Fix for take smart rules into account in "if all unavailable"
Bug 24683: whole test formatted by 'perltidy'
Bug 24683: IsAvailableForItemLevelRequest sub description expanded
Bug 24683: Subroutine name changed (fix), no code logic changed This is the intermediate refactor: renamed subroutine only.
Bug 24683: Optimize loop in ItemsAnyAvailableAndNotRestricted
Bug 24683: Fix for take smart rules into account in "if all unavailable"
Bug 24683: whole test formatted by 'perltidy'
Bug 24683: IsAvailableForItemLevelRequest sub description expanded

Description Hans Pålsson 2020-02-19 16:19:32 UTC
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.
Comment 1 Hans Pålsson 2020-02-28 13:10:49 UTC
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.
Comment 2 Nick Clemens 2020-03-18 14:37:05 UTC Comment hidden (obsolete)
Comment 3 Magnus Enger 2020-03-18 14:38:29 UTC Comment hidden (obsolete)
Comment 4 Hans Pålsson 2020-03-20 07:43:04 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2020-04-05 13:02:49 UTC
Changing to NEW as there is no patch and no assignee. Please update if you plan to work on this!
Comment 6 Hans Pålsson 2020-04-06 07:22:21 UTC
Thank you Katrin. Yes, we need a solution in some way, will continue working on this.
Comment 7 Hans Pålsson 2020-05-13 13:48:53 UTC
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;
}
Comment 8 Andrii Nugged 2020-06-08 22:09:32 UTC Comment hidden (obsolete)
Comment 9 Andrii Nugged 2020-06-08 22:10:28 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2020-07-03 09:59:47 UTC
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.
Comment 11 Katrin Fischer 2020-07-05 00:16:40 UTC Comment hidden (obsolete)
Comment 12 Andrii Nugged 2020-07-05 00:21:05 UTC Comment hidden (obsolete)
Comment 13 Andrii Nugged 2020-07-05 00:22:10 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2020-07-05 00:23:09 UTC Comment hidden (obsolete)
Comment 15 Andrii Nugged 2020-07-19 21:29:06 UTC
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.
Comment 16 Andrii Nugged 2020-07-19 21:29:37 UTC
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.
Comment 17 Andrii Nugged 2020-07-19 21:29:55 UTC
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".
Comment 18 Andrii Nugged 2020-07-19 21:30:16 UTC
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.
Comment 19 Andrii Nugged 2020-07-19 21:31:10 UTC
Created attachment 107076 [details] [review]
Bug 24683: IsAvailableForItemLevelRequest sub description expanded
Comment 20 Andrii Nugged 2020-07-19 21:37:06 UTC
(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.
Comment 21 Jonathan Druart 2020-07-20 13:26:46 UTC Comment hidden (obsolete)
Comment 22 Agustín Moyano 2020-07-20 15:28:26 UTC
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>
Comment 23 Agustín Moyano 2020-07-20 15:28:30 UTC
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>
Comment 24 Agustín Moyano 2020-07-20 15:28:34 UTC
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>
Comment 25 Agustín Moyano 2020-07-20 15:28:39 UTC
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>
Comment 26 Agustín Moyano 2020-07-20 15:28:44 UTC
Created attachment 107120 [details] [review]
Bug 24683: IsAvailableForItemLevelRequest sub description expanded

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 27 Agustín Moyano 2020-07-20 15:29:27 UTC
Great work Andrew!
Comment 28 Andrii Nugged 2020-07-20 16:03:31 UTC Comment hidden (obsolete)
Comment 29 Andrii Nugged 2020-07-20 16:03:57 UTC
(In reply to Agustín Moyano from comment #27)
> Great work Andrew!

Agustín: :safe_handshake: thanks!
Comment 30 Jonathan Druart 2020-07-20 16:37:07 UTC Comment hidden (obsolete)
Comment 31 Katrin Fischer 2020-08-22 11:30:06 UTC
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>
Comment 32 Katrin Fischer 2020-08-22 11:30:11 UTC
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>
Comment 33 Katrin Fischer 2020-08-22 11:30:16 UTC
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>
Comment 34 Katrin Fischer 2020-08-22 11:30:21 UTC
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>
Comment 35 Katrin Fischer 2020-08-22 11:30:26 UTC
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>
Comment 36 Jonathan Druart 2020-08-24 10:32:48 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 37 Magnus Enger 2020-08-24 11:16:49 UTC
Awesome! Hope this can be backported to the stable versions as well.
Comment 38 Andrii Nugged 2020-08-24 11:19:09 UTC
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).
Comment 39 Hans Pålsson 2020-08-24 11:50:50 UTC
I can only give my humble thanks to all involved!
Comment 40 Lucas Gass 2020-09-04 17:54:16 UTC
backported to 20.05.x for 20.05.04
Comment 41 Aleisha Amohia 2020-09-10 03:59:37 UTC
missing dependencies, not backported to 19.11.x