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 timezone convertion in case a given timezone has a RRULE in the future and the start date in the past of the datetime in question #421

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DeepDiver1975
Copy link

Please correct me if I'm wrong!

It looks like that the timezone conversion logic is not properly working for the given test timezone Europe/Berlin2

The standard timezone does not apply but the daylight one does for the given datetime 2019-10-25T15:30:00Z

I'll try to come up with a fix. Please meanwhile let me know if the test assumption is correct. THX

BEGIN:VCALENDAR
PRODID:-//tzurl.org//NONSGML Olson 2012h//EN
VERSION:2.0
BEGIN:VTIMEZONE
TZID:Europe/Berlin2
BEGIN:STANDARD
DTSTART:20191027T030000
TZOFFSETFROM:+0200
TZOFFSETTO:+0100
TZNAME:CET
END:STANDARD
BEGIN:DAYLIGHT
DTSTART:20190331T020000
TZOFFSETFROM:+0100
TZOFFSETTO:+0200
RDATE:20200329T020000
TZNAME:CEST
END:DAYLIGHT
END:VTIMEZONE
END:VCALENDAR

@DeepDiver1975
Copy link
Author

Looks like only the RDATE of daylight is taken into account when computing the changes array:

Screenshot from 2020-02-26 11-31-52

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 97.088% when pulling 3e3b3c1 on DeepDiver1975:test/convert-timezone into 78122e6 on mozilla-comm:master.

@DeepDiver1975 DeepDiver1975 changed the title Test convertion between two timezones Test conversion between two timezones Feb 26, 2020
@DeepDiver1975 DeepDiver1975 changed the title Test conversion between two timezones Fix timezone convertion in case a given timezone has a RRULE in the future and the start date in the past of the datetime in question Mar 2, 2020
@DeepDiver1975
Copy link
Author

Who can review this please? @kewisch maybe? THX a lot!

@DeepDiver1975
Copy link
Author

Who can review this please? @kewisch maybe? THX a lot!

👋 @kewisch

@DeepDiver1975
Copy link
Author

💤

@DeepDiver1975
Copy link
Author

👋 anybody out there?

@kewisch kewisch self-requested a review June 30, 2020 10:27
@kewisch
Copy link
Owner

kewisch commented Jun 30, 2020

Thanks for the patch, I'll see what I can do to get this in soon. I need to somehow fix my overflowing github notification emails folder.

@kewisch
Copy link
Owner

kewisch commented Jul 13, 2021

Since you may have seen some other activity here recently, I'm acknowledging that I've seen this and still have not fixed my notifications. This change is slightly to large for me to make a call right now, though I really want to get some of these PRs merged. Hoping to try again when I have a larger chunk of time.

@titanism titanism mentioned this pull request Feb 16, 2024
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

Successfully merging this pull request may close these issues.

3 participants