New config option - holiday hours

[edwinwills]

Unsure if this is useful for many people using this gem, however we had a recent need to set specific working hours during "holiday season" - e.g. over Christmas we'd like our working hours to be slightly different to our regular hours.

I've introduced a new config option here holiday_hours that's a hash of dates (in the string format YYYY-MM-DD, that behaves in a very similar way to working_hours, in that each key takes a hash of start times (e.g. it can still support multiple times within 1 day).

No worries if this isn't the direction you'd like this gem to take, just thought I'd share some code that was helpful to us - thanks for an excellent gem!

[jarthod]

Thanks for your contribution @edwinwills, I think this feature could be useful to others though in this state it's not really acceptable because you only changed one method which will create discrepancies (for example it won't work when going backward). In order for this to work in all cases, you'll have to check the other places where we use config[:working_hours][time.wday] and update them too (and add various specs for these cases). If you're not willing to spend more time on that that's fine, you can keep using this as an alpha implementation and if somebody else is interested maybe they'll take your branch and complete it.

[edwinwills]

Ah good point - I'll take a look at fixing up the other methods as well.

Whilst I do that, are specs passing on master on CI? Just want to see if it's my changes that have caused CI to have so many failing specs.

Cheers

[jarthod]

It looks like most of the failures would also be present on master and are caused by third party updates that happened since last build (bundler, etc..). We should try fixing those in another PR.

[edwinwills]

Great - I've added the config to cover:

• advance_to_working_time
• advance_to_closing_time
• return_to_exact_working_time
• in_working_hours?

And specs now cover:

• add_seconds
• advance_to_working_time
• advance_to_closing_time
• in_working_hours?

I couldn't find any specs for return_to_exact_working_time, so I haven't added any, but can set some up if required.

[jarthod]

About the specs on master, I just updated the travis config so they pass again so you can merge master in your branch to get green specs.

About the method updates, I still see some methods accessing config[:working_hours] which have not been updated, namely: add_seconds, working_day? and working_time_between AFAICS. Is there a good reason for that?

Thanks for you work!

[edwinwills]

add_seconds - I actually added a spec for this, but didn't need to add any change to the method, as it calls advance_to_working_time, which is already updated with the new holiday_hours config.

working_day I left as-is, as the way I expected holiday hours to work, is that you're just adjusting hours that are already present in the standard working days (so working_day? logic will be unchanged, as only the hours are changing).

working_time_between looks like it needs some extra work from me - will take a look and update, thanks.

[jarthod]

About add_seconds, yes for your spec it doesn't need changing because you only override the starting hour, and that's when we use advance_to_working_time, but then during the day we use holiday_hours to compute time, so for example if you holiday hour finish earlier, this won't be taken into account currently (or if there are two ranges and the second range start changes for example)

About working_day I agree for your case but nothing prevents anybody to put some exceptional days here that is usually not worked for example, and we don't want to have inconsistent results that will make people feel like the gem is not reliable. So IMO either we validate what you say at configuration time, or we make it work with every days.

Thanks again for your work and sorry for being picky about it but this is the kind of details that could made the gem untrustworthy and that's what made us loose weeks debugging others gems and finally writing our own so we really want to spare the trouble for our users. It's much easier if we adresse everything we can think of now than let people spend hours debugging why they don't get the correct result down the road.

[edwinwills]

Appreciate your thorough response! Will get back to you with fixes for the points you raised.

[edwinwills]

Hi @jarthod, somehow a whole year has gone past, and I completely forgot to update this PR, until I ran into a limitation with the working_day? method that you highlighted in your comment.

I've tried to fix up the remaining methods now, and added some tests to ensure that adjustments to the start and end of the day are covered.

I've also now ensured that holiday_hours work regardless of whether the day is an existing "working day" or not.

Thanks.

[edwinwills]

Appreciate all your comments, and your time to take a look through again, and totally agree with wanting to get it as simple as possible to avoid any problems in the future.

I've made the suggested changes which simplify things considerably. I've also added some notes to the Readme (but let me know if you'd like me to be a bit more verbose here).

Finally, I've added an extra spec around the add_seconds method - as far as I can tell it's behaving as expected (e.g. it takes into account the holiday_hours configuration). I now have specs covering adding seconds to the beginning, middle, and end of the day (to test that, e.g. if holiday_hours is set to let people have a longer lunch, the add_seconds takes into account a longer period of non-working time in the middle of the day.

Thanks again.

[edwinwills]

Sorry for the delay in update, a few things got in the way, but in the end, fixing up the PR wasn't that much effort. Hopefully this resolves the outstanding issues (including fixing up the add_seconds method). Thanks again for your clear feedback 🙂

[jarthod]

Thanks @edwinwills! no problem for the delay. All the fixes here looks good. Though having a last look I noticed there is still one method which wasn't updated: #working_time_between. I mentioned it before (but not in my last comments) so it looks like it got forgotten in the process.

[edwinwills]

Ah sorry about that - added the config to that method, and added a few new specs that were broken before this config was added. Thanks!

[jarthod]

Merged and released as 1.4.0 :tada: