Created attachment 106207 [details] [review] Bug 25850: Adapt day of week in Koha::Calendar::get_push_amt to 0..6 with 0 being Sunday. Test plan
1. Go to tools -> calendar and make sure Sundays are holidays by adding a holiday on a Sunday and selecting "Holiday repeated every same day of the week" and "Copy to all libraries" and save.
2. Go to administration -> system preferences and select "Use the calendar to push the due date to the next open matching weekday for weekly loan periods, or the next open day otherwise" for the system preference "useDaysMode" and save the system preferences.
3. Put the below code in a file name test.pl and execute it using the command "sudo koha-shell -c 'perl test.pl' kohadev"
use Koha::Calendar;
my $calendar = Koha::Calendar->new( branchcode => 'CPL' );
$dt = DateTime->new(
year => 2020,
month => 06,
day => 21
);
print "This is a sunday: " . $dt->day_of_week . "\n";
my $ndt = $calendar->next_open_days($dt, 0);
print "This is a monday: " . $ndt->day_of_week . "\n";
4. Without the patch applied, this script will freeze after printing "This is a sunday ...". Abort using ctrl-c.
5. Apply patch and run the script again. The test script will now complete.
I like the modulo approach you've used, but I wonder if perhaps it's a tiny bit clearer if you use the same approach as the 'is_holiday' method uses a few lines beneath your fix? Also.. I'd love to see a unit test added to prevent future regression. Created attachment 106346 [details] [review] Bug 25850: Add tests for weekday holidays Created attachment 106347 [details] [review] Bug 25850: WIP - Add Unit Test Added the beginnings of a test there.. first one is unrelated but I wrote it whilst working out options to test each weekday holiday option.. second patch I've not managed to get failing yet.. bear with. Created attachment 106348 [details] [review] Bug 25850: Add Unit Test That's better. So.. before I signoff I'd be interested in opinions on my above question regarding readability and making it match other blocks that do the same logical fix. Created attachment 106381 [details] [review] Bug 25850: Add tests for weekday holidays Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 106382 [details] [review] Bug 25850: Regression Test This patch adds a unit test to test for an infinite loop as highlighted by the bug. Test plan 1/ Run the test before applying the fix 2/ The test should fail for 'Sundays' 3/ Apply the subsquent patch 4/ Re-run the test 5/ It should now pass Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 106383 [details] [review] Bug 25850: Adapt day of week in Koha::Calendar::get_push_amt to 0..6 with 0 being Sunday. 1. Go to tools -> calendar and make sure Sundays are holidays by adding a holiday on a Sunday and selecting "Holiday repeated every same day of the week" and "Copy to all libraries" and save. 2. Go to administration -> system preferences and select "Use the calendar to push the due date to the next open matching weekday for weekly loan periods, or the next open day otherwise" for the system preference "useDaysMode" and save the system preferences. 3. Put the below code in a file name test.pl and execute it using the command "sudo koha-shell -c 'perl test.pl' kohadev" use Koha::Calendar; my $calendar = Koha::Calendar->new( branchcode => 'CPL' ); $dt = DateTime->new( year => 2020, month => 06, day => 21 ); print "This is a sunday: " . $dt->day_of_week . "\n"; my $ndt = $calendar->next_open_days($dt, 0); print "This is a monday: " . $ndt->day_of_week . "\n"; 4. Without the patch applied, this script will freeze after printing "This is a sunday ...". Abort using ctrl-c. 5. Apply patch and run the script again. The test script will now complete. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> I did a quick benchmark on '`Modulo` Vs `comparison + assignment`'.
Rate Modulo Assign
Modulo 23693859/s -- -22%
Assign 30513086/s 29% --
So.. it looks like the test + assign is the quicker operation... I'll add a followup here to make that conversion to match the logic here to the logic in the is_holiday routine.
Created attachment 106384 [details] [review] Bug 25850: (QA follow-up) Match logic in is_holiday Created attachment 106626 [details] [review] Bug 25850: Add tests for weekday holidays Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 106627 [details] [review] Bug 25850: Regression Test This patch adds a unit test to test for an infinite loop as highlighted by the bug. Test plan 1/ Run the test before applying the fix 2/ The test should fail for 'Sundays' 3/ Apply the subsquent patch 4/ Re-run the test 5/ It should now pass Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 106628 [details] [review] Bug 25850: Adapt day of week in Koha::Calendar::get_push_amt to 0..6 with 0 being Sunday. 1. Go to tools -> calendar and make sure Sundays are holidays by adding a holiday on a Sunday and selecting "Holiday repeated every same day of the week" and "Copy to all libraries" and save. 2. Go to administration -> system preferences and select "Use the calendar to push the due date to the next open matching weekday for weekly loan periods, or the next open day otherwise" for the system preference "useDaysMode" and save the system preferences. 3. Put the below code in a file name test.pl and execute it using the command "sudo koha-shell -c 'perl test.pl' kohadev" use Koha::Calendar; my $calendar = Koha::Calendar->new( branchcode => 'CPL' ); $dt = DateTime->new( year => 2020, month => 06, day => 21 ); print "This is a sunday: " . $dt->day_of_week . "\n"; my $ndt = $calendar->next_open_days($dt, 0); print "This is a monday: " . $ndt->day_of_week . "\n"; 4. Without the patch applied, this script will freeze after printing "This is a sunday ...". Abort using ctrl-c. 5. Apply patch and run the script again. The test script will now complete. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 106629 [details] [review] Bug 25850: (QA follow-up) Match logic in is_holiday Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 106630 [details] [review] Bug 25850: (QA follow-up) use dt_from_string Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Pushed to master for 20.11, thanks everyone. backported to 20.05.x for 20.05.03 backported to 19.11.x for 19.11.09 t/db_dependent/Calendar.t needed an additional patch to mock the useDaysMode syspref, otherwise the test would fail if useDaysMode wasn't set to 'calendar'. @19.05.x rmaint: You will need my additional patch as well because Koha::Calendar->_init checks the syspref. Thanks for the notice Aleisha. Can't backport to 19.05.x, the test uses next_open_days which isn't in 19.05
If someone needs this for 19.05.x, please send a patch or code extract that would allow the test to be backported. And thus ensuring that the fix works.
kohadev-koha@891563ed356b:/kohadevbox/koha$ prove t/db_dependent/Calendar.t
t/db_dependent/Calendar.t .. 1/8 # No tests run!
# Failed test 'No tests run for subtest "weekday holidays"'
# at t/db_dependent/Calendar.t line 183.
# Looks like you failed 1 test of 1.
# Failed test 'get_push_amt'
# at t/db_dependent/Calendar.t line 184.
Can't locate object method "next_open_days" via package "Koha::Calendar" at t/db_dependent/Calendar.t line 165.
...propagated at t/db_dependent/Calendar.t line 169.
# Looks like your test exited with 11 just after 8.
t/db_dependent/Calendar.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 1/8 subtests
|
From the DateTime manual page: $dt->day_of_week() Returns the day of the week as a number, from 1..7, with 1 being Monday and 7 being Sunday. Also available as "$dt->wday()" and "$dt->dow()". But the method "get_push_amt" assumes that day_of_week is numbered 0..6 with 0 being Sunday. (Also, there is a funny equality test of a boolean value and the number 1.) The result is that for Sundays, $self->{weekly_closed_days}->[$dow] is undefined and its negation is thus true, which results in a push_amt of seven days, which lands on the next Sunday and the process repeats. sub get_push_amt { my ( $self, $base_date) = @_; my $dow = $base_date->day_of_week; return ( # We're using Dayweek useDaysMode option $self->{days_mode} eq 'Dayweek' && # It's not a permanently closed day !$self->{weekly_closed_days}->[$dow] == 1 ) ? 7 : 1; }