From 1680adf15851a593bd641b6e65f14c8c726551ac Mon Sep 17 00:00:00 2001 From: Simone Chemelli Date: Mon, 3 Feb 2025 20:48:50 +0100 Subject: [PATCH] Add device cleanup to Vodafone Station (#116024) * add device cleanup * apply review comments * fix description * make cleanup automatic * . * rework approach based on IQS021 rule * add initial devices list from registry * use connections instead of identifiers * apply review comment * add some coordinator tests * one more test * cleanup tests * allign tests * apply review comment * removed sensor test * cleanup test * align test to latest code * typo * fix after rebase * introduce generic helper * apply some review comments * add comments to clarify design * apply latest review comment * ruff * improved coverage * more coverage * 100% helpers.py test coverage * improve test --------- Co-authored-by: J. Nick Koston --- .../vodafone_station/coordinator.py | 28 ++++++++ .../vodafone_station/device_tracker.py | 5 +- .../components/vodafone_station/helpers.py | 72 +++++++++++++++++++ tests/components/vodafone_station/conftest.py | 13 +++- tests/components/vodafone_station/const.py | 3 + .../snapshots/test_device_tracker.ambr | 63 ++++++++++++++-- .../snapshots/test_diagnostics.ambr | 6 ++ .../vodafone_station/test_coordinator.py | 68 ++++++++++++++++++ .../vodafone_station/test_device_tracker.py | 4 +- 9 files changed, 251 insertions(+), 11 deletions(-) create mode 100644 homeassistant/components/vodafone_station/helpers.py create mode 100644 tests/components/vodafone_station/test_coordinator.py diff --git a/homeassistant/components/vodafone_station/coordinator.py b/homeassistant/components/vodafone_station/coordinator.py index de794488040..b1f49349260 100644 --- a/homeassistant/components/vodafone_station/coordinator.py +++ b/homeassistant/components/vodafone_station/coordinator.py @@ -8,13 +8,16 @@ from typing import Any from aiovodafone import VodafoneStationDevice, VodafoneStationSercommApi, exceptions from homeassistant.components.device_tracker import DEFAULT_CONSIDER_HOME +from homeassistant.config_entries import ConfigEntry from homeassistant.core import HomeAssistant from homeassistant.exceptions import ConfigEntryAuthFailed +from homeassistant.helpers import device_registry as dr from homeassistant.helpers.device_registry import DeviceInfo from homeassistant.helpers.update_coordinator import DataUpdateCoordinator, UpdateFailed from homeassistant.util import dt as dt_util from .const import _LOGGER, DOMAIN, SCAN_INTERVAL +from .helpers import cleanup_device_tracker CONSIDER_HOME_SECONDS = DEFAULT_CONSIDER_HOME.total_seconds() @@ -39,6 +42,8 @@ class UpdateCoordinatorDataType: class VodafoneStationRouter(DataUpdateCoordinator[UpdateCoordinatorDataType]): """Queries router running Vodafone Station firmware.""" + config_entry: ConfigEntry + def __init__( self, hass: HomeAssistant, @@ -61,6 +66,17 @@ class VodafoneStationRouter(DataUpdateCoordinator[UpdateCoordinatorDataType]): name=f"{DOMAIN}-{host}-coordinator", update_interval=timedelta(seconds=SCAN_INTERVAL), ) + device_reg = dr.async_get(self.hass) + device_list = dr.async_entries_for_config_entry( + device_reg, self.config_entry.entry_id + ) + + self.previous_devices = { + connection[1].upper() + for device in device_list + for connection in device.connections + if connection[0] == dr.CONNECTION_NETWORK_MAC + } def _calculate_update_time_and_consider_home( self, device: VodafoneStationDevice, utc_point_in_time: datetime @@ -125,6 +141,18 @@ class VodafoneStationRouter(DataUpdateCoordinator[UpdateCoordinatorDataType]): ) for dev_info in (raw_data_devices).values() } + current_devices = set(data_devices) + _LOGGER.debug( + "Loaded current %s devices: %s", len(current_devices), current_devices + ) + if stale_devices := self.previous_devices - current_devices: + _LOGGER.debug( + "Found %s stale devices: %s", len(stale_devices), stale_devices + ) + await cleanup_device_tracker(self.hass, self.config_entry, data_devices) + + self.previous_devices = current_devices + return UpdateCoordinatorDataType(data_devices, data_sensors) @property diff --git a/homeassistant/components/vodafone_station/device_tracker.py b/homeassistant/components/vodafone_station/device_tracker.py index 3e4d7763bff..4af0b85e003 100644 --- a/homeassistant/components/vodafone_station/device_tracker.py +++ b/homeassistant/components/vodafone_station/device_tracker.py @@ -61,6 +61,7 @@ class VodafoneStationTracker(CoordinatorEntity[VodafoneStationRouter], ScannerEn """Representation of a Vodafone Station device.""" _attr_translation_key = "device_tracker" + _attr_has_entity_name = True mac_address: str def __init__( @@ -72,7 +73,9 @@ class VodafoneStationTracker(CoordinatorEntity[VodafoneStationRouter], ScannerEn mac = device_info.device.mac self._attr_mac_address = mac self._attr_unique_id = mac - self._attr_hostname = device_info.device.name or mac.replace(":", "_") + self._attr_hostname = self._attr_name = device_info.device.name or mac.replace( + ":", "_" + ) @property def _device_info(self) -> VodafoneStationDeviceInfo: diff --git a/homeassistant/components/vodafone_station/helpers.py b/homeassistant/components/vodafone_station/helpers.py new file mode 100644 index 00000000000..aa0fda3f6be --- /dev/null +++ b/homeassistant/components/vodafone_station/helpers.py @@ -0,0 +1,72 @@ +"""Vodafone Station helpers.""" + +from typing import Any + +from homeassistant.components.device_tracker import DOMAIN as DEVICE_TRACKER_DOMAIN +from homeassistant.config_entries import ConfigEntry +from homeassistant.core import HomeAssistant +from homeassistant.helpers import device_registry as dr, entity_registry as er + +from .const import _LOGGER + + +async def cleanup_device_tracker( + hass: HomeAssistant, config_entry: ConfigEntry, devices: dict[str, Any] +) -> None: + """Cleanup stale device tracker.""" + entity_reg: er.EntityRegistry = er.async_get(hass) + + entities_removed: bool = False + + device_hosts_macs: set[str] = set() + device_hosts_names: set[str] = set() + for mac, device_info in devices.items(): + device_hosts_macs.add(mac) + device_hosts_names.add(device_info.device.name) + + for entry in er.async_entries_for_config_entry(entity_reg, config_entry.entry_id): + if entry.domain != DEVICE_TRACKER_DOMAIN: + continue + entry_name = entry.name or entry.original_name + entry_host = entry_name.partition(" ")[0] if entry_name else None + entry_mac = entry.unique_id.partition("_")[0] + + # Some devices, mainly routers, allow to change the hostname of the connected devices. + # This can lead to entities no longer aligned to the device UI + if ( + entry_host + and entry_host in device_hosts_names + and entry_mac in device_hosts_macs + ): + _LOGGER.debug( + "Skipping entity %s [mac=%s, host=%s]", + entry_name, + entry_mac, + entry_host, + ) + continue + # Entity is removed so that at the next coordinator update + # the correct one will be created + _LOGGER.info("Removing entity: %s", entry_name) + entity_reg.async_remove(entry.entity_id) + entities_removed = True + + if entities_removed: + _async_remove_empty_devices(hass, entity_reg, config_entry) + + +def _async_remove_empty_devices( + hass: HomeAssistant, entity_reg: er.EntityRegistry, config_entry: ConfigEntry +) -> None: + """Remove devices with no entities.""" + + device_reg = dr.async_get(hass) + device_list = dr.async_entries_for_config_entry(device_reg, config_entry.entry_id) + for device_entry in device_list: + if not er.async_entries_for_device( + entity_reg, + device_entry.id, + include_disabled_entities=True, + ): + _LOGGER.info("Removing device: %s", device_entry.name) + device_reg.async_remove_device(device_entry.id) diff --git a/tests/components/vodafone_station/conftest.py b/tests/components/vodafone_station/conftest.py index 7763db5044a..a065a1e8065 100644 --- a/tests/components/vodafone_station/conftest.py +++ b/tests/components/vodafone_station/conftest.py @@ -8,7 +8,7 @@ import pytest from homeassistant.components.vodafone_station import DOMAIN from homeassistant.const import CONF_HOST, CONF_PASSWORD, CONF_USERNAME -from .const import DEVICE_1_MAC +from .const import DEVICE_1_HOST, DEVICE_1_MAC, DEVICE_2_MAC from tests.common import ( AsyncMock, @@ -48,11 +48,20 @@ def mock_vodafone_station_router() -> Generator[AsyncMock]: connected=True, connection_type="wifi", ip_address="192.168.1.10", - name="WifiDevice0", + name=DEVICE_1_HOST, mac=DEVICE_1_MAC, type="laptop", wifi="2.4G", ), + DEVICE_2_MAC: VodafoneStationDevice( + connected=False, + connection_type="lan", + ip_address="192.168.1.11", + name="LanDevice1", + mac=DEVICE_2_MAC, + type="desktop", + wifi="", + ), } router.get_sensor_data.return_value = load_json_object_fixture( "get_sensor_data.json", DOMAIN diff --git a/tests/components/vodafone_station/const.py b/tests/components/vodafone_station/const.py index 0f1ed2ba7da..cf6c274e5d5 100644 --- a/tests/components/vodafone_station/const.py +++ b/tests/components/vodafone_station/const.py @@ -1,3 +1,6 @@ """Common stuff for Vodafone Station tests.""" +DEVICE_1_HOST = "WifiDevice0" DEVICE_1_MAC = "xx:xx:xx:xx:xx:xx" +DEVICE_2_HOST = "LanDevice1" +DEVICE_2_MAC = "yy:yy:yy:yy:yy:yy" diff --git a/tests/components/vodafone_station/snapshots/test_device_tracker.ambr b/tests/components/vodafone_station/snapshots/test_device_tracker.ambr index 834c8b14459..e019ea73ab9 100644 --- a/tests/components/vodafone_station/snapshots/test_device_tracker.ambr +++ b/tests/components/vodafone_station/snapshots/test_device_tracker.ambr @@ -1,5 +1,5 @@ # serializer version: 1 -# name: test_all_entities[device_tracker.vodafone_station_xx_xx_xx_xx_xx_xx-entry] +# name: test_all_entities[device_tracker.landevice1-entry] EntityRegistryEntrySnapshot({ 'aliases': set({ }), @@ -11,8 +11,8 @@ 'disabled_by': None, 'domain': 'device_tracker', 'entity_category': , - 'entity_id': 'device_tracker.vodafone_station_xx_xx_xx_xx_xx_xx', - 'has_entity_name': False, + 'entity_id': 'device_tracker.landevice1', + 'has_entity_name': True, 'hidden_by': None, 'icon': None, 'id': , @@ -23,7 +23,57 @@ }), 'original_device_class': None, 'original_icon': None, - 'original_name': None, + 'original_name': 'LanDevice1', + 'platform': 'vodafone_station', + 'previous_unique_id': None, + 'supported_features': 0, + 'translation_key': 'device_tracker', + 'unique_id': 'yy:yy:yy:yy:yy:yy', + 'unit_of_measurement': None, + }) +# --- +# name: test_all_entities[device_tracker.landevice1-state] + StateSnapshot({ + 'attributes': ReadOnlyDict({ + 'friendly_name': 'LanDevice1', + 'host_name': 'LanDevice1', + 'ip': '192.168.1.11', + 'mac': 'yy:yy:yy:yy:yy:yy', + 'source_type': , + }), + 'context': , + 'entity_id': 'device_tracker.landevice1', + 'last_changed': , + 'last_reported': , + 'last_updated': , + 'state': 'not_home', + }) +# --- +# name: test_all_entities[device_tracker.wifidevice0-entry] + EntityRegistryEntrySnapshot({ + 'aliases': set({ + }), + 'area_id': None, + 'capabilities': None, + 'config_entry_id': , + 'device_class': None, + 'device_id': , + 'disabled_by': None, + 'domain': 'device_tracker', + 'entity_category': , + 'entity_id': 'device_tracker.wifidevice0', + 'has_entity_name': True, + 'hidden_by': None, + 'icon': None, + 'id': , + 'labels': set({ + }), + 'name': None, + 'options': dict({ + }), + 'original_device_class': None, + 'original_icon': None, + 'original_name': 'WifiDevice0', 'platform': 'vodafone_station', 'previous_unique_id': None, 'supported_features': 0, @@ -32,16 +82,17 @@ 'unit_of_measurement': None, }) # --- -# name: test_all_entities[device_tracker.vodafone_station_xx_xx_xx_xx_xx_xx-state] +# name: test_all_entities[device_tracker.wifidevice0-state] StateSnapshot({ 'attributes': ReadOnlyDict({ + 'friendly_name': 'WifiDevice0', 'host_name': 'WifiDevice0', 'ip': '192.168.1.10', 'mac': 'xx:xx:xx:xx:xx:xx', 'source_type': , }), 'context': , - 'entity_id': 'device_tracker.vodafone_station_xx_xx_xx_xx_xx_xx', + 'entity_id': 'device_tracker.wifidevice0', 'last_changed': , 'last_reported': , 'last_updated': , diff --git a/tests/components/vodafone_station/snapshots/test_diagnostics.ambr b/tests/components/vodafone_station/snapshots/test_diagnostics.ambr index c258b14dc2d..478080700cd 100644 --- a/tests/components/vodafone_station/snapshots/test_diagnostics.ambr +++ b/tests/components/vodafone_station/snapshots/test_diagnostics.ambr @@ -9,6 +9,12 @@ 'hostname': 'WifiDevice0', 'type': 'laptop', }), + dict({ + 'connected': False, + 'connection_type': 'lan', + 'hostname': 'LanDevice1', + 'type': 'desktop', + }), ]), 'last_exception': None, 'last_update success': True, diff --git a/tests/components/vodafone_station/test_coordinator.py b/tests/components/vodafone_station/test_coordinator.py new file mode 100644 index 00000000000..1a9470245c7 --- /dev/null +++ b/tests/components/vodafone_station/test_coordinator.py @@ -0,0 +1,68 @@ +"""Define tests for the Vodafone Station coordinator.""" + +import logging +from unittest.mock import AsyncMock + +from aiovodafone import VodafoneStationDevice +from freezegun.api import FrozenDateTimeFactory +import pytest + +from homeassistant.components.vodafone_station.const import DOMAIN, SCAN_INTERVAL +from homeassistant.core import HomeAssistant +from homeassistant.helpers import device_registry as dr + +from . import setup_integration +from .const import DEVICE_1_HOST, DEVICE_1_MAC, DEVICE_2_HOST, DEVICE_2_MAC + +from tests.common import MockConfigEntry, async_fire_time_changed + + +@pytest.mark.usefixtures("entity_registry_enabled_by_default") +async def test_coordinator_device_cleanup( + hass: HomeAssistant, + freezer: FrozenDateTimeFactory, + mock_vodafone_station_router: AsyncMock, + mock_config_entry: MockConfigEntry, + caplog: pytest.LogCaptureFixture, + device_registry: dr.DeviceRegistry, +) -> None: + """Test Device cleanup on coordinator update.""" + + caplog.set_level(logging.DEBUG) + await setup_integration(hass, mock_config_entry) + + device = device_registry.async_get_or_create( + config_entry_id=mock_config_entry.entry_id, + identifiers={(DOMAIN, DEVICE_1_MAC)}, + name=DEVICE_1_HOST, + ) + assert device is not None + + device_tracker = f"device_tracker.{DEVICE_1_HOST}" + + state = hass.states.get(device_tracker) + assert state is not None + + mock_vodafone_station_router.get_devices_data.return_value = { + DEVICE_2_MAC: VodafoneStationDevice( + connected=True, + connection_type="lan", + ip_address="192.168.1.11", + name=DEVICE_2_HOST, + mac=DEVICE_2_MAC, + type="desktop", + wifi="", + ), + } + + freezer.tick(SCAN_INTERVAL) + async_fire_time_changed(hass) + await hass.async_block_till_done(wait_background_tasks=True) + + state = hass.states.get(device_tracker) + assert state is None + assert f"Skipping entity {DEVICE_2_HOST}" in caplog.text + + device = device_registry.async_get_device(identifiers={(DOMAIN, DEVICE_1_MAC)}) + assert device is None + assert f"Removing device: {DEVICE_1_HOST}" in caplog.text diff --git a/tests/components/vodafone_station/test_device_tracker.py b/tests/components/vodafone_station/test_device_tracker.py index 5133d0da980..e172fa76de5 100644 --- a/tests/components/vodafone_station/test_device_tracker.py +++ b/tests/components/vodafone_station/test_device_tracker.py @@ -13,7 +13,7 @@ from homeassistant.core import HomeAssistant from homeassistant.helpers import entity_registry as er from . import setup_integration -from .const import DEVICE_1_MAC +from .const import DEVICE_1_HOST, DEVICE_1_MAC from tests.common import MockConfigEntry, async_fire_time_changed, snapshot_platform @@ -45,7 +45,7 @@ async def test_consider_home( """Test if device is considered not_home when disconnected.""" await setup_integration(hass, mock_config_entry) - device_tracker = "device_tracker.vodafone_station_xx_xx_xx_xx_xx_xx" + device_tracker = f"device_tracker.{DEVICE_1_HOST}" state = hass.states.get(device_tracker) assert state