Fix Squeezebox dhcp discovery (#54137)

* Fix Squeezebox dhcp discovery and allow ignore

* Test ignoring known Squeezebox players

* Fix linter errors

homeassistant/components/squeezebox/browse_media.py

@@ -137,7 +137,6 @@ async def build_item_response(entity, player, payload):
 
 async def library_payload(player):
     """Create response payload to describe contents of library."""
-
     library_info = {
         "title": "Music Library",
         "media_class": MEDIA_CLASS_DIRECTORY,

homeassistant/components/squeezebox/config_flow.py

@@ -5,8 +5,9 @@ import logging
 from pysqueezebox import Server, async_discover
 import voluptuous as vol
 
-from homeassistant import config_entries
+from homeassistant import config_entries, data_entry_flow
-from homeassistant.components.dhcp import IP_ADDRESS, MAC_ADDRESS
+from homeassistant.components.dhcp import MAC_ADDRESS
+from homeassistant.components.media_player import DOMAIN as MP_DOMAIN
 from homeassistant.const import (
     CONF_HOST,
     CONF_PASSWORD,
@@ -15,6 +16,8 @@ from homeassistant.const import (
     HTTP_UNAUTHORIZED,
 )
 from homeassistant.helpers.aiohttp_client import async_get_clientsession
+from homeassistant.helpers.device_registry import format_mac
+from homeassistant.helpers.entity_registry import async_get
 
 from .const import DEFAULT_PORT, DOMAIN
 
@@ -166,28 +169,18 @@ class SqueezeboxConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
             return self.async_abort(reason=error)
         return self.async_create_entry(title=config[CONF_HOST], data=config)
 
-    async def async_step_discovery(self, discovery_info):
+    async def async_step_integration_discovery(self, discovery_info):
-        """Handle discovery."""
+        """Handle discovery of a server."""
-        _LOGGER.debug("Reached discovery flow with info: %s", discovery_info)
+        _LOGGER.debug("Reached server discovery flow with info: %s", discovery_info)
         if "uuid" in discovery_info:
             await self.async_set_unique_id(discovery_info.pop("uuid"))
             self._abort_if_unique_id_configured()
         else:
             # attempt to connect to server and determine uuid. will fail if
             # password required
-
-            if CONF_HOST not in discovery_info and IP_ADDRESS in discovery_info:
-                discovery_info[CONF_HOST] = discovery_info[IP_ADDRESS]
-
-            if CONF_PORT not in discovery_info:
-                discovery_info[CONF_PORT] = DEFAULT_PORT
-
             error = await self._validate_input(discovery_info)
             if error:
-                if MAC_ADDRESS in discovery_info:
+                await self._async_handle_discovery_without_unique_id()
-                    await self.async_set_unique_id(discovery_info[MAC_ADDRESS])
-                else:
-                    await self._async_handle_discovery_without_unique_id()
 
         # update schema with suggested values from discovery
         self.data_schema = _base_schema(discovery_info)
@@ -195,3 +188,23 @@ class SqueezeboxConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
         self.context.update({"title_placeholders": {"host": discovery_info[CONF_HOST]}})
 
         return await self.async_step_edit()
+
+    async def async_step_dhcp(self, discovery_info):
+        """Handle dhcp discovery of a Squeezebox player."""
+        _LOGGER.debug(
+            "Reached dhcp discovery of a player with info: %s", discovery_info
+        )
+        await self.async_set_unique_id(format_mac(discovery_info[MAC_ADDRESS]))
+        self._abort_if_unique_id_configured()
+
+        _LOGGER.debug("Configuring dhcp player with unique id: %s", self.unique_id)
+
+        registry = async_get(self.hass)
+
+        # if we have detected this player, do nothing. if not, there must be a server out there for us to configure, so start the normal user flow (which tries to autodetect server)
+        if registry.async_get_entity_id(MP_DOMAIN, DOMAIN, self.unique_id) is not None:
+            # this player is already known, so do nothing other than mark as configured
+            raise data_entry_flow.AbortFlow("already_configured")
+
+        # if the player is unknown, then we likely need to configure its server
+        return await self.async_step_user()

homeassistant/components/squeezebox/media_player.py

@@ -27,7 +27,7 @@ from homeassistant.components.media_player.const import (
     SUPPORT_VOLUME_MUTE,
     SUPPORT_VOLUME_SET,
 )
-from homeassistant.config_entries import SOURCE_DISCOVERY
+from homeassistant.config_entries import SOURCE_INTEGRATION_DISCOVERY
 from homeassistant.const import (
     ATTR_COMMAND,
     CONF_HOST,
@@ -43,6 +43,7 @@ from homeassistant.const import (
 from homeassistant.core import callback
 from homeassistant.helpers import config_validation as cv, entity_platform
 from homeassistant.helpers.aiohttp_client import async_get_clientsession
+from homeassistant.helpers.device_registry import format_mac
 from homeassistant.helpers.dispatcher import (
     async_dispatcher_connect,
     async_dispatcher_send,
@@ -127,7 +128,7 @@ async def start_server_discovery(hass):
         asyncio.create_task(
             hass.config_entries.flow.async_init(
                 DOMAIN,
-                context={"source": SOURCE_DISCOVERY},
+                context={"source": SOURCE_INTEGRATION_DISCOVERY},
                 data={
                     CONF_HOST: server.host,
                     CONF_PORT: int(server.port),
@@ -146,7 +147,6 @@ async def start_server_discovery(hass):
 
 async def async_setup_platform(hass, config, async_add_entities, discovery_info=None):
     """Set up squeezebox platform from platform entry in configuration.yaml (deprecated)."""
-
     if config:
         await hass.config_entries.flow.async_init(
             DOMAIN, context={"source": config_entries.SOURCE_IMPORT}, data=config
@@ -283,7 +283,7 @@ class SqueezeBoxEntity(MediaPlayerEntity):
     @property
     def unique_id(self):
         """Return a unique ID."""
-        return self._player.player_id
+        return format_mac(self._player.player_id)
 
     @property
     def available(self):
@@ -573,7 +573,6 @@ class SqueezeBoxEntity(MediaPlayerEntity):
 
     async def async_browse_media(self, media_content_type=None, media_content_id=None):
         """Implement the websocket media browsing helper."""
-
         _LOGGER.debug(
             "Reached async_browse_media with content_type %s and content_id %s",
             media_content_type,

tests/components/squeezebox/test_config_flow.py

@@ -178,7 +178,7 @@ async def test_discovery(hass):
     ):
         result = await hass.config_entries.flow.async_init(
             DOMAIN,
-            context={"source": config_entries.SOURCE_DISCOVERY},
+            context={"source": config_entries.SOURCE_INTEGRATION_DISCOVERY},
             data={CONF_HOST: HOST, CONF_PORT: PORT, "uuid": UUID},
         )
         assert result["type"] == RESULT_TYPE_FORM
@@ -190,7 +190,7 @@ async def test_discovery_no_uuid(hass):
     with patch("pysqueezebox.Server.async_query", new=patch_async_query_unauthorized):
         result = await hass.config_entries.flow.async_init(
             DOMAIN,
-            context={"source": config_entries.SOURCE_DISCOVERY},
+            context={"source": config_entries.SOURCE_INTEGRATION_DISCOVERY},
             data={CONF_HOST: HOST, CONF_PORT: PORT},
         )
         assert result["type"] == RESULT_TYPE_FORM
@@ -199,9 +199,8 @@ async def test_discovery_no_uuid(hass):
 
 async def test_dhcp_discovery(hass):
     """Test we can process discovery from dhcp."""
-    with patch(
+    with patch("pysqueezebox.Server.async_query", return_value={"uuid": UUID},), patch(
-        "pysqueezebox.Server.async_query",
+        "homeassistant.components.squeezebox.config_flow.async_discover", mock_discover
-        return_value={"uuid": UUID},
     ):
         result = await hass.config_entries.flow.async_init(
             DOMAIN,
@@ -216,9 +215,12 @@ async def test_dhcp_discovery(hass):
         assert result["step_id"] == "edit"
 
 
-async def test_dhcp_discovery_no_connection(hass):
+async def test_dhcp_discovery_no_server_found(hass):
-    """Test we can process discovery from dhcp without connecting to squeezebox server."""
+    """Test we can handle dhcp discovery when no server is found."""
-    with patch("pysqueezebox.Server.async_query", new=patch_async_query_unauthorized):
+    with patch(
+        "homeassistant.components.squeezebox.config_flow.async_discover",
+        mock_failed_discover,
+    ):
         result = await hass.config_entries.flow.async_init(
             DOMAIN,
             context={"source": config_entries.SOURCE_DHCP},
@@ -229,7 +231,25 @@ async def test_dhcp_discovery_no_connection(hass):
             },
         )
         assert result["type"] == RESULT_TYPE_FORM
-        assert result["step_id"] == "edit"
+        assert result["step_id"] == "user"
+
+
+async def test_dhcp_discovery_existing_player(hass):
+    """Test that we properly ignore known players during dhcp discover."""
+    with patch(
+        "homeassistant.helpers.entity_registry.EntityRegistry.async_get_entity_id",
+        return_value="test_entity",
+    ):
+        result = await hass.config_entries.flow.async_init(
+            DOMAIN,
+            context={"source": config_entries.SOURCE_DHCP},
+            data={
+                IP_ADDRESS: "1.1.1.1",
+                MAC_ADDRESS: "AA:BB:CC:DD:EE:FF",
+                HOSTNAME: "any",
+            },
+        )
+        assert result["type"] == RESULT_TYPE_ABORT
 
 
 async def test_import(hass):