From 2e4f892b3e0f3c1a7049fa4bb9efbfb47150da4e Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Sun, 26 Apr 2020 22:11:08 +0200 Subject: [PATCH 01/14] Improve UPnP configuration flow --- .../components/discovery/__init__.py | 2 +- homeassistant/components/upnp/__init__.py | 18 +- homeassistant/components/upnp/config_flow.py | 172 +++++++++++++++++- homeassistant/components/upnp/device.py | 23 ++- .../components/upnp/translations/en.json | 21 +-- tests/components/upnp/test_init.py | 6 +- 6 files changed, 210 insertions(+), 32 deletions(-) diff --git a/homeassistant/components/discovery/__init__.py b/homeassistant/components/discovery/__init__.py index 64816acaaf36c3..9bb342d460b07f 100644 --- a/homeassistant/components/discovery/__init__.py +++ b/homeassistant/components/discovery/__init__.py @@ -49,7 +49,6 @@ CONFIG_ENTRY_HANDLERS = { SERVICE_DAIKIN: "daikin", SERVICE_TELLDUSLIVE: "tellduslive", - SERVICE_IGD: "upnp", } SERVICE_HANDLERS = { @@ -94,6 +93,7 @@ SERVICE_HEOS, "harmony", "homekit", + SERVICE_IGD, "ikea_tradfri", "philips_hue", "sonos", diff --git a/homeassistant/components/upnp/__init__.py b/homeassistant/components/upnp/__init__.py index 4d599be88b1cbd..c937295c1baf03 100644 --- a/homeassistant/components/upnp/__init__.py +++ b/homeassistant/components/upnp/__init__.py @@ -6,6 +6,7 @@ import voluptuous as vol from homeassistant import config_entries +from homeassistant.components import ssdp from homeassistant.config_entries import ConfigEntry from homeassistant.const import EVENT_HOMEASSISTANT_STOP from homeassistant.exceptions import ConfigEntryNotReady @@ -94,17 +95,17 @@ async def async_discover_and_construct( return None if udn: - # get the discovery info with specified UDN - _LOGGER.debug("Discovery_infos: %s", discovery_infos) + # get the discovery info with specified UDN/ST filtered = [di for di in discovery_infos if di["udn"] == udn] if st: _LOGGER.debug("Filtering on ST: %s", st) filtered = [di for di in discovery_infos if di["st"] == st] if not filtered: _LOGGER.warning( - 'Wanted UPnP/IGD device with UDN "%s" not found, ' "aborting", udn + 'Wanted UPnP/IGD device with UDN "%s" not found, aborting', udn ) return None + # ensure we're always taking the latest filtered = sorted(filtered, key=itemgetter("st"), reverse=True) discovery_info = filtered[0] @@ -117,12 +118,13 @@ async def async_discover_and_construct( ) _LOGGER.info("Detected multiple UPnP/IGD devices, using: %s", device_name) - ssdp_description = discovery_info["ssdp_description"] - return await Device.async_create_device(hass, ssdp_description) + ssdp_location = discovery_info[ssdp.ATTR_SSDP_LOCATION] + return await Device.async_create_device(hass, ssdp_location) async def async_setup(hass: HomeAssistantType, config: ConfigType): """Set up UPnP component.""" + _LOGGER.debug("async_setup, config: %s", config) conf_default = CONFIG_SCHEMA({DOMAIN: {}})[DOMAIN] conf = config.get(DOMAIN, conf_default) local_ip = await hass.async_add_executor_job(get_local_ip) @@ -133,7 +135,8 @@ async def async_setup(hass: HomeAssistantType, config: ConfigType): "ports": conf.get(CONF_PORTS), } - if conf is not None: + # Only start if set up via configuration.yaml. + if DOMAIN in config: hass.async_create_task( hass.config_entries.flow.async_init( DOMAIN, context={"source": config_entries.SOURCE_IMPORT} @@ -145,6 +148,7 @@ async def async_setup(hass: HomeAssistantType, config: ConfigType): async def async_setup_entry(hass: HomeAssistantType, config_entry: ConfigEntry) -> bool: """Set up UPnP/IGD device from a config entry.""" + _LOGGER.debug("async_setup_entry, config_entry: %s", config_entry.data) domain_data = hass.data[DOMAIN] conf = domain_data["config"] @@ -160,6 +164,8 @@ async def async_setup_entry(hass: HomeAssistantType, config_entry: ConfigEntry) hass.data[DOMAIN]["devices"][device.udn] = device hass.config_entries.async_update_entry( entry=config_entry, + unique_id=device.unique_id, + title=device.name, data={**config_entry.data, "udn": device.udn, "st": device.device_type}, ) diff --git a/homeassistant/components/upnp/config_flow.py b/homeassistant/components/upnp/config_flow.py index 1601595b6a9f5d..bd59643498e584 100644 --- a/homeassistant/components/upnp/config_flow.py +++ b/homeassistant/components/upnp/config_flow.py @@ -1,10 +1,172 @@ """Config flow for UPNP.""" +from typing import Mapping, Optional +import voluptuous as vol + from homeassistant import config_entries -from homeassistant.helpers import config_entry_flow +from homeassistant.components import ssdp -from .const import DOMAIN +from .const import ( # pylint: disable=unused-import + DOMAIN, + LOGGER as _LOGGER, +) from .device import Device +from urllib.parse import urlparse -config_entry_flow.register_discovery_flow( - DOMAIN, "UPnP/IGD", Device.async_discover, config_entries.CONN_CLASS_LOCAL_POLL -) + +LOCATION = "location" +NAME = "name" +ST = "st" +UDN = "udn" + + +class UpnpFlowHandler(config_entries.ConfigFlow, domain=DOMAIN): + """Handle a UPnP/IGD config flow.""" + + VERSION = 1 + CONNECTION_CLASS = config_entries.CONN_CLASS_LOCAL_POLL + + # Paths: + # - ssdp(discovery_info) --> ssdp_confirm(None) --> ssdp_confirm({}) --> create_entry() + # - user(None): scan --> user({...}) --> create_entry() + # - import(None) --> create_entry() + + def __init__(self): + """Initialize the UPnP/IGD config flow.""" + self._data: Mapping = None + self._discoveries: Mapping = None + + async def async_step_user(self, user_input=None): + """Handle a flow start.""" + _LOGGER.debug("async_step_user: user_input: %s", user_input) + + if user_input is not None: + # Ensure wanted device was discovered. + matching_discoveries = [ + discovery + for discovery in self._discoveries + if discovery["unique_id"] == user_input["unique_id"] + ] + if not matching_discoveries: + errors = {"base": "no_devices_discovered"} + return self.async_show_form(step_id="user", errors=errors,) + + # Title/name will be updated later on. + self._data = matching_discoveries[0] + self._data[NAME] = urlparse(self._data[ssdp.ATTR_SSDP_LOCATION]).hostname + return self._async_create_entry_from_data() + + # Discover devices. + self._discoveries = await Device.async_discover(self.hass) + + # Filter discoveries which are already configured. + current_unique_ids = [ + entry.unique_id for entry in self._async_current_entries() + ] + self._discoveries = [ + discovery + for discovery in self._discoveries + if discovery["unique_id"] not in current_unique_ids + ] + + # Ensure anything to add. + if not self._discoveries: + errors = {"base": "no_devices_discovered"} + return self.async_show_form(step_id="user", errors=errors,) + + data_schema = vol.Schema( + { + vol.Required("unique_id"): vol.In( + { + discovery["unique_id"]: urlparse( + discovery[ssdp.ATTR_SSDP_LOCATION] + ).hostname + for discovery in self._discoveries + } + ), + } + ) + return self.async_show_form(step_id="user", data_schema=data_schema,) + + async def async_step_import(self, import_info: Optional[Mapping]): + """Import a new UPnP/IGD device as a config entry. + + This flow is triggered by `async_setup`. + """ + _LOGGER.debug("async_step_import: import_info: %s", import_info) + + if import_info is None: + # Landed here via configuration.yaml entry. + # Any device already added, then abort. + if self._async_current_entries(): + _LOGGER.debug("aborting, already configured") + return self.async_abort(reason="already_configured") + + # Test if import_info isn't already configured. + if import_info is not None and any( + import_info["udn"] == entry.data["udn"] + and import_info["st"] == entry.data["st"] + for entry in self._async_current_entries() + ): + return self.async_abort(reason="already_configured") + + # Discover devices. + self._discoveries = await Device.async_discover(self.hass) + + # Ensure anything to add. If not, silently abort. + if not self._discoveries: + _LOGGER.info("No UPnP devices discovered, aborting.") + return self.async_abort(reason="no_devices_found") + + # Create new config_entry. + self._data = self._discoveries[0] + return self._async_create_entry_from_data() + + async def async_step_ssdp(self, discovery_info: Mapping): + """Handle a discovered UPnP/IGD device. + + This flow is triggered by the SSDP component. It will check if the + host is already configured and delegate to the import step if not. + """ + _LOGGER.debug("async_step_ssdp: discovery_info: %s", discovery_info) + + # Ensure not already configuring/configured. + udn = discovery_info[ssdp.ATTR_UPNP_UDN] + st = discovery_info[ssdp.ATTR_SSDP_ST] # pylint: disable=invalid-name + usn = f"{udn}::{st}" + await self.async_set_unique_id(usn) + self._abort_if_unique_id_configured(updates={UDN: udn, ST: st}) + + # Store discovery. + name = discovery_info.get("friendlyName", "") + self._data = { + UDN: udn, + ST: st, + NAME: name, + } + + # Ensure user recognizable. + location = discovery_info[ssdp.ATTR_SSDP_LOCATION] + # pylint: disable=no-member # https://github.com/PyCQA/pylint/issues/3167 + self.context["title_placeholders"] = { + "name": name, + "host": urlparse(location).hostname, + } + + return await self.async_step_ssdp_confirm() + + async def async_step_ssdp_confirm(self, user_input=None): + """Confirm integration via SSDP.""" + _LOGGER.debug("async_step_ssdp_confirm: user_input: %s", user_input) + if user_input is None: + return self.async_show_form(step_id="ssdp_confirm") + + return self._async_create_entry_from_data() + + def _async_create_entry_from_data(self): + """Create an entry from own _data.""" + title = self._data["name"] + data = { + UDN: self._data["udn"], + ST: self._data["st"], + } + return self.async_create_entry(title=title, data=data) diff --git a/homeassistant/components/upnp/device.py b/homeassistant/components/upnp/device.py index 73ae06d9945039..7edf9aba1ec278 100644 --- a/homeassistant/components/upnp/device.py +++ b/homeassistant/components/upnp/device.py @@ -1,13 +1,14 @@ """Home Assistant representation of an UPnP/IGD.""" import asyncio from ipaddress import IPv4Address -from typing import Mapping +from typing import List, Mapping import aiohttp from async_upnp_client import UpnpError, UpnpFactory from async_upnp_client.aiohttp import AiohttpSessionRequester from async_upnp_client.profiles.igd import IgdDevice +from homeassistant.components import ssdp from homeassistant.helpers.aiohttp_client import async_get_clientsession from homeassistant.helpers.typing import HomeAssistantType import homeassistant.util.dt as dt_util @@ -33,7 +34,7 @@ def __init__(self, igd_device): self._mapped_ports = [] @classmethod - async def async_discover(cls, hass: HomeAssistantType): + async def async_discover(cls, hass: HomeAssistantType) -> List[Mapping]: """Discover UPnP/IGD devices.""" _LOGGER.debug("Discovering UPnP/IGD devices") local_ip = None @@ -47,8 +48,13 @@ async def async_discover(cls, hass: HomeAssistantType): # add extra info and store devices devices = [] for discovery_info in discovery_infos: - discovery_info["udn"] = discovery_info["_udn"] - discovery_info["ssdp_description"] = discovery_info["location"] + # Become more ssdp-component-discovery-like. + discovery_info[ssdp.ATTR_UPNP_UDN] = discovery_info["_udn"] + discovery_info[ssdp.ATTR_SSDP_ST] = discovery_info["st"] + discovery_info[ssdp.ATTR_SSDP_LOCATION] = discovery_info["location"] + + unique_id = f"{discovery_info[ssdp.ATTR_UPNP_UDN]}::{discovery_info[ssdp.ATTR_SSDP_ST]}" + discovery_info["unique_id"] = unique_id discovery_info["source"] = "async_upnp_client" _LOGGER.debug("Discovered device: %s", discovery_info) @@ -57,7 +63,7 @@ async def async_discover(cls, hass: HomeAssistantType): return devices @classmethod - async def async_create_device(cls, hass: HomeAssistantType, ssdp_description: str): + async def async_create_device(cls, hass: HomeAssistantType, ssdp_location: str): """Create UPnP/IGD device.""" # build async_upnp_client requester session = async_get_clientsession(hass) @@ -65,7 +71,7 @@ async def async_create_device(cls, hass: HomeAssistantType, ssdp_description: st # create async_upnp_client device factory = UpnpFactory(requester, disable_state_variable_validation=True) - upnp_device = await factory.async_create_device(ssdp_description) + upnp_device = await factory.async_create_device(ssdp_location) igd_device = IgdDevice(upnp_device, None) @@ -96,6 +102,11 @@ def device_type(self) -> str: """Get the device type.""" return self._igd_device.device_type + @property + def unique_id(self) -> str: + """Get the unique id.""" + return f"{self.udn}::{self.device_type}" + def __str__(self) -> str: """Get string representation.""" return f"IGD Device: {self.name}/{self.udn}" diff --git a/homeassistant/components/upnp/translations/en.json b/homeassistant/components/upnp/translations/en.json index 6da89c0e3d601b..503ffc56b455bd 100644 --- a/homeassistant/components/upnp/translations/en.json +++ b/homeassistant/components/upnp/translations/en.json @@ -1,26 +1,25 @@ { "config": { + "flow_title": "UPnP/IGD: {name} ({host})", "abort": { "already_configured": "UPnP/IGD is already configured", - "incomplete_device": "Ignoring incomplete UPnP device", "no_devices_discovered": "No UPnP/IGDs discovered", - "no_devices_found": "No UPnP/IGD devices found on the network.", - "no_sensors_or_port_mapping": "Enable at least sensors or port mapping", - "single_instance_allowed": "Only a single configuration of UPnP/IGD is necessary." + "no_devices_found": "No UPnP/IGD devices found on the network." }, "step": { - "confirm": { - "description": "Do you want to set up UPnP/IGD?", - "title": "UPnP/IGD" - }, "init": { "title": "UPnP/IGD" }, + "ssdp_confirm": { + "data": { + "name": "Name" + }, + "title": "UPnP/IGD via SSDP Confirm DEBUG", + "description": "Do you want to set up the UPnP/IGD device?" + }, "user": { "data": { - "enable_port_mapping": "Enable port mapping for Home Assistant", - "enable_sensors": "Add traffic sensors", - "igd": "UPnP/IGD" + "unique_id": "Host" }, "title": "Configuration options" } diff --git a/tests/components/upnp/test_init.py b/tests/components/upnp/test_init.py index 464ccc90675d5c..f365db8117ad12 100644 --- a/tests/components/upnp/test_init.py +++ b/tests/components/upnp/test_init.py @@ -24,7 +24,7 @@ def __init__(self, udn): self.removed_port_mappings = [] @classmethod - async def async_create_device(cls, hass, ssdp_description): + async def async_create_device(cls, hass, ssdp_location): """Return self.""" return cls("UDN") @@ -83,7 +83,7 @@ async def test_async_setup_entry_default(hass): # mock homeassistant.components.upnp.device.Device mock_device = MockDevice(udn) discovery_infos = [ - {"udn": udn, "ssdp_description": "http://192.168.1.1/desc.xml"} + {"udn": udn, "ssdp_location": "http://192.168.1.1/desc.xml"} ] create_device.return_value = mock_coro(return_value=mock_device) @@ -125,7 +125,7 @@ async def test_async_setup_entry_port_mapping(hass): mock_device = MockDevice(udn) discovery_infos = [ - {"udn": udn, "ssdp_description": "http://192.168.1.1/desc.xml"} + {"udn": udn, "ssdp_location": "http://192.168.1.1/desc.xml"} ] create_device.return_value = mock_coro(return_value=mock_device) From 27055c14880ac680610086c2d37c080344ce4411 Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Mon, 27 Apr 2020 22:59:16 +0200 Subject: [PATCH 02/14] Changes after review --- homeassistant/components/upnp/__init__.py | 53 ++++++----- homeassistant/components/upnp/config_flow.py | 88 ++++++++++--------- homeassistant/components/upnp/const.py | 7 ++ homeassistant/components/upnp/device.py | 18 ++-- homeassistant/components/upnp/manifest.json | 10 ++- .../components/upnp/translations/en.json | 4 +- homeassistant/generated/ssdp.py | 8 ++ 7 files changed, 115 insertions(+), 73 deletions(-) diff --git a/homeassistant/components/upnp/__init__.py b/homeassistant/components/upnp/__init__.py index c937295c1baf03..fe3df4ea11089b 100644 --- a/homeassistant/components/upnp/__init__.py +++ b/homeassistant/components/upnp/__init__.py @@ -6,7 +6,6 @@ import voluptuous as vol from homeassistant import config_entries -from homeassistant.components import ssdp from homeassistant.config_entries import ConfigEntry from homeassistant.const import EVENT_HOMEASSISTANT_STOP from homeassistant.exceptions import ConfigEntryNotReady @@ -15,12 +14,18 @@ from homeassistant.util import get_local_ip from .const import ( + CONFIG_ENTRY_ST, + CONFIG_ENTRY_UDN, CONF_ENABLE_PORT_MAPPING, CONF_ENABLE_SENSORS, CONF_HASS, CONF_LOCAL_IP, CONF_PORTS, + DISCOVERY_ST, + DISCOVERY_UDN, + DISCOVERY_USN, DOMAIN, + DISCOVERY_LOCATION, LOGGER as _LOGGER, ) from .device import Device @@ -95,31 +100,30 @@ async def async_discover_and_construct( return None if udn: - # get the discovery info with specified UDN/ST - filtered = [di for di in discovery_infos if di["udn"] == udn] + # Get the discovery info with specified UDN/ST. + filtered = [di for di in discovery_infos if di[DISCOVERY_UDN] == udn] if st: - _LOGGER.debug("Filtering on ST: %s", st) - filtered = [di for di in discovery_infos if di["st"] == st] + filtered = [di for di in discovery_infos if di[DISCOVERY_ST] == st] if not filtered: _LOGGER.warning( 'Wanted UPnP/IGD device with UDN "%s" not found, aborting', udn ) return None - # ensure we're always taking the latest - filtered = sorted(filtered, key=itemgetter("st"), reverse=True) + # Ensure we're always taking the latest, if we filtered only on UDN. + filtered = sorted(filtered, key=itemgetter(DISCOVERY_ST), reverse=True) discovery_info = filtered[0] else: - # get the first/any + # Get the first/any. discovery_info = discovery_infos[0] if len(discovery_infos) > 1: device_name = discovery_info.get( - "usn", discovery_info.get("ssdp_description", "") + DISCOVERY_USN, discovery_info.get(DISCOVERY_LOCATION, "") ) _LOGGER.info("Detected multiple UPnP/IGD devices, using: %s", device_name) - ssdp_location = discovery_info[ssdp.ATTR_SSDP_LOCATION] - return await Device.async_create_device(hass, ssdp_location) + location = discovery_info[DISCOVERY_LOCATION] + return await Device.async_create_device(hass, location) async def async_setup(hass: HomeAssistantType, config: ConfigType): @@ -153,21 +157,28 @@ async def async_setup_entry(hass: HomeAssistantType, config_entry: ConfigEntry) conf = domain_data["config"] # discover and construct - udn = config_entry.data.get("udn") - st = config_entry.data.get("st") # pylint: disable=invalid-name + udn = config_entry.data.get(CONFIG_ENTRY_UDN) + st = config_entry.data.get(CONFIG_ENTRY_ST) # pylint: disable=invalid-name device = await async_discover_and_construct(hass, udn, st) if not device: _LOGGER.info("Unable to create UPnP/IGD, aborting") raise ConfigEntryNotReady - # 'register'/save UDN + ST + # 'register'/save device hass.data[DOMAIN]["devices"][device.udn] = device - hass.config_entries.async_update_entry( - entry=config_entry, - unique_id=device.unique_id, - title=device.name, - data={**config_entry.data, "udn": device.udn, "st": device.device_type}, - ) + + # Ensure entry has proper unique_id. + if config_entry.unique_id != device.unique_id: + hass.config_entries.async_update_entry( + entry=config_entry, unique_id=device.unique_id, + ) + + # Ensure entry has a title. + # Only via SSDP-discovery the title can be set directly. + if not config_entry.title: + hass.config_entries.async_update_entry( + entry=config_entry, title=device.name, + ) # create device registry entry device_registry = await dr.async_get_registry(hass) @@ -217,7 +228,7 @@ async def async_unload_entry( hass: HomeAssistantType, config_entry: ConfigEntry ) -> bool: """Unload a UPnP/IGD device from a config entry.""" - udn = config_entry.data["udn"] + udn = config_entry.data[CONFIG_ENTRY_UDN] device = hass.data[DOMAIN]["devices"][udn] # remove port mapping diff --git a/homeassistant/components/upnp/config_flow.py b/homeassistant/components/upnp/config_flow.py index bd59643498e584..7d68dea5f00f40 100644 --- a/homeassistant/components/upnp/config_flow.py +++ b/homeassistant/components/upnp/config_flow.py @@ -6,6 +6,13 @@ from homeassistant.components import ssdp from .const import ( # pylint: disable=unused-import + CONFIG_ENTRY_ST, + CONFIG_ENTRY_UDN, + DISCOVERY_LOCATION, + DISCOVERY_NAME, + DISCOVERY_ST, + DISCOVERY_UDN, + DISCOVERY_USN, DOMAIN, LOGGER as _LOGGER, ) @@ -13,12 +20,6 @@ from urllib.parse import urlparse -LOCATION = "location" -NAME = "name" -ST = "st" -UDN = "udn" - - class UpnpFlowHandler(config_entries.ConfigFlow, domain=DOMAIN): """Handle a UPnP/IGD config flow.""" @@ -32,53 +33,51 @@ class UpnpFlowHandler(config_entries.ConfigFlow, domain=DOMAIN): def __init__(self): """Initialize the UPnP/IGD config flow.""" - self._data: Mapping = None self._discoveries: Mapping = None async def async_step_user(self, user_input=None): """Handle a flow start.""" _LOGGER.debug("async_step_user: user_input: %s", user_input) + # This uses DISCOVERY_USN as the unique_id for user input. if user_input is not None: # Ensure wanted device was discovered. matching_discoveries = [ discovery for discovery in self._discoveries - if discovery["unique_id"] == user_input["unique_id"] + if discovery[DISCOVERY_USN] == user_input["unique_id"] ] if not matching_discoveries: - errors = {"base": "no_devices_discovered"} - return self.async_show_form(step_id="user", errors=errors,) + return self.async_abort(reason="no_devices_discovered") - # Title/name will be updated later on. - self._data = matching_discoveries[0] - self._data[NAME] = urlparse(self._data[ssdp.ATTR_SSDP_LOCATION]).hostname - return self._async_create_entry_from_data() + # Title/name will be updated in `async_setup_entry`. + discovery = matching_discoveries[0] + self.async_set_unique_id(discovery[DISCOVERY_USN], raise_on_progress=False) + return self._async_create_entry_from_data(discovery) # Discover devices. - self._discoveries = await Device.async_discover(self.hass) + discoveries = await Device.async_discover(self.hass) # Filter discoveries which are already configured. - current_unique_ids = [ + current_unique_ids = { entry.unique_id for entry in self._async_current_entries() - ] + } self._discoveries = [ discovery - for discovery in self._discoveries - if discovery["unique_id"] not in current_unique_ids + for discovery in discoveries + if discovery[DISCOVERY_USN] not in current_unique_ids ] # Ensure anything to add. if not self._discoveries: - errors = {"base": "no_devices_discovered"} - return self.async_show_form(step_id="user", errors=errors,) + return self.async_abort(reason="no_devices_found") data_schema = vol.Schema( { vol.Required("unique_id"): vol.In( { - discovery["unique_id"]: urlparse( - discovery[ssdp.ATTR_SSDP_LOCATION] + discovery[DISCOVERY_USN]: urlparse( + discovery[DISCOVERY_LOCATION] ).hostname for discovery in self._discoveries } @@ -103,11 +102,18 @@ async def async_step_import(self, import_info: Optional[Mapping]): # Test if import_info isn't already configured. if import_info is not None and any( - import_info["udn"] == entry.data["udn"] - and import_info["st"] == entry.data["st"] + import_info["udn"] == entry.data[CONFIG_ENTRY_UDN] + and import_info["st"] == entry.data[CONFIG_ENTRY_ST] for entry in self._async_current_entries() ): - return self.async_abort(reason="already_configured") + usn = f"{import_info['udn']}::{import_info['st']}" + await self.async_set_unique_id(usn, raise_on_progress=False) + self._abort_if_unique_id_configured( + updates={ + DISCOVERY_UDN: import_info["udn"], + DISCOVERY_ST: import_info["st"], + } + ) # Discover devices. self._discoveries = await Device.async_discover(self.hass) @@ -118,8 +124,8 @@ async def async_step_import(self, import_info: Optional[Mapping]): return self.async_abort(reason="no_devices_found") # Create new config_entry. - self._data = self._discoveries[0] - return self._async_create_entry_from_data() + discovery = self._discoveries[0] + return self._async_create_entry_from_data(discovery) async def async_step_ssdp(self, discovery_info: Mapping): """Handle a discovered UPnP/IGD device. @@ -132,17 +138,18 @@ async def async_step_ssdp(self, discovery_info: Mapping): # Ensure not already configuring/configured. udn = discovery_info[ssdp.ATTR_UPNP_UDN] st = discovery_info[ssdp.ATTR_SSDP_ST] # pylint: disable=invalid-name - usn = f"{udn}::{st}" - await self.async_set_unique_id(usn) - self._abort_if_unique_id_configured(updates={UDN: udn, ST: st}) + unique_id = f"{udn}::{st}" + await self.async_set_unique_id(unique_id) + self._abort_if_unique_id_configured() # Store discovery. name = discovery_info.get("friendlyName", "") - self._data = { - UDN: udn, - ST: st, - NAME: name, + discovery = { + DISCOVERY_UDN: udn, + DISCOVERY_ST: st, + DISCOVERY_NAME: name, } + self._discoveries = [discovery] # Ensure user recognizable. location = discovery_info[ssdp.ATTR_SSDP_LOCATION] @@ -160,13 +167,14 @@ async def async_step_ssdp_confirm(self, user_input=None): if user_input is None: return self.async_show_form(step_id="ssdp_confirm") - return self._async_create_entry_from_data() + discovery = self._discoveries[0] + return self._async_create_entry_from_data(discovery) - def _async_create_entry_from_data(self): + def _async_create_entry_from_data(self, discovery): """Create an entry from own _data.""" - title = self._data["name"] + title = discovery.get(DISCOVERY_NAME, "") data = { - UDN: self._data["udn"], - ST: self._data["st"], + CONFIG_ENTRY_UDN: discovery[DISCOVERY_UDN], + CONFIG_ENTRY_ST: discovery[DISCOVERY_ST], } return self.async_create_entry(title=title, data=data) diff --git a/homeassistant/components/upnp/const.py b/homeassistant/components/upnp/const.py index 80b5b718bbb622..ee3b5873310bc5 100644 --- a/homeassistant/components/upnp/const.py +++ b/homeassistant/components/upnp/const.py @@ -20,3 +20,10 @@ DATA_RATE_PACKETS_PER_SECOND = f"{DATA_PACKETS}/{TIME_SECONDS}" KIBIBYTE = 1024 UPDATE_INTERVAL = timedelta(seconds=30) +DISCOVERY_NAME = "name" +DISCOVERY_LOCATION = "location" +DISCOVERY_ST = "st" +DISCOVERY_UDN = "udn" +DISCOVERY_USN = "usn" +CONFIG_ENTRY_UDN = "udn" +CONFIG_ENTRY_ST = "st" diff --git a/homeassistant/components/upnp/device.py b/homeassistant/components/upnp/device.py index 7edf9aba1ec278..ec7753bce87f07 100644 --- a/homeassistant/components/upnp/device.py +++ b/homeassistant/components/upnp/device.py @@ -8,7 +8,6 @@ from async_upnp_client.aiohttp import AiohttpSessionRequester from async_upnp_client.profiles.igd import IgdDevice -from homeassistant.components import ssdp from homeassistant.helpers.aiohttp_client import async_get_clientsession from homeassistant.helpers.typing import HomeAssistantType import homeassistant.util.dt as dt_util @@ -17,6 +16,10 @@ BYTES_RECEIVED, BYTES_SENT, CONF_LOCAL_IP, + DISCOVERY_LOCATION, + DISCOVERY_ST, + DISCOVERY_UDN, + DISCOVERY_USN, DOMAIN, LOGGER as _LOGGER, PACKETS_RECEIVED, @@ -48,14 +51,11 @@ async def async_discover(cls, hass: HomeAssistantType) -> List[Mapping]: # add extra info and store devices devices = [] for discovery_info in discovery_infos: - # Become more ssdp-component-discovery-like. - discovery_info[ssdp.ATTR_UPNP_UDN] = discovery_info["_udn"] - discovery_info[ssdp.ATTR_SSDP_ST] = discovery_info["st"] - discovery_info[ssdp.ATTR_SSDP_LOCATION] = discovery_info["location"] - - unique_id = f"{discovery_info[ssdp.ATTR_UPNP_UDN]}::{discovery_info[ssdp.ATTR_SSDP_ST]}" - discovery_info["unique_id"] = unique_id - discovery_info["source"] = "async_upnp_client" + discovery_info[DISCOVERY_UDN] = discovery_info["_udn"] + discovery_info[DISCOVERY_ST] = discovery_info["st"] + discovery_info[DISCOVERY_LOCATION] = discovery_info["location"] + usn = f"{discovery_info[DISCOVERY_UDN]}::{discovery_info[DISCOVERY_ST]}" + discovery_info[DISCOVERY_USN] = usn _LOGGER.debug("Discovered device: %s", discovery_info) devices.append(discovery_info) diff --git a/homeassistant/components/upnp/manifest.json b/homeassistant/components/upnp/manifest.json index 2f6e5de5884e61..e3b30cec9a487a 100644 --- a/homeassistant/components/upnp/manifest.json +++ b/homeassistant/components/upnp/manifest.json @@ -5,5 +5,13 @@ "documentation": "https://www.home-assistant.io/integrations/upnp", "requirements": ["async-upnp-client==0.14.13"], "dependencies": [], - "codeowners": ["@StevenLooman"] + "codeowners": ["@StevenLooman"], + "ssdp": [ + { + "st": "urn:schemas-upnp-org:device:InternetGatewayDevice:1" + }, + { + "st": "urn:schemas-upnp-org:device:InternetGatewayDevice:2" + } + ] } diff --git a/homeassistant/components/upnp/translations/en.json b/homeassistant/components/upnp/translations/en.json index 503ffc56b455bd..2b79fee42d8529 100644 --- a/homeassistant/components/upnp/translations/en.json +++ b/homeassistant/components/upnp/translations/en.json @@ -14,8 +14,8 @@ "data": { "name": "Name" }, - "title": "UPnP/IGD via SSDP Confirm DEBUG", - "description": "Do you want to set up the UPnP/IGD device?" + "title": "UPnP/IGD", + "description": "Do you want to set up this UPnP/IGD device?" }, "user": { "data": { diff --git a/homeassistant/generated/ssdp.py b/homeassistant/generated/ssdp.py index 5dbef37d9bf169..f46ba1611a8d1f 100644 --- a/homeassistant/generated/ssdp.py +++ b/homeassistant/generated/ssdp.py @@ -81,6 +81,14 @@ "manufacturer": "Synology" } ], + "upnp": [ + { + "st": "urn:schemas-upnp-org:device:InternetGatewayDevice:1" + }, + { + "st": "urn:schemas-upnp-org:device:InternetGatewayDevice:2" + } + ], "wemo": [ { "manufacturer": "Belkin International Inc." From a39a3e189b0677e284157ffe2967f3c9699ef1af Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Mon, 27 Apr 2020 23:22:47 +0200 Subject: [PATCH 03/14] Linting etc --- homeassistant/components/upnp/__init__.py | 6 +++--- homeassistant/components/upnp/config_flow.py | 3 ++- tests/components/upnp/test_init.py | 8 ++------ 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/homeassistant/components/upnp/__init__.py b/homeassistant/components/upnp/__init__.py index fe3df4ea11089b..81e77ac141a914 100644 --- a/homeassistant/components/upnp/__init__.py +++ b/homeassistant/components/upnp/__init__.py @@ -14,18 +14,18 @@ from homeassistant.util import get_local_ip from .const import ( - CONFIG_ENTRY_ST, - CONFIG_ENTRY_UDN, CONF_ENABLE_PORT_MAPPING, CONF_ENABLE_SENSORS, CONF_HASS, CONF_LOCAL_IP, CONF_PORTS, + CONFIG_ENTRY_ST, + CONFIG_ENTRY_UDN, + DISCOVERY_LOCATION, DISCOVERY_ST, DISCOVERY_UDN, DISCOVERY_USN, DOMAIN, - DISCOVERY_LOCATION, LOGGER as _LOGGER, ) from .device import Device diff --git a/homeassistant/components/upnp/config_flow.py b/homeassistant/components/upnp/config_flow.py index 7d68dea5f00f40..5c6af606072d91 100644 --- a/homeassistant/components/upnp/config_flow.py +++ b/homeassistant/components/upnp/config_flow.py @@ -1,5 +1,7 @@ """Config flow for UPNP.""" from typing import Mapping, Optional +from urllib.parse import urlparse + import voluptuous as vol from homeassistant import config_entries @@ -17,7 +19,6 @@ LOGGER as _LOGGER, ) from .device import Device -from urllib.parse import urlparse class UpnpFlowHandler(config_entries.ConfigFlow, domain=DOMAIN): diff --git a/tests/components/upnp/test_init.py b/tests/components/upnp/test_init.py index f365db8117ad12..d0b47f0707bded 100644 --- a/tests/components/upnp/test_init.py +++ b/tests/components/upnp/test_init.py @@ -82,9 +82,7 @@ async def test_async_setup_entry_default(hass): # mock homeassistant.components.upnp.device.Device mock_device = MockDevice(udn) - discovery_infos = [ - {"udn": udn, "ssdp_location": "http://192.168.1.1/desc.xml"} - ] + discovery_infos = [{"udn": udn, "ssdp_location": "http://192.168.1.1/desc.xml"}] create_device.return_value = mock_coro(return_value=mock_device) async_discover.return_value = mock_coro(return_value=discovery_infos) @@ -124,9 +122,7 @@ async def test_async_setup_entry_port_mapping(hass): await hass.async_block_till_done() mock_device = MockDevice(udn) - discovery_infos = [ - {"udn": udn, "ssdp_location": "http://192.168.1.1/desc.xml"} - ] + discovery_infos = [{"udn": udn, "ssdp_location": "http://192.168.1.1/desc.xml"}] create_device.return_value = mock_coro(return_value=mock_device) async_discover.return_value = mock_coro(return_value=discovery_infos) From 00dcfdb3ce01ade243d9bad613b96677125c987b Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Tue, 28 Apr 2020 22:16:31 +0200 Subject: [PATCH 04/14] Changes after review @balloob --- homeassistant/components/upnp/__init__.py | 7 ---- homeassistant/components/upnp/config_flow.py | 34 +++++++++---------- .../components/upnp/translations/en.json | 2 +- 3 files changed, 17 insertions(+), 26 deletions(-) diff --git a/homeassistant/components/upnp/__init__.py b/homeassistant/components/upnp/__init__.py index 81e77ac141a914..5a186ecd11b3d6 100644 --- a/homeassistant/components/upnp/__init__.py +++ b/homeassistant/components/upnp/__init__.py @@ -173,13 +173,6 @@ async def async_setup_entry(hass: HomeAssistantType, config_entry: ConfigEntry) entry=config_entry, unique_id=device.unique_id, ) - # Ensure entry has a title. - # Only via SSDP-discovery the title can be set directly. - if not config_entry.title: - hass.config_entries.async_update_entry( - entry=config_entry, title=device.name, - ) - # create device registry entry device_registry = await dr.async_get_registry(hass) device_registry.async_get_or_create( diff --git a/homeassistant/components/upnp/config_flow.py b/homeassistant/components/upnp/config_flow.py index 5c6af606072d91..7a5f8d2314772b 100644 --- a/homeassistant/components/upnp/config_flow.py +++ b/homeassistant/components/upnp/config_flow.py @@ -51,10 +51,9 @@ async def async_step_user(self, user_input=None): if not matching_discoveries: return self.async_abort(reason="no_devices_discovered") - # Title/name will be updated in `async_setup_entry`. discovery = matching_discoveries[0] - self.async_set_unique_id(discovery[DISCOVERY_USN], raise_on_progress=False) - return self._async_create_entry_from_data(discovery) + await self.async_set_unique_id(discovery[DISCOVERY_USN], raise_on_progress=False) + return await self._async_create_entry_from_data(discovery) # Discover devices. discoveries = await Device.async_discover(self.hass) @@ -90,7 +89,9 @@ async def async_step_user(self, user_input=None): async def async_step_import(self, import_info: Optional[Mapping]): """Import a new UPnP/IGD device as a config entry. - This flow is triggered by `async_setup`. + This flow is triggered by `async_setup`. If no device has been + configured before, find any device and create a config_entry for it. + Otherwise, do nothing. """ _LOGGER.debug("async_step_import: import_info: %s", import_info) @@ -107,14 +108,7 @@ async def async_step_import(self, import_info: Optional[Mapping]): and import_info["st"] == entry.data[CONFIG_ENTRY_ST] for entry in self._async_current_entries() ): - usn = f"{import_info['udn']}::{import_info['st']}" - await self.async_set_unique_id(usn, raise_on_progress=False) - self._abort_if_unique_id_configured( - updates={ - DISCOVERY_UDN: import_info["udn"], - DISCOVERY_ST: import_info["st"], - } - ) + return self.async_abort(reason="already_configured") # Discover devices. self._discoveries = await Device.async_discover(self.hass) @@ -124,9 +118,8 @@ async def async_step_import(self, import_info: Optional[Mapping]): _LOGGER.info("No UPnP devices discovered, aborting.") return self.async_abort(reason="no_devices_found") - # Create new config_entry. discovery = self._discoveries[0] - return self._async_create_entry_from_data(discovery) + return await self._async_create_entry_from_data(discovery) async def async_step_ssdp(self, discovery_info: Mapping): """Handle a discovered UPnP/IGD device. @@ -139,8 +132,8 @@ async def async_step_ssdp(self, discovery_info: Mapping): # Ensure not already configuring/configured. udn = discovery_info[ssdp.ATTR_UPNP_UDN] st = discovery_info[ssdp.ATTR_SSDP_ST] # pylint: disable=invalid-name - unique_id = f"{udn}::{st}" - await self.async_set_unique_id(unique_id) + usn = f"{udn}::{st}" + await self.async_set_unique_id(usn) self._abort_if_unique_id_configured() # Store discovery. @@ -169,10 +162,15 @@ async def async_step_ssdp_confirm(self, user_input=None): return self.async_show_form(step_id="ssdp_confirm") discovery = self._discoveries[0] - return self._async_create_entry_from_data(discovery) + return await self._async_create_entry_from_data(discovery) - def _async_create_entry_from_data(self, discovery): + async def _async_create_entry_from_data(self, discovery): """Create an entry from own _data.""" + # Get name from device, if not found already. + if DISCOVERY_NAME not in discovery and DISCOVERY_LOCATION in discovery: + device = await Device.async_create_device(self.hass, discovery[DISCOVERY_LOCATION]) + discovery[DISCOVERY_NAME] = device.name + title = discovery.get(DISCOVERY_NAME, "") data = { CONFIG_ENTRY_UDN: discovery[DISCOVERY_UDN], diff --git a/homeassistant/components/upnp/translations/en.json b/homeassistant/components/upnp/translations/en.json index 2b79fee42d8529..b5e1dff17152b8 100644 --- a/homeassistant/components/upnp/translations/en.json +++ b/homeassistant/components/upnp/translations/en.json @@ -1,6 +1,6 @@ { "config": { - "flow_title": "UPnP/IGD: {name} ({host})", + "flow_title": "{name} ({host})", "abort": { "already_configured": "UPnP/IGD is already configured", "no_devices_discovered": "No UPnP/IGDs discovered", From 9b2d6ade5b97e20dba16bdcaffbc74dd1f02aa71 Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Wed, 29 Apr 2020 19:55:43 +0200 Subject: [PATCH 05/14] Linting etc --- homeassistant/components/upnp/config_flow.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/homeassistant/components/upnp/config_flow.py b/homeassistant/components/upnp/config_flow.py index 7a5f8d2314772b..d9614d880f9ab6 100644 --- a/homeassistant/components/upnp/config_flow.py +++ b/homeassistant/components/upnp/config_flow.py @@ -52,7 +52,9 @@ async def async_step_user(self, user_input=None): return self.async_abort(reason="no_devices_discovered") discovery = matching_discoveries[0] - await self.async_set_unique_id(discovery[DISCOVERY_USN], raise_on_progress=False) + await self.async_set_unique_id( + discovery[DISCOVERY_USN], raise_on_progress=False + ) return await self._async_create_entry_from_data(discovery) # Discover devices. @@ -168,7 +170,9 @@ async def _async_create_entry_from_data(self, discovery): """Create an entry from own _data.""" # Get name from device, if not found already. if DISCOVERY_NAME not in discovery and DISCOVERY_LOCATION in discovery: - device = await Device.async_create_device(self.hass, discovery[DISCOVERY_LOCATION]) + device = await Device.async_create_device( + self.hass, discovery[DISCOVERY_LOCATION] + ) discovery[DISCOVERY_NAME] = device.name title = discovery.get(DISCOVERY_NAME, "") From 7cba66281c09906d79e2d5b5da72182573f4e4cb Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Thu, 30 Apr 2020 17:56:58 +0200 Subject: [PATCH 06/14] Fix upnp unit tests --- tests/components/upnp/test_init.py | 35 +++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/tests/components/upnp/test_init.py b/tests/components/upnp/test_init.py index d0b47f0707bded..6d359e9fb8b4c8 100644 --- a/tests/components/upnp/test_init.py +++ b/tests/components/upnp/test_init.py @@ -5,6 +5,11 @@ from asynctest import patch from homeassistant.components import upnp +from homeassistant.components.upnp.const import ( + DISCOVERY_LOCATION, + DISCOVERY_ST, + DISCOVERY_UDN, +) from homeassistant.components.upnp.device import Device from homeassistant.const import EVENT_HOMEASSISTANT_STOP from homeassistant.setup import async_setup_component @@ -69,20 +74,29 @@ async def _async_delete_port_mapping(self, external_port: int) -> None: async def test_async_setup_entry_default(hass): """Test async_setup_entry.""" udn = "uuid:device_1" - entry = MockConfigEntry(domain=upnp.DOMAIN) + st = MockDevice.device_type + entry = MockConfigEntry(domain=upnp.DOMAIN, data={"udn": udn, "st": st}) config = { # no upnp } with patch.object(Device, "async_create_device") as create_device, patch.object( - Device, "async_discover", return_value=mock_coro([]) + Device, "async_discover" ) as async_discover: + async_discover.return_value = mock_coro(return_value=[]) + await async_setup_component(hass, "upnp", config) await hass.async_block_till_done() # mock homeassistant.components.upnp.device.Device mock_device = MockDevice(udn) - discovery_infos = [{"udn": udn, "ssdp_location": "http://192.168.1.1/desc.xml"}] + discovery_infos = [ + { + DISCOVERY_UDN: udn, + DISCOVERY_ST: st, + DISCOVERY_LOCATION: "http://192.168.1.1/desc.xml", + } + ] create_device.return_value = mock_coro(return_value=mock_device) async_discover.return_value = mock_coro(return_value=discovery_infos) @@ -104,7 +118,8 @@ async def test_async_setup_entry_port_mapping(hass): """Test async_setup_entry.""" # pylint: disable=invalid-name udn = "uuid:device_1" - entry = MockConfigEntry(domain=upnp.DOMAIN) + st = MockDevice.device_type + entry = MockConfigEntry(domain=upnp.DOMAIN, data={"udn": udn, "st": st}) config = { "http": {}, @@ -115,14 +130,22 @@ async def test_async_setup_entry_port_mapping(hass): }, } with patch.object(Device, "async_create_device") as create_device, patch.object( - Device, "async_discover", return_value=mock_coro([]) + Device, "async_discover" ) as async_discover: + async_discover.return_value = mock_coro(return_value=[]) + await async_setup_component(hass, "http", config) await async_setup_component(hass, "upnp", config) await hass.async_block_till_done() mock_device = MockDevice(udn) - discovery_infos = [{"udn": udn, "ssdp_location": "http://192.168.1.1/desc.xml"}] + discovery_infos = [ + { + DISCOVERY_UDN: udn, + DISCOVERY_ST: st, + DISCOVERY_LOCATION: "http://192.168.1.1/desc.xml", + } + ] create_device.return_value = mock_coro(return_value=mock_device) async_discover.return_value = mock_coro(return_value=discovery_infos) From bfb247a5b6e3f7a5055b39f49cc522f5cf088650 Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Fri, 1 May 2020 22:11:59 +0200 Subject: [PATCH 07/14] More improvements after review --- homeassistant/components/upnp/config_flow.py | 38 ++++++++++--------- homeassistant/components/upnp/strings.json | 22 +++++------ .../components/upnp/translations/en.json | 7 +--- 3 files changed, 33 insertions(+), 34 deletions(-) diff --git a/homeassistant/components/upnp/config_flow.py b/homeassistant/components/upnp/config_flow.py index d9614d880f9ab6..601197ffaa1f9c 100644 --- a/homeassistant/components/upnp/config_flow.py +++ b/homeassistant/components/upnp/config_flow.py @@ -1,6 +1,5 @@ """Config flow for UPNP.""" from typing import Mapping, Optional -from urllib.parse import urlparse import voluptuous as vol @@ -39,14 +38,14 @@ def __init__(self): async def async_step_user(self, user_input=None): """Handle a flow start.""" _LOGGER.debug("async_step_user: user_input: %s", user_input) - # This uses DISCOVERY_USN as the unique_id for user input. + # This uses DISCOVERY_USN as the identifier for the device. if user_input is not None: # Ensure wanted device was discovered. matching_discoveries = [ discovery for discovery in self._discoveries - if discovery[DISCOVERY_USN] == user_input["unique_id"] + if discovery[DISCOVERY_USN] == user_input["usn"] ] if not matching_discoveries: return self.async_abort(reason="no_devices_discovered") @@ -60,14 +59,15 @@ async def async_step_user(self, user_input=None): # Discover devices. discoveries = await Device.async_discover(self.hass) - # Filter discoveries which are already configured. - current_unique_ids = { - entry.unique_id for entry in self._async_current_entries() - } + # Store discoveries which have not been configured, add name for each discovery. + current_usns = {entry.unique_id for entry in self._async_current_entries()} self._discoveries = [ - discovery + { + **discovery, + DISCOVERY_NAME: await self._async_get_name_for_discovery(discovery), + } for discovery in discoveries - if discovery[DISCOVERY_USN] not in current_unique_ids + if discovery[DISCOVERY_USN] not in current_usns ] # Ensure anything to add. @@ -76,11 +76,9 @@ async def async_step_user(self, user_input=None): data_schema = vol.Schema( { - vol.Required("unique_id"): vol.In( + vol.Required("usn"): vol.In( { - discovery[DISCOVERY_USN]: urlparse( - discovery[DISCOVERY_LOCATION] - ).hostname + discovery[DISCOVERY_USN]: discovery[DISCOVERY_NAME] for discovery in self._discoveries } ), @@ -148,11 +146,9 @@ async def async_step_ssdp(self, discovery_info: Mapping): self._discoveries = [discovery] # Ensure user recognizable. - location = discovery_info[ssdp.ATTR_SSDP_LOCATION] # pylint: disable=no-member # https://github.com/PyCQA/pylint/issues/3167 self.context["title_placeholders"] = { "name": name, - "host": urlparse(location).hostname, } return await self.async_step_ssdp_confirm() @@ -170,10 +166,9 @@ async def _async_create_entry_from_data(self, discovery): """Create an entry from own _data.""" # Get name from device, if not found already. if DISCOVERY_NAME not in discovery and DISCOVERY_LOCATION in discovery: - device = await Device.async_create_device( - self.hass, discovery[DISCOVERY_LOCATION] + discovery[DISCOVERY_NAME] = await self._async_get_name_for_discovery( + discovery ) - discovery[DISCOVERY_NAME] = device.name title = discovery.get(DISCOVERY_NAME, "") data = { @@ -181,3 +176,10 @@ async def _async_create_entry_from_data(self, discovery): CONFIG_ENTRY_ST: discovery[DISCOVERY_ST], } return self.async_create_entry(title=title, data=data) + + async def _async_get_name_for_discovery(self, discovery): + """Get the name of the device from a discovery.""" + device = await Device.async_create_device( + self.hass, discovery[DISCOVERY_LOCATION] + ) + return device.name diff --git a/homeassistant/components/upnp/strings.json b/homeassistant/components/upnp/strings.json index 5ad90b2c0cb92a..b986a4cdb3009b 100644 --- a/homeassistant/components/upnp/strings.json +++ b/homeassistant/components/upnp/strings.json @@ -1,25 +1,25 @@ { "config": { + "flow_title": "UPnP/IGD: {name}", "step": { - "confirm": { - "description": "Do you want to set up UPnP/IGD?" + "init": { + "title": "UPnP/IGD" + }, + "ssdp_confirm": { + "title": "UPnP/IGD", + "description": "Do you want to set up this UPnP/IGD device?" }, "user": { - "title": "Configuration options", "data": { - "enable_port_mapping": "Enable port mapping for Home Assistant", - "enable_sensors": "Add traffic sensors", - "igd": "UPnP/IGD" - } + "usn": "Device" + }, + "title": "Configuration options" } }, "abort": { "already_configured": "UPnP/IGD is already configured", - "incomplete_device": "Ignoring incomplete UPnP device", "no_devices_discovered": "No UPnP/IGDs discovered", - "no_devices_found": "No UPnP/IGD devices found on the network.", - "no_sensors_or_port_mapping": "Enable at least sensors or port mapping", - "single_instance_allowed": "Only a single configuration of UPnP/IGD is necessary." + "no_devices_found": "No UPnP/IGD devices found on the network." } } } diff --git a/homeassistant/components/upnp/translations/en.json b/homeassistant/components/upnp/translations/en.json index b5e1dff17152b8..da4b5e80ce1005 100644 --- a/homeassistant/components/upnp/translations/en.json +++ b/homeassistant/components/upnp/translations/en.json @@ -1,6 +1,6 @@ { "config": { - "flow_title": "{name} ({host})", + "flow_title": "UPnP/IGD: {name}", "abort": { "already_configured": "UPnP/IGD is already configured", "no_devices_discovered": "No UPnP/IGDs discovered", @@ -11,15 +11,12 @@ "title": "UPnP/IGD" }, "ssdp_confirm": { - "data": { - "name": "Name" - }, "title": "UPnP/IGD", "description": "Do you want to set up this UPnP/IGD device?" }, "user": { "data": { - "unique_id": "Host" + "usn": "Device" }, "title": "Configuration options" } From 982709657ab7bca5e3fa259411259f8fa6b7eb2d Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Fri, 1 May 2020 22:16:16 +0200 Subject: [PATCH 08/14] Remove SERVICE_IGD from discovery component --- homeassistant/components/discovery/__init__.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/homeassistant/components/discovery/__init__.py b/homeassistant/components/discovery/__init__.py index 47d0570b41fabd..c73bca9d036344 100644 --- a/homeassistant/components/discovery/__init__.py +++ b/homeassistant/components/discovery/__init__.py @@ -32,7 +32,6 @@ SERVICE_HASS_IOS_APP = "hass_ios" SERVICE_HASSIO = "hassio" SERVICE_HEOS = "heos" -SERVICE_IGD = "igd" SERVICE_KONNECTED = "konnected" SERVICE_MOBILE_APP = "hass_mobile_app" SERVICE_NETGEAR = "netgear_router" @@ -90,7 +89,6 @@ SERVICE_HEOS, "harmony", "homekit", - SERVICE_IGD, "ikea_tradfri", "philips_hue", "sonos", From 99f4df404479c2f52c72b2178af2b909fa41ff3d Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Fri, 1 May 2020 23:14:36 +0200 Subject: [PATCH 09/14] Retest --- tests/components/upnp/test_init.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/components/upnp/test_init.py b/tests/components/upnp/test_init.py index 6d359e9fb8b4c8..61bc6c787132af 100644 --- a/tests/components/upnp/test_init.py +++ b/tests/components/upnp/test_init.py @@ -97,7 +97,6 @@ async def test_async_setup_entry_default(hass): DISCOVERY_LOCATION: "http://192.168.1.1/desc.xml", } ] - create_device.return_value = mock_coro(return_value=mock_device) async_discover.return_value = mock_coro(return_value=discovery_infos) @@ -146,7 +145,6 @@ async def test_async_setup_entry_port_mapping(hass): DISCOVERY_LOCATION: "http://192.168.1.1/desc.xml", } ] - create_device.return_value = mock_coro(return_value=mock_device) async_discover.return_value = mock_coro(return_value=discovery_infos) From cd8a9b0ec5ea7cc1c3e8a724e6904ed47d1e1910 Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Sat, 2 May 2020 11:58:14 +0200 Subject: [PATCH 10/14] Add unit tests + changes after review --- homeassistant/components/upnp/config_flow.py | 10 +- homeassistant/components/upnp/strings.json | 5 +- .../components/upnp/translations/en.json | 5 +- tests/components/upnp/mock_device.py | 59 +++++++++ tests/components/upnp/test_config_flow.py | 121 ++++++++++++++++++ tests/components/upnp/test_init.py | 73 ++--------- 6 files changed, 197 insertions(+), 76 deletions(-) create mode 100644 tests/components/upnp/mock_device.py create mode 100644 tests/components/upnp/test_config_flow.py diff --git a/homeassistant/components/upnp/config_flow.py b/homeassistant/components/upnp/config_flow.py index 601197ffaa1f9c..ad025482e82ea0 100644 --- a/homeassistant/components/upnp/config_flow.py +++ b/homeassistant/components/upnp/config_flow.py @@ -35,7 +35,7 @@ def __init__(self): """Initialize the UPnP/IGD config flow.""" self._discoveries: Mapping = None - async def async_step_user(self, user_input=None): + async def async_step_user(self, user_input: Optional[Mapping] = None): """Handle a flow start.""" _LOGGER.debug("async_step_user: user_input: %s", user_input) # This uses DISCOVERY_USN as the identifier for the device. @@ -153,7 +153,7 @@ async def async_step_ssdp(self, discovery_info: Mapping): return await self.async_step_ssdp_confirm() - async def async_step_ssdp_confirm(self, user_input=None): + async def async_step_ssdp_confirm(self, user_input: Optional[Mapping] = None): """Confirm integration via SSDP.""" _LOGGER.debug("async_step_ssdp_confirm: user_input: %s", user_input) if user_input is None: @@ -162,8 +162,9 @@ async def async_step_ssdp_confirm(self, user_input=None): discovery = self._discoveries[0] return await self._async_create_entry_from_data(discovery) - async def _async_create_entry_from_data(self, discovery): + async def _async_create_entry_from_data(self, discovery: Mapping): """Create an entry from own _data.""" + _LOGGER.debug("_async_create_entry_from_data: discovery: %s", discovery) # Get name from device, if not found already. if DISCOVERY_NAME not in discovery and DISCOVERY_LOCATION in discovery: discovery[DISCOVERY_NAME] = await self._async_get_name_for_discovery( @@ -177,8 +178,9 @@ async def _async_create_entry_from_data(self, discovery): } return self.async_create_entry(title=title, data=data) - async def _async_get_name_for_discovery(self, discovery): + async def _async_get_name_for_discovery(self, discovery: Mapping): """Get the name of the device from a discovery.""" + _LOGGER.debug('_async_get_name_for_discovery: discovery: %s', discovery) device = await Device.async_create_device( self.hass, discovery[DISCOVERY_LOCATION] ) diff --git a/homeassistant/components/upnp/strings.json b/homeassistant/components/upnp/strings.json index b986a4cdb3009b..9f2f6978341212 100644 --- a/homeassistant/components/upnp/strings.json +++ b/homeassistant/components/upnp/strings.json @@ -3,17 +3,14 @@ "flow_title": "UPnP/IGD: {name}", "step": { "init": { - "title": "UPnP/IGD" }, "ssdp_confirm": { - "title": "UPnP/IGD", "description": "Do you want to set up this UPnP/IGD device?" }, "user": { "data": { "usn": "Device" - }, - "title": "Configuration options" + } } }, "abort": { diff --git a/homeassistant/components/upnp/translations/en.json b/homeassistant/components/upnp/translations/en.json index da4b5e80ce1005..124ccfdd17d4ac 100644 --- a/homeassistant/components/upnp/translations/en.json +++ b/homeassistant/components/upnp/translations/en.json @@ -8,17 +8,14 @@ }, "step": { "init": { - "title": "UPnP/IGD" }, "ssdp_confirm": { - "title": "UPnP/IGD", "description": "Do you want to set up this UPnP/IGD device?" }, "user": { "data": { "usn": "Device" - }, - "title": "Configuration options" + } } } } diff --git a/tests/components/upnp/mock_device.py b/tests/components/upnp/mock_device.py new file mode 100644 index 00000000000000..ee123e73dd99a9 --- /dev/null +++ b/tests/components/upnp/mock_device.py @@ -0,0 +1,59 @@ +"""Mock device for testing purposes.""" + +from homeassistant.components.upnp.device import Device + + +class MockDevice(Device): + """Mock device for Device.""" + + def __init__(self, udn): + """Initialize mock device.""" + igd_device = object() + super().__init__(igd_device) + self._udn = udn + self.added_port_mappings = [] + self.removed_port_mappings = [] + + @classmethod + async def async_create_device(cls, hass, ssdp_location): + """Return self.""" + return cls("UDN") + + @property + def udn(self) -> str: + """Get the UDN.""" + return self._udn + + @property + def manufacturer(self) -> str: + """Get manufacturer.""" + return "mock-manufacturer" + + @property + def name(self) -> str: + """Get name.""" + return "mock-name" + + @property + def model_name(self) -> str: + """Get the model name.""" + return "mock-model-name" + + @property + def device_type(self) -> str: + """Get the device type.""" + return "urn:schemas-upnp-org:device:InternetGatewayDevice:1" + + async def _async_add_port_mapping( + self, external_port: int, local_ip: str, internal_port: int + ) -> None: + """Add a port mapping.""" + entry = [external_port, local_ip, internal_port] + self.added_port_mappings.append(entry) + + async def _async_delete_port_mapping(self, external_port: int) -> None: + """Remove a port mapping.""" + entry = external_port + self.removed_port_mappings.append(entry) + + diff --git a/tests/components/upnp/test_config_flow.py b/tests/components/upnp/test_config_flow.py new file mode 100644 index 00000000000000..eabbdcace7cb9b --- /dev/null +++ b/tests/components/upnp/test_config_flow.py @@ -0,0 +1,121 @@ +"""Test UPnP/IGD config flow.""" + +from asynctest import patch + +from homeassistant import data_entry_flow +from homeassistant.components import ssdp +from homeassistant.components.upnp.const import ( + DISCOVERY_LOCATION, + DISCOVERY_ST, + DISCOVERY_UDN, + DISCOVERY_USN, + DOMAIN, +) +from homeassistant.components.upnp.device import Device +from homeassistant.helpers.typing import HomeAssistantType + +from tests.common import mock_coro +from .mock_device import MockDevice + + +async def test_flow_ssdp_discovery(hass: HomeAssistantType): + """Test config flow: discovered + configured through ssdp.""" + # Discovered via step ssdp. + udn = "uuid:device_1" + mock_device = MockDevice(udn) + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": "ssdp"}, data={ + ssdp.ATTR_SSDP_ST: mock_device.device_type, + ssdp.ATTR_UPNP_UDN: mock_device.udn, + "friendlyName": mock_device.name, + } + ) + assert result["type"] == data_entry_flow.RESULT_TYPE_FORM + assert result["step_id"] == "ssdp_confirm" + + # Confirm via step ssdp_confirm. + with patch.object(Device, "async_create_device") as create_device, \ + patch.object(Device, "async_discover") as async_discover: + create_device.return_value = mock_coro(return_value=mock_device) + async_discover.return_value = mock_coro(return_value=[{ + DISCOVERY_ST: mock_device.device_type, + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_LOCATION: "dummy", + }]) + + result = await hass.config_entries.flow.async_configure( + result["flow_id"], + user_input={}, + ) + + assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY + assert result["title"] == mock_device.name + assert result["data"] == { + "st": mock_device.device_type, + "udn": mock_device.udn, + } + +async def test_flow_user(hass: HomeAssistantType): + """Test config flow: discovered + configured through user.""" + udn = "uuid:device_1" + mock_device = MockDevice(udn) + usn = f"{mock_device.udn}::{mock_device.device_type}" + + with patch.object(Device, "async_create_device") as create_device, \ + patch.object(Device, "async_discover") as async_discover: + # Discovered via step user. + async_discover.return_value = mock_coro(return_value=[{ + DISCOVERY_USN: usn, + DISCOVERY_ST: mock_device.device_type, + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_LOCATION: "dummy", + }]) + create_device.return_value = mock_coro(return_value=mock_device) + + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": "user"} + ) + assert result["type"] == data_entry_flow.RESULT_TYPE_FORM + assert result["step_id"] == "user" + + # Confirmed via step user. + result = await hass.config_entries.flow.async_configure( + result["flow_id"], + user_input={"usn": usn}, + ) + + assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY + assert result["title"] == mock_device.name + assert result["data"] == { + "st": mock_device.device_type, + "udn": mock_device.udn, + } + + +async def test_flow_config(hass: HomeAssistantType): + """Test config flow: discovered + configured through configuration.yaml.""" + udn = "uuid:device_1" + mock_device = MockDevice(udn) + usn = f"{mock_device.udn}::{mock_device.device_type}" + + with patch.object(Device, "async_create_device") as create_device, \ + patch.object(Device, "async_discover") as async_discover: + # Discovered via step import. + async_discover.return_value = mock_coro(return_value=[{ + DISCOVERY_USN: usn, + DISCOVERY_ST: mock_device.device_type, + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_LOCATION: "dummy", + }]) + create_device.return_value = mock_coro(return_value=mock_device) + + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": "import"} + ) + + assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY + assert result["title"] == mock_device.name + assert result["data"] == { + "st": mock_device.device_type, + "udn": mock_device.udn, + } diff --git a/tests/components/upnp/test_init.py b/tests/components/upnp/test_init.py index f8ef451615223b..159d0c489995c6 100644 --- a/tests/components/upnp/test_init.py +++ b/tests/components/upnp/test_init.py @@ -14,67 +14,14 @@ from tests.async_mock import patch from tests.common import MockConfigEntry, mock_coro - - -class MockDevice(Device): - """Mock device for Device.""" - - def __init__(self, udn): - """Initialize mock device.""" - igd_device = object() - super().__init__(igd_device) - self._udn = udn - self.added_port_mappings = [] - self.removed_port_mappings = [] - - @classmethod - async def async_create_device(cls, hass, ssdp_location): - """Return self.""" - return cls("UDN") - - @property - def udn(self) -> str: - """Get the UDN.""" - return self._udn - - @property - def manufacturer(self) -> str: - """Get manufacturer.""" - return "mock-manufacturer" - - @property - def name(self) -> str: - """Get name.""" - return "mock-name" - - @property - def model_name(self) -> str: - """Get the model name.""" - return "mock-model-name" - - @property - def device_type(self) -> str: - """Get the device type.""" - return "urn:schemas-upnp-org:device:InternetGatewayDevice:1" - - async def _async_add_port_mapping( - self, external_port: int, local_ip: str, internal_port: int - ) -> None: - """Add a port mapping.""" - entry = [external_port, local_ip, internal_port] - self.added_port_mappings.append(entry) - - async def _async_delete_port_mapping(self, external_port: int) -> None: - """Remove a port mapping.""" - entry = external_port - self.removed_port_mappings.append(entry) +from .mock_device import MockDevice async def test_async_setup_entry_default(hass): """Test async_setup_entry.""" udn = "uuid:device_1" - st = MockDevice.device_type - entry = MockConfigEntry(domain=upnp.DOMAIN, data={"udn": udn, "st": st}) + mock_device = MockDevice(udn) + entry = MockConfigEntry(domain=upnp.DOMAIN, data={"udn": udn, "st": mock_device.device_type}) config = { # no upnp @@ -88,11 +35,10 @@ async def test_async_setup_entry_default(hass): await hass.async_block_till_done() # mock homeassistant.components.upnp.device.Device - mock_device = MockDevice(udn) discovery_infos = [ { - DISCOVERY_UDN: udn, - DISCOVERY_ST: st, + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_ST: mock_device.device_type, DISCOVERY_LOCATION: "http://192.168.1.1/desc.xml", } ] @@ -116,8 +62,8 @@ async def test_async_setup_entry_port_mapping(hass): """Test async_setup_entry.""" # pylint: disable=invalid-name udn = "uuid:device_1" - st = MockDevice.device_type - entry = MockConfigEntry(domain=upnp.DOMAIN, data={"udn": udn, "st": st}) + mock_device = MockDevice(udn) + entry = MockConfigEntry(domain=upnp.DOMAIN, data={"udn": udn, "st": mock_device.device_type}) config = { "http": {}, @@ -136,11 +82,10 @@ async def test_async_setup_entry_port_mapping(hass): await async_setup_component(hass, "upnp", config) await hass.async_block_till_done() - mock_device = MockDevice(udn) discovery_infos = [ { - DISCOVERY_UDN: udn, - DISCOVERY_ST: st, + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_ST: mock_device.device_type, DISCOVERY_LOCATION: "http://192.168.1.1/desc.xml", } ] From 0295c69dfb9601ecfa34e6a92d83cfef11ee99e8 Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Sat, 2 May 2020 12:18:04 +0200 Subject: [PATCH 11/14] Linting etc --- homeassistant/components/upnp/config_flow.py | 2 +- tests/components/upnp/mock_device.py | 2 - tests/components/upnp/test_config_flow.py | 77 ++++++++++++-------- tests/components/upnp/test_init.py | 11 ++- 4 files changed, 56 insertions(+), 36 deletions(-) diff --git a/homeassistant/components/upnp/config_flow.py b/homeassistant/components/upnp/config_flow.py index ad025482e82ea0..4701f21633c22e 100644 --- a/homeassistant/components/upnp/config_flow.py +++ b/homeassistant/components/upnp/config_flow.py @@ -180,7 +180,7 @@ async def _async_create_entry_from_data(self, discovery: Mapping): async def _async_get_name_for_discovery(self, discovery: Mapping): """Get the name of the device from a discovery.""" - _LOGGER.debug('_async_get_name_for_discovery: discovery: %s', discovery) + _LOGGER.debug("_async_get_name_for_discovery: discovery: %s", discovery) device = await Device.async_create_device( self.hass, discovery[DISCOVERY_LOCATION] ) diff --git a/tests/components/upnp/mock_device.py b/tests/components/upnp/mock_device.py index ee123e73dd99a9..4510534a0f34d5 100644 --- a/tests/components/upnp/mock_device.py +++ b/tests/components/upnp/mock_device.py @@ -55,5 +55,3 @@ async def _async_delete_port_mapping(self, external_port: int) -> None: """Remove a port mapping.""" entry = external_port self.removed_port_mappings.append(entry) - - diff --git a/tests/components/upnp/test_config_flow.py b/tests/components/upnp/test_config_flow.py index eabbdcace7cb9b..ef099aa99904d1 100644 --- a/tests/components/upnp/test_config_flow.py +++ b/tests/components/upnp/test_config_flow.py @@ -14,9 +14,10 @@ from homeassistant.components.upnp.device import Device from homeassistant.helpers.typing import HomeAssistantType -from tests.common import mock_coro from .mock_device import MockDevice +from tests.common import mock_coro + async def test_flow_ssdp_discovery(hass: HomeAssistantType): """Test config flow: discovered + configured through ssdp.""" @@ -24,28 +25,34 @@ async def test_flow_ssdp_discovery(hass: HomeAssistantType): udn = "uuid:device_1" mock_device = MockDevice(udn) result = await hass.config_entries.flow.async_init( - DOMAIN, context={"source": "ssdp"}, data={ + DOMAIN, + context={"source": "ssdp"}, + data={ ssdp.ATTR_SSDP_ST: mock_device.device_type, ssdp.ATTR_UPNP_UDN: mock_device.udn, "friendlyName": mock_device.name, - } + }, ) assert result["type"] == data_entry_flow.RESULT_TYPE_FORM assert result["step_id"] == "ssdp_confirm" # Confirm via step ssdp_confirm. - with patch.object(Device, "async_create_device") as create_device, \ - patch.object(Device, "async_discover") as async_discover: + with patch.object(Device, "async_create_device") as create_device, patch.object( + Device, "async_discover" + ) as async_discover: create_device.return_value = mock_coro(return_value=mock_device) - async_discover.return_value = mock_coro(return_value=[{ - DISCOVERY_ST: mock_device.device_type, - DISCOVERY_UDN: mock_device.udn, - DISCOVERY_LOCATION: "dummy", - }]) + async_discover.return_value = mock_coro( + return_value=[ + { + DISCOVERY_ST: mock_device.device_type, + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_LOCATION: "dummy", + } + ] + ) result = await hass.config_entries.flow.async_configure( - result["flow_id"], - user_input={}, + result["flow_id"], user_input={}, ) assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY @@ -55,21 +62,27 @@ async def test_flow_ssdp_discovery(hass: HomeAssistantType): "udn": mock_device.udn, } + async def test_flow_user(hass: HomeAssistantType): """Test config flow: discovered + configured through user.""" udn = "uuid:device_1" mock_device = MockDevice(udn) usn = f"{mock_device.udn}::{mock_device.device_type}" - with patch.object(Device, "async_create_device") as create_device, \ - patch.object(Device, "async_discover") as async_discover: + with patch.object(Device, "async_create_device") as create_device, patch.object( + Device, "async_discover" + ) as async_discover: # Discovered via step user. - async_discover.return_value = mock_coro(return_value=[{ - DISCOVERY_USN: usn, - DISCOVERY_ST: mock_device.device_type, - DISCOVERY_UDN: mock_device.udn, - DISCOVERY_LOCATION: "dummy", - }]) + async_discover.return_value = mock_coro( + return_value=[ + { + DISCOVERY_USN: usn, + DISCOVERY_ST: mock_device.device_type, + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_LOCATION: "dummy", + } + ] + ) create_device.return_value = mock_coro(return_value=mock_device) result = await hass.config_entries.flow.async_init( @@ -80,8 +93,7 @@ async def test_flow_user(hass: HomeAssistantType): # Confirmed via step user. result = await hass.config_entries.flow.async_configure( - result["flow_id"], - user_input={"usn": usn}, + result["flow_id"], user_input={"usn": usn}, ) assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY @@ -98,15 +110,20 @@ async def test_flow_config(hass: HomeAssistantType): mock_device = MockDevice(udn) usn = f"{mock_device.udn}::{mock_device.device_type}" - with patch.object(Device, "async_create_device") as create_device, \ - patch.object(Device, "async_discover") as async_discover: + with patch.object(Device, "async_create_device") as create_device, patch.object( + Device, "async_discover" + ) as async_discover: # Discovered via step import. - async_discover.return_value = mock_coro(return_value=[{ - DISCOVERY_USN: usn, - DISCOVERY_ST: mock_device.device_type, - DISCOVERY_UDN: mock_device.udn, - DISCOVERY_LOCATION: "dummy", - }]) + async_discover.return_value = mock_coro( + return_value=[ + { + DISCOVERY_USN: usn, + DISCOVERY_ST: mock_device.device_type, + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_LOCATION: "dummy", + } + ] + ) create_device.return_value = mock_coro(return_value=mock_device) result = await hass.config_entries.flow.async_init( diff --git a/tests/components/upnp/test_init.py b/tests/components/upnp/test_init.py index 159d0c489995c6..485ec427b5520c 100644 --- a/tests/components/upnp/test_init.py +++ b/tests/components/upnp/test_init.py @@ -12,16 +12,19 @@ from homeassistant.const import EVENT_HOMEASSISTANT_STOP from homeassistant.setup import async_setup_component +from .mock_device import MockDevice + from tests.async_mock import patch from tests.common import MockConfigEntry, mock_coro -from .mock_device import MockDevice async def test_async_setup_entry_default(hass): """Test async_setup_entry.""" udn = "uuid:device_1" mock_device = MockDevice(udn) - entry = MockConfigEntry(domain=upnp.DOMAIN, data={"udn": udn, "st": mock_device.device_type}) + entry = MockConfigEntry( + domain=upnp.DOMAIN, data={"udn": udn, "st": mock_device.device_type} + ) config = { # no upnp @@ -63,7 +66,9 @@ async def test_async_setup_entry_port_mapping(hass): # pylint: disable=invalid-name udn = "uuid:device_1" mock_device = MockDevice(udn) - entry = MockConfigEntry(domain=upnp.DOMAIN, data={"udn": udn, "st": mock_device.device_type}) + entry = MockConfigEntry( + domain=upnp.DOMAIN, data={"udn": udn, "st": mock_device.device_type} + ) config = { "http": {}, From 4db9b991d4bf6fcb2682424077ac41b60913d636 Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Sat, 2 May 2020 14:19:51 +0200 Subject: [PATCH 12/14] Fixing tests --- tests/components/upnp/mock_device.py | 20 ++++++++++++++++++++ tests/components/upnp/test_init.py | 12 ++++++------ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/tests/components/upnp/mock_device.py b/tests/components/upnp/mock_device.py index 4510534a0f34d5..17d9b5659c5e80 100644 --- a/tests/components/upnp/mock_device.py +++ b/tests/components/upnp/mock_device.py @@ -1,6 +1,16 @@ """Mock device for testing purposes.""" +from typing import Mapping + +from homeassistant.components.upnp.const import ( + BYTES_RECEIVED, + BYTES_SENT, + PACKETS_RECEIVED, + PACKETS_SENT, + TIMESTAMP, +) from homeassistant.components.upnp.device import Device +import homeassistant.util.dt as dt_util class MockDevice(Device): @@ -55,3 +65,13 @@ async def _async_delete_port_mapping(self, external_port: int) -> None: """Remove a port mapping.""" entry = external_port self.removed_port_mappings.append(entry) + + async def async_get_traffic_data(self) -> Mapping[str, any]: + """Get traffic data.""" + return { + TIMESTAMP: dt_util.utcnow(), + BYTES_RECEIVED: 0, + BYTES_SENT: 0, + PACKETS_RECEIVED: 0, + PACKETS_SENT: 0, + } diff --git a/tests/components/upnp/test_init.py b/tests/components/upnp/test_init.py index 485ec427b5520c..2e27874baead29 100644 --- a/tests/components/upnp/test_init.py +++ b/tests/components/upnp/test_init.py @@ -23,7 +23,7 @@ async def test_async_setup_entry_default(hass): udn = "uuid:device_1" mock_device = MockDevice(udn) entry = MockConfigEntry( - domain=upnp.DOMAIN, data={"udn": udn, "st": mock_device.device_type} + domain=upnp.DOMAIN, data={"udn": mock_device.udn, "st": mock_device.device_type} ) config = { @@ -45,8 +45,8 @@ async def test_async_setup_entry_default(hass): DISCOVERY_LOCATION: "http://192.168.1.1/desc.xml", } ] - create_device.return_value = mock_device - async_discover.return_value = discovery_infos + create_device.return_value = mock_coro(return_value=mock_device) + async_discover.return_value = mock_coro(return_value=discovery_infos) assert await upnp.async_setup_entry(hass, entry) is True @@ -67,7 +67,7 @@ async def test_async_setup_entry_port_mapping(hass): udn = "uuid:device_1" mock_device = MockDevice(udn) entry = MockConfigEntry( - domain=upnp.DOMAIN, data={"udn": udn, "st": mock_device.device_type} + domain=upnp.DOMAIN, data={"udn": mock_device.udn, "st": mock_device.device_type} ) config = { @@ -94,8 +94,8 @@ async def test_async_setup_entry_port_mapping(hass): DISCOVERY_LOCATION: "http://192.168.1.1/desc.xml", } ] - create_device.return_value = mock_device - async_discover.return_value = discovery_infos + create_device.return_value = mock_coro(return_value=mock_device) + async_discover.return_value = mock_coro(return_value=discovery_infos) assert await upnp.async_setup_entry(hass, entry) is True From 913d84d7585d363e52b4c75b9f667ffd39e9ad2e Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Sat, 2 May 2020 21:24:22 +0200 Subject: [PATCH 13/14] Re-test --- homeassistant/components/upnp/__init__.py | 1 + tests/components/upnp/test_config_flow.py | 90 +++++++++++------------ tests/components/upnp/test_init.py | 37 +++++----- 3 files changed, 63 insertions(+), 65 deletions(-) diff --git a/homeassistant/components/upnp/__init__.py b/homeassistant/components/upnp/__init__.py index 5a186ecd11b3d6..f183daa4ae9356 100644 --- a/homeassistant/components/upnp/__init__.py +++ b/homeassistant/components/upnp/__init__.py @@ -95,6 +95,7 @@ async def async_discover_and_construct( """Discovery devices and construct a Device for one.""" # pylint: disable=invalid-name discovery_infos = await Device.async_discover(hass) + _LOGGER.debug("Discovered devices: %s", discovery_infos) if not discovery_infos: _LOGGER.info("No UPnP/IGD devices discovered") return None diff --git a/tests/components/upnp/test_config_flow.py b/tests/components/upnp/test_config_flow.py index ef099aa99904d1..c35663067172cb 100644 --- a/tests/components/upnp/test_config_flow.py +++ b/tests/components/upnp/test_config_flow.py @@ -2,7 +2,7 @@ from asynctest import patch -from homeassistant import data_entry_flow +from homeassistant import config_entries, data_entry_flow from homeassistant.components import ssdp from homeassistant.components.upnp.const import ( DISCOVERY_LOCATION, @@ -21,35 +21,35 @@ async def test_flow_ssdp_discovery(hass: HomeAssistantType): """Test config flow: discovered + configured through ssdp.""" - # Discovered via step ssdp. udn = "uuid:device_1" mock_device = MockDevice(udn) - result = await hass.config_entries.flow.async_init( - DOMAIN, - context={"source": "ssdp"}, - data={ - ssdp.ATTR_SSDP_ST: mock_device.device_type, - ssdp.ATTR_UPNP_UDN: mock_device.udn, - "friendlyName": mock_device.name, - }, - ) - assert result["type"] == data_entry_flow.RESULT_TYPE_FORM - assert result["step_id"] == "ssdp_confirm" - - # Confirm via step ssdp_confirm. + discovery_infos = [ + { + DISCOVERY_ST: mock_device.device_type, + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_LOCATION: "dummy", + } + ] + with patch.object(Device, "async_create_device") as create_device, patch.object( Device, "async_discover" ) as async_discover: - create_device.return_value = mock_coro(return_value=mock_device) - async_discover.return_value = mock_coro( - return_value=[ - { - DISCOVERY_ST: mock_device.device_type, - DISCOVERY_UDN: mock_device.udn, - DISCOVERY_LOCATION: "dummy", - } - ] + # Discovered via step ssdp. + result = await hass.config_entries.flow.async_init( + DOMAIN, + context={"source": config_entries.SOURCE_SSDP}, + data={ + ssdp.ATTR_SSDP_ST: mock_device.device_type, + ssdp.ATTR_UPNP_UDN: mock_device.udn, + "friendlyName": mock_device.name, + }, ) + assert result["type"] == data_entry_flow.RESULT_TYPE_FORM + assert result["step_id"] == "ssdp_confirm" + + # Confirm via step ssdp_confirm. + create_device.return_value = mock_coro(return_value=mock_device) + async_discover.return_value = mock_coro(return_value=discovery_infos) result = await hass.config_entries.flow.async_configure( result["flow_id"], user_input={}, @@ -68,25 +68,24 @@ async def test_flow_user(hass: HomeAssistantType): udn = "uuid:device_1" mock_device = MockDevice(udn) usn = f"{mock_device.udn}::{mock_device.device_type}" + discovery_infos = [ + { + DISCOVERY_USN: usn, + DISCOVERY_ST: mock_device.device_type, + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_LOCATION: "dummy", + } + ] with patch.object(Device, "async_create_device") as create_device, patch.object( Device, "async_discover" ) as async_discover: # Discovered via step user. - async_discover.return_value = mock_coro( - return_value=[ - { - DISCOVERY_USN: usn, - DISCOVERY_ST: mock_device.device_type, - DISCOVERY_UDN: mock_device.udn, - DISCOVERY_LOCATION: "dummy", - } - ] - ) + async_discover.return_value = mock_coro(return_value=discovery_infos) create_device.return_value = mock_coro(return_value=mock_device) result = await hass.config_entries.flow.async_init( - DOMAIN, context={"source": "user"} + DOMAIN, context={"source": config_entries.SOURCE_USER} ) assert result["type"] == data_entry_flow.RESULT_TYPE_FORM assert result["step_id"] == "user" @@ -109,25 +108,24 @@ async def test_flow_config(hass: HomeAssistantType): udn = "uuid:device_1" mock_device = MockDevice(udn) usn = f"{mock_device.udn}::{mock_device.device_type}" + discovery_infos = [ + { + DISCOVERY_USN: usn, + DISCOVERY_ST: mock_device.device_type, + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_LOCATION: "dummy", + } + ] with patch.object(Device, "async_create_device") as create_device, patch.object( Device, "async_discover" ) as async_discover: # Discovered via step import. - async_discover.return_value = mock_coro( - return_value=[ - { - DISCOVERY_USN: usn, - DISCOVERY_ST: mock_device.device_type, - DISCOVERY_UDN: mock_device.udn, - DISCOVERY_LOCATION: "dummy", - } - ] - ) + async_discover.return_value = mock_coro(return_value=discovery_infos) create_device.return_value = mock_coro(return_value=mock_device) result = await hass.config_entries.flow.async_init( - DOMAIN, context={"source": "import"} + DOMAIN, context={"source": config_entries.SOURCE_IMPORT} ) assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY diff --git a/tests/components/upnp/test_init.py b/tests/components/upnp/test_init.py index 2e27874baead29..8e53cee7822208 100644 --- a/tests/components/upnp/test_init.py +++ b/tests/components/upnp/test_init.py @@ -22,6 +22,13 @@ async def test_async_setup_entry_default(hass): """Test async_setup_entry.""" udn = "uuid:device_1" mock_device = MockDevice(udn) + discovery_infos = [ + { + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_ST: mock_device.device_type, + DISCOVERY_LOCATION: "http://192.168.1.1/desc.xml", + } + ] entry = MockConfigEntry( domain=upnp.DOMAIN, data={"udn": mock_device.udn, "st": mock_device.device_type} ) @@ -32,22 +39,14 @@ async def test_async_setup_entry_default(hass): with patch.object(Device, "async_create_device") as create_device, patch.object( Device, "async_discover" ) as async_discover: + # initialisation of component, no device discovered async_discover.return_value = mock_coro(return_value=[]) - await async_setup_component(hass, "upnp", config) await hass.async_block_till_done() - # mock homeassistant.components.upnp.device.Device - discovery_infos = [ - { - DISCOVERY_UDN: mock_device.udn, - DISCOVERY_ST: mock_device.device_type, - DISCOVERY_LOCATION: "http://192.168.1.1/desc.xml", - } - ] + # loading of config_entry, device discovered create_device.return_value = mock_coro(return_value=mock_device) async_discover.return_value = mock_coro(return_value=discovery_infos) - assert await upnp.async_setup_entry(hass, entry) is True # ensure device is stored/used @@ -66,6 +65,13 @@ async def test_async_setup_entry_port_mapping(hass): # pylint: disable=invalid-name udn = "uuid:device_1" mock_device = MockDevice(udn) + discovery_infos = [ + { + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_ST: mock_device.device_type, + DISCOVERY_LOCATION: "http://192.168.1.1/desc.xml", + } + ] entry = MockConfigEntry( domain=upnp.DOMAIN, data={"udn": mock_device.udn, "st": mock_device.device_type} ) @@ -81,22 +87,15 @@ async def test_async_setup_entry_port_mapping(hass): with patch.object(Device, "async_create_device") as create_device, patch.object( Device, "async_discover" ) as async_discover: + # initialisation of component, no device discovered async_discover.return_value = mock_coro(return_value=[]) - await async_setup_component(hass, "http", config) await async_setup_component(hass, "upnp", config) await hass.async_block_till_done() - discovery_infos = [ - { - DISCOVERY_UDN: mock_device.udn, - DISCOVERY_ST: mock_device.device_type, - DISCOVERY_LOCATION: "http://192.168.1.1/desc.xml", - } - ] + # loading of config_entry, device discovered create_device.return_value = mock_coro(return_value=mock_device) async_discover.return_value = mock_coro(return_value=discovery_infos) - assert await upnp.async_setup_entry(hass, entry) is True # ensure device is stored/used From ac0f067da5b4c107266c13c638451076a1157a36 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Sat, 2 May 2020 17:19:14 -0700 Subject: [PATCH 14/14] Fix tests --- tests/components/upnp/test_config_flow.py | 32 +++++++---------------- tests/components/upnp/test_init.py | 26 +++++++++--------- 2 files changed, 22 insertions(+), 36 deletions(-) diff --git a/tests/components/upnp/test_config_flow.py b/tests/components/upnp/test_config_flow.py index c35663067172cb..c6e383bae554ea 100644 --- a/tests/components/upnp/test_config_flow.py +++ b/tests/components/upnp/test_config_flow.py @@ -1,7 +1,5 @@ """Test UPnP/IGD config flow.""" -from asynctest import patch - from homeassistant import config_entries, data_entry_flow from homeassistant.components import ssdp from homeassistant.components.upnp.const import ( @@ -16,7 +14,7 @@ from .mock_device import MockDevice -from tests.common import mock_coro +from tests.async_mock import AsyncMock, patch async def test_flow_ssdp_discovery(hass: HomeAssistantType): @@ -30,10 +28,9 @@ async def test_flow_ssdp_discovery(hass: HomeAssistantType): DISCOVERY_LOCATION: "dummy", } ] - - with patch.object(Device, "async_create_device") as create_device, patch.object( - Device, "async_discover" - ) as async_discover: + with patch.object( + Device, "async_create_device", AsyncMock(return_value=mock_device) + ), patch.object(Device, "async_discover", AsyncMock(return_value=discovery_infos)): # Discovered via step ssdp. result = await hass.config_entries.flow.async_init( DOMAIN, @@ -48,9 +45,6 @@ async def test_flow_ssdp_discovery(hass: HomeAssistantType): assert result["step_id"] == "ssdp_confirm" # Confirm via step ssdp_confirm. - create_device.return_value = mock_coro(return_value=mock_device) - async_discover.return_value = mock_coro(return_value=discovery_infos) - result = await hass.config_entries.flow.async_configure( result["flow_id"], user_input={}, ) @@ -77,13 +71,10 @@ async def test_flow_user(hass: HomeAssistantType): } ] - with patch.object(Device, "async_create_device") as create_device, patch.object( - Device, "async_discover" - ) as async_discover: + with patch.object( + Device, "async_create_device", AsyncMock(return_value=mock_device) + ), patch.object(Device, "async_discover", AsyncMock(return_value=discovery_infos)): # Discovered via step user. - async_discover.return_value = mock_coro(return_value=discovery_infos) - create_device.return_value = mock_coro(return_value=mock_device) - result = await hass.config_entries.flow.async_init( DOMAIN, context={"source": config_entries.SOURCE_USER} ) @@ -117,13 +108,10 @@ async def test_flow_config(hass: HomeAssistantType): } ] - with patch.object(Device, "async_create_device") as create_device, patch.object( - Device, "async_discover" - ) as async_discover: + with patch.object( + Device, "async_create_device", AsyncMock(return_value=mock_device) + ), patch.object(Device, "async_discover", AsyncMock(return_value=discovery_infos)): # Discovered via step import. - async_discover.return_value = mock_coro(return_value=discovery_infos) - create_device.return_value = mock_coro(return_value=mock_device) - result = await hass.config_entries.flow.async_init( DOMAIN, context={"source": config_entries.SOURCE_IMPORT} ) diff --git a/tests/components/upnp/test_init.py b/tests/components/upnp/test_init.py index 8e53cee7822208..7d32c37c9ef4e7 100644 --- a/tests/components/upnp/test_init.py +++ b/tests/components/upnp/test_init.py @@ -14,8 +14,8 @@ from .mock_device import MockDevice -from tests.async_mock import patch -from tests.common import MockConfigEntry, mock_coro +from tests.async_mock import AsyncMock, patch +from tests.common import MockConfigEntry async def test_async_setup_entry_default(hass): @@ -36,17 +36,16 @@ async def test_async_setup_entry_default(hass): config = { # no upnp } - with patch.object(Device, "async_create_device") as create_device, patch.object( - Device, "async_discover" - ) as async_discover: + async_discover = AsyncMock(return_value=[]) + with patch.object( + Device, "async_create_device", AsyncMock(return_value=mock_device) + ), patch.object(Device, "async_discover", async_discover): # initialisation of component, no device discovered - async_discover.return_value = mock_coro(return_value=[]) await async_setup_component(hass, "upnp", config) await hass.async_block_till_done() # loading of config_entry, device discovered - create_device.return_value = mock_coro(return_value=mock_device) - async_discover.return_value = mock_coro(return_value=discovery_infos) + async_discover.return_value = discovery_infos assert await upnp.async_setup_entry(hass, entry) is True # ensure device is stored/used @@ -84,18 +83,17 @@ async def test_async_setup_entry_port_mapping(hass): "ports": {"hass": "hass"}, }, } - with patch.object(Device, "async_create_device") as create_device, patch.object( - Device, "async_discover" - ) as async_discover: + async_discover = AsyncMock(return_value=[]) + with patch.object( + Device, "async_create_device", AsyncMock(return_value=mock_device) + ), patch.object(Device, "async_discover", async_discover): # initialisation of component, no device discovered - async_discover.return_value = mock_coro(return_value=[]) await async_setup_component(hass, "http", config) await async_setup_component(hass, "upnp", config) await hass.async_block_till_done() # loading of config_entry, device discovered - create_device.return_value = mock_coro(return_value=mock_device) - async_discover.return_value = mock_coro(return_value=discovery_infos) + async_discover.return_value = discovery_infos assert await upnp.async_setup_entry(hass, entry) is True # ensure device is stored/used