Description
Jonathan Druart
2013-03-15 12:47:49 UTC
Created attachment 16162 [details] [review] Bug 9823: Code optimization: Change prototype of GetReservesFromBiblionumber The prototype of the GetReservesFromBiblionumber routine is quite weird. It is useless to return the number of reserves and an array containing reserves. The $count variable can be calculate with scalar(@$reserves). Does not apply against master. Jonathan could you rebase? Also could we change patch name to something like Refactor return from GetReservesFromBiblionumber as prototypes are not involved? Created attachment 16236 [details] [review] Bug 9823: Refactor return from GetReservesFromBiblionumber as prototypes are not involved The prototype of the GetReservesFromBiblionumber routine is quite weird. It is useless to return the number of reserves and an array containing reserves. The $count variable can be calculate with scalar(@$reserves). Created attachment 16237 [details] [review] Bug 9823: Refactor return from GetReservesFromBiblionumber as prototypes are not involved The prototype of the GetReservesFromBiblionumber routine is quite weird. It is useless to return the number of reserves and an array containing reserves. The $count variable can be calculate with scalar(@$reserves). Created attachment 16256 [details] [review] Amended Patch Patch as amended still did not apply to master, was rejected because it made changes to a subroutine GetReserveNextRank which is not present in master. Have removed the offending line from the patch, modified version attached. I also took the liberty of altering the commit message to clarify what the patch addresses (In reply to comment #5) > Created attachment 16256 [details] [review] [review] > Amended Patch > > Patch as amended still did not apply to master, was rejected because it made > changes to a subroutine GetReserveNextRank which is not present in master. > Have removed the offending line from the patch, modified version attached. > I also took the liberty of altering the commit message to clarify what the > patch addresses Hi Colin, I marked this patch dependent on 8918 because it introduces a call to GetReservesFromBiblionumber. Ok I'll re(In reply to comment #6) > (In reply to comment #5) > > Created attachment 16256 [details] [review] [review] [review] > > Amended Patch > > > > Patch as amended still did not apply to master, was rejected because it made > > changes to a subroutine GetReserveNextRank which is not present in master. > > Have removed the offending line from the patch, modified version attached. > > I also took the liberty of altering the commit message to clarify what the > > patch addresses > > Hi Colin, > I marked this patch dependent on 8918 because it introduces a call to > GetReservesFromBiblionumber. OK I'll retest when that has been pushed, otherwise it looks ready for signoff I'm setting this to "BLOCKED" because it is waiting for another patch to be pushed. Created attachment 22706 [details] [review] Bug 9823: Refactor return from GetReservesFromBiblionumber The return from GetReservesFromBiblionumber contains an unnecessary extra variable. In scalar context an array returns its element count. Maintaing a separate count can lead to unforeseen bugs and imposes ugly constructions on the subroutine's users. Remove the useless count variable from the return This patch also changes the parameters: now the routine takes a hashref. I remove the dependencies on bug 8918. Removing dependency on pushed 9788. Will look further next week. Still applies. Hi Jonathan, Am I wrong if I conclude that you are assigning an arrayref to $count? Quick fix? :) diff --git a/acqui/parcel.pl b/acqui/parcel.pl index ad0e51d..d374fa0 100755 --- a/acqui/parcel.pl +++ b/acqui/parcel.pl @@ -174,7 +174,7 @@ for my $order ( @orders ) { $line{holds} = 0; my @itemnumbers = GetItemnumbersFromOrder( $order->{ordernumber} ); for my $itemnumber ( @itemnumbers ) { - my ( $count ) = &GetReservesFromBiblionumber($line{biblionumber}, undef, $itemnumber); + my ( $count ) = &GetReservesFromBiblionumber({ biblionumber => $line{biblionumber}, itemnumber => $itemnumber }); Created attachment 24653 [details] [review] Bug 9823: Refactor return from GetReservesFromBiblionumber The return from GetReservesFromBiblionumber contains an unnecessary extra variable. In scalar context an array returns its element count. Maintaing a separate count can lead to unforeseen bugs and imposes ugly constructions on the subroutine's users. Remove the useless count variable from the return This patch also changes the parameters: now the routine takes a hashref. (In reply to M. de Rooy from comment #12) > Hi Jonathan, > Am I wrong if I conclude that you are assigning an arrayref to $count? > Quick fix? :) Yes, done! I also changed new occurrences of GetReservesFromBiblionumber in t/db_dependent/Holds.t (In reply to Jonathan Druart from comment #14) > Yes, done! > I also changed new occurrences of GetReservesFromBiblionumber in > t/db_dependent/Holds.t Great. Have been testing some already. Looks good until now. But I cannot finish it today anymore. Will soon pick it up again.. Created attachment 24668 [details] [review] Bug 9823: Refactor return from GetReservesFromBiblionumber The return from GetReservesFromBiblionumber contains an unnecessary extra variable. In scalar context an array returns its element count. Maintaing a separate count can lead to unforeseen bugs and imposes ugly constructions on the subroutine's users. Remove the useless count variable from the return This patch also changes the parameters: now the routine takes a hashref. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Placed biblio holds, future holds and item holds. Works as expected. Tested Holds.t and Reserves.t. Pass. Tested /cgi-bin/koha/ilsdi.pl?service=GetRecords&id=999 with two holds on one item. Fine. C4/SIP/ILS/Item.pm: Looked for "whatever" and "arrayref" and could not find them anymore. Looks good. Handled a few unneeded calls in QA follow-up. Left only one point to-do for serials/routing-preview.pl. See Bugzilla. Created attachment 24669 [details] [review] Bug 9823: QA follow-up for GetReservesFromBiblionumber calls The template params holds and holdcount are not used in the ISBD template. Removed the associated code from catalogue/ISBDdetail.pl. Same applies for catalogue/MARCdetail.pl and labeledMARCdetail.pl. Same applies also for catalogue/imageviewer.pl. Same applies also for catalogue/moredetail.pl. In catalogue detail.tt only the number of holds is used. Removed the code that passed the holds array to the template. For consistency opac-detail should also incorporate future holds into its holds count; added the all_dates parameter for that reason. The Reserves module is no longer needed in moredetail. Removed it. Checked the other scripts also. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Comments from testing (some preliminary QA comments for another QAer): Tested /cgi-bin/koha/ilsdi.pl?service=GetRecords&id=999 with two holds on one item. This attracted my attention: - $biblioitem->{'reserves'}->{'reserve'} = $reserves[1]; + $biblioitem->{'reserves'}->{'reserve'} = $reserves; But it is okay: $reserves[1] was previously the arrayref. So this is a code readability improvement too :) With and without patch I see my two holds in the output. Tested Holds.t and Reserves.t. Pass. Placed biblio holds, future holds and item holds. Works as expected (small note below on opac-detail). C4/SIP/ILS/Item.pm: Looked for "whatever" and "arrayref" and could not find them anymore. OK Template params holds and holdcount are not used in the ISBD template; removed them from catalogue/ISBDdetail.pl in QA followup Same for MARCdetail.pl, imageviewer.pl, labeledMARCdetail.pl and moredetail.pl. Removed use Reserves from moredetail (no longer 'in use'.) In catalogue detail.tt only the number of holds is used. Removed the code that passed the holds array to the template. Tested various settings of OPACShowHoldQueueDetails and corresponding display in opac-detail. For consistency I added the all_dates parameter to the call in opac-detail to incorporate future holds in its count. I just note that the display of count and priority could be improved here but I am already at the scope borders of this report :) [The meaning of something like Total holds: 3 (priority 2) under the items list is rather obscure..] serials/routing-preview.pl: I have the strong feeling that this call of GetReservesFromBiblionumber also needs the all_dates parameter. I have not tested this call (there is no test plan for it too :), so I did not want to touch it now. If you could still have a look, Jonathan, perhaps you could still add that one in a follow-up? No complaints from koha-qa.pl. Jonathan: the remark on routing-preview.pl actually applies too for: ILSDI/Services.pm C4/Biblio.pm (This one handles deleting reserves, so future ones should be cancelled too) C4/SIP/ILS/Item.pm acqui/parcel.pl Although you could ignore future holds in some counts etc., it makes me wonder if we still need the all_dates parameter and not just include them Always? Created attachment 24690 [details] [review] Bug 9823: Refactor return from GetReservesFromBiblionumber The return from GetReservesFromBiblionumber contains an unnecessary extra variable. In scalar context an array returns its element count. Maintaing a separate count can lead to unforeseen bugs and imposes ugly constructions on the subroutine's users. Remove the useless count variable from the return This patch also changes the parameters: now the routine takes a hashref. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Placed biblio holds, future holds and item holds. Works as expected. Tested Holds.t and Reserves.t. Pass. Tested /cgi-bin/koha/ilsdi.pl?service=GetRecords&id=999 with two holds on one item. Fine. C4/SIP/ILS/Item.pm: Looked for "whatever" and "arrayref" and could not find them anymore. Looks good. Handled a few unneeded calls in QA follow-up. Left only one point to-do for serials/routing-preview.pl. See Bugzilla. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 24691 [details] [review] Bug 9823: QA follow-up for GetReservesFromBiblionumber calls The template params holds and holdcount are not used in the ISBD template. Removed the associated code from catalogue/ISBDdetail.pl. Same applies for catalogue/MARCdetail.pl and labeledMARCdetail.pl. Same applies also for catalogue/imageviewer.pl. Same applies also for catalogue/moredetail.pl. In catalogue detail.tt only the number of holds is used. Removed the code that passed the holds array to the template. For consistency opac-detail should also incorporate future holds into its holds count; added the all_dates parameter for that reason. The Reserves module is no longer needed in moredetail. Removed it. Checked the other scripts also. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Pushed to master, along with a follow-up that improves the POD for GetReservesFromBiblionumber. Thanks, Jonathan! Note bug 11643. Pushed to 3.14.x, will be in 3.14.07 (I've backported this enhancement to be able to backport 11829) |