Aurora abb improvements (#58504)

* Add type hints.

* Refactor AuroraDevice to AuroraDeviceEntity

* Refactor AuroraDevice to AuroraDeviceEntity

* Connection class is defined in manifest.

* Separate words with underscore in variable names

* Remove duplicated code.

* Remove unused "unknown" string

* Test import yaml when integration already setup

* Remove test already done in config_flow test

* Convert variable names to snake case

* Shorten AuroraDeviceEntity to AuroraEntity

* Add typing

* Remove unnecessary integration setup in test.

* Refactor "already_setup" to "already_configured"

* Use common string

* Reduce the amount of code in the try block.

* Fix merge

* Allow yaml setup to be deferred if no comms

* Properly setup all sensors for defered yaml setup.

* Apply suggestions from code review

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

* Add type hints.

* Refactor AuroraDevice to AuroraDeviceEntity

* Refactor AuroraDevice to AuroraDeviceEntity

* Connection class is defined in manifest.

* Separate words with underscore in variable names

* Remove duplicated code.

* Remove unused "unknown" string

* Test import yaml when integration already setup

* Remove test already done in config_flow test

* Convert variable names to snake case

* Shorten AuroraDeviceEntity to AuroraEntity

* Add typing

* Remove unnecessary integration setup in test.

* Refactor "already_setup" to "already_configured"

* Use common string

* Reduce the amount of code in the try block.

* Allow yaml setup to be deferred if no comms

* Properly setup all sensors for defered yaml setup.

* Code review: move line out of try block.

* Improve test coverage

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>

homeassistant/components/aurora_abb_powerone/__init__.py

@@ -25,12 +25,12 @@ PLATFORMS = ["sensor"]
 _LOGGER = logging.getLogger(__name__)
 
 
-async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry):
+async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
     """Set up Aurora ABB PowerOne from a config entry."""
 
     comport = entry.data[CONF_PORT]
     address = entry.data[CONF_ADDRESS]
-    serclient = AuroraSerialClient(address, comport, parity="N", timeout=1)
+    ser_client = AuroraSerialClient(address, comport, parity="N", timeout=1)
     # To handle yaml import attempts in darkeness, (re)try connecting only if
     # unique_id not yet assigned.
     if entry.unique_id is None:
@@ -67,19 +67,19 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry):
                     return False
             hass.config_entries.async_update_entry(entry, unique_id=new_id)
 
-    hass.data.setdefault(DOMAIN, {})[entry.unique_id] = serclient
+    hass.data.setdefault(DOMAIN, {})[entry.entry_id] = ser_client
     hass.config_entries.async_setup_platforms(entry, PLATFORMS)
 
     return True
 
 
-async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry):
+async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
     """Unload a config entry."""
 
     unload_ok = await hass.config_entries.async_unload_platforms(entry, PLATFORMS)
     # It should not be necessary to close the serial port because we close
     # it after every use in sensor.py, i.e. no need to do entry["client"].close()
     if unload_ok:
-        hass.data[DOMAIN].pop(entry.unique_id)
+        hass.data[DOMAIN].pop(entry.entry_id)
 
     return unload_ok

homeassistant/components/aurora_abb_powerone/aurora_device.py

@@ -1,11 +1,13 @@
 """Top level class for AuroraABBPowerOneSolarPV inverters and sensors."""
 from __future__ import annotations
 
+from collections.abc import Mapping
 import logging
+from typing import Any
 
 from aurorapy.client import AuroraSerialClient
 
-from homeassistant.helpers.entity import Entity
+from homeassistant.helpers.entity import DeviceInfo, Entity
 
 from .const import (
     ATTR_DEVICE_NAME,
@@ -20,10 +22,10 @@ from .const import (
 _LOGGER = logging.getLogger(__name__)
 
 
-class AuroraDevice(Entity):
+class AuroraEntity(Entity):
     """Representation of an Aurora ABB PowerOne device."""
 
-    def __init__(self, client: AuroraSerialClient, data) -> None:
+    def __init__(self, client: AuroraSerialClient, data: Mapping[str, Any]) -> None:
         """Initialise the basic device."""
         self._data = data
         self.type = "device"
@@ -44,7 +46,7 @@ class AuroraDevice(Entity):
         return self._available
 
     @property
-    def device_info(self):
+    def device_info(self) -> DeviceInfo:
         """Return device specific attributes."""
         return {
             "identifiers": {(DOMAIN, self._data[ATTR_SERIAL_NUMBER])},

homeassistant/components/aurora_abb_powerone/config_flow.py

@@ -1,5 +1,8 @@
 """Config flow for Aurora ABB PowerOne integration."""
+from __future__ import annotations
+
 import logging
+from typing import Any
 
 from aurorapy.client import AuroraError, AuroraSerialClient
 import serial.tools.list_ports
@@ -7,6 +10,7 @@ import voluptuous as vol
 
 from homeassistant import config_entries, core
 from homeassistant.const import CONF_ADDRESS, CONF_PORT
+from homeassistant.data_entry_flow import FlowResult
 
 from .const import (
     ATTR_FIRMWARE,
@@ -22,7 +26,9 @@ from .const import (
 _LOGGER = logging.getLogger(__name__)
 
 
-def validate_and_connect(hass: core.HomeAssistant, data):
+def validate_and_connect(
+    hass: core.HomeAssistant, data: dict[str, Any]
+) -> dict[str, str]:
     """Validate the user input allows us to connect.
 
     Data has the keys from DATA_SCHEMA with values provided by the user.
@@ -50,15 +56,15 @@ def validate_and_connect(hass: core.HomeAssistant, data):
     return ret
 
 
-def scan_comports():
+def scan_comports() -> tuple[list[str] | None, str | None]:
     """Find and store available com ports for the GUI dropdown."""
-    comports = serial.tools.list_ports.comports(include_links=True)
+    com_ports = serial.tools.list_ports.comports(include_links=True)
-    comportslist = []
+    com_ports_list = []
-    for port in comports:
+    for port in com_ports:
-        comportslist.append(port.device)
+        com_ports_list.append(port.device)
         _LOGGER.debug("COM port option: %s", port.device)
-    if len(comportslist) > 0:
+    if len(com_ports_list) > 0:
-        return comportslist, comportslist[0]
+        return com_ports_list, com_ports_list[0]
     _LOGGER.warning("No com ports found.  Need a valid RS485 device to communicate")
     return None, None
 
@@ -67,18 +73,17 @@ class AuroraABBConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
     """Handle a config flow for Aurora ABB PowerOne."""
 
     VERSION = 1
-    CONNECTION_CLASS = config_entries.CONN_CLASS_LOCAL_POLL
 
     def __init__(self):
         """Initialise the config flow."""
         self.config = None
-        self._comportslist = None
+        self._com_ports_list = None
-        self._defaultcomport = None
+        self._default_com_port = None
 
-    async def async_step_import(self, config: dict):
+    async def async_step_import(self, config: dict[str, Any]) -> FlowResult:
         """Import a configuration from config.yaml."""
         if self.hass.config_entries.async_entries(DOMAIN):
-            return self.async_abort(reason="already_setup")
+            return self.async_abort(reason="already_configured")
 
         conf = {}
         conf[CONF_PORT] = config["device"]
@@ -87,14 +92,16 @@ class AuroraABBConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
 
         return self.async_create_entry(title=DEFAULT_INTEGRATION_TITLE, data=conf)
 
-    async def async_step_user(self, user_input=None):
+    async def async_step_user(
+        self, user_input: dict[str, Any] | None = None
+    ) -> FlowResult:
         """Handle a flow initialised by the user."""
 
         errors = {}
-        if self._comportslist is None:
+        if self._com_ports_list is None:
             result = await self.hass.async_add_executor_job(scan_comports)
-            self._comportslist, self._defaultcomport = result
+            self._com_ports_list, self._default_com_port = result
-            if self._defaultcomport is None:
+            if self._default_com_port is None:
                 return self.async_abort(reason="no_serial_ports")
 
         # Handle the initial step.
@@ -103,14 +110,6 @@ class AuroraABBConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
                 info = await self.hass.async_add_executor_job(
                     validate_and_connect, self.hass, user_input
                 )
-                info.update(user_input)
-                # Bomb out early if someone has already set up this device.
-                device_unique_id = info["serial_number"]
-                await self.async_set_unique_id(device_unique_id)
-                self._abort_if_unique_id_configured()
-
-                return self.async_create_entry(title=info["title"], data=info)
-
             except OSError as error:
                 if error.errno == 19:  # No such device.
                     errors["base"] = "invalid_serial_port"
@@ -127,10 +126,18 @@ class AuroraABBConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
                         error,
                     )
                     errors["base"] = "cannot_connect"
+            else:
+                info.update(user_input)
+                # Bomb out early if someone has already set up this device.
+                device_unique_id = info["serial_number"]
+                await self.async_set_unique_id(device_unique_id)
+                self._abort_if_unique_id_configured()
+                return self.async_create_entry(title=info["title"], data=info)
+
         # If no user input, must be first pass through the config.  Show  initial form.
         config_options = {
-            vol.Required(CONF_PORT, default=self._defaultcomport): vol.In(
+            vol.Required(CONF_PORT, default=self._default_com_port): vol.In(
-                self._comportslist
+                self._com_ports_list
             ),
             vol.Required(CONF_ADDRESS, default=DEFAULT_ADDRESS): vol.In(
                 range(MIN_ADDRESS, MAX_ADDRESS + 1)

homeassistant/components/aurora_abb_powerone/sensor.py

@@ -29,7 +29,7 @@ from homeassistant.const import (
 )
 import homeassistant.helpers.config_validation as cv
 
-from .aurora_device import AuroraDevice
+from .aurora_device import AuroraEntity
 from .const import DEFAULT_ADDRESS, DOMAIN
 
 _LOGGER = logging.getLogger(__name__)
@@ -84,7 +84,7 @@ async def async_setup_entry(hass, config_entry, async_add_entities) -> None:
     """Set up aurora_abb_powerone sensor based on a config entry."""
     entities = []
 
-    client = hass.data[DOMAIN][config_entry.unique_id]
+    client = hass.data[DOMAIN][config_entry.entry_id]
     data = config_entry.data
 
     for sens in SENSOR_TYPES:
@@ -94,7 +94,7 @@ async def async_setup_entry(hass, config_entry, async_add_entities) -> None:
     async_add_entities(entities, True)
 
 
-class AuroraSensor(AuroraDevice, SensorEntity):
+class AuroraSensor(AuroraEntity, SensorEntity):
     """Representation of a Sensor on a Aurora ABB PowerOne Solar inverter."""
 
     def __init__(
@@ -106,7 +106,7 @@ class AuroraSensor(AuroraDevice, SensorEntity):
         """Initialize the sensor."""
         super().__init__(client, data)
         self.entity_description = entity_description
-        self.availableprev = True
+        self.available_prev = True
 
     def update(self):
         """Fetch new state data for the sensor.
@@ -114,7 +114,7 @@ class AuroraSensor(AuroraDevice, SensorEntity):
         This is the only method that should fetch new data for Home Assistant.
         """
         try:
-            self.availableprev = self._attr_available
+            self.available_prev = self._attr_available
             self.client.connect()
             if self.entity_description.key == "instantaneouspower":
                 # read ADC channel 3 (grid power output)
@@ -145,7 +145,7 @@ class AuroraSensor(AuroraDevice, SensorEntity):
             else:
                 raise error
         finally:
-            if self._attr_available != self.availableprev:
+            if self._attr_available != self.available_prev:
                 if self._attr_available:
                     _LOGGER.info("Communication with %s back online", self.name)
                 else:

homeassistant/components/aurora_abb_powerone/strings.json

@@ -12,11 +12,10 @@
     "error": {
       "cannot_connect": "Unable to connect, please check serial port, address, electrical connection and that inverter is on (in daylight)",
       "invalid_serial_port": "Serial port is not a valid device or could not be openned",
-      "cannot_open_serial_port": "Cannot open serial port, please check and try again",
+      "cannot_open_serial_port": "Cannot open serial port, please check and try again"
-      "unknown": "[%key:common::config_flow::error::unknown%]"
     },
     "abort": {
-      "already_configured": "Device is already configured",
+      "already_configured": "[%key:common::config_flow::abort::already_configured_device%]",
       "no_serial_ports": "No com ports found.  Need a valid RS485 device to communicate."
     }
   }

tests/components/aurora_abb_powerone/test_config_flow.py

@@ -1,5 +1,6 @@
 """Test the Aurora ABB PowerOne Solar PV config flow."""
 from datetime import timedelta
+import logging
 from logging import INFO
 from unittest.mock import patch
 
@@ -17,7 +18,9 @@ from homeassistant.config_entries import ConfigEntryState
 from homeassistant.const import CONF_ADDRESS, CONF_PORT
 from homeassistant.util.dt import utcnow
 
-from tests.common import async_fire_time_changed
+from tests.common import MockConfigEntry, async_fire_time_changed
+
+TEST_DATA = {"device": "/dev/ttyUSB7", "address": 3, "name": "MyAuroraPV"}
 
 
 def _simulated_returns(index, global_measure=None):
@@ -163,10 +166,60 @@ async def test_form_invalid_com_ports(hass):
     assert len(mock_clientclose.mock_calls) == 1
 
 
+async def test_import_invalid_com_ports(hass, caplog):
+    """Test we display correct info when the comport is invalid.."""
+
+    caplog.set_level(logging.ERROR)
+    with patch(
+        "aurorapy.client.AuroraSerialClient.connect",
+        side_effect=OSError(19, "...no such device..."),
+        return_value=None,
+    ):
+        await hass.config_entries.flow.async_init(
+            DOMAIN, context={"source": config_entries.SOURCE_IMPORT}, data=TEST_DATA
+        )
+    configs = hass.config_entries.async_entries(DOMAIN)
+    assert len(configs) == 1
+    entry = configs[0]
+    assert entry.state == ConfigEntryState.SETUP_ERROR
+    assert "Failed to connect to inverter: " in caplog.text
+
+
+async def test_import_com_port_wont_open(hass):
+    """Test we display correct info when comport won't open."""
+
+    with patch(
+        "aurorapy.client.AuroraSerialClient.connect",
+        side_effect=AuroraError("..could not open port..."),
+    ):
+        await hass.config_entries.flow.async_init(
+            DOMAIN, context={"source": config_entries.SOURCE_IMPORT}, data=TEST_DATA
+        )
+    configs = hass.config_entries.async_entries(DOMAIN)
+    assert len(configs) == 1
+    entry = configs[0]
+    assert entry.state == ConfigEntryState.SETUP_ERROR
+
+
+async def test_import_other_oserror(hass):
+    """Test we display correct info when comport won't open."""
+
+    with patch(
+        "aurorapy.client.AuroraSerialClient.connect",
+        side_effect=OSError(18, "...another error..."),
+    ):
+        await hass.config_entries.flow.async_init(
+            DOMAIN, context={"source": config_entries.SOURCE_IMPORT}, data=TEST_DATA
+        )
+    configs = hass.config_entries.async_entries(DOMAIN)
+    assert len(configs) == 1
+    entry = configs[0]
+    assert entry.state == ConfigEntryState.SETUP_ERROR
+
+
 # Tests below can be deleted after deprecation period is finished.
 async def test_import_day(hass):
     """Test .yaml import when the inverter is able to communicate."""
-    TEST_DATA = {"device": "/dev/ttyUSB7", "address": 3, "name": "MyAuroraPV"}
 
     with patch("aurorapy.client.AuroraSerialClient.connect", return_value=None,), patch(
         "aurorapy.client.AuroraSerialClient.serial_number",
@@ -195,7 +248,6 @@ async def test_import_day(hass):
 
 async def test_import_night(hass):
     """Test .yaml import when the inverter is inaccessible (e.g. darkness)."""
-    TEST_DATA = {"device": "/dev/ttyUSB7", "address": 3, "name": "MyAuroraPV"}
 
     # First time round, no response.
     with patch(
@@ -241,13 +293,14 @@ async def test_import_night(hass):
         assert entry.unique_id
 
         assert len(mock_connect.mock_calls) == 1
-        assert hass.states.get("sensor.power_output").state == "45.7"
+        power = hass.states.get("sensor.power_output")
+        assert power
+        assert power.state == "45.7"
     assert len(hass.config_entries.async_entries(DOMAIN)) == 1
 
 
 async def test_import_night_then_user(hass):
     """Attempt yaml import and fail (dark), but user sets up manually before auto retry."""
-    TEST_DATA = {"device": "/dev/ttyUSB7", "address": 3, "name": "MyAuroraPV"}
 
     # First time round, no response.
     with patch(
@@ -322,3 +375,29 @@ async def test_import_night_then_user(hass):
         await hass.async_block_till_done()
         assert entry.state == ConfigEntryState.NOT_LOADED
     assert len(hass.config_entries.async_entries(DOMAIN)) == 1
+
+
+async def test_import_already_existing(hass):
+    """Test configuration.yaml import when already configured."""
+    TESTDATA = {"device": "/dev/ttyUSB7", "address": 7, "name": "MyAuroraPV"}
+
+    entry = MockConfigEntry(
+        domain=DOMAIN,
+        title="MyAuroraPV",
+        unique_id="0123456",
+        data={
+            CONF_PORT: "/dev/ttyUSB7",
+            CONF_ADDRESS: 7,
+            ATTR_FIRMWARE: "1.234",
+            ATTR_MODEL: "9.8.7.6 (A.B.C)",
+            ATTR_SERIAL_NUMBER: "9876543",
+            "title": "PhotoVoltaic Inverters",
+        },
+    )
+    entry.add_to_hass(hass)
+
+    result = await hass.config_entries.flow.async_init(
+        DOMAIN, context={"source": config_entries.SOURCE_IMPORT}, data=TESTDATA
+    )
+    assert result["type"] == data_entry_flow.RESULT_TYPE_ABORT
+    assert result["reason"] == "already_configured"

tests/components/aurora_abb_powerone/test_sensor.py

@@ -12,16 +12,10 @@ from homeassistant.components.aurora_abb_powerone.const import (
     DEFAULT_INTEGRATION_TITLE,
     DOMAIN,
 )
-from homeassistant.config_entries import SOURCE_IMPORT
 from homeassistant.const import CONF_ADDRESS, CONF_PORT
-from homeassistant.setup import async_setup_component
 import homeassistant.util.dt as dt_util
 
-from tests.common import (
+from tests.common import MockConfigEntry, async_fire_time_changed
-    MockConfigEntry,
-    assert_setup_component,
-    async_fire_time_changed,
-)
 
 TEST_CONFIG = {
     "sensor": {
@@ -56,49 +50,10 @@ def _mock_config_entry():
         },
         source="dummysource",
         entry_id="13579",
+        unique_id="654321",
     )
 
 
-async def test_setup_platform_valid_config(hass):
-    """Test that (deprecated) yaml import still works."""
-    with patch("aurorapy.client.AuroraSerialClient.connect", return_value=None), patch(
-        "aurorapy.client.AuroraSerialClient.measure",
-        side_effect=_simulated_returns,
-    ), patch(
-        "aurorapy.client.AuroraSerialClient.serial_number",
-        return_value="9876543",
-    ), patch(
-        "aurorapy.client.AuroraSerialClient.version",
-        return_value="9.8.7.6",
-    ), patch(
-        "aurorapy.client.AuroraSerialClient.pn",
-        return_value="A.B.C",
-    ), patch(
-        "aurorapy.client.AuroraSerialClient.firmware",
-        return_value="1.234",
-    ), patch(
-        "aurorapy.client.AuroraSerialClient.cumulated_energy",
-        side_effect=_simulated_returns,
-    ), assert_setup_component(
-        1, "sensor"
-    ):
-        assert await async_setup_component(hass, "sensor", TEST_CONFIG)
-        await hass.async_block_till_done()
-    power = hass.states.get("sensor.power_output")
-    assert power
-    assert power.state == "45.7"
-
-    # try to set up a second time - should abort.
-
-    result = await hass.config_entries.flow.async_init(
-        DOMAIN,
-        data=TEST_CONFIG,
-        context={"source": SOURCE_IMPORT},
-    )
-    assert result["type"] == "abort"
-    assert result["reason"] == "already_setup"
-
-
 async def test_sensors(hass):
     """Test data coming back from inverter."""
     mock_entry = _mock_config_entry()