Created attachment 69382 [details] [review]
Bug 19690: Smart rules: Term "If any unavailable" is confusing and just means No

This patch replaces the term by No.
Also "If all unavailable" is refined to "Only if other items unavailable".
The online help text is adjusted.
Two comments in DisallowHoldIfItemsAvailable.t are adjusted too.

Test plan:
Go to smart rules, look at on shelf holds.
Read the help text for this page.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

I think the patch is actually wrong:

[% ELSIF rule.onshelfholds == 2 %]
- If all unavailable
+ Only if other items unavailable
[% ELSE %]
-  If any unavailable
+  No

OnShelfHolds Allowed
If all unavailable = No (no onshelf holds are allowed)
If any anavailable = If at least one is checked out
Yes = It doesn't matter if anything is checked out or not (also yes is good here)

Please have a look at the code in IsAvailableForItemLevelRequest:

    } elsif ( $on_shelf_holds == 2 ) {
        my @items = ...
        my $any_available = 0;
        foreach my $i (@items) {
...
            $any_available = 1
              unless $i->itemlost
...
        }
        return $any_available ? 0 : 1;
    }

The variable name could put you on the wrong track, but 2 == all_unavailable.
What does the above say? If you choose for all_unavailable and there is at lease one item available ("On shelf"), you cannot place the hold. If all are unavailable, you can place the item level hold.

Finally, we have the remaining case (on_shelf_holds == 0 /"If any unavailable"):

    return $item->{onloan} || GetReserveStatus($item->{itemnumber}) eq "Waiting"

The item level hold is only allowed now if the current item is on loan or waiting. (Using the term "any" here is making no sense to me..)

===
What makes it more complicated, is the use of the sub OnShelfHoldsAllowed in Koha (opac-detail, opac-shelves). In opac-detail-sidebar.inc the following test is used before displaying a place hold link:
[% IF ( AllowOnShelfHolds OR ItemsIssued ) %]
opac-shelves does more or less the same by first testing allow_onshelf_holds and testing if issued in the else branch.
Note that in these tests the values 1 and 2 are just interpreted as one case (shelf holds are allowed).

===
Conclusion: The name is confusing/misleading. This is partially hidden by the obscure use of onshelfholds in other places. We cannot solve this by a rename only. So I am leaving it as-is.

Sorry, you lost me :(

(In reply to Katrin Fischer from comment #4)
> Sorry, you lost me :(

Well, this confirms that the term confusing is quite correct! :)

Would "If on loan" make more sense? That seems to be what is actually checked by the code.

(In reply to Magnus Enger from comment #6)
> Would "If on loan" make more sense? That seems to be what is actually
> checked by the code.

Not quite, because it doesn't transport that it will take effect if at least one is on loan.

Maybe:

If at one or more are checked out ?

(In reply to Katrin Fischer from comment #7)
> Not quite, because it doesn't transport that it will take effect if at least
> one is on loan.

Ah, but that is not how the code in C4::Reserves::IsAvailableForItemLevelRequest() works, I think: 

1163 sub IsAvailableForItemLevelRequest {
1164     my $item = shift;
1165     my $borrower = shift;
1166     my $pickup_branchcode = shift;
...
1196     if ( $on_shelf_holds == 1 ) { # Yes
1197         return 1;
1198     } elsif ( $on_shelf_holds == 2 ) { # If all unavailable
1199         my @items =
1200           Koha::Items->search( { biblionumber => $item->{biblionumber} } );
1201 
1202         my $any_available = 0;
1203 
1204         foreach my $i (@items) {
1205 
...
1221         return $any_available ? 0 : 1;
1222     } else { # on_shelf_holds == 0 "If any unavailable" (the description is rather cryptic and could still be improved)
1223         return $item->{onloan} || IsItemOnHoldAndFound( $item->{itemnumber} );
1224     }
1225 }

If on_shelf_holds == 1 it always returns 1, the item that was passed in can always be be put a hold on.

If on_shelf_holds == 2 it loops over all the items attached to the biblio of the item that was passed in, so it can check if they all are unavailable or not, and if any are available it returns 0. (There is a bug in how this is done when holdallowed = 0 that I will report and submit a patch for soon.)

If on_shelf_holds == 0 it only checks the item that was passed in, there is no looping over all the items to tell if at least one is onloan. (There is a small bug in how this is done too...) So what the code does is look at the item that was passed in to IsAvailableForItemLevelRequest and if it is onloan (or OnHoldAndFound) it says it is OK to place a hold on that item. That is why I think "If on loan" might make sense. Now this might not be what was intended for the "If any unavailable" setting, but then the code is wrong, IMHO.

Hm, I am not sure about the code sample, but I am pretty sure that's the behaviour of you have onshelfholds = 0 as this was the way it used to work before 2 was introduced. Maybe the check is done somewhere else to determine biblio-level-holdability?