Currently the 2 GetLatestAutoRenewDate and GetSoonestRenewDate subroutines takes $borrowernumber and $itemnumber in parameter. They refetch the patron, item and issue information but they already are available from where they are called. It would make sense to change the prototype of these 2 subroutines to accept the patron, item and issue information directly to avoid unnecessary refetches.
Created attachment 51100 [details] [review] Bug 16413: Change prototype GetLatestAutoRenewDate Currently the 2 GetLatestAutoRenewDate and GetSoonestRenewDate subroutines takes $borrowernumber and $itemnumber in parameter. They refetch the patron, item and issue information but they already are available from where they are called. It would make sense to change the prototype of these 2 subroutines to accept the patron, item and issue information directly to avoid unnecessary refetches. Test plan: Make sure this change does not introduce regressions on bug 15581 and bug 16344.
Created attachment 51101 [details] [review] Bug 16413: Change prototype of GetSoonestRenewDate Same change as previous patch for the GetSoonestRenewDate subroutine. Test plan: Make sure this change does not introduce regressions on bug 7413.
I think this one needs rebasing
(In reply to Srdjan Jankovic from comment #3) > I think this one needs rebasing Apply cleanly on top of dependencies (bug 15581 and bug 16344).
Comment on attachment 51101 [details] [review] Bug 16413: Change prototype of GetSoonestRenewDate Review of attachment 51101 [details] [review]: ----------------------------------------------------------------- ::: circ/renew.pl @@ +78,5 @@ > > if ( $error && ($error eq 'too_soon' or $error eq 'auto_too_soon') ) { > $soonest_renew_date = C4::Circulation::GetSoonestRenewDate( > + { > + borrower => $borrower->unblessed, shouldn't this be: patron => $borrower->unblessed, ?
Comment on attachment 51100 [details] [review] Bug 16413: Change prototype GetLatestAutoRenewDate Review of attachment 51100 [details] [review]: ----------------------------------------------------------------- ::: circ/renew.pl @@ +84,5 @@ > } > if ( $error && ( $error eq 'auto_too_late' ) ) { > $latest_auto_renew_date = C4::Circulation::GetLatestAutoRenewDate( > + { > + borrower => $borrower->unblessed, shouldn't this be: patron => $borrower->unblessed, ?
Created attachment 58671 [details] [review] Bug 16413: Change prototype of GetLatestAutoRenewDate Currently the 2 GetLatestAutoRenewDate and GetSoonestRenewDate subroutines takes $borrowernumber and $itemnumber in parameter. They refetch the patron, item and issue information but they already are available from where they are called. It would make sense to change the prototype of these 2 subroutines to accept the patron, item and issue information directly to avoid unnecessary refetches. Test plan: Make sure this change does not introduce regressions on bug 15581 and bug 16344.
Created attachment 58672 [details] [review] Bug 16413: Change prototype of GetSoonestRenewDate Same change as previous patch for the GetSoonestRenewDate subroutine. Test plan: Make sure this change does not introduce regressions on bug 7413.
Created attachment 61514 [details] [review] Bug 16413: Change prototype of GetLatestAutoRenewDate Currently the 2 GetLatestAutoRenewDate and GetSoonestRenewDate subroutines takes $borrowernumber and $itemnumber in parameter. They refetch the patron, item and issue information but they already are available from where they are called. It would make sense to change the prototype of these 2 subroutines to accept the patron, item and issue information directly to avoid unnecessary refetches. Test plan: Make sure this change does not introduce regressions on bug 15581 and bug 16344. Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Created attachment 61515 [details] [review] Bug 16413: Change prototype of GetSoonestRenewDate Same change as previous patch for the GetSoonestRenewDate subroutine. Test plan: Make sure this change does not introduce regressions on bug 7413. Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Acting as git gopher to Jonathan Field who did the testing and actual signoff.
(In reply to Jonathan Druart from comment #7) > Test plan: > Make sure this change does not introduce regressions on bug 15581 and bug > 16344. Test plan?
(In reply to Marcel de Rooy from comment #12) > (In reply to Jonathan Druart from comment #7) > > Test plan: > > Make sure this change does not introduce regressions on bug 15581 and bug > > 16344. > > Test plan? Could you be more verbose please, I do not understand the question. Yes the test plan is to test that the patches from bugs 15581 and 16344 are still ok. At the moment of writing, none was pushed and so the goal was to test it on top of them and QA them at the same time.
Calls to GetSoonestRenewDate haven't been updated in opac-user.pl and svc/checkouts at the very least.
Created attachment 65405 [details] [review] Bug 16413: Change prototype of GetLatestAutoRenewDate Currently the 2 GetLatestAutoRenewDate and GetSoonestRenewDate subroutines takes $borrowernumber and $itemnumber in parameter. They refetch the patron, item and issue information but they already are available from where they are called. It would make sense to change the prototype of these 2 subroutines to accept the patron, item and issue information directly to avoid unnecessary refetches.
Created attachment 65406 [details] [review] Bug 16413: Change prototype of GetSoonestRenewDate Same change as previous patch for the GetSoonestRenewDate subroutine. This is not finished, 2 other calls could not be updated (performance issue). They may be later, when objects will be used in svc/checkouts and opac-user.pl
Jonathan, do you want to keep this one open or should we close it MOVED as it should be fixed within the Koha namespace?
I will take care of this when it will be possible, when svc/checkouts and opac-user.pl will use Koha::Checkout objects.
(In reply to Jonathan Druart from comment #18) > I will take care of this when it will be possible, when svc/checkouts > and opac-user.pl will use Koha::Checkout objects. That's what I was thinking - because we will probably kill those off anyway :)