Add configuration flow to Todoist integration #100094
Conversation
home-assistant
bot
added
cla-signed
config-flow
|
Hey there @boralyl, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| _LOGGER.exception("Unexpected exception") | ||
| errors["base"] = "unknown" | ||
| else: | ||
| await self.async_set_unique_id(user_input[CONF_TOKEN]) |
There was a problem hiding this comment.
Why do we set a unique id if we only allow one config entry? And the inverse question, why only allow one config entry if we can set a unique id?
There was a problem hiding this comment.
Second question first: I first set this up with a unique id then saw the service call adds tasks without referencing a config entry or entity, and so that looked like a problem.
(The config entry however does reference a project name, so it could be possible perhaps to infer but that seemed sketchy).
First question: However, I thought it might be possible to add more config entries in the future if the service changes or is deprecated so I left the unique id. I didn't think it hurts, is there a down side?
As we talk through, I also realize this could potentially be updated in the future to support re-auth updating the API key so maybe this isn't a valid in unique id. I was thinking of it like an oauth client id but realize how it's not really like than. Given that, perhaps needs to be removed anyway. I'm not sure what, then, could be a unique id but theoretically I'd like this to be able to support multiple accounts since once the service is fixed there's no big reason it couldn't otherwise.
There was a problem hiding this comment.
Ok. Yeah, I don't think token is a good unique_id since we want to be able to re-authenticate without changing unique_id.
I'd migrate the service to an entity service. The service call targets a specific project which equals an entity, right? Then we don't need to worry about unique_id for the config entry for the sake of the service.
There was a problem hiding this comment.
Will remove the unique id...
Yeah entity service sounds right. When I did the same thing for Google calendar, it needed to be a new service with a new name etc and s deprecation period before removal of the old service? That is, it can't be immediately moved.
I'm thinking here that using a new Todo entity service may work...
Am I thinking about that right or is there a shortcut?
There was a problem hiding this comment.
Yeah, it will need to be a new service so we can keep the old one around and create a repair issue when the old service is called, telling users to use the new service instead. The entity service will require an entity target so we can't reuse the old service name if we want old service calls to still work.
|
This PR was merged without docs? It should not have been merged in that case. @allenporter Please make sure to open up a documentation PR. ../Frenck |
Thanks for catching, definitely a mistake and something I knew about then forgot to do -- the checks looked all green, but obviously I can see they aren't now. Was not intentional! |
Proposed change
Add a config flow to the Todoist integration. The motivation for doing this is to prepare for adding more platforms. (todo platform proposal)
The existing yaml configuration supports many options that are not supported in the config flow. The platform yaml configuration will remain, and those cannot be imported.
The service that creates new tasks currently does not support multiple configurations. Therefore, the new configuration flow only allows one instance.
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: