From c6ea57458c1f7898e99a6e914eb531c906225ebd Mon Sep 17 00:00:00 2001 From: Michael Hansen Date: Sat, 3 Feb 2024 05:14:33 -0600 Subject: [PATCH] Pass slots to error messages instead of IDs [rework] (#109410) Co-authored-by: tetele --- .../components/conversation/__init__.py | 2 +- .../components/conversation/default_agent.py | 28 ++++--- .../components/conversation/manifest.json | 2 +- homeassistant/helpers/intent.py | 23 +++-- homeassistant/package_constraints.txt | 2 +- requirements_all.txt | 2 +- requirements_test_all.txt | 2 +- .../conversation/snapshots/test_init.ambr | 12 +-- .../conversation/test_default_agent.py | 84 ++++++++++++------- 9 files changed, 95 insertions(+), 62 deletions(-) diff --git a/homeassistant/components/conversation/__init__.py b/homeassistant/components/conversation/__init__.py index 7ca7fec115f..09b0e8e2310 100644 --- a/homeassistant/components/conversation/__init__.py +++ b/homeassistant/components/conversation/__init__.py @@ -349,7 +349,7 @@ async def websocket_hass_agent_debug( }, # Slot values that would be received by the intent "slots": { # direct access to values - entity_key: entity.value + entity_key: entity.text or entity.value for entity_key, entity in result.entities.items() }, # Extra slot details, such as the originally matched text diff --git a/homeassistant/components/conversation/default_agent.py b/homeassistant/components/conversation/default_agent.py index a2cb3b68041..fb33d87e107 100644 --- a/homeassistant/components/conversation/default_agent.py +++ b/homeassistant/components/conversation/default_agent.py @@ -1,4 +1,5 @@ """Standard conversation implementation for Home Assistant.""" + from __future__ import annotations import asyncio @@ -264,9 +265,11 @@ class DefaultAgent(AbstractConversationAgent): _LOGGER.debug( "Recognized intent '%s' for template '%s' but had unmatched: %s", result.intent.name, - result.intent_sentence.text - if result.intent_sentence is not None - else "", + ( + result.intent_sentence.text + if result.intent_sentence is not None + else "" + ), result.unmatched_entities_list, ) error_response_type, error_response_args = _get_unmatched_response(result) @@ -285,7 +288,8 @@ class DefaultAgent(AbstractConversationAgent): # Slot values to pass to the intent slots = { - entity.name: {"value": entity.value} for entity in result.entities_list + entity.name: {"value": entity.value, "text": entity.text or entity.value} + for entity in result.entities_list } try: @@ -474,9 +478,11 @@ class DefaultAgent(AbstractConversationAgent): for entity_name, entity_value in recognize_result.entities.items() }, # First matched or unmatched state - "state": template.TemplateState(self.hass, state1) - if state1 is not None - else None, + "state": ( + template.TemplateState(self.hass, state1) + if state1 is not None + else None + ), "query": { # Entity states that matched the query (e.g, "on") "matched": [ @@ -734,7 +740,7 @@ class DefaultAgent(AbstractConversationAgent): if not entity: # Default name - entity_names.append((state.name, state.name, context)) + entity_names.append((state.name, state.entity_id, context)) continue if entity.aliases: @@ -742,10 +748,10 @@ class DefaultAgent(AbstractConversationAgent): if not alias.strip(): continue - entity_names.append((alias, alias, context)) + entity_names.append((alias, state.entity_id, context)) # Default name - entity_names.append((state.name, state.name, context)) + entity_names.append((state.name, state.entity_id, context)) # Expose all areas areas = ar.async_get(self.hass) @@ -785,7 +791,7 @@ class DefaultAgent(AbstractConversationAgent): if device_area is None: return None - return {"area": device_area.id} + return {"area": {"value": device_area.id, "text": device_area.name}} def _get_error_text( self, diff --git a/homeassistant/components/conversation/manifest.json b/homeassistant/components/conversation/manifest.json index 1e46170024c..e4317052b04 100644 --- a/homeassistant/components/conversation/manifest.json +++ b/homeassistant/components/conversation/manifest.json @@ -7,5 +7,5 @@ "integration_type": "system", "iot_class": "local_push", "quality_scale": "internal", - "requirements": ["hassil==1.6.0", "home-assistant-intents==2024.2.2"] + "requirements": ["hassil==1.6.1", "home-assistant-intents==2024.2.2"] } diff --git a/homeassistant/helpers/intent.py b/homeassistant/helpers/intent.py index 26468f1fdb7..fe399659a56 100644 --- a/homeassistant/helpers/intent.py +++ b/homeassistant/helpers/intent.py @@ -1,4 +1,5 @@ """Module to coordinate user intentions.""" + from __future__ import annotations import asyncio @@ -401,17 +402,21 @@ class ServiceIntentHandler(IntentHandler): hass = intent_obj.hass slots = self.async_validate_slots(intent_obj.slots) - name: str | None = slots.get("name", {}).get("value") - if name == "all": + name_slot = slots.get("name", {}) + entity_id: str | None = name_slot.get("value") + entity_name: str | None = name_slot.get("text") + if entity_id == "all": # Don't match on name if targeting all entities - name = None + entity_id = None # Look up area first to fail early - area_name = slots.get("area", {}).get("value") + area_slot = slots.get("area", {}) + area_id = area_slot.get("value") + area_name = area_slot.get("text") area: area_registry.AreaEntry | None = None - if area_name is not None: + if area_id is not None: areas = area_registry.async_get(hass) - area = areas.async_get_area(area_name) or areas.async_get_area_by_name( + area = areas.async_get_area(area_id) or areas.async_get_area_by_name( area_name ) if area is None: @@ -431,7 +436,7 @@ class ServiceIntentHandler(IntentHandler): states = list( async_match_states( hass, - name=name, + name=entity_id, area=area, domains=domains, device_classes=device_classes, @@ -442,8 +447,8 @@ class ServiceIntentHandler(IntentHandler): if not states: # No states matched constraints raise NoStatesMatchedError( - name=name, - area=area_name, + name=entity_name or entity_id, + area=area_name or area_id, domains=domains, device_classes=device_classes, ) diff --git a/homeassistant/package_constraints.txt b/homeassistant/package_constraints.txt index 96efd08bc1f..7746745da6b 100644 --- a/homeassistant/package_constraints.txt +++ b/homeassistant/package_constraints.txt @@ -26,7 +26,7 @@ ha-av==10.1.1 ha-ffmpeg==3.1.0 habluetooth==2.4.0 hass-nabucasa==0.76.0 -hassil==1.6.0 +hassil==1.6.1 home-assistant-bluetooth==1.12.0 home-assistant-frontend==20240202.0 home-assistant-intents==2024.2.2 diff --git a/requirements_all.txt b/requirements_all.txt index 7c4e1045123..6b87dd3d5b0 100644 --- a/requirements_all.txt +++ b/requirements_all.txt @@ -1025,7 +1025,7 @@ hass-nabucasa==0.76.0 hass-splunk==0.1.1 # homeassistant.components.conversation -hassil==1.6.0 +hassil==1.6.1 # homeassistant.components.jewish_calendar hdate==0.10.4 diff --git a/requirements_test_all.txt b/requirements_test_all.txt index 14cd5e5f8a1..f3674dd283c 100644 --- a/requirements_test_all.txt +++ b/requirements_test_all.txt @@ -830,7 +830,7 @@ habluetooth==2.4.0 hass-nabucasa==0.76.0 # homeassistant.components.conversation -hassil==1.6.0 +hassil==1.6.1 # homeassistant.components.jewish_calendar hdate==0.10.4 diff --git a/tests/components/conversation/snapshots/test_init.ambr b/tests/components/conversation/snapshots/test_init.ambr index 468f3215cb7..034bfafc1f5 100644 --- a/tests/components/conversation/snapshots/test_init.ambr +++ b/tests/components/conversation/snapshots/test_init.ambr @@ -1397,7 +1397,7 @@ 'name': dict({ 'name': 'name', 'text': 'my cool light', - 'value': 'my cool light', + 'value': 'light.kitchen', }), }), 'intent': dict({ @@ -1422,7 +1422,7 @@ 'name': dict({ 'name': 'name', 'text': 'my cool light', - 'value': 'my cool light', + 'value': 'light.kitchen', }), }), 'intent': dict({ @@ -1498,7 +1498,7 @@ 'sentence_template': '[tell me] how many {on_off_domains:domain} (is|are) {on_off_states:state} [in ]', 'slots': dict({ 'area': 'kitchen', - 'domain': 'light', + 'domain': 'lights', 'state': 'on', }), 'source': 'builtin', @@ -1572,7 +1572,7 @@ 'name': dict({ 'name': 'name', 'text': 'test light', - 'value': 'test light', + 'value': 'light.demo_1234', }), }), 'intent': dict({ @@ -1581,7 +1581,7 @@ 'match': True, 'sentence_template': '[] brightness [to] ', 'slots': dict({ - 'brightness': 100, + 'brightness': '100%', 'name': 'test light', }), 'source': 'builtin', @@ -1604,7 +1604,7 @@ 'name': dict({ 'name': 'name', 'text': 'test light', - 'value': 'test light', + 'value': 'light.demo_1234', }), }), 'intent': dict({ diff --git a/tests/components/conversation/test_default_agent.py b/tests/components/conversation/test_default_agent.py index d7182aa3c2f..0cf343a3e20 100644 --- a/tests/components/conversation/test_default_agent.py +++ b/tests/components/conversation/test_default_agent.py @@ -1,4 +1,5 @@ """Test for the default agent.""" + from collections import defaultdict from unittest.mock import AsyncMock, patch @@ -85,8 +86,10 @@ async def test_exposed_areas( entity_registry: er.EntityRegistry, ) -> None: """Test that all areas are exposed.""" - area_kitchen = area_registry.async_get_or_create("kitchen") - area_bedroom = area_registry.async_get_or_create("bedroom") + area_kitchen = area_registry.async_get_or_create("kitchen_id") + area_kitchen = area_registry.async_update(area_kitchen.id, name="kitchen") + area_bedroom = area_registry.async_get_or_create("bedroom_id") + area_bedroom = area_registry.async_update(area_bedroom.id, name="bedroom") entry = MockConfigEntry() entry.add_to_hass(hass) @@ -122,6 +125,9 @@ async def test_exposed_areas( # All is well for the exposed kitchen light assert result.response.response_type == intent.IntentResponseType.ACTION_DONE + assert result.response.intent is not None + assert result.response.intent.slots["area"]["value"] == area_kitchen.id + assert result.response.intent.slots["area"]["text"] == area_kitchen.normalized_name # Bedroom has no exposed entities result = await conversation.async_converse( @@ -195,7 +201,8 @@ async def test_unexposed_entities_skipped( entity_registry: er.EntityRegistry, ) -> None: """Test that unexposed entities are skipped in exposed areas.""" - area_kitchen = area_registry.async_get_or_create("kitchen") + area_kitchen = area_registry.async_get_or_create("kitchen_id") + area_kitchen = area_registry.async_update(area_kitchen.id, name="kitchen") # Both lights are in the kitchen exposed_light = entity_registry.async_get_or_create("light", "demo", "1234") @@ -224,6 +231,9 @@ async def test_unexposed_entities_skipped( assert len(calls) == 1 assert result.response.response_type == intent.IntentResponseType.ACTION_DONE + assert result.response.intent is not None + assert result.response.intent.slots["area"]["value"] == area_kitchen.id + assert result.response.intent.slots["area"]["text"] == area_kitchen.normalized_name # Only one light should be returned hass.states.async_set(exposed_light.entity_id, "on") @@ -314,8 +324,10 @@ async def test_device_area_context( turn_on_calls = async_mock_service(hass, "light", "turn_on") turn_off_calls = async_mock_service(hass, "light", "turn_off") - area_kitchen = area_registry.async_get_or_create("Kitchen") - area_bedroom = area_registry.async_get_or_create("Bedroom") + area_kitchen = area_registry.async_get_or_create("kitchen_id") + area_kitchen = area_registry.async_update(area_kitchen.id, name="kitchen") + area_bedroom = area_registry.async_get_or_create("bedroom_id") + area_bedroom = area_registry.async_update(area_bedroom.id, name="bedroom") # Create 2 lights in each area area_lights = defaultdict(list) @@ -363,13 +375,14 @@ async def test_device_area_context( assert result.response.response_type == intent.IntentResponseType.ACTION_DONE assert result.response.intent is not None assert result.response.intent.slots["area"]["value"] == area_kitchen.id + assert result.response.intent.slots["area"]["text"] == area_kitchen.normalized_name # Verify only kitchen lights were targeted assert {s.entity_id for s in result.response.matched_states} == { - e.entity_id for e in area_lights["kitchen"] + e.entity_id for e in area_lights[area_kitchen.id] } assert {c.data["entity_id"][0] for c in turn_on_calls} == { - e.entity_id for e in area_lights["kitchen"] + e.entity_id for e in area_lights[area_kitchen.id] } turn_on_calls.clear() @@ -386,13 +399,14 @@ async def test_device_area_context( assert result.response.response_type == intent.IntentResponseType.ACTION_DONE assert result.response.intent is not None assert result.response.intent.slots["area"]["value"] == area_bedroom.id + assert result.response.intent.slots["area"]["text"] == area_bedroom.normalized_name # Verify only bedroom lights were targeted assert {s.entity_id for s in result.response.matched_states} == { - e.entity_id for e in area_lights["bedroom"] + e.entity_id for e in area_lights[area_bedroom.id] } assert {c.data["entity_id"][0] for c in turn_on_calls} == { - e.entity_id for e in area_lights["bedroom"] + e.entity_id for e in area_lights[area_bedroom.id] } turn_on_calls.clear() @@ -409,13 +423,14 @@ async def test_device_area_context( assert result.response.response_type == intent.IntentResponseType.ACTION_DONE assert result.response.intent is not None assert result.response.intent.slots["area"]["value"] == area_bedroom.id + assert result.response.intent.slots["area"]["text"] == area_bedroom.normalized_name # Verify only bedroom lights were targeted assert {s.entity_id for s in result.response.matched_states} == { - e.entity_id for e in area_lights["bedroom"] + e.entity_id for e in area_lights[area_bedroom.id] } assert {c.data["entity_id"][0] for c in turn_off_calls} == { - e.entity_id for e in area_lights["bedroom"] + e.entity_id for e in area_lights[area_bedroom.id] } turn_off_calls.clear() @@ -463,7 +478,8 @@ async def test_error_no_device_in_area( hass: HomeAssistant, init_components, area_registry: ar.AreaRegistry ) -> None: """Test error message when area is missing a device/entity.""" - area_registry.async_get_or_create("kitchen") + area_kitchen = area_registry.async_get_or_create("kitchen_id") + area_kitchen = area_registry.async_update(area_kitchen.id, name="kitchen") result = await conversation.async_converse( hass, "turn on missing entity in the kitchen", None, Context(), None ) @@ -482,7 +498,7 @@ async def test_error_no_domain( """Test error message when no devices/entities exist for a domain.""" # We don't have a sentence for turning on all fans - fan_domain = MatchEntity(name="domain", value="fan", text="") + fan_domain = MatchEntity(name="domain", value="fan", text="fans") recognize_result = RecognizeResult( intent=Intent("HassTurnOn"), intent_data=IntentData([]), @@ -513,7 +529,8 @@ async def test_error_no_domain_in_area( hass: HomeAssistant, init_components, area_registry: ar.AreaRegistry ) -> None: """Test error message when no devices/entities for a domain exist in an area.""" - area_registry.async_get_or_create("kitchen") + area_kitchen = area_registry.async_get_or_create("kitchen_id") + area_kitchen = area_registry.async_update(area_kitchen.id, name="kitchen") result = await conversation.async_converse( hass, "turn on the lights in the kitchen", None, Context(), None ) @@ -526,13 +543,11 @@ async def test_error_no_domain_in_area( ) -async def test_error_no_device_class( - hass: HomeAssistant, init_components, area_registry: ar.AreaRegistry -) -> None: +async def test_error_no_device_class(hass: HomeAssistant, init_components) -> None: """Test error message when no entities of a device class exist.""" # We don't have a sentence for opening all windows - window_class = MatchEntity(name="device_class", value="window", text="") + window_class = MatchEntity(name="device_class", value="window", text="windows") recognize_result = RecognizeResult( intent=Intent("HassTurnOn"), intent_data=IntentData([]), @@ -563,7 +578,8 @@ async def test_error_no_device_class_in_area( hass: HomeAssistant, init_components, area_registry: ar.AreaRegistry ) -> None: """Test error message when no entities of a device class exist in an area.""" - area_registry.async_get_or_create("bedroom") + area_bedroom = area_registry.async_get_or_create("bedroom_id") + area_bedroom = area_registry.async_update(area_bedroom.id, name="bedroom") result = await conversation.async_converse( hass, "open bedroom windows", None, Context(), None ) @@ -600,7 +616,8 @@ async def test_no_states_matched_default_error( hass: HomeAssistant, init_components, area_registry: ar.AreaRegistry ) -> None: """Test default response when no states match and slots are missing.""" - area_registry.async_get_or_create("kitchen") + area_kitchen = area_registry.async_get_or_create("kitchen_id") + area_kitchen = area_registry.async_update(area_kitchen.id, name="kitchen") with patch( "homeassistant.components.conversation.default_agent.intent.async_handle", @@ -629,9 +646,9 @@ async def test_empty_aliases( entity_registry: er.EntityRegistry, ) -> None: """Test that empty aliases are not added to slot lists.""" - area_kitchen = area_registry.async_get_or_create("kitchen") - assert area_kitchen.id is not None - area_registry.async_update(area_kitchen.id, aliases={" "}) + area_kitchen = area_registry.async_get_or_create("kitchen_id") + area_kitchen = area_registry.async_update(area_kitchen.id, name="kitchen") + area_kitchen = area_registry.async_update(area_kitchen.id, aliases={" "}) entry = MockConfigEntry() entry.add_to_hass(hass) @@ -643,11 +660,16 @@ async def test_empty_aliases( device_registry.async_update_device(kitchen_device.id, area_id=area_kitchen.id) kitchen_light = entity_registry.async_get_or_create("light", "demo", "1234") - entity_registry.async_update_entity( - kitchen_light.entity_id, device_id=kitchen_device.id, aliases={" "} + kitchen_light = entity_registry.async_update_entity( + kitchen_light.entity_id, + device_id=kitchen_device.id, + name="kitchen light", + aliases={" "}, ) hass.states.async_set( - kitchen_light.entity_id, "on", attributes={ATTR_FRIENDLY_NAME: "kitchen light"} + kitchen_light.entity_id, + "on", + attributes={ATTR_FRIENDLY_NAME: kitchen_light.name}, ) with patch( @@ -665,16 +687,16 @@ async def test_empty_aliases( assert slot_lists.keys() == {"area", "name"} areas = slot_lists["area"] assert len(areas.values) == 1 - assert areas.values[0].value_out == "kitchen" + assert areas.values[0].value_out == area_kitchen.id + assert areas.values[0].text_in.text == area_kitchen.normalized_name names = slot_lists["name"] assert len(names.values) == 1 - assert names.values[0].value_out == "kitchen light" + assert names.values[0].value_out == kitchen_light.entity_id + assert names.values[0].text_in.text == kitchen_light.name -async def test_all_domains_loaded( - hass: HomeAssistant, init_components, area_registry: ar.AreaRegistry -) -> None: +async def test_all_domains_loaded(hass: HomeAssistant, init_components) -> None: """Test that sentences for all domains are always loaded.""" # light domain is not loaded