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 <nick@koston.org>

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

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:

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)

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

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"

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': <EntityCategory.DIAGNOSTIC: 'diagnostic'>,
-    'entity_id': 'device_tracker.vodafone_station_xx_xx_xx_xx_xx_xx',
+    'entity_id': 'device_tracker.landevice1',
-    'has_entity_name': False,
+    'has_entity_name': True,
     'hidden_by': None,
     'icon': None,
     'id': <ANY>,
@@ -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': <SourceType.ROUTER: 'router'>,
+    }),
+    'context': <ANY>,
+    'entity_id': 'device_tracker.landevice1',
+    'last_changed': <ANY>,
+    'last_reported': <ANY>,
+    'last_updated': <ANY>,
+    'state': 'not_home',
+  })
+# ---
+# name: test_all_entities[device_tracker.wifidevice0-entry]
+  EntityRegistryEntrySnapshot({
+    'aliases': set({
+    }),
+    'area_id': None,
+    'capabilities': None,
+    'config_entry_id': <ANY>,
+    'device_class': None,
+    'device_id': <ANY>,
+    'disabled_by': None,
+    'domain': 'device_tracker',
+    'entity_category': <EntityCategory.DIAGNOSTIC: 'diagnostic'>,
+    'entity_id': 'device_tracker.wifidevice0',
+    'has_entity_name': True,
+    'hidden_by': None,
+    'icon': None,
+    'id': <ANY>,
+    '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': <SourceType.ROUTER: 'router'>,
     }),
     'context': <ANY>,
-    'entity_id': 'device_tracker.vodafone_station_xx_xx_xx_xx_xx_xx',
+    'entity_id': 'device_tracker.wifidevice0',
     'last_changed': <ANY>,
     'last_reported': <ANY>,
     'last_updated': <ANY>,

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,

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

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