Bug 9367 - Code optimization: CheckReserves is too often called
Summary: Code optimization: CheckReserves is too often called
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 9761
  Show dependency treegraph
 
Reported: 2013-01-08 15:49 UTC by Jonathan Druart
Modified: 2014-05-26 21:04 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9367: Code optimization: CheckReserves is too often called (9.85 KB, patch)
2013-01-08 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9367: Code optimization: CheckReserves is too often called (9.90 KB, patch)
2013-01-11 19:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9367: Followup: Code optimization: CheckReserves is too often called (2.48 KB, patch)
2013-03-11 10:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9367: Followup: Code optimization: CheckReserves is too often called (3.04 KB, patch)
2013-03-11 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9367: Code optimization: CheckReserves is too often called (10.00 KB, patch)
2013-03-11 12:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9367: Followup: Code optimization: CheckReserves is too often called (3.13 KB, patch)
2013-03-11 12:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9367: Followup finalizing QA (1.98 KB, patch)
2013-03-11 12:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-01-08 15:49:40 UTC
In many places, the C4::Reserves::CheckReserves routine is called when we just want to get the status of the reserve.
Comment 1 Jonathan Druart 2013-01-08 15:59:04 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-01-11 19:22:08 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2013-03-11 09:52:49 UTC
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..
Comment 4 Jonathan Druart 2013-03-11 10:32:13 UTC
(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...
Comment 5 Jonathan Druart 2013-03-11 10:34:05 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2013-03-11 10:37:24 UTC
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..
Comment 7 Marcel de Rooy 2013-03-11 10:41:29 UTC
(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.
Comment 8 Marcel de Rooy 2013-03-11 10:45:47 UTC
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.
Comment 9 Jonathan Druart 2013-03-11 11:05:25 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2013-03-11 11:06:16 UTC
The last patch fixes all your comments.
Comment 11 Marcel de Rooy 2013-03-11 12:10:02 UTC
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>
Comment 12 Marcel de Rooy 2013-03-11 12:10:06 UTC
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>
Comment 13 Marcel de Rooy 2013-03-11 12:10:11 UTC
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.
Comment 14 Marcel de Rooy 2013-03-11 12:11:18 UTC
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
Comment 15 Jared Camins-Esakov 2013-03-16 15:53:07 UTC
This patch has been pushed to master.