Bug 27032 - CanBookBeRenewed is not understandable and needs refactoring
Summary: CanBookBeRenewed is not understandable and needs refactoring
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Joonas Kylmälä
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-11-16 13:52 UTC by Joonas Kylmälä
Modified: 2021-10-08 14:47 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
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 (816 bytes, patch)
2020-11-16 13:59 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27032: Move auto renewal code out of CanBookBeRenewed (9.09 KB, patch)
2020-11-16 13:59 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27032: Move auto renewal code out of CanBookBeRenewed (9.11 KB, patch)
2020-11-16 14:01 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27032: Remove unused variables (872 bytes, patch)
2021-03-22 13:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27032: Move auto renewal code out of CanBookBeRenewed (9.16 KB, patch)
2021-03-22 13:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27032: (follow-up) Pass rather than fetch variables (1.69 KB, patch)
2021-03-22 13:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27032: Remove unused variables (1020 bytes, patch)
2021-05-28 16:26 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27032: Move auto renewal code out of CanBookBeRenewed (9.32 KB, patch)
2021-05-28 16:26 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27032: (follow-up) Pass rather than fetch variables (1.83 KB, patch)
2021-05-28 16:26 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 27032: Remove unused variables (1.07 KB, patch)
2021-09-10 16:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27032: Move auto renewal code out of CanBookBeRenewed (9.38 KB, patch)
2021-09-10 16:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 27032: (follow-up) Pass rather than fetch variables (1.90 KB, patch)
2021-09-10 16:10 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 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!