Bug 27032

Summary: CanBookBeRenewed is not understandable and needs refactoring
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: Architecture, internals, and plumbingAssignee: Joonas Kylmälä <joonas.kylmala>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: enhancement    
Priority: P5 - low CC: dcook, jonathan.druart, joonas.kylmala, kyle, m.de.rooy, nugged
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28013
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Improvements to readability of CanBookBeRenewed function
Version(s) released in:
21.11.00
Attachments: Bug 27032: Remove unused variables
Bug 27032: Move auto renewal code out of CanBookBeRenewed
Bug 27032: Move auto renewal code out of CanBookBeRenewed
Bug 27032: Remove unused variables
Bug 27032: Move auto renewal code out of CanBookBeRenewed
Bug 27032: (follow-up) Pass rather than fetch variables
Bug 27032: Remove unused variables
Bug 27032: Move auto renewal code out of CanBookBeRenewed
Bug 27032: (follow-up) Pass rather than fetch variables
Bug 27032: Remove unused variables
Bug 27032: Move auto renewal code out of CanBookBeRenewed
Bug 27032: (follow-up) Pass rather than fetch variables

Description Joonas Kylmälä 2020-11-16 13:52:59 UTC
While trying to resolve the bugs found in bug 26659 I deemed the CanBookBeRenewed subroutine to be too difficult to understand and make changes. This bug report is opened in the hope that we can refactor it a bit and allow making changes to it easier in future.
Comment 1 Joonas Kylmälä 2020-11-16 13:59:01 UTC
Created attachment 113666 [details] [review]
Bug 27032: Remove unused variables
Comment 2 Joonas Kylmälä 2020-11-16 13:59:04 UTC Comment hidden (obsolete)
Comment 3 Joonas Kylmälä 2020-11-16 14:01:07 UTC
Created attachment 113668 [details] [review]
Bug 27032: Move auto renewal code out of CanBookBeRenewed

This moves the checks related to whether auto renewal can be done on
the checkout to its own function _CanBookBeAutoRenewed(). This makes
it more clear which parts of the code are related to auto renewal
checking and which are not.

To test:
  1) Make sure prove t/db_dependent/Circulation.t still passes
Comment 4 Joonas Kylmälä 2021-03-05 09:04:04 UTC
I thought I would do a bit more refactoring in the future but that day didn't come up so moving this to Needs Sign-off so the work done so far doesn't go to waste.
Comment 5 Nick Clemens 2021-03-22 13:30:32 UTC
Created attachment 118593 [details] [review]
Bug 27032: Remove unused variables

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 Nick Clemens 2021-03-22 13:30:35 UTC
Created attachment 118594 [details] [review]
Bug 27032: Move auto renewal code out of CanBookBeRenewed

This moves the checks related to whether auto renewal can be done on
the checkout to its own function _CanBookBeAutoRenewed(). This makes
it more clear which parts of the code are related to auto renewal
checking and which are not.

To test:
  1) Make sure prove t/db_dependent/Circulation.t still passes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 7 Nick Clemens 2021-03-22 13:30:38 UTC
Created attachment 118595 [details] [review]
Bug 27032: (follow-up) Pass rather than fetch variables

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Joonas Kylmälä 2021-05-28 16:26:10 UTC
Created attachment 121492 [details] [review]
Bug 27032: Remove unused variables

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 9 Joonas Kylmälä 2021-05-28 16:26:14 UTC
Created attachment 121493 [details] [review]
Bug 27032: Move auto renewal code out of CanBookBeRenewed

This moves the checks related to whether auto renewal can be done on
the checkout to its own function _CanBookBeAutoRenewed(). This makes
it more clear which parts of the code are related to auto renewal
checking and which are not.

To test:
  1) Make sure prove t/db_dependent/Circulation.t still passes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 10 Joonas Kylmälä 2021-05-28 16:26:19 UTC
Created attachment 121494 [details] [review]
Bug 27032: (follow-up) Pass rather than fetch variables

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 11 Joonas Kylmälä 2021-05-28 16:28:28 UTC
Hi Kyle, I attached rebased patches. I simply did git rebase origin/master and git was able to resolve the conflict automatically and tests still passes so I think this is ready for QA.
Comment 12 Kyle M Hall 2021-09-10 16:09:20 UTC
Created attachment 124780 [details] [review]
Bug 27032: Remove unused variables

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2021-09-10 16:10:32 UTC
Created attachment 124781 [details] [review]
Bug 27032: Move auto renewal code out of CanBookBeRenewed

This moves the checks related to whether auto renewal can be done on
the checkout to its own function _CanBookBeAutoRenewed(). This makes
it more clear which parts of the code are related to auto renewal
checking and which are not.

To test:
  1) Make sure prove t/db_dependent/Circulation.t still passes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2021-09-10 16:10:35 UTC
Created attachment 124782 [details] [review]
Bug 27032: (follow-up) Pass rather than fetch variables

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Jonathan Druart 2021-09-29 10:15:26 UTC
Joonas, don't you think we should have the same return than CanBookBeRenewed?

  ($boolean, $reason);

The 'return "no";' seems weird to me.

We could then replace:

    $auto_renew = _CanBookBeAutoRenewed($borrowernumber, $itemnumber);
    return ( 0, $auto_renew  ) if $auto_renew =~ 'auto_account_expired';
    return ( 0, $auto_renew  ) if $auto_renew =~ 'auto_too_late';
    return ( 0, $auto_renew  ) if $auto_renew =~ 'auto_too_much_oweing';

with:

    my ( $can_be_auto_renewed, $reason ) = _CanBookBeAutoRenewed($borrowernumber, $itemnumber);
    return ( 0, $reason ) unless $can_be_auto_renewed;
Comment 16 Joonas Kylmälä 2021-09-29 16:01:30 UTC
(In reply to Jonathan Druart from comment #15)
> Joonas, don't you think we should have the same return than CanBookBeRenewed?
> 
>   ($boolean, $reason);
> 
> The 'return "no";' seems weird to me.
> 
> We could then replace:
> 
>     $auto_renew = _CanBookBeAutoRenewed($borrowernumber, $itemnumber);
>     return ( 0, $auto_renew  ) if $auto_renew =~ 'auto_account_expired';
>     return ( 0, $auto_renew  ) if $auto_renew =~ 'auto_too_late';
>     return ( 0, $auto_renew  ) if $auto_renew =~ 'auto_too_much_oweing';
> 
> with:
> 
>     my ( $can_be_auto_renewed, $reason ) =
> _CanBookBeAutoRenewed($borrowernumber, $itemnumber);
>     return ( 0, $reason ) unless $can_be_auto_renewed;

This is a good plan but I purposefully left it out from this refactoring because we would have to touch the code in the bottom of CanBookBeRenewed() where there is the cron section doing magic with "$auto_renew =~ 'too_soon'" or alternatively make _CanBookBeAutoRenewed return 1 for "too_soon".

It was too big of a task for me to verify the correctness of the code and given Circulation.pm module is frequently changed I didn't want to have to do it again due to a merge conflict so I decided to do that work after the code is merged. Currently this was simply just copy&paste of the existing code with variable assignments changed to returns. Having the auto renew code now in a separate function I can start doing more changes to it without having to worry about it merge conflicting and having to start tedious verification process for correct logic again.


Also clarifying the return "no" since you mentioned it: it is to keep intact the 

> my $auto_renew = "no";

value assigned in the beginning of CanBookBeRenewed.
Comment 17 Jonathan Druart 2021-09-30 07:06:20 UTC
(In reply to Joonas Kylmälä from comment #16)
> (In reply to Jonathan Druart from comment #15)
> Also clarifying the return "no" since you mentioned it: it is to keep intact
> the 
> 
> > my $auto_renew = "no";
> 
> value assigned in the beginning of CanBookBeRenewed.

Indeed, I missed that. Looking forward follow-up bugs to continue the cleaning of this module!
Comment 18 Jonathan Druart 2021-10-06 12:56:57 UTC
Pushed to master for 21.11, thanks to everybody involved!