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

Bug: fix google_assistant 'allow_unlock' config option #18874

Merged
merged 1 commit into from Dec 2, 2018

Conversation

Projects
None yet
4 participants
@ahayworth
Contributor

ahayworth commented Dec 1, 2018

Description

I failed to thread the allow_unlock preference down from where we actually read the google_assistant preferences, through to the Config object that holds preferences in the http view. This was done correctly for the cloud component, but I missed it here. This PR also removes the default value for this parameter on the Config object, which is how it was missed before. As a required param, future refactoring will not miss it.

Related issue (if applicable): fixes #18848

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.
@omriasta

This comment has been minimized.

omriasta commented Dec 1, 2018

Figured I would comment here as well. This change doesn't appear to fix the issue for me. Behavior is identical to #18848 for me

@ahayworth

This comment has been minimized.

Contributor

ahayworth commented Dec 1, 2018

^ Thanks for the report. I've done more testing, and I agree it's actually not fixed. The cloud component is working, but the google_assistant component is not. I'll look a little bit more later.

@ahayworth ahayworth changed the title from Bug: fix google_assistant 'allow_unlock' config option to [WIP] Bug: fix google_assistant 'allow_unlock' config option Dec 1, 2018

bugfix: ensure the `google_assistant` component respects `allow_unlock`
The `Config` object specific to the `google_assistant` component
had a default value for `allow_unlock`. We were not overriding this
default when constructing the Config object during `google_assistant`
component setup, whereas we do when setting up the `cloud` component.

To fix, we thread the `allow_unlock` parameter down through http setup,
and ensure that it's set correctly. Moreover, we also change the
ordering of the `Config` parameters, and remove the default. Future
refactoring should not miss it, as it is now a required parameter.

@ahayworth ahayworth force-pushed the ahayworth:ahayworth-google-locks branch from 61f7a61 to d06a781 Dec 1, 2018

@ahayworth ahayworth requested a review from home-assistant/core as a code owner Dec 1, 2018

@ahayworth

This comment has been minimized.

Contributor

ahayworth commented Dec 1, 2018

For any reviewers out there, I believe this is ready for review now.

@omriasta

This comment has been minimized.

omriasta commented Dec 1, 2018

Happy to test but not sure how to? Is there a zip file I could place in custom_components?

@ahayworth

This comment has been minimized.

Contributor

ahayworth commented Dec 1, 2018

@omriasta - Thank you, I appreciate that. I do believe it's fixed now - I was able to reproduce the bug and work on it directly - but your confirmation would be nice. Here's a zip file:
google.zip

You should be able to unzip that into custom_components. Make sure to move the zipfile out of custom_components before you restart. 😄

@ahayworth ahayworth changed the title from [WIP] Bug: fix google_assistant 'allow_unlock' config option to Bug: fix google_assistant 'allow_unlock' config option Dec 1, 2018

@omriasta

This comment has been minimized.

omriasta commented Dec 1, 2018

Thanks, just tested and have success!

@omriasta

This comment has been minimized.

omriasta commented Dec 1, 2018

Spoke too soon....for some reason, now when I unlock the door using hass or Google assistant, the status is not updated in hass...the UI shows that the door is still locked and will not let me lock it again.... Google assistant will relock if i try through there...very odd

@ahayworth

This comment has been minimized.

Contributor

ahayworth commented Dec 1, 2018

Spoke too soon....for some reason, now when I unlock the door using hass or Google assistant, the status is not updated in hass...the UI shows that the door is still locked and will not let me lock it again.... Google assistant will relock if i try through there...very odd

Since you have a Schlage BE469, that's actually a different bug - #18737 has a fix for that. If Google successfully unlocked your door when you asked, then this patch does what it was supposed to do. 😄

@omriasta

This comment has been minimized.

omriasta commented Dec 1, 2018

Just put the zwave.py in custom_components/lock and now it's working correctly and updating. Thanks for all your help!!!

@balloob

balloob approved these changes Dec 2, 2018

@balloob balloob added this to the 0.83.3 milestone Dec 2, 2018

@balloob balloob merged commit b7e2522 into home-assistant:dev Dec 2, 2018

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 First build on ahayworth-google-locks at 92.921%
Details

@wafflebot wafflebot bot removed the in progress label Dec 2, 2018

mxworm added a commit to mxworm/home-assistant that referenced this pull request Dec 2, 2018

Merge branch 'dev' of https://github.com/home-assistant/home-assistant
…into dev

* 'dev' of https://github.com/home-assistant/home-assistant: (35 commits)
  Add lightwave components for switches and lights (home-assistant#18026)
  Remove commented out code (home-assistant#18925)
  No more opt-out auth (home-assistant#18854)
  Reconfigure MQTT switch component if discovery info is changed (home-assistant#18179)
  Reconfigure MQTT light component if discovery info is changed (home-assistant#18176)
  Set sensor to unavailable if battery is dead. (home-assistant#18802)
  Fix hdmi_cec entity race (home-assistant#18753)
  Show ANSI color codes in logs in Hass.io (home-assistant#18834)
  bugfix: ensure the `google_assistant` component respects `allow_unlock` (home-assistant#18874)
  Fibaro ubs (home-assistant#18889)
  Restore states when removing/adding entities (home-assistant#18890)
  Optionally do not log template rendering errors (home-assistant#18724)
  Small refactoring of MQTT climate (home-assistant#18814)
  Small refactoring of MQTT alarm (home-assistant#18813)
  Small refactoring of MQTT cover (home-assistant#18850)
  Upgrade keyring to 17.0.0 (home-assistant#18901)
  Upgrade pillow to 5.3.0
  Upgrade ruamel.yaml to 0.15.80
  Upgrade restrictedpython to 4.0b7
  Fix change
  ...

balloob added a commit that referenced this pull request Dec 3, 2018

bugfix: ensure the `google_assistant` component respects `allow_unloc…
…k` (#18874)

The `Config` object specific to the `google_assistant` component
had a default value for `allow_unlock`. We were not overriding this
default when constructing the Config object during `google_assistant`
component setup, whereas we do when setting up the `cloud` component.

To fix, we thread the `allow_unlock` parameter down through http setup,
and ensure that it's set correctly. Moreover, we also change the
ordering of the `Config` parameters, and remove the default. Future
refactoring should not miss it, as it is now a required parameter.

@balloob balloob referenced this pull request Dec 3, 2018

Merged

0.83.3 #18958

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