Bug 18146

Summary: C4::Circulation CanBookBeRenewed lacks full test coverage
Product: Koha Reporter: Mark Tompsett <mtompset>
Component: Test SuiteAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: david, fridolin.somers, kyle, nick
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement improves the test coverage for OverduesBlockRenewing and removes some of the warning messages.
Version(s) released in:
21.05.00
Attachments: Bug 18146: Fix tests for OverduesBlockRenewing
Bug 18146: (follow-up) Remove warn for uninitialized value
Bug 18146: Fix tests for OverduesBlockRenewing
Bug 18146: (follow-up) Remove warn for uninitialized value
Bug 18146: Fix tests for OverduesBlockRenewing
Bug 18146: (follow-up) Remove warn for uninitialized value

Description Mark Tompsett 2017-02-20 16:02:50 UTC
While testing bug 17941, I discovered two cases where test coverage was lacking from t/db_dependent/Circulation.t

Around line 2725:
    if ( $restricted and $restrictionblockrenewing ) {
        return ( 0, 'restriction');
    } elsif ( ($hasoverdues and $overduesblockrenewing eq 'block') || ($itemissue->{overdue} and $overduesblockrenewing eq 'blockitem') ) {
        return ( 0, 'overdue');
    }

The elseif is never triggered.


Around line 2759:
        if ( $soonestrenewal > DateTime->now( time_zone => C4::Context->tz() ) )
        {
            return ( 0, "auto_too_soon" ) if $itemissue->{auto_renew};
            return ( 0, "too_soon" );
        }
        elsif ( $itemissue->{auto_renew} ) {
            return ( 0, "auto_renew" );
        }

There is no else, but the code never attempts something that would trigger one.

To reproduce add:
print STDERR "CHECK!\n";
just before the return ( 0, 'overdue') in the first case, and after the elsif's curly brace in the second case.

running t/db_dependent/Circulation should not generate the CHECK points.
Comment 1 Nick Clemens 2021-01-29 15:27:54 UTC
Created attachment 116040 [details] [review]
Bug 18146: Fix tests for OverduesBlockRenewing

This pref was supposedly covered by tests, but the conditions were wrong
and we didn't test the reasons we were failing so the code was being missed

To test:
1 - Add a warn around in the conditional at line 2748:
} elsif ( ($hasoverdues and $overduesblockrenewing eq 'block') || ($itemissue->{overdue} and $overduesblockrenewing eq 'blockitem') ) {
     warn "SUCCESS!"
     return ( 0, 'overdue');
}
2 - prove -v t/db_dependent/Circulation.t | grep SUCCESS
3 - No output
4 - Apply patch
5 - Repeat
6 - SUCCESS!

https://bugs.koha-community.org/show_bug.cgi?id=18146
Comment 2 Nick Clemens 2021-01-29 15:27:56 UTC
Created attachment 116041 [details] [review]
Bug 18146: (follow-up) Remove warn for uninitialized value

In the previous patch you may have noticed many warns when running the tests

We add guarantor charges to a variable to determine if over the limit, but we don't initialize that value

We should

To test:
1 - Apply first patch and follow test plan
2 - Note warns when proving test
3 - Apply this patch
4 - prove
5 - No more warns
Comment 3 David Nind 2021-01-30 05:51:05 UTC
Created attachment 116089 [details] [review]
Bug 18146: Fix tests for OverduesBlockRenewing

This pref was supposedly covered by tests, but the conditions were wrong
and we didn't test the reasons we were failing so the code was being missed

To test:
1 - Add a warn around in the conditional at line 2748:
} elsif ( ($hasoverdues and $overduesblockrenewing eq 'block') || ($itemissue->{overdue} and $overduesblockrenewing eq 'blockitem') ) {
     warn "SUCCESS!"
     return ( 0, 'overdue');
}
2 - prove -v t/db_dependent/Circulation.t | grep SUCCESS
3 - No output
4 - Apply patch
5 - Repeat
6 - SUCCESS!

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2021-01-30 05:51:08 UTC
Created attachment 116090 [details] [review]
Bug 18146: (follow-up) Remove warn for uninitialized value

In the previous patch you may have noticed many warns when running the tests

We add guarantor charges to a variable to determine if over the limit, but we don't initialize that value

We should

To test:
1 - Apply first patch and follow test plan
2 - Note warns when proving test
3 - Apply this patch
4 - prove
5 - No more warns

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2021-01-30 05:53:46 UTC
Testing notes - for step 1 of the first test plan:
- the file to edit is C4/Circulation.pm
- there should be a semicolon after warn "SUCCESS!" (that is: warn "SUCCESS!";)
Comment 6 Katrin Fischer 2021-02-14 15:16:41 UTC
Created attachment 116865 [details] [review]
Bug 18146: Fix tests for OverduesBlockRenewing

This pref was supposedly covered by tests, but the conditions were wrong
and we didn't test the reasons we were failing so the code was being missed

To test:
1 - Add a warn around in the conditional at line 2748:
} elsif ( ($hasoverdues and $overduesblockrenewing eq 'block') || ($itemissue->{overdue} and $overduesblockrenewing eq 'blockitem') ) {
     warn "SUCCESS!"
     return ( 0, 'overdue');
}
2 - prove -v t/db_dependent/Circulation.t | grep SUCCESS
3 - No output
4 - Apply patch
5 - Repeat
6 - SUCCESS!

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

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Katrin Fischer 2021-02-14 15:16:45 UTC
Created attachment 116866 [details] [review]
Bug 18146: (follow-up) Remove warn for uninitialized value

In the previous patch you may have noticed many warns when running the tests

We add guarantor charges to a variable to determine if over the limit, but we don't initialize that value

We should

To test:
1 - Apply first patch and follow test plan
2 - Note warns when proving test
3 - Apply this patch
4 - prove
5 - No more warns

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Katrin Fischer 2021-02-14 15:17:38 UTC
More tests are better, but there are still a lot of warns running theses thests before and after the patch:

Use of uninitialized value in concatenation (.) or string at /home/vagrant/kohaclone/C4/Biblio.pm line 657.
t/db_dependent/Circulation.t .. 32/52 Use of uninitialized value in concatenation (.) or string at /home/vagrant/kohaclone/C4/Biblio.pm line 657.
Comment 9 Jonathan Druart 2021-02-15 11:08:29 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 10 Fridolin Somers 2021-02-19 15:33:22 UTC
Enhancement not pushed to 20.11.x