Post review comments on APsystems (#117504)

* Cleanup for apsystems and fix for strings

* Migrate to typed ConfigEntry Data for apsystems

* Improve strings for apsystems

* Improve config flow tests for apsystems by cleaning up fixtures

* Do not use Dataclass for Config Entry Typing

* Improve translations for apsystems by using sentence case and removing an apostrophe

* Rename test fixture and remove unnecessary comment in tests from apsystems

* Remove default override with default in coordinator from apsystems

.coveragerc

@@ -82,7 +82,6 @@ omit =
     homeassistant/components/aprilaire/coordinator.py
     homeassistant/components/aprilaire/entity.py
     homeassistant/components/apsystems/__init__.py
-    homeassistant/components/apsystems/const.py
     homeassistant/components/apsystems/coordinator.py
     homeassistant/components/apsystems/sensor.py
     homeassistant/components/aqualogic/*

homeassistant/components/apsystems/__init__.py

@@ -2,8 +2,6 @@
 
 from __future__ import annotations
 
-import logging
-
 from APsystemsEZ1 import APsystemsEZ1M
 
 from homeassistant.config_entries import ConfigEntry
@@ -12,18 +10,17 @@ from homeassistant.core import HomeAssistant
 
 from .coordinator import ApSystemsDataCoordinator
 
-_LOGGER = logging.getLogger(__name__)
-
 PLATFORMS: list[Platform] = [Platform.SENSOR]
 
+ApsystemsConfigEntry = ConfigEntry[ApSystemsDataCoordinator]
 
-async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
+
+async def async_setup_entry(hass: HomeAssistant, entry: ApsystemsConfigEntry) -> bool:
     """Set up this integration using UI."""
-    entry.runtime_data = {}
     api = APsystemsEZ1M(ip_address=entry.data[CONF_IP_ADDRESS], timeout=8)
     coordinator = ApSystemsDataCoordinator(hass, api)
     await coordinator.async_config_entry_first_refresh()
-    entry.runtime_data = {"COORDINATOR": coordinator}
+    entry.runtime_data = coordinator
     await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS)
 
     return True

homeassistant/components/apsystems/config_flow.py

@@ -1,14 +1,16 @@
 """The config_flow for APsystems local API integration."""
 
-from aiohttp import client_exceptions
+from typing import Any
+
+from aiohttp.client_exceptions import ClientConnectionError
 from APsystemsEZ1 import APsystemsEZ1M
 import voluptuous as vol
 
-from homeassistant import config_entries
+from homeassistant.config_entries import ConfigFlow, ConfigFlowResult
 from homeassistant.const import CONF_IP_ADDRESS
 from homeassistant.helpers.aiohttp_client import async_get_clientsession
 
-from .const import DOMAIN, LOGGER
+from .const import DOMAIN
 
 DATA_SCHEMA = vol.Schema(
     {
@@ -17,35 +19,34 @@ DATA_SCHEMA = vol.Schema(
 )
 
 
-class APsystemsLocalAPIFlow(config_entries.ConfigFlow, domain=DOMAIN):
+class APsystemsLocalAPIFlow(ConfigFlow, domain=DOMAIN):
     """Config flow for Apsystems local."""
 
     VERSION = 1
 
     async def async_step_user(
-        self,
+        self, user_input: dict[str, Any] | None = None
-        user_input: dict | None = None,
+    ) -> ConfigFlowResult:
-    ) -> config_entries.ConfigFlowResult:
         """Handle a flow initialized by the user."""
-        _errors = {}
+        errors = {}
-        session = async_get_clientsession(self.hass, False)
 
         if user_input is not None:
+            session = async_get_clientsession(self.hass, False)
+            api = APsystemsEZ1M(user_input[CONF_IP_ADDRESS], session=session)
             try:
-                session = async_get_clientsession(self.hass, False)
-                api = APsystemsEZ1M(user_input[CONF_IP_ADDRESS], session=session)
                 device_info = await api.get_device_info()
-                await self.async_set_unique_id(device_info.deviceId)
+            except (TimeoutError, ClientConnectionError):
-            except (TimeoutError, client_exceptions.ClientConnectionError) as exception:
+                errors["base"] = "cannot_connect"
-                LOGGER.warning(exception)
-                _errors["base"] = "connection_refused"
             else:
+                await self.async_set_unique_id(device_info.deviceId)
+                self._abort_if_unique_id_configured()
                 return self.async_create_entry(
                     title="Solar",
                     data=user_input,
                 )
+
         return self.async_show_form(
             step_id="user",
             data_schema=DATA_SCHEMA,
-            errors=_errors,
+            errors=errors,
         )

homeassistant/components/apsystems/coordinator.py

@@ -3,35 +3,27 @@
 from __future__ import annotations
 
 from datetime import timedelta
-import logging
 
 from APsystemsEZ1 import APsystemsEZ1M, ReturnOutputData
 
 from homeassistant.core import HomeAssistant
 from homeassistant.helpers.update_coordinator import DataUpdateCoordinator
 
-_LOGGER = logging.getLogger(__name__)
+from .const import LOGGER
 
 
-class InverterNotAvailable(Exception):
+class ApSystemsDataCoordinator(DataUpdateCoordinator[ReturnOutputData]):
-    """Error used when Device is offline."""
-
-
-class ApSystemsDataCoordinator(DataUpdateCoordinator):
     """Coordinator used for all sensors."""
 
     def __init__(self, hass: HomeAssistant, api: APsystemsEZ1M) -> None:
         """Initialize my coordinator."""
         super().__init__(
             hass,
-            _LOGGER,
+            LOGGER,
-            # Name of the data. For logging purposes.
             name="APSystems Data",
-            # Polling interval. Will only be polled if there are subscribers.
             update_interval=timedelta(seconds=12),
         )
         self.api = api
-        self.always_update = True
 
     async def _async_update_data(self) -> ReturnOutputData:
         return await self.api.get_output_data()

homeassistant/components/apsystems/manifest.json

@@ -3,11 +3,7 @@
   "name": "APsystems",
   "codeowners": ["@mawoka-myblock", "@SonnenladenGmbH"],
   "config_flow": true,
-  "dependencies": [],
   "documentation": "https://www.home-assistant.io/integrations/apsystems",
-  "homekit": {},
   "iot_class": "local_polling",
-  "requirements": ["apsystems-ez1==1.3.1"],
+  "requirements": ["apsystems-ez1==1.3.1"]
-  "ssdp": [],
-  "zeroconf": []
 }

homeassistant/components/apsystems/sensor.py

@@ -7,20 +7,22 @@ from dataclasses import dataclass
 
 from APsystemsEZ1 import ReturnOutputData
 
-from homeassistant import config_entries
 from homeassistant.components.sensor import (
     SensorDeviceClass,
     SensorEntity,
     SensorEntityDescription,
     SensorStateClass,
+    StateType,
 )
+from homeassistant.config_entries import ConfigEntry
 from homeassistant.const import UnitOfEnergy, UnitOfPower
-from homeassistant.core import HomeAssistant, callback
+from homeassistant.core import HomeAssistant
 from homeassistant.helpers.device_registry import DeviceInfo
 from homeassistant.helpers.entity_platform import AddEntitiesCallback
 from homeassistant.helpers.typing import DiscoveryInfoType
 from homeassistant.helpers.update_coordinator import CoordinatorEntity
 
+from .const import DOMAIN
 from .coordinator import ApSystemsDataCoordinator
 
 
@@ -109,23 +111,23 @@ SENSORS: tuple[ApsystemsLocalApiSensorDescription, ...] = (
 
 async def async_setup_entry(
     hass: HomeAssistant,
-    config_entry: config_entries.ConfigEntry,
+    config_entry: ConfigEntry,
     add_entities: AddEntitiesCallback,
     discovery_info: DiscoveryInfoType | None = None,
 ) -> None:
     """Set up the sensor platform."""
     config = config_entry.runtime_data
-    coordinator = config["COORDINATOR"]
+    device_id = config_entry.unique_id
-    device_name = config_entry.title
+    assert device_id
-    device_id: str = config_entry.unique_id  # type: ignore[assignment]
 
     add_entities(
-        ApSystemsSensorWithDescription(coordinator, desc, device_name, device_id)
+        ApSystemsSensorWithDescription(config, desc, device_id) for desc in SENSORS
-        for desc in SENSORS
     )
 
 
-class ApSystemsSensorWithDescription(CoordinatorEntity, SensorEntity):
+class ApSystemsSensorWithDescription(
+    CoordinatorEntity[ApSystemsDataCoordinator], SensorEntity
+):
     """Base sensor to be used with description."""
 
     entity_description: ApsystemsLocalApiSensorDescription
@@ -134,32 +136,20 @@ class ApSystemsSensorWithDescription(CoordinatorEntity, SensorEntity):
         self,
         coordinator: ApSystemsDataCoordinator,
         entity_description: ApsystemsLocalApiSensorDescription,
-        device_name: str,
         device_id: str,
     ) -> None:
         """Initialize the sensor."""
         super().__init__(coordinator)
         self.entity_description = entity_description
-        self._device_name = device_name
-        self._device_id = device_id
         self._attr_unique_id = f"{device_id}_{entity_description.key}"
-
+        self._attr_device_info = DeviceInfo(
-    @property
+            identifiers={(DOMAIN, device_id)},
-    def device_info(self) -> DeviceInfo:
+            serial_number=device_id,
-        """Get the DeviceInfo."""
-        return DeviceInfo(
-            identifiers={("apsystems", self._device_id)},
-            name=self._device_name,
-            serial_number=self._device_id,
             manufacturer="APsystems",
             model="EZ1-M",
         )
 
-    @callback
+    @property
-    def _handle_coordinator_update(self) -> None:
+    def native_value(self) -> StateType:
-        if self.coordinator.data is None:
+        """Return value of sensor."""
-            return  # type: ignore[unreachable]
+        return self.entity_description.value_fn(self.coordinator.data)
-        self._attr_native_value = self.entity_description.value_fn(
-            self.coordinator.data
-        )
-        self.async_write_ha_state()

homeassistant/components/apsystems/strings.json

@@ -3,19 +3,28 @@
     "step": {
       "user": {
         "data": {
-          "host": "[%key:common::config_flow::data::host%]",
+          "ip_address": "[%key:common::config_flow::data::ip%]"
-          "username": "[%key:common::config_flow::data::username%]",
-          "password": "[%key:common::config_flow::data::password%]"
         }
       }
     },
     "error": {
-      "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]",
+      "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]"
-      "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]",
-      "unknown": "[%key:common::config_flow::error::unknown%]"
     },
     "abort": {
       "already_configured": "[%key:common::config_flow::abort::already_configured_device%]"
     }
+  },
+  "entity": {
+    "sensor": {
+      "total_power": { "name": "Total power" },
+      "total_power_p1": { "name": "Power of P1" },
+      "total_power_p2": { "name": "Power of P2" },
+      "lifetime_production": { "name": "Total lifetime production" },
+      "lifetime_production_p1": { "name": "Lifetime production of P1" },
+      "lifetime_production_p2": { "name": "Lifetime production of P2" },
+      "today_production": { "name": "Production of today" },
+      "today_production_p1": { "name": "Production of today from P1" },
+      "today_production_p2": { "name": "Production of today from P2" }
+    }
   }
 }

tests/components/apsystems/conftest.py

@@ -1,7 +1,7 @@
 """Common fixtures for the APsystems Local API tests."""
 
 from collections.abc import Generator
-from unittest.mock import AsyncMock, patch
+from unittest.mock import AsyncMock, MagicMock, patch
 
 import pytest
 
@@ -14,3 +14,16 @@ def mock_setup_entry() -> Generator[AsyncMock, None, None]:
         return_value=True,
     ) as mock_setup_entry:
         yield mock_setup_entry
+
+
+@pytest.fixture
+def mock_apsystems():
+    """Override APsystemsEZ1M.get_device_info() to return MY_SERIAL_NUMBER as the serial number."""
+    ret_data = MagicMock()
+    ret_data.deviceId = "MY_SERIAL_NUMBER"
+    with patch(
+        "homeassistant.components.apsystems.config_flow.APsystemsEZ1M",
+        return_value=AsyncMock(),
+    ) as mock_api:
+        mock_api.return_value.get_device_info.return_value = ret_data
+        yield mock_api

tests/components/apsystems/test_config_flow.py

@@ -1,97 +1,77 @@
 """Test the APsystems Local API config flow."""
 
-from unittest.mock import AsyncMock, MagicMock, patch
+from unittest.mock import AsyncMock
 
-from homeassistant import config_entries
 from homeassistant.components.apsystems.const import DOMAIN
 from homeassistant.config_entries import SOURCE_USER
 from homeassistant.const import CONF_IP_ADDRESS
 from homeassistant.core import HomeAssistant
 from homeassistant.data_entry_flow import FlowResultType
 
+from tests.common import MockConfigEntry
+
+
+async def test_form_create_success(
+    hass: HomeAssistant, mock_setup_entry, mock_apsystems
+) -> None:
+    """Test we handle creatinw with success."""
+    result = await hass.config_entries.flow.async_init(
+        DOMAIN,
+        context={"source": SOURCE_USER},
+        data={
+            CONF_IP_ADDRESS: "127.0.0.1",
+        },
+    )
+    assert result["result"].unique_id == "MY_SERIAL_NUMBER"
+    assert result.get("type") is FlowResultType.CREATE_ENTRY
+    assert result["data"].get(CONF_IP_ADDRESS) == "127.0.0.1"
+
 
 async def test_form_cannot_connect_and_recover(
-    hass: HomeAssistant, mock_setup_entry
+    hass: HomeAssistant, mock_apsystems: AsyncMock, mock_setup_entry
 ) -> None:
     """Test we handle cannot connect error."""
+
+    mock_apsystems.return_value.get_device_info.side_effect = TimeoutError
     result = await hass.config_entries.flow.async_init(
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
+        DOMAIN,
+        context={"source": SOURCE_USER},
+        data={
+            CONF_IP_ADDRESS: "127.0.0.2",
+        },
     )
-    with patch(
-        "homeassistant.components.apsystems.config_flow.APsystemsEZ1M",
-        return_value=AsyncMock(),
-    ) as mock_api:
-        mock_api.side_effect = TimeoutError
-        result = await hass.config_entries.flow.async_init(
-            DOMAIN,
-            context={"source": SOURCE_USER},
-            data={
-                CONF_IP_ADDRESS: "127.0.0.2",
-            },
-        )
 
-    assert result["type"] == FlowResultType.FORM
+    assert result["type"] is FlowResultType.FORM
-    assert result["errors"] == {"base": "connection_refused"}
+    assert result["errors"] == {"base": "cannot_connect"}
 
-    # Make sure the config flow tests finish with either an
+    mock_apsystems.return_value.get_device_info.side_effect = None
-    # FlowResultType.CREATE_ENTRY or FlowResultType.ABORT so
+
-    # we can show the config flow is able to recover from an error.
+    result2 = await hass.config_entries.flow.async_configure(
-    with patch(
+        result["flow_id"],
-        "homeassistant.components.apsystems.config_flow.APsystemsEZ1M",
+        {
-        return_value=AsyncMock(),
+            CONF_IP_ADDRESS: "127.0.0.1",
-    ) as mock_api:
+        },
-        ret_data = MagicMock()
+    )
-        ret_data.deviceId = "MY_SERIAL_NUMBER"
+    assert result2["result"].unique_id == "MY_SERIAL_NUMBER"
-        mock_api.return_value.get_device_info = AsyncMock(return_value=ret_data)
+    assert result2.get("type") is FlowResultType.CREATE_ENTRY
-        result2 = await hass.config_entries.flow.async_configure(
+    assert result2["data"].get(CONF_IP_ADDRESS) == "127.0.0.1"
-            result["flow_id"],
-            {
-                CONF_IP_ADDRESS: "127.0.0.1",
-            },
-        )
-        assert result2["result"].unique_id == "MY_SERIAL_NUMBER"
-        assert result2.get("type") == FlowResultType.CREATE_ENTRY
-        assert result2["data"].get(CONF_IP_ADDRESS) == "127.0.0.1"
 
 
-async def test_form_cannot_connect(hass: HomeAssistant, mock_setup_entry) -> None:
+async def test_form_unique_id_already_configured(
+    hass: HomeAssistant, mock_setup_entry, mock_apsystems
+) -> None:
     """Test we handle cannot connect error."""
-    result = await hass.config_entries.flow.async_init(
+    entry = MockConfigEntry(
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
+        domain=DOMAIN, data={CONF_IP_ADDRESS: "127.0.0.2"}, unique_id="MY_SERIAL_NUMBER"
     )
-    with patch(
+    entry.add_to_hass(hass)
-        "homeassistant.components.apsystems.config_flow.APsystemsEZ1M",
-        return_value=AsyncMock(),
-    ) as mock_api:
-        mock_api.side_effect = TimeoutError
-        result = await hass.config_entries.flow.async_init(
-            DOMAIN,
-            context={"source": SOURCE_USER},
-            data={
-                CONF_IP_ADDRESS: "127.0.0.2",
-            },
-        )
 
-    assert result["type"] == FlowResultType.FORM
+    result = await hass.config_entries.flow.async_init(
-    assert result["errors"] == {"base": "connection_refused"}
+        DOMAIN,
-
+        context={"source": SOURCE_USER},
-
+        data={
-async def test_form_create_success(hass: HomeAssistant, mock_setup_entry) -> None:
+            CONF_IP_ADDRESS: "127.0.0.2",
-    """Test we handle creatinw with success."""
+        },
-    with patch(
+    )
-        "homeassistant.components.apsystems.config_flow.APsystemsEZ1M",
+    assert result["reason"] == "already_configured"
-        return_value=AsyncMock(),
+    assert result.get("type") is FlowResultType.ABORT
-    ) as mock_api:
-        ret_data = MagicMock()
-        ret_data.deviceId = "MY_SERIAL_NUMBER"
-        mock_api.return_value.get_device_info = AsyncMock(return_value=ret_data)
-        result = await hass.config_entries.flow.async_init(
-            DOMAIN,
-            context={"source": SOURCE_USER},
-            data={
-                CONF_IP_ADDRESS: "127.0.0.1",
-            },
-        )
-        assert result["result"].unique_id == "MY_SERIAL_NUMBER"
-        assert result.get("type") == FlowResultType.CREATE_ENTRY
-        assert result["data"].get(CONF_IP_ADDRESS) == "127.0.0.1"