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

Add LogBook support to HomeKit #17180

Merged
merged 14 commits into from Oct 16, 2018

Conversation

Projects
None yet
4 participants
@ehendrix23
Contributor

ehendrix23 commented Oct 6, 2018

Description:

Initial set to add HomeKit entries to LogBook. Setting the framework for it.
Currently following logbook entries will be created:
-) When an accessory is added for HomeKit.
-) When a lock is opened or closed.

Checklist:

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

If the code does not interact with devices:

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

This comment has been minimized.

Contributor

ehendrix23 commented Oct 6, 2018

@cdce8p
Here is the first try. Have a look on what I did. :-)

It is working on mine. When I start HASS I get entries in LogBook about the accessories that have been added to HomeKit. It provides the accessory type and the friendly name.
For example:

HomeKit Add accessory Lock for Front Door

Then when I unlock (or lock) the door it puts in logbook an entry like:

HomeKit Send command unlock for Front Door

Front Door here is my display name.

@cdce8p cdce8p self-assigned this Oct 6, 2018

@cdce8p

Left a few comments. Mostly about unnecessary empty lines 🙂
Personally I think we should move the logbook changes to a new PR, but we could leave them in there until the rest is done to make live testing easier.

Show resolved Hide resolved homeassistant/components/homekit/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/accessories.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/accessories.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/accessories.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/accessories.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/accessories.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/type_locks.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/type_locks.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/type_locks.py Outdated
Show resolved Hide resolved homeassistant/components/logbook.py
@ehendrix23

This comment has been minimized.

Contributor

ehendrix23 commented Oct 6, 2018

@cdce8p Completed the changes, have a look. One item you marked for some reason did not get resolved/outdated. I checked logbook.py with flake8, pylint, and pydocstyle and no issues reported with the updated one.

@cdce8p

Already looking much better now!
Will do an in deep review tomorrow but wanted to leave you some quick comments.
Can you try moving the logger debug statment to call_service as well?

Show resolved Hide resolved homeassistant/components/homekit/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/accessories.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/accessories.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/accessories.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/accessories.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/accessories.py Outdated
Show resolved Hide resolved homeassistant/components/logbook.py Outdated
if message:
action = "send command {}".format(message)
elif entity_id:

This comment has been minimized.

@cdce8p

cdce8p Oct 8, 2018

Member

Instead of if / elif / else blocks. It could be useful to construct a block beforehand and just add it:
msg_value = " to {}".format(value) if value else ''

message = "send command {}{} for {}".format(service, msg_value, display_name)

This comment has been minimized.

@ehendrix23

ehendrix23 Oct 8, 2018

Contributor

Updated

@cdce8p cdce8p added the Hacktoberfest label Oct 8, 2018

Show resolved Hide resolved homeassistant/components/homekit/accessories.py Outdated
Show resolved Hide resolved homeassistant/components/homekit/accessories.py Outdated
elif event.event_type == EVENT_HOMEKIT_CHANGED:
data = event.data
entity_id = data.get(ATTR_ENTITY_ID)
dsp_name = data.get(ATTR_DISPLAY_NAME) or entity_id

This comment has been minimized.

@cdce8p

cdce8p Oct 9, 2018

Member

Just remove or entitiy_id. It isn't needed.

This comment has been minimized.

@cdce8p

cdce8p Oct 9, 2018

Member

I would rename it display_name

This comment has been minimized.

@cdce8p

cdce8p Oct 9, 2018

Member

Actually, instead of data.get just use data[ATTR_ENTITY_ID] and so on, since they are all passed in the event_data every time.

This comment has been minimized.

@ehendrix23

ehendrix23 Oct 9, 2018

Contributor

My reasoning for this was if the event is sent from something else or so and not from call_service itself, hence did it this way from a safety perspective.
If you do not believe this is required and we assume that data is always there then I can process it accordingly. :-)

This comment has been minimized.

@cdce8p

cdce8p Oct 9, 2018

Member

Since this is a HomeKit only event, I think we'll be fine. We can even streamline it further and integrate those calls directly into the message construction (like Alexa is doing). No need to define a new variable for just one interaction. An exception for me would be the entity_id (if we want to be a crazy, it could even remain with get ;)

This comment has been minimized.

@ehendrix23

ehendrix23 Oct 9, 2018

Contributor

All done. Did not go crazy, too early in the morning here right now. :-)

Show resolved Hide resolved homeassistant/components/logbook.py Outdated
Show resolved Hide resolved homeassistant/components/logbook.py Outdated
Show resolved Hide resolved homeassistant/components/logbook.py Outdated
Show resolved Hide resolved homeassistant/components/logbook.py Outdated
@ehendrix23

This comment has been minimized.

Contributor

ehendrix23 commented Oct 9, 2018

@cdce8p I've already addressed all items in my local copy except the one where I check for the values. Awaiting your final thoughts on that one and will then update accordingly. :-)

'when': event.time_fired,
'name': 'HomeKit',
'message': message,
'domain': 'homekit',

This comment has been minimized.

@cdce8p

cdce8p Oct 9, 2018

Member

Can you also import 'homekit' from homekit.const with from .. import DOMAIN as DOMAIN_HOMEKIT?

This comment has been minimized.

@ehendrix23

ehendrix23 Oct 9, 2018

Contributor

Done, should it be DOMAIN_HOMEKIT or HOMEKIT_DOMAIN? Asking as in logbook there is an import DOMAIN AS HA_DOMAIN from homeassistant.const.

This comment has been minimized.

@cdce8p

cdce8p Oct 9, 2018

Member

Doesn't really matter, as long as it is unique.

@ehendrix23

This comment has been minimized.

Contributor

ehendrix23 commented Oct 9, 2018

@cdce8p Just did the commit & push. :-)

@cdce8p

Just two quick comments.
After these I think we only need to update the tests, expand to all other types and we are done.

Show resolved Hide resolved homeassistant/components/homekit/accessories.py
entity_id = data[ATTR_ENTITY_ID]
value_msg = " to {}".format(
data.get[ATTR_VALUE]) if data.get[ATTR_VALUE] else ''

This comment has been minimized.

@cdce8p

cdce8p Oct 9, 2018

Member

I think that I might have mislead you here. Sorry for that.
For value it is probably better to define it beforehand:

value = data.get('ATTR_VALUE`)

value_msg = " to {}".format(value) if value else ''

For ATTR_SERVICE and ATTR_DISPLAY_NAME below the formatting is correct.

Another point probably also unnecessary, since we'll have use get for value can you use it for entity_id as well? Just looks a bit better :)

The empty line between value_msg and message would be unnecessary.

This comment has been minimized.

@ehendrix23

ehendrix23 Oct 9, 2018

Contributor

Done :-)

@ehendrix23

This comment has been minimized.

Contributor

ehendrix23 commented Oct 9, 2018

Both those are done.

If we're good then I'll go ahead and update all other ones to call call_service for state changes.
For updating the tests I would really need help and guidance :-) (Like I have not received any up till now ).

@cdce8p

This comment has been minimized.

Member

cdce8p commented Oct 9, 2018

Good work so far!

When you change the other types, make sure to rebase your branch. I'm not 100% sure there haven't been any updates:
https://developers.home-assistant.io/docs/en/development_catching_up.html
That way you're on the save side and we don't miss anything.

As for tests: I'll take a look at them later and get back to you.

@ehendrix23 ehendrix23 requested review from amelchio, Danielhiversen, fabaff, rytilahti, syssi and home-assistant/core as code owners Oct 9, 2018

@cdce8p cdce8p removed request for home-assistant/core and fabaff Oct 9, 2018

@cdce8p cdce8p removed request for syssi, rytilahti and amelchio Oct 9, 2018

ehendrix23 added some commits Oct 6, 2018

HomeKit LogBook
Initial set to add HomeKit entries to LogBook. Setting the framework for it.
Currently following logbook entries will be created:
-) When an accessory is added for HomeKit.
-) When a lock is opened or closed.
Updates based on comments
Updates performed based on review and comments.
Further updates based on review
Moved firing of event into call_service method, removing sent_logbook_message.
Based that logbook messages will only be done for requests from HomeKit, simplified message creation accordingly.
Added display_name to data for event.
Changed to remove message and use service&attribute
Changed for the log message now to be created using the service & attribute instead of providing a specific log message.
Added constants to homekit/const.
Updated parameter name acc_domain to domain in call_service.
Cleaunup in logbook
Missed renaming of parameter in accessories => fixed.
Some cleanup updates in logbook streamlining it based that this is a HomeKit only event and thus expectation is that required values will be passed.
Last cleanup :-)
Added variable value for data.get(ATTR_VALUE)
Added empty line before bus.fire and removed after bus.fire
Added logging to logbook for all other types
Added the calling of self.call_service instead of self.hass.services.call for all other types.
@ehendrix23

This comment has been minimized.

Contributor

ehendrix23 commented Oct 9, 2018

@cdce8p Well, that rebase did not work out as it should. Made a mistake and resulted in a whole bunch of other commit being part of my branch and part of my commits.
I think I got it resolved now. :-)

Updated all the other types for HomeKit and are part of this last commit.

cdce8p added some commits Oct 11, 2018

Added tests
* Logbook test
* HomeAccessory.call_service test
* Updated homekit/test_type_cover.py
@cdce8p

This comment has been minimized.

Member

cdce8p commented Oct 11, 2018

I went ahead and pushed some changes to your branch. Take a look at the last commits.
Mainly I converted call_service to async and started with the tests.

Take a look at my changes to test_type_covers.py to see what to do for the other type tests. Should be pretty strait forward. Except for the remaining type ones, all tests should be finished.

Before you start working on it, make sure to fetch your remote branch first and then rebase your local branch onto it, so you have my commits as well.

Enhanced lock test to confirm logbook event.
Enhanced lock test for logbook event from locks.
@@ -1,6 +1,8 @@
"""Test different accessory types: Locks."""
import pytest
from homeassistant.components.homekit.const import (
ATTR_VALUE, ATTR_DISPLAY_NAME)

This comment has been minimized.

@cdce8p

cdce8p Oct 13, 2018

Member

Since you only need ATTR_VALUE, one line is completely enough here:
from homeassistant.components.homekit.const import ATTR_VALUE

ehendrix23 added some commits Oct 15, 2018

Added all other tests
Add tests for all other types.
Fixed issue for type_thermostat where entry in logbook would always state ºC instead of the actual measurement (ºC or ºF).
Fixed import of ATTR_VALUE
Changed:
from homeassistant.components.homekit.const import (
    ATTR_VALUE)
to
from homeassistant.components.homekit.const import ATTR_VALUE
@@ -1,6 +1,7 @@
"""Test different accessory types: Locks."""
import pytest
from homeassistant.components.homekit.const import ATTR_VALUE)

This comment has been minimized.

@houndci-bot

houndci-bot Oct 15, 2018

SyntaxError: invalid syntax

Fixed import issue in test_type_locks
Fixed the import issue in test_type_locks.

@cdce8p cdce8p changed the title from WIP: Add LogBook support to HomeKit to Add LogBook support to HomeKit Oct 16, 2018

@cdce8p

cdce8p approved these changes Oct 16, 2018

@cdce8p cdce8p added the new-feature label Oct 16, 2018

@cdce8p

This comment has been minimized.

Member

cdce8p commented Oct 16, 2018

Looks good to me. Great work you did here 🥇

@cdce8p cdce8p merged commit fee87cd into home-assistant:dev Oct 16, 2018

4 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Oct 16, 2018

@ehendrix23 ehendrix23 deleted the ehendrix23:HomeKit-LogBook-Entries branch Oct 16, 2018

@balloob balloob referenced this pull request Oct 26, 2018

Merged

0.81 #17809

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