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

business_time_until and ActiveSupport::TimeWithZone #50

Closed
sbounmy opened this issue Oct 25, 2013 · 8 comments
Closed

business_time_until and ActiveSupport::TimeWithZone #50

sbounmy opened this issue Oct 25, 2013 · 8 comments

Comments

@sbounmy
Copy link
Contributor

sbounmy commented Oct 25, 2013

Hello,

Assuming

  • we are in a full working day (00:00:00-23:59:59)
  • Time.zone = 'Paris'

current issue is :

5.hours.ago.business_time_until 1.hours.ago
# => 7200.00005698204 // 2 hours
5.hours.ago.to_time.business_time_until 1.hours.ago.to_time
# => 14400.000063 // 4 hours

So 5 hours - 1 hours = 4 hours, the correct result is :

5.hours.ago.to_time.business_time_until 1.hours.ago.to_time

So why these 2 results are different ?

5.hours.ago.class
# => ActiveSupport::TimeWithZone
5.hours.ago.to_time
# => Time

Basically the main difference is the class used when dealing with business_time_until Time vs ActiveSupport::TimeWithZone
https://github.com/bokmann/business_time/blob/master/lib/business_time/core_ext/time.rb#L100 when using an ActiveSupport::TimeWithZone, it is converted to Time without converting using timezone properly.

I know that business_time is timezone agnostic, but using rails .ago, .from_now helpers are pretty convenient.

Any feedback is appreciated :) !

@bokmann
Copy link
Owner

bokmann commented Oct 25, 2013

TimeZones are painful. I think the bug you are showing is not due to BusinessTime, but due to mixing Time and TimeWithZone, which would happen in any rails app that happened to mix those two classes.

I'm not a fan of the business_time_until method; I tend to do these things by calculating the 'due date', as in:

due_at = 12.business_hours.from_now

and then doing a test based on a new business_time object... for instance, if something is considered 'urgent' if there are less than 4 hours left, I prefer to do:

urgent = 4.business_hours.from_now > due_at

I think that embodies the requirement better than:

urgent = Time.now.business_hours_until(due_at) < 4

and it doesn't require an extra method and the extra logic that accompanies it. Its why I originally didn't include this method.

Does this line of reasoning help with your immediate issue?

I'll decide this weekend if this is a bug for us to deal with or simply a gotcha to add to the documentation.

@sbounmy
Copy link
Contributor Author

sbounmy commented Oct 28, 2013

thanks for your feedback.
my main issue is when dealing with datetime fields which returns TimeWithZone :

ticket_resolved_in = ticket_reported.created_at.business_time_until(ticket_resolved.created_at)

@bokmann
Copy link
Owner

bokmann commented Oct 28, 2013

i am not getting the results you are getting in your example above.

You say that:

5.hours.ago.class

=> ActiveSupport::TimeWithZone

but I see:

5.hours.ago.class
=> Time

Further, the example you provide works for me:

5.hours.ago.business_time_until 1.hours.ago
=> 7444.091564
5.hours.ago.to_time.business_time_until 1.hours.ago.to_time
=> 7449.364577

What version of ActiveSupport are you using? These results are consistent for me with both activesupport 3.2.13 and 4.0.0.

@sbounmy
Copy link
Contributor Author

sbounmy commented Oct 28, 2013

Hm this is really weird, I am using rails 3.2.13.

5.hours.ago.ancestors
# => [ActiveSupport::TimeWithZone, Mongoid::Extensions::TimeWithZone, Origin::Extensions::TimeWithZone, Comparable, Object, Sinatra::Delegator, V8::Conversion::Object, PP::ObjectMixin, ActiveSupport::Dependencies::Loadable, Mongoid::Extensions::Object, Moped::BSON::Extensions::Object, Origin::Extensions::Object, JSON::Ext::Generator::GeneratorMethods::Object, Kernel, BasicObject]

Might come from some mixin...

@bokmann
Copy link
Owner

bokmann commented Oct 28, 2013

Are you copying and pasting directly from your console? In your first example, you obviously didn't, because the response you pasted was from 5.hours.ago.to_time.class, but your method called was just 5.hours.ago.to_time.

From your most recent example, I think you mean

5.hours.ago.class.ancestors

to which my console replies:

[Time, DateAndTime::Calculations, Comparable, Object, JSON::Ext::Generator::GeneratorMethods::Object, Kernel, BasicObject]

TimeZones are difficult. Something else in your ancestor chain is causing this problem.

@bokmann bokmann closed this as completed Oct 28, 2013
@sbounmy
Copy link
Contributor Author

sbounmy commented Oct 28, 2013

@bokmann yes first example is a typo, my bad.

5.hours.ago.class.ancestors
# => [ActiveSupport::TimeWithZone, Mongoid::Extensions::TimeWithZone, Origin::Extensions::TimeWithZone, Comparable, Object, Sinatra::Delegator, V8::Conversion::Object, PP::ObjectMixin, ActiveSupport::Dependencies::Loadable, Mongoid::Extensions::Object, Moped::BSON::Extensions::Object, Origin::Extensions::Object, JSON::Ext::Generator::GeneratorMethods::Object, Kernel, BasicObject]

Thanks for taking time to investigate. I will give update once I find out something in case similar ppl encounter this issue!

@sbounmy
Copy link
Contributor Author

sbounmy commented Nov 7, 2013

@bokmann what version of ruby you are using ?

with a freshly bootstrapped rails 4 project I still have

5.hours.ago.class
=> ActiveSupport::TimeWithZone
$ ruby -v
ruby 1.9.3p194 (2012-04-20 revision 35410) [x86_64-darwin12.2.0]

@barmstrong
Copy link

I'm experiencing some issues here as well. Here is an interesting failing test case:

irb(main):008:0> DateTime.now.business_days_until(1.business_days.from_now)
=> 1
irb(main):007:0> DateTime.now.business_days_until(2.business_days.from_now)
=> 3
irb(main):009:0> DateTime.now.business_days_until(3.business_days.from_now)
=> 4
irb(main):010:0> DateTime.now
=> Thu, 07 Nov 2013 17:09:19 -0800

Monday is a holiday, which the second and third examples overlap with. I'm using the https://github.com/alexdunae/holidays gem.

Does this spark any ideas? Thanks!

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

No branches or pull requests

3 participants