-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
Add read-only calendar from url #126862
Add read-only calendar from url #126862
Changes from all commits
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 | ||
---|---|---|---|---|
|
@@ -2,18 +2,44 @@ | |||
|
||||
from __future__ import annotations | ||||
|
||||
import logging | ||||
from typing import Any | ||||
|
||||
import httpx | ||||
import voluptuous as vol | ||||
|
||||
from homeassistant.config_entries import ConfigFlow, ConfigFlowResult | ||||
from homeassistant.helpers.httpx_client import get_async_client | ||||
from homeassistant.helpers.selector import ( | ||||
DurationSelector, | ||||
DurationSelectorConfig, | ||||
TextSelector, | ||||
TextSelectorConfig, | ||||
TextSelectorType, | ||||
) | ||||
from homeassistant.util import slugify | ||||
|
||||
from .const import CONF_CALENDAR_NAME, CONF_STORAGE_KEY, DOMAIN | ||||
from .const import ( | ||||
CONF_CALENDAR_NAME, | ||||
CONF_CALENDAR_URL, | ||||
CONF_STORAGE_KEY, | ||||
CONF_SYNC_INTERVAL, | ||||
DOMAIN, | ||||
) | ||||
|
||||
_LOGGER = logging.getLogger(__name__) | ||||
|
||||
STEP_USER_DATA_SCHEMA = vol.Schema( | ||||
{ | ||||
vol.Required(CONF_CALENDAR_NAME): str, | ||||
vol.Optional(CONF_CALENDAR_URL): TextSelector( | ||||
TextSelectorConfig(type=TextSelectorType.URL) | ||||
), | ||||
vol.Optional(CONF_SYNC_INTERVAL): DurationSelector( | ||||
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. This needs to be removed from the config flow. The design standards for how to do updates are here https://developers.home-assistant.io/docs/integration_fetching_data/ -- This is something generic that applies to any entity and not something we want to add to the integration specific configuration flow. The way we'll want to handle this is:
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. I will follow those instructions. Thank you. I have a lot to learn about coding for home assistant integrations. Since each remote calendar entity has its own URL, I first thought I should go for Separate polling for each individual entity, and use the However I believe the 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. Great thanks. Very happy to have your contribution, and while there is a learning curve it is quite rewarding to contribute to home assistant so really happy to help here. You have a great start already and reviewers are happy to point out best practices that may not be easy to find. Yeah doing the polling in each entity is fine. OK so the calendar entity is a bit special, and so i can explain here how it works. Consider here a scenario for Google calendar: Say every 15 minutes you refresh the cloud calendar, but then a new event shows up. It starts in 5 minutes, so you also need to handle the state transition from "off" to "on". How this works is the calendar base class will handle this for you. You'd just refresh the calendar event every 15 minutes and the base class will set an additional timer based on See
So yeah doing it per entity as you said is fine. |
||||
DurationSelectorConfig( | ||||
enable_day=True, enable_millisecond=False, allow_negative=False | ||||
) | ||||
), | ||||
} | ||||
) | ||||
|
||||
|
@@ -35,6 +61,31 @@ | |||
key = slugify(user_input[CONF_CALENDAR_NAME]) | ||||
self._async_abort_entries_match({CONF_STORAGE_KEY: key}) | ||||
user_input[CONF_STORAGE_KEY] = key | ||||
if url := user_input.get(CONF_CALENDAR_URL): | ||||
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. Increase test coverage for this file. 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. Sure, tests and documentation are a must for the merge. I was coding this from my phone, on my live home assistant instance and I did not have access to tools to run the testsuite. I will add the tests and a documentation pull request as well |
||||
try: | ||||
vol.Schema(vol.Url())(url) | ||||
except vol.Invalid: | ||||
return self.async_show_form( | ||||
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. Restructure this method to have a single show form call, not one per exception. |
||||
step_id="user", | ||||
data_schema=self.add_suggested_values_to_schema( | ||||
data_schema=STEP_USER_DATA_SCHEMA, suggested_values=user_input | ||||
), | ||||
errors={CONF_CALENDAR_URL: "invalid_url"}, | ||||
last_step=True, | ||||
) | ||||
client = get_async_client(self.hass) | ||||
res = await client.get(url) | ||||
try: | ||||
res.raise_for_status() | ||||
except httpx.HTTPError: | ||||
return self.async_show_form( | ||||
step_id="user", | ||||
data_schema=self.add_suggested_values_to_schema( | ||||
data_schema=STEP_USER_DATA_SCHEMA, suggested_values=user_input | ||||
), | ||||
errors={CONF_CALENDAR_URL: "cannot_connect"}, | ||||
last_step=True, | ||||
) | ||||
return self.async_create_entry( | ||||
title=user_input[CONF_CALENDAR_NAME], data=user_input | ||||
) |
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.
I imagined this working more like a sync where we fetch the url, writing down to the local filesystem store, but from there the local calendar integration works like normal and is able to serve from the local filesystem store. This means it continues to work offline and truly is still a local calendar. I think it could fetch and write to the store, then work like normal (but still readonly).
I think this could be a good way to think about business logic separation for the "Store" class and fetching/reading/writing. (Right now there is a "store" for a remote calendar entity and it is ambiguous what its for so something needs to be addressed here)
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.
I understand your proposal and I agree with it. It makes a lot of sense. I will implement the changes and tests and ask for a second review.
Thank you very much for your time