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.
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
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
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>
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>
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!";)
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>
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>
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.
Pushed to master for 21.05, thanks to everybody involved!
Enhancement not pushed to 20.11.x