Handle errors retrieving Ondilo data and bump ondilo to 0.5.0 (#115926)

* Bump ondilo to 0.5.0 and handle errors retrieving data

* Bump ondilo to 0.5.0 and handle errors retrieving data

* Updated ruff recommendation

* Refactor

* Refactor

* Added exception log and updated call to update data

* Updated test cases to test through state machine

* Updated test cases

* Updated test cases after comments

* REnamed file

---------

Co-authored-by: Joostlek <joostlek@outlook.com>

homeassistant/components/ondilo_ico/api.py

@@ -2,7 +2,6 @@
 
 from asyncio import run_coroutine_threadsafe
 import logging
-from typing import Any
 
 from ondilo import Ondilo
 
@@ -36,17 +35,3 @@ class OndiloClient(Ondilo):
         ).result()
 
         return self.session.token
-
-    def get_all_pools_data(self) -> list[dict[str, Any]]:
-        """Fetch pools and add pool details and last measures to pool data."""
-
-        pools = self.get_pools()
-        for pool in pools:
-            _LOGGER.debug(
-                "Retrieving data for pool/spa: %s, id: %d", pool["name"], pool["id"]
-            )
-            pool["ICO"] = self.get_ICO_details(pool["id"])
-            pool["sensors"] = self.get_last_pool_measures(pool["id"])
-            _LOGGER.debug("Retrieved the following sensors data: %s", pool["sensors"])
-
-        return pools

homeassistant/components/ondilo_ico/coordinator.py

@@ -31,7 +31,36 @@ class OndiloIcoCoordinator(DataUpdateCoordinator[list[dict[str, Any]]]):
     async def _async_update_data(self) -> list[dict[str, Any]]:
         """Fetch data from API endpoint."""
         try:
-            return await self.hass.async_add_executor_job(self.api.get_all_pools_data)
+            return await self.hass.async_add_executor_job(self._update_data)
 
         except OndiloError as err:
+            _LOGGER.exception("Error getting pools")
             raise UpdateFailed(f"Error communicating with API: {err}") from err
+
+    def _update_data(self) -> list[dict[str, Any]]:
+        """Fetch data from API endpoint."""
+        res = []
+        pools = self.api.get_pools()
+        _LOGGER.debug("Pools: %s", pools)
+        for pool in pools:
+            try:
+                ico = self.api.get_ICO_details(pool["id"])
+                if not ico:
+                    _LOGGER.debug(
+                        "The pool id %s does not have any ICO attached", pool["id"]
+                    )
+                    continue
+                sensors = self.api.get_last_pool_measures(pool["id"])
+            except OndiloError:
+                _LOGGER.exception("Error communicating with API for %s", pool["id"])
+                continue
+            res.append(
+                {
+                    **pool,
+                    "ICO": ico,
+                    "sensors": sensors,
+                }
+            )
+        if not res:
+            raise UpdateFailed("No data available")
+        return res

homeassistant/components/ondilo_ico/manifest.json

@@ -5,7 +5,8 @@
   "config_flow": true,
   "dependencies": ["auth"],
   "documentation": "https://www.home-assistant.io/integrations/ondilo_ico",
+  "integration_type": "hub",
   "iot_class": "cloud_polling",
   "loggers": ["ondilo"],
-  "requirements": ["ondilo==0.4.0"]
+  "requirements": ["ondilo==0.5.0"]
 }

requirements_all.txt

@@ -1450,7 +1450,7 @@ ollama-hass==0.1.7
 omnilogic==0.4.5
 
 # homeassistant.components.ondilo_ico
-ondilo==0.4.0
+ondilo==0.5.0
 
 # homeassistant.components.onkyo
 onkyo-eiscp==1.2.7

requirements_test_all.txt

@@ -1165,7 +1165,7 @@ ollama-hass==0.1.7
 omnilogic==0.4.5
 
 # homeassistant.components.ondilo_ico
-ondilo==0.4.0
+ondilo==0.5.0
 
 # homeassistant.components.onvif
 onvif-zeep-async==3.1.12

tests/components/ondilo_ico/__init__.py

@@ -1 +1,17 @@
 """Tests for the Ondilo ICO integration."""
+
+from unittest.mock import MagicMock
+
+from homeassistant.core import HomeAssistant
+
+from tests.common import MockConfigEntry
+
+
+async def setup_integration(
+    hass: HomeAssistant, config_entry: MockConfigEntry, mock_ondilo_client: MagicMock
+) -> None:
+    """Fixture for setting up the component."""
+    config_entry.add_to_hass(hass)
+
+    await hass.config_entries.async_setup(config_entry.entry_id)
+    await hass.async_block_till_done()

tests/components/ondilo_ico/conftest.py

@@ -0,0 +1,84 @@
+"""Provide basic Ondilo fixture."""
+
+from collections.abc import Generator
+from typing import Any
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+from homeassistant.components.ondilo_ico.const import DOMAIN
+
+from tests.common import (
+    MockConfigEntry,
+    load_json_array_fixture,
+    load_json_object_fixture,
+)
+
+
+@pytest.fixture(name="config_entry")
+def mock_config_entry() -> MockConfigEntry:
+    """Mock a config entry."""
+    return MockConfigEntry(
+        domain=DOMAIN,
+        title="Ondilo ICO",
+        data={"auth_implementation": DOMAIN, "token": {"access_token": "fake_token"}},
+    )
+
+
+@pytest.fixture
+def mock_ondilo_client(
+    two_pools: list[dict[str, Any]],
+    ico_details1: dict[str, Any],
+    ico_details2: dict[str, Any],
+    last_measures: list[dict[str, Any]],
+) -> Generator[MagicMock, None, None]:
+    """Mock a Homeassistant Ondilo client."""
+    with (
+        patch(
+            "homeassistant.components.ondilo_ico.api.OndiloClient",
+            autospec=True,
+        ) as mock_ondilo,
+    ):
+        client = mock_ondilo.return_value
+        client.get_pools.return_value = two_pools
+        client.get_ICO_details.side_effect = [ico_details1, ico_details2]
+        client.get_last_pool_measures.return_value = last_measures
+        yield client
+
+
+@pytest.fixture(scope="session")
+def pool1() -> list[dict[str, Any]]:
+    """First pool description."""
+    return [load_json_object_fixture("pool1.json", DOMAIN)]
+
+
+@pytest.fixture(scope="session")
+def pool2() -> list[dict[str, Any]]:
+    """Second pool description."""
+    return [load_json_object_fixture("pool2.json", DOMAIN)]
+
+
+@pytest.fixture(scope="session")
+def ico_details1() -> dict[str, Any]:
+    """ICO details of first pool."""
+    return load_json_object_fixture("ico_details1.json", DOMAIN)
+
+
+@pytest.fixture(scope="session")
+def ico_details2() -> dict[str, Any]:
+    """ICO details of second pool."""
+    return load_json_object_fixture("ico_details2.json", DOMAIN)
+
+
+@pytest.fixture(scope="session")
+def last_measures() -> list[dict[str, Any]]:
+    """Pool measurements."""
+    return load_json_array_fixture("last_measures.json", DOMAIN)
+
+
+@pytest.fixture(scope="session")
+def two_pools(
+    pool1: list[dict[str, Any]], pool2: list[dict[str, Any]]
+) -> list[dict[str, Any]]:
+    """Two pools description."""
+    return [*pool1, *pool2]

tests/components/ondilo_ico/fixtures/ico_details1.json

@@ -0,0 +1,5 @@
+{
+  "uuid": "111112222233333444445555",
+  "serial_number": "W1122333044455",
+  "sw_version": "1.7.1-stable"
+}

tests/components/ondilo_ico/fixtures/ico_details2.json

@@ -0,0 +1,5 @@
+{
+  "uuid": "222223333344444555566666",
+  "serial_number": "W2233304445566",
+  "sw_version": "1.7.1-stable"
+}

tests/components/ondilo_ico/fixtures/last_measures.json

@@ -0,0 +1,51 @@
+[
+  {
+    "data_type": "temperature",
+    "value": 19,
+    "value_time": "2024-01-01 01:00:00",
+    "is_valid": true,
+    "exclusion_reason": null
+  },
+  {
+    "data_type": "ph",
+    "value": 9.29,
+    "value_time": "2024-01-01 01:00:00",
+    "is_valid": true,
+    "exclusion_reason": null
+  },
+  {
+    "data_type": "orp",
+    "value": 647,
+    "value_time": "2024-01-01 01:00:00",
+    "is_valid": true,
+    "exclusion_reason": null
+  },
+  {
+    "data_type": "salt",
+    "value": null,
+    "value_time": "2024-01-01 01:00:00",
+    "is_valid": true,
+    "exclusion_reason": null
+  },
+  {
+    "data_type": "battery",
+    "value": 50,
+    "value_time": "2024-01-01 01:00:00",
+    "is_valid": true,
+    "exclusion_reason": null
+  },
+  {
+    "data_type": "tds",
+    "value": 845,
+    "value_time": "2024-01-01 01:00:00",
+    "is_valid": true,
+    "exclusion_reason": null
+  },
+  {
+    "data_type": "rssi",
+    "value": 60,
+    "value_time": "2024-01-01 01:00:00",
+    "is_valid": true,
+    "exclusion_reason": null
+  }
+]

tests/components/ondilo_ico/fixtures/pool1.json

@@ -0,0 +1,19 @@
+{
+  "id": 1,
+  "name": "Pool 1",
+  "type": "outdoor_inground_pool",
+  "volume": 100,
+  "disinfection": {
+    "primary": "chlorine",
+    "secondary": { "uv_sanitizer": false, "ozonator": false }
+  },
+  "address": {
+    "street": "1 Rue de Paris",
+    "zipcode": "75000",
+    "city": "Paris",
+    "country": "France",
+    "latitude": 48.861783,
+    "longitude": 2.337421
+  },
+  "updated_at": "2024-01-01T01:00:00+0000"
+}

tests/components/ondilo_ico/fixtures/pool2.json

@@ -0,0 +1,19 @@
+{
+  "id": 2,
+  "name": "Pool 2",
+  "type": "outdoor_inground_pool",
+  "volume": 120,
+  "disinfection": {
+    "primary": "chlorine",
+    "secondary": { "uv_sanitizer": false, "ozonator": false }
+  },
+  "address": {
+    "street": "1 Rue de Paris",
+    "zipcode": "75000",
+    "city": "Paris",
+    "country": "France",
+    "latitude": 48.861783,
+    "longitude": 2.337421
+  },
+  "updated_at": "2024-01-01T01:00:00+0000"
+}

tests/components/ondilo_ico/test_init.py

@@ -0,0 +1,31 @@
+"""Test Ondilo ICO initialization."""
+
+from typing import Any
+from unittest.mock import MagicMock
+
+from homeassistant.config_entries import ConfigEntryState
+from homeassistant.core import HomeAssistant
+
+from . import setup_integration
+
+from tests.common import MockConfigEntry
+
+
+async def test_init_with_no_ico_attached(
+    hass: HomeAssistant,
+    mock_ondilo_client: MagicMock,
+    config_entry: MockConfigEntry,
+    pool1: dict[str, Any],
+) -> None:
+    """Test if an ICO is not attached to a pool, then no sensor is created."""
+    # Only one pool, but no ICO attached
+    mock_ondilo_client.get_pools.return_value = pool1
+    mock_ondilo_client.get_ICO_details.side_effect = None
+    mock_ondilo_client.get_ICO_details.return_value = None
+    await setup_integration(hass, config_entry, mock_ondilo_client)
+
+    # No sensor should be created
+    assert len(hass.states.async_all()) == 0
+    # We should not have tried to retrieve pool measures
+    mock_ondilo_client.get_last_pool_measures.assert_not_called()
+    assert config_entry.state is ConfigEntryState.SETUP_RETRY

tests/components/ondilo_ico/test_sensor.py

@@ -0,0 +1,83 @@
+"""Test Ondilo ICO integration sensors."""
+
+from typing import Any
+from unittest.mock import MagicMock
+
+from ondilo import OndiloError
+
+from homeassistant.core import HomeAssistant
+
+from . import setup_integration
+
+from tests.common import MockConfigEntry
+
+
+async def test_can_get_pools_when_no_error(
+    hass: HomeAssistant,
+    mock_ondilo_client: MagicMock,
+    config_entry: MockConfigEntry,
+) -> None:
+    """Test that I can get all pools data when no error."""
+    await setup_integration(hass, config_entry, mock_ondilo_client)
+
+    # All sensors were created
+    assert len(hass.states.async_all()) == 14
+
+    # Check 2 of the sensors.
+    assert hass.states.get("sensor.pool_1_temperature").state == "19"
+    assert hass.states.get("sensor.pool_2_rssi").state == "60"
+
+
+async def test_no_ico_for_one_pool(
+    hass: HomeAssistant,
+    mock_ondilo_client: MagicMock,
+    config_entry: MockConfigEntry,
+    two_pools: list[dict[str, Any]],
+    ico_details2: dict[str, Any],
+    last_measures: list[dict[str, Any]],
+) -> None:
+    """Test if an ICO is not attached to a pool, then no sensor for that pool is created."""
+    mock_ondilo_client.get_pools.return_value = two_pools
+    mock_ondilo_client.get_ICO_details.side_effect = [None, ico_details2]
+
+    await setup_integration(hass, config_entry, mock_ondilo_client)
+    # Only the second pool is created
+    assert len(hass.states.async_all()) == 7
+    assert hass.states.get("sensor.pool_1_temperature") is None
+    assert hass.states.get("sensor.pool_2_rssi").state == next(
+        str(item["value"]) for item in last_measures if item["data_type"] == "rssi"
+    )
+
+
+async def test_error_retrieving_ico(
+    hass: HomeAssistant,
+    mock_ondilo_client: MagicMock,
+    config_entry: MockConfigEntry,
+    pool1: dict[str, Any],
+) -> None:
+    """Test if there's an error retrieving ICO data, then no sensor is created."""
+    mock_ondilo_client.get_pools.return_value = pool1
+    mock_ondilo_client.get_ICO_details.side_effect = OndiloError(400, "error")
+
+    await setup_integration(hass, config_entry, mock_ondilo_client)
+
+    # No sensor should be created
+    assert len(hass.states.async_all()) == 0
+
+
+async def test_error_retrieving_measures(
+    hass: HomeAssistant,
+    mock_ondilo_client: MagicMock,
+    config_entry: MockConfigEntry,
+    pool1: dict[str, Any],
+    ico_details1: dict[str, Any],
+) -> None:
+    """Test if there's an error retrieving measures of ICO, then no sensor is created."""
+    mock_ondilo_client.get_pools.return_value = pool1
+    mock_ondilo_client.get_ICO_details.return_value = ico_details1
+    mock_ondilo_client.get_last_pool_measures.side_effect = OndiloError(400, "error")
+
+    await setup_integration(hass, config_entry, mock_ondilo_client)
+
+    # No sensor should be created
+    assert len(hass.states.async_all()) == 0