From dff98b024cfd43e8be1150f9d88fba4bb466541f Mon Sep 17 00:00:00 2001 From: Dave T <17680170+davet2001@users.noreply.github.com> Date: Wed, 3 Nov 2021 00:30:29 +0000 Subject: [PATCH] Aurora abb defer unique_id assignment during yaml import (#58887) * Defer unique_id assignment during yaml import if dark * Back out variable name change to simplify. * Allow config flow yaml setup deferral. * Fix deferred yaml import * Code review: only wrap necessary lines in try blk * Code review: catch possible duplicate unique_id * Simplify assignment. * Code review: use timedelta to retry yaml import * Code review: if a different error occurs, raise it * Remove current config entry if duplicate unique_id * Code review: remove unnecessary line. * Code review: revert change, leave to other PR. * Code review: remove unnecessary patch & min->sec * Remove unnecessary else after raise. * Increase test coverage. * Check the number of config entries at each stage * Raise ConfigEntryNotReady when connection fails. * Log & return false for error on yaml import --- .../aurora_abb_powerone/__init__.py | 42 ++++- .../aurora_abb_powerone/aurora_device.py | 8 +- .../aurora_abb_powerone/config_flow.py | 6 - .../aurora_abb_powerone/test_config_flow.py | 175 +++++++++++++++++- .../aurora_abb_powerone/test_init.py | 12 ++ .../aurora_abb_powerone/test_sensor.py | 36 ++++ 6 files changed, 260 insertions(+), 19 deletions(-) diff --git a/homeassistant/components/aurora_abb_powerone/__init__.py b/homeassistant/components/aurora_abb_powerone/__init__.py index ff26c3770f0..2c3d0c546cd 100644 --- a/homeassistant/components/aurora_abb_powerone/__init__.py +++ b/homeassistant/components/aurora_abb_powerone/__init__.py @@ -10,13 +10,15 @@ import logging -from aurorapy.client import AuroraSerialClient +from aurorapy.client import AuroraError, AuroraSerialClient from homeassistant.config_entries import ConfigEntry from homeassistant.const import CONF_ADDRESS, CONF_PORT from homeassistant.core import HomeAssistant +from homeassistant.exceptions import ConfigEntryNotReady -from .const import DOMAIN +from .config_flow import validate_and_connect +from .const import ATTR_SERIAL_NUMBER, DOMAIN PLATFORMS = ["sensor"] @@ -29,9 +31,43 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry): comport = entry.data[CONF_PORT] address = entry.data[CONF_ADDRESS] serclient = 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: + try: + res = await hass.async_add_executor_job( + validate_and_connect, hass, entry.data + ) + except AuroraError as error: + if "No response after" in str(error): + raise ConfigEntryNotReady("No response (could be dark)") from error + _LOGGER.error("Failed to connect to inverter: %s", error) + return False + except OSError as error: + if error.errno == 19: # No such device. + _LOGGER.error("Failed to connect to inverter: no such COM port") + return False + _LOGGER.error("Failed to connect to inverter: %s", error) + return False + else: + # If we got here, the device is now communicating (maybe after + # being in darkness). But there's a small risk that the user has + # configured via the UI since we last attempted the yaml setup, + # which means we'd get a duplicate unique ID. + new_id = res[ATTR_SERIAL_NUMBER] + # Check if this unique_id has already been used + for existing_entry in hass.config_entries.async_entries(DOMAIN): + if existing_entry.unique_id == new_id: + _LOGGER.debug( + "Remove already configured config entry for id %s", new_id + ) + hass.async_create_task( + hass.config_entries.async_remove(entry.entry_id) + ) + return False + hass.config_entries.async_update_entry(entry, unique_id=new_id) hass.data.setdefault(DOMAIN, {})[entry.unique_id] = serclient - hass.config_entries.async_setup_platforms(entry, PLATFORMS) return True diff --git a/homeassistant/components/aurora_abb_powerone/aurora_device.py b/homeassistant/components/aurora_abb_powerone/aurora_device.py index 3913515a9b9..d2aed5ec7a8 100644 --- a/homeassistant/components/aurora_abb_powerone/aurora_device.py +++ b/homeassistant/components/aurora_abb_powerone/aurora_device.py @@ -1,4 +1,6 @@ """Top level class for AuroraABBPowerOneSolarPV inverters and sensors.""" +from __future__ import annotations + import logging from aurorapy.client import AuroraSerialClient @@ -29,9 +31,11 @@ class AuroraDevice(Entity): self._available = True @property - def unique_id(self) -> str: + def unique_id(self) -> str | None: """Return the unique id for this device.""" - serial = self._data[ATTR_SERIAL_NUMBER] + serial = self._data.get(ATTR_SERIAL_NUMBER) + if serial is None: + return None return f"{serial}_{self.entity_description.key}" @property diff --git a/homeassistant/components/aurora_abb_powerone/config_flow.py b/homeassistant/components/aurora_abb_powerone/config_flow.py index c0c87e9e103..012fe7b14bb 100644 --- a/homeassistant/components/aurora_abb_powerone/config_flow.py +++ b/homeassistant/components/aurora_abb_powerone/config_flow.py @@ -81,16 +81,10 @@ class AuroraABBConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): return self.async_abort(reason="already_setup") conf = {} - conf[ATTR_SERIAL_NUMBER] = "sn_unknown_yaml" - conf[ATTR_MODEL] = "model_unknown_yaml" - conf[ATTR_FIRMWARE] = "fw_unknown_yaml" conf[CONF_PORT] = config["device"] conf[CONF_ADDRESS] = config["address"] # config["name"] from yaml is ignored. - await self.async_set_unique_id(self.flow_id) - self._abort_if_unique_id_configured() - return self.async_create_entry(title=DEFAULT_INTEGRATION_TITLE, data=conf) async def async_step_user(self, user_input=None): diff --git a/tests/components/aurora_abb_powerone/test_config_flow.py b/tests/components/aurora_abb_powerone/test_config_flow.py index d385d33ddd9..620d7e10e53 100644 --- a/tests/components/aurora_abb_powerone/test_config_flow.py +++ b/tests/components/aurora_abb_powerone/test_config_flow.py @@ -1,4 +1,5 @@ """Test the Aurora ABB PowerOne Solar PV config flow.""" +from datetime import timedelta from logging import INFO from unittest.mock import patch @@ -12,7 +13,20 @@ from homeassistant.components.aurora_abb_powerone.const import ( ATTR_SERIAL_NUMBER, DOMAIN, ) +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 + + +def _simulated_returns(index, global_measure=None): + returns = { + 3: 45.678, # power + 21: 9.876, # temperature + 5: 12345, # energy + } + return returns[index] async def test_form(hass): @@ -150,16 +164,161 @@ async def test_form_invalid_com_ports(hass): # Tests below can be deleted after deprecation period is finished. -async def test_import(hass): - """Test configuration.yaml import used during migration.""" - TESTDATA = {"device": "/dev/ttyUSB7", "address": 3, "name": "MyAuroraPV"} - with patch( - "homeassistant.components.generic.camera.GenericCamera.async_camera_image", - return_value=None, - ): +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", + 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", + ) as mock_setup_entry: result = await hass.config_entries.flow.async_init( - DOMAIN, context={"source": config_entries.SOURCE_IMPORT}, data=TESTDATA + DOMAIN, context={"source": config_entries.SOURCE_IMPORT}, data=TEST_DATA ) + assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY assert result["data"][CONF_PORT] == "/dev/ttyUSB7" assert result["data"][CONF_ADDRESS] == 3 + assert len(hass.config_entries.async_entries(DOMAIN)) == 1 + + assert len(mock_setup_entry.mock_calls) == 1 + + +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( + "aurorapy.client.AuroraSerialClient.connect", + side_effect=AuroraError("No response after"), + ) as mock_connect: + result = 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 not entry.unique_id + assert entry.state == ConfigEntryState.SETUP_RETRY + + assert len(mock_connect.mock_calls) == 1 + assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY + assert result["data"][CONF_PORT] == "/dev/ttyUSB7" + assert result["data"][CONF_ADDRESS] == 3 + + # Second time round, talking this time. + with patch("aurorapy.client.AuroraSerialClient.connect", return_value=None,), 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.measure", + side_effect=_simulated_returns, + ): + # Wait >5seconds for the config to auto retry. + async_fire_time_changed(hass, utcnow() + timedelta(seconds=6)) + await hass.async_block_till_done() + assert entry.state == ConfigEntryState.LOADED + assert entry.unique_id + + assert len(mock_connect.mock_calls) == 1 + assert hass.states.get("sensor.power_output").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( + "aurorapy.client.AuroraSerialClient.connect", + side_effect=AuroraError("No response after"), + ) as mock_connect: + result = 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 not entry.unique_id + assert entry.state == ConfigEntryState.SETUP_RETRY + + assert len(mock_connect.mock_calls) == 1 + assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY + assert result["data"][CONF_PORT] == "/dev/ttyUSB7" + assert result["data"][CONF_ADDRESS] == 3 + + # Failed once, now simulate the user initiating config flow with valid settings. + fakecomports = [] + fakecomports.append(list_ports_common.ListPortInfo("/dev/ttyUSB7")) + with patch( + "serial.tools.list_ports.comports", + return_value=fakecomports, + ): + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + assert result["type"] == "form" + assert result["errors"] == {} + + with patch("aurorapy.client.AuroraSerialClient.connect", return_value=None,), 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", + ): + result2 = await hass.config_entries.flow.async_configure( + result["flow_id"], + {CONF_PORT: "/dev/ttyUSB7", CONF_ADDRESS: 7}, + ) + + assert result2["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY + assert len(hass.config_entries.async_entries(DOMAIN)) == 2 + + # Now retry yaml - it should fail with duplicate + with patch("aurorapy.client.AuroraSerialClient.connect", return_value=None,), 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", + ): + # Wait >5seconds for the config to auto retry. + async_fire_time_changed(hass, utcnow() + timedelta(seconds=6)) + await hass.async_block_till_done() + assert entry.state == ConfigEntryState.NOT_LOADED + assert len(hass.config_entries.async_entries(DOMAIN)) == 1 diff --git a/tests/components/aurora_abb_powerone/test_init.py b/tests/components/aurora_abb_powerone/test_init.py index bd0f1c727cd..3bef40a14d3 100644 --- a/tests/components/aurora_abb_powerone/test_init.py +++ b/tests/components/aurora_abb_powerone/test_init.py @@ -19,6 +19,18 @@ async def test_unload_entry(hass): with patch("aurorapy.client.AuroraSerialClient.connect", return_value=None), patch( "homeassistant.components.aurora_abb_powerone.sensor.AuroraSensor.update", return_value=None, + ), 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", ): mock_entry = MockConfigEntry( domain=DOMAIN, diff --git a/tests/components/aurora_abb_powerone/test_sensor.py b/tests/components/aurora_abb_powerone/test_sensor.py index ae9360498c7..c1d1d4ad5c8 100644 --- a/tests/components/aurora_abb_powerone/test_sensor.py +++ b/tests/components/aurora_abb_powerone/test_sensor.py @@ -64,6 +64,18 @@ async def test_setup_platform_valid_config(hass): 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, @@ -94,6 +106,18 @@ async def test_sensors(hass): 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, @@ -123,6 +147,18 @@ async def test_sensor_dark(hass): # sun is up 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", ): mock_entry.add_to_hass(hass) await hass.config_entries.async_setup(mock_entry.entry_id)