From b3814aa4e62764db4e1a843b66599acc800d930b Mon Sep 17 00:00:00 2001 From: Franck Nijhof Date: Thu, 10 Feb 2022 09:53:26 +0100 Subject: [PATCH] Refactor Plugwise command handling (#66202) --- homeassistant/components/plugwise/climate.py | 62 ++++-------- homeassistant/components/plugwise/switch.py | 43 +++----- homeassistant/components/plugwise/util.py | 36 +++++++ tests/components/plugwise/test_climate.py | 100 ++++++++++--------- tests/components/plugwise/test_switch.py | 85 ++++++++++------ 5 files changed, 178 insertions(+), 148 deletions(-) create mode 100644 homeassistant/components/plugwise/util.py diff --git a/homeassistant/components/plugwise/climate.py b/homeassistant/components/plugwise/climate.py index b3e7472369c..4cea3d9499e 100644 --- a/homeassistant/components/plugwise/climate.py +++ b/homeassistant/components/plugwise/climate.py @@ -1,8 +1,6 @@ """Plugwise Climate component for Home Assistant.""" from typing import Any -from plugwise.exceptions import PlugwiseException - from homeassistant.components.climate import ClimateEntity from homeassistant.components.climate.const import ( CURRENT_HVAC_COOL, @@ -20,16 +18,10 @@ from homeassistant.const import ATTR_TEMPERATURE, TEMP_CELSIUS from homeassistant.core import HomeAssistant, callback from homeassistant.helpers.entity_platform import AddEntitiesCallback -from .const import ( - DEFAULT_MAX_TEMP, - DEFAULT_MIN_TEMP, - DOMAIN, - LOGGER, - SCHEDULE_OFF, - SCHEDULE_ON, -) +from .const import DEFAULT_MAX_TEMP, DEFAULT_MIN_TEMP, DOMAIN, SCHEDULE_OFF, SCHEDULE_ON from .coordinator import PlugwiseDataUpdateCoordinator from .entity import PlugwiseEntity +from .util import plugwise_command HVAC_MODES_HEAT_ONLY = [HVAC_MODE_HEAT, HVAC_MODE_AUTO, HVAC_MODE_OFF] HVAC_MODES_HEAT_COOL = [HVAC_MODE_HEAT, HVAC_MODE_COOL, HVAC_MODE_AUTO, HVAC_MODE_OFF] @@ -76,21 +68,16 @@ class PlugwiseClimateEntity(PlugwiseEntity, ClimateEntity): self._loc_id = coordinator.data.devices[device_id]["location"] + @plugwise_command async def async_set_temperature(self, **kwargs: Any) -> None: """Set new target temperature.""" - temperature = kwargs.get(ATTR_TEMPERATURE) - if (temperature is not None) and ( - self._attr_min_temp < temperature < self._attr_max_temp + if ((temperature := kwargs.get(ATTR_TEMPERATURE)) is None) or ( + self._attr_max_temp < temperature < self._attr_min_temp ): - try: - await self.coordinator.api.set_temperature(self._loc_id, temperature) - self._attr_target_temperature = temperature - self.async_write_ha_state() - except PlugwiseException: - LOGGER.error("Error while communicating to device") - else: - LOGGER.error("Invalid temperature requested") + raise ValueError("Invalid temperature requested") + await self.coordinator.api.set_temperature(self._loc_id, temperature) + @plugwise_command async def async_set_hvac_mode(self, hvac_mode: str) -> None: """Set the hvac mode.""" state = SCHEDULE_OFF @@ -98,35 +85,22 @@ class PlugwiseClimateEntity(PlugwiseEntity, ClimateEntity): if hvac_mode == HVAC_MODE_AUTO: state = SCHEDULE_ON - try: - await self.coordinator.api.set_temperature( - self._loc_id, climate_data.get("schedule_temperature") - ) - self._attr_target_temperature = climate_data.get("schedule_temperature") - except PlugwiseException: - LOGGER.error("Error while communicating to device") - - try: - await self.coordinator.api.set_schedule_state( - self._loc_id, climate_data.get("last_used"), state + await self.coordinator.api.set_temperature( + self._loc_id, climate_data.get("schedule_temperature") ) - self._attr_hvac_mode = hvac_mode - self.async_write_ha_state() - except PlugwiseException: - LOGGER.error("Error while communicating to device") + self._attr_target_temperature = climate_data.get("schedule_temperature") + await self.coordinator.api.set_schedule_state( + self._loc_id, climate_data.get("last_used"), state + ) + + @plugwise_command async def async_set_preset_mode(self, preset_mode: str) -> None: """Set the preset mode.""" - if not (presets := self.coordinator.data.devices[self._dev_id].get("presets")): + if not self.coordinator.data.devices[self._dev_id].get("presets"): raise ValueError("No presets available") - try: - await self.coordinator.api.set_preset(self._loc_id, preset_mode) - self._attr_preset_mode = preset_mode - self._attr_target_temperature = presets.get(preset_mode, "none")[0] - self.async_write_ha_state() - except PlugwiseException: - LOGGER.error("Error while communicating to device") + await self.coordinator.api.set_preset(self._loc_id, preset_mode) @callback def _handle_coordinator_update(self) -> None: diff --git a/homeassistant/components/plugwise/switch.py b/homeassistant/components/plugwise/switch.py index c95474a2b5a..5365d5834a8 100644 --- a/homeassistant/components/plugwise/switch.py +++ b/homeassistant/components/plugwise/switch.py @@ -3,8 +3,6 @@ from __future__ import annotations from typing import Any -from plugwise.exceptions import PlugwiseException - from homeassistant.components.switch import SwitchEntity, SwitchEntityDescription from homeassistant.config_entries import ConfigEntry from homeassistant.core import HomeAssistant, callback @@ -13,6 +11,7 @@ from homeassistant.helpers.entity_platform import AddEntitiesCallback from .const import DOMAIN, LOGGER from .coordinator import PlugwiseDataUpdateCoordinator from .entity import PlugwiseEntity +from .util import plugwise_command SWITCHES: tuple[SwitchEntityDescription, ...] = ( SwitchEntityDescription( @@ -54,37 +53,25 @@ class PlugwiseSwitchEntity(PlugwiseEntity, SwitchEntity): self._attr_unique_id = f"{device_id}-{description.key}" self._attr_name = coordinator.data.devices[device_id].get("name") + @plugwise_command async def async_turn_on(self, **kwargs: Any) -> None: """Turn the device on.""" - try: - state_on = await self.coordinator.api.set_switch_state( - self._dev_id, - self.coordinator.data.devices[self._dev_id].get("members"), - self.entity_description.key, - "on", - ) - except PlugwiseException: - LOGGER.error("Error while communicating to device") - else: - if state_on: - self._attr_is_on = True - self.async_write_ha_state() + await self.coordinator.api.set_switch_state( + self._dev_id, + self.coordinator.data.devices[self._dev_id].get("members"), + self.entity_description.key, + "on", + ) + @plugwise_command async def async_turn_off(self, **kwargs: Any) -> None: """Turn the device off.""" - try: - state_off = await self.coordinator.api.set_switch_state( - self._dev_id, - self.coordinator.data.devices[self._dev_id].get("members"), - self.entity_description.key, - "off", - ) - except PlugwiseException: - LOGGER.error("Error while communicating to device") - else: - if state_off: - self._attr_is_on = False - self.async_write_ha_state() + await self.coordinator.api.set_switch_state( + self._dev_id, + self.coordinator.data.devices[self._dev_id].get("members"), + self.entity_description.key, + "off", + ) @callback def _handle_coordinator_update(self) -> None: diff --git a/homeassistant/components/plugwise/util.py b/homeassistant/components/plugwise/util.py new file mode 100644 index 00000000000..58c7715815e --- /dev/null +++ b/homeassistant/components/plugwise/util.py @@ -0,0 +1,36 @@ +"""Utilities for Plugwise.""" +from collections.abc import Awaitable, Callable, Coroutine +from typing import Any, TypeVar + +from plugwise.exceptions import PlugwiseException +from typing_extensions import Concatenate, ParamSpec + +from homeassistant.exceptions import HomeAssistantError + +from .entity import PlugwiseEntity + +_P = ParamSpec("_P") +_R = TypeVar("_R") +_T = TypeVar("_T", bound=PlugwiseEntity) + + +def plugwise_command( + func: Callable[Concatenate[_T, _P], Awaitable[_R]] # type: ignore[misc] +) -> Callable[Concatenate[_T, _P], Coroutine[Any, Any, _R]]: # type: ignore[misc] + """Decorate Plugwise calls that send commands/make changes to the device. + + A decorator that wraps the passed in function, catches Plugwise errors, + and requests an coordinator update to update status of the devices asap. + """ + + async def handler(self: _T, *args: _P.args, **kwargs: _P.kwargs) -> _R: + try: + return await func(self, *args, **kwargs) + except PlugwiseException as error: + raise HomeAssistantError( + f"Error communicating with API: {error}" + ) from error + finally: + await self.coordinator.async_request_refresh() + + return handler diff --git a/tests/components/plugwise/test_climate.py b/tests/components/plugwise/test_climate.py index a7af9f9c0c9..5eb60ebeb6f 100644 --- a/tests/components/plugwise/test_climate.py +++ b/tests/components/plugwise/test_climate.py @@ -1,6 +1,7 @@ """Tests for the Plugwise Climate integration.""" from plugwise.exceptions import PlugwiseException +import pytest from homeassistant.components.climate.const import ( HVAC_MODE_AUTO, @@ -8,6 +9,7 @@ from homeassistant.components.climate.const import ( HVAC_MODE_OFF, ) from homeassistant.config_entries import ConfigEntryState +from homeassistant.exceptions import HomeAssistantError from tests.components.plugwise.common import async_init_integration @@ -56,32 +58,38 @@ async def test_adam_climate_adjust_negative_testing(hass, mock_smile_adam): entry = await async_init_integration(hass, mock_smile_adam) assert entry.state is ConfigEntryState.LOADED - await hass.services.async_call( - "climate", - "set_temperature", - {"entity_id": "climate.zone_lisa_wk", "temperature": 25}, - blocking=True, - ) + with pytest.raises(HomeAssistantError): + await hass.services.async_call( + "climate", + "set_temperature", + {"entity_id": "climate.zone_lisa_wk", "temperature": 25}, + blocking=True, + ) state = hass.states.get("climate.zone_lisa_wk") attrs = state.attributes assert attrs["temperature"] == 21.5 - await hass.services.async_call( - "climate", - "set_preset_mode", - {"entity_id": "climate.zone_thermostat_jessie", "preset_mode": "home"}, - blocking=True, - ) + with pytest.raises(HomeAssistantError): + await hass.services.async_call( + "climate", + "set_preset_mode", + {"entity_id": "climate.zone_thermostat_jessie", "preset_mode": "home"}, + blocking=True, + ) state = hass.states.get("climate.zone_thermostat_jessie") attrs = state.attributes assert attrs["preset_mode"] == "asleep" - await hass.services.async_call( - "climate", - "set_hvac_mode", - {"entity_id": "climate.zone_thermostat_jessie", "hvac_mode": HVAC_MODE_AUTO}, - blocking=True, - ) + with pytest.raises(HomeAssistantError): + await hass.services.async_call( + "climate", + "set_hvac_mode", + { + "entity_id": "climate.zone_thermostat_jessie", + "hvac_mode": HVAC_MODE_AUTO, + }, + blocking=True, + ) state = hass.states.get("climate.zone_thermostat_jessie") attrs = state.attributes @@ -97,10 +105,11 @@ async def test_adam_climate_entity_climate_changes(hass, mock_smile_adam): {"entity_id": "climate.zone_lisa_wk", "temperature": 25}, blocking=True, ) - state = hass.states.get("climate.zone_lisa_wk") - attrs = state.attributes - assert attrs["temperature"] == 25.0 + assert mock_smile_adam.set_temperature.call_count == 1 + mock_smile_adam.set_temperature.assert_called_with( + "c50f167537524366a5af7aa3942feb1e", 25.0 + ) await hass.services.async_call( "climate", @@ -108,12 +117,11 @@ async def test_adam_climate_entity_climate_changes(hass, mock_smile_adam): {"entity_id": "climate.zone_lisa_wk", "preset_mode": "away"}, blocking=True, ) - state = hass.states.get("climate.zone_lisa_wk") - attrs = state.attributes - assert attrs["preset_mode"] == "away" - - assert attrs["supported_features"] == 17 + assert mock_smile_adam.set_preset.call_count == 1 + mock_smile_adam.set_preset.assert_called_with( + "c50f167537524366a5af7aa3942feb1e", "away" + ) await hass.services.async_call( "climate", @@ -122,10 +130,10 @@ async def test_adam_climate_entity_climate_changes(hass, mock_smile_adam): blocking=True, ) - state = hass.states.get("climate.zone_thermostat_jessie") - attrs = state.attributes - - assert attrs["temperature"] == 25.0 + assert mock_smile_adam.set_temperature.call_count == 2 + mock_smile_adam.set_temperature.assert_called_with( + "82fa13f017d240daa0d0ea1775420f24", 25.0 + ) await hass.services.async_call( "climate", @@ -133,10 +141,11 @@ async def test_adam_climate_entity_climate_changes(hass, mock_smile_adam): {"entity_id": "climate.zone_thermostat_jessie", "preset_mode": "home"}, blocking=True, ) - state = hass.states.get("climate.zone_thermostat_jessie") - attrs = state.attributes - assert attrs["preset_mode"] == "home" + assert mock_smile_adam.set_preset.call_count == 2 + mock_smile_adam.set_preset.assert_called_with( + "82fa13f017d240daa0d0ea1775420f24", "home" + ) async def test_anna_climate_entity_attributes(hass, mock_smile_anna): @@ -176,10 +185,10 @@ async def test_anna_climate_entity_climate_changes(hass, mock_smile_anna): blocking=True, ) - state = hass.states.get("climate.anna") - attrs = state.attributes - - assert attrs["temperature"] == 25.0 + assert mock_smile_anna.set_temperature.call_count == 1 + mock_smile_anna.set_temperature.assert_called_with( + "c784ee9fdab44e1395b8dee7d7a497d5", 25.0 + ) await hass.services.async_call( "climate", @@ -188,10 +197,10 @@ async def test_anna_climate_entity_climate_changes(hass, mock_smile_anna): blocking=True, ) - state = hass.states.get("climate.anna") - attrs = state.attributes - - assert attrs["preset_mode"] == "away" + assert mock_smile_anna.set_preset.call_count == 1 + mock_smile_anna.set_preset.assert_called_with( + "c784ee9fdab44e1395b8dee7d7a497d5", "away" + ) await hass.services.async_call( "climate", @@ -200,7 +209,8 @@ async def test_anna_climate_entity_climate_changes(hass, mock_smile_anna): blocking=True, ) - state = hass.states.get("climate.anna") - attrs = state.attributes - - assert state.state == "heat_cool" + assert mock_smile_anna.set_temperature.call_count == 1 + assert mock_smile_anna.set_schedule_state.call_count == 1 + mock_smile_anna.set_schedule_state.assert_called_with( + "c784ee9fdab44e1395b8dee7d7a497d5", None, "false" + ) diff --git a/tests/components/plugwise/test_switch.py b/tests/components/plugwise/test_switch.py index 4d09489944d..2816b2dece6 100644 --- a/tests/components/plugwise/test_switch.py +++ b/tests/components/plugwise/test_switch.py @@ -1,10 +1,11 @@ """Tests for the Plugwise switch integration.""" - from plugwise.exceptions import PlugwiseException +import pytest from homeassistant.components.plugwise.const import DOMAIN from homeassistant.components.switch import DOMAIN as SWITCH_DOMAIN from homeassistant.config_entries import ConfigEntryState +from homeassistant.exceptions import HomeAssistantError from homeassistant.helpers import entity_registry as er from tests.common import MockConfigEntry @@ -29,23 +30,31 @@ async def test_adam_climate_switch_negative_testing(hass, mock_smile_adam): entry = await async_init_integration(hass, mock_smile_adam) assert entry.state is ConfigEntryState.LOADED - await hass.services.async_call( - "switch", - "turn_off", - {"entity_id": "switch.cv_pomp"}, - blocking=True, - ) - state = hass.states.get("switch.cv_pomp") - assert str(state.state) == "on" + with pytest.raises(HomeAssistantError): + await hass.services.async_call( + "switch", + "turn_off", + {"entity_id": "switch.cv_pomp"}, + blocking=True, + ) - await hass.services.async_call( - "switch", - "turn_on", - {"entity_id": "switch.fibaro_hc2"}, - blocking=True, + assert mock_smile_adam.set_switch_state.call_count == 1 + mock_smile_adam.set_switch_state.assert_called_with( + "78d1126fc4c743db81b61c20e88342a7", None, "relay", "off" + ) + + with pytest.raises(HomeAssistantError): + await hass.services.async_call( + "switch", + "turn_on", + {"entity_id": "switch.fibaro_hc2"}, + blocking=True, + ) + + assert mock_smile_adam.set_switch_state.call_count == 2 + mock_smile_adam.set_switch_state.assert_called_with( + "a28f588dc4a049a483fd03a30361ad3a", None, "relay", "on" ) - state = hass.states.get("switch.fibaro_hc2") - assert str(state.state) == "on" async def test_adam_climate_switch_changes(hass, mock_smile_adam): @@ -59,8 +68,11 @@ async def test_adam_climate_switch_changes(hass, mock_smile_adam): {"entity_id": "switch.cv_pomp"}, blocking=True, ) - state = hass.states.get("switch.cv_pomp") - assert str(state.state) == "off" + + assert mock_smile_adam.set_switch_state.call_count == 1 + mock_smile_adam.set_switch_state.assert_called_with( + "78d1126fc4c743db81b61c20e88342a7", None, "relay", "off" + ) await hass.services.async_call( "switch", @@ -68,17 +80,23 @@ async def test_adam_climate_switch_changes(hass, mock_smile_adam): {"entity_id": "switch.fibaro_hc2"}, blocking=True, ) - state = hass.states.get("switch.fibaro_hc2") - assert str(state.state) == "off" + + assert mock_smile_adam.set_switch_state.call_count == 2 + mock_smile_adam.set_switch_state.assert_called_with( + "a28f588dc4a049a483fd03a30361ad3a", None, "relay", "off" + ) await hass.services.async_call( "switch", - "toggle", + "turn_on", {"entity_id": "switch.fibaro_hc2"}, blocking=True, ) - state = hass.states.get("switch.fibaro_hc2") - assert str(state.state) == "on" + + assert mock_smile_adam.set_switch_state.call_count == 3 + mock_smile_adam.set_switch_state.assert_called_with( + "a28f588dc4a049a483fd03a30361ad3a", None, "relay", "on" + ) async def test_stretch_switch_entities(hass, mock_stretch): @@ -104,9 +122,10 @@ async def test_stretch_switch_changes(hass, mock_stretch): {"entity_id": "switch.koelkast_92c4a"}, blocking=True, ) - - state = hass.states.get("switch.koelkast_92c4a") - assert str(state.state) == "off" + assert mock_stretch.set_switch_state.call_count == 1 + mock_stretch.set_switch_state.assert_called_with( + "e1c884e7dede431dadee09506ec4f859", None, "relay", "off" + ) await hass.services.async_call( "switch", @@ -114,17 +133,21 @@ async def test_stretch_switch_changes(hass, mock_stretch): {"entity_id": "switch.droger_52559"}, blocking=True, ) - state = hass.states.get("switch.droger_52559") - assert str(state.state) == "off" + assert mock_stretch.set_switch_state.call_count == 2 + mock_stretch.set_switch_state.assert_called_with( + "cfe95cf3de1948c0b8955125bf754614", None, "relay", "off" + ) await hass.services.async_call( "switch", - "toggle", + "turn_on", {"entity_id": "switch.droger_52559"}, blocking=True, ) - state = hass.states.get("switch.droger_52559") - assert str(state.state) == "on" + assert mock_stretch.set_switch_state.call_count == 3 + mock_stretch.set_switch_state.assert_called_with( + "cfe95cf3de1948c0b8955125bf754614", None, "relay", "on" + ) async def test_unique_id_migration_plug_relay(hass, mock_smile_adam):