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.

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

Ability to edit/update config entries #377

Closed
jjlawren opened this issue Apr 29, 2020 · 18 comments
Closed

Ability to edit/update config entries #377

jjlawren opened this issue Apr 29, 2020 · 18 comments

Comments

@jjlawren
Copy link

Context

With the move away from YAML configuration for integrations, there's a clear need for the ability to allow the user to edit/update config entries without touching .storage/ by hand. Config entries can be updated from within the integrations today, but the exposed workflow for this process needs polish.

Besides the user-initiated decisions to edit an existing integration's config, there's also no good way for the integration to mark itself as failed (e.g., auth credentials invalidated). The integration can begin a new config flow to update the existing by reusing the same entry_id, but it will show up as a duplicated "Discovered" integration in the UI.

Summary of known problems:

  1. There's no obvious interface to start a config entry "edit".
  2. No clean way to complete an interactive update of a config entry. Neither async_abort nor async_create_entry seem applicable to this use case. ConfigFlow_abort_if_unique_id_configured can be used as a convenience, but it will tell the user the setup has been aborted which is confusing.
  3. "Update" flows for failed config entries that are initiated by the backend are marked as "Discovered".
  4. Related to point 3, if a user begins and cancels an integration-initiated "update" flow then it will disappear. This can leave the config entry in a broken state with no obvious indication.

Proposal

Some point-by-point suggestions for the problems above:

  1. Add a new standard step to enter the config flow (SOURCE_EDIT) which can be called either by the UI or from the integration during runtime.
  2. Add a new helper (i.e., ConfigFlow._update_if_unique_id_configured) which will update an existing config entry. Question: should this also unload/reload a failed entry?
  3. Add a new context parameter to the config entry which indicates that it requires attention. This could allow the entry to be shown at the top of the integration page with a "Needs Attention" label and highlighted in red. This could also change the "Configure" button to redirect to the SOURCE_EDIT step mentioned in point 1.
  4. If we mark the config entry instead of creating a new flow, then it will be:
    • Persistent in case the user aborts during reconfiguration
    • Won't show a duplicate in the Integrations page
    • Allows us to keep the delete/options/etc buttons exposed for the already-configured integration

Consequences

Editing/updating existing config entries should become more usable to the user and more obvious on how to implement to the maintainers. The benefit of easy editability of YAML with the benefits of runtime validation for config flows can both be realized when using the config entry model.

@SeanPM5
Copy link

SeanPM5 commented Apr 29, 2020

Was asked to post the mockup I shared on Discord a few days ago. Integrations in a fail state could be clearly marked as such with a red background and shown first above anything else. There'd be a button to fix it, and an overflow menu (3 dots) that would let you delete if necessary.

mockup

@cgarwood
Copy link
Member

Agreed. Something like this is definitely needed.

@Jc2k
Copy link
Member

Jc2k commented Apr 29, 2020

I think something like @SeanPM5 posted would be great (👍 for that), but thats probably seperate to a discussion about editing config entries as yaml?

@bdraco
Copy link
Member

bdraco commented May 1, 2020

I'm thinking this could be a bit more generic call to action. Hopefully the integration could set the action string and button title.

For HomeKit instead of "Credentials expired [Reauthorize]"

"Pairing Required [Pair]"

@balloob
Copy link
Member

balloob commented May 1, 2020

I think that we can solve this using the UI and existing infrastructure.

  • Allow rendering async_abort as being a successful abort.
  • Whenever a discovered config flow has the same unique ID as an existing config entry, we know it's an update flow and can call it out (color it differently).
  • Allow specifying a reason in context that will then render strings.json/config/update/<reason>/description and strings.json/config/update/<reason>/call_to_action

@bdraco
Copy link
Member

bdraco commented May 1, 2020

Since not all entries have a unique id, I think passing in the config entry id would make sense to start updating a flow. I expect every integration would have the config entry id since the failure they are trapping or seeing would be inside the setup which knows it.

@jjlawren
Copy link
Author

jjlawren commented May 2, 2020

I think that we can solve this using the UI and existing infrastructure.

  • Whenever a discovered config flow has the same unique ID as an existing config entry, we know it's an update flow and can call it out (color it differently).

What about for point 4? If the user aborts an update flow for a failed config entry without completing, the flow would be removed from the UI and the integration would remain in a broken state without an obvious indication. Having a persistent card for a config entry that needs attention would be good for usability. It would stay there until either the update flow is successfully completed or if the user deletes the existing broken config entry.

@balloob
Copy link
Member

balloob commented May 5, 2020

I think that we should require a unique ID for updating a config entry. How else will we be able to refer to it?

I've added step_id to the flows in progress endpoint (home-assistant/core#35272). This will allow us to load translations based on the step ID.

For aborting the flow, let's make sure that's not allowed :) Maybe add something to context? allow_abort = boolean

@bdraco
Copy link
Member

bdraco commented May 5, 2020

I think that we should require a unique ID for updating a config entry. How else will we be able to refer to it?

I'm probably missing the big picture on this, but is there a case where you want to update the config entry from outside of the integration where you won't know the entry.entry_id?

@balloob
Copy link
Member

balloob commented May 5, 2020

Very good point 😆

@teharris1
Copy link

Couldn't we use the OptionsFlow class for this? If OptionsFlow has an async_update_entry method it seems to me most of what is discussed in this thread becomes possible. Not all integrations make sense for reconfiguration so the reconfigure option would only be there if it is needed.

@bdraco
Copy link
Member

bdraco commented Jun 3, 2020

Maybe a new step in the config flow async_step_reconfig that re-runs async_step_user with the existing data filled in which would lead to an update instead of create.

@bdraco
Copy link
Member

bdraco commented Jun 8, 2020

It might be nice to raise ConfigEntryNeedsReconfig and start the config flow automatically.

@balloob
Copy link
Member

balloob commented Jun 8, 2020

I think that the approach that MQTT is taking is fine as a solution for now: it uses an options flow and it will update data in the config entry before finishing the options flow with a create entry: home-assistant/core#36537

@jameshilliard
Copy link

Supporting reconfiguration of devices without unique ID's would be helpful for certain types of devices such as the hlk-sw16 which is a 16 switch device that does not have hardware backed unique ID's at all. Being able to move the device to a different IP/port without reconfiguration of the 16 switch entities would be quite helpful.

I think that the approach that MQTT is taking is fine as a solution for now: it uses an options flow and it will update data in the config entry before finishing the options flow with a create entry: home-assistant/core#36537

Would this approach be useable for hardware like the hlk-sw16?

@bdraco
Copy link
Member

bdraco commented Aug 9, 2020

Here is another approach https://github.com/home-assistant/core/pull/38341/files

@jameshilliard
Copy link

Here is another approach https://github.com/home-assistant/core/pull/38341/files

I think the main issue with that approach is that it's not really possible to trigger the reauth flow manually. For devices like the hlk-sw16 that don't use authentication the reconfigure would have to be manually triggered from my understanding.

@frenck
Copy link
Member

frenck commented May 11, 2023

This architecture issue is old, stale, and possibly obsolete. Things changed a lot over the years. Additionally, we have been moving to discussions for these architectural discussions.

For that reason, I'm going to close this issue.

../Frenck

@frenck frenck closed this as not planned Won't fix, can't repro, duplicate, stale May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

9 participants