Discussion: Formalizing approach to storing date and time values

[dpaquette]

Current Situation

AllReady stores campaign/event/task dates as the .NET DateTimeOffset type

• DateTimeOffset is a .NET Framework type that represents a particular point in time relative to UTC. It is a Date and Time with an Offset relative to UTC. (https://msdn.microsoft.com/en-us/library/system.datetimeoffset.aspx)
• Each Campaign has a Windows/.NET TimeZoneID specifying the timezone where the campaign is located. For example, a campaign in Seattle would have the TimeZoneID = "Pacific Standard Time"
• An Event within a campaign also has a TimeZoneID specifying the timezone where the event is located. The default TimeZoneID is the same as the campaign but can differ when events are located across the country.
• All Tasks for an event inherit the TimeZoneID from the event.
• A Campaign / Event / Task start and end date / time is stored as a DateTimeOffset with the offset set to the UTC offset according to the TimeZoneID of the campaign.
• For example, consider the following campaign located in Seattle

Event	Start Date Time with Offset	End Date Time with Offset
Event 1	10/23/2016 5:00 PM -7:00	10/23/2016 8:00 PM -7:00
Event 2	12/20/2016 5:00 PM -8:00	12/20/2016 8:00 PM -8:00

• Note the offset is different depending of whether or not the date and time falls during daylight savings time for the Seattle area. AllReady calculates this offset whenever date/time values are saved by calling TimeZoneInfo.GetUtcOffset for date/time that was specified by the user.
• //TODO: Add example here of how and why the offset if adjusted when values are saved

Potential Risks
Thank you to Jon Skeet for pointing out the following potential problems

• Since the offset for a particular DateTimeOffset is calculated and stored when a user creates (or modifies) a Campaign / Event / Task, there is the potential that changes to time zone rules could cause problems. As an example, if an event were scheduled several months in advance and changes to the daylight savings time rules for that timezone changed after the event was created, the offset for that event might not be stored correctly.
• AllReady uses Windows timezones which are updated via regular Windows Updates. These updates happen regularly but updates are typically not as timely as the IANA timezone database which is used by the Noda Time framework
• Both these issues could be resolved if we used the Noda time framework, stored values as local date time values and did time zone conversions on every read. // TODO: Add example if we decide this is the path forward
• Noda Time framework currently does not support .NET Core. Version 2.0 of Noda Time does support .NET core but it is currently in Alpha. It is considered stable but we should expect some API changes.

Display Campaign / Event / Task date time values

• The default behavior in AllReady is to display Campaign / Event / Task dates in the timezone where the campaign is located. Since the values are stored as a DateTimeOffset, this means we simply display the value using .ToString(). No additional timezone adjustments are necessary.
• To avoid confusing, we display the TimeZone after any date/time values as shown here:
  
• Optionally, a user can specify their local TimeZone in their user profile. If a user has specified their local timezone, we also display the Campaign / Event / Task datetimes in their local timezone as shown here:
  
  • TODO: // Add example code showing how this conversion happens

Date Time formats

• AllReady currenlty only supports the en-US culture, meaning that DateTime values are always displayed in MM/dd/yyyy format. This includes the input fields where users enter DateTime values.
  From Startup.cs

 // Configure the HTTP request pipeline.
var usCultureInfo = new CultureInfo("en-US");
app.UseRequestLocalization(new RequestLocalizationOptions
{
     SupportedCultures = new List<CultureInfo>(new[] { usCultureInfo }),
     SupportedUICultures = new List<CultureInfo>(new[] { usCultureInfo })
});

• This can be extended in the future by adding more supported cultures at startup. The challenge here however is in aligning ASP.NET's request culture (and associated DateTime format) with the momentjs DateTime format used by the eonasdan-bootstrap-datetimepicker UI component for datetime input fields.
• Currently, the datetimepicker is setup to display in the 'L' format for Dates and 'L LT' formats for DateTime. Since we don't include any additional momentjs locales, this forces the DateTime pictures to use the MM/dd/yyyy format.

 $(".datepicker").datetimepicker({
    format: 'L',
    showClose: false,
    toolbarPlacement: 'bottom'
});

$(".datetimepicker").datetimepicker({
    format: 'L LT',
    showClose: false,
    sideBySide: true,
   toolbarPlacement: 'bottom'
});

• ASP.NET Core request culture: https://docs.asp.net/en/latest/fundamentals/localization.html
• momentjs locales http://momentjs.com/docs/#/i18n/
• Datetimepicker control https://eonasdan.github.io/bootstrap-datetimepicker/#bootstrap-3-datepicker-v4-docs

[joelhulen]

This is a great summary! I've dealt with this issue several times, as has any developer dealing with events on a multi-regional or global scale. I've used Noda Time recently, and was pleasantly surprised with how it simplified things like timezone conversions.

However, one thing about your summary confuses me. In the 3rd bullet point underneath Potential Risks, you state the following:

Both these issues could be resolved if we used the Noda time framework, stored values as local date time values and did time zone conversions on every read.

Are you sure the stored values would be in local time, and not UTC? Storing in UTC has been a best practice for a long time, and even suggested as such by Jon Skeet. Can you please clarify this point?

[mgmccarthy]

@joelhulen, I was part of the email exchange with Jon Skeet, and in this email, he mentioned that storing local time, then converting to correct timezone'd DateTime would work on reads and writes.

But, storing the values as UTC and converting on reads writes would work as well... at my current job, that's exactly our approach to this problem... we store everything as UTC and have a abstraction injected into whatever class need to covert on reads/writes for DateTime values.

We could still use DateTimeOffset, but like @dpaquette mentioned above, the one place we could get into trouble with that is if we create a future event as DateTimeOffset, time zone rules change, and now our stored offset is wrong.

[mgmccarthy]

thanks for doing this @dpaquette

[MisterJames]

As an example, if an event were scheduled several months in advance and changes to the daylight savings time rules for that timezone changed after the event was created, the offset for that event might not be stored correctly

I just want to point out that this is a very outside edge case, with low frequency and chance of occurrence ( @tonysurma please correct me here if need be ). I'm not suggesting we completely negate the fact that it may come into play; however, we need to remember that local times don't tend to change so wholesale changes shouldn't be predicated on that use case.

That said, everyone kinda gets UTC, so from a simplicity standpoint I get the argument for devs coming on board the project and not having the DateTimeOffset friction. But UTC by no means solves the problem we're talking about, so we still need to know whether or not we actually need to be concerned about time zone rule changes. @tonysurma?

[dpaquette]

@mgmccarthy Storing the Campaign / Event / Task dates as UTC would bring up the same problem as DateTimeOffset. By converting to UTC at the time of saving a Campaign / Event / Task, we are effectively doing the exact same thing as calculating the Offset at the time of the change. If a timezone rule changed, the value saved in UTC time would now be 1 hr off when we converted it back to local time.

[mgmccarthy]

@dpaquette, you are right. We're still using the time zone info we have available at the time to do the conversion from "user's local time" to UTC to store in the db.

so the decision to make is, are the time zone rules changing (which should not happen that often) be considered important enough to the project to:

• wait for a ASP.NET Core compatible NodaTime and use that?
• store LocalDateTime in the db... then convert when reading? (I think Jon Skeet mentioned that)
• keep everything as DateTimeOffset? (which would required the work to be done in Issue Change Request, ItineraryRequest and Itinerary models date storage to DateTimeOffset... #1386). If we stay with what we have now, we need to consider DateTimeOffset as a barrier to entry to new contributors (but I don't think it will be)

[dpaquette]

Just an update on this. I will be working on some code samples for this on Sunday so we can hopefully get this nailed down.

[dpaquette]

See PR #1416 for current WIP related to this issue. I am proposing a approach that uses a custom model binder to do the adjustment for the Campaign or Event's timezone. This pulls the logic out of the controllers / command handlers and ensures the date time offsets are set correctly BEFORE they ever reach the controller.

I will be adding some documentation and unit tests later this week but wanted to share what I had working so far.

[mgmccarthy]

@dpaquette, this a great approach. Not only does it centralize the logic, but it makes it easy for new contributors to join w/out having to understand how we handle DateTime... they just need to understand when to use the attribute.

For the record, it looks like the model binder is meant to be used on write operations?

[stevejgordon]

Looks good @dpaquette. What's really handy is that what you've done regarding binding pretty much matches what I've job for a work requirement I had and have started writing up for my blog. So at least my approach looks good for that too!

[niwrA]

I wrote an extensive blog on this subject that seems appropriate for the problems here. Of course a lot of it is inspired by everything that John Skeet already solves in nodatime, but in summary what I think is useful for this project is this:

• do definitely centralize and wrap all datetime logic in your own implementation of an IDateTimeProvider. Then write meaningful functions in there that mean they are easily maintainable and self-documenting, such as .GetUserDateTimeNow, GetUserDateNow, GetLocationDateTimeNow, GetLoggingDateTimeNow. This class indeed needs to be injected so that it can be tested properly. Also make sure that if you use Now in a workflow/controller, use the DateTimeProvider at the top level to get the Now, and then pass on that value instead of asking for Now at several points in the controller because ... Now changes, obviously (but it is frequently forgotten).
• many of your datetimes for this project seem to be tightly coupled to your entities. Time is nothing without context, and in this case the context seems relatively simple.

For instance, a campaign takes place at a specific location. So all you really need are datetimes that do not have any localisation specified (similar to the basic DateTimeKind.Unspecified), so long as you can connect them to a location for which you known the timezone information, you are good.

Similarly, if a user specifies when he or she is available, then the start and endtime will be relative to her timezone. But here it should not matter whether the timezone changes or not. If I start to work at 7am in the summer, I don't suddenly get up at 6am in the winter. If I sent an email at 10am in the summer, I don't want to have the sent date tell me it was sent at 9am if I happen to look at the email in the winter. A store that opens at 7:00 in the summer also opens at 7 in the winter. Etc.

Using DateTimeKind.Unspecified all the way through your application (webclient to sql) means that you can also prevent issues at various transitions - your json date should not have timezone or utc indicators, and your SQL and EntityFramework interaction doesn't have to compensate for it, and you can use the most basic datetime field (as SQL's default datetime is basically DateTimeKind.Unspecified as well).

(Hopefully this helps rather than confuses!)

[tonysurma]

@dpaquette have we finalized the approach here and documented it somewhere in the wiki etc.? If so then with all the info on this issue (maybe link to it from the wiki?) we can close this. Thanks all!