Fix todoist end time for tasks with due date in the future #91874
Conversation
home-assistant
bot
added
bugfix
cla-signed
has-tests
integration: todoist
small-pr
| await coordinator.async_config_entry_first_refresh() | ||
| await coordinator.async_refresh() |
There was a problem hiding this comment.
There was a problem hiding this comment.
It is better to move setting up coordinators into the integration setup.
There was a problem hiding this comment.
After looking at this again, I'm not exactly sure where to put this. I thought you meant within an async def async_setup function in __init__.py. But it's not clear how to get the config for a platform in that function or if platforms should have a function like that. If you could point me to an example I'd appreciate it, thanks!
There was a problem hiding this comment.
Yeah, I think the issue is that this integration should move to config entries for configuration via the UI rather than yaml. So, in addition to "ConfigEntryNotReady" not being the right error in the platform, its also not the right error if not using config entries.
There was a problem hiding this comment.
It would be nice to have this setup to support config entries in the future for sure. Perhaps after all of the bugs are fixed and it is fully working again I can take a look into that.
There was a problem hiding this comment.
Yeah, I am not aware of straight forward way to address this (e.g. i don't know how you'd get platform specific yaml config in the integration setup). Given this fixes the immediate problem of the wrong exception being thrown, i think this is a good fix. Config entries is the right path forward in future PRs, so that sounds good.
| @@ -475,16 +475,16 @@ def create_todoist_task(self, data: Task): | |||
| end = dt.parse_datetime( | |||
| data.due.datetime if data.due.datetime else data.due.date | |||
| ) | |||
There was a problem hiding this comment.
The start date is set to the user's local time here:
So we need to make sure this end date is also in the user's local time so that we aren't comparing local time against utc.
There was a problem hiding this comment.
FWIW i removed this restriction in a follow up PR as there are valid use cases for these differences in calendar applications. That said, i still think this is worth changing to local time.
| state = hass.states.get("calendar.all_projects") | ||
| assert state.state == "off" |
There was a problem hiding this comment.
This assertion was actually incorrect and was only passing due to the invalid logic fixed in this PR.
MartinHjelmare
changed the title
frenck
added
the
smash
boralyl
force-pushed
the
bugfix/task-endtime
branch
from
e6e2c34
to
38ccf64
Compare
There was a problem hiding this comment.
There turns out to be something flaky here:
https://github.com/home-assistant/core/actions/runs/5116454796/jobs/9198676595#step:10:488
FAILED tests/components/todoist/test_calendar.py::test_update_entity_for_calendar_with_due_date_in_the_future[due0] - AssertionError: assert '2023-06-02 00:00:00' == '2023-06-01 00:00:00'
- 2023-06-01 00:00:00
? ^
- 2023-06-02 00:00:00
?
|
Actually, not flaky. Deterministic? I'll have a look |
|
I've sent #93775 which may fix |
Breaking change
Proposed change
While looking into #91780 I noticed the following error when a calendar had a due date of tomorrow:
This is due to having a start date in local time and comparing it to and end date in UTC. This PR addresses this but ensuring the end date is also in local time.
Type of change
Additional information
Checklist
black --fast homeassistant tests)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.To help with the load of incoming pull requests: