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.
Created attachment 113666 [details] [review] Bug 27032: Remove unused variables
Created attachment 113667 [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 Circulation.t still passes
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
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.
Created attachment 118593 [details] [review] Bug 27032: Remove unused variables Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
Created attachment 118595 [details] [review] Bug 27032: (follow-up) Pass rather than fetch variables Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
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>
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>
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.
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>
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>
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>
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;
(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.
(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!
Pushed to master for 21.11, thanks to everybody involved!