[Bug 27032] New: CanBookBeRenewed is not understandable and needs refactoring
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Bug ID: 27032 Summary: CanBookBeRenewed is not understandable and needs refactoring Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: joonas.kylmala@helsinki.fi Reporter: joonas.kylmala@helsinki.fi QA Contact: testopia@bugs.koha-community.org Blocks: 26659 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. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 [Bug 26659] An item can be renewed even if there are holds waiting to be filled -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 --- Comment #1 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 113666 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113666&action=edit Bug 27032: Remove unused variables -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 --- Comment #2 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 113667 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113667&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113667|0 |1 is obsolete| | --- Comment #3 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 113668 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113668&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff --- Comment #4 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nugged@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=28013 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113666|0 |1 is obsolete| | --- Comment #5 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 118593 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118593&action=edit Bug 27032: Remove unused variables Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113668|0 |1 is obsolete| | --- Comment #6 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 118594 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118594&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 --- Comment #7 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 118595 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=118595&action=edit Bug 27032: (follow-up) Pass rather than fetch variables Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |Improvements to readability release notes| |of CanBookBeRenewed | |function -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|26659 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659 [Bug 26659] An item can be renewed even if there are holds waiting to be filled -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |kyle@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Joonas Kylmälä <joonas.kylmala@helsinki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #118593|0 |1 is obsolete| | Attachment #118594|0 |1 is obsolete| | Attachment #118595|0 |1 is obsolete| | --- Comment #8 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 121492 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121492&action=edit Bug 27032: Remove unused variables Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 --- Comment #9 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 121493 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121493&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 --- Comment #10 from Joonas Kylmälä <joonas.kylmala@helsinki.fi> --- Created attachment 121494 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=121494&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 --- Comment #11 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #121492|0 |1 is obsolete| | Attachment #121493|0 |1 is obsolete| | Attachment #121494|0 |1 is obsolete| | --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 124780 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=124780&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 124781 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=124781&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 124782 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=124782&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #15 from Jonathan Druart <jonathan.druart+koha@gmail.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; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Joonas Kylmälä <joonas.kylmala@iki.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joonas.kylmala@iki.fi --- Comment #16 from Joonas Kylmälä <joonas.kylmala@iki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 --- Comment #17 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 --- Comment #18 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Pushed to master for 21.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to master |RESOLVED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org