We might need to restore start_date and end_date fields.

[mrmachine]

@cogat @jmurty

Here are some points to consider (maybe they will help us write failing tests and decide if we can survive with just datetime fields for regular and all day events):

1. Django localises datetime fields to UTC before saving to the database for consistency. When given a naive datetime, it assumes it is in the current/default timezone then localises it to UTC.

2. For us to consistently store/retrieve a date from a timezone aware datetime field, we have to do some manual conversion in Python in both directions (to/from UTC) every single time we interact with the DB field, so Django doesn't do any localisation for us. I think this conversion can get tricky when filtering events near or that cross a DST boundary.

3. This requirement is especially easy for new developers and template authors to miss. A template with {{ event.starts|date }} will output a date and time localised to the current timezone. We have to do {{ event.starts|date:"d/m/Y" }} to get just the date (and can't take advantage of the default DATE_FORMAT setting), and it will be the wrong date if the current timezone is earlier than UTC.

4. When some objects (regular events) don't need this conversion, but others (all day events) do, and they are both stored in the same field, things get complicated and fragile. I'm not sure if it's even possible to sort such a queryset of all day and regular events together. A regular event on 21 June at 07:00 AEST will be saved as 20 June at 21:00 UTC. An all day event will be saved as 21 June at 00:00 UTC. If we just sort by starts, a bunch of all day events will appear somewhere in the middle of a sequence of regular events. For a single day, you could sort/group by is_all_day first, but for a sequence of events spanning multiple days, that wouldn't work (at least not without some complicated ORM/SQL or Python iteration).

5. Events can be toggled from all day and back in the change form. If there's only one DB field, this becomes more complicated to implement than simply hiding one field or the other in the form, and we potentially lose data if we toggle from a regular event to an all day event and then back again.

6. I am against using <= 23:59:59.999999 (inclusive) for ends instead of < 00:00:00.000000 (exclusive). It seems semantically incorrect and fragile. One full day is 24 hours, not 1 microsecond less than 24 hours. With this setup, ends - starts is not timedelta(days=1). To get a 2 day event you can't just do ends = starts + timedelta(days=2). And the fullcalendar.js docs say (about the end date): "It is the moment immediately after the event has ended. For example, if the last full day of an event is Thursday, the exclusive end of the event will be 00:00:00 on Friday!" -- I think using 23:59:59.999999 is perhaps clearer in natural language to avoid date ambiguity, but in code it's just unecessary complexity.

7. If we have starts and starts_date fields, we don't need to do any conversion for all day events. We just get/set date objects. For regular events, we just get/set datetime objects and we additionally derive and store a denormalised date from the datetime field one time only in pre_save (which happens before Django strips the tzinfo by localising to UTC), that we can use to sort/group/filter/display many times without conversion. A queryset sorted by date_starts, starts with events spanning multiple days will group all events by date and group "all day" events together before regular events within each individual day.

[jmurty]

I think Tai is right, the two main compelling reasons to store a naive date field separately are to:

• avoid fiddly and non-obvious explicit conversions of date times into and out of all-day events; and
• to make it possibly to sort all-day events first with just DB queries (edited)

We could do everything but the in-DB sorting with datetimes alone, but I kept finding extra cases where special handling was required but missing in the current implementation.

Fighting with Django's inbuilt datetime handling is not worth the complexity cost vs adding date-only fields to sidestep it.

I think we can get away with adding date-only fields only to Occurrences, without needing to also add these fields to the generators or mess with the admin too much. But we will need detailed test cases (then unit tests) to properly flush out all the issues.

My recommendations (not fully considered, so potentially misguided or wrong) are:

• add start_date and end_date date fields to the Occurrence model. These fields are non-nullable and always set with the naive date when an occurrence is saved -- whether it is an all-day or timed occurrence
• adjust EventRepeatsGenerator to store a "zeroed" UTC-timezone value when it is an all-day occurrence generator. This is already the case, but it may not be guaranteed to be UTC. When a generator generates all-day occurrences, it will retrieve the UTC timestamp and use the naive date value to set the new start_date and end_date fields
• add default ordering for Occurrence by start_date then start to order all-day events first (regardless of timezone) followed by timed events within the timezone
• adjust Occurrence queryset filters – and any other filtering through icekit-events or django-icekit – to use the new start_date and end_date fields when appropriate
• add unit tests that create all-day and timed events in a range of timezones (Pacific / AEST / GMT) then confirm that they a) sort as we want with naive all-day events listed first, and b) produce expected results when you fetch the start_date and start fields in different timezone contexts (e.g. as a user in Pacific / AEST / GMT timezones)
• add unit tests for the calendar widget data generated in the admin for similar Pacific / AEST / GMT situations, to confirm both that this widget is working as expected and that the timezone handling works properly in a real use-case.

[cogat]

I'm not opposed, but it would be good to do this on the back of some demonstrable test failures.

(If we go this route, it would be handy to use the date fields to (redundantly) store the localised date of the (UTC) datetime for is_all_day=False events, because that would make querying occurrences that start and finish on the same day more efficient.)

[mrmachine]

@cogat I think that's the idea, and what we used to do before the refactor. All events (occurrences now) will have a start/end date. For all day events, the date will be chosen explicitly. For regular events, the date will be derived from the timezone aware datetime at the time of input. So we can query all events by date to group/filter/sort.

@jmurty I'm curious about your thinking behind only adding date fields to occurrences (and not repeat generators). Do we gain anything from this, or is it just for the sake of less change?

I think the concept of "zeroed" dates stored in datetime fields is confusing and error-prone enough (especially if a single field is storing both zeroed and non-zeroed values) and would need a level of explanation for any new developers that would outweigh the one time cost of adding back a date field and simply toggling the date/datetime fields in the admin. This was previously implemented, so we should be able to find that code in commit history.