Created attachment 85074 [details] [review]
Bug 22333: Replace duplicated code in CanBookBeRenewed with GetSoonestRenewDate

This patch replaces some duplicated code in CanBookBeRenewed by
using the GetSoonestRenewDate function.

This patch also refactors some of the auto_renew logic in CanBookBeRenewed,
so that it's more readable/maintainable.

I don't have a formal test plan yet, since it's a fiddly bit of code and I have other things to do, but I wanted to share the change anyway.

(In reply to David Cook from comment #2)
> I don't have a formal test plan yet, since it's a fiddly bit of code and I
> have other things to do, but I wanted to share the change anyway.

I think run test t/db_dependent/Circulation.t could be fine test plan here ;)

(In reply to Josef Moravec from comment #3)
> (In reply to David Cook from comment #2)
> > I don't have a formal test plan yet, since it's a fiddly bit of code and I
> > have other things to do, but I wanted to share the change anyway.
> 
> I think run test t/db_dependent/Circulation.t could be fine test plan here ;)

I'm happy to go with that if you are!

Test fails with the following error:

t/db_dependent/Circulation.t .. 122/126     # No tests run!
t/db_dependent/Circulation.t .. 123/126 
#   Failed test 'No tests run for subtest "AddRenewal and AddIssuingCharge tests"'
#   at t/db_dependent/Circulation.t line 2948.
Can't call method "lengthunit" on an undefined value at /home/vagrant/kohaclone/Koha/Charges/Fees.pm line 101.
# Looks like your test exited with 11 just after 123.
t/db_dependent/Circulation.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 4/126 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 2816 Tests: 123 Failed: 1)
  Failed test:  123
  Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 126 tests but ran 123.
Files=1, Tests=123, 38 wallclock secs ( 0.07 usr  0.01 sys + 18.59 cusr  3.22 csys = 21.89 CPU)
Result: FAIL

(In reply to Hayley Mapley from comment #5)
> Test fails with the following error:
> 
> t/db_dependent/Circulation.t .. 122/126     # No tests run!
> t/db_dependent/Circulation.t .. 123/126 
> #   Failed test 'No tests run for subtest "AddRenewal and AddIssuingCharge
> tests"'
> #   at t/db_dependent/Circulation.t line 2948.
> Can't call method "lengthunit" on an undefined value at
> /home/vagrant/kohaclone/Koha/Charges/Fees.pm line 101.
> # Looks like your test exited with 11 just after 123.
> t/db_dependent/Circulation.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
> Failed 4/126 subtests 
> 
> Test Summary Report
> -------------------
> t/db_dependent/Circulation.t (Wstat: 2816 Tests: 123 Failed: 1)
>   Failed test:  123
>   Non-zero exit status: 11
>   Parse errors: Bad plan.  You planned 126 tests but ran 123.
> Files=1, Tests=123, 38 wallclock secs ( 0.07 usr  0.01 sys + 18.59 cusr 
> 3.22 csys = 21.89 CPU)
> Result: FAIL

Thanks, Hayley!

I'm guessing this highlights a problem with that test rather than this patch though. Hmm. What are your thoughts, Tomas?

I'm going to reset this one to needs signoff, as I think the problem is with the tests rather than the patch.

Hi David,

I got test failures too, as follows, have a look?

Liz

#   Failed test 'Bug 14101: Cannot renew, renewal is automatic (returned code is auto_renew)'
#   at t/db_dependent/Circulation.t line 601.
#          got: 'auto_too_soon'
#     expected: 'auto_renew'

    #   Failed test 'Cannot renew, renew is automatic'
    #   at t/db_dependent/Circulation.t line 643.
    #          got: 'auto_too_soon'
    #     expected: 'auto_renew'

    #   Failed test 'Cannot renew, renew is automatic'
    #   at t/db_dependent/Circulation.t line 661.
    #          got: 'auto_too_soon'
    #     expected: 'auto_renew'
    # Looks like you failed 2 tests of 14.

#   Failed test 'too_late_renewal / no_auto_renewal_after'
#   at t/db_dependent/Circulation.t line 662.

    #   Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 5'
    #   at t/db_dependent/Circulation.t line 695.
    #          got: 'auto_too_soon'
    #     expected: 'auto_renew'

    #   Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 10'
    #   at t/db_dependent/Circulation.t line 708.
    #          got: 'auto_too_soon'
    #     expected: 'auto_renew'
    # Looks like you failed 2 tests of 6.
t/db_dependent/Circulation.t .. 60/125 
#   Failed test 'auto_too_much_oweing | OPACFineNoRenewalsBlockAutoRenew'
#   at t/db_dependent/Circulation.t line 724.

    #   Failed test 'Can auto renew, patron is expired but BlockExpiredPatronOpacActions=0'
    #   at t/db_dependent/Circulation.t line 750.
    #          got: 'auto_too_soon'
    #     expected: 'auto_renew'

    #   Failed test 'Can auto renew, BlockExpiredPatronOpacActions=1 but patron is not expired'
    #   at t/db_dependent/Circulation.t line 774.
    #          got: 'auto_too_soon'
    #     expected: 'auto_renew'
    # Looks like you failed 2 tests of 6.

#   Failed test 'auto_account_expired | BlockExpiredPatronOpacActions'
#   at t/db_dependent/Circulation.t line 776.
t/db_dependent/Circulation.t .. 125/125 # Looks like you failed 4 tests of 125.
t/db_dependent/Circulation.t .. Dubious, test returned 4 (wstat 1024, 0x400)
Failed 4/125 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation.t (Wstat: 1024 Tests: 125 Failed: 4)
  Failed tests:  58-61
  Non-zero exit status: 4
Files=1, Tests=125, 36 wallclock secs ( 0.11 usr  0.01 sys + 25.60 cusr  4.60 csys = 30.32 CPU)
Result: FAIL

(In reply to Liz Rea from comment #8)
> Hi David,
> 
> I got test failures too, as follows, have a look?
> 
> Liz
> 

Ahhh thanks for that output. I'll take a look when I have a moment.

I have too many other priorities, so I'm probably not going to be looking at this any time soon.

I think this has been addressed elsewhere and can be closed

*** This bug has been marked as a duplicate of bug 29474 ***