Show persistent notification on Doorbird schedule failure#20033
Conversation
Add persistent notification on failure to configure doorbird schedule.
homeassistant
added
cla-signed
integration: doorbird
small-pr
ghost
added
the
|
@MartinHjelmare Here are the changes promised on #18730 |
| try: | ||
| doorstation.update_schedule(hass) | ||
| except HTTPError: | ||
| hass.components.persistent_notification.create( |
There was a problem hiding this comment.
Shouldn't we just log an error and return False as the other setup errors do?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
We can, but this gets the error in front of the user without having to dig
through logs. It is rather common among users who don’t read the docs
thoroughly.
…On Sun, Jan 13, 2019 at 7:29 AM Martin Hjelmare ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In homeassistant/components/doorbird.py
<#20033 (comment)>
:
> @@ -113,7 +114,16 @@ 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(
Shouldn't we just log an error and return False as the other setup errors
do?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#20033 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAbs0DTaE-rWT0XkxUamyPjLVmvNMBLiks5vCzShgaJpZM4Z82pk>
.
|
…tant#20033) * Remove unnecessary return. Add persistent notification on failure to configure doorbird schedule. * Update doorbirdpy to 2.0.5 * Fix bare except * Bump version again * Lint * Return false
Ported to current upstream dev format (unified GridSourceType). Adds `name: NotRequired[str]` to GridSourceType, SolarSourceType, BatterySourceType, GasSourceType, WaterSourceType and corresponding voluptuous schemas. This mirrors the same pattern already established for DeviceConsumption in PR home-assistant#20033.
5 participants
Description:
Cleanup unnecessary return inside of loop. Adds persistent notification for failed schedule setup.
Related issue (if applicable): fixes #19661
Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>
Checklist:
tox. Your PR cannot be merged unless tests passIf the code communicates with devices, web services, or third-party tools:
REQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py.