caldav all day events are UTC only #25814
Comments
|
Just some info why this probably happens: According to RFC 5545, All Day events do not have a time. They just have a date. Converting an all day event from the PST timezone to the GMT timezone should have the start/end date now be set to midnight at GMT. So I think the correct fix here is to remove datetime stamps from the attributes and make sure they are dates. |
|
I think this extends to the google_calendar component as well. I have all day events that start at midnight (as expected) and end at 8 PM in UTC-4 (my time zone is Eastern Time). |
|
There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. |
|
The problem still exists with 0.103 |
|
There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. |
|
This problem still exists in 0.106.6. |
|
There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. |
|
Issue still occurs in HA 0.111.2
… On Jun 14, 2020, at 9:49 PM, stale[bot] ***@***.***> wrote:
There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates.
Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍
This issue now has been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. |
|
This is still a problem in 0.114.4. |
|
After further searching, I think this might be a bug in the caldav component used by HA. For certain, balloob is correct that the events have a datetime instead of a date object for start/end. The wrapper is very thin, and the code in caldav < 0.6.2 unconditionally converts any date without a timezone into a datetime. Since the all-day events (at least from my calendar, but AFAIK, that's normal) don't have timezones, the dates get converted. In 0.6.2 and later, there's a check before adjusting the date and time to be sure it's a datetime. It still appears to be returning both date and time, but updating caldav might hide the issue. |
|
I'm also having troubles with the all-day events, here in 0.111.4. I think the timezone is the problem, but not sure. The python/caldav event is like this : (not using the timezone, and see that the date has come to Oct 5th, a monday...) Forcing "requirements": ["caldav==0.7.1"] doesn't help. Besides, seems also like the exceptions are not handled ? |
|
The problem still exists with 0.117 :-/ |
|
Sadly, still the same with v.2020.12.0 and v.2020.12.1 |
|
There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. |
|
Still a problem in 2021.3.4 |
|
Anyone up for taking this on? |
|
I finally made the time to step through this. It looks like the real problem is in WebDavCalendarData.to_datetime(). The logic appears to be incorrect for dates. When this is called to check if the all-day event is over (see WebDavCalendarData.is_over() called from ), it receives a date for the all day event. This date is combined with a time of 00:00, and then treated as UTC and converted to local time! That's incorrect. If it's only a date, it must be treated as being in the current timezone.Recommended fix: diff --git a/homeassistant/components/caldav/calendar.py b/homeassistant/components/caldav/calendar.py
index 62be361df3..61186249c5 100644
--- a/homeassistant/components/caldav/calendar.py
+++ b/homeassistant/components/caldav/calendar.py
@@ -310,7 +310,9 @@ class WebDavCalendarData:
# represent same time regardless of which time zone is currently being observed
return obj.replace(tzinfo=dt.DEFAULT_TIME_ZONE)
return obj
- return dt.as_local(dt.dt.datetime.combine(obj, dt.dt.time.min))
+ return dt.dt.datetime.combine(obj, dt.dt.time.min).replace(
+ tzinfo=dt.DEFAULT_TIME_ZONE
+ )
@staticmethod
def get_attr_value(obj, attribute):That change fixes the all-day events for me, and should fix them for everyone. I'll make a PR after rebasing and the tests finish. :) |
|
Yes, I've been trying the mod there. I now have recurring full day events working fine (not showing one day before). Anyway, thanks a lot ! |
|
@Mister-Slowhand If the exceptions were also for all day events and the events were daily, could it be an artifact of having the time shifted inappropriately that made it appear as if the exception dates still had the event? If so, that would explain how it's fixed. Otherwise, I don't know; the patched method is effectively only called once, and should not impact exceptions directly. |
|
I spoke too soon ! |
|
@Mister-Slowhand If the exceptions worked without this patch, then it's probably related. If they didn't work without the patch, I'd suggest filing a separate bug report. I might try to find some time to reproduce the problem either way, and see if I can figure out what's wrong. But it'll be a while, as I'm still setting up a better dev environment. |
|
I don't think the exceptions changed with your patch, they probably didn't work before (but as nothing worked, it's hard to tell !) |
|
Up to HA core-2021.6.3 franc6's patch for fixing "caldav all day events are UTC only" is working flawlessly and the local timezone gets perfectly respected after applying the patch. However: Since this patch has still not found it's way into HA's core one must re-apply above patch after every single HA core-update which is getting quite bothering after some time. Therefore it would be nice if franc6's fix will get officially implemented into the core soon. As for Mister-Slowhand's thread-hijacking post above this is a completely different problem/bug: "caldav all day events are UTC only" vs. "exceptions for recurring events" and therefore should be moved to a different bug report instead of decelerating the implementation of a working fix for a bug report which obviously is already solved by applying franc6's proposed fix. |
|
@Tamsy As far as I am aware, Mister-Slowhand's comments are not holding anything up. There are some weird issues with the current set of unit tests, and I can certainly understand a very strong desire to get fully corrected unit tests. At the moment, this exact case is covered by the existing unit tests, and they succeed, because the UTC timezone is forced. I've been given a couple hints (earlier this week) on how to possibly address the unit tests, but I'm really quite busy right now. You're welcome to take a look at PR #48642 if you'd like to help, just please note my comments about the timezone being kept in a global, the tests running asynchronously, and my own desire to ensure the tests run in multiple time zones, not just the local time zone. It's a lot more work than the simple patch above, but getting these done will provide an form for updating unit tests for all calendars, which can improve quality for more than just those using the caldav platform. I know exactly how annoying it is to apply the patch manually with every update, as I do it, too. :) It would have been nice to get some additional feedback on the unit tests sooner, but I know HA is a really big project with only a handful of developers. |
|
@franc6 Thank you for the quick reply and enlightement on the matter. To me it looked like your patch isn't finding its way into the core because that 2nd. bug (report) should be fixed before this can be done. I will look at PR #48642 and see if I can help there. Thank you again for the enlightement. |
Please notes that unit tests were inadequate in the past, or 28514 would have been caught. The new tests set one of three time zones explicitly for all day event tests, and the all day event tests have been modified to patch the return value of homeassistant.util.dt.now to be in the local time zone, instead of always UTC. By having it return times in the local time zone, the tests better match real-world usage -- homeassistant.util.dt.now wouldn't return a UTC time when a different time zone is set. The unit tests now check for a single occurrence all day event at the beginning of the local day, in the middle of the local day, and at the end of the local day. Each check is run in three separate time zones, Asia/Baghdad, UTC, and America/New_York. The times are chose such that the early in the local day time for Asia/Baghdad is actually the day before in UTC, and the late in the local day time for America/New_York is actually the next day in UTC. This guarantees that when the tests pass, there are no internal checks that shifted what should be a local time as if it were UTC being shifted to local time, which was bug home-assistant#25814. Similar changes were made for the recurring all day event test, too. Note that if you run the new unit tests without the fixed code in this patch, that the following tests will fail: test_all_day_event_returned_early[pyloop-baghdad] test_all_day_event_returned_late[pyloop-new_york] test_event_rrule_all_day_late[pyloop-new_york] After applying the fix, all tests will succeed. If you're wondering why test_event_rrule_all_day_early[pyloop-baghdad] doesn't fail without the fix, it's because we're not checking on the first occurrence of the event. If we did that, the test would fail just like test_all_day_event_returned_early[pyloop-baghdad]. However, since by checking the first occurrence we're running the same condition that test_all_day_event_returned_early[pyloop-baghdad] already checks, it would be redundant to add a test which fails for a recurring event in that time zone. The whole point of the test_event_rrule_all_day tests is to ensure that recurrences are returned when it's not the first occurrence.
Home Assistant release with the issue:
0.97
Last working Home Assistant release (if known):
Operating environment (Hass.io/Docker/Windows/etc.):
hass.io
Component/platform:
caldav https://www.home-assistant.io/components/caldav
Description of problem:
All day events appear to be only in UTC when viewing the entity attributes. I have a caldav calendar that contains only all day events, one per day. The calendar entity attributes change at midnight, UTC, instead of at midnight local time. E.g. I am currently in the UTC-4 timezone, so I see the change at 8pm local time. The entity attributes show start and end times, with no time zone information. Times (with or without timezones) should not be displayed for all day events; only the date information should appear.
Problem-relevant
configuration.yamlentries and (fill out even if it seems unimportant):Traceback (if applicable):
N/A
Additional information:
I suspect the problem comes from caldav entries from the server not returning a timezone for all day events (google does this, too, for public calendars). I think they don't bother with a timezone, because it doesn't really make sense to have a timezone associated with an all day event. Since the an all_day event can be (and is) detected as such, it would be better to store not a "dateTime" when caldav.get_hass_date() is called, but explicitly a "date". Additionally, when storing a "date", .isoformat() should not be used -- that will include the time, too. Instead, use .format('YYYY-mm-dd'). That will store only the date, and the rest of the code will work properly. This also makes more sense; there should be no "time" associated with an all day event.
If requested, I can submit a pull request which fixes this -- I've done testing in a different platform for the calendar component, and this strategy solves the same problem nicely, and requires no additional changes.
The text was updated successfully, but these errors were encountered: