Always use naive times in RRULE spec to respect local time definitions

Convert RRULE document spec to always use naive date/time definitions
for portions like DTSTART and RDATE instead of UTC timestamps, to avoid
situations where a repeating event starting at 9am each day ends up with
later occurrences starting at 8am or 10am instead because of daylight
savings changes during the course of the repetition.

This change had knock-on effects when comparing existing Occurrence
datetimes against RRULE-generated datetimes, since the former are often
timezone-aware and the latter are now always naive datetimes.

icekit_events/models.py

@@ -76,13 +76,18 @@ def default_date_ends():
     return default_date_starts() + appsettings.DEFAULT_DATE_ENDS_DELTA
 
 
-def format_ical_dt(date_or_datetime):
-    """ Return datetime formatted for use in iCal """
+def format_naive_ical_dt(date_or_datetime):
+    """
+    Return datetime formatted for use in iCal as a *naive* datetime value to
+    work more like people expect, e.g. creating a series of events starting
+    at 9am should not create some occurrences that start at 8am or 10am after
+    a daylight savings change.
+    """
     dt = coerce_dt_awareness(date_or_datetime)
     if is_naive(dt):
         return dt.strftime('%Y%m%dT%H%M%S')
     else:
-        return dt.astimezone(pytz.utc).strftime('%Y%m%dT%H%M%SZ')
+        return dt.astimezone(get_current_timezone()).strftime('%Y%m%dT%H%M%S')
 
 
 def coerce_dt_awareness(date_or_datetime, tz=None):
@@ -91,20 +96,36 @@ def coerce_dt_awareness(date_or_datetime, tz=None):
     timezone-naive `datetime` result, depending on which is appropriate for
     the project's settings.
     """
-    if tz is None:
-        tz = get_current_timezone()
     if isinstance(date_or_datetime, datetime):
         dt = date_or_datetime
     else:
         dt = datetime.combine(date_or_datetime, datetime_time())
     is_project_tz_aware = settings.USE_TZ
-    if is_project_tz_aware and is_naive(dt):
-        return make_aware(dt, tz)
-    elif not is_project_tz_aware and is_aware(dt):
-        return make_naive(dt, tz)
+    if is_project_tz_aware:
+        return coerce_aware(dt, tz)
+    elif not is_project_tz_aware:
+        return coerce_naive(dt, tz)
     # No changes necessary
     return dt
 
+
+def coerce_naive(dt, tz=None):
+    if is_naive(dt):
+        return dt
+    else:
+        if tz is None:
+            tz = get_current_timezone()
+        return make_naive(dt, tz)
+
+
+def coerce_aware(dt, tz=None):
+    if is_aware(dt):
+        return dt
+    else:
+        if tz is None:
+            tz = get_current_timezone()
+        return make_aware(dt, tz)
+
 # FIELDS ######################################################################
 
 
@@ -506,6 +527,10 @@ def generate(self, until=None):
                 until = self.repeat_end
             else:
                 until = timezone.now() + appsettings.REPEAT_LIMIT
+        # Make datetimes naive, since RRULE spec contains naive datetimes so
+        # our constraints must be the same
+        start_dt = coerce_naive(start_dt)
+        until = coerce_naive(until)
         # Determine duration to add to each start time
         occurrence_duration = self.duration or timedelta(days=1)
         # `start_dt` and `until` datetimes are exclusive for our rruleset
@@ -538,13 +563,13 @@ def _build_complete_rrule(self, start_dt=None, until=None):
                 or timezone.now() + appsettings.REPEAT_LIMIT
         # We assume `recurrence_rule` is always a RRULE repeat spec of the form
         # "FREQ=DAILY", "FREQ=WEEKLY", etc?
-        rrule_spec = "DTSTART:%s" % format_ical_dt(start_dt)
+        rrule_spec = "DTSTART:%s" % format_naive_ical_dt(start_dt)
         if not self.recurrence_rule:
-            rrule_spec += "\nRDATE:%s" % format_ical_dt(start_dt)
+            rrule_spec += "\nRDATE:%s" % format_naive_ical_dt(start_dt)
         else:
             rrule_spec += "\nRRULE:%s" % self.recurrence_rule
             # Apply this event's end repeat
-            rrule_spec += ";UNTIL=%s" % format_ical_dt(
+            rrule_spec += ";UNTIL=%s" % format_naive_ical_dt(
                 until - timedelta(seconds=1))
         return rrule_spec
 
@@ -782,19 +807,24 @@ def is_past(self):
     def get_absolute_url(self):
         return self.event.get_absolute_url()
 
+
 def get_occurrence_times_for_event(event):
     """
-    Return a tuple with two sets containing the (start, end) datetimes of an
-    Event's Occurrences, or the original start datetime if an Occurrence's
-    start was modified by a user.
+    Return a tuple with two sets containing the (start, end) *naive* datetimes
+    of an Event's Occurrences, or the original start datetime if an
+    Occurrence's start was modified by a user.
     """
     occurrences_starts = set()
     occurrences_ends = set()
     for start, original_start, end, original_end in \
             event.occurrences.all().values_list('start', 'original_start',
                                                 'end', 'original_end'):
-        occurrences_starts.add(original_start or start)
-        occurrences_ends.add(original_end or end)
+        occurrences_starts.add(
+            coerce_naive(original_start or start)
+        )
+        occurrences_ends.add(
+            coerce_naive(original_end or end)
+        )
     return occurrences_starts, occurrences_ends
 
 

icekit_events/tests/tests.py

@@ -19,6 +19,7 @@
 from django.forms.models import fields_for_model
 from django.test import TestCase
 from django.test.utils import override_settings
+from django.utils.timezone import make_naive
 
 from django_dynamic_fixture import G
 from django_webtest import WebTest
@@ -28,8 +29,6 @@
 from icekit_events.models import get_occurrence_times_for_event
 from icekit_events.utils import timeutils
 
-from timezone.timezone import localize, now
-
 
 class TestAdmin(WebTest):
 
@@ -727,9 +726,9 @@ def test_event_without_occurrences(self):
 
 class TestEventRepeatsGeneratorStrangeness(TestCase):
     def setUp(self):
-        self.start = datetime(2016,10,1, 9,0)
-        self.end = datetime(2016,10,1, 17,0)
-        self.repeat_end = datetime(2016,10,31, 17,0)
+        self.start = timezone.datetime(2016,10,1, 9,0)
+        self.end = timezone.datetime(2016,10,1, 17)
+        self.repeat_end = timezone.datetime(2016,10,31, 17)
 
         self.event = G(SimpleEvent)
 
@@ -745,14 +744,14 @@ def test_daily_occurrences(self):
         occurrences = self.event.occurrences.all()
         self.assertEquals(occurrences.count(), 31)
 
-        st = localize(self.start).time()
-        et = localize(self.end).time()
+        st = timezone.localize(self.start).time()
+        et = timezone.localize(self.end).time()
         self.assertEquals(st, time(9,0))
         self.assertEquals(et, time(17,0))
 
         for o in occurrences:
-            self.assertEquals(localize(o.start).time(), st)
-            self.assertEquals(localize(o.end).time(), et)
+            self.assertEquals(timezone.localize(o.start).time(), st)
+            self.assertEquals(timezone.localize(o.end).time(), et)
 
 class TestEventRepeatsGeneratorModel(TestCase):
 
@@ -764,6 +763,11 @@ def setUp(self):
             rounding=timeutils.ROUND_DOWN)
         self.end = self.start + appsettings.DEFAULT_ENDS_DELTA
 
+        self.naive_start = make_naive(
+            self.start, timezone.get_current_timezone())
+        self.naive_end = make_naive(
+            self.end, timezone.get_current_timezone())
+
     def test_uses_recurrencerulefield(self):
         """
         Test form field and validators.
@@ -882,28 +886,28 @@ def test_limited_daily_repeating_generator(self):
         # Repeating generator has expected date entries in its RRULESET
         rruleset = generator.get_rruleset()
         self.assertEqual(20, rruleset.count())
-        self.assertTrue(self.start in rruleset)
-        self.assertTrue(self.start + timedelta(days=1) in rruleset)
-        self.assertTrue(self.start + timedelta(days=2) in rruleset)
-        self.assertTrue(self.start + timedelta(days=19) in rruleset)
-        self.assertFalse(self.start + timedelta(days=20) in rruleset)
+        self.assertTrue(self.naive_start in rruleset)
+        self.assertTrue(self.naive_start + timedelta(days=1) in rruleset)
+        self.assertTrue(self.naive_start + timedelta(days=2) in rruleset)
+        self.assertTrue(self.naive_start + timedelta(days=19) in rruleset)
+        self.assertFalse(self.naive_start + timedelta(days=20) in rruleset)
         # Repeating generator generates expected start/end times
         start_and_end_times_list = list(generator.generate())
         self.assertEqual(20, len(start_and_end_times_list))
         self.assertEqual(
-            (self.start, self.end),
+            (self.naive_start, self.naive_end),
             start_and_end_times_list[0])
         self.assertEqual(
-            (self.start + timedelta(days=1), self.end + timedelta(days=1)),
+            (self.naive_start + timedelta(days=1), self.naive_end + timedelta(days=1)),
             start_and_end_times_list[1])
         self.assertEqual(
-            (self.start + timedelta(days=2), self.end + timedelta(days=2)),
+            (self.naive_start + timedelta(days=2), self.naive_end + timedelta(days=2)),
             start_and_end_times_list[2])
         self.assertEqual(
-            (self.start + timedelta(days=19), self.end + timedelta(days=19)),
+            (self.naive_start + timedelta(days=19), self.naive_end + timedelta(days=19)),
             start_and_end_times_list[19])
         self.assertFalse(
-            (self.start + timedelta(days=20), self.end + timedelta(days=20))
+            (self.naive_start + timedelta(days=20), self.naive_end + timedelta(days=20))
             in start_and_end_times_list)
 
     def test_unlimited_daily_repeating_generator(self):
@@ -915,33 +919,33 @@ def test_unlimited_daily_repeating_generator(self):
         )
         # Repeating generator has expected date entries in its RRULESET
         rruleset = generator.get_rruleset()
-        self.assertTrue(self.start in rruleset)
-        self.assertTrue(self.start + timedelta(days=1) in rruleset)
-        self.assertTrue(self.start + timedelta(days=2) in rruleset)
+        self.assertTrue(self.naive_start in rruleset)
+        self.assertTrue(self.naive_start + timedelta(days=1) in rruleset)
+        self.assertTrue(self.naive_start + timedelta(days=2) in rruleset)
         # Default ``appsettings.REPEAT_LIMIT`` is 13 weeks
-        self.assertTrue(self.start + timedelta(days=7 * 13) in rruleset)
-        self.assertFalse(self.start + timedelta(days=7 * 13 + 1) in rruleset)
+        self.assertTrue(self.naive_start + timedelta(days=7 * 13) in rruleset)
+        self.assertFalse(self.naive_start + timedelta(days=7 * 13 + 1) in rruleset)
         # Repeating generator generates expected start/end times
         start_and_end_times = generator.generate()
         self.assertEqual(
-            (self.start, self.end),
+            (self.naive_start, self.naive_end),
             next(start_and_end_times))
         self.assertEqual(
-            (self.start + timedelta(days=1), self.end + timedelta(days=1)),
+            (self.naive_start + timedelta(days=1), self.naive_end + timedelta(days=1)),
             next(start_and_end_times))
         self.assertEqual(
-            (self.start + timedelta(days=2), self.end + timedelta(days=2)),
+            (self.naive_start + timedelta(days=2), self.naive_end + timedelta(days=2)),
             next(start_and_end_times))
         for i in range(16):
             next(start_and_end_times)
         self.assertEqual(
-            (self.start + timedelta(days=19), self.end + timedelta(days=19)),
+            (self.naive_start + timedelta(days=19), self.naive_end + timedelta(days=19)),
             next(start_and_end_times))
         # Default ``appsettings.REPEAT_LIMIT`` is 13 weeks
         for i in range(13 * 7 - 20):
             next(start_and_end_times)
         self.assertEqual(
-            (self.start + timedelta(days=91), self.end + timedelta(days=91)),
+            (self.naive_start + timedelta(days=91), self.naive_end + timedelta(days=91)),
             next(start_and_end_times))
 
 
@@ -975,11 +979,15 @@ def test_initial_event_occurrences_automatically_created(self):
         first_occurrence = event.occurrences.all()[0]
         for days_hence in range(20):
             start = first_occurrence.start + timedelta(days=days_hence)
-            self.assertTrue(start in occurrence_starts,
-                            "Missing start time %d days hence" % days_hence)
+            self.assertTrue(
+                make_naive(start, timezone.get_current_timezone())
+                in occurrence_starts,
+                "Missing start time %d days hence" % days_hence)
             end = first_occurrence.end + timedelta(days=days_hence)
-            self.assertTrue(end in occurrence_ends,
-                            "Missing end time %d days hence" % days_hence)
+            self.assertTrue(
+                make_naive(end, timezone.get_current_timezone())
+                in occurrence_ends,
+                "Missing end time %d days hence" % days_hence)
         # Confirm Event correctly returns first & last occurrences
         self.assertEqual(
             self.start, event.get_occurrences_range()[0].start)
@@ -1036,11 +1044,18 @@ def test_unlimited_daily_repeating_occurrences(self):
             event.occurrences.all()[19].start)
         # Default repeat limit prevents far-future occurrences but we can
         # override that if we want
-        event.extend_occurrences(until=self.end + timedelta(days=999))
+        event.extend_occurrences(
+            until=self.end + timedelta(days=999, seconds=1))
         self.assertEqual(1000, event.occurrences.all().count())
-        self.assertEqual(
-            self.start + timedelta(days=999),
-            event.occurrences.all()[999].start)
+        # We need to be careful comparing timese because our test start time
+        # is affected by daylight savings changes whereas the occurrence start
+        # time will remain at 9am (for example) regardless of daylight saving
+        # changes.
+        delta = \
+            self.start + timedelta(days=999) \
+            - event.occurrences.all()[999].start
+        self.assertTrue(
+            delta <= timedelta(hours=1) or delta <= timedelta(hours=-1))
 
     def test_add_arbitrary_occurrence_to_nonrepeating_event(self):
         event = G(SimpleEvent)
@@ -1060,8 +1075,12 @@ def test_add_arbitrary_occurrence_to_nonrepeating_event(self):
         self.assertTrue(added_occurrence.is_user_modified)
 
     def test_add_arbitrary_occurrences_to_repeating_event(self):
-        arbitrary_dt1 = self.start + timedelta(days=3, hours=-2)
-        arbitrary_dt2 = self.start + timedelta(days=7, hours=5)
+        arbitrary_dt1 = make_naive(
+            self.start + timedelta(days=3, hours=-2),
+            timezone.get_current_timezone())
+        arbitrary_dt2 = make_naive(
+            self.start + timedelta(days=7, hours=5),
+            timezone.get_current_timezone())
         event = G(SimpleEvent)
         generator = G(
             models.EventRepeatsGenerator,
@@ -1123,8 +1142,12 @@ def test_cancel_arbitrary_occurrence_from_repeating_event(self):
         occurrence_to_cancel_1 = event.occurrences.all()[3]
         occurrence_to_cancel_2 = event.occurrences.all()[5]
         occurrence_starts, __ = get_occurrence_times_for_event(event)
-        self.assertTrue(occurrence_to_cancel_1.start in occurrence_starts)
-        self.assertTrue(occurrence_to_cancel_2.start in occurrence_starts)
+        self.assertTrue(
+            make_naive(occurrence_to_cancel_1.start, timezone.get_current_timezone())
+            in occurrence_starts)
+        self.assertTrue(
+            make_naive(occurrence_to_cancel_2.start, timezone.get_current_timezone())
+            in occurrence_starts)
         # Cancel occurrences
         event.cancel_occurrence(occurrence_to_cancel_1)
         event.cancel_occurrence(
@@ -1228,5 +1251,5 @@ def test_round_datetime(self):
         )
         for dt1, dt2, precision, rounding in data:
             self.assertEqual(
-                timeutils.ROUND_datetime(datetime(*dt1), precision, rounding),
+                timeutils.round_datetime(datetime(*dt1), precision, rounding),
                 datetime(*dt2))