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.

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

Store notifications in component. Add ws endpoint for fetching. #16503

Merged
merged 2 commits into from
Sep 11, 2018

Conversation

jeradM
Copy link
Member

@jeradM jeradM commented Sep 8, 2018

Description:

Depends on home-assistant/frontend#1649

This is initial MVP for component storage of persistent notifications. Eventually they will be removed from the state machine, but this leaves them there for now so it is not a breaking change.

Related issue (if applicable): fixes #15071

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@awarecan
Copy link
Contributor

awarecan commented Sep 9, 2018

Persistent Notification shall aware user context, it may no be in this pull request although.

@jeradM
Copy link
Member Author

jeradM commented Sep 9, 2018

I'm not sure what you mean by aware user context. They are created through service calls now so that context should be captured. Do you mean notifications will be created for a specific user and only shown to that user?

@awarecan
Copy link
Contributor

awarecan commented Sep 9, 2018

I think we should have two (or three) type of notifications

  • broadcast: all users can see it, all users can dismiss it, it will be deleted after one user dismissed it. This is current behavior

  • user specific notification: only the user specified in the service call can see and dismiss the notification.

  • maybe too complicated broadcast and ensure everyone read: all users can see the notification, all user can dismiss it, but one user dismiss it, it will only become invisible for this user, other user still can see it, until all users dismissed it.

@jeradM
Copy link
Member Author

jeradM commented Sep 9, 2018

Ah yes 👍 these are all good ideas. I think once Lovelace is default and we move notifications out of the state machine we can add more of this type of functionality.

@jeradM jeradM changed the title WIP: Store notifications in component. Add ws endpoint for fetching. Store notifications in component. Add ws endpoint for fetching. Sep 10, 2018
@@ -57,6 +75,12 @@ def dismiss(hass, notification_id):
hass.add_job(async_dismiss, hass, notification_id)


@bind_hass
def mark_read(hass, notification_id):
Copy link
Member

Choose a reason for hiding this comment

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

Let's not add this. I want to get rid of these functions.

"""Remove a notification."""
data = {ATTR_NOTIFICATION_ID: notification_id}

hass.async_add_job(hass.services.async_call(DOMAIN, SERVICE_DISMISS, data))


@callback
@bind_hass
def async_mark_read(hass: HomeAssistant, notification_id: str) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Let's not add this.

vol.Required('type'): WS_TYPE_GET_NOTIFICATIONS,
})

PERSISTENT_NOTIFICATIONS = OrderedDict()
Copy link
Member

Choose a reason for hiding this comment

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

All globals should be stored in hass.data.

@callback
def dismiss_service(call):
"""Handle the dismiss notification service call."""
notification_id = call.data.get(ATTR_NOTIFICATION_ID)
entity_id = ENTITY_ID_FORMAT.format(slugify(notification_id))

if entity_id not in PERSISTENT_NOTIFICATIONS:
_LOGGER.error('Dismissing persistent_notification failed: '
Copy link
Member

Choose a reason for hiding this comment

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

I think that it's fine to silently return. The goal is to dismiss a certain notification_id, which has been achieved.

def websocket_get_notifications(
hass: HomeAssistant, connection: websocket_api.ActiveConnection, msg):
"""Return a list of persistent_notifications."""
connection.send_message_outside(
Copy link
Member

Choose a reason for hiding this comment

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

Don't use send_message_outside. You are inside the callback that has to generate the response. Use connection.to_write.put_nowait

@balloob balloob merged commit 50fb594 into home-assistant:dev Sep 11, 2018
@ghost ghost removed the in progress label Sep 11, 2018
@balloob balloob mentioned this pull request Sep 28, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move persistent notifications out of state machine
4 participants