Support readonly selectors in config_flows #129456

karwosts · 2024-10-29T19:42:23Z

Proposed change

A common complaint for a UI configured entity is that after the initial config flow, the inputs to that config flow are no longer visible, and you can't review how you configured it in the first place.

As a solution to this I propose supporting readonly/disabled selectors in options flows, which can show the values chosen during config flow, but freeze them so that they cannot be changed.

I've configured history_stats in this PR as a proof of concept of this flow.

A small frontend change is also required to support the way the disabled flag is passed to the form.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

gjohansson-ST

While I didn't receive any response on my proposal for a read only selector (which is guess should close as this is likely a better suggestion anyhow), that and this proposal share the same issue eg. They should be only for display and not sent back to core (the helper should not need to consider fields which are there only for display purpose I think).

gjohansson-ST · 2024-10-29T19:59:04Z

tests/components/history_stats/test_config_flow.py

@@ -44,6 +44,9 @@ async def test_form(
    result = await hass.config_entries.flow.async_configure(
        result["flow_id"],
        {
+            CONF_ENTITY_ID: "binary_sensor.test_monitored",


Read only fields should not be set.

Yeah I was kind of disappointed I had to do this to pass the testing, was initially hoping it would be less invasive.

If I mark the disabled fields as vol.Required, I have to pass them into this function.

If I mark the disabled fields as vol.Optional, (and don't pass them in here), they don't get passed through the config+options flow and the assert fails at the end.

Will have to think about this some more.

Ok trying a bit different approach.

Make readonly fields vol.Optional instead of vol.Required.

Change frontend to delete the readonly fields before sending data back to backend.

Change backend to not delete optional/readonly fields when they are not provided by the step flow (which they never will be provided).

With this can revert all the changes to the test.

MindFreeze · 2024-10-30T08:13:21Z

While I didn't receive any response on my proposal for a read only selector (which is guess should close as this is likely a better suggestion anyhow), that and this proposal share the same issue eg. They should be only for display and not sent back to core (the helper should not need to consider fields which are there only for display purpose I think).

Yeah, readonly sounds better. disabled is a consequence of the fact that the field is readonly and is usually something you toggle. Read only makes it clear that we don't need to process this data on save

epenet · 2024-10-30T13:16:56Z

While I didn't receive any response on my proposal for a read only selector (which is guess should close as this is likely a better suggestion anyhow), that and this proposal share the same issue eg. They should be only for display and not sent back to core (the helper should not need to consider fields which are there only for display purpose I think).

Yeah, readonly sounds better. disabled is a consequence of the fact that the field is readonly and is usually something you toggle. Read only makes it clear that we don't need to process this data on save

Small nit: I would rename readonly to displayonly as with readonly I would expect to have them to make the round trip and be sent back to core.

karwosts · 2024-10-30T14:55:15Z

Small nit: I would rename readonly to displayonly as with readonly I would expect to have them to make the round trip and be sent back to core.

I'll let a little more time to accumulate for further opinions, then I will change this if no other objections.

tetele · 2024-10-30T14:59:01Z

Small nit: I would rename readonly to displayonly as with readonly I would expect to have them to make the round trip and be sent back to core.

I've never heard the term "display only", whereas I know exactly what "read only" means. That said, this option is only in the code so maybe from a core perspective it makes more sense.

MindFreeze · 2024-10-30T14:59:42Z

While I didn't receive any response on my proposal for a read only selector (which is guess should close as this is likely a better suggestion anyhow), that and this proposal share the same issue eg. They should be only for display and not sent back to core (the helper should not need to consider fields which are there only for display purpose I think).

Yeah, readonly sounds better. disabled is a consequence of the fact that the field is readonly and is usually something you toggle. Read only makes it clear that we don't need to process this data on save

Small nit: I would rename readonly to displayonly as with readonly I would expect to have them to make the round trip and be sent back to core.

I don't agree as readonly seems clear enough to me but displayonly is also fine. I just wanted it to be differentiated from the existing disabled option on the frontend.

gjohansson-ST · 2024-11-23T11:09:19Z

homeassistant/components/history_stats/config_flow.py

@@ -66,6 +66,17 @@ async def validate_options(
 )
 DATA_SCHEMA_OPTIONS = vol.Schema(
    {
+        vol.Optional(CONF_ENTITY_ID, description={"readonly": True}): EntitySelector(),


See home-assistant/frontend#22592 (comment)

renamed to read_only

…tory_stats.

joostlek · 2025-01-16T12:25:24Z

So we discussed this PR in the core team and we had the following remarks:

Concept is good
Developer documentation should make it clear with examples when a read only selector is useful
The PR doesn't do what the title suggests, the PR adds a flag to voluptuous. Ideally we add the read_only flag to the SelectorConfig
Either frontend should not be allowed to send data for a read_only selector, or the data must match. In either case, this must be checked. We think frontend should decide which option is better. Ideally we align with what the const selector does.

MindFreeze · 2025-01-16T12:43:07Z

Frontend shouldn't be allowed to send read only or core can just discard it. Checking if it matches is not worth it.

karwosts · 2025-01-18T20:25:16Z

The PR doesn't do what the title suggests, the PR adds a flag to voluptuous. Ideally we add the read_only flag to the SelectorConfig

I've changed such that read_only is a flag to the selector config. Hope I have understood what you were asking for.

        vol.Optional(CONF_STATE): TextSelector(
            TextSelectorConfig(multiple=True, read_only=True)
        ),

Either frontend should not be allowed to send data for a read_only selector, or the data must match. In either case, this must be checked. We think frontend should decide which option is better. Ideally we align with what the const selector does.

Current behavior of frontend is that it does not send data for read_only selector.

karwosts requested a review from a team as a code owner October 29, 2024 19:42

home-assistant bot added cla-signed core Hacktoberfest has-tests integration: history_stats new-feature small-pr PRs with less than 30 lines. Quality Scale: internal labels Oct 29, 2024

karwosts mentioned this pull request Oct 29, 2024

Statistics Helper: state characteristics not mentioned in settings #125767

Open

gjohansson-ST reviewed Oct 29, 2024

View reviewed changes

MartinHjelmare changed the title ~~Support disabled selectors in config_flows. Add config visibility to history_stats.~~ Support disabled selectors in config_flows Oct 29, 2024

karwosts mentioned this pull request Oct 29, 2024

Support for disabled selectors in config flow home-assistant/frontend#22592

Closed

9 tasks

karwosts changed the title ~~Support disabled selectors in config_flows~~ Support readonly selectors in config_flows Oct 30, 2024

MindFreeze approved these changes Nov 23, 2024

View reviewed changes

gjohansson-ST reviewed Nov 23, 2024

View reviewed changes

karwosts added 4 commits November 23, 2024 05:53

Allow disabled selectors in config flows. Show hidden options for his…

c321d75

…tory_stats.

fix tests

e5ef581

use optional instead of required

9407b78

rename flag to readonly

a1ce4a5

karwosts force-pushed the config-flow-disabled-selectors branch from b501945 to a1ce4a5 Compare November 23, 2024 13:53

rename to read_only

4d3aa45

karwosts marked this pull request as draft January 16, 2025 15:53

Update to use read_only field as part of selector definition

958d76f

karwosts marked this pull request as ready for review January 18, 2025 17:24

karwosts added 2 commits January 18, 2025 18:42

lint fix

4915f25

Fix test

58489d5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support readonly selectors in config_flows #129456

Support readonly selectors in config_flows #129456

karwosts commented Oct 29, 2024 •

edited

Loading

gjohansson-ST left a comment •

edited

Loading

gjohansson-ST Oct 29, 2024

karwosts Oct 29, 2024

karwosts Oct 29, 2024

MindFreeze commented Oct 30, 2024

epenet commented Oct 30, 2024

karwosts commented Oct 30, 2024

tetele commented Oct 30, 2024

MindFreeze commented Oct 30, 2024

gjohansson-ST Nov 23, 2024

karwosts Nov 23, 2024

joostlek commented Jan 16, 2025

MindFreeze commented Jan 16, 2025

karwosts commented Jan 18, 2025

Support readonly selectors in config_flows #129456

Are you sure you want to change the base?

Support readonly selectors in config_flows #129456

Conversation

karwosts commented Oct 29, 2024 • edited Loading

Proposed change

Type of change

Additional information

Checklist

gjohansson-ST left a comment • edited Loading

Choose a reason for hiding this comment

gjohansson-ST Oct 29, 2024

Choose a reason for hiding this comment

karwosts Oct 29, 2024

Choose a reason for hiding this comment

karwosts Oct 29, 2024

Choose a reason for hiding this comment

MindFreeze commented Oct 30, 2024

epenet commented Oct 30, 2024

karwosts commented Oct 30, 2024

tetele commented Oct 30, 2024

MindFreeze commented Oct 30, 2024

gjohansson-ST Nov 23, 2024

Choose a reason for hiding this comment

karwosts Nov 23, 2024

Choose a reason for hiding this comment

joostlek commented Jan 16, 2025

MindFreeze commented Jan 16, 2025

karwosts commented Jan 18, 2025

karwosts commented Oct 29, 2024 •

edited

Loading

gjohansson-ST left a comment •

edited

Loading