core
core copied to clipboard
Bypass validation empty code for lock
Proposed change
PR #85842 implemented a validation for lock which provides a problem in two use-cases
- Can no longer use a default code configured by the user
- Locks where code is only required one-way (unlock but not lock) the user know need to provide a code anyway
Change so validation is only tested if code is provided, otherwise leave it to integration to do necessary checks (as mentioned above).
Type of change
- [ ] Dependency upgrade
- [x] Bugfix (non-breaking change which fixes an issue)
- [ ] New integration (thank you!)
- [ ] New feature (which adds functionality to an existing integration)
- [ ] Deprecation (breaking change to happen in the future)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [ ] Code quality improvements to existing code or addition of tests
Additional information
- This PR fixes or closes issue: fixes #87474 fixes #86963
- This PR is related to issue:
- Link to documentation pull request:
Checklist
- [x] The code change is tested and works locally.
- [x] Local tests pass. Your PR cannot be merged unless tests pass
- [x] There is no commented out code in this PR.
- [x] I have followed the development checklist
- [x] The code has been formatted using Black (
black --fast homeassistant tests
) - [ ] Tests have been added to verify that the new code works.
If user exposed functionality or configuration variables are added/changed:
- [ ] Documentation added/updated for www.home-assistant.io
If the code communicates with devices, web services, or third-party tools:
- [ ] The manifest file has all fields filled out correctly.
Updated and included derived files by running:python3 -m script.hassfest
. - [ ] New or updated dependencies have been added to
requirements_all.txt
.
Updated by runningpython3 -m script.gen_requirements_all
. - [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
- [ ] Untested files have been added to
.coveragerc
.
To help with the load of incoming pull requests:
- [x] I have reviewed two other open pull requests in this repository.
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (lock
) you are listed as a code owner for? Thanks!
Code owner commands
Code owners of lock
can trigger bot actions by commenting:
-
@home-assistant close
Closes the issue. -
@home-assistant rename Awesome new title
Change the title of the issue. -
@home-assistant reopen
Reopen the issue. -
@home-assistant unassign lock
Removes the current integration label and assignees on the issue, add the integration domain after the command.
Meanwhile we have discussions in the architecture repo should we merge this one as current implementation breaks functionality for several integrations? I would not like to lose the validation but reverting #85842 is also an option obviously
@gjohansson-ST set this to draft until is clear if this is the way to go.
Let us wait for the architecture discussion.
When is this planned? If it's a long time away, would you consider accepting this merge as a temporary fix until then? If you decide to revert the previous change you can always revert these commits too.
And if you do decide to go along this path then a fix is already implemented and the users hit by this in the linked issues won't be affected longer than necessary.
Let us wait for the architecture discussion.
When is this planned? If it's a long time away, would you consider accepting this merge as a temporary fix until then? If you decide to revert the previous change you can always revert these commits too.
And if you do decide to go along this path then a fix is already implemented and the users hit by this in the linked issues won't be affected longer than necessary.
I do not believe a fix is needed.
Why would i want to provide a code to lock the door which is now the case? This means i would have to provide the default code to the system. By that HA would have the "keys" to my house. I dont quite like that. Obviously opening the door is different and more critical. By this the automations that close the doors dont work. I love the HA and all the capabilities but now one of the very core functions is broken in my opinion with this.
Still cant lock my door from HA. This should not happen and for sure it should nok take weeks to revert or fix.
Still cant lock my door from HA. This should not happen and for sure it should nok take weeks to revert or fix.
Please do not use this PR to log issues, open an issue instead.
Still cant lock my door from HA. This should not happen and for sure it should nok take weeks to revert or fix.
Please do not use this PR to log issues, open an issue instead.
Already done that. Still No answer from anybody. https://github.com/home-assistant/core/issues/86963
I do not believe a fix is needed.
Well, the case right now is that users are presented with a GUI that lets them add a default code for their lock entities, and then when they try to lock/unlock these entities an exception is thrown. The same happens for integrations outside HA, for example voice assistants like Google Assistant. If you give Google Assistant access to the entity it will be fail to lock/unlock unless you explicitly set up an routine to call an automation (and this is not the default way Google Assistant handles the lock entity).
I think the code validation should be added, but as a deprecation, and if there are discussions concerning what architectural direction this should be taken (bypass code vs storing the default code in the entity itself) the breaking change should be reverted in the meantime as to not impact users while this is decided.
Anyway, I have found the discussion threads in the architecture repo so I'll follow along there. I agree this PR is not the place for this discussion.
I don't think the suggested solution takes into consideration that there is also an issue with codes consisting of > 4 digits. My error reads: "Call-service error. Code '123456' for locking lock.myYaleDoormanLock doesn't match pattern ^\d{4}$"
Verisure forces users to use 6 digit codes for Yale Doorman locks. A six digit code will not match the pattern above and as far as I understand this use case is not covered by the proposed change in this PR.
I don't think the suggested solution takes into consideration that there is also an issue with codes consisting of > 4 digits. My error reads: "Call-service error. Code '123456' for locking lock.myYaleDoormanLock doesn't match pattern ^\d{4}$"
Verisure forces users to use 6 digit codes for Yale Doorman locks. A six digit code will not match the pattern above and as far as I understand this use case is not covered by the proposed change in this PR.
Just purchased a very cheap Yale V2N and connected it through the Yale Smart Living integration. When I first setup the hub i made a 4-digit 1234 code for verification which worked, but then I updated it and was required to use a 6-digit code, which now puts me in the same scenario as you and I am not able to unlock through HA.
I don't think the suggested solution takes into consideration that there is also an issue with codes consisting of > 4 digits. My error reads: "Call-service error. Code '123456' for locking lock.myYaleDoormanLock doesn't match pattern ^\d{4}$" Verisure forces users to use 6 digit codes for Yale Doorman locks. A six digit code will not match the pattern above and as far as I understand this use case is not covered by the proposed change in this PR.
Just purchased a very cheap Yale V2N and connected it through the Yale Smart Living integration. When I first setup the hub i made a 4-digit 1234 code for verification which worked, but then I updated it and was required to use a 6-digit code, which now puts me in the same scenario as you and I am not able to unlock through HA.
If the code should be 6 digits and code_format
is set to ^\d{4}$
this means validation is done on 4 digits. The code format can be changed to accept both formats, e.g. ^\d{4,6}$
accepts all numeric codes of 4, 5 or 6 digits. That is not a reason to bypass,
Pleas keep these discussions out of the PR, and open an issue for the integration causing issues instead.
Closing this as it's clear we will not add it