Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up OpenUV config flow #17349

Merged
merged 10 commits into from
Oct 15, 2018
Merged

Clean up OpenUV config flow #17349

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a0f3715
Cleaned up OpenUV config flow
bachya Oct 12, 2018
a63c95e
Added proper listener removal
bachya Oct 12, 2018
6e71810
Added proper exception
bachya Oct 12, 2018
a1d31fe
Unnecessary exception message
bachya Oct 12, 2018
fe7704a
Moved API key error to correct place
bachya Oct 12, 2018
851ff3f
Member-requested changes (part 1)
bachya Oct 12, 2018
31d25ad
Hound
bachya Oct 12, 2018
2782afa
Member-requested changes (part 2)
bachya Oct 12, 2018
478fd17
Cleanup
bachya Oct 12, 2018
b4f6010
Fixed tests
bachya Oct 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions homeassistant/components/binary_sensor/openuv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,12 @@ def __init__(self, openuv, sensor_type, name, icon, entry_id):
"""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

Expand All @@ -80,16 +80,20 @@ def unique_id(self) -> str:
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."""
Expand Down
94 changes: 43 additions & 51 deletions homeassistant/components/openuv/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,14 @@
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__)
Expand All @@ -31,7 +32,6 @@
DATA_UV = 'uv'

DEFAULT_ATTRIBUTION = 'Data provided by OpenUV'
DEFAULT_SCAN_INTERVAL = timedelta(minutes=30)

NOTIFICATION_ID = 'openuv_notification'
NOTIFICATION_TITLE = 'OpenUV Component Setup'
Expand Down Expand Up @@ -85,18 +85,17 @@
})

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)


Expand All @@ -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

Expand All @@ -145,56 +143,50 @@ 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(
CONF_MONITORED_CONDITIONS, list(SENSORS)))
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}<br />'
'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()
async_dispatcher_send(hass, TOPIC_UPDATE)

hass.data[DOMAIN][DATA_OPENUV_LISTENER][
config_entry.entry_id] = async_track_time_interval(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsubscribe this listener before unloading the platforms. Otherwise there could perhaps be a race condition and another update come in before we disconnect the update callbacks in the platforms.

Copy link
Member

@MartinHjelmare MartinHjelmare Oct 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we do this, there could perhaps be a race if the state update has already been scheduled?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does 851ff3f address what you were thinking?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I'm not sure it actually prevents anything. I was thinking about the problem you described earlier, and that a scheduled update could be the cause.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Let me experiment with removing the dispatcher altogether and see if that clears up my prior issue.

Copy link
Contributor Author

@bachya bachya Oct 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried commenting all out all references to the dispatcher, but when I remove the config entry, I'm back to getting this weird exception:

Traceback (most recent call last):
  File "/Users/abach/.local/share/virtualenvs/home-assistant-E4Cz_ciq/lib/python3.7/site-packages/aiohttp/web_protocol.py", line 390, in start
    resp = await self._request_handler(request)
  File "/Users/abach/.local/share/virtualenvs/home-assistant-E4Cz_ciq/lib/python3.7/site-packages/aiohttp/web_app.py", line 366, in _handle
    resp = await handler(request)
  File "/Users/abach/.local/share/virtualenvs/home-assistant-E4Cz_ciq/lib/python3.7/site-packages/aiohttp/web_middlewares.py", line 106, in impl
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/static.py", line 66, in staticresource_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/real_ip.py", line 34, in real_ip_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/ban.py", line 66, in ban_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/auth.py", line 68, in auth_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/real_ip.py", line 34, in real_ip_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/ban.py", line 66, in ban_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/auth.py", line 68, in auth_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/view.py", line 113, in handle
    result = await result
  File "/Users/abach/Git/home-assistant/homeassistant/components/config/config_entries.py", line 69, in delete
    result = await hass.config_entries.async_remove(entry_id)
  File "/Users/abach/Git/home-assistant/homeassistant/config_entries.py", line 392, in async_remove
    entity_registry.async_clear_config_entry(entry_id)
  File "/Users/abach/Git/home-assistant/homeassistant/helpers/entity_registry.py", line 254, in async_clear_config_entry
    self._async_update_entity(entity_id, config_entry_id=None)
  File "/Users/abach/Git/home-assistant/homeassistant/helpers/entity_registry.py", line 196, in _async_update_entity
    new.update_listeners.remove(ref)
ValueError: list.remove(x): x not in list

I don't understand what listener the config entry is trying to remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, tracking this issue separately: #17370.

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()

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank line contains whitespace

for component in ('binary_sensor', 'sensor'):
await hass.config_entries.async_forward_entry_unload(
config_entry, component)

return True

Expand Down
84 changes: 50 additions & 34 deletions homeassistant/components/openuv/config_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@
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
Expand All @@ -31,7 +32,19 @@ class OpenUvFlowHandler(config_entries.ConfigFlow):

def __init__(self):
"""Initialize the config flow."""
pass
self.data_schema = OrderedDict()
self.data_schema[vol.Required(CONF_API_KEY)] = str
self.data_schema[vol.Optional(CONF_LATITUDE)] = cv.latitude
self.data_schema[vol.Optional(CONF_LONGITUDE)] = cv.longitude
self.data_schema[vol.Optional(CONF_ELEVATION)] = vol.Coerce(float)

async def _show_form(self, errors=None):
"""Show the form to the user."""
return self.async_show_form(
step_id='user',
data_schema=vol.Schema(self.data_schema),
errors=errors if errors else {},
)

async def async_step_import(self, import_config):
"""Import a config entry from configuration.yaml."""
Expand All @@ -41,34 +54,37 @@ async def async_step_user(self, user_input=None):
"""Handle the start of the config flow."""
from pyopenuv.util import validate_api_key

errors = {}

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))

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'

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)

return self.async_show_form(
step_id='user',
data_schema=vol.Schema(data_schema),
errors=errors,
)
if not user_input:
return await self._show_form()

latitude = user_input.get(CONF_LATITUDE)
if not latitude:
latitude = self.hass.config.latitude
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move defaults to form schema?

longitude = user_input.get(CONF_LONGITUDE)
if not longitude:
longitude = self.hass.config.longitude
elevation = user_input.get(CONF_ELEVATION)
if not elevation:
elevation = self.hass.config.elevation

identifier = '{0}, {1}'.format(latitude, longitude)
if identifier in configured_instances(self.hass):
return await self._show_form({CONF_LATITUDE: 'identifier_exists'})

websession = aiohttp_client.async_get_clientsession(self.hass)
api_key_validation = await validate_api_key(
user_input[CONF_API_KEY], websession)

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)
3 changes: 3 additions & 0 deletions homeassistant/components/openuv/const.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
"""Define constants for the OpenUV component."""
from datetime import timedelta

DOMAIN = 'openuv'

DEFAULT_SCAN_INTERVAL = timedelta(minutes=30)
22 changes: 13 additions & 9 deletions homeassistant/components/sensor/openuv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def __init__(self, openuv, sensor_type, name, icon, unit, entry_id):
"""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
Expand Down Expand Up @@ -100,16 +100,20 @@ def unit_of_measurement(self):
"""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."""
Expand Down
Loading