Bug 9823 - Code optimization: Change prototype of GetReservesFromBiblionumber
Summary: Code optimization: Change prototype of GetReservesFromBiblionumber
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 11829
  Show dependency treegraph
 
Reported: 2013-03-15 12:47 UTC by Jonathan Druart
Modified: 2015-06-04 23:30 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9823: Code optimization: Change prototype of GetReservesFromBiblionumber (10.65 KB, patch)
2013-03-15 12:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9823: Refactor return from GetReservesFromBiblionumber as prototypes are not involved (10.85 KB, patch)
2013-03-18 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9823: Refactor return from GetReservesFromBiblionumber as prototypes are not involved (10.66 KB, patch)
2013-03-18 12:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Amended Patch (10.25 KB, patch)
2013-03-18 14:56 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 9823: Refactor return from GetReservesFromBiblionumber (12.23 KB, patch)
2013-11-04 13:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9823: Refactor return from GetReservesFromBiblionumber (13.92 KB, patch)
2014-01-23 13:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9823: Refactor return from GetReservesFromBiblionumber (14.42 KB, patch)
2014-01-24 10:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9823: QA follow-up for GetReservesFromBiblionumber calls (5.73 KB, patch)
2014-01-24 10:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9823: Refactor return from GetReservesFromBiblionumber (14.44 KB, patch)
2014-01-24 14:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9823: QA follow-up for GetReservesFromBiblionumber calls (5.75 KB, patch)
2014-01-24 14:04 UTC, Kyle M Hall
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-03-15 12:47:49 UTC
GetReservesFromBiblionumber returns two values while one is enough.
Comment 1 Jonathan Druart 2013-03-15 12:49:26 UTC Comment hidden (obsolete)
Comment 2 Colin Campbell 2013-03-18 11:50:53 UTC
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?
Comment 3 Jonathan Druart 2013-03-18 12:32:11 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-03-18 12:38:31 UTC Comment hidden (obsolete)
Comment 5 Colin Campbell 2013-03-18 14:56:03 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2013-03-18 15:03:03 UTC
(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.
Comment 7 Colin Campbell 2013-03-18 16:11:50 UTC
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
Comment 8 Magnus Enger 2013-03-20 14:11:39 UTC
I'm setting this to "BLOCKED" because it is waiting for another patch to be pushed.
Comment 9 Jonathan Druart 2013-11-04 13:53:48 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2013-11-04 13:54:15 UTC
I remove the dependencies on bug 8918.
Comment 11 Marcel de Rooy 2014-01-17 17:17:31 UTC
Removing dependency on pushed 9788. Will look further next week.
Still applies.
Comment 12 Marcel de Rooy 2014-01-23 12:25:45 UTC
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 });
Comment 13 Jonathan Druart 2014-01-23 13:15:18 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2014-01-23 13:15:57 UTC
(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
Comment 15 Marcel de Rooy 2014-01-23 14:20:42 UTC
(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..
Comment 16 Marcel de Rooy 2014-01-24 10:39:17 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2014-01-24 10:39:26 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2014-01-24 10:40:56 UTC
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.
Comment 19 Marcel de Rooy 2014-01-24 10:46:19 UTC
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?
Comment 20 Kyle M Hall 2014-01-24 14:04:27 UTC
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>
Comment 21 Kyle M Hall 2014-01-24 14:04:44 UTC
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>
Comment 22 Galen Charlton 2014-01-30 16:50:05 UTC
Pushed to master, along with a follow-up that improves the POD for GetReservesFromBiblionumber.  Thanks, Jonathan!
Comment 23 Galen Charlton 2014-01-30 16:54:28 UTC
Note bug 11643.
Comment 24 Fridolin Somers 2014-05-27 10:21:09 UTC
Pushed to 3.14.x, will be in 3.14.07

(I've backported this enhancement to be able to backport 11829)