From cf8d40ec00f584f77d10b0ca432a717cd002936d Mon Sep 17 00:00:00 2001 From: James Murty Date: Tue, 18 Oct 2016 17:20:24 +1100 Subject: [PATCH] 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 | 64 +++++++++++++++------ icekit_events/tests/tests.py | 107 +++++++++++++++++++++-------------- 2 files changed, 112 insertions(+), 59 deletions(-) diff --git a/icekit_events/models.py b/icekit_events/models.py index baa56d6..71cd3ad 100644 --- a/icekit_events/models.py +++ b/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 diff --git a/icekit_events/tests/tests.py b/icekit_events/tests/tests.py index 4a06448..674ed1a 100644 --- a/icekit_events/tests/tests.py +++ b/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))