Incorrect offset applied to hour of day

[jakebrady5]

In this situation, schedule.occurrences_between(start_time, end_time) will return the correct occurrence, but adding spans: true will yield no results. I have opened a PR with a fix, but could use assistance is assessing whether this fix is appropriate.

Schedule:

=> #<IceCube::Schedule:0x007f8e2abc8558
 @all_exception_rules=[],
 @all_recurrence_rules=
  [#<IceCube::WeeklyRule:0x007f8e2abeb6e8
    @interval=2,
    @start_time=2018-08-11 18:00:00 UTC,
    @time=2018-08-12 10:00:00 UTC,
    @uses=0,
    @validations=
     {:interval=>
       [#<IceCube::Validations::WeeklyInterval::Validation:0x007f8e2abea1a8
         @interval=2,
         @week_start=:sunday>],
      :day=>[#<IceCube::Validations::Day::Validation:0x007f8e2ac1c680 @day=6>],
      :hour_of_day=>[#<IceCube::Validations::HourOfDay::Validation:0x007f8e2ac07dc0 @hour=10>],
      :minute_of_hour=>
       [#<IceCube::Validations::MinuteOfHour::Validation:0x007f8e2ac07708 @minute=0>],
      :second_of_minute=>
       [#<IceCube::Validations::SecondOfMinute::Validation:0x007f8e2ac070a0 @second=0>]},
    @week_start=:sunday>],
 @end_time=2018-08-07 15:00:00 UTC,
 @start_time=2018-08-07 07:00:00 UTC>

start_time = Time.utc(2018, 8, 11, 7, 0, 0)
end_time = Time.utc(2018, 8, 12, 6, 59, 59)

[16] pry(main)> schedule.occurrences_between(start_time, end_time)
=> [2018-08-11 10:00:00 UTC]
[17] pry(main)> schedule.occurrences_between(start_time, end_time, spans: true)
=> []

In debugging, I found that this line was causing time to be moved beyond the expected occurrence result. Additionally freq in this context was 2 from the biweekly rule.

The PR has a spec to cover this situation, but again I am not convinced that my approach is best.

Any assistance is appreciated!

[jakebrady5]

@seejohnrun @avit any chance I could get your initial impressions on this fix? I'm happy to chat briefly if that would be helpful. We're stuck using a fork for the time being.

[avit]

Hi @jakebrady5 thanks for investigating & reporting. Apologies for not seeing it sooner.

I think the relevant issue for that align method is #416, which was intended to minimize the possibility of creating invalid rule combinations with different frequencies/multiples.

I haven't had time to digest this yet, but is there a reason why that frequency check only applies to hour_of_day as you've done? Or could it be the same issue for other validations like minute_of_hour and second_of_minute, too? Let me know if you can take a look; I don't have any objections to the approach in the PR otherwise.

(I don't remember, so we could just check the other intervals with similar spec examples to what you've done there, just to be sure.)

[jakebrady5]

No worries! I appreciate the help as I'm pretty new to the internals of this gem.

My understanding is that hour_of_day was running into issues due to referencing base_interval_validation.

Using the PR spec as an example, base_interval_validation is #<IceCube::Validations::WeeklyInterval::Validation:0x007f8508307610 @interval=2, @week_start=:sunday>

This causes L28 to subtract 2 (weeks) from 11 (hours) without their respective units.

The minute_of_hour and second_of_minute do not seem to reference base_interval_validation at all, instead using validations[:minute_of_hour] and validations[:second_of_minute]. I don't see opportunity for the same unit mismatching.

I share your concerns though as to whether this is a partial solution. I would have to research to see whether there could be a situation where @validations[:interval] could contain both weekly and hourly interval validations. We may still need to extract the hourly interval in this case. Additionally here there is potential for interval unit mismatch (although I do not have context to know whether externalities already prevent this).

[jakebrady5]

resolved by #452