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.

Sign up for GitHub

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

Jump to bottom

Fix todoist end time for tasks with due date in the future #91874

Merged
merged 7 commits into from
May 27, 2023
Merged

Fix todoist end time for tasks with due date in the future #91874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6ae99cd
Fix end time for tasks with due date in the future.
boralyl Apr 22, 2023
ec44fd6
Merge branch 'dev' into bugfix/task-endtime
boralyl Apr 30, 2023
38ccf64
Merge branch 'dev' into bugfix/task-endtime
boralyl May 9, 2023
28bafa2
Merge branch 'dev' into bugfix/task-endtime
allenporter May 13, 2023
84ea7a6
Merge branch 'dev' into bugfix/task-endtime
boralyl May 22, 2023
f46a4df
Merge branch 'dev' into bugfix/task-endtime
boralyl May 22, 2023
70583d4
Merge branch 'dev' into bugfix/task-endtime
boralyl May 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions homeassistant/components/todoist/calendar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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!

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.


async def _shutdown_coordinator(_: Event) -> None:
await coordinator.async_shutdown()
Expand Down Expand Up @@ -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
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The start date is set to the user's local time here:

core/homeassistant/components/todoist/calendar.py

Line 454 in 6ae99cd

START: dt.now(),

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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]:
Expand Down
25 changes: 25 additions & 0 deletions tests/components/todoist/test_calendar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""Unit tests for the Todoist calendar platform."""
from datetime import timedelta
from http import HTTPStatus
from typing import Any
from unittest.mock import AsyncMock, patch
Expand Down Expand Up @@ -182,6 +183,30 @@ async def test_update_entity_for_custom_project_no_due_date_on(
assert state.state == "on"


@pytest.mark.parametrize(
"due",
[
Due(
date=(dt.now() + timedelta(days=3)).strftime("%Y-%m-%d"),
is_recurring=False,
string="3 days from today",
)
],
)
async def test_update_entity_for_calendar_with_due_date_in_the_future(
hass: HomeAssistant,
api: AsyncMock,
) -> None:
"""Test that a task with a due date in the future has on state and correct end_time."""
await async_update_entity(hass, "calendar.name")
state = hass.states.get("calendar.name")
assert state.state == "on"

# The end time should be in the user's timezone
expected_end_time = (dt.now() + timedelta(days=3)).strftime("%Y-%m-%d 00:00:00")
assert state.attributes["end_time"] == expected_end_time


@pytest.mark.parametrize("setup_integration", [None])
async def test_failed_coordinator_update(hass: HomeAssistant, api: AsyncMock) -> None:
"""Test a failed data coordinator update is handled correctly."""
Expand Down