bokmann / business_time

Support for doing time math in business hours and days
MIT License
1.27k stars 215 forks source link

Nested calls to BusinessTime::Config.with break #172

Closed saverio-kantox closed 7 years ago

saverio-kantox commented 7 years ago

Example code

def show_config(prefix)
  puts "#{prefix} holidays: #{BusinessTime::Config.holidays}, work_week: #{BusinessTime::Config.work_week}"
end

show_config('Root')
BusinessTime::Config.with(holidays: [Date.today]) do
  show_config('Level 1')
  BusinessTime::Config.with(work_week: %w[mon tue]) do
    show_config('Level 2')
  end
  show_config('Level 1')
end
show_config('Root')

which prints

Root holidays: #<SortedSet:0x007fdeaa269f40>, work_week: ["mon", "tue", "wed", "thu", "fri"]
Level 1 holidays: [Thu, 27 Jul 2017], work_week: ["mon", "tue", "wed", "thu", "fri"]
Level 2 holidays: [Thu, 27 Jul 2017], work_week: ["mon", "tue"]
Level 1 holidays: #<SortedSet:0x007fdeaa269f40>, work_week: ["mon", "tue", "wed", "thu", "fri"]
Root holidays: #<SortedSet:0x007fdeaa269f40>, work_week: ["mon", "tue", "wed", "thu", "fri"]

Notice that the level-1 block has lost its local config.

saverio-kantox commented 7 years ago

With these changes, it prints

Root holidays: #<SortedSet:0x007fa05a8429a0>, work_week: ["mon", "tue", "wed", "thu", "fri"]
Level 1 holidays: [Thu, 27 Jul 2017], work_week: ["mon", "tue", "wed", "thu", "fri"]
Level 2 holidays: [Thu, 27 Jul 2017], work_week: ["mon", "tue"]
Level 1 holidays: [Thu, 27 Jul 2017], work_week: ["mon", "tue", "wed", "thu", "fri"]
Root holidays: #<SortedSet:0x007fa05a8429a0>, work_week: ["mon", "tue", "wed", "thu", "fri"]

you can see that the level-1 block still has its local config.

bokmann commented 7 years ago

Interesting. I've never had a use case for nested configs, but you gave me a nice test for it. What's your scenario where you need something like this?

saverio-kantox commented 6 years ago

Hey late answer, sorry.

The use case (which is quite simple indeed) is that we have objects that have their own calendar (possibly different for each object) and expose an api with_calendar(&block) that creates a local config. Here you can see an example (completely made up for the purpose)

module Calendarizable
  included do
    attr_reader :calendar
  end

  def with_calendar(&block)
    BusinessTime::Config.with(holidays: calendar, &block)
  end
end

class DueObject
  include Calendarizable
  attr_reader :due_date

  def reminder_date
    with_calendar { 5.business_days.before(:due_date) }
  end

  def rollover!
     with_calendar { persist(due_date: 1.business_day.after(Date.today)) }
  end
end

class ReminderService
  include Calendarizable

  def send_reminder(obj)
    with_calendar do
      if Date.today >= obj.due_date
        RolloverMailJob.perform_at(0.business_day.from_now, arg: obj)
        obj.rollover!
      elsif Date.today > obj.reminder_date
        # send reminder as soon as _our_ office is open
        ReminderMailJob.perform_at(0.business_day.from_now, arg: obj)
      end
    end
  end
end

The ReminderService object does not know that obj may use a BT local config, and assumes that both calls to 0.business_day.from_now will use the same calendar. Instead, due to the call to obj.reminder_date which uses a local config, the second job will be performed at a global calendar workday, which breaks the contract expectation.