Squeezebox: Fix Allow server device details to merge with players with the same MAC (#133517)

* Disambiguate bewtween servers and player to stop them being merged

* ruff format

* make SqueezeLite players not a service

* ruff

* Tidy redunant code

* config url

* revert config url

* change to domain server

* use default to see how they are mereged with server device

* refactor to use defaults so where a player is part of a bigger ie server service device in the same intergration it doesnt replace its information

* ruff

* make test match the new data

* Fix merge

* Fix tests

* Fix meregd test data

* Fix all tests add new test for merged device in reg

* Remove info from device_info so its only a lookup

* manual merge of server player shared devices

* Fix format of merged entires

* fixes for testing

* Fix test with input from @peteS-UK device knowlonger exits for this test

* Fix test now device doesnt exits for tests

* Update homeassistant/components/squeezebox/media_player.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Fix Copilots formatting

* Apply suggestions from code review

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Erik Montnemery <erik@montnemery.com>

homeassistant/components/squeezebox/__init__.py

@@ -39,8 +39,9 @@ from .const import (
     DOMAIN,
     KNOWN_PLAYERS,
     KNOWN_SERVERS,
-    MANUFACTURER,
+    SERVER_MANUFACTURER,
     SERVER_MODEL,
+    SERVER_MODEL_ID,
     SIGNAL_PLAYER_DISCOVERED,
     SIGNAL_PLAYER_REDISCOVERED,
     STATUS_API_TIMEOUT,
@@ -173,8 +174,9 @@ async def async_setup_entry(hass: HomeAssistant, entry: SqueezeboxConfigEntry) -
         config_entry_id=entry.entry_id,
         identifiers={(DOMAIN, lms.uuid)},
         name=lms.name,
-        manufacturer=MANUFACTURER,
+        manufacturer=SERVER_MANUFACTURER,
         model=SERVER_MODEL,
+        model_id=SERVER_MODEL_ID,
         sw_version=version,
         entry_type=DeviceEntryType.SERVICE,
         connections=mac_connect,

homeassistant/components/squeezebox/const.py

@@ -6,10 +6,11 @@ DOMAIN = "squeezebox"
 DEFAULT_PORT = 9000
 KNOWN_PLAYERS = "known_players"
 KNOWN_SERVERS = "known_servers"
-MANUFACTURER = "https://lyrion.org/"
 PLAYER_DISCOVERY_UNSUB = "player_discovery_unsub"
 SENSOR_UPDATE_INTERVAL = 60
+SERVER_MANUFACTURER = "https://lyrion.org/"
 SERVER_MODEL = "Lyrion Music Server"
+SERVER_MODEL_ID = "LMS"
 STATUS_API_TIMEOUT = 10
 STATUS_SENSOR_LASTSCAN = "lastscan"
 STATUS_SENSOR_NEEDSRESTART = "needsrestart"

homeassistant/components/squeezebox/entity.py

@@ -26,11 +26,7 @@ class SqueezeboxEntity(CoordinatorEntity[SqueezeBoxPlayerUpdateCoordinator]):
         self._player = coordinator.player
         self._attr_device_info = DeviceInfo(
             identifiers={(DOMAIN, format_mac(self._player.player_id))},
-            name=self._player.name,
             connections={(CONNECTION_NETWORK_MAC, format_mac(self._player.player_id))},
-            via_device=(DOMAIN, coordinator.server_uuid),
-            model=self._player.model,
-            manufacturer=self._player.creator,
         )
 
     @property

homeassistant/components/squeezebox/media_player.py

@@ -33,11 +33,12 @@ from homeassistant.core import HomeAssistant, callback
 from homeassistant.exceptions import ServiceValidationError
 from homeassistant.helpers import (
     config_validation as cv,
+    device_registry as dr,
     discovery_flow,
     entity_platform,
     entity_registry as er,
 )
-from homeassistant.helpers.device_registry import format_mac
+from homeassistant.helpers.device_registry import CONNECTION_NETWORK_MAC, format_mac
 from homeassistant.helpers.dispatcher import async_dispatcher_connect
 from homeassistant.helpers.entity_platform import AddConfigEntryEntitiesCallback
 from homeassistant.helpers.start import async_at_start
@@ -61,6 +62,9 @@ from .const import (
     DOMAIN,
     KNOWN_PLAYERS,
     KNOWN_SERVERS,
+    SERVER_MANUFACTURER,
+    SERVER_MODEL,
+    SERVER_MODEL_ID,
     SIGNAL_PLAYER_DISCOVERED,
     SQUEEZEBOX_SOURCE_STRINGS,
 )
@@ -125,9 +129,52 @@ async def async_setup_entry(
     """Set up the Squeezebox media_player platform from a server config entry."""
 
     # Add media player entities when discovered
-    async def _player_discovered(player: SqueezeBoxPlayerUpdateCoordinator) -> None:
+    async def _player_discovered(
-        _LOGGER.debug("Setting up media_player entity for player %s", player)
+        coordinator: SqueezeBoxPlayerUpdateCoordinator,
-        async_add_entities([SqueezeBoxMediaPlayerEntity(player)])
+    ) -> None:
+        player = coordinator.player
+        _LOGGER.debug("Setting up media_player device and entity for player %s", player)
+        device_registry = dr.async_get(hass)
+        server_device = device_registry.async_get_device(
+            identifiers={(DOMAIN, coordinator.server_uuid)},
+        )
+
+        name = player.name
+        model = player.model
+        manufacturer = player.creator
+        model_id = player.model_type
+        sw_version = ""
+        # Why? so we nicely merge with a server and a player linked by a MAC server is not all info lost
+        if (
+            server_device
+            and (CONNECTION_NETWORK_MAC, format_mac(player.player_id))
+            in server_device.connections
+        ):
+            _LOGGER.debug("Shared server & player device %s", server_device)
+            name = server_device.name
+            sw_version = server_device.sw_version or sw_version
+            model = SERVER_MODEL + "/" + model if model else SERVER_MODEL
+            manufacturer = (
+                SERVER_MANUFACTURER + " / " + manufacturer
+                if manufacturer
+                else SERVER_MANUFACTURER
+            )
+            model_id = SERVER_MODEL_ID + "/" + model_id if model_id else SERVER_MODEL_ID
+
+        device = device_registry.async_get_or_create(
+            config_entry_id=entry.entry_id,
+            identifiers={(DOMAIN, player.player_id)},
+            connections={(CONNECTION_NETWORK_MAC, player.player_id)},
+            name=name,
+            model=model,
+            manufacturer=manufacturer,
+            model_id=model_id,
+            hw_version=player.firmware,
+            sw_version=sw_version,
+            via_device=(DOMAIN, coordinator.server_uuid),
+        )
+        _LOGGER.debug("Creating / Updating player device %s", device)
+        async_add_entities([SqueezeBoxMediaPlayerEntity(coordinator)])
 
     entry.async_on_unload(
         async_dispatcher_connect(hass, SIGNAL_PLAYER_DISCOVERED, _player_discovered)

tests/components/squeezebox/conftest.py

@@ -30,7 +30,6 @@ from homeassistant.components.squeezebox.const import (
 )
 from homeassistant.const import CONF_HOST, CONF_PORT, Platform
 from homeassistant.core import HomeAssistant
-from homeassistant.helpers.device_registry import format_mac
 
 from tests.common import MockConfigEntry
 
@@ -44,7 +43,7 @@ SERVER_UUIDS = [
     "12345678-1234-1234-1234-123456789012",
     "87654321-4321-4321-4321-210987654321",
 ]
-TEST_MAC = ["aa:bb:cc:dd:ee:ff", "ff:ee:dd:cc:bb:aa"]
+TEST_MAC = ["aa:bb:cc:dd:ee:ff", "de:ad:be:ef:de:ad", "ff:ee:dd:cc:bb:aa"]
 TEST_PLAYER_NAME = "Test Player"
 TEST_SERVER_NAME = "Test Server"
 TEST_ALARM_ID = "1"
@@ -52,14 +51,13 @@ FAKE_VALID_ITEM_ID = "1234"
 FAKE_INVALID_ITEM_ID = "4321"
 
 FAKE_IP = "42.42.42.42"
-FAKE_MAC = "deadbeefdead"
+FAKE_UUID = "deadbeefdeadbeefbeefdeafbddeef42"
-FAKE_UUID = "deadbeefdeadbeefbeefdeafbeef42"
 FAKE_PORT = 9000
 FAKE_VERSION = "42.0"
 
 FAKE_QUERY_RESPONSE = {
-    STATUS_QUERY_UUID: FAKE_UUID,
+    STATUS_QUERY_UUID: SERVER_UUIDS[0],
-    STATUS_QUERY_MAC: FAKE_MAC,
+    STATUS_QUERY_MAC: TEST_MAC[2],
     STATUS_QUERY_VERSION: FAKE_VERSION,
     STATUS_SENSOR_RESCAN: 1,
     STATUS_SENSOR_LASTSCAN: 0,
@@ -268,6 +266,7 @@ def player_factory() -> MagicMock:
 
 def mock_pysqueezebox_player(uuid: str) -> MagicMock:
     """Mock a Lyrion Media Server player."""
+    assert uuid
     with patch(
         "homeassistant.components.squeezebox.Player", autospec=True
     ) as mock_player:
@@ -294,6 +293,8 @@ def mock_pysqueezebox_player(uuid: str) -> MagicMock:
         mock_player.image_url = None
         mock_player.model = "SqueezeLite"
         mock_player.creator = "Ralph Irving & Adrian Smith"
+        mock_player.model_type = None
+        mock_player.firmware = None
         mock_player.alarms_enabled = True
 
         return mock_player
@@ -310,7 +311,7 @@ def lms_factory(player_factory: MagicMock) -> MagicMock:
 @pytest.fixture
 def lms(player_factory: MagicMock) -> MagicMock:
     """Mock a Lyrion Media Server with one mock player attached."""
-    return mock_pysqueezebox_server(player_factory, 1, uuid=TEST_MAC[0])
+    return mock_pysqueezebox_server(player_factory, 1, uuid=SERVER_UUIDS[0])
 
 
 def mock_pysqueezebox_server(
@@ -323,9 +324,11 @@ def mock_pysqueezebox_server(
 
         mock_lms.uuid = uuid
         mock_lms.name = TEST_SERVER_NAME
-        mock_lms.async_query = AsyncMock(return_value={"uuid": format_mac(uuid)})
+        mock_lms.async_query = AsyncMock(
+            return_value={"uuid": uuid, "mac": TEST_MAC[2]}
+        )
         mock_lms.async_status = AsyncMock(
-            return_value={"uuid": format_mac(uuid), "version": FAKE_VERSION}
+            return_value={"uuid": uuid, "version": FAKE_VERSION}
         )
         return mock_lms
 
@@ -428,6 +431,6 @@ async def configured_players(
     hass: HomeAssistant, config_entry: MockConfigEntry, lms_factory: MagicMock
 ) -> list[MagicMock]:
     """Fixture mocking calls to two pysqueezebox Players from a configured squeezebox."""
-    lms = lms_factory(2, uuid=SERVER_UUIDS[0])
+    lms = lms_factory(3, uuid=SERVER_UUIDS[0])
     await configure_squeezebox_media_player_platform(hass, config_entry, lms)
     return await lms.async_get_players()

tests/components/squeezebox/snapshots/test_media_player.ambr

@@ -12,7 +12,7 @@
       ),
     }),
     'disabled_by': None,
-    'entry_type': <DeviceEntryType.SERVICE: 'service'>,
+    'entry_type': None,
     'hw_version': None,
     'id': <ANY>,
     'identifiers': set({
@@ -32,7 +32,48 @@
     'primary_config_entry': <ANY>,
     'serial_number': None,
     'suggested_area': None,
-    'sw_version': None,
+    'sw_version': '',
+    'via_device_id': <ANY>,
+  })
+# ---
+# name: test_device_registry_server_merged
+  DeviceRegistryEntrySnapshot({
+    'area_id': None,
+    'config_entries': <ANY>,
+    'config_entries_subentries': <ANY>,
+    'configuration_url': None,
+    'connections': set({
+      tuple(
+        'mac',
+        'ff:ee:dd:cc:bb:aa',
+      ),
+    }),
+    'disabled_by': None,
+    'entry_type': <DeviceEntryType.SERVICE: 'service'>,
+    'hw_version': None,
+    'id': <ANY>,
+    'identifiers': set({
+      tuple(
+        'squeezebox',
+        '12345678-1234-1234-1234-123456789012',
+      ),
+      tuple(
+        'squeezebox',
+        'ff:ee:dd:cc:bb:aa',
+      ),
+    }),
+    'is_new': False,
+    'labels': set({
+    }),
+    'manufacturer': 'https://lyrion.org/ / Ralph Irving & Adrian Smith',
+    'model': 'Lyrion Music Server/SqueezeLite',
+    'model_id': 'LMS',
+    'name': '1.2.3.4',
+    'name_by_user': None,
+    'primary_config_entry': <ANY>,
+    'serial_number': None,
+    'suggested_area': None,
+    'sw_version': '',
     'via_device_id': <ANY>,
   })
 # ---

tests/components/squeezebox/snapshots/test_switch.ambr

@@ -1,5 +1,5 @@
 # serializer version: 1
-# name: test_entity_registry[switch.test_player_alarm_1-entry]
+# name: test_entity_registry[switch.none_alarm_1-entry]
   EntityRegistryEntrySnapshot({
     'aliases': set({
     }),
@@ -12,7 +12,7 @@
     'disabled_by': None,
     'domain': 'switch',
     'entity_category': <EntityCategory.CONFIG: 'config'>,
-    'entity_id': 'switch.test_player_alarm_1',
+    'entity_id': 'switch.none_alarm_1',
     'has_entity_name': True,
     'hidden_by': None,
     'icon': None,
@@ -34,21 +34,21 @@
     'unit_of_measurement': None,
   })
 # ---
-# name: test_entity_registry[switch.test_player_alarm_1-state]
+# name: test_entity_registry[switch.none_alarm_1-state]
   StateSnapshot({
     'attributes': ReadOnlyDict({
       'alarm_id': '1',
-      'friendly_name': 'Test Player Alarm (1)',
+      'friendly_name': 'Alarm (1)',
     }),
     'context': <ANY>,
-    'entity_id': 'switch.test_player_alarm_1',
+    'entity_id': 'switch.none_alarm_1',
     'last_changed': <ANY>,
     'last_reported': <ANY>,
     'last_updated': <ANY>,
     'state': 'on',
   })
 # ---
-# name: test_entity_registry[switch.test_player_alarms_enabled-entry]
+# name: test_entity_registry[switch.none_alarms_enabled-entry]
   EntityRegistryEntrySnapshot({
     'aliases': set({
     }),
@@ -61,7 +61,7 @@
     'disabled_by': None,
     'domain': 'switch',
     'entity_category': <EntityCategory.CONFIG: 'config'>,
-    'entity_id': 'switch.test_player_alarms_enabled',
+    'entity_id': 'switch.none_alarms_enabled',
     'has_entity_name': True,
     'hidden_by': None,
     'icon': None,
@@ -83,13 +83,13 @@
     'unit_of_measurement': None,
   })
 # ---
-# name: test_entity_registry[switch.test_player_alarms_enabled-state]
+# name: test_entity_registry[switch.none_alarms_enabled-state]
   StateSnapshot({
     'attributes': ReadOnlyDict({
-      'friendly_name': 'Test Player Alarms enabled',
+      'friendly_name': 'Alarms enabled',
     }),
     'context': <ANY>,
-    'entity_id': 'switch.test_player_alarms_enabled',
+    'entity_id': 'switch.none_alarms_enabled',
     'last_changed': <ANY>,
     'last_reported': <ANY>,
     'last_updated': <ANY>,

tests/components/squeezebox/test_button.py

@@ -14,7 +14,7 @@ async def test_squeezebox_press(
     await hass.services.async_call(
         BUTTON_DOMAIN,
         SERVICE_PRESS,
-        {ATTR_ENTITY_ID: "button.test_player_preset_1"},
+        {ATTR_ENTITY_ID: "button.none_preset_1"},
         blocking=True,
     )
 

tests/components/squeezebox/test_media_player.py

@@ -94,6 +94,18 @@ async def test_device_registry(
     assert reg_device == snapshot
 
 
+async def test_device_registry_server_merged(
+    hass: HomeAssistant,
+    device_registry: DeviceRegistry,
+    configured_players: MagicMock,
+    snapshot: SnapshotAssertion,
+) -> None:
+    """Test squeezebox device registered in the device registry."""
+    reg_device = device_registry.async_get_device(identifiers={(DOMAIN, TEST_MAC[2])})
+    assert reg_device is not None
+    assert reg_device == snapshot
+
+
 async def test_entity_registry(
     hass: HomeAssistant,
     entity_registry: EntityRegistry,

tests/components/squeezebox/test_switch.py

@@ -34,13 +34,13 @@ async def test_switch_state(
     freezer: FrozenDateTimeFactory,
 ) -> None:
     """Test the state of the switch."""
-    assert hass.states.get(f"switch.test_player_alarm_{TEST_ALARM_ID}").state == "on"
+    assert hass.states.get(f"switch.none_alarm_{TEST_ALARM_ID}").state == "on"
 
     mock_alarms_player.alarms[0]["enabled"] = False
     freezer.tick(timedelta(seconds=PLAYER_UPDATE_INTERVAL))
     async_fire_time_changed(hass)
     await hass.async_block_till_done()
-    assert hass.states.get(f"switch.test_player_alarm_{TEST_ALARM_ID}").state == "off"
+    assert hass.states.get(f"switch.none_alarm_{TEST_ALARM_ID}").state == "off"
 
 
 async def test_switch_deleted(
@@ -49,13 +49,13 @@ async def test_switch_deleted(
     freezer: FrozenDateTimeFactory,
 ) -> None:
     """Test detecting switch deleted."""
-    assert hass.states.get(f"switch.test_player_alarm_{TEST_ALARM_ID}").state == "on"
+    assert hass.states.get(f"switch.none_alarm_{TEST_ALARM_ID}").state == "on"
 
     mock_alarms_player.alarms = []
     freezer.tick(timedelta(seconds=PLAYER_UPDATE_INTERVAL))
     async_fire_time_changed(hass)
     await hass.async_block_till_done()
-    assert hass.states.get(f"switch.test_player_alarm_{TEST_ALARM_ID}") is None
+    assert hass.states.get(f"switch.none_alarm_{TEST_ALARM_ID}") is None
 
 
 async def test_turn_on(
@@ -66,7 +66,7 @@ async def test_turn_on(
     await hass.services.async_call(
         SWITCH_DOMAIN,
         SERVICE_TURN_ON,
-        {CONF_ENTITY_ID: f"switch.test_player_alarm_{TEST_ALARM_ID}"},
+        {CONF_ENTITY_ID: f"switch.none_alarm_{TEST_ALARM_ID}"},
         blocking=True,
     )
     mock_alarms_player.async_update_alarm.assert_called_once_with(
@@ -82,7 +82,7 @@ async def test_turn_off(
     await hass.services.async_call(
         SWITCH_DOMAIN,
         SERVICE_TURN_OFF,
-        {CONF_ENTITY_ID: f"switch.test_player_alarm_{TEST_ALARM_ID}"},
+        {CONF_ENTITY_ID: f"switch.none_alarm_{TEST_ALARM_ID}"},
         blocking=True,
     )
     mock_alarms_player.async_update_alarm.assert_called_once_with(
@@ -97,14 +97,14 @@ async def test_alarms_enabled_state(
 ) -> None:
     """Test the alarms enabled switch."""
 
-    assert hass.states.get("switch.test_player_alarms_enabled").state == "on"
+    assert hass.states.get("switch.none_alarms_enabled").state == "on"
 
     mock_alarms_player.alarms_enabled = False
     freezer.tick(timedelta(seconds=PLAYER_UPDATE_INTERVAL))
     async_fire_time_changed(hass)
     await hass.async_block_till_done()
 
-    assert hass.states.get("switch.test_player_alarms_enabled").state == "off"
+    assert hass.states.get("switch.none_alarms_enabled").state == "off"
 
 
 async def test_alarms_enabled_turn_on(
@@ -115,7 +115,7 @@ async def test_alarms_enabled_turn_on(
     await hass.services.async_call(
         SWITCH_DOMAIN,
         SERVICE_TURN_ON,
-        {CONF_ENTITY_ID: "switch.test_player_alarms_enabled"},
+        {CONF_ENTITY_ID: "switch.none_alarms_enabled"},
         blocking=True,
     )
     mock_alarms_player.async_set_alarms_enabled.assert_called_once_with(True)
@@ -129,7 +129,7 @@ async def test_alarms_enabled_turn_off(
     await hass.services.async_call(
         SWITCH_DOMAIN,
         SERVICE_TURN_OFF,
-        {CONF_ENTITY_ID: "switch.test_player_alarms_enabled"},
+        {CONF_ENTITY_ID: "switch.none_alarms_enabled"},
         blocking=True,
     )
     mock_alarms_player.async_set_alarms_enabled.assert_called_once_with(False)