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

Bug: fix google_assistant 'allow_unlock' config option #18874

Merged
merged 1 commit into from
Dec 2, 2018

Conversation

ahayworth
Copy link
Contributor

@ahayworth 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
Copy link
Contributor

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
Copy link
Contributor Author

^ 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 Bug: fix google_assistant 'allow_unlock' config option [WIP] Bug: fix google_assistant 'allow_unlock' config option Dec 1, 2018
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-google-locks branch from 61f7a61 to d06a781 Compare December 1, 2018 21:45
@ahayworth ahayworth requested a review from a team as a code owner December 1, 2018 21:45
@ahayworth
Copy link
Contributor Author

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

@omriasta
Copy link
Contributor

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
Copy link
Contributor Author

@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 [WIP] Bug: fix google_assistant 'allow_unlock' config option Bug: fix google_assistant 'allow_unlock' config option Dec 1, 2018
@omriasta
Copy link
Contributor

omriasta commented Dec 1, 2018

Thanks, just tested and have success!

@omriasta
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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 added this to the 0.83.3 milestone Dec 2, 2018
@balloob balloob merged commit b7e2522 into home-assistant:dev Dec 2, 2018
@ghost ghost removed the in progress label Dec 2, 2018
balloob pushed a commit that referenced this pull request Dec 3, 2018
…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 mentioned this pull request Dec 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Google Assistant Allow Unlock 0.83, 0.83.1
4 participants