closeio / closeio-api

Python API Client for Close
http://developer.close.com/
MIT License
65 stars 47 forks source link

datetimes do not take timezone offset into account properly. #91

Closed eengoron closed 6 years ago

eengoron commented 6 years ago

As an example, today I ran this request:

resp = api.get('report/activity/ORGID', params=
{'user_id':'USERID', 'date_start':'2018-01-02', 'date_end':'2018-01-02'})

The response I got back was:

{
    "revenue_created_annual_created": 0, 
    "opportunities_created": 0, 
    "revenue_lost_annual_created": 0, 
    "sms_sent": 0, 
    "leads_created": 0, 
    "opportunities_lost": 0, 
    "revenue_won_monthly": 0, 
    "revenue_won_annual": 0, 
    "leads_contacted": 3, 
    "revenue_won_one_time": 0, 
    "revenue_lost_annual": 0, 
    "revenue_lost_one_time_created": 0, 
    "revenue_lost_one_time": 0, 
    "revenue_created_monthly": 0, 
    "revenue_won_annual_created": 0, 
    "opportunities_won": 0, 
    "opportunities_created_created": 0, 
    "emails_sent": 0, 
    "revenue_created_monthly_created": 0, 
    "emails_received": 0, 
    "calls_duration_total": 0, 
    "sms_received": 0, 
    "calls_duration_average": 0, 
    "revenue_won_one_time_created": 0, 
    "revenue_created_one_time": 0, 
    "revenue_won_monthly_created": 0, 
    "revenue_lost_monthly_created": 0, 
    "opportunities_won_created": 0, 
    "opportunities_lost_created": 0, 
    "revenue_lost_monthly": 0, 
    "_queries": {
        "leads_contacted": "call(duration > 0  date >= 2018-01-01 date <= 2018-01-01) or email(direction:sent  date >= 2018-01-01 date <= 2018-01-01) or sms(direction:sent  date >= 2018-01-01 date <= 2018-01-01)", 
        "emails_received": "email(direction:received  date >= 2018-01-01 date <= 2018-01-01)", 
        "calls": "call( date >= 2018-01-01 date <= 2018-01-01)", 
        "opportunities_created": "opportunity( created >= 2018-01-01 created <= 2018-01-01)", 
        "opportunities_won_created": "opportunity(status_type:won  closed >= 2018-01-01 closed <= 2018-01-01)", 
        "opportunities_lost_created": "opportunity(status_type:lost  lost >= 2018-01-01 lost <= 2018-01-01)", 
        "sms_sent": "sms(direction:sent  date >= 2018-01-01 date <= 2018-01-01)", 
        "leads_created": " created >= 2018-01-01 created <= 2018-01-01", 
        "opportunities_won": "opportunity(status_type:won  closed >= 2018-01-01 closed <= 2018-01-01)", 
        "opportunities_created_created": "opportunity( created >= 2018-01-01 created <= 2018-01-01)", 
        "emails_sent": "email(direction:sent  date >= 2018-01-01 date <= 2018-01-01)", 
        "sms_received": "smsdate >= 2018-01-01 date <= 2018-01-01)", 
        "opportunities_lost": "opportunity(status_type:lost" lost >= 2018-01-01 lost <= 2018-01-01)"
    }, 
    "revenue_created_one_time_created": 0, 
    "calls": 0, 
    "revenue_created_annual": 0
}

since the bounding dates are 2018-01-01 and 2018-01-01, it means that we aren't correctly factoring in timezone when passing through date params.

We need to add the tz.offset*-1 to the datetime in both directions for it to appear the same as it does in app.

tsx commented 6 years ago

@eengoron have you set tz_offset when constructing API client?

eengoron commented 6 years ago

Yup! The offset is correct.

tsx commented 6 years ago

Looks like frontend just always sends UTC timestamps and backend assumes UTC without taking tz offset header into account.

philfreo commented 6 years ago

Many endpoints do not support X-TZ-Offset (internally tracked as https://github.com/closeio/closeio/issues/1780), as in, they expect you to submit a full timestamp including offset. The header was initially designed for places like Search Queries where it was really necessary (like search queries that use today).

Supporting the header on endpoints like this isn't really a bug, just something we could add as a convenience, but not necessary. Something I had previously written internally about this issue:

To be clear, even if we added support for it, the header would only be used if date_start/date_end is in a format that doesn't specify the timezone. 2014-01-29T00:00:00.000Z or 2014-01-29T00:00:00+00 should not be affected by X-TZ-Offset because Z or +00 specifically says "this datetime is IN UTC on purpose".

Adding support for this would only a convenience for API developers who want to specify simple dates (e.g. 2014-01-29) without computing offsets themselves.

philfreo commented 6 years ago

Closing since this isn't an issue for this repo, regardless.