Bug 16413 - Prototype for GetLatestAutoRenewDate and GetSoonestRenewDate should be changed
Summary: Prototype for GetLatestAutoRenewDate and GetSoonestRenewDate should be changed
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 16344
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-02 13:49 UTC by Jonathan Druart
Modified: 2020-01-13 13:00 UTC (History)
7 users (show)

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


Attachments
Bug 16413: Change prototype GetLatestAutoRenewDate (8.34 KB, patch)
2016-05-02 13:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16413: Change prototype of GetSoonestRenewDate (6.17 KB, patch)
2016-05-02 13:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16413: Change prototype of GetLatestAutoRenewDate (8.21 KB, patch)
2017-01-09 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16413: Change prototype of GetSoonestRenewDate (5.24 KB, patch)
2017-01-09 08:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16413: Change prototype of GetLatestAutoRenewDate (8.27 KB, patch)
2017-03-23 08:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 16413: Change prototype of GetSoonestRenewDate (5.30 KB, patch)
2017-03-23 08:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 16413: Change prototype of GetLatestAutoRenewDate (9.16 KB, patch)
2017-08-01 18:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 16413: Change prototype of GetSoonestRenewDate (5.29 KB, patch)
2017-08-01 18:14 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-05-02 13:49:53 UTC
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.
Comment 1 Jonathan Druart 2016-05-02 13:53:13 UTC
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.
Comment 2 Jonathan Druart 2016-05-02 13:53:17 UTC
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.
Comment 3 Srdjan Jankovic 2016-06-01 02:21:50 UTC
I think this one needs rebasing
Comment 4 Jonathan Druart 2016-06-05 17:09:01 UTC
(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 5 Mark Tompsett 2017-01-05 20:17:38 UTC
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 6 Mark Tompsett 2017-01-05 20:18:41 UTC
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,
?
Comment 7 Jonathan Druart 2017-01-09 08:05:20 UTC
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.
Comment 8 Jonathan Druart 2017-01-09 08:05:23 UTC
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.
Comment 9 Martin Renvoize 2017-03-23 08:49:10 UTC
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>
Comment 10 Martin Renvoize 2017-03-23 08:49:13 UTC
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>
Comment 11 Martin Renvoize 2017-03-23 08:49:59 UTC
Acting as git gopher to Jonathan Field who did the testing and actual signoff.
Comment 12 Marcel de Rooy 2017-04-07 09:23:37 UTC
(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?
Comment 13 Jonathan Druart 2017-04-07 12:51:06 UTC
(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.
Comment 14 Kyle M Hall 2017-06-15 14:24:14 UTC
Calls to GetSoonestRenewDate haven't been updated in opac-user.pl and svc/checkouts at the very least.
Comment 15 Jonathan Druart 2017-08-01 18:14:07 UTC
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.
Comment 16 Jonathan Druart 2017-08-01 18:14:12 UTC
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
Comment 17 Katrin Fischer 2020-01-11 12:14:18 UTC
Jonathan, do you want to keep this one open or should we close it MOVED as it should be fixed within the Koha namespace?
Comment 18 Jonathan Druart 2020-01-13 11:41:25 UTC
I will take care of this when it will be possible, when svc/checkouts
and opac-user.pl will use Koha::Checkout objects.
Comment 19 Katrin Fischer 2020-01-13 13:00:26 UTC
(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 :)