diff --git a/custom_components/nvidia_shield/config_flow.py b/custom_components/nvidia_shield/config_flow.py index acf90cd..bffafbf 100644 --- a/custom_components/nvidia_shield/config_flow.py +++ b/custom_components/nvidia_shield/config_flow.py @@ -179,20 +179,42 @@ def _user_schema_dict(sel, defaults: dict | None = None) -> dict: return schema_dict +class _ClearingEntitySelector(selector.EntitySelector): + """An entity selector that accepts a CLEARED field (t47 F1). + + Clearing an optional entity field used to depend on the frontend OMITTING the + key. It does not have to: a submission can carry ``''`` or ``None``, and real + Home Assistant's EntitySelector rejects both — the reviewer's real-HA run + produced "Schema validation failed at script_turn_on" from an explicit clear. + That made clearing look like a broken form rather than a choice, on every + entity field of this page (the script selectors are the newest, not the only + ones). + + Returning the empty value unchanged lets the flow read it as "no selection", + while a real entity id still goes through the parent's validation. The class + stays an EntitySelector, so the frontend still renders an entity picker. + """ + + def __call__(self, data): + if data is None or (isinstance(data, str) and not data.strip()): + return "" + return super().__call__(data) + + def _build_selectors(): """Build reusable selector instances.""" return { - "media_player": selector.EntitySelector( + "media_player": _ClearingEntitySelector( selector.EntitySelectorConfig(domain="media_player") ), - "receiver": selector.EntitySelector( + "receiver": _ClearingEntitySelector( selector.EntitySelectorConfig(domain="media_player", device_class="receiver") ), - "power": selector.EntitySelector( + "power": _ClearingEntitySelector( selector.EntitySelectorConfig(domain=["switch", "binary_sensor"]) ), # t44: a script entity per command override - "script": selector.EntitySelector( + "script": _ClearingEntitySelector( selector.EntitySelectorConfig(domain="script") ), } @@ -846,6 +868,11 @@ class NvidiaShieldOptionsFlow(config_entries.OptionsFlowWithReload): """Single-page options.""" errors = {} if user_input is not None: + # t47 F1: a field submitted as None means "not supplied" — only an + # explicit empty value ('' or a cleared selector) clears a setting, so + # the two cannot be confused. Note the schema itself already tolerates + # the empty value (see _ClearingEntitySelector). + user_input = {key: value for key, value in user_input.items() if value is not None} # P5-2: validate the digit delay (0.05–1.0 s, §7 window) delay = user_input.pop(CONF_CHANNEL_DIGIT_DELAY, None) if delay is not None: diff --git a/tests/entity_stublib.py b/tests/entity_stublib.py index f3198f0..5d59a4b 100644 --- a/tests/entity_stublib.py +++ b/tests/entity_stublib.py @@ -42,6 +42,37 @@ ENTITIES_STUB_NAMES = ( ) +class EntitySelectorStub: + """Stand-in for HA's EntitySelector that REJECTS '' and None, like the real one. + + Real HA answers a cleared entity field with "Schema validation failed at …" + (t46 F1), so a stub that accepted anything would hide exactly the trap the flow + has to tolerate; tests/test_config_flow_ux.py validates a schema with both + values to prove it. + """ + + def __init__(self, config=None): + self.config = config + + def __call__(self, data): + if not isinstance(data, str) or not data.strip(): + raise ValueError("Entity is not a valid entity id") + return data + + +def install_selector_faces(selector_module) -> None: + """Populate a ``homeassistant.helpers.selector`` stub module. + + One definition for every installer (entity_stublib and the four per-file + installers in the flow tests), so the stubs cannot drift apart. + """ + selector_module.EntitySelector = EntitySelectorStub + selector_module.EntitySelectorConfig = lambda *args, **kwargs: "" + selector_module.SelectSelector = lambda *args, **kwargs: None + selector_module.SelectSelectorConfig = lambda **kwargs: "" + selector_module.SelectSelectorMode = types.SimpleNamespace(DROPDOWN="dropdown") + + class MediaPlayerState(enum.StrEnum): OFF = "off" ON = "on" @@ -428,7 +459,7 @@ def install() -> None: event.async_track_state_change_event = lambda *args, **kwargs: (lambda: None) helpers.event = event helpers.selector = types.SimpleNamespace( - EntitySelector=lambda *a, **k: None, + EntitySelector=EntitySelectorStub, EntitySelectorConfig=lambda *a, **k: "", SelectSelector=lambda *a, **k: None, SelectSelectorConfig=lambda **k: "", @@ -489,8 +520,7 @@ def install() -> None: # `_STUB_FACES` honest: install() then really does provide every face a test # file may have popped, which is what its guard checks. selector = types.ModuleType("homeassistant.helpers.selector") - selector.EntitySelector = lambda *args, **kwargs: None - selector.EntitySelectorConfig = lambda *args, **kwargs: "" + install_selector_faces(selector) selector.SelectSelector = lambda *args, **kwargs: None selector.SelectSelectorConfig = lambda **kwargs: "" selector.SelectSelectorMode = types.SimpleNamespace(DROPDOWN="dropdown") diff --git a/tests/test_config_flow_pairing.py b/tests/test_config_flow_pairing.py index 96ad0b7..81a1c0b 100644 --- a/tests/test_config_flow_pairing.py +++ b/tests/test_config_flow_pairing.py @@ -127,11 +127,7 @@ def _install_import_stubs() -> None: helpers.device_registry = device_registry sys.modules["homeassistant.helpers.device_registry"] = device_registry selector = types.ModuleType("homeassistant.helpers.selector") - selector.EntitySelector = lambda *a, **k: None - selector.EntitySelectorConfig = lambda *a, **k: "" - selector.SelectSelector = lambda *a, **k: None - selector.SelectSelectorConfig = lambda **k: "" - selector.SelectSelectorMode = types.SimpleNamespace(DROPDOWN="dropdown") + entity_stublib.install_selector_faces(selector) sys.modules["homeassistant.helpers.selector"] = selector diff --git a/tests/test_config_flow_protocol.py b/tests/test_config_flow_protocol.py index c255d23..5d07911 100644 --- a/tests/test_config_flow_protocol.py +++ b/tests/test_config_flow_protocol.py @@ -220,11 +220,7 @@ def _install_stubs() -> None: helpers.device_registry = device_registry sys.modules["homeassistant.helpers.device_registry"] = device_registry selector = types.ModuleType("homeassistant.helpers.selector") - selector.EntitySelector = lambda *a, **k: None - selector.EntitySelectorConfig = lambda *a, **k: "" - selector.SelectSelector = lambda *a, **k: None - selector.SelectSelectorConfig = lambda **k: "" - selector.SelectSelectorMode = types.SimpleNamespace(DROPDOWN="dropdown") + entity_stublib.install_selector_faces(selector) sys.modules["homeassistant.helpers.selector"] = selector diff --git a/tests/test_config_flow_ux.py b/tests/test_config_flow_ux.py index fcb6af8..1a71dd5 100644 --- a/tests/test_config_flow_ux.py +++ b/tests/test_config_flow_ux.py @@ -37,6 +37,7 @@ from nsh.const import ( CONF_HOST, CONF_JELLYFIN_ENTITY, CONF_PAIRING_CODE, + CONF_POWER_ENTITY, CONF_PROTOCOL_ENABLED, CONF_SKIP_PAIRING, DEFAULT_ADB_PORT, @@ -227,11 +228,7 @@ def _install_stubs() -> None: sys.modules["homeassistant.helpers.device_registry"] = device_registry selector = types.ModuleType("homeassistant.helpers.selector") - selector.EntitySelector = lambda *a, **k: None - selector.EntitySelectorConfig = lambda *a, **k: "" - selector.SelectSelector = lambda *a, **k: None - selector.SelectSelectorConfig = lambda **k: "" - selector.SelectSelectorMode = types.SimpleNamespace(DROPDOWN="dropdown") + entity_stublib.install_selector_faces(selector) helpers.selector = selector sys.modules["homeassistant.helpers.selector"] = selector sys.modules["homeassistant.helpers"] = helpers @@ -1451,6 +1448,17 @@ class TestCopyContract: assert field in step["data"], f"options.{field} has no label" +def _options_flow(tmp_path, options=None): + """An options flow over an entry with `options`, on a fake hass.""" + module = _form_module() + flow = module.NvidiaShieldOptionsFlow() + flow.config_entry = SimpleNamespace( + options=dict(options or {}), entry_id="e1", title="Shield" + ) + flow.hass = FakeHass(str(tmp_path)) + return flow + + class TestCommandScriptOptions: """t44: one script selector per command, stored as one dict option. @@ -1460,13 +1468,7 @@ class TestCommandScriptOptions: """ def _flow(self, tmp_path, options=None): - module = _form_module() - flow = module.NvidiaShieldOptionsFlow() - flow.config_entry = SimpleNamespace( - options=dict(options or {}), entry_id="e1", title="Shield" - ) - flow.hass = FakeHass(str(tmp_path)) - return flow + return _options_flow(tmp_path, options) @staticmethod def _submitted(schema): @@ -1542,6 +1544,158 @@ class TestCommandScriptOptions: ) +class TestClearingAnEntityField: + """t47 F1: clearing must work whichever shape the field arrives in. + + Home Assistant validates the submitted page against the schema the flow last + returned, and real HA's EntitySelector rejects BOTH ``''`` and ``None`` + ("Entity is neither a valid entity ID nor a valid UUID") — so an explicit clear + was reported as a malformed form. The three shapes are pinned here, validated + through the schema exactly as HA does, and the happy path is pinned too so the + tolerance cannot start swallowing real edits. + """ + + @staticmethod + def _validate(form, data): + """Run a submitted page through the schema the form returned, as HA does. + + This is the step between the frontend and the flow, and it is where the + clearing trap used to fire. The harness's voluptuous mirror carries marker + defaults but does not validate (``vol.Schema`` is the identity there), so + each field is sent through its own schema entry — the same call HA makes, + and the one that used to raise for a cleared entity field. + """ + raw = form["data_schema"] + validators = { + getattr(key, "schema", None): value + for key, value in raw.items() + if getattr(key, "schema", None) is not None + } + validated = {} + for field, value in data.items(): + validator = validators.get(field) + validated[field] = validator(value) if callable(validator) else value + return validated + + async def test_the_schema_accepts_an_omitted_none_and_empty_value(self, tmp_path): + flow = _options_flow( + tmp_path, {CONF_COMMAND_SCRIPTS: {"turn_on": "script.watch"}} + ) + form = await flow.async_step_init(None) + + for value in ("", None): + assert self._validate(form, {f"{SCRIPT_FIELD_PREFIX}turn_on": value}) == { + f"{SCRIPT_FIELD_PREFIX}turn_on": "" + } + # a real entity id is still passed through, and an omitted key stays omitted + validated = self._validate(form, {f"{SCRIPT_FIELD_PREFIX}turn_on": "script.other"}) + assert validated[f"{SCRIPT_FIELD_PREFIX}turn_on"] == "script.other" + assert f"{SCRIPT_FIELD_PREFIX}volume_up" not in validated + + def test_a_plain_entity_selector_rejects_the_same_values(self): + """The trap itself: without the tolerant selector these shapes blow up. + + The stub mirrors real HA here (see entity_stublib.EntitySelectorStub), so + this stays honest without needing a real-HA interpreter. + """ + import homeassistant.helpers.selector as selector_module + + strict = selector_module.EntitySelector( + selector_module.EntitySelectorConfig(domain="script") + ) + + # the stub raises ValueError; real HA raises probatio.Invalid for the same + # values ("Entity is neither a valid entity ID nor a valid UUID", verified) + for value in ("", None): + with pytest.raises(ValueError): + strict(value) + + @pytest.mark.parametrize("value", ["", None]) + async def test_an_explicit_empty_value_clears_the_script(self, tmp_path, value): + flow = _options_flow( + tmp_path, {CONF_COMMAND_SCRIPTS: {"turn_on": "script.watch"}} + ) + form = await flow.async_step_init(None) + + # what HA does: validate the submitted page, then run the step + result = await flow.async_step_init( + self._validate(form, {f"{SCRIPT_FIELD_PREFIX}turn_on": value}) + ) + + assert result["type"] == "create_entry" + assert result["data"][CONF_COMMAND_SCRIPTS] == {} + + @pytest.mark.parametrize("value", ["", None]) + async def test_an_explicit_empty_value_clears_a_preexisting_entity_field( + self, tmp_path, value + ): + """The pattern predates the script selectors — jellyfin had it too.""" + flow = _options_flow( + tmp_path, + {CONF_JELLYFIN_ENTITY: "media_player.jellyfin", CONF_HOST: "10.0.0.9"}, + ) + form = await flow.async_step_init(None) + + result = await flow.async_step_init( + self._validate(form, {CONF_JELLYFIN_ENTITY: value}) + ) + + assert not result["data"][CONF_JELLYFIN_ENTITY] # cleared, not kept + assert result["data"][CONF_HOST] == "10.0.0.9" # untouched fields survive + + async def test_an_omitted_script_field_is_cleared_as_well(self, tmp_path): + """The script dict is rebuilt from the page, so omitting a command clears it.""" + flow = _options_flow( + tmp_path, {CONF_COMMAND_SCRIPTS: {"turn_on": "script.watch"}} + ) + form = await flow.async_step_init(None) + + result = await flow.async_step_init(self._validate(form, {})) + + assert result["data"][CONF_COMMAND_SCRIPTS] == {} + + async def test_an_omitted_entity_field_keeps_its_stored_value(self, tmp_path): + """An omitted key is "not touched" for the patched options, not a clear.""" + flow = _options_flow( + tmp_path, {CONF_JELLYFIN_ENTITY: "media_player.jellyfin"} + ) + form = await flow.async_step_init(None) + + result = await flow.async_step_init(self._validate(form, {CONF_HOST: "10.0.0.9"})) + + assert result["data"][CONF_JELLYFIN_ENTITY] == "media_player.jellyfin" + + async def test_a_prefill_submit_round_trip_changes_nothing(self, tmp_path): + stored = { + CONF_COMMAND_SCRIPTS: {"turn_on": "script.watch"}, + CONF_JELLYFIN_ENTITY: "media_player.jellyfin", + CONF_POWER_ENTITY: "switch.plug", + CONF_HOST: "10.0.0.9", + } + flow = _options_flow(tmp_path, stored) + form = await flow.async_step_init(None) + suggested = form["data_schema"].get("__suggested__", {}) + + # the page comes back carrying the stored values, and the user submits them + assert suggested[f"{SCRIPT_FIELD_PREFIX}turn_on"] == "script.watch" + result = await flow.async_step_init( + self._validate( + form, + { + f"{SCRIPT_FIELD_PREFIX}turn_on": "script.watch", + CONF_JELLYFIN_ENTITY: "media_player.jellyfin", + CONF_POWER_ENTITY: "switch.plug", + CONF_HOST: "10.0.0.9", + }, + ) + ) + + assert result["data"][CONF_COMMAND_SCRIPTS] == {"turn_on": "script.watch"} + assert result["data"][CONF_JELLYFIN_ENTITY] == "media_player.jellyfin" + assert result["data"][CONF_POWER_ENTITY] == "switch.plug" + assert result["data"][CONF_HOST] == "10.0.0.9" + + class TestStringsTranslationsParity: """Both copies of the UI copy must expose the same key tree.""" diff --git a/tests/test_standalone_setup.py b/tests/test_standalone_setup.py index 8e87fb5..a384f48 100644 --- a/tests/test_standalone_setup.py +++ b/tests/test_standalone_setup.py @@ -215,11 +215,7 @@ def _install_stubs() -> None: helpers.device_registry = device_registry sys.modules["homeassistant.helpers.device_registry"] = device_registry selector = types.ModuleType("homeassistant.helpers.selector") - selector.EntitySelector = lambda *a, **k: None - selector.EntitySelectorConfig = lambda *a, **k: "" - selector.SelectSelector = lambda *a, **k: None - selector.SelectSelectorConfig = lambda **k: "" - selector.SelectSelectorMode = types.SimpleNamespace(DROPDOWN="dropdown") + entity_stublib.install_selector_faces(selector) sys.modules["homeassistant.helpers.selector"] = selector