weekly rule DST issue #551
Conversation
|
Hi @larskuhnt, Thank you for your contribution ... this looks great! Just a couple of comments, but otherwise looks spot on. 👍🏻 |
|
Don't worry about fixing the linting issues ... that is resolved on #552 which I'll merge first. I'm just waiting on @seejohnrun updating my access so I can actually fix things. |
|
Hi @pacso, |
|
@larskuhnt - could you please pull |
|
@pacso I merged |
|
@larskuhnt - Could you resolve the linting errors? |
|
@pacso all checks pass now |
|
Amazing, thank you! I'll do a proper review either at lunch or after work and get this merged in 👍🏻 |
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Hi,
I added some specs for the following edge case where the calculation of the next occurrences returns faulty values.
The base of this issue is
IceCube::WeeklyRule.realignwhen an occurrence happens within a DST time switch. For example on26th April 2024 00:00->01:00. Realign will change the start_time of e.g.22th May 2022 00:20to26th April 2024 01:20and then the starting hour is wrong. The result is occurrences with the time01:20instead of00:20.Thanks
Lars