Skip to content

Show persistent notification on Doorbird schedule failure #20033

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

Merged
fabaff merged 6 commits into from
Jan 13, 2019
+16 −5
Merged

Show persistent notification on Doorbird schedule failure #20033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bff1a07
Remove unnecessary return.
oblogic7 Jan 12, 2019
da8969a
Update doorbirdpy to 2.0.5
oblogic7 Jan 12, 2019
e414080
Fix bare except
oblogic7 Jan 12, 2019
5ee6347
Bump version again
oblogic7 Jan 12, 2019
b31539f
Lint
oblogic7 Jan 12, 2019
7ec4d0a
Return false
oblogic7 Jan 13, 2019
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
19 changes: 15 additions & 4 deletions homeassistant/components/doorbird.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"""
import logging

from urllib.error import HTTPError
import voluptuous as vol

from homeassistant.components.http import HomeAssistantView
Expand All @@ -14,7 +15,7 @@
import homeassistant.helpers.config_validation as cv
from homeassistant.util import slugify, dt as dt_util

REQUIREMENTS = ['doorbirdpy==2.0.4']
REQUIREMENTS = ['doorbirdpy==2.0.6']

_LOGGER = logging.getLogger(__name__)

Expand Down Expand Up @@ -113,7 +114,18 @@ def setup(hass, config):

# Subscribe to doorbell or motion events
if events:
doorstation.update_schedule(hass)
try:
doorstation.update_schedule(hass)
except HTTPError:
hass.components.persistent_notification.create(

@MartinHjelmare MartinHjelmare Jan 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't we just log an error and return False as the other setup errors do?

@MartinHjelmare MartinHjelmare Jan 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But should we not return False? We say they need to fix and restart. Why should we set up the component after this error? And components that fail set up already get a persistent notification.

@oblogic7 oblogic7 Jan 13, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Returning false will result in multiple notifications like below. The notification triggered by the false return implies that the issue is with the HA configuration. This is not accurate since it is truly an issue with the permissions on the user. The user is valid and has authenticated successfully, but does not have permission to set the schedule on the device. I believe the custom notification communicates this better and not having a return means unnecessary duplication doesn't happen on the notifications.

image

@MartinHjelmare MartinHjelmare Jan 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't set up the component if its not working properly. Please return False. It's ok to have two persistent notifications if you really want to have a custom one.

@oblogic7 oblogic7 Jan 13, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Technically, the component isn't affected by this failure since it is simply a failure to confiure the remote device, but I have made the change as requested.

'Doorbird configuration failed. Please verify that API '
'Operator permission is enabled for the Doorbird user. '
'A restart will be required once permissions have been '
'verified.',
title='Doorbird Configuration Failure',
notification_id='doorbird_schedule_error')

return False

hass.data[DOMAIN] = doorstations

Expand Down Expand Up @@ -230,6 +242,7 @@ def _register_event(self, hass_url, event, schedule):
if not self.webhook_is_registered(hass_url):
self.device.change_favorite('http', 'Home Assistant ({} events)'
.format(event), hass_url)

fav_id = self.get_webhook_id(hass_url)

if not fav_id:
Expand All @@ -253,8 +266,6 @@ def _register_event(self, hass_url, event, schedule):
for relay in self._relay_nums:
entry = self.device.get_schedule_entry(event, str(relay))
entry.output.append(output)
resp = self.device.change_schedule(entry)
return resp
else:
entry = self.device.get_schedule_entry(event)
entry.output.append(output)
Expand Down
2 changes: 1 addition & 1 deletion requirements_all.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ distro==1.3.0
dlipower==0.7.165

# homeassistant.components.doorbird
doorbirdpy==2.0.4
doorbirdpy==2.0.6

# homeassistant.components.sensor.dovado
dovado==0.4.1
Expand Down