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

Daylight aware repeats #48

Closed
ttmx opened this issue Oct 26, 2020 · 18 comments
Closed

Daylight aware repeats #48

ttmx opened this issue Oct 26, 2020 · 18 comments

Comments

@ttmx
Copy link
Contributor

ttmx commented Oct 26, 2020

Hello!
My script just stopped working today, and I'm preeetty sure its due to daylight savings time.
This would be the ICS file I'm using https://termbin.com/pws2
And here are my scripts https://github.com/ttmx/auto-uni , running python 3.8.6 and the latest RIE module version.
The part that isn't working properly is line 47 of openlink.py
events = recurring_ical_events.of(cal).at(datetime.now(TZ))
Where it seems like either datetime.now is not returning daylight aware times, or the RIE module isn't processing them how I would expect it to,

Weirdly enough, the entire countdown.py works.
I'm sure this is something on my end that I am doing wrong, and thus, didn't mark this as a bug.
Would there be any chance you could help me fix this? Totally understand if it is outside the purpose of github issues.

Thank you!

@niccokunzmann
Copy link
Owner

If we can boil it down to a reproducible example, that could help. Can you show python code which creates the same object as datetime.now(TZ)? So we talk about the same object.
Second: which outcome do you expect? I am still not quite good at reading ics files.
Thanks for providing so much information already!

@ttmx
Copy link
Contributor Author

ttmx commented Oct 26, 2020

Hey thank you for answering!
TZ = timezone("Europe/Lisbon") I have it the same as in the example file.
the datetime.now(TZ) is just the default libs datetime

I'm expecting it to return the events that are happening right now, but it is instead returning the events that will happen in an hour.
Example, right now its returning nothing, but an hour ago it was returning the RC-p event.
This calendar is from the same ICS file.
image

Anything else I can provide? Thank you for your time!

@niccokunzmann
Copy link
Owner

Can you provide python code to create a datetime for these cases: 1 no event expected but one given, 2 one event expected but none given, 3 an event given and expected, 4 no event given and none expected.

This will be enough to create reproducible tests.

For clarity, you can answer like
case 1: datetime.datetime(...)
...

@niccokunzmann
Copy link
Owner

I would like to have tests to nail down the issue and find out where this error comes from.

@ttmx
Copy link
Contributor Author

ttmx commented Nov 2, 2020

Essentially, whenever I put that thing as the argument for recurring_ical_events.of(cal).at(datetime_here), the events returned are sometimes mismatched by one hour, due to daylight savings.

I'll always use the same "MDS-t" event, and the calendar I uploaded above as cal for simplicity.

Case 1:

fakedate = datetime(2020,11,2,12,15,0,00)
events = recurring_ical_events.of(cal).at(fakedate)

This returns no events, even though MDS-t is happening at that time. image

Case 2:

fakedate = datetime(2020,11,2,11,10,0,00)
events = recurring_ical_events.of(cal).at(fakedate)

This also returns no events, but nothing is happening at that time, so this is correct.

Case 3:

fakedate = datetime(2020,11,2,11,40,0,00)
events = recurring_ical_events.of(cal).at(fakedate)

This does return the event, and it is happening here.

Due to these tests and some others, I have concluded the RIE function used, only returns correctly, if the event has already started AND if there is more than one hour left until the end of that event.
It seems something checking against the beginning is using daylight time, but when it checks against the end its using "regular" time.

Anything I can add? Thank you!

@niccokunzmann
Copy link
Owner

Thanks. This is quite some info. I will look into it. If you like, you are also invited to create test cases. Maybe that is a learning opportunity for you.

@niccokunzmann
Copy link
Owner

niccokunzmann commented Nov 9, 2020

@ttmx I added a pull request and may go ahead with implementation. I am only rarely online these days, so if you want to do it, you are welcome! It is a well trodden path as it should be with a module that is expected to be reliable, see #49.

@ttmx
Copy link
Contributor Author

ttmx commented Nov 10, 2020

Just here to say I did not forget nor am I ignoring this. I want to eventually get around to it, but I do not have the know how or the time right now as I am in the middle of exam season.
Thank you for helping me out :)

@niccokunzmann
Copy link
Owner

niccokunzmann commented Nov 12, 2020 via email

@iprak
Copy link
Contributor

iprak commented Nov 17, 2020

I also noticed something off in this regards. I had created many recurring events before Daylight change happened and some events are not retrieved depending on the start_time passed to between().

localTZ = pytz.timezone("America/Chicago")

def dump_events(hour, minute):
    start_time = datetime.now()
    start_time = start_time.replace(
        hour=hour, minute=minute, second=0, microsecond=0)
    end_time = start_time.replace(hour=17, minute=0)

    start_time=localTZ.localize(start_time)
    end_time=localTZ.localize(end_time)

    feed_text = requests.get(calendar_feed).text
    calendar = Calendar.from_ical(feed_text)
    events = recurring_ical_events_of(calendar).between(start_time, end_time)

    print("Events between {} and {}".format(start_time, end_time))
    for event in events:
        summary = event["SUMMARY"]
        event_start = event["DTSTART"].dt
        print(event_start)

dump_events(9, 30)
dump_events(10, 0)

which generates this. Note that the 10:15 entry is lost when lookup is from 10:00

Events between 2020-11-17 09:30:00-06:00 and 2020-11-17 17:00:00-06:00
2020-11-17 12:30:00-06:00
2020-11-17 14:15:00-06:00
2020-11-17 10:15:00-06:00
2020-11-17 14:00:00-06:00
Events between 2020-11-17 10:00:00-06:00 and 2020-11-17 17:00:00-06:00
2020-11-17 12:30:00-06:00
2020-11-17 14:15:00-06:00
2020-11-17 14:00:00-06:00

Debugging a bit into the within method confirmed this. I added

print("span_start={}, span_stop={} self.start={}".format(span_start,span_stop, self.start))

And got mixed time zone offsets

span_start=2020-11-17 09:45:00-06:00, span_stop=2020-11-17 17:00:00-06:00 self.start=2020-09-28 10:15:00-05:00

span_start=2020-11-17 09:45:00-06:00, span_stop=2020-11-17 17:00:00-06:00 self.start=2020-11-03 14:15:00-06:00
        2020-11-17 14:15:00-06:00

When the event was created, the offset was 5 hours but now it is 6 hours and so they get skipped.

@niccokunzmann
Copy link
Owner

Hmm interesting.

@Cilyan
Copy link

Cilyan commented Nov 29, 2020

It looks related to these issues raised on dateutil, and in fact are due to how pytz behaves. It seems that using dateutil.rrule with icalendar is simply a no-go for DST timezones (or actually any timezone that appear to be changing in the course of time).

dateutil/dateutil#641
dateutil/dateutil#102
dateutil/dateutil#88

Another example I stumbled upon and looks unlikely to be fixable easily:

rru = dateutil.rrule.rrulestr("""DTSTART;TZID=Europe/Paris:20201010T153000
RRULE:FREQ=WEEKLY;WKST=MO;UNTIL=20201115T142959Z;BYDAY=SA,SU
"""
)
[d.astimezone(pytz.utc) for d in rru]

outputs what is expected:

[datetime.datetime(2020, 10, 10, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 10, 11, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 10, 17, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 10, 18, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 10, 24, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 10, 25, 14, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 10, 31, 14, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 11, 1, 14, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 11, 7, 14, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 11, 8, 14, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 11, 14, 14, 30, tzinfo=<UTC>)]

But now, using pytz in the same way as icalendar and recurring_ical_events:

rru = dateutil.rrule.rrulestr("""
RRULE:FREQ=WEEKLY;WKST=MO;UNTIL=20201115T142959Z;BYDAY=SA,SU
""",
    dtstart=pytz.timezone("Europe/Paris").localize(datetime.datetime(2020, 10, 10, 15, 30))
)
[d.astimezone(pytz.utc) for d in rru]

outputs

[datetime.datetime(2020, 10, 10, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 10, 11, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 10, 17, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 10, 18, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 10, 24, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 10, 25, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 10, 31, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 11, 1, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 11, 7, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 11, 8, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 11, 14, 13, 30, tzinfo=<UTC>),
 datetime.datetime(2020, 11, 15, 13, 30, tzinfo=<UTC>)]

which is wrong, both for the start times after the CEST to CET change and the last occurence is also incorrect.

@Cilyan
Copy link

Cilyan commented Nov 29, 2020

See also collective/icalendar#162

@niccokunzmann
Copy link
Owner

@iprak, you already added code. If you like to create a pull request which puts it into tests, this would be one step closer to implementing the expected behavior. Same for you @Cilyan, there is always the invitation. This template documents the steps to be taken for a test-driven implementation. I am happy to help and @ttmx already added some tests. At the same time, I live in an Eco-Community right now, Internet far away, so I am slow to respond and do not take much time on programming.

@iprak
Copy link
Contributor

iprak commented Dec 11, 2020

I submitted a PR (#51). The calendar data and the code are both close to my actual usage.

niccokunzmann added a commit that referenced this issue Dec 17, 2020
niccokunzmann added a commit that referenced this issue Dec 23, 2020
@niccokunzmann
Copy link
Owner

@ttmx @iprak @Cilyan Thanks for getting involved in this. Both PRs are merged, release v0.1.21b should be on its way. I close this issue now, feel free to comment or open new issues!

@ttmx
Copy link
Contributor Author

ttmx commented Dec 27, 2020

Does this fix the issue? I recall testing it some days ago and I still had the problem.
Thank you!

@niccokunzmann
Copy link
Owner

The tests are running. That should fix the issue, then. We can re-open it.

Also, be aware how to create a datetime object with the right time-zone: use tz.localize(dt) and not set it in the constructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants