-
-
Notifications
You must be signed in to change notification settings - Fork 35.8k
Add services for managing Schlage door codes #151014
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
base: dev
Are you sure you want to change the base?
Conversation
|
Hey there @dknowles2, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
570adb6 to
bdbc056
Compare
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
bdbc056 to
460eb9f
Compare
460eb9f to
bb141ca
Compare
554a45b to
336de4d
Compare
336de4d to
c9838d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds three new services to the Schlage integration for managing door lock access codes: add_code, delete_code, and get_codes. These services enable users to programmatically manage PIN codes on their Schlage locks through Home Assistant.
Key changes:
- New service implementations with validation for code names and values
- Service definitions and localization strings
- Comprehensive test coverage for all service scenarios
- Integration with existing coordinator for data refresh
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
homeassistant/components/schlage/lock.py |
Implements the three new services with validation logic and entity service registration |
homeassistant/components/schlage/services.yaml |
Defines service schemas and field configurations for the UI |
homeassistant/components/schlage/strings.json |
Adds service descriptions and error message translations |
homeassistant/components/schlage/icons.json |
Provides MDI icons for the new services |
homeassistant/components/schlage/const.py |
Defines service name constants |
homeassistant/components/schlage/coordinator.py |
Updates API call to include access codes in lock data |
homeassistant/components/schlage/__init__.py |
Minor formatting change |
tests/components/schlage/test_lock.py |
Comprehensive test coverage for all service scenarios including edge cases |
|
@joostlek and @dknowles2 I've implemented the requested changes. NOTE pytest is complaining that there is no translation defined for two things when the translation is defined. I don't know why it's doing that. My local test system uses the translations with no problems so I don't know why it's failing. @dknowles2 I had to re-arrange some of the strings.json file as it was not passing hassfest locally. |
| return {} | ||
|
|
||
| platform = entity_platform.async_get_current_platform() | ||
| platform.async_register_entity_service( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now have a new helper to register entity services from async_setup, which addresses the concerns joostlek pointed out: homeassistant.helpers.service.async_register_platform_entity_service
Can you refactor to use that one? It should not require significant changes to the services implementation.
See https://developers.home-assistant.io/docs/dev_101_services/#entity-service-actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abmantis I'm sorry, are you asking me to move the service registrations for 2 of the services up to the main schlage component, particularly when the services are calling private functions against the lock entity?
I say 2, because the 3rd one returns data and the documentation you linked still has me defining that one where I have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abmantis I'm sorry, are you asking me to move the service registrations for 2 of the services up to the main schlage component, particularly when the services are calling private functions against the lock entity?
Yes. I would recommend removing the underscore prefix from the the function names because they are not private (even with the current implementation). They are passed to async_register_entity_service and called by the service helper, not by the entity class.
I say 2, because the 3rd one returns data and the documentation you linked still has me defining that one where I have it.
Not sure I understand what you mean. The docs on the Response data don't use async_register_entity_service. async_register_platform_entity_service also has support for returning data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://developers.home-assistant.io/docs/dev_101_services/#response-data <- the example for the response data is still shows it being done in async_setup_entry via async_rengister and not async_setup via async_register_platform_entity_service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
developers.home-assistant.io/docs/dev_101_services#response-data <- the example for the response data is still shows it being done in
async_setup_entryviaasync_rengisterand notasync_setupviaasync_register_platform_entity_service
That one is not an entity service like the ones in this PR. But that example needs to be updated too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note: We should update the developer documentation. @abmantis do you have time? If not, I can open a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abmantis I implemented in async_setup_entry as the integration doesn't do yaml based configuration, only config flow configuration and all the documentation I've read says async_setup is for yaml based configuration while async_setup_entry is for config flow based setup. If the integration supported yaml based configuration, then I would have stuck it there but it doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abmantis is right, services should be registered in async_setup. can you please change it?
We recently changed the guidelines, and the documentation is outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll work through it when I can this weekend. I'm aware I already missed the cut-off to make 2025.11 :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the documentation, I only found one remaining example of registering services from async_setup_entry, this PR fixes that: home-assistant/developers.home-assistant#2855
This commit introduces three new services for the Schlage integration: - `add_code`: Allows users to add a new access code to a Schlage lock. - `delete_code`: Enables users to delete an existing access code from a Schlage lock. - `get_codes`: Retrieves all access codes associated with a Schlage lock. Signed-off-by: Andrew Grimberg <[email protected]>
* Move service registration to __init__.py and convert lock services to instance methods. * Update services.yaml and strings.json to use target instead of entity_id. Signed-off-by: Andrew Grimberg <[email protected]>
c9838d4 to
0309971
Compare
| await self.hass.async_add_executor_job(self._lock.unlock) | ||
| await self.coordinator.async_request_refresh() | ||
|
|
||
| # Door code services |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Door code services |
| return {} | ||
|
|
||
| platform = entity_platform.async_get_current_platform() | ||
| platform.async_register_entity_service( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to register them in async_setup_entry instead of async_setup as previously suggested?
| mock_lock.access_codes = {"1": existing_code} | ||
|
|
||
| with pytest.raises( | ||
| ServiceValidationError, match="Code name 'test_user' already exists" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not seem to match any string
| # code is required by voluptuous, the following is just to satisfy type | ||
| # checker, it should never be None | ||
| if code is None: | ||
| raise ServiceValidationError( | ||
| translation_domain=DOMAIN, | ||
| translation_key="schlage_code_required", | ||
| ) # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # code is required by voluptuous, the following is just to satisfy type | |
| # checker, it should never be None | |
| if code is None: | |
| raise ServiceValidationError( | |
| translation_domain=DOMAIN, | |
| translation_key="schlage_code_required", | |
| ) # pragma: no cover | |
| if TYPE_CHECKING: | |
| assert code is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that code is str, so this code should not be needed at all.
| self._validate_code_value(codes, code) | ||
|
|
||
| access_code = AccessCode(name=name, code=code) | ||
| await self.coordinator.hass.async_add_executor_job( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| await self.coordinator.hass.async_add_executor_job( | |
| await self.hass.async_add_executor_job( |
| """Delete a lock code.""" | ||
| codes = self._lock.access_codes | ||
| if not codes: | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this raise ServiceValidationError?
| ) | ||
|
|
||
| if not code_id_to_delete: | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this raise ServiceValidationError?
| if not code_id_to_delete: | ||
| return | ||
|
|
||
| if self._lock.access_codes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already check if codes is empty above, so we can remove the check here
|
|
||
| if self._lock.access_codes: | ||
| await self.coordinator.hass.async_add_executor_job( | ||
| self._lock.access_codes[code_id_to_delete].delete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self._lock.access_codes[code_id_to_delete].delete | |
| codes[code_id_to_delete].delete |
| "schlage_name_exists": { | ||
| "message": "A PIN code with this name already exists on the lock" | ||
| }, | ||
| "schlage_name_required": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is unused
Proposed change
This commit introduces three new services for the Schlage integration:
add_code: Allows users to add a new access code to a Schlage lock.delete_code: Enables users to delete an existing access code from aSchlage lock.
get_codes: Retrieves all access codes associated with a Schlagelock.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: