Bug 22333 - Replace duplicated code in CanBookBeRenewed with GetSoonestRenewDate
Summary: Replace duplicated code in CanBookBeRenewed with GetSoonestRenewDate
Status: RESOLVED DUPLICATE of bug 29474
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-02-14 04:38 UTC by David Cook
Modified: 2022-04-20 12:29 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22333: Replace duplicated code in CanBookBeRenewed with GetSoonestRenewDate (2.76 KB, patch)
2019-02-14 05:45 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2019-02-14 04:38:52 UTC
There is some duplicated code between CanBookBeRenewed and GetSoonestRenewDate.
Comment 1 David Cook 2019-02-14 05:45:28 UTC
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.
Comment 2 David Cook 2019-02-14 05:46:47 UTC
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.
Comment 3 Josef Moravec 2019-02-18 10:27:41 UTC
(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 ;)
Comment 4 David Cook 2019-02-19 01:00:52 UTC
(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!
Comment 5 Hayley Pelham 2019-03-12 03:45:31 UTC
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
Comment 6 David Cook 2019-03-13 01:03:46 UTC
(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?
Comment 7 David Cook 2019-03-25 04:04:54 UTC
I'm going to reset this one to needs signoff, as I think the problem is with the tests rather than the patch.
Comment 8 Liz Rea 2019-04-02 19:45:11 UTC
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
Comment 9 David Cook 2019-04-03 00:49:00 UTC
(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.
Comment 10 David Cook 2019-11-11 02:31:23 UTC
I have too many other priorities, so I'm probably not going to be looking at this any time soon.
Comment 11 Nick Clemens 2022-04-20 12:29:47 UTC
I think this has been addressed elsewhere and can be closed

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