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

Ensure that the cut-off time is in the same timezone as the extended object #1

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

giddie
Copy link

@giddie giddie commented Aug 18, 2014

If we don't match the UTC offset, and the system timezone is not UTC, you end up
with the following scenario:

Scenario 1 (correct)
Time.now               # 2014-08-15 15:30:00 +0100
Time.now.cut_off_time  # 2014-08-15 16:00:00 +0100
Time.now.despatch_day  # Fri, 15 Aug 2014
Scenario 2 (incorrect)
Time.zone.now               # Fri, 15 Aug 2014 15:30:00 BST +01:00
Time.zone.now.cut_off_time  # Fri, 15 Aug 2014 16:00:00 BST +01:00
Time.zone.now.despatch_day  # Mon, 18 Aug 2014

The incorrect result in Scenario 2 is due to the fact that the
ActiveSupport::TimeWithZone class wraps a UTC time object and maintains the
timezone data separately. When DateExtension.despatch_day is called in Scenario
2, the context is:

self          # 2014-08-15 15:30:00 UTC
cut_off_time  # 2014-08-15 16:00:00 +0100

This comparison is incorrect due to the mismatch in timezones and produces the
wrong result. The fix is to construct the cut_off_time based on the timezone
of #self.

…object

If we don't match the UTC offset, and the system timezone is not UTC, you end up
with the following scenario:

= Scenario 1 (correct)
Time.now               # 2014-08-15 15:30:00 +0100
Time.now.cut_off_time  # 2014-08-15 16:00:00 +0100
Time.now.despatch_day  # Fri, 15 Aug 2014

= Scenario 2 (incorrect)
Time.zone.now               # Fri, 15 Aug 2014 15:30:00 BST +01:00
Time.zone.now.cut_off_time  # Fri, 15 Aug 2014 16:00:00 BST +01:00
Time.zone.now.despatch_day  # Mon, 18 Aug 2014

The incorrect result in Scenario 2 is due to the fact that the
ActiveSupport::TimeWithZone class wraps a UTC time object and maintains the
timezone data separately. When DateExtension.despatch_day is called in Scenario
2, the context is:

self          # 2014-08-15 15:30:00 UTC
cut_off_time  # 2014-08-15 16:00:00 +0100

This comparison is incorrect due to the mismatch in timezones and produces the
wrong result. The fix is to construct the cut_off_time based on the timezone
of #self.
@giddie
Copy link
Author

giddie commented Aug 18, 2014

Sorry, I spent ages trying to construct a spec to test this, but ran into a brick wall with the way the built-in Time class determines the local time zone.

@robwilliams
Copy link
Owner

cheers @giddie, I'll see if I can add a test round it.

@RichGuk
Copy link

RichGuk commented Oct 13, 2014

Written that test yet? :)

RichGuk and others added 3 commits December 30, 2014 11:26
Switched to fetching the public holidays from the Government API. This ensures
that the dates are always up to date and correct (with a server reboot anyhow).
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

Successfully merging this pull request may close these issues.

3 participants