Bug 25850 - CalcDateDue freezes with 'useDaysMode' set to 'Dayweek' and the due date lands on a Sunday
Summary: CalcDateDue freezes with 'useDaysMode' set to 'Dayweek' and the due date land...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 15260
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-23 13:53 UTC by Andreas Jonsson
Modified: 2020-09-11 16:00 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off: 20.11.00
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03, 19.11.09


Attachments
Bug 25850: Adapt day of week in Koha::Calendar::get_push_amt to 0..6 with 0 being Sunday. (787 bytes, patch)
2020-06-23 14:00 UTC, Andreas Jonsson
Details | Diff | Splinter Review
Bug 25850: Add tests for weekday holidays (2.03 KB, patch)
2020-06-26 15:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25850: WIP - Add Unit Test (2.52 KB, patch)
2020-06-26 15:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25850: Add Unit Test (2.57 KB, patch)
2020-06-26 16:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25850: Add tests for weekday holidays (2.10 KB, patch)
2020-06-29 08:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25850: Regression Test (2.92 KB, patch)
2020-06-29 08:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25850: Adapt day of week in Koha::Calendar::get_push_amt to 0..6 with 0 being Sunday. (1.96 KB, patch)
2020-06-29 08:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25850: (QA follow-up) Match logic in is_holiday (1.01 KB, patch)
2020-06-29 08:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25850: Add tests for weekday holidays (2.16 KB, patch)
2020-07-07 12:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25850: Regression Test (2.97 KB, patch)
2020-07-07 12:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25850: Adapt day of week in Koha::Calendar::get_push_amt to 0..6 with 0 being Sunday. (2.02 KB, patch)
2020-07-07 12:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25850: (QA follow-up) Match logic in is_holiday (1.07 KB, patch)
2020-07-07 12:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25850: (QA follow-up) use dt_from_string (1.09 KB, patch)
2020-07-07 12:07 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Jonsson 2020-06-23 13:53:02 UTC
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;
}
Comment 1 Andreas Jonsson 2020-06-23 14:00:35 UTC
Created attachment 106207 [details] [review]
Bug 25850: Adapt day of week in Koha::Calendar::get_push_amt to 0..6 with 0 being Sunday.
Comment 2 Andreas Jonsson 2020-06-23 14:30:13 UTC
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.
Comment 3 Martin Renvoize 2020-06-26 14:35:47 UTC
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.
Comment 4 Martin Renvoize 2020-06-26 15:53:51 UTC
Created attachment 106346 [details] [review]
Bug 25850: Add tests for weekday holidays
Comment 5 Martin Renvoize 2020-06-26 15:53:54 UTC
Created attachment 106347 [details] [review]
Bug 25850: WIP - Add Unit Test
Comment 6 Martin Renvoize 2020-06-26 15:55:05 UTC
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.
Comment 7 Martin Renvoize 2020-06-26 16:36:31 UTC
Created attachment 106348 [details] [review]
Bug 25850: Add Unit Test
Comment 8 Martin Renvoize 2020-06-26 16:37:41 UTC
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.
Comment 9 Martin Renvoize 2020-06-29 08:24:00 UTC
Created attachment 106381 [details] [review]
Bug 25850: Add tests for weekday holidays

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2020-06-29 08:24:04 UTC
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>
Comment 11 Martin Renvoize 2020-06-29 08:24:07 UTC
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>
Comment 12 Martin Renvoize 2020-06-29 08:51:33 UTC
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.
Comment 13 Martin Renvoize 2020-06-29 08:55:33 UTC
Created attachment 106384 [details] [review]
Bug 25850: (QA follow-up) Match logic in is_holiday
Comment 14 Nick Clemens 2020-07-07 12:07:00 UTC
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>
Comment 15 Nick Clemens 2020-07-07 12:07:04 UTC
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>
Comment 16 Nick Clemens 2020-07-07 12:07:07 UTC
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>
Comment 17 Nick Clemens 2020-07-07 12:07:10 UTC
Created attachment 106629 [details] [review]
Bug 25850: (QA follow-up) Match logic in is_holiday

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens 2020-07-07 12:07:13 UTC
Created attachment 106630 [details] [review]
Bug 25850: (QA follow-up) use dt_from_string

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Martin Renvoize 2020-07-16 14:41:53 UTC
Pushed to master for 20.11, thanks everyone.
Comment 20 Lucas Gass 2020-07-24 20:45:24 UTC
backported to 20.05.x for 20.05.03
Comment 21 Aleisha Amohia 2020-08-03 21:19:25 UTC
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.
Comment 22 Victor Grousset/tuxayo 2020-09-11 15:56:34 UTC
Thanks for the notice Aleisha.
Comment 23 Victor Grousset/tuxayo 2020-09-11 16:00:25 UTC
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