From b41d0be9522fabda0ac8affd2add6876a66205ea Mon Sep 17 00:00:00 2001 From: Duco Sebel <74970928+DCSBL@users.noreply.github.com> Date: Fri, 16 Dec 2022 16:53:54 +0100 Subject: [PATCH] Improve HomeWizard request issue reporting (#82366) * Trigger reauth flow when HomeWizard API was disabled * Add tests for reauth flow * Fix typo in test * Add parallel updates constant * Improve error message when device in unreachable during config * Set quality scale * Remove quality scale * Throw error instead of abort when setup fails * Adjust test for new setup behaviour * Trigger reauth flow when API is disabled and continue retrying * Reload entry and raise AuthFailed during init * Abort running config flow * Listen for coordinator updates to trigger reload * Use build-in backoff system * Fix failing test * Test reauth flow is active after disable-api init * Test reauth flow removal --- .../components/homewizard/__init__.py | 26 +++-- .../components/homewizard/config_flow.py | 97 +++++++++++++------ .../components/homewizard/coordinator.py | 20 +++- homeassistant/components/homewizard/sensor.py | 3 +- .../components/homewizard/strings.json | 13 ++- .../components/homewizard/test_config_flow.py | 92 +++++++++++++++--- tests/components/homewizard/test_init.py | 48 ++++++++- tests/components/homewizard/test_sensor.py | 10 -- 8 files changed, 243 insertions(+), 66 deletions(-) diff --git a/homeassistant/components/homewizard/__init__.py b/homeassistant/components/homewizard/__init__.py index a236c392c07..df58ccccd30 100644 --- a/homeassistant/components/homewizard/__init__.py +++ b/homeassistant/components/homewizard/__init__.py @@ -1,7 +1,7 @@ """The Homewizard integration.""" import logging -from homeassistant.config_entries import SOURCE_IMPORT, ConfigEntry +from homeassistant.config_entries import SOURCE_IMPORT, SOURCE_REAUTH, ConfigEntry from homeassistant.const import CONF_IP_ADDRESS from homeassistant.core import HomeAssistant from homeassistant.exceptions import ConfigEntryNotReady @@ -64,13 +64,28 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: hass.async_create_task(hass.config_entries.async_remove(old_config_entry_id)) # Create coordinator - coordinator = Coordinator(hass, entry.data[CONF_IP_ADDRESS]) + coordinator = Coordinator(hass, entry.entry_id, entry.data[CONF_IP_ADDRESS]) try: await coordinator.async_config_entry_first_refresh() + except ConfigEntryNotReady: + await coordinator.api.close() + + if coordinator.api_disabled: + entry.async_start_reauth(hass) + raise + # Abort reauth config flow if active + for progress_flow in hass.config_entries.flow.async_progress_by_handler(DOMAIN): + if progress_flow["context"].get("source") == SOURCE_REAUTH: + hass.config_entries.flow.async_abort(progress_flow["flow_id"]) + + # Setup entry + hass.data.setdefault(DOMAIN, {}) + hass.data[DOMAIN][entry.entry_id] = coordinator + # Register device device_registry = dr.async_get(hass) device_registry.async_get_or_create( @@ -83,9 +98,6 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: ) # Finalize - hass.data.setdefault(DOMAIN, {}) - hass.data[DOMAIN][entry.entry_id] = coordinator - await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS) return True @@ -98,7 +110,7 @@ async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: unload_ok = await hass.config_entries.async_unload_platforms(entry, PLATFORMS) if unload_ok: - config_data = hass.data[DOMAIN].pop(entry.entry_id) - await config_data.api.close() + coordinator = hass.data[DOMAIN].pop(entry.entry_id) + await coordinator.api.close() return unload_ok diff --git a/homeassistant/components/homewizard/config_flow.py b/homeassistant/components/homewizard/config_flow.py index 6f164637a7c..02d0976157e 100644 --- a/homeassistant/components/homewizard/config_flow.py +++ b/homeassistant/components/homewizard/config_flow.py @@ -1,6 +1,7 @@ """Config flow for Homewizard.""" from __future__ import annotations +from collections.abc import Mapping import logging from typing import Any, cast @@ -33,6 +34,7 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): def __init__(self) -> None: """Initialize the HomeWizard config flow.""" self.config: dict[str, str | int] = {} + self.entry: config_entries.ConfigEntry | None = None async def async_step_import(self, import_config: dict[str, Any]) -> FlowResult: """Handle a flow initiated by older `homewizard_energy` component.""" @@ -57,27 +59,38 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): _LOGGER.debug("config_flow async_step_user") + data_schema = Schema( + { + Required(CONF_IP_ADDRESS): str, + } + ) + if user_input is None: return self.async_show_form( step_id="user", - data_schema=Schema( - { - Required(CONF_IP_ADDRESS): str, - } - ), + data_schema=data_schema, errors=None, ) - device_info = await self._async_try_connect_and_fetch( - user_input[CONF_IP_ADDRESS] - ) + error = await self._async_try_connect(user_input[CONF_IP_ADDRESS]) + if error is not None: + return self.async_show_form( + step_id="user", + data_schema=data_schema, + errors={"base": error}, + ) + + # Fetch device information + api = HomeWizardEnergy(user_input[CONF_IP_ADDRESS]) + device_info = await api.device() + await api.close() # Sets unique ID and aborts if it is already exists await self._async_set_and_check_unique_id( { CONF_IP_ADDRESS: user_input[CONF_IP_ADDRESS], - CONF_PRODUCT_TYPE: device_info[CONF_PRODUCT_TYPE], - CONF_SERIAL: device_info[CONF_SERIAL], + CONF_PRODUCT_TYPE: device_info.product_type, + CONF_SERIAL: device_info.serial, } ) @@ -90,7 +103,7 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): # Add entry return self.async_create_entry( - title=f"{device_info[CONF_PRODUCT_NAME]} ({device_info[CONF_SERIAL]})", + title=f"{device_info.product_name} ({device_info.serial})", data=data, ) @@ -138,11 +151,14 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): ) -> FlowResult: """Confirm discovery.""" if user_input is not None: - if (self.config[CONF_API_ENABLED]) != "1": - raise AbortFlow(reason="api_not_enabled") # Check connection - await self._async_try_connect_and_fetch(str(self.config[CONF_IP_ADDRESS])) + error = await self._async_try_connect(str(self.config[CONF_IP_ADDRESS])) + if error is not None: + return self.async_show_form( + step_id="discovery_confirm", + errors={"base": error}, + ) return self.async_create_entry( title=f"{self.config[CONF_PRODUCT_NAME]} ({self.config[CONF_SERIAL]})", @@ -166,46 +182,67 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): }, ) + async def async_step_reauth(self, entry_data: Mapping[str, Any]) -> FlowResult: + """Handle re-auth if API was disabled.""" + + self.entry = self.hass.config_entries.async_get_entry(self.context["entry_id"]) + return await self.async_step_reauth_confirm() + + async def async_step_reauth_confirm( + self, user_input: dict[str, Any] | None = None + ) -> FlowResult: + """Confirm reauth dialog.""" + + if user_input is not None: + assert self.entry is not None + + error = await self._async_try_connect(self.entry.data[CONF_IP_ADDRESS]) + if error is not None: + return self.async_show_form( + step_id="reauth_confirm", + errors={"base": error}, + ) + + await self.hass.config_entries.async_reload(self.entry.entry_id) + return self.async_abort(reason="reauth_successful") + + return self.async_show_form( + step_id="reauth_confirm", + ) + @staticmethod - async def _async_try_connect_and_fetch(ip_address: str) -> dict[str, Any]: + async def _async_try_connect(ip_address: str) -> str | None: """Try to connect.""" - _LOGGER.debug("config_flow _async_try_connect_and_fetch") + _LOGGER.debug("config_flow _async_try_connect") # Make connection with device # This is to test the connection and to get info for unique_id energy_api = HomeWizardEnergy(ip_address) try: - device = await energy_api.device() + await energy_api.device() - except DisabledError as ex: + except DisabledError: _LOGGER.error("API disabled, API must be enabled in the app") - raise AbortFlow("api_not_enabled") from ex + return "api_not_enabled" except UnsupportedError as ex: _LOGGER.error("API version unsuppored") raise AbortFlow("unsupported_api_version") from ex except RequestError as ex: - _LOGGER.error("Unexpected or no response") - raise AbortFlow("unknown_error") from ex + _LOGGER.exception(ex) + return "network_error" except Exception as ex: - _LOGGER.exception( - "Error connecting with Energy Device at %s", - ip_address, - ) + _LOGGER.exception(ex) raise AbortFlow("unknown_error") from ex finally: await energy_api.close() - return { - CONF_PRODUCT_NAME: device.product_name, - CONF_PRODUCT_TYPE: device.product_type, - CONF_SERIAL: device.serial, - } + return None async def _async_set_and_check_unique_id(self, entry_info: dict[str, Any]) -> None: """Validate if entry exists.""" diff --git a/homeassistant/components/homewizard/coordinator.py b/homeassistant/components/homewizard/coordinator.py index dc441836d9a..d8c20a6cc92 100644 --- a/homeassistant/components/homewizard/coordinator.py +++ b/homeassistant/components/homewizard/coordinator.py @@ -1,6 +1,7 @@ """Update coordinator for HomeWizard.""" from __future__ import annotations +from datetime import timedelta import logging from homewizard_energy import HomeWizardEnergy @@ -15,20 +16,25 @@ from .const import DOMAIN, UPDATE_INTERVAL, DeviceResponseEntry _LOGGER = logging.getLogger(__name__) +MAX_UPDATE_INTERVAL = timedelta(minutes=30) + class HWEnergyDeviceUpdateCoordinator(DataUpdateCoordinator[DeviceResponseEntry]): """Gather data for the energy device.""" api: HomeWizardEnergy + api_disabled: bool = False def __init__( self, hass: HomeAssistant, + entry_id: str, host: str, ) -> None: """Initialize Update Coordinator.""" super().__init__(hass, _LOGGER, name=DOMAIN, update_interval=UPDATE_INTERVAL) + self.entry_id = entry_id self.api = HomeWizardEnergy(host, clientsession=async_get_clientsession(hass)) @property @@ -55,9 +61,19 @@ class HWEnergyDeviceUpdateCoordinator(DataUpdateCoordinator[DeviceResponseEntry] data["system"] = await self.api.system() except RequestError as ex: - raise UpdateFailed("Device did not respond as expected") from ex + raise UpdateFailed(ex) from ex except DisabledError as ex: - raise UpdateFailed("API disabled, API must be enabled in the app") from ex + if not self.api_disabled: + self.api_disabled = True + + # Do not reload when performing first refresh + if self.data is not None: + await self.hass.config_entries.async_reload(self.entry_id) + + raise UpdateFailed(ex) from ex + + else: + self.api_disabled = False return data diff --git a/homeassistant/components/homewizard/sensor.py b/homeassistant/components/homewizard/sensor.py index 8d1b6c9d458..a246a08d692 100644 --- a/homeassistant/components/homewizard/sensor.py +++ b/homeassistant/components/homewizard/sensor.py @@ -1,7 +1,6 @@ """Creates Homewizard sensor entities.""" from __future__ import annotations -import logging from typing import Final, cast from homeassistant.components.sensor import ( @@ -26,7 +25,7 @@ from homeassistant.helpers.update_coordinator import CoordinatorEntity from .const import DOMAIN, DeviceResponseEntry from .coordinator import HWEnergyDeviceUpdateCoordinator -_LOGGER = logging.getLogger(__name__) +PARALLEL_UPDATES = 1 SENSORS: Final[tuple[SensorEntityDescription, ...]] = ( SensorEntityDescription( diff --git a/homeassistant/components/homewizard/strings.json b/homeassistant/components/homewizard/strings.json index e774c26aa6f..085ea208b52 100644 --- a/homeassistant/components/homewizard/strings.json +++ b/homeassistant/components/homewizard/strings.json @@ -11,14 +11,21 @@ "discovery_confirm": { "title": "Confirm", "description": "Do you want to set up {product_type} ({serial}) at {ip_address}?" + }, + "reauth_confirm": { + "description": "The local API is disabled. Go to the HomeWizard Energy app and enable the API in the device settings." } }, + "error": { + "api_not_enabled": "The API is not enabled. Enable API in the HomeWizard Energy App under settings", + "network_error": "Device unreachable, make sure that you have entered the correct IP address and that the device is available in your network" + }, "abort": { "already_configured": "[%key:common::config_flow::abort::already_configured_device%]", "invalid_discovery_parameters": "Detected unsupported API version", - "api_not_enabled": "The API is not enabled. Enable API in the HomeWizard Energy App under settings", "device_not_supported": "This device is not supported", - "unknown_error": "[%key:common::config_flow::error::unknown%]" + "unknown_error": "[%key:common::config_flow::error::unknown%]", + "reauth_successful": "Enabeling API was successful" } } -} +} \ No newline at end of file diff --git a/tests/components/homewizard/test_config_flow.py b/tests/components/homewizard/test_config_flow.py index cdd67cbf7ea..453a29c74f8 100644 --- a/tests/components/homewizard/test_config_flow.py +++ b/tests/components/homewizard/test_config_flow.py @@ -46,8 +46,7 @@ async def test_manual_flow_works(hass, aioclient_mock): assert len(hass.config_entries.async_entries(DOMAIN)) == 1 - assert len(device.device.mock_calls) == 1 - assert len(device.close.mock_calls) == 1 + assert len(device.close.mock_calls) == len(device.device.mock_calls) assert len(mock_setup_entry.mock_calls) == 1 @@ -142,8 +141,7 @@ async def test_config_flow_imports_entry(aioclient_mock, hass): assert result["data"][CONF_IP_ADDRESS] == "1.2.3.4" assert len(hass.config_entries.async_entries(DOMAIN)) == 1 - assert len(device.device.mock_calls) == 1 - assert len(device.close.mock_calls) == 1 + assert len(device.device.mock_calls) == len(device.close.mock_calls) assert len(mock_setup_entry.mock_calls) == 1 @@ -174,19 +172,25 @@ async def test_discovery_disabled_api(hass, aioclient_mock): assert result["type"] == FlowResultType.FORM + def mock_initialize(): + raise DisabledError + + device = get_mock_device() + device.device.side_effect = mock_initialize + with patch( "homeassistant.components.homewizard.async_setup_entry", return_value=True, ), patch( "homeassistant.components.homewizard.config_flow.HomeWizardEnergy", - return_value=get_mock_device(), + return_value=device, ): result = await hass.config_entries.flow.async_configure( result["flow_id"], user_input={"ip_address": "192.168.43.183"} ) - assert result["type"] == FlowResultType.ABORT - assert result["reason"] == "api_not_enabled" + assert result["type"] == FlowResultType.FORM + assert result["errors"] == {"base": "api_not_enabled"} async def test_discovery_missing_data_in_service_info(hass, aioclient_mock): @@ -271,8 +275,8 @@ async def test_check_disabled_api(hass, aioclient_mock): result["flow_id"], {CONF_IP_ADDRESS: "2.2.2.2"} ) - assert result["type"] == FlowResultType.ABORT - assert result["reason"] == "api_not_enabled" + assert result["type"] == FlowResultType.FORM + assert result["errors"] == {"base": "api_not_enabled"} async def test_check_error_handling_api(hass, aioclient_mock): @@ -355,5 +359,71 @@ async def test_check_requesterror(hass, aioclient_mock): result["flow_id"], {CONF_IP_ADDRESS: "2.2.2.2"} ) - assert result["type"] == FlowResultType.ABORT - assert result["reason"] == "unknown_error" + assert result["type"] == FlowResultType.FORM + assert result["errors"] == {"base": "network_error"} + + +async def test_reauth_flow(hass, aioclient_mock): + """Test reauth flow while API is enabled.""" + + mock_entry = MockConfigEntry( + domain="homewizard_energy", data={CONF_IP_ADDRESS: "1.2.3.4"} + ) + + mock_entry.add_to_hass(hass) + + result = await hass.config_entries.flow.async_init( + DOMAIN, + context={ + "source": config_entries.SOURCE_REAUTH, + "entry_id": mock_entry.entry_id, + }, + ) + + assert result["type"] == "form" + assert result["step_id"] == "reauth_confirm" + + device = get_mock_device() + with patch( + "homeassistant.components.homewizard.config_flow.HomeWizardEnergy", + return_value=device, + ): + result = await hass.config_entries.flow.async_configure(result["flow_id"], {}) + + assert result["type"] == FlowResultType.ABORT + assert result["reason"] == "reauth_successful" + + +async def test_reauth_error(hass, aioclient_mock): + """Test reauth flow while API is still disabled.""" + + def mock_initialize(): + raise DisabledError() + + mock_entry = MockConfigEntry( + domain="homewizard_energy", data={CONF_IP_ADDRESS: "1.2.3.4"} + ) + + mock_entry.add_to_hass(hass) + + result = await hass.config_entries.flow.async_init( + DOMAIN, + context={ + "source": config_entries.SOURCE_REAUTH, + "entry_id": mock_entry.entry_id, + }, + ) + + assert result["type"] == "form" + assert result["step_id"] == "reauth_confirm" + + device = get_mock_device() + device.device.side_effect = mock_initialize + with patch( + "homeassistant.components.homewizard.config_flow.HomeWizardEnergy", + return_value=device, + ): + result = await hass.config_entries.flow.async_configure(result["flow_id"], {}) + + assert result["type"] == FlowResultType.FORM + assert result["errors"] == {"base": "api_not_enabled"} diff --git a/tests/components/homewizard/test_init.py b/tests/components/homewizard/test_init.py index c7cc5bd7bdb..dea2972d79f 100644 --- a/tests/components/homewizard/test_init.py +++ b/tests/components/homewizard/test_init.py @@ -6,7 +6,7 @@ from homewizard_energy.errors import DisabledError, HomeWizardEnergyException from homeassistant import config_entries from homeassistant.components.homewizard.const import DOMAIN -from homeassistant.config_entries import ConfigEntryState +from homeassistant.config_entries import SOURCE_REAUTH, ConfigEntryState from homeassistant.const import CONF_IP_ADDRESS from homeassistant.helpers import entity_registry as er @@ -187,6 +187,52 @@ async def test_load_detect_api_disabled(aioclient_mock, hass): assert entry.state is ConfigEntryState.SETUP_RETRY + flows = hass.config_entries.flow.async_progress() + assert len(flows) == 1 + + flow = flows[0] + assert flow.get("step_id") == "reauth_confirm" + assert flow.get("handler") == DOMAIN + + assert "context" in flow + assert flow["context"].get("source") == SOURCE_REAUTH + assert flow["context"].get("entry_id") == entry.entry_id + + +async def test_load_removes_reauth_flow(aioclient_mock, hass): + """Test setup removes reauth flow when API is enabled.""" + + device = get_mock_device() + + entry = MockConfigEntry( + domain=DOMAIN, + data={CONF_IP_ADDRESS: "1.1.1.1"}, + unique_id=DOMAIN, + ) + entry.add_to_hass(hass) + + # Add reauth flow from 'previously' failed init + entry.async_start_reauth(hass) + await hass.async_block_till_done() + + flows = hass.config_entries.flow.async_progress_by_handler(DOMAIN) + assert len(flows) == 1 + + # Initialize entry + with patch( + "homeassistant.components.homewizard.coordinator.HomeWizardEnergy", + return_value=device, + ): + await hass.config_entries.async_setup(entry.entry_id) + + await hass.async_block_till_done() + + assert entry.state is ConfigEntryState.LOADED + + # Flow should be removed + flows = hass.config_entries.flow.async_progress_by_handler(DOMAIN) + assert len(flows) == 0 + async def test_load_handles_homewizardenergy_exception(aioclient_mock, hass): """Test setup handles exception from API.""" diff --git a/tests/components/homewizard/test_sensor.py b/tests/components/homewizard/test_sensor.py index d405a713edb..8ed6f9365c8 100644 --- a/tests/components/homewizard/test_sensor.py +++ b/tests/components/homewizard/test_sensor.py @@ -774,13 +774,3 @@ async def test_api_disabled(hass, mock_config_entry_data, mock_config_entry): ).state == "unavailable" ) - - api.data.side_effect = None - async_fire_time_changed(hass, utcnow + timedelta(seconds=10)) - await hass.async_block_till_done() - assert ( - hass.states.get( - "sensor.product_name_aabbccddeeff_total_power_import_t1" - ).state - == "1234.123" - )