QA Comment:
This looks quite good. Code improvement! koha-qa is happy too :)
A few minor details needing some attention:

1) Circ   $error->{message} = "on_reserve"; Should be just $error= "on_reserve"

2) GetReserveStatus needs some error checking to prevent warnings on $found/$priority at the end (if there is no reserve) 

3) IsAvailableForItemLevelRequest: the check GetReserveStatus($itemnumber) eq "W" is replaced by $status eq "Waiting" or $status = "Reserved"
  Please clarify. As I understand, AllowOnShelfHolds is OFF here. So the second part of the check should be: AND Is it on loan or waiting (current behavior)? 
  You could even argue that you should test only for on loan.. 
  Why do you add Reserved?

Please send a followup. I will gladly sign off on that..

(In reply to comment #3)
> QA Comment:

Hello Marcel,

> This looks quite good. Code improvement! koha-qa is happy too :)
> A few minor details needing some attention:
> 
> 1) Circ   $error->{message} = "on_reserve"; Should be just $error=
> "on_reserve"

Yep, good catch! Will be fixed.

> 2) GetReserveStatus needs some error checking to prevent warnings on
> $found/$priority at the end (if there is no reserve) 

Will be fixed too.

> 3) IsAvailableForItemLevelRequest: the check GetReserveStatus($itemnumber)
> eq "W" is replaced by $status eq "Waiting" or $status = "Reserved"
>   Please clarify. As I understand, AllowOnShelfHolds is OFF here. So the
> second part of the check should be: AND Is it on loan or waiting (current
> behavior)? 
>   You could even argue that you should test only for on loan.. 
>   Why do you add Reserved?

You are right too, I don't find any reason. The test should only be on "Waiting".

> Please send a followup. I will gladly sign off on that..

It's coming...

There is another error too in GetReserveStatus.
If you pass an itemnumber and no biblionumber, you always return undef.

You should put the biblionumber stuff in the else part of the itemnumber test..

(In reply to comment #6)
> There is another error too in GetReserveStatus.
> If you pass an itemnumber and no biblionumber, you always return undef.
> 
> You should put the biblionumber stuff in the else part of the itemnumber
> test..
Do this not too literally :) You will understand hopefully.

This should work including testing uninitialized:

    if ( $itemnumber ) {
        $sth = $dbh->prepare("SELECT found, priority FROM reserves WHERE itemnumber = ? order by priority LIMIT 1");
        $sth->execute($itemnumber);
        ($found, $priority) = $sth->fetchrow_array;
    }
    if ( $biblionumber and not defined $found and not defined $priority ) {
        $sth = $dbh->prepare("SELECT found, priority FROM reserves WHERE biblionumber = ? order by priority LIMIT 1");
        $sth->execute($biblionumber);
       ($found, $priority) = $sth->fetchrow_array;
    }

    return if not defined $found;
etc.

The last patch fixes all your comments.

Created attachment 16021 [details] [review]
Bug 9367: Code optimization: CheckReserves is too often called

This patch rewrites the GetReserveStatus routine in order to take in
parameter the itemnumber and/or the biblionumber.

In some places, the C4::Reserves::CheckReserves routine is called when
we just want to get the status of the reserve. In these cases, the
C4::Reserves::GetReserveStatus is now called.
This routine executes 1 sql query (or 2 max).

Test plan:
Check that there is no regression on the different pages where reserves
are used. The different status will be the same than before applying
this patch.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 16022 [details] [review]
Bug 9367: Followup: Code optimization: CheckReserves is too often called

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

Created attachment 16023 [details] [review]
Bug 9367: Followup finalizing QA

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Keeping fetchrow close to its execute worked even better in GetReserveStatus.
Instead of returning undef, I return empty string.
I checked all calls; this value is mostly not checked for undef.
So we eliminate a lot of warnings in log.

Final QA Comments:
Keeping fetchrow close to its execute worked even better in GetReserveStatus.
Instead of returning undef, I return empty string.
I checked all calls; this value is mostly not checked for undef.
So we eliminate a lot of warnings in log.
Instead of harassing Jonathan again, I added a followup myself. 

From opac include item-status.inc:
[% ELSIF ( item.waiting ) %]
    On hold
Funny thing: If you set waiting=1, the message is On hold. If you set onhold=1 (for a pending reserve), the variable is not used.
Conclusion is probably that the line in opac-detail setting var onhold, is not needed..
Can be handled later or somewhere else..

Passed QA

This patch has been pushed to master.