http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9367 --- Comment #1 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 14471 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=14471&action=edit 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9367 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |jonathan.druart@biblibre.co |ity.org |m -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9367 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #14471|0 |1 is obsolete| | --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 14544 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=14544&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9367 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl QA Contact| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9367 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #4 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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... -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9367 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #6 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9367 --- Comment #8 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9367 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #10 from Jonathan Druart <jonathan.druart@biblibre.com> --- The last patch fixes all your comments. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9367 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #14544|0 |1 is obsolete| | Attachment #16018|0 |1 is obsolete| | --- Comment #11 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 16021 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=16021&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9367 --- Comment #13 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 16023 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=16023&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9367 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |9788 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9367 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #15 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- This patch has been pushed to master. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9367 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=10663 -- You are receiving this mail because: You are watching all bug changes.