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

Fix first weekly occurrences with non-Sunday week start #383

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions lib/ice_cube/rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ class Rule

attr_reader :uses

def reset
end

# Is this a terminating schedule?
def terminating?
until_time || occurrence_count
Expand Down Expand Up @@ -51,11 +54,6 @@ def to_hash
raise MethodNotImplemented, "Expected to be overridden by subclasses"
end

# Reset the uses on the rule to 0
def reset
@uses = 0
end

def next_time(time, schedule, closing_time)
end

Expand Down
2 changes: 1 addition & 1 deletion lib/ice_cube/rules/daily_rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class DailyRule < ValidatedRule

include Validations::DailyInterval

def initialize(interval = 1, week_start = :sunday)
def initialize(interval = 1)
super
interval(interval)
schedule_lock(:hour, :min, :sec)
Expand Down
2 changes: 1 addition & 1 deletion lib/ice_cube/rules/hourly_rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class HourlyRule < ValidatedRule

include Validations::HourlyInterval

def initialize(interval = 1, week_start = :sunday)
def initialize(interval = 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

always thought these were a little clunky so I'm happy to see them go

super
interval(interval)
schedule_lock(:min, :sec)
Expand Down
2 changes: 1 addition & 1 deletion lib/ice_cube/rules/minutely_rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class MinutelyRule < ValidatedRule

include Validations::MinutelyInterval

def initialize(interval = 1, week_start = :sunday)
def initialize(interval = 1)
super
interval(interval)
schedule_lock(:sec)
Expand Down
2 changes: 1 addition & 1 deletion lib/ice_cube/rules/monthly_rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class MonthlyRule < ValidatedRule

include Validations::MonthlyInterval

def initialize(interval = 1, week_start = :sunday)
def initialize(interval = 1)
super
interval(interval)
schedule_lock(:day, :hour, :min, :sec)
Expand Down
2 changes: 1 addition & 1 deletion lib/ice_cube/rules/secondly_rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class SecondlyRule < ValidatedRule

include Validations::SecondlyInterval

def initialize(interval = 1, week_start = :sunday)
def initialize(interval = 1)
super
interval(interval)
reset
Expand Down
28 changes: 27 additions & 1 deletion lib/ice_cube/rules/weekly_rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,39 @@ class WeeklyRule < ValidatedRule

include Validations::WeeklyInterval

attr_reader :week_start

def initialize(interval = 1, week_start = :sunday)
super
super(interval)
interval(interval, week_start)
schedule_lock(:wday, :hour, :min, :sec)
reset
end

# Calculate the effective start time for when the given start time is later
# in the week than one of the weekday validations, such that times could be
# missed by a 7-day jump using the weekly interval, or when selecting from a
# date that is misaligned from the schedule interval.
#
def realign(step_time, start_time)
time = TimeUtil::TimeWrapper.new(start_time)
offset = wday_offset(step_time, start_time)
time.add(:day, offset) if offset
time.to_time
end

def wday_offset(step_time, start_time)
wday_validations = other_interval_validations.select { |v| v.type == :wday }
return if wday_validations.none?

days = (step_time - start_time).to_i / ONE_DAY
interval = base_interval_validation.validate(step_time, start_time).to_i
min_wday = TimeUtil.normalize_wday(wday_validations.min_by(&:day).day, week_start)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe here this is the equivalent of map(&:day).min which may read nicer

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did it this way because min_by takes one pass over the array and map.min takes two.

(However, this method is only called once per query, and it's a tiny array of 7 so it's not going to be significant either way.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely - sounds good!

step_wday = TimeUtil.normalize_wday(step_time.wday, week_start)

days + interval - step_wday + min_wday
end

end

end
2 changes: 1 addition & 1 deletion lib/ice_cube/rules/yearly_rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class YearlyRule < ValidatedRule

include Validations::YearlyInterval

def initialize(interval = 1, week_start = :sunday)
def initialize(interval = 1)
super
interval(interval)
schedule_lock(:month, :day, :hour, :min, :sec)
Expand Down
28 changes: 3 additions & 25 deletions lib/ice_cube/schedule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ def enumerate_occurrences(opening_time, closing_time = nil, options = {}, &block
spans = options[:spans] == true && duration != 0
Enumerator.new do |yielder|
reset
t1 = full_required? ? start_time : realign(opening_time) - (spans ? duration : 0)
t1 = full_required? ? start_time : opening_time - (spans ? duration : 0)
loop do
break unless (t0 = next_time(t1, closing_time))
break if closing_time && t0 > closing_time
Expand All @@ -439,7 +439,7 @@ def next_time(time, closing_time)
loop do
min_time = recurrence_rules_with_implicit_start_occurrence.reduce(nil) do |min_time, rule|
begin
new_time = rule.next_time(time, self, min_time || closing_time)
new_time = rule.next_time(time, start_time, min_time || closing_time)
[min_time, new_time].compact.min
rescue StopIteration
min_time
Expand All @@ -462,7 +462,7 @@ def full_required?
# is excluded from the schedule
def exception_time?(time)
@all_exception_rules.any? do |rule|
rule.on?(time, self)
rule.on?(time, start_time)
end
end

Expand Down Expand Up @@ -502,28 +502,6 @@ def wind_back_dst
end
end

# If any rule has validations for values within the period, (overriding the
# interval from start time, e.g. `day[_of_week]`), and the opening time is
# offset from the interval multiplier such that it might miss the first
# correct occurrence (e.g. repeat is every N weeks, but selecting from end
# of week N-1, the first jump would go to end of week N and miss any
# earlier validations in the week). This realigns the opening time to
# the start of the interval's correct period (e.g. move to start of week N)
# TODO: check if this is needed for validations other than `:wday`
#
def realign(opening_time)
time = TimeUtil::TimeWrapper.new(opening_time)
recurrence_rules.each do |rule|
wday_validations = rule.other_interval_validations.select { |v| v.type == :wday } or next
interval = rule.base_interval_validation.validate(opening_time, self).to_i
offset = wday_validations
.map { |v| v.validate(opening_time, self).to_i }
.reduce(0) { |least, i| i > 0 && i <= interval && (i < least || least == 0) ? i : least }
time.add(rule.base_interval_type, 7 - time.to_time.wday) if offset > 0
end
time.to_time
end

end

end
2 changes: 1 addition & 1 deletion lib/ice_cube/single_occurrence_rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def terminating?
true
end

def next_time(t, schedule, closing_time)
def next_time(t, _, closing_time)
unless closing_time && closing_time < t
time if time.to_i >= t.to_i
end
Expand Down
21 changes: 16 additions & 5 deletions lib/ice_cube/validated_rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,17 @@ class ValidatedRule < Rule

attr_reader :validations

def initialize(interval = 1, *)
def initialize(interval = 1)
@validations = Hash.new
end

# Reset the uses on the rule to 0
def reset
@time = nil
@start_time = nil
@uses = 0
end

def base_interval_validation
@validations[:interval].first
end
Expand All @@ -49,17 +56,21 @@ def base_interval_type
end

# Compute the next time after (or including) the specified time in respect
# to the given schedule
def next_time(time, schedule, closing_time)
# to the given start time
def next_time(time, start_time, closing_time)
@time = time
@schedule = schedule
@start_time ||= realign(time, start_time)

return nil unless find_acceptable_time_before(closing_time)

@uses += 1 if @time
@time
end

def realign(opening_time, start_time)
start_time
end

def skipped_for_dst
@uses -= 1 if @uses > 0
end
Expand Down Expand Up @@ -145,7 +156,7 @@ def find_acceptable_time_before(boundary)
#
def validation_accepts_or_updates_time?(validations_for_type)
res = validations_for_type.each_with_object([]) do |validation, offsets|
r = validation.validate(@time, @schedule)
r = validation.validate(@time, @start_time)
return true if r.nil? || r == 0
offsets << r
end
Expand Down
2 changes: 1 addition & 1 deletion lib/ice_cube/validations/count.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def dst_adjust?
false
end

def validate(time, schedule)
def validate(time, start_time)
raise CountExceeded if rule.uses && rule.uses >= count
end

Expand Down
4 changes: 2 additions & 2 deletions lib/ice_cube/validations/daily_interval.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ def dst_adjust?
true
end

def validate(step_time, schedule)
t0, t1 = schedule.start_time, step_time
def validate(step_time, start_time)
t0, t1 = start_time, step_time
days = Date.new(t1.year, t1.month, t1.day) -
Date.new(t0.year, t0.month, t0.day)
offset = (days % interval).nonzero?
Expand Down
2 changes: 1 addition & 1 deletion lib/ice_cube/validations/day_of_week.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def dst_adjust?
true
end

def validate(step_time, schedule)
def validate(step_time, start_time)
wday = step_time.wday
offset = (day < wday) ? (7 - wday + day) : (day - wday)
wrapper = TimeUtil::TimeWrapper.new(step_time)
Expand Down
2 changes: 1 addition & 1 deletion lib/ice_cube/validations/day_of_year.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def dst_adjust?
true
end

def validate(step_time, schedule)
def validate(step_time, start_time)
days_in_year = TimeUtil.days_in_year(step_time)
yday = day < 0 ? day + days_in_year + 1 : day
offset = yday - step_time.yday
Expand Down
20 changes: 10 additions & 10 deletions lib/ice_cube/validations/fixed_value.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ class Validations::FixedValue

INTERVALS = {:min => 60, :sec => 60, :hour => 24, :month => 12, :wday => 7}

def validate(time, schedule)
def validate(time, start_time)
case type
when :day then validate_day_lock(time, schedule)
when :hour then validate_hour_lock(time, schedule)
else validate_interval_lock(time, schedule)
when :day then validate_day_lock(time, start_time)
when :hour then validate_hour_lock(time, start_time)
else validate_interval_lock(time, start_time)
end
end

Expand All @@ -25,17 +25,17 @@ def validate(time, schedule)
# Validate if the current time unit matches the same unit from the schedule
# start time, returning the difference to the interval
#
def validate_interval_lock(time, schedule)
t0 = starting_unit(schedule.start_time)
def validate_interval_lock(time, start_time)
t0 = starting_unit(start_time)
t1 = time.send(type)
t0 >= t1 ? t0 - t1 : INTERVALS[type] - t1 + t0
end

# Lock the hour if explicitly set by hour_of_day, but allow for the nearest
# hour during DST start to keep the correct interval.
#
def validate_hour_lock(time, schedule)
h0 = starting_unit(schedule.start_time)
def validate_hour_lock(time, start_time)
h0 = starting_unit(start_time)
h1 = time.hour
if h0 >= h1
h0 - h1
Expand All @@ -57,7 +57,7 @@ def validate_hour_lock(time, schedule)
# Positive day values are taken literally so months with fewer days will
# be skipped.
#
def validate_day_lock(time, schedule)
def validate_day_lock(time, start_time)
days_in_month = TimeUtil.days_in_month(time)
date = Date.new(time.year, time.month, time.day)

Expand All @@ -68,7 +68,7 @@ def validate_day_lock(time, schedule)
start = value
month_overflow = 0
else
start = TimeUtil.day_of_month(schedule.start_time.day, date)
start = TimeUtil.day_of_month(start_time.day, date)
month_overflow = 0
end

Expand Down
4 changes: 2 additions & 2 deletions lib/ice_cube/validations/hourly_interval.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ def dst_adjust?
false
end

def validate(step_time, schedule)
t0, t1 = schedule.start_time.to_i, step_time.to_i
def validate(step_time, start_time)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

t0, t1 = start_time.to_i, step_time.to_i
sec = (t1 - t1 % ONE_HOUR) -
(t0 - t0 % ONE_HOUR)
hours = sec / ONE_HOUR
Expand Down
20 changes: 10 additions & 10 deletions lib/ice_cube/validations/lock.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ module Validations::Lock

INTERVALS = {:min => 60, :sec => 60, :hour => 24, :month => 12, :wday => 7}

def validate(time, schedule)
def validate(time, start_time)
case type
when :day then validate_day_lock(time, schedule)
when :hour then validate_hour_lock(time, schedule)
else validate_interval_lock(time, schedule)
when :day then validate_day_lock(time, start_time)
when :hour then validate_hour_lock(time, start_time)
else validate_interval_lock(time, start_time)
end
end

Expand All @@ -25,17 +25,17 @@ def validate(time, schedule)
# Validate if the current time unit matches the same unit from the schedule
# start time, returning the difference to the interval
#
def validate_interval_lock(time, schedule)
t0 = starting_unit(schedule.start_time)
def validate_interval_lock(time, start_time)
t0 = starting_unit(start_time)
t1 = time.send(type)
t0 >= t1 ? t0 - t1 : INTERVALS[type] - t1 + t0
end

# Lock the hour if explicitly set by hour_of_day, but allow for the nearest
# hour during DST start to keep the correct interval.
#
def validate_hour_lock(time, schedule)
h0 = starting_unit(schedule.start_time)
def validate_hour_lock(time, start_time)
h0 = starting_unit(start_time)
h1 = time.hour
if h0 >= h1
h0 - h1
Expand All @@ -57,7 +57,7 @@ def validate_hour_lock(time, schedule)
# Positive day values are taken literally so months with fewer days will
# be skipped.
#
def validate_day_lock(time, schedule)
def validate_day_lock(time, start_time)
days_in_month = TimeUtil.days_in_month(time)
date = Date.new(time.year, time.month, time.day)

Expand All @@ -68,7 +68,7 @@ def validate_day_lock(time, schedule)
start = value
month_overflow = 0
else
start = TimeUtil.day_of_month(schedule.start_time.day, date)
start = TimeUtil.day_of_month(start_time.day, date)
month_overflow = 0
end

Expand Down
Loading