search

Shopify / active_shipping

ActiveShipping is a simple shipping abstraction library extracted from Shopify
http://shopify.github.io/active_shipping
MIT License
812 stars 546 forks source link

Adds notion of whether parsed timestamp for ShipmentEvent has a timezone #468

Closed qq99 closed 7 years ago

qq99 commented 7 years ago

Information around timezones can be lost when creating tracking events. We've attempted to normalize the shipment event time in tracking events to a "zoneless" fashion, but from an API consumer perspective, it can be handy to know if the UTC time value on the event is really UTC, or UTC only by virtue of the carrier not having supplied a timezone with their timestamp.

Examples: USPS timestamps look a bit like:

      <EventTime>9:01 am</EventTime>
      <EventDate>April 28, 2015</EventDate>

with no other info as to convey the timezone. It's probably based on the city/province/country in question, but perhaps it could be USPS server time. In any case, it's ambiguous and timezone_ambiguous? would&should be true.

FedEx timestamps (most? all?) look like this:

<Timestamp>2014-11-18T18:13:00+11:00</Timestamp>

since the TZ is included in the timestamp, timezone_ambiguous? would&should be false.

This PR adds a method to be more sure whether an instance of ShipmentEvent is really zoneless or not

kmcphillips commented 7 years ago

@qq99 We can backport this to 1.x when it's merged.

qq99 commented 7 years ago

@jonathankwok any thoughts on naming method? uses_timezone and has_timezone bug me a bit, since technically it's a Time instance so it always has some notion of a timezone on it. Maybe timezone_unambiguous? or has_known_timezone??

qq99 commented 7 years ago

reping for reviews

qq99 commented 7 years ago

Alright, all should be well now

qq99 commented 7 years ago

Spoke with and handed this off to @jonathankwok since I need to prioritize some other work.

I think we all agree we should properly note timezone ambiguity for each carrier doing tracking events before pushing this. IMO this should be considered low priority since any new occurrences of the underlying issue are related to DST changeover time (old tracking numbers probably long irrelevant).

We should get it fixed up a bit before the next major DST changeover date, which sounds like it might be around Mar 12, 2017 for most of continental US

qq99 commented 7 years ago

I think we can close this, @nicolaslupien found an elegant solution to handle these times differently, for any future readers see https://github.com/rails/rails/blob/e0397b0c25e790eba0c914d9ff97360219f49e29/activesupport/lib/active_support/time_with_zone.rb#L472-L478