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

weekly rule DST issue #551

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Updated supported versions of Ruby and Rails and fixed standardrb lint issues. ([#552](https://github.com/ice-cube-ruby/ice_cube/pull/552)) by [@pacso](https://github.com/pacso)

### Fixed
- Fix for weekly occurrences when the next occurrence happens within a DST switch timespan ([#551](https://github.com/ice-cube-ruby/ice_cube/pull/551)) by [@larskuhnt](https://github.com/larskuhnt)
- Fix for weekly interval results when requesting `occurrences_between` on a narrow range ([#487](https://github.com/seejohnrun/ice_cube/pull/487)) by [@jakebrady5](https://github.com/jakebrady5)
- When using a rule with hour_of_day validations, and asking for occurrences on the day that DST skips forward, valid occurrences would be missed. ([#464](https://github.com/seejohnrun/ice_cube/pull/464)) by [@jakebrady5](https://github.com/jakebrady5)
- Include `exrules` when exporting a schedule to YAML, JSON or a Hash. ([#519](https://github.com/ice-cube-ruby/ice_cube/pull/519)) by [@pacso](https://github.com/pacso)
Expand Down
12 changes: 11 additions & 1 deletion lib/ice_cube/rules/weekly_rule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,17 @@ def realign(step_time, start_time)
time = TimeUtil::TimeWrapper.new(start_time)
offset = wday_offset(step_time, start_time)
time.add(:day, offset)
super(step_time, time.to_time)
realigned_time = time.to_time
# when the realigned time is in a different hour, it means that
# time falls to the DST switch timespan. In this case, we need to
# move the time back by one day to ensure that the hour stays the same
# WARNING: this could not work if the DST change is on a monday
# as the realigned time would be moved to the previous week.
if realigned_time.hour != start_time.hour
time.add(:day, -1)
realigned_time = time.to_time
end
Comment on lines +38 to +47
Copy link
Collaborator

@pacso pacso May 13, 2024

Choose a reason for hiding this comment

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

We need to add some tests to cover the edge case you've described.

Also, what happens if the threshold for DST isn't -1 days back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case can only happen when the realigned time changed the hour in the internals of Time because of a DST change, so -1 day will always be a time which does not fall into the DST change time imho.

I also see that this is not the ideal solution, it would be better to somehow "fix" the hour to the start time hour of the schedule somewhere in ValidatedRule#next_time but I did not find a way to access the start time of the schedule in ValidatedRule. Maybe you have an idea to that approach.

Regarding the edge case: that was just a guess based on the documentation for the message. I don't really understand the described edge case and thought that the movement to the previous day could somehow trigger that case again.

super(step_time, realigned_time)
end

# Calculate how many days to the first wday validation in the correct
Expand Down
16 changes: 16 additions & 0 deletions spec/examples/fixed_value_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
require "spec_helper"

RSpec.describe IceCube::Validations::HourOfDay::Validation do
describe :validate do
let(:timezone) { "Africa/Cairo" }
let(:time) { "2024-05-03 00:20:00" }
let(:time_in_zone) { ActiveSupport::TimeZone[timezone].parse(time) }
let(:start_time) { ActiveSupport::TimeZone[timezone].parse("2024-04-26 01:20:00") }

let(:validation) { IceCube::Validations::HourOfDay::Validation.new(nil) }

it "returns the correct offset for the same hour" do
expect(validation.validate(time_in_zone, start_time)).to eq 1
end
end
end
45 changes: 45 additions & 0 deletions spec/examples/schedule_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,51 @@
expect(next_hours).to eq [Time.utc(2014, 1, 2, 0o0, 34, 56),
Time.utc(2014, 1, 2, 0o1, 34, 56)]
end

context "Cairo timezone" do
require "active_support/time"

let(:schedule) do
IceCube::Schedule.new(ActiveSupport::TimeZone["Africa/Cairo"].parse("2022-05-05 00:20:00")).tap do |schedule|
schedule.add_recurrence_rule IceCube::Rule.weekly.day(:friday)
end
end

it "has the correct start time" do
expect(schedule.start_time.iso8601).to eq("2022-05-05T00:20:00+02:00")
end

it "has the correct start time timezone" do
expect(schedule.start_time.zone).to eq("EET")
end

it "calculates the correct occurrences from 2024-04-24" do
occurrences = schedule.next_occurrences(3, Time.utc(2024, 4, 24, 12, 0, 0))
expect(occurrences.map(&:iso8601)).to eq([
"2024-04-26T01:20:00+03:00",
"2024-05-03T00:20:00+03:00",
"2024-05-10T00:20:00+03:00"
])
end

it "calculates the correct occurrences from 2024-04-17" do
occurrences = schedule.next_occurrences(3, Time.utc(2024, 4, 17, 12, 0, 0))
expect(occurrences.map(&:iso8601)).to eq([
"2024-04-19T00:20:00+02:00",
"2024-04-26T01:20:00+03:00",
"2024-05-03T00:20:00+03:00"
])
end

it "preserves the timezone for the next DST switch" do
occurrences = schedule.next_occurrences(28, Time.utc(2024, 4, 24, 12, 0, 0))
expect(occurrences.map(&:iso8601).last(3)).to eq([
"2024-10-18T00:20:00+03:00",
"2024-10-25T00:20:00+03:00",
"2024-11-01T00:20:00+02:00"
])
end
end
end

describe :next_occurrence do
Expand Down
16 changes: 16 additions & 0 deletions spec/examples/time_util_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -118,5 +118,21 @@ module IceCube
end
end
end

describe :dst_change do
let(:timezone) { "Africa/Cairo" }
let(:time) { "2024-04-26 00:20:00" }
let(:time_in_zone) { ActiveSupport::TimeZone[timezone].parse(time) }

subject { TimeUtil.dst_change(time_in_zone) }

it { is_expected.to eql(1) }

context "when time is not on a DST change" do
let(:time) { "2024-04-25 00:20:00" }

it { is_expected.to be_nil }
end
end
end
end
64 changes: 64 additions & 0 deletions spec/examples/weekly_rule_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -390,5 +390,69 @@ module IceCube
end
end
end

context "when start time is within a timezone shift in Africa/Cairo timezone", system_time_zone: "UTC" do
let(:cairo_tz) { ActiveSupport::TimeZone["Africa/Cairo"] }
let(:utc_tz) { ActiveSupport::TimeZone["UTC"] }
let(:start_time) { cairo_tz.parse("2022-05-22 00:20:00") }
let(:rule) { Rule.weekly(1, :monday).day(:friday) }

it "parses the start time correctly" do
expect(start_time.iso8601).to eq("2022-05-22T00:20:00+02:00")
end

it "calculates the correct time from 2024-04-24 12:00:00 UTC" do
expect(rule.next_time(utc_tz.parse("2024-04-24 12:00:00"), start_time, nil).iso8601).to eq("2024-04-26T01:20:00+03:00")
end

it "calculates the correct time from 2024-04-26 00:20:01 Africa/Cairo" do
expect(rule.next_time(cairo_tz.parse("2024-04-26 00:20:01"), start_time, nil).iso8601).to eq("2024-05-03T00:20:00+03:00")
end
end

describe :realign do
require "active_support/time"

let(:timezone_name) { "Africa/Cairo" }
let(:timezone) { ActiveSupport::TimeZone[timezone_name] }
let(:utc_tz) { ActiveSupport::TimeZone["UTC"] }
let(:start_time) { timezone.parse("2022-05-22 00:20:00") }
let(:time) { utc_tz.parse("2024-04-24 12:00:00") }
let(:recurrence_day) { :friday }
let(:rule) { Rule.weekly(1, :monday).day(recurrence_day) }

subject { rule.realign(time, start_time) }

it "realigns the start time to the correct time" do
expect(subject.iso8601).to eq("2024-04-25T00:20:00+02:00")
end

context "Berlin timezone CET -> CEST " do
let(:recurrence_day) { :sunday }
let(:timezone_name) { "Europe/Berlin" }
let(:start_time) { timezone.parse("2024-03-24 02:30:00") }
let(:time) { timezone.parse("2024-03-27 02:30:00") }

# the next occurrence is on sunday within the DST shift
# where the clock is set forward from 02:00 to 03:00
# so the next occurrence is on actually on 03:30 but this
# would result in faulty start times for the following
# occurrences (03:30 instead of 02:30)
it "realigns the start time to the correct time" do
expect(subject.iso8601).to eq("2024-03-30T02:30:00+01:00")
end
end

context "Berlin timezone CEST -> CET " do
let(:recurrence_day) { :sunday }
let(:timezone_name) { "Europe/Berlin" }
let(:start_time) { timezone.parse("2023-10-22 02:30:00") }
let(:time) { timezone.parse("2023-10-24 02:30:00") }

it "realigns the start time to the correct time" do
expect(subject.iso8601).to eq("2023-10-29T02:30:00+02:00")
end
end
end
end
end
Loading