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

Proactive Alexa ChangeReport messages #18114

Merged
merged 30 commits into from Jan 3, 2019

Conversation

Projects
None yet
5 participants
@abmantis
Copy link
Contributor

abmantis commented Nov 2, 2018

Description:

Implement Alexa's proactive ChangeReport API messages. This allows the alexa component to notify the skill that an entity has changed.
Requires that the user provides the skill's client_id and client_secret.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#7452

Example entry for configuration.yaml (if applicable):

alexa:
  smart_home:
    endpoint: https://api.amazonalexa.com/v3/events
    client_id: !secret alexa_client_id
    client_secret: !secret alexa_client_secret

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:

@abmantis abmantis force-pushed the abmantis:alexa_proactive_reports branch from 60c8645 to f241890 Nov 3, 2018

# TODO: we should check if this entity was already discovered
# by Alexa (store them in the prefs?) and, if not, send a
# proactive discovery message instead.
hass.async_add_job(async_send_changereport_message, hass,

This comment has been minimized.

@balloob

balloob Nov 3, 2018

Member

I am confused, we're not passing in the interface, just the entity. So for each interface that is reported, we report on the entity multiple times?

We should also not use async_add_job but, if we really need multiple tasks, create them but await asyncio.wait(…) them, so that stacktraces can lead to the right piece in the code.

This comment has been minimized.

@abmantis

abmantis Nov 6, 2018

Contributor

There is a return on the next line so that it will only report once for each entity.

This comment has been minimized.

@balloob

balloob Nov 12, 2018

Member

In that case we should just make this whole method async and await the changereport sending method instead of adding it as a job.

This comment has been minimized.

@abmantis

abmantis Nov 12, 2018

Contributor

If I remember correctly, that was my first approach, but it wouldn't work since the async_entity_state_listener has to be a callback and could not be async. I've done that a while ago, so I may be wrong.

This comment has been minimized.

@balloob

balloob Nov 12, 2018

Member

async_track_state_change can handle async, callback and normal methods.

This comment has been minimized.

@balloob

balloob Nov 20, 2018

Member

Not sure why this is resolved. We should await async_send_changereport_message(…) here and mark async_entity_state_listener as async.

@abmantis abmantis requested a review from home-assistant/core as a code owner Nov 7, 2018

abmantis and others added some commits Nov 2, 2018

use iterable directly
Co-Authored-By: abmantis <abmantis@users.noreply.github.com>
@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 12, 2018

Coveralls complains, means we need more tests:

  • Receiving AcceptGrant message is not tested
  • Hardly anything from alexa/auth.py is tested

@fabaff fabaff changed the title Alexa: proactive ChangeReport messages Proactive Alexa ChangeReport messages Dec 12, 2018

@abmantis abmantis changed the title Proactive Alexa ChangeReport messages [WIP] Proactive Alexa ChangeReport messages Dec 12, 2018

@abmantis

This comment has been minimized.

Copy link
Contributor

abmantis commented Dec 12, 2018

Coveralls complains, means we need more tests:

  • Receiving AcceptGrant message is not tested
  • Hardly anything from alexa/auth.py is tested

Ok. Added one for AcceptGrant (let me know if I missed anything). I'll try to add more for auth.

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 13, 2018

The coveralls check is a pretty good guidance here. You don't need full 100%, but critical code paths should be tested, or else they will end up throwing exceptions and users complain. For example, what happens when our get access token function throws an exception ?

abmantis added some commits Dec 16, 2018

@abmantis

This comment has been minimized.

Copy link
Contributor

abmantis commented Dec 16, 2018

I've added some more tests now. Do we still need more?

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 17, 2018

Nope! I'll do one last pass tomorrow

@abmantis abmantis changed the title [WIP] Proactive Alexa ChangeReport messages Proactive Alexa ChangeReport messages Dec 18, 2018

@abmantis

This comment has been minimized.

Copy link
Contributor

abmantis commented Dec 22, 2018

@balloob have you made the changes to the cloud component for this?

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 22, 2018

Damn it. I forgot to review this because of all the Logitech stuff. I actually did cloud component changes partially, but then realized I shouldn't return the refresh token but use that to fetch the access token 🤷‍♂️ I also forgot to do that last pass I promised to do 5 days ago 🤦‍♂️

Right now I'm busy with Christmas, but I will make sure that this will get merged before next beta, which is Jan 3. I'll also work on the Alexa tokens

@abmantis

This comment has been minimized.

Copy link
Contributor

abmantis commented Dec 22, 2018

@balloob no problem! Not in a hurry ;) Have a nice Christmas!

@abmantis

This comment has been minimized.

Copy link
Contributor

abmantis commented Jan 3, 2019

@balloob Will this still be in the next beta?

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 3, 2019

Yes! Just got back from by break, slowly waking up too, this is literally the first PR I will look at.

should_expose=config[CONF_FILTER],
entity_config=config.get(CONF_ENTITY_CONFIG),
)
hass.http.register_view(SmartHomeView(smart_home_config))

if AUTH_KEY in hass.data:
hass.loop.create_task(

This comment has been minimized.

@balloob

balloob Jan 3, 2019

Member

Why do this in create task?

This comment has been minimized.

@abmantis

abmantis Jan 3, 2019

Contributor

Because this method (async_setup) is a callback and async_enable_proactive_mode is a async method. I can't do await async_enable_proactive_mode() here.

This comment has been minimized.

@balloob

balloob Jan 4, 2019

Member

I guess we could make it async


headers = {
"Authorization": "Bearer {}".format(token),
"Content-Type": "application/json;charset=UTF-8"

This comment has been minimized.

@balloob

balloob Jan 3, 2019

Member

this will be set automatically by aiohttp if you pass an object as json= instead of setting data=

This comment has been minimized.

@abmantis

abmantis Jan 3, 2019

Contributor

should I do that on a new PR?

This comment has been minimized.

@balloob
@balloob

balloob approved these changes Jan 3, 2019

@balloob balloob merged commit ead38f6 into home-assistant:dev Jan 3, 2019

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.06%) to 92.863%
Details

@wafflebot wafflebot bot removed the in progress label Jan 3, 2019

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Jan 3, 2019

🎉 🎉 🎉 🎉 🎉 🎉

@abmantis abmantis referenced this pull request Jan 5, 2019

Merged

Small refactoring for the alexa component #19782

3 of 8 tasks complete

mxworm added a commit to mxworm/home-assistant that referenced this pull request Jan 5, 2019

Merge branch 'homematicip_cloud-update_rest_api_version' into current
* homematicip_cloud-update_rest_api_version:
  Update of requirements files
  Update version to 0.10.1
  Update HAP-python to 2.4.2 (home-assistant#19776)
  update powerview scene component to latest api. (home-assistant#19717)
  Fix some ESPHome race conditions (home-assistant#19772)
  Use manufacturer id only for configure_reporting only when specified. (home-assistant#19729)
  Don't slugify unique id (home-assistant#19770)
  Support for Homekit controller/alarm control panel (home-assistant#19612)
  check config instead of config_entry for quirks flag (home-assistant#19730)
  Round illumination and lux value to one (home-assistant#19747)
  Upgrade tibber library (home-assistant#19768)
  Add mychevy optional country parameter (home-assistant#19727)
  Move envisalink component to package and add services.yaml (home-assistant#19731)
  Proactive Alexa ChangeReport messages (home-assistant#18114)
  Filter urllib3.connectionpool warnings in camera.axis and camera.zoneminder (home-assistant#19641)
  Add exception handling to ADS shutdown (home-assistant#19682)
  Drop bme680 os_lookup for temp_offset (home-assistant#19733)
  Fix WeMo incorrect mapping of device type during discovery (home-assistant#19691)
  Do not choke on no awair data (home-assistant#19708)

@balloob balloob removed the new-platform label Jan 10, 2019

@balloob balloob referenced this pull request Jan 10, 2019

Merged

0.85.0 #19897

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment