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

Use ActiveSupport::Duration to apply time offsets #18

Merged
merged 1 commit into from
Nov 8, 2017

Conversation

jonmast
Copy link
Contributor

@jonmast jonmast commented Nov 6, 2017

Using seconds to apply the time offsets when the parsed time is in the past is not time zone aware. This causes issues when DST begins/ends between the two times. For example, daily jobs are scheduled an hour earlier than the are supposed to when DST ends since it doesn't take the extra hour into account. This causes further issues when calculating the next offset time, the offset to prevent scheduling past times doesn't apply as expected and the job is scheduled over and over. As a related issue, we sometimes need a two hour jump to bring the parsed time to the future, add a while loop to handle this case.

Using seconds to apply the time offsets when the parsed time is in the
past is not time zone aware. This causes issues when DST begins/ends
between the two times. For example, daily jobs are scheduled an hour
earlier than the are supposed to when DST ends since it doesn't take the
extra hour into account. This causes further issues when calculating the
next offset time, the offset to prevent scheduling past times doesn't
apply as expected and the job is scheduled over and over. As a related
issue, we sometimes need a two hour jump to bring the parsed time to the
future, add a while loop to handle this case.
@jonmast jonmast requested review from jGRUBBS and kilgore5 November 6, 2017 16:59
Copy link
Contributor

@jGRUBBS jGRUBBS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good user of active_support and good tests! More readable to boot 👍

@jGRUBBS jGRUBBS merged commit 17c7d43 into master Nov 8, 2017
@jGRUBBS jGRUBBS deleted the feature/fix-dst-end-bug branch November 8, 2017 13:44
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.

2 participants