Schedules passing midnight are never considered active

[cabal95]

Prerequisites

• Put an X between the brackets on this line if you have done all of the following:
  • Can you reproduce the problem on a fresh install or the [http://rock.rocksolidchurchdemo.com/ demo site ]()?
  • Did you include your Rock version number and [https://github.com/SparkDevNetwork/Rock/wiki/Environment-and-Diagnostics-Information client culture ]() setting?
  • Did you [https://github.com/issues?q=is%3Aissue+user%3ASparkDevNetwork+-repo%3ASparkDevNetwork%2FSlack perform a cursory search ]() to see if your bug or enhancement is already reported?

Description

The way the WasScheduleActive check works, it assumes a schedule will never go past midnight. It is passed a DateTime that for discussion we will call now. It gets the time of day of now and checks if it is greater than the start time of the schedule and less than the end time of the schedule.

The easiest way to give an example of this is with a schedule that is meant to be "all day long". You want a schedule that is for every Tuesday, and is for ALL DAY. So you create a Weekly Repeating schedule that starts on Tuesday at midnight. Since you want it to run all day long, you make the duration 24 hours. Then if we check if "tuesday 8am" is inside that schedule, we are essentially doing this check:

    if 8am > midnight and 8am < midnight then...

That check will never pass. A similar issue happens if you create a schedule that passes beyond the midnight mark. For example, every Tuesday from 8pm to Wednesday 4am. Weekly Schedule, starts Tuesday at 8pm, runs 8 hours. If you attempt to test for Wednesday at 2am:

    if 2am > 8pm and 2am < 4am then...

Again, it does not pass the check. Obviously, the same problem applies for an even that is longer than 24 hours. Below is a chart showing a few schedules and the WasScheduleActive test results.

Steps to Reproduce

1. Go to Check-in Schedules, Create a new schedule that starts at midnight and runs for 24 hours.
2. Attempt to check if the schedule is active.

Expected behavior:

The schedules should either support testing for an active schedule that crosses a midnight boundary OR the schedule editor should not allow for such a situation.

Changing the system to not allow for schedules that go past midnight would probably cause a lot of problems for churches that already use this behavior for calendaring purposes (including us). I would prefer to fix it by fixing the tests to be smarter.

This is something that could be added to, I think, non-datebase unit tests.

I would be willing to work on code for adding both unit tests and the proper functionality to checking if a schedule is/was active.

Actual behavior:

Schedules do not follow normal logic that we would expect as humans.

Testing code

{% assign wednesdayNoon = '2018-07-11 12:00:00' | DateAdd:0 %}
{% assign wednesday2am = '2018-07-11 02:00:00' | DateAdd:0 %}
{% assign tuesdayNoon = '2018-07-10 12:00:00' | DateAdd:0 %}
{% assign tuesday2am = '2018-07-10 02:00:00' | DateAdd:0 %}

{% capture data %}{% execute import:'System.Linq' %}
    var scheduleService = new ScheduleService( new RockContext() );
    var schedules = scheduleService.Queryable().Where( s => s.CategoryId == 217 ).ToList();
    var wednesdayNoon = new DateTime( 2018, 7, 11, 12, 0, 0 );
    var wednesday2am = new DateTime( 2018, 7, 11, 2, 0, 0 );
    var tuesdayNoon = new DateTime( 2018, 7, 10, 12, 0, 0 );
    var tuesday2am = new DateTime( 2018, 7, 10, 2, 0, 0 );
    return string.Join( "|", schedules.Select( s => string.Format( "{0},{1},{2},{3},{4}",
        s.Name,
        s.WasScheduleActive( wednesdayNoon ),
        s.WasScheduleActive( wednesday2am ),
        s.WasScheduleActive( tuesdayNoon ),
        s.WasScheduleActive( tuesday2am )
        ) ) );
{% endexecute %}{% endcapture %}
{% assign rows = data | Split:'|' %}
<table width="100%" style="margin-bottom: 30px;">
    <thead>
        <tr>
            <th>Schedule</th>
            <th>Tue 2am Active</th>
            <th>Tue Noon Active</th>
            <th>Wed 2am Active</th>
            <th>Wed Noon Active</th>
        </tr>
    </thead>
    <tbody>
        {% for row in rows %}
            {% assign values = row | Split:',' %}
            <tr>
                <td>{{ values[0] }}</td>
                <td>{{ values[1] }}</td>
                <td>{{ values[2] }}</td>
                <td>{{ values[3] }}</td>
                <td>{{ values[4] }}</td>
            </tr>
        {% endfor %}
    </tbody>
</table>

Versions

• Rock Version: 6.x, 7.x, 8.x
• Client Culture Setting: en-US

[cabal95]

Just some extra information, I've been playing with ways to resolve some of these issues in a plugin I'm working on. It looks like we can this to work better, and have it properly recognize schedules that go past midnight once. Going past midnight twice seems to just be completely broken in the DDay.iCal internals.

Said another way, if I create a schedule that starts at 4pm on Saturday and goes 24 hours and then check if that schedule is active on Sunday at noon, I can get a truth to that check.

But if I create a schedule that starts at 4pm on Friday and goes 48 hours and then check if that schedule is active on Sunday at noon, I cannot get truth on that check. This is because DDay.iCal's internal GetOccurrences() method doesn't return any dates that fall within that calendar event entry - which is a bug in their code.

Since it DDay.iCal is a completely dead project (at least the version we use, and the next version is not API compatible), I'm not sure what the best solution is.

1. We can do a partial fix to catch the "past midnight one time" scenarios, as they are probably the most common because of weekend check-in and leave the rest of it broken.
2. We can look at replacing DDay.iCal with ical.net, which is the newer version. Since they have different namespaces we might be able to have a pretty smooth transition by leaving the old DLL in case any plugins use it directly and then list it as deprecated "to be removed in version X" so there is crossover time for devs to switch to the new DLL.
3. We can just leave it as is.

I'm personally in favor of the second option, or at least in favor of looking into it more to see if it indeed can be done without massive work - AND to verify that it does indeed fix the problem.

[jonedmiston]

Option #2 is the best bet, but that would be a significant amount of work to change out and more importantly test. This would impact some pretty critical areas live events, check-in (very scary) and soon volunteer scheduling. We'd also want to ensure that there were a complete suite of test cases written before the change in each of these areas so we could ensure the quality through the change.

All this to say that #2 needs to happen, but it's a matter of when we'd have the significant amount of time to dedicate towards it.

[nairdo]

@cabal95 With replacement of DDay.iCal in Rock for v12 (develop branch), would you re-test this to let us know where things now stand with this issue?

[cabal95]

@nairdo Still get the exact same results.

Part of the problem is here: https://github.com/SparkDevNetwork/Rock/blob/develop/Rock/Model/Schedule.cs#L1097

The check looks only at the start time of day and doesn't take into account the day itself. I.e. the logic it uses is:

"Well, it's currently 10am and that time of day is before 8pm, so this schedule is not active"

rather than:

"Hmm, it's 10am on TUESDAY and the schedule starts at 8pm on MONDAY, so it might be active."

--

I think the code in that function needs to check if the schedule passes midnight. If not, then use the current logic. If it does pass midnight, it probably just needs to get the occurrences and then check each occurrence to see if one of them contains the date requested.

The logic to detect passing midnight might be something like

If ( CalEvent.End - CalEvent.Start ).TotalSeconds >= 86400 OR CalEvent.End.TimeOfDay < CalEvent.Start.TimeOfDay
THEN
  Use new logic to check each occurrence
ELSE
  Use current logic
END