Fix caldav TZ interpretation of all day events #48642
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
probot-home-assistant
bot
added
bugfix
integration: caldav
small-pr
MartinHjelmare
changed the title
TastyPi
approved these changes
Apr 19, 2021
ralmn
approved these changes
May 7, 2021
mshish
added a commit
to mshish/O365-HomeAssistant
that referenced
this pull request
May 22, 2021
Attempts to fix PTST#85 Two changes: 1. Bring in fix from home-assistant/core#48642 which doesn't make much of a difference here (branch doesn't appear to get hit with all day events). 1. Fix the timezone shift for all day events. I'm not sure this is in the right place since it appears o365 is returning the event as if it were on GMT (but it's not). This may only work if the all day event was actually in the local timezone.
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
Is there something still missing from this PR? The bot wants to remove it because there's been no activity for a while. |
|
For the tests we can use a pytest fixture that has core/tests/components/time_date/test_sensor.py Lines 9 to 16 in a59460a |
|
@franc6 can you add a test to make sure it works now. |
|
Probably, it will depend on how much time I have. My own preference would be for the existing tests to run in multiple time zones every time. Having written the code, I feel confident it works in all time zones, but if I hadn’t written it, I’d want to see it tested in at least UTC, UTC + <some value>, and UTC - <some value> as a minimum. To be sure there’s no “magic time zone” that lets it work — as is the case without the patch, the code works well, if it’s running in UTC. :)
… On Jun 7, 2021, at 1:36 PM, Paulus Schoutsen ***@***.***> wrote:
@franc6 can you add a test to make sure it works now.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
A few notes to myself:
|
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.
|
OK, unit tests have been updated. Please see the comment for the commit for the unit tests to explain the reasons for the changes. I strongly recommend running the updated unit tests WITHOUT the fix, so the failures can be observed, then apply the fix, and observe the tests pass. :) |
balloob
approved these changes
Jun 29, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed change
In the caldav component, all-day events are treated as if they are from 00:00 to 00:00 in UTC, instead of in the local timezone. This code fixes the logic which interprets date-only events as being that date in UTC to being that date in local time. This is similar to how datetimes without a timezone are treated.
Type of change
Additional information
Checklist
black --fast homeassistant tests)Note: There's already a test for this change, but it was passing because the tests set the timezone to UTC and there's no good way to change it on a per-test basis (changing the global DEFAULT_TIME_ZONE is a bad idea since the tests aren't synchronous, and the patch decorator doesn't work because await is used for the component setup which references DEFAULT_TIME_ZONE and causes failure). Several tests fail (before and after this PR) if the tests are forced to run in another timezone (by modifying homeassistant/util/dt.py). However, the all-day tests will pass when the timezone is not UTC (after this PR).
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: