From 29c2b2fe79c9b6f26157e65d3f9cf036f138d0fd Mon Sep 17 00:00:00 2001 From: Aaron Bach Date: Mon, 15 Oct 2018 13:21:21 -0600 Subject: [PATCH] Clean up OpenUV config flow (#17349) * Cleaned up OpenUV config flow * Added proper listener removal * Added proper exception * Unnecessary exception message * Moved API key error to correct place * Member-requested changes (part 1) * Hound * Member-requested changes (part 2) * Cleanup * Fixed tests --- .../components/binary_sensor/openuv.py | 22 +++-- homeassistant/components/openuv/__init__.py | 94 +++++++++---------- .../components/openuv/config_flow.py | 76 +++++++++------ homeassistant/components/openuv/const.py | 3 + homeassistant/components/sensor/openuv.py | 22 +++-- tests/components/openuv/test_config_flow.py | 66 ++++++++----- 6 files changed, 158 insertions(+), 125 deletions(-) diff --git a/homeassistant/components/binary_sensor/openuv.py b/homeassistant/components/binary_sensor/openuv.py index bd6e4d1d5dc..3e9bb0b0bc3 100644 --- a/homeassistant/components/binary_sensor/openuv.py +++ b/homeassistant/components/binary_sensor/openuv.py @@ -50,12 +50,12 @@ class OpenUvBinarySensor(OpenUvEntity, BinarySensorDevice): """Initialize the sensor.""" super().__init__(openuv) + self._async_unsub_dispatcher_connect = None self._entry_id = entry_id self._icon = icon self._latitude = openuv.client.latitude self._longitude = openuv.client.longitude self._name = name - self._dispatch_remove = None self._sensor_type = sensor_type self._state = None @@ -80,16 +80,20 @@ class OpenUvBinarySensor(OpenUvEntity, BinarySensorDevice): return '{0}_{1}_{2}'.format( self._latitude, self._longitude, self._sensor_type) - @callback - def _update_data(self): - """Update the state.""" - self.async_schedule_update_ha_state(True) - async def async_added_to_hass(self): """Register callbacks.""" - self._dispatch_remove = async_dispatcher_connect( - self.hass, TOPIC_UPDATE, self._update_data) - self.async_on_remove(self._dispatch_remove) + @callback + def update(): + """Update the state.""" + self.async_schedule_update_ha_state(True) + + self._async_unsub_dispatcher_connect = async_dispatcher_connect( + self.hass, TOPIC_UPDATE, update) + + async def async_will_remove_from_hass(self): + """Disconnect dispatcher listener when removed.""" + if self._async_unsub_dispatcher_connect: + self._async_unsub_dispatcher_connect() async def async_update(self): """Update the state.""" diff --git a/homeassistant/components/openuv/__init__.py b/homeassistant/components/openuv/__init__.py index 8485e1e3201..a45d9ceb0d6 100644 --- a/homeassistant/components/openuv/__init__.py +++ b/homeassistant/components/openuv/__init__.py @@ -14,13 +14,14 @@ from homeassistant.const import ( ATTR_ATTRIBUTION, CONF_API_KEY, CONF_BINARY_SENSORS, CONF_ELEVATION, CONF_LATITUDE, CONF_LONGITUDE, CONF_MONITORED_CONDITIONS, CONF_SCAN_INTERVAL, CONF_SENSORS) +from homeassistant.exceptions import ConfigEntryNotReady from homeassistant.helpers import aiohttp_client, config_validation as cv from homeassistant.helpers.dispatcher import async_dispatcher_send from homeassistant.helpers.entity import Entity from homeassistant.helpers.event import async_track_time_interval from .config_flow import configured_instances -from .const import DOMAIN +from .const import DEFAULT_SCAN_INTERVAL, DOMAIN REQUIREMENTS = ['pyopenuv==1.0.4'] _LOGGER = logging.getLogger(__name__) @@ -31,7 +32,6 @@ DATA_PROTECTION_WINDOW = 'protection_window' DATA_UV = 'uv' DEFAULT_ATTRIBUTION = 'Data provided by OpenUV' -DEFAULT_SCAN_INTERVAL = timedelta(minutes=30) NOTIFICATION_ID = 'openuv_notification' NOTIFICATION_TITLE = 'OpenUV Component Setup' @@ -85,18 +85,17 @@ SENSOR_SCHEMA = vol.Schema({ }) CONFIG_SCHEMA = vol.Schema({ - DOMAIN: - vol.Schema({ - vol.Required(CONF_API_KEY): cv.string, - vol.Optional(CONF_ELEVATION): float, - vol.Optional(CONF_LATITUDE): cv.latitude, - vol.Optional(CONF_LONGITUDE): cv.longitude, - vol.Optional(CONF_BINARY_SENSORS, default={}): - BINARY_SENSOR_SCHEMA, - vol.Optional(CONF_SENSORS, default={}): SENSOR_SCHEMA, - vol.Optional(CONF_SCAN_INTERVAL, default=DEFAULT_SCAN_INTERVAL): - cv.time_period, - }) + DOMAIN: vol.Schema({ + vol.Required(CONF_API_KEY): cv.string, + vol.Optional(CONF_ELEVATION): float, + vol.Optional(CONF_LATITUDE): cv.latitude, + vol.Optional(CONF_LONGITUDE): cv.longitude, + vol.Optional(CONF_BINARY_SENSORS, default={}): + BINARY_SENSOR_SCHEMA, + vol.Optional(CONF_SENSORS, default={}): SENSOR_SCHEMA, + vol.Optional(CONF_SCAN_INTERVAL, default=DEFAULT_SCAN_INTERVAL): + cv.time_period, + }) }, extra=vol.ALLOW_EXTRA) @@ -110,27 +109,26 @@ async def async_setup(hass, config): return True conf = config[DOMAIN] - latitude = conf.get(CONF_LATITUDE, hass.config.latitude) - longitude = conf.get(CONF_LONGITUDE, hass.config.longitude) - elevation = conf.get(CONF_ELEVATION, hass.config.elevation) + latitude = conf.get(CONF_LATITUDE) + longitude = conf.get(CONF_LONGITUDE) identifier = '{0}, {1}'.format(latitude, longitude) + if identifier in configured_instances(hass): + return True - if identifier not in configured_instances(hass): - hass.async_add_job( - hass.config_entries.flow.async_init( - DOMAIN, - context={'source': SOURCE_IMPORT}, - data={ - CONF_API_KEY: conf[CONF_API_KEY], - CONF_LATITUDE: latitude, - CONF_LONGITUDE: longitude, - CONF_ELEVATION: elevation, - CONF_BINARY_SENSORS: conf[CONF_BINARY_SENSORS], - CONF_SENSORS: conf[CONF_SENSORS], - })) - - hass.data[DOMAIN][CONF_SCAN_INTERVAL] = conf[CONF_SCAN_INTERVAL] + hass.async_create_task( + hass.config_entries.flow.async_init( + DOMAIN, + context={'source': SOURCE_IMPORT}, + data={ + CONF_API_KEY: conf[CONF_API_KEY], + CONF_LATITUDE: latitude, + CONF_LONGITUDE: longitude, + CONF_ELEVATION: conf.get(CONF_ELEVATION), + CONF_BINARY_SENSORS: conf[CONF_BINARY_SENSORS], + CONF_SENSORS: conf[CONF_SENSORS], + CONF_SCAN_INTERVAL: conf[CONF_SCAN_INTERVAL], + })) return True @@ -145,11 +143,10 @@ async def async_setup_entry(hass, config_entry): openuv = OpenUV( Client( config_entry.data[CONF_API_KEY], - config_entry.data.get(CONF_LATITUDE, hass.config.latitude), - config_entry.data.get(CONF_LONGITUDE, hass.config.longitude), + config_entry.data[CONF_LATITUDE], + config_entry.data[CONF_LONGITUDE], websession, - altitude=config_entry.data.get( - CONF_ELEVATION, hass.config.elevation)), + altitude=config_entry.data[CONF_ELEVATION]), config_entry.data.get(CONF_BINARY_SENSORS, {}).get( CONF_MONITORED_CONDITIONS, list(BINARY_SENSORS)), config_entry.data.get(CONF_SENSORS, {}).get( @@ -157,20 +154,14 @@ async def async_setup_entry(hass, config_entry): await openuv.async_update() hass.data[DOMAIN][DATA_OPENUV_CLIENT][config_entry.entry_id] = openuv except OpenUvError as err: - _LOGGER.error('An error occurred: %s', str(err)) - hass.components.persistent_notification.create( - 'Error: {0}
' - 'You will need to restart hass after fixing.' - ''.format(err), - title=NOTIFICATION_TITLE, - notification_id=NOTIFICATION_ID) - return False + _LOGGER.error('Config entry failed: %s', err) + raise ConfigEntryNotReady for component in ('binary_sensor', 'sensor'): hass.async_create_task(hass.config_entries.async_forward_entry_setup( config_entry, component)) - async def refresh_sensors(event_time): + async def refresh(event_time): """Refresh OpenUV data.""" _LOGGER.debug('Refreshing OpenUV data') await openuv.async_update() @@ -178,24 +169,25 @@ async def async_setup_entry(hass, config_entry): hass.data[DOMAIN][DATA_OPENUV_LISTENER][ config_entry.entry_id] = async_track_time_interval( - hass, refresh_sensors, - hass.data[DOMAIN].get(CONF_SCAN_INTERVAL, DEFAULT_SCAN_INTERVAL)) + hass, + refresh, + timedelta(seconds=config_entry.data[CONF_SCAN_INTERVAL])) return True async def async_unload_entry(hass, config_entry): """Unload an OpenUV config entry.""" - for component in ('binary_sensor', 'sensor'): - await hass.config_entries.async_forward_entry_unload( - config_entry, component) - hass.data[DOMAIN][DATA_OPENUV_CLIENT].pop(config_entry.entry_id) remove_listener = hass.data[DOMAIN][DATA_OPENUV_LISTENER].pop( config_entry.entry_id) remove_listener() + for component in ('binary_sensor', 'sensor'): + await hass.config_entries.async_forward_entry_unload( + config_entry, component) + return True diff --git a/homeassistant/components/openuv/config_flow.py b/homeassistant/components/openuv/config_flow.py index 6d7ae0f65bd..27ffe5c3985 100644 --- a/homeassistant/components/openuv/config_flow.py +++ b/homeassistant/components/openuv/config_flow.py @@ -1,16 +1,15 @@ """Config flow to configure the OpenUV component.""" -from collections import OrderedDict - import voluptuous as vol from homeassistant import config_entries from homeassistant.core import callback from homeassistant.const import ( - CONF_API_KEY, CONF_ELEVATION, CONF_LATITUDE, CONF_LONGITUDE) + CONF_API_KEY, CONF_ELEVATION, CONF_LATITUDE, CONF_LONGITUDE, + CONF_SCAN_INTERVAL) from homeassistant.helpers import aiohttp_client, config_validation as cv -from .const import DOMAIN +from .const import DEFAULT_SCAN_INTERVAL, DOMAIN @callback @@ -33,6 +32,24 @@ class OpenUvFlowHandler(config_entries.ConfigFlow): """Initialize the config flow.""" pass + async def _show_form(self, errors=None): + """Show the form to the user.""" + data_schema = vol.Schema({ + vol.Required(CONF_API_KEY): str, + vol.Optional(CONF_LATITUDE, default=self.hass.config.latitude): + cv.latitude, + vol.Optional(CONF_LONGITUDE, default=self.hass.config.longitude): + cv.longitude, + vol.Optional(CONF_ELEVATION, default=self.hass.config.elevation): + vol.Coerce(float), + }) + + return self.async_show_form( + step_id='user', + data_schema=data_schema, + errors=errors if errors else {}, + ) + async def async_step_import(self, import_config): """Import a config entry from configuration.yaml.""" return await self.async_step_user(import_config) @@ -41,34 +58,31 @@ class OpenUvFlowHandler(config_entries.ConfigFlow): """Handle the start of the config flow.""" from pyopenuv.util import validate_api_key - errors = {} + if not user_input: + return await self._show_form() - if user_input is not None: - identifier = '{0}, {1}'.format( - user_input.get(CONF_LATITUDE, self.hass.config.latitude), - user_input.get(CONF_LONGITUDE, self.hass.config.longitude)) + latitude = user_input[CONF_LATITUDE] + longitude = user_input[CONF_LONGITUDE] + elevation = user_input[CONF_ELEVATION] - if identifier in configured_instances(self.hass): - errors['base'] = 'identifier_exists' - else: - websession = aiohttp_client.async_get_clientsession(self.hass) - api_key_validation = await validate_api_key( - user_input[CONF_API_KEY], websession) - if api_key_validation: - return self.async_create_entry( - title=identifier, - data=user_input, - ) - errors['base'] = 'invalid_api_key' + identifier = '{0}, {1}'.format(latitude, longitude) + if identifier in configured_instances(self.hass): + return await self._show_form({CONF_LATITUDE: 'identifier_exists'}) - data_schema = OrderedDict() - data_schema[vol.Required(CONF_API_KEY)] = str - data_schema[vol.Optional(CONF_LATITUDE)] = cv.latitude - data_schema[vol.Optional(CONF_LONGITUDE)] = cv.longitude - data_schema[vol.Optional(CONF_ELEVATION)] = vol.Coerce(float) + websession = aiohttp_client.async_get_clientsession(self.hass) + api_key_validation = await validate_api_key( + user_input[CONF_API_KEY], websession) - return self.async_show_form( - step_id='user', - data_schema=vol.Schema(data_schema), - errors=errors, - ) + if not api_key_validation: + return await self._show_form({CONF_API_KEY: 'invalid_api_key'}) + + scan_interval = user_input.get( + CONF_SCAN_INTERVAL, DEFAULT_SCAN_INTERVAL) + user_input.update({ + CONF_LATITUDE: latitude, + CONF_LONGITUDE: longitude, + CONF_ELEVATION: elevation, + CONF_SCAN_INTERVAL: scan_interval.seconds, + }) + + return self.async_create_entry(title=identifier, data=user_input) diff --git a/homeassistant/components/openuv/const.py b/homeassistant/components/openuv/const.py index 1aa3d2abcaa..16623e45642 100644 --- a/homeassistant/components/openuv/const.py +++ b/homeassistant/components/openuv/const.py @@ -1,3 +1,6 @@ """Define constants for the OpenUV component.""" +from datetime import timedelta DOMAIN = 'openuv' + +DEFAULT_SCAN_INTERVAL = timedelta(minutes=30) diff --git a/homeassistant/components/sensor/openuv.py b/homeassistant/components/sensor/openuv.py index 22712aa306b..63527db42a6 100644 --- a/homeassistant/components/sensor/openuv.py +++ b/homeassistant/components/sensor/openuv.py @@ -64,7 +64,7 @@ class OpenUvSensor(OpenUvEntity): """Initialize the sensor.""" super().__init__(openuv) - self._dispatch_remove = None + self._async_unsub_dispatcher_connect = None self._entry_id = entry_id self._icon = icon self._latitude = openuv.client.latitude @@ -100,16 +100,20 @@ class OpenUvSensor(OpenUvEntity): """Return the unit the value is expressed in.""" return self._unit - @callback - def _update_data(self): - """Update the state.""" - self.async_schedule_update_ha_state(True) - async def async_added_to_hass(self): """Register callbacks.""" - self._dispatch_remove = async_dispatcher_connect( - self.hass, TOPIC_UPDATE, self._update_data) - self.async_on_remove(self._dispatch_remove) + @callback + def update(): + """Update the state.""" + self.async_schedule_update_ha_state(True) + + self._async_unsub_dispatcher_connect = async_dispatcher_connect( + self.hass, TOPIC_UPDATE, update) + + async def async_will_remove_from_hass(self): + """Disconnect dispatcher listener when removed.""" + if self._async_unsub_dispatcher_connect: + self._async_unsub_dispatcher_connect() async def async_update(self): """Update the state.""" diff --git a/tests/components/openuv/test_config_flow.py b/tests/components/openuv/test_config_flow.py index 07946fbbc09..60aa990333f 100644 --- a/tests/components/openuv/test_config_flow.py +++ b/tests/components/openuv/test_config_flow.py @@ -1,10 +1,12 @@ """Define tests for the OpenUV config flow.""" +from datetime import timedelta from unittest.mock import patch from homeassistant import data_entry_flow from homeassistant.components.openuv import DOMAIN, config_flow from homeassistant.const import ( - CONF_API_KEY, CONF_ELEVATION, CONF_LATITUDE, CONF_LONGITUDE) + CONF_API_KEY, CONF_ELEVATION, CONF_LATITUDE, CONF_LONGITUDE, + CONF_SCAN_INTERVAL) from tests.common import MockConfigEntry, mock_coro @@ -23,7 +25,7 @@ async def test_duplicate_error(hass): flow.hass = hass result = await flow.async_step_user(user_input=conf) - assert result['errors'] == {'base': 'identifier_exists'} + assert result['errors'] == {CONF_LATITUDE: 'identifier_exists'} async def test_invalid_api_key(hass): @@ -41,7 +43,7 @@ async def test_invalid_api_key(hass): with patch('pyopenuv.util.validate_api_key', return_value=mock_coro(False)): result = await flow.async_step_user(user_input=conf) - assert result['errors'] == {'base': 'invalid_api_key'} + assert result['errors'] == {CONF_API_KEY: 'invalid_api_key'} async def test_show_form(hass): @@ -57,25 +59,6 @@ async def test_show_form(hass): async def test_step_import(hass): """Test that the import step works.""" - conf = { - CONF_API_KEY: '12345abcde', - } - - flow = config_flow.OpenUvFlowHandler() - flow.hass = hass - - with patch('pyopenuv.util.validate_api_key', - return_value=mock_coro(True)): - result = await flow.async_step_import(import_config=conf) - - assert result['type'] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY - assert result['title'] == '{0}, {1}'.format( - hass.config.latitude, hass.config.longitude) - assert result['data'] == conf - - -async def test_step_user(hass): - """Test that the user step works.""" conf = { CONF_API_KEY: '12345abcde', CONF_ELEVATION: 59.1234, @@ -86,11 +69,44 @@ async def test_step_user(hass): flow = config_flow.OpenUvFlowHandler() flow.hass = hass + with patch('pyopenuv.util.validate_api_key', + return_value=mock_coro(True)): + result = await flow.async_step_import(import_config=conf) + + assert result['type'] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY + assert result['title'] == '39.128712, -104.9812612' + assert result['data'] == { + CONF_API_KEY: '12345abcde', + CONF_ELEVATION: 59.1234, + CONF_LATITUDE: 39.128712, + CONF_LONGITUDE: -104.9812612, + CONF_SCAN_INTERVAL: 1800, + } + + +async def test_step_user(hass): + """Test that the user step works.""" + conf = { + CONF_API_KEY: '12345abcde', + CONF_ELEVATION: 59.1234, + CONF_LATITUDE: 39.128712, + CONF_LONGITUDE: -104.9812612, + CONF_SCAN_INTERVAL: timedelta(minutes=5) + } + + flow = config_flow.OpenUvFlowHandler() + flow.hass = hass + with patch('pyopenuv.util.validate_api_key', return_value=mock_coro(True)): result = await flow.async_step_user(user_input=conf) assert result['type'] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY - assert result['title'] == '{0}, {1}'.format( - conf[CONF_LATITUDE], conf[CONF_LONGITUDE]) - assert result['data'] == conf + assert result['title'] == '39.128712, -104.9812612' + assert result['data'] == { + CONF_API_KEY: '12345abcde', + CONF_ELEVATION: 59.1234, + CONF_LATITUDE: 39.128712, + CONF_LONGITUDE: -104.9812612, + CONF_SCAN_INTERVAL: 300, + }