From 52a99aea0cd0c69a34f3e72e03a3ebb9e32b0de4 Mon Sep 17 00:00:00 2001 From: "Phill (pssc)" Date: Mon, 30 Jun 2025 10:41:22 +0100 Subject: [PATCH] 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 --- .../components/squeezebox/__init__.py | 6 +- homeassistant/components/squeezebox/const.py | 3 +- homeassistant/components/squeezebox/entity.py | 4 -- .../components/squeezebox/media_player.py | 55 +++++++++++++++++-- tests/components/squeezebox/conftest.py | 23 ++++---- .../snapshots/test_media_player.ambr | 45 ++++++++++++++- .../squeezebox/snapshots/test_switch.ambr | 20 +++---- tests/components/squeezebox/test_button.py | 2 +- .../squeezebox/test_media_player.py | 12 ++++ tests/components/squeezebox/test_switch.py | 20 +++---- 10 files changed, 146 insertions(+), 44 deletions(-) diff --git a/homeassistant/components/squeezebox/__init__.py b/homeassistant/components/squeezebox/__init__.py index 596a44c498c..8bd0e2fca52 100644 --- a/homeassistant/components/squeezebox/__init__.py +++ b/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, diff --git a/homeassistant/components/squeezebox/const.py b/homeassistant/components/squeezebox/const.py index 92eb3736341..9d78605aee1 100644 --- a/homeassistant/components/squeezebox/const.py +++ b/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" diff --git a/homeassistant/components/squeezebox/entity.py b/homeassistant/components/squeezebox/entity.py index 95fd2d60461..f2be716320f 100644 --- a/homeassistant/components/squeezebox/entity.py +++ b/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 diff --git a/homeassistant/components/squeezebox/media_player.py b/homeassistant/components/squeezebox/media_player.py index b29e19c1e3c..8cf945cd7e9 100644 --- a/homeassistant/components/squeezebox/media_player.py +++ b/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: - _LOGGER.debug("Setting up media_player entity for player %s", player) - async_add_entities([SqueezeBoxMediaPlayerEntity(player)]) + async def _player_discovered( + coordinator: SqueezeBoxPlayerUpdateCoordinator, + ) -> 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) diff --git a/tests/components/squeezebox/conftest.py b/tests/components/squeezebox/conftest.py index a3adf05f5f0..97aca31fa05 100644 --- a/tests/components/squeezebox/conftest.py +++ b/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 = "deadbeefdeadbeefbeefdeafbeef42" +FAKE_UUID = "deadbeefdeadbeefbeefdeafbddeef42" FAKE_PORT = 9000 FAKE_VERSION = "42.0" FAKE_QUERY_RESPONSE = { - STATUS_QUERY_UUID: FAKE_UUID, - STATUS_QUERY_MAC: FAKE_MAC, + STATUS_QUERY_UUID: SERVER_UUIDS[0], + 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() diff --git a/tests/components/squeezebox/snapshots/test_media_player.ambr b/tests/components/squeezebox/snapshots/test_media_player.ambr index 4bb00dea5c6..d86c839019c 100644 --- a/tests/components/squeezebox/snapshots/test_media_player.ambr +++ b/tests/components/squeezebox/snapshots/test_media_player.ambr @@ -12,7 +12,7 @@ ), }), 'disabled_by': None, - 'entry_type': , + 'entry_type': None, 'hw_version': None, 'id': , 'identifiers': set({ @@ -32,7 +32,48 @@ 'primary_config_entry': , 'serial_number': None, 'suggested_area': None, - 'sw_version': None, + 'sw_version': '', + 'via_device_id': , + }) +# --- +# name: test_device_registry_server_merged + DeviceRegistryEntrySnapshot({ + 'area_id': None, + 'config_entries': , + 'config_entries_subentries': , + 'configuration_url': None, + 'connections': set({ + tuple( + 'mac', + 'ff:ee:dd:cc:bb:aa', + ), + }), + 'disabled_by': None, + 'entry_type': , + 'hw_version': None, + 'id': , + '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': , + 'serial_number': None, + 'suggested_area': None, + 'sw_version': '', 'via_device_id': , }) # --- diff --git a/tests/components/squeezebox/snapshots/test_switch.ambr b/tests/components/squeezebox/snapshots/test_switch.ambr index 275fc26baa7..6d53eb38021 100644 --- a/tests/components/squeezebox/snapshots/test_switch.ambr +++ b/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': , - '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': , - 'entity_id': 'switch.test_player_alarm_1', + 'entity_id': 'switch.none_alarm_1', 'last_changed': , 'last_reported': , 'last_updated': , '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': , - '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': , - 'entity_id': 'switch.test_player_alarms_enabled', + 'entity_id': 'switch.none_alarms_enabled', 'last_changed': , 'last_reported': , 'last_updated': , diff --git a/tests/components/squeezebox/test_button.py b/tests/components/squeezebox/test_button.py index 16ced65be61..53c4e9ef626 100644 --- a/tests/components/squeezebox/test_button.py +++ b/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, ) diff --git a/tests/components/squeezebox/test_media_player.py b/tests/components/squeezebox/test_media_player.py index f71a7db23ba..e1f480e33a0 100644 --- a/tests/components/squeezebox/test_media_player.py +++ b/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, diff --git a/tests/components/squeezebox/test_switch.py b/tests/components/squeezebox/test_switch.py index e4c8c3b5e4d..2e6e9bafeb0 100644 --- a/tests/components/squeezebox/test_switch.py +++ b/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)