Update DATETIMETYPE for better QuickBooks UTC offset bug handling

[tofer]

I made some significant changes to DATETIMETYPE to have it inherently handle the UTC offset bug, even without a TimeZoneBugFix set (though still supported), as well as honor the behavior of the built in DateTime type as best I could. I made a few breaking changes where I thought it was critical, but as few as possible, marking Obsolete in other situations.

I included LOTS of tests, and also verified that all tests work in multiple time zones, including UTC.

QB testing results that predicated these changes

It appears that the DATETIMETYPE values returned from QuickBooks have correct times, it is just the UTC offset that is incorrect [This was already established, but wanted to confirm]. Seems they just always append the BaseUtcOffset for the local time zone of the QuickBooks host computer.

I tested query requests using dates without an offset, and QuickBooks interpreted that value as the local time of the host computer (This matches what their documentation says).

Combining this together, I tested using the value as returned from quickbooks with the offset ignored to query a date range. My test customer had a TimeModified reported from QuickBooks of 2018-08-01T16:05:45-08:00. This should have been -07:00 for this date in my time zone (Pacific), A query with range 2018-08-01T16:05:00 to 2018-08-01T16:06:00 returned the same result.

Basic summary of changes

To most easily handle the offset issue, DATETIMETYPE needed to handle parsing the dates without the offset, as well as being able to query without an offset, so the same returned value could be used in a query.

• DATETIMETYPE now has an option offset. If no TimeZoneBugFix is specified, or it is parsed from a string without an offset, No offset will be included.
• Handling of the offset correction has been moved to ReadXml, since this is only an issue when reading from QuickBooks. Constructors with TimeZoneInfo have been deprecated.
• Added support for construction using a DateTimeOffset
• Added range validation (1970 - 2038)
• Deprecated implicit cast from DateTime, since a ArgumentOutOfRangeException may be thrown
• Deprecated implicit cast to DateTime, since there is a loss of data
• ToDateTime() returns date with correct DateTimeKind depending on the situation (generally DateTimeKind.Unspecified)
• Deprecated construction from a string, in favor of static Parse method
• A bunch of other things, but I'll let the code and tests speak for the rest

NOTE: If you like these changes, and think you want to accept them, the other TYPE classes should probably also be updated to have similar style and usage, which I can do before you merge if you want

[tofer]

Glad you like it.

Firstly, yes of course I'll fix the coding style items. I tried to match your style while coding, but old habits die hard.

Regarding the implicit casting, I had lots of thoughts about this last night, and tried even more experiments with both the existing version and my changes to double check my logic, and ultimately I came to the conclusion that there has to be a concession one way or the other. I'm not sure how to express my thoughts succinctly, but I'll give it a go [EDIT: Definitely not succinct]

The existing version of DATETIMETYPE pretty much "just works", but requires the bug fix time zone to be configured. It can pretty much just cast back and forth without issue in many situations, but unfortunately not all. The problems with casting in my changed version get worse, and are of my own making, but are a result of both the UTC bug handling, and the changes I made to solve issues in certain not-as-obvious situations. I'll highlight some of these below.

NOTE: There are additional issues with casting FROM System.DateTime I may address in a later follow-up. This is already horrendously long

Time value changed to match local time zone

Since System.DateTime doesn't store an offset, it will convert any parsed dates that supplied one into a machine local time zone to retain its "point in time" accuracy. Technically, if you directly cast to a System.DateTime and then back, or pass the same DATETIMETYPE object back to QuickBooks, it will give you the correct results. However, the value does not represent accurately what QuickBooks returned.

var dtt = new DATETIMETYPE("2019-01-01T12:00:00-10:00"); // QuickBooks in Hawaii time zone
var str = dtt.ToString(); // Server in Pacific time: 2019-01-01T14:00:00-08:00 

While this is the same moment to the computer, the time component reads differently to a human. OK, this may not be an actual issue, and may just be my preference; However, this can trickle into an actual problem. Let's continue...

After casting/converting to DateTime, default ToString() format does not include offset

DateTime date = dtt;
var str2 = date.ToString(); // 1/1/2019 2:00:00 PM

Since DATETIMETYPE implicitly casts to System.DateTime, this can happen really easily. Now the default format no longer includes the offset or zone. This may be confusing to an end-user seeing a time modified that of 2:00pm instead of their local time of 12:00pm. Its only off by 2 hours in this example, but this could very easily by off by 8 or 10 hours if the server is in UTC (like many of mine are).

OK, so far this can still just be considered preference, but still going forward, a true issue can happen...

Storing as string, or other loss of kind/offset value, will cause the date to be inaccurate

If the developer then stores that above string value to persist it, when reconstructing later the time be off by the difference between the server's and QuickBooks host computer's time zones.

var dtt = new DATETIMETYPE("1/1/2019 2:00:00 PM");
var str = dtt.ToString(); // 2019-01-01T14:00:00

This date will be interpreted as local time for the QuickBooks host computer, which is now 2 hours later than the originally returned value, and if querying since last modified, any records in that 2 hour span will not be returned.

---

So.....

To address some of the above issues, as well as the UTC bug fix, part of my changes to DATETIMETYPE were to retain the (corrected) offset as returned from QuickBooks, and to not convert the value into the server's local time zone. To support installs without a bug fix time zone configured (which would be nice, since this could easily go wrong) default behavior is to ignore the offset, and not include it in ToString().

After all the changes to accomplish this, DATETIMETYPE on its own felt good to me. It seemed to more accurately represented the date as it was stored in QuickBooks. It could handle UTC offset issues out of the box. It also didn't suffer some of the problems with DateTime formatting highlighted above. If a time zone was supplied, we could even convert to System.DateTimeOffset which is way less ambiguous. We did have a new problem though...

Not converting the time to the application server's local time means the offset in DATETIMETYPE may not match the offset of the server the application is running on. Because of this, I could not return a DateTimeKind.Local date without re-introducing the issues highlighted above. It has to be DateTimeKind.Unspecified (unless QuickBooks is in UTC), so an offset could no longer be inferred from the resulting System.DateTime. In most situations, this still works fine though, as QuickBooks will infer this in the local time of its host computer, which is accurate since the time component was not modified. Ultimately though there is some loss of resolution here.

A note about Unspecified dates. Microsoft for some reason will treat an Unspecified date as Local for ToUniversalTime(). This has the additional lovely consequence of creating a time that is off by the difference in clocks between the QuickBooks host and application server. This is partly my reason for adding TryGetDateTimeOffset() as an alternative to this conversion, indicating it might not be possible to have a time relative to UTC

Bringing this full-circle

Coming all the way back around to the issue of implicit casting, the decision seemed relatively clear to me. ToDateTime() loses information and has some oddities. If data is lost in a type conversion, that seems like a good case for not including an implicit cast. Because this was a previous feature though, and not completely terrible, ultimately it seemed OK to simply mark the cast as Obsolete without compiler errors, so it could still be technically used.

Really, if we want to be absolutely clear about what is going on, I would like to see ToQuickBooksLocalDateTime(), TryGetLocalDateTime(), TryGetUtcDateTime(), and TryGetDateTimeOffset() as conversions.

I can totally understand your desire to have this type be as transparent as possible. I'm just not convinced that is more helpful to the developer, as it is hiding potential issues they may have if they don't understand all the underlying intricacies.

Options moving forward

Now that I've beaten this to death, some potential choices for moving forward 1) I've made you a believer. No changes. 2) I've really convinced you, remove implicit cast completely. 3) Include implicit cast as is returning the unspecified kind so it retains the QuickBooks host computer's local time (but no offset). 4) Include implicit cast, but convert the time to the application server's local time zone (if possible). This will re-introduce the issues detailed above. 5) Woah, this got way too complicated and crazy. Abandon ship completely.

Sorry this was so long. Probably not worth this much effort, but date challenges are interesting to me for some reason, and working on this repo is really fun. Thanks for entertaining me, and for your fast responses.

[jsgoupil]

I'm convinced 👍 I would go with 1 and 2. Since we are making a breaking change, I wouldn't leave the Obsolete in there. It's a small library and we are not shipping on a fast cadence.

Also, I think it would be beneficial to show how to use this in the WebApplicationSample/CustomerQuery the right way. Basically saving the latest TimeModified in the response, and use the MinValue if we don't have a saved value or the correct DateTime in the request.

I have some sample code from another application that we can use. You probably have some too, let's merge the best of both? My code is on the old version of the NuGet package. Also my code is using other repositories that are not present in this sample, and I am using NodaTime to handle the timezone, but your code update should handle this better right? Do you mind to give that a shot? If you spent too much time on this, I can try to handle it.

if (response.CustomerRet.Count() > 0)
{
    var lastModifiedCustomer = response.CustomerRet.OrderBy(c => c.TimeModified).Last();
    contractorSettingRepository.SaveIfNewer(MODIFIED_SETTING, lastModifiedCustomer.TimeModified, contractorId, authenticatedTicket.GetLoginId());
}

public static void SaveIfNewer(this IContractorSettingRepository2 contractorSettingRepository, string setting, QbSync.QbXml.Objects.DATETIMETYPE moment, int contractorId, string loginId)
{
    var existingTicks = contractorSettingRepository.GetSetting(setting, contractorId)?.Value;
    var existingTicksLong = 0L;
    var ticks = moment.ToDateTime().ToUniversalTime().AddSeconds(1).Ticks;
    if (existingTicks == null || (long.TryParse(existingTicks, out existingTicksLong) && existingTicksLong < ticks))
    {
        contractorSettingRepository.SaveSetting(setting, ticks.ToString(), contractorId, loginId, true);
    }
}

var lastSync = contractorSettingRepository.GetSettingValue(MODIFIED_SETTING, contractorId) ?? new DateTime(1971, 1, 1, 0, 0, 0, DateTimeKind.Utc).Ticks.ToString();

var ret = await base.ExecuteRequestAsync(authenticatedTicket, request);

if (request.iterator == IteratorType.Start)
{
    var contractor = authenticatedTicket.GetContractor();
    request.FromModifiedDate = Instant.FromDateTimeUtc(new DateTime(long.Parse(lastSync), DateTimeKind.Utc)).InZone(contractor.LocalTimeZone).ToDateTimeUnspecified();
}

We would probably need to add a

public DbSet<QbSetting> QbSettings { get; set; }
        public class QbSetting
        {
            public int Id { get; set; }
            public string Name { get; set; }
            public string Value { get; set; }
        }

[tofer]

Awesomesauce!

Yes, I agree about showing usage in the sample application. I have definitely spent too much time on this, but certainly doesn't mean I won't spend more. I'll do some additional cleanup on this, and see if I can take a crack as the sample app.

Also since you brought up NodaTime... one option I was thinking of today would be to actually ship a separate package to add NodaTime support to this library. It would be pretty simple, mostly adding some extension methods to DATETIMETYPE.

[tofer]

I was just reviewing your sample code, and regarding

var ticks = moment.ToDateTime().ToUniversalTime().AddSeconds(1).Ticks;

Makes me wonder if we should rename ToDateTime(). My code changes will change this output slightly, and since you use ToUniversalTime(), you are going to fall victim to one of the issues I mentioned:

A note about Unspecified dates. Microsoft for some reason will treat an Unspecified date as Local for ToUniversalTime(). This has the additional lovely consequence of creating a time that is off by the difference in clocks between the QuickBooks host and application server. This is partly my reason for adding TryGetDateTimeOffset() as an alternative to this conversion, indicating it might not be possible to have a time relative to UTC

Renaming the method would force you to verify your decision everywhere it was used. Not renaming would allow it to compile, but in your case may cause your app to do weird things.

Thoughts?

[jsgoupil]

It would be a good idea if you want to add support to Nodatime with another package.

If you go that route, I'll also refactor some stuff in the WebConnector, there are great classes in there that I need to bring in other projects but that project depends on SoapCore and that's no good.

I'll wait for you to finish though. And I can hook up the nuget packages and stuff, that's never simple but I think I understand a bit better :) Just make sure to target the correct framework(s).

As for renaming the method name... That method is quite the correct name 😃 I think I'll do a +1 major on all the packages.