-
-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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 todoist end time for tasks with due date in the future #91874
Changes from all commits
6ae99cd
ec44fd6
38ccf64
28bafa2
84ea7a6
f46a4df
70583d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -120,7 +120,7 @@ async def async_setup_platform( | |||
|
||||
api = TodoistAPIAsync(token) | ||||
coordinator = TodoistCoordinator(hass, _LOGGER, SCAN_INTERVAL, api) | ||||
await coordinator.async_config_entry_first_refresh() | ||||
await coordinator.async_refresh() | ||||
|
||||
async def _shutdown_coordinator(_: Event) -> None: | ||||
await coordinator.async_shutdown() | ||||
|
@@ -477,16 +477,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||
task[END] = dt.as_utc(end) if end is not None else end | ||||
task[END] = dt.as_local(end) if end is not None else end | ||||
if task[END] is not None: | ||||
if self._due_date_days is not None and ( | ||||
task[END] > dt.utcnow() + self._due_date_days | ||||
task[END] > dt.now() + self._due_date_days | ||||
): | ||||
# This task is out of range of our due date; | ||||
# it shouldn't be counted. | ||||
return None | ||||
|
||||
task[DUE_TODAY] = task[END].date() == dt.utcnow().date() | ||||
task[DUE_TODAY] = task[END].date() == dt.now().date() | ||||
|
||||
# Special case: Task is overdue. | ||||
if task[END] <= task[START]: | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressing this comment: https://github.com/home-assistant/core/pull/91808/files/d7976634462f4cd7ae6c301a1352c2f837b8669e#r1173537067
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to move setting up coordinators into the integration setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.