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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Lock capability to SmartThings platform #20977

Merged
merged 23 commits into from Feb 14, 2019

Conversation

@bendews
Copy link
Contributor

bendews commented Feb 11, 2019

Description:

This PR adds functionality for Locks expressed through the SmartThings platform.

Locking/Unlocking is working great, however I'm not sure whether we should update the state straight away (faster UI feedback) or wait for the confirmation from SmartThings (removes false positives).

Also as mentioned in the referenced issue, there are some other attributes that could possibly be added to the entity. @andrewsayre any chance you could expand on those here? 馃槃

Related issue (if applicable): fixes #20889

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

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@OttoWinter
Copy link
Member

OttoWinter left a comment

馃憤

Show resolved Hide resolved homeassistant/components/smartthings/lock.py Outdated

@andrewsayre andrewsayre self-assigned this Feb 11, 2019

@andrewsayre

This comment has been minimized.

Copy link
Contributor

andrewsayre commented Feb 11, 2019

This looks really good -- but there's a few things to address. I'll add my review this evening and we get this get this to done!! Please don't merge until then.

Edit: Review comments are in!

@andrewsayre
Copy link
Contributor

andrewsayre left a comment

Really nice job!! Just some simple clean-up/suggestions!

Show resolved Hide resolved homeassistant/components/smartthings/lock.py Outdated
Show resolved Hide resolved tests/components/smartthings/test_lock.py Outdated
Show resolved Hide resolved homeassistant/components/smartthings/lock.py
Show resolved Hide resolved homeassistant/components/smartthings/lock.py
@bendews

This comment has been minimized.

Copy link
Contributor Author

bendews commented Feb 12, 2019

@andrewsayre - Thanks for the review and the awesome work with library/integration, it's so easy to work with! Will make those changes you have mentioned, great idea grabbing the attributes such as the CodeId, that's going to be useful for notifications etc.

@andrewsayre andrewsayre referenced this pull request Feb 12, 2019

Merged

Add SmartThings Lock platform docs #8493

2 of 2 tasks complete
@andrewsayre

This comment has been minimized.

Copy link
Contributor

andrewsayre commented Feb 12, 2019

@bendews bendews force-pushed the bendews:feature/smartthings-lock branch from cfe2d93 to e677277 Feb 12, 2019

Show resolved Hide resolved homeassistant/components/smartthings/lock.py Outdated
Show resolved Hide resolved homeassistant/components/smartthings/lock.py Outdated
@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Nice!

Show resolved Hide resolved homeassistant/components/smartthings/lock.py Outdated
status = self._device.status.attributes[Attribute.lock]
status_data = status.data or {}
return {
'method': status_data.get('method'),

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Feb 13, 2019

Member

We usually don't include the item at all in the attributes if the value of the item is None.

This comment has been minimized.

@bendews

bendews Feb 13, 2019

Author Contributor

I've implemented a mapping structure to map the ST attributes to HA-formatted ones, and removed null values.

@bendews

This comment has been minimized.

Copy link
Contributor Author

bendews commented Feb 13, 2019

The tests are going to start failing until #21025 is merged.

@andrewsayre

This comment has been minimized.

Copy link
Contributor

andrewsayre commented Feb 13, 2019

The tests are going to start failing until #21025 is merged.

It's merged and I restarted the build.

@andrewsayre

This comment has been minimized.

Copy link
Contributor

andrewsayre commented Feb 13, 2019

馃憤 Approved. Can be merged once build passes.

@andrewsayre

This comment has been minimized.

Copy link
Contributor

andrewsayre commented Feb 14, 2019

I think we're good here!

@andrewsayre andrewsayre merged commit c20e0b9 into home-assistant:dev Feb 14, 2019

4 of 5 checks passed

coverage/coveralls Coverage decreased (-0.5%) to 92.795%
Details
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 Feb 14, 2019

@xfgavin

This comment has been minimized.

Copy link

xfgavin commented Feb 14, 2019

Cool! Time to stop mqtt and smartthings bridge to save power.

@bendews bendews deleted the bendews:feature/smartthings-lock branch Feb 14, 2019

@bendews

This comment has been minimized.

Copy link
Contributor Author

bendews commented Feb 14, 2019

Thanks @andrewsayre!! 馃槃

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

Merge branch 'dev' into current
* dev: (213 commits)
  Bump pyHik library to 0.2.2, improve connections, add sensors (home-assistant#21086)
  Check against unlinked user (home-assistant#21081)
  Rename CONF_ATTRIBUTION to ATTRIBUTION (home-assistant#21069)
  Fix pushover schema
  Climate const.py move (home-assistant#20945)
  Update file header
  Update file header (home-assistant#21061)
  M茅t茅o-France platform for the weather component (home-assistant#18404)
  Bumped version to 0.89.0.dev0
  Don't directly update config entries (home-assistant#20877)
  Update file header (home-assistant#21054)
  Upgrade ruamel.yaml to 0.15.88 (home-assistant#21055)
  fix webhook update (home-assistant#21048)
  Add integration method to sensor.integration (home-assistant#21050)
  Person: Ignore unavailable states (home-assistant#21058)
  Person checks (home-assistant#21056)
  Create a person during onboarding (home-assistant#21057)
  Add template support to Bayesian sensor (home-assistant#20757)
  Add Lock capability to SmartThings platform (home-assistant#20977)
  Update translations
  ...

# Conflicts:
#	homeassistant/components/homematicip_cloud/climate.py

@balloob balloob referenced this pull request Feb 20, 2019

Merged

0.88.0 #21238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.