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

Add default_time_now option to make Chronic use current time on specified date if no time is specified #362

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

Conversation

dubroe
Copy link

@dubroe dubroe commented Aug 30, 2017

My company has implemented "Time Travel" functionality into our dev/demo environments so the user can specify the specific date/time an action should occur. This is accomplished by providing a text field where the user can type in the requested date/time. This string is parsed by Chronic and then we use Timecop in the ApplicationController to travel to the time before yielding.

Often times the QA will type in a date like "September 19 2017" and then start performing a series of actions that they want to occur on that date. The problem is that this causes each action to be performed on Sept. 19th 2017 at 12:00pm. This leads to the activity logs not being ordered correctly and functionality that depends on the order of when previous actions occurred to break.

This PR resolves the issue. We've changed our code to call Chronic.parse(time_string, default_time_now: true). This makes it so the actions will take place on September 19th 2017 but at the current time, so everything will be logged as having been created/updated in the correct order.

Note that I've made it so if the user specifies a time in the time_string, that's the time that will be used as opposed to the current time.

What do you think?

@davispuh
Copy link
Collaborator

I don't think this is that useful generally for anyone else to be in Chronic. Especially if you parse date in past or in future then how current time is relevant to it. You could just get time and set it yourself on returned result.
But what would be useful for Chronic would be to return if there were only Time part, only Date part or Date and Time. There could be different method like Chronic.analyse(time_string) which could give more information about provided time string.

Anyway this is what I suggest you could do

now = Time.now
parsed = Chronic.parse(time_string, :guess => false).begin
if parsed.hour == 0 and parsed.min == 0 and parsed.sec == 0
    # assume user didn't specify time, we'll be wrong in case user specified 00:00:00
    with_time = Time.new(parsed.year, parsed.month, parsed.day, now.hour, now.min, now.sec)
else
    with_time = parsed
end

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