Review fixes: persisted flow-user admin seed and real cv.multi_select

Review rounds 1-2 (Codex gpt-5.6-terra) found two HIGHs against real HA:
- entry.context is not a durable record — the config flow now persists
  the completing HA user into entry data (CONF_ADMIN_USER_IDS) and the
  seed precedence becomes: options copy (the current list) first, then
  the config flow's entry-data copy, then the context user as a last
  resort. Config-flow tests cover the persisted user and the
  context-free path; a seed-precedence test covers options > data.
- the hand-rolled closure could not serialize as an HA multi-select.
  The harness now mocks homeassistant.helpers.config_validation with a
  faithful multi_select, and the flow uses cv.multi_select exactly as
  real HA code does. Suite: 904 passed.
This commit is contained in:
Joshua King
2026-09-15 17:13:32 -04:00
parent 0d61ea2c88
commit d90c170460
6 changed files with 114 additions and 35 deletions
+18 -7
View File
@@ -242,10 +242,16 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
# record without a live connection (a previous setup's # record without a live connection (a previous setup's
# close-on-failure, or a hand-replaced file) must get the # close-on-failure, or a hand-replaced file) must get the
# missing/valid/corrupt classification too, never a raw # missing/valid/corrupt classification too, never a raw
# sqlite3 error and never skipped migrations. # sqlite3 error and never skipped migrations — and the
# same allowlist seeding, so a reopened fresh file is
# never ownerless.
db = await _async_open_database( db = await _async_open_database(
hass, await async_get_db_path(hass) hass, await async_get_db_path(hass)
) )
stored_admin_ids = (
entry.options.get(CONF_ADMIN_USER_IDS)
or entry.data.get(CONF_ADMIN_USER_IDS)
)
context = getattr(entry, "context", None) context = getattr(entry, "context", None)
context_user_id = ( context_user_id = (
context.get("user_id") context.get("user_id")
@@ -254,7 +260,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
) )
await seed_setup_admin( await seed_setup_admin(
db, db,
stored_admin_ids=entry.data.get(CONF_ADMIN_USER_IDS), stored_admin_ids=stored_admin_ids,
context_user_id=context_user_id, context_user_id=context_user_id,
) )
existing.database = db existing.database = db
@@ -270,10 +276,15 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
database = await _async_open_database(hass, await async_get_db_path(hass)) database = await _async_open_database(hass, await async_get_db_path(hass))
try: try:
# Seed the admin allowlist on first setup so the owner is never # Seed the admin allowlist on first setup so the owner is never
# locked out: an empty database takes the config entry's stored # locked out: an empty database takes the persisted admin copy
# admin copy (disaster recovery), else the HA user who # — the options flow's saved list first (it is the current
# completed the config flow. Non-empty allowlists are never # one), else the config flow's user from entry data — and only
# touched (fail-closed narrowing is deliberate). # then the HA user still present on the entry context. No-op
# on a non-empty list (fail-closed narrowing is deliberate).
stored_admin_ids = (
entry.options.get(CONF_ADMIN_USER_IDS)
or entry.data.get(CONF_ADMIN_USER_IDS)
)
context = getattr(entry, "context", None) context = getattr(entry, "context", None)
context_user_id = ( context_user_id = (
context.get("user_id") context.get("user_id")
@@ -282,7 +293,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
) )
await seed_setup_admin( await seed_setup_admin(
database, database,
stored_admin_ids=entry.data.get(CONF_ADMIN_USER_IDS), stored_admin_ids=stored_admin_ids,
context_user_id=context_user_id, context_user_id=context_user_id,
) )
remove_update_listener = entry.add_update_listener(_async_update_listener) remove_update_listener = entry.add_update_listener(_async_update_listener)
+18 -3
View File
@@ -7,7 +7,7 @@ from homeassistant import config_entries
from homeassistant.core import callback from homeassistant.core import callback
from homeassistant.data_entry_flow import FlowResult from homeassistant.data_entry_flow import FlowResult
from .const import DOMAIN from .const import CONF_ADMIN_USER_IDS, DOMAIN
class NestQuestConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): class NestQuestConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
@@ -28,7 +28,14 @@ class NestQuestConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
async def async_step_user( async def async_step_user(
self, user_input: dict[str, Any] | None = None self, user_input: dict[str, Any] | None = None
) -> FlowResult: ) -> FlowResult:
"""Handle the user step of the config flow.""" """Handle the user step of the config flow.
The HA user completing the flow is persisted into the entry
data as the initial admin allowlist copy: real HA does not
guarantee the flow context survives to setup, and entry data is
the persisted record first setup seeds from — the owner is
never locked out.
"""
await self.async_set_unique_id(DOMAIN, raise_on_progress=False) await self.async_set_unique_id(DOMAIN, raise_on_progress=False)
if self._async_in_progress() or self._async_current_entries(): if self._async_in_progress() or self._async_current_entries():
return self.async_abort(reason="single_instance_allowed") return self.async_abort(reason="single_instance_allowed")
@@ -36,4 +43,12 @@ class NestQuestConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
if user_input is None: if user_input is None:
return self.async_show_form(step_id="user") return self.async_show_form(step_id="user")
return self.async_create_entry(title="NestQuest", data=user_input) data = dict(user_input)
flow_user_id = (
self.context.get("user_id")
if isinstance(self.context, dict)
else None
)
if flow_user_id:
data[CONF_ADMIN_USER_IDS] = [flow_user_id]
return self.async_create_entry(title="NestQuest", data=data)
+4 -24
View File
@@ -7,6 +7,7 @@ from typing import Any
import voluptuous as vol import voluptuous as vol
from homeassistant import config_entries from homeassistant import config_entries
from homeassistant.data_entry_flow import FlowResult from homeassistant.data_entry_flow import FlowResult
from homeassistant.helpers import config_validation as cv
from .const import ( from .const import (
CONF_ADMIN_USER_IDS, CONF_ADMIN_USER_IDS,
@@ -31,29 +32,6 @@ def _parse_time(value: str) -> bool:
return _TIME_PATTERN.fullmatch(value) is not None return _TIME_PATTERN.fullmatch(value) is not None
def _multi_select(choices: dict[str, str]):
"""A voluptuous validator for a multi-select of ``choices``.
The mock-only test harness cannot import HA's config-validation
helpers (they pull in real HA), so this mirrors the shape of
``homeassistant.helpers.config_validation.multi_select``: accepts a
list of keys from ``choices``, returns the list unchanged, rejects
anything else (None becomes the empty selection).
"""
def _validate(value):
if value is None:
return []
if not isinstance(value, list):
raise vol.Invalid("expected a list of selections")
for item in value:
if item not in choices:
raise vol.Invalid(f"{item!r} is not a valid choice")
return list(value)
return _validate
def _async_validate(user_input: dict[str, Any]) -> dict[str, str]: def _async_validate(user_input: dict[str, Any]) -> dict[str, str]:
"""Validate user input and return field-level errors instead of raising.""" """Validate user input and return field-level errors instead of raising."""
errors: dict[str, str] = {} errors: dict[str, str] = {}
@@ -106,11 +84,13 @@ def _build_schema(
): int, ): int,
} }
if admin_choices is not None: if admin_choices is not None:
# HA's supported multi-select validator: the frontend can
# serialize the field as a real picker of the given choices.
fields[ fields[
vol.Optional( vol.Optional(
CONF_ADMIN_USER_IDS, default=list(admin_default or []) CONF_ADMIN_USER_IDS, default=list(admin_default or [])
) )
] = _multi_select(admin_choices) ] = cv.multi_select(admin_choices)
return vol.Schema(fields) return vol.Schema(fields)
+28
View File
@@ -42,6 +42,8 @@ _HA_MODULES = (
"homeassistant.config_entries", "homeassistant.config_entries",
"homeassistant.data_entry_flow", "homeassistant.data_entry_flow",
"homeassistant.exceptions", "homeassistant.exceptions",
"homeassistant.helpers",
"homeassistant.helpers.config_validation",
) )
def _find_spec(name: str): def _find_spec(name: str):
@@ -79,6 +81,8 @@ for _parent, _child in (
("homeassistant", "data_entry_flow"), ("homeassistant", "data_entry_flow"),
("homeassistant", "core"), ("homeassistant", "core"),
("homeassistant", "exceptions"), ("homeassistant", "exceptions"),
("homeassistant", "helpers"),
("homeassistant.helpers", "config_validation"),
): ):
setattr(sys.modules[_parent], _child, sys.modules[f"{_parent}.{_child}"]) setattr(sys.modules[_parent], _child, sys.modules[f"{_parent}.{_child}"])
@@ -181,6 +185,30 @@ class ConfigEntryNotReady(Exception):
_exceptions_mock.ConfigEntryNotReady = ConfigEntryNotReady _exceptions_mock.ConfigEntryNotReady = ConfigEntryNotReady
import voluptuous as vol
def _multi_select(choices: dict):
"""Mirror homeassistant.helpers.config_validation.multi_select.
The options flow's admin picker uses HA's supported multi-select
validator (schema-serializable by HA's frontend); the mock provides
the same contract: a list whose members are keys of ``choices``.
"""
def _validate(selected):
if not isinstance(selected, list):
raise vol.Invalid("Not a list")
for value in selected:
if value not in choices:
raise vol.Invalid(f"{value} is not a valid option")
return selected
return _validate
_ha_mock("homeassistant.helpers.config_validation").multi_select = _multi_select
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
# Shared helpers for the behavior tests. # Shared helpers for the behavior tests.
+24
View File
@@ -278,3 +278,27 @@ async def test_setup_does_not_reseed_narrowed_allowlist(
finally: finally:
await database.close() await database.close()
assert await async_unload_entry(hass, entry_reloaded) is True assert await async_unload_entry(hass, entry_reloaded) is True
async def test_setup_seed_prefers_options_over_entry_data(
hass, make_entry
) -> None:
"""Seed precedence on a fresh database: the options flow's saved
list (the current one) beats the config flow's original copy."""
from custom_components.nestquest.const import CONF_ADMIN_USER_IDS
entry = await _setup_entry(
hass,
make_entry(
options={CONF_ADMIN_USER_IDS: ["user-current"]},
data={CONF_ADMIN_USER_IDS: ["ha-owner"]},
context={"user_id": "ha-owner"},
),
hass.registry,
)
database = entry.runtime_data.database
try:
assert await list_admin_ids(database) == ["user-current"]
finally:
await database.close()
assert await async_unload_entry(hass, entry) is True
+22 -1
View File
@@ -154,4 +154,25 @@ def test_no_hardcoded_domain_or_db_filename_in_new_modules() -> None:
) )
assert node.value != "nestquest.db", ( assert node.value != "nestquest.db", (
f"Found hard-coded database filename in {py_file.name}:{node.lineno}" f"Found hard-coded database filename in {py_file.name}:{node.lineno}"
) )
async def test_flow_persists_flow_user_as_initial_admin(hass, make_flow) -> None:
"""The HA user completing the flow lands in entry data as the
initial admin copy: entry.context is not persisted by real HA, so
the durable record must carry the owner."""
from custom_components.nestquest.const import CONF_ADMIN_USER_IDS
flow = make_flow(hass)
flow.context = {"user_id": "ha-owner"}
result = await flow.async_step_user({})
assert result["type"] == "create_entry"
assert result["data"] == {CONF_ADMIN_USER_IDS: ["ha-owner"]}
async def test_flow_without_context_user_keeps_data_untouched(
hass, make_flow
) -> None:
flow = make_flow(hass)
result = await flow.async_step_user({"horizon_days": 7})
assert result["type"] == "create_entry"
assert result["data"] == {"horizon_days": 7}