zendesk / biz

Time calculations using business hours.
Apache License 2.0
490 stars 23 forks source link

The ability to know the next business time after X days of a certain time #14

Closed take closed 9 years ago

take commented 9 years ago

Hi thanks for the gem! Really well organized and clean, I love it.

I've been using the gem but I found out that I can't calculate the next business time after X days of a certain time. Thought that Biz.time(X, :days).after(certain_time) will do it but seems like it's returning the end of the business time after X + 1 business day.

irb(main):115:0> Biz.configure do |config|
irb(main):116:1*   config.hours = {
irb(main):117:2*     mon: {'09:00' => '17:00'},
irb(main):118:2*     tue: {'09:00' => '17:00'},
irb(main):119:2*     wed: {'09:00' => '17:00'},
irb(main):120:2*     thu: {'09:00' => '17:00'},
irb(main):121:2*     fri: {'09:00' => '17:00'}
irb(main):122:2>   }
irb(main):123:1>
irb(main):124:1*   config.holidays = []
irb(main):125:1>
irb(main):126:1*   config.time_zone = 'Etc/GMT'
irb(main):127:1> end
=> #<Biz::Schedule:0x007ffc62cc4870 @configuration=#<Biz::Configuration:0x007ffc62cc47f8 @raw=#<struct Biz::Configuration::Raw hours={:mon=>{"09:00"=>"17:00"}, :tue=>{"09:00"=>"17:00"}, :wed=>{"09:00"=>"17:00"}, :thu=>{"09:00"=>"17:00"}, :fri=>{"09:00"=>"17:00"}}, holidays=[], time_zone="Etc/GMT">>>
irb(main):128:0> time = Time.utc(2015, 3, 9, 17, 1)
=> 2015-03-09 17:01:00 UTC
irb(main):129:0> time.business_hours?
=> false
irb(main):130:0> Biz.time(1, :days).after(time)
=> 2015-03-12 17:00:00 UTC
irb(main):131:0> Biz.time(1, :days).after(time).business_hours?
=> false

In above's case, I expected Biz.time(1, :days).after(time) to return 2015-03-11 09:00:00 UTC, since 1 business day after 2015-03-09 17:01:00 UTC is the next business time after 2015-03-10 17:00:00 UTC.

I guess Biz.time(X, :days) isn't officially supported?(don't see them in the docs, and in the core extensions)

Thanks!

craiglittle commented 9 years ago

Hey, @take! Glad to hear you're finding the gem useful.

You're correct that Biz#time calculations using day-based durations aren't currently supported. Unlike duration calculations using seconds, minutes, and hours, day-based calculations move into a realm of behavior where people start to have differing opinions about what the optimal behavior should be. Black and white begins to give way to a muddy grey.

After spending good deal of time reading through the issues of similar gems, I know this functionality is important to a subset of users, but it's often plagued by inconsistencies and weird corner cases in practice.

For example, given a standard configuration:

Biz.configure do |config|
  config.hours = {
    mon: {'09:00' => '17:00'},
    tue: {'09:00' => '17:00'},
    wed: {'09:00' => '17:00'},
    thu: {'09:00' => '17:00'},
    fri: {'09:00' => '17:00'},
  }

  config.time_zone = 'Etc/UTC'
end

Consider these examples and possible return values:

# Business day, before hours
Biz.time(1, :day).after(Time.utc(2015, 3, 9, 6))
# => 2015-03-09 09:00:00 UTC?
# *or*
# => 2015-03-10 09:00:00 UTC?

# Business day, during hours
Biz.time(1, :day).after(Time.utc(2015, 3, 9, 12))
# => 2015-03-10 09:00:00 UTC?
# *or*
# => 2015-03-10 12:00:00 UTC?

# Business day, after hours
Biz.time(1, :day).after(Time.utc(2015, 3, 9, 20))
# => 2015-03-10 09:00:00 UTC?
# *or*
# => 2015-03-11 09:00:00 UTC?

# Non-business day
Biz.time(1, :day).after(Time.utc(2015, 3, 8, 12))
# => 2015-03-09 09:00:00 UTC?
# *or*
# => 2015-03-10 09:00:00 UTC?

I currently have an implementation in the works that would work like the following for the above examples:

# Business day, before hours
Biz.time(1, :day).after(Time.utc(2015, 3, 9, 6)) # => 2015-03-10 09:00:00 UTC

# Business day, during hours
Biz.time(1, :day).after(Time.utc(2015, 3, 9, 12)) # => 2015-03-10 09:00:00 UTC

# Business day, after hours
Biz.time(1, :day).after(Time.utc(2015, 3, 9, 20)) # => 2015-03-10 09:00:00 UTC

# Non-business day
Biz.time(1, :day).after(Time.utc(2015, 3, 8, 12)) # => 2015-03-09 09:00:00 UTC

The main idea is to have the calculation operate exclusively on the day level before dropping down to the hours configuration to return the first time after the number of specified business days has elapsed. I don't know if it'll satisfy everyone, but it will at least be consistent and easy to reason about.

In the meantime, I've already merged a PR to remove 'day' duration support for the time being until the real implementation is available. I'd love to hear any comments you have about the proposed implementation.

I'll keep this issue open and marked as a feature request for now.

take commented 9 years ago

@craiglittle

Thanks for the detailed informations!

Yea I realised that there are weird corner cases and I also found out some alternative gems has an API for it, and some doesn't.

For example the working_hours gem solves this by adding an API which advances to the next business time.

[9] pry(main)> time
=> 2015-03-09 17:01:00 UTC
[10] pry(main)> time + 1.working.days
=> Tue, 10 Mar 2015 17:01:00 UTC +00:00
[11] pry(main)> (time + 1.working.days).in_working_hours?
=> false
[12] pry(main)> WorkingHours.advance_to_working_time(time + 1.working.days)
=> Wed, 11 Mar 2015 09:00:00 UTC +00:00

but I think we can just include that operation inside the day calculation process, like you mentioned above.

I have an opinion about the current implementation you're working on, would be awesome if you read through it. Basically I agree with the The main idea is to have the calculation operate exclusively on the day level before dropping down to the hours configuration to return the first time after the number of specified business days has elapsed. part, but I think I have a different opinion about how the day exclusive calculation works.

So for the 4 examples you posted above,

# Business day, before hours
Biz.time(1, :day).after(Time.utc(2015, 3, 9, 6)) # => 2015-03-10 09:00:00 UTC

:+1:

# Business day, during hours
Biz.time(1, :day).after(Time.utc(2015, 3, 9, 12)) # => 2015-03-10 09:00:00 UTC

:-1:

# Business day, after hours
Biz.time(1, :day).after(Time.utc(2015, 3, 9, 20)) # => 2015-03-10 09:00:00 UTC

:-1:

# Non-business day
Biz.time(1, :day).after(Time.utc(2015, 3, 8, 12)) # => 2015-03-09 09:00:00 UTC

:-1:

I'm basically against the last 3 because I think adding 1 business day means the next business time after the 24hours including the next business time.

So I think the last 3 should be

# Business day, during hours
Biz.time(1, :day).after(Time.utc(2015, 3, 9, 12)) # => 2015-03-10 12:00:00 UTC

# Business day, after hours
Biz.time(1, :day).after(Time.utc(2015, 3, 9, 20)) # => 2015-03-11 09:00:00 UTC

# Non-business day
Biz.time(1, :day).after(Time.utc(2015, 3, 8, 12)) # => 2015-03-09 12:00:00 UTC

Of course I don't think this the next business time after the 24hours including the next business time definition will be the definition for everyone, it might have some deficit, and I think there's a high possibility I'll change my mind.

So please tell me what you think about this definition.

Thanks! :D

craiglittle commented 9 years ago

@take Thanks for taking the time to write that up. I think it makes a lot of sense, and the underlying rule is simple and consistent. Let me see what I can do!

take commented 9 years ago

@craiglittle Thanks! Please feel free to ping me anytime, I'll be happy to help you :)

craiglittle commented 9 years ago

@take I just opened a PR (https://github.com/zendesk/biz/pull/17) for this feature. Let's move the discussion over there!