Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QuickBooks does not handle UTC time properly, so we should have a way to handle timezone for the developer #19

Closed
jsgoupil opened this issue May 18, 2016 · 13 comments

Comments

@jsgoupil
Copy link
Owner

A request as such:
2016-05-17T20:00:00
Actually means that it checks for 20:00 LOCAL time on QuickBooks machine.

If we are in PST. For 20:00 local time, it's actually 3:00am (next day)

Now, if we set
request.FromModifiedDate = new DateTime(long.Parse(lastSync), DateTimeKind.Utc); // 3:00am UTC

This will actually end up requesting a real UTC time. Then QuickBooks will actually think you are requesting a 3:00am local time.

Then I have to end up doing this:
request.FromModifiedDate = new DateTime(long.Parse(lastSync), DateTimeKind.Utc).ToLocalTime(); // Which would be the client timezone

or

request.FromModifiedDate = Instant.FromDateTimeUtc(new DateTime(long.Parse(lastSync), DateTimeKind.Utc)).InZone(localTimeZone).ToDateTimeUnspecified();

@valmont
Copy link

valmont commented Jan 22, 2018

I need some help on this. We store our dates as UTC in MS SQL Server and are trying to use FromModifiedDate. Our Quick Books instance is running is PST and we are currently calling this:

FromModifiedDate = new DATETIMETYPE(SyncLog.LastRun, TimeZoneInfo.FindSystemTimeZoneById("Pacific Standard Time"))

SyncLog.LastRun is a DateTime object. How should we correctly use DATETIMETYPE to get our desired results?

@jsgoupil
Copy link
Owner Author

Hey @valmont
I mentioned in the bug how to handle it. You need to convert to local time. You can use DateTime.
The current passing of the TimeZone is only to handle the Daylight Time Saving bug. Which you can leave the SyncManager handle as well if you set the TimeZoneBugFix

@rogerfar
Copy link
Contributor

@jsgoupil A suggestion was to send the date to QuickBooks as "2016-05-17T20:00:00+0:00", have you tried this?

Issue I'm having is that I don't know which timezone our clients are on, so converting them to the server localtime is still potentially off by a few hours.

@jsgoupil
Copy link
Owner Author

He @farlock85 , this bug is a little old, so you can try to use the TimeZoneBugFix? I don't remember if trying to send that specific date works to QuickBooks but I don't think it does work. Let me know if it does?

Maybe you could get the timezone information from the CompanyQueryRs.
That way you get where the company is located.

@rogerfar
Copy link
Contributor

rogerfar commented Jan 31, 2019

@jsgoupil I updated the DATETIMETYPE.cs ToString() to

public override string ToString()
        {
            var k = value.ToString(" K", CultureInfo.InvariantCulture).Trim();

            // QuickBooks doesn't support Z format.
            if (k == "Z")
            {
                k = "+00:00";
            }

            return value.ToString("yyyy-MM-ddTHH:mm:ss", CultureInfo.InvariantCulture) + k;
        }

And that seems to work fine, I haven't tested in different timezones yet though.

@jsgoupil
Copy link
Owner Author

I can give it a shot, but it all depends what QuickBooks return to you when you make a request. If I remember well from a few years ago, the timezone was simply ignored. So I was missing some customers when querying.

@jsgoupil
Copy link
Owner Author

jsgoupil commented Feb 1, 2019

@farlock85 You seem to be right, with that fix, I think we could get rid of the entire TimezoneBugFix

@jsgoupil
Copy link
Owner Author

jsgoupil commented Feb 1, 2019

OK. After further investigation, the TimezoneBugFix is necessary...

As an example, I set my time to somewhere in April (when DST is active)
I modify 1 customer in QuickBooks.

image

Its modified date is off by 1h with the current time.

However, your fix might still be good.

@rogerfar
Copy link
Contributor

rogerfar commented Feb 2, 2019

Thanks @jsgoupil, appreciate all the effort, it helped us alot with our integration platform!

@jsgoupil
Copy link
Owner Author

jsgoupil commented Feb 2, 2019

@farlock85 You're welcome, I just added a way to add multiple requests in one step in this new version.
I also offer consulting if you guys are interested! You can find my info on my personal website / github profile.

@tofer
Copy link
Contributor

tofer commented Feb 6, 2019

I have been testing a completely different fix for this, and thought I would chime in here and see what your thoughts are. What got me going on this was the point you made above

If I remember well from a few years ago, the timezone was simply ignored

For all I can tell this seems to be correct. If we were to ignore the offset value at the end of the timestamp returned from QuickBooks, the time would be accurate with respect to the Local time of the machine QuickBooks is installed on. The fact that that is parsed with the timestamp is what's causing it to be adjust by an hour during DST when that offset is wrong.

While yes this is Intuit's fault, I think the simpler answer to solving it is to simply not parse the offset portion of the returned timestamp, and always considering it a local time with no zone or offset information. Consumers of the library, if needing an offset, could convert the value to a DateTimeOffset using the zone information they configure themselves.

I'm still working on testing all the various operations when leaving off the Offset for requests, but from all the documentation I've read from Intuit it doesn't seem like QuickBooks needs an offset provided on requests... it will just consider it local time.

To completely remove ambiguity, I think what this ideally calls for is to use Jon Skeet's excellent NodaTime library. I've been using it for a few years now, and it's really perfect for situations like this where DateTime is simply too ambiguous. The LocalDateTime type would be helpful, but of course the biggest problem with that is the massive breaking change that would impose.

The breaking change doesn't matter for me though, so I've been testing a branch were I use NodaTime.LocalDateTime as the wrapped value on DATETIMETYPE and NodaTime.LocalDate for DATETYPE. If it works out, I love using the NodaTime library so much I may just maintain a separate branch going forward for my use... we'll see.

@jsgoupil
Copy link
Owner Author

jsgoupil commented Feb 6, 2019

@tofer Do you think the recent changes break the correct "implementation" to talk with QuickBooks? In regards to getting the latest TimeUpdated and making another request afterwards.
My client is currently using an old version of this library and I haven't had time to update it.

@tofer
Copy link
Contributor

tofer commented Feb 6, 2019

Do you think the recent changes break the correct "implementation" to talk with QuickBooks? In regards to getting the latest TimeUpdated and making another request afterwards.

Just to be clear, you're referring to this change correct?

if (k == "Z")
{
    k = string.empty;
}

to

if (k == "Z")
{
    k = "+00:00";
}

"Z" should only happen for DateTimeKind.Utc dates, so if they are using the value of TimeUpdated as returned from QuickBooks, my guess is that this should be fine (as in this nets them no change).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants