[Koha-bugs] [Bug 27032] CanBookBeRenewed is not understandable and needs refactoring

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27032

Joonas Kylmälä <joonas.kylmala at iki.fi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |joonas.kylmala at iki.fi

--- Comment #16 from Joonas Kylmälä <joonas.kylmala at 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.