Add zwave js services get_lock_usercode and g 8000 et_lock_usercodes #119714
Conversation
There was a problem hiding this comment.
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @home-assistant/z-wave, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
3f1d96f to
759d880
Compare
| "get_lock_usercode": "mdi:lock-percent-outline", | ||
| "get_lock_usercodes": "mdi:lock-percent", |
There was a problem hiding this comment.
I struggled to find an appropriate icon. If you know of something more intuitive or uniform with other icons in the app, let me know!
759d880 to
5c7d10c
Compare
|
This review could use some eyes from Home Assistant dev team, maybe @home-assistant/z-wave |
|
I just tested this with HA Core 2024.08.04, and it still works great. No merge conflicts. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
| and ATTR_USERCODE in code_slot | ||
| and code_slot.get(ATTR_USERCODE) | ||
| } | ||
| return {"usercodes": usercodes} |
There was a problem hiding this comment.
why don't we just return the usercodes?
| "User code at slot %s on lock %s retrieved", code_slot, self.entity_id | ||
| ) | ||
| usercode: str | None = code_slot_obj.get(ATTR_USERCODE) # type: ignore[assignment] | ||
| return {"usercode": usercode} |
There was a problem hiding this comment.
why don't we return just the user code?
There was a problem hiding this comment.
I agree. Makes more sense to return the code vs an object.
There was a problem hiding this comment.
@raman325 I did this to avoid breaking changes in the future if additional metadata is added. As is, adding a new key-value pair would not break any existing code, but switching from a single object to a dict in the future would. Since this project is used by many users, each of which would have do update their code for any breaking changes, I err slightly on the side of future-proofing over simplicity.
I based this on other HA modules that used a similar pattern, but I submitted this PR back in early June but wasn't reviewed until almost November, so I no longer recall which modules I studied.
I'll go ahead and make the change to expedite getting this PR out, but in the future I suggest avoiding holding up PRs over minor nitpicks, especially if the review cycle is around 5 months.
There was a problem hiding this comment.
@holocronweaver thanks for giving me additional context. I see your point about future compatibility, but I am not sure that we want these services to change in the future, their scope is extremely limited. Are there future features that you are thinking about in particular?
As for whether this change should block this PR from being merged, it's not a minor nitpick because once we merge it, we can't change the return signature without a lot of extra work and time to deprecate etc. It would be nice to shorten the review cycle, but that's a resourcing problem (not enough reviewers), not a minor nitpicking problem, and we don't lower the quality bar to speed up velocity.
|
@holocronweaver any progress on the small requested change? |
5c7d10c to
9d9c763
Compare
9d9c763 to
840896c
Compare
| if action_type == SERVICE_GET_LOCK_USERCODE: | ||
| return { | ||
| "extra_fields": vol.Schema( | ||
| { | ||
| vol.Required(ATTR_CODE_SLOT): cv.string, | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
you'd have to update the device action tests to cover this. There are multiple tests that likely need to be updated in the test_device_action model to cover this additional service
There was a problem hiding this comment.
you can also pull out the device action changes instead and submit separately, I don't think we are a 6D47 ctually at parity with the new services for device actions so you could include those other ones. Just a thought, not a requirement
8f7aaaa to
2e4c914
Compare
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
I will get around to finishing this in a few weeks, sorry for the delay. |
| @@ -302,6 +304,21 @@ | |||
| }, | |||
| "name": "Clear lock user code" | |||
| }, | |||
| "get_lock_usercode": { | |||
| "description": "Get a user code for a lock.", | |||
There was a problem hiding this comment.
For consistency with other actions please change to:
- "Retrieves a user code for a lock."
and for the other action:
- "Retrieves all user codes for a lock."
Using descriptive wording also helps in getting consistent translations.
Any chance you could also make a forum post showing how you created the Lovelace view in the OP? |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
I cherrypicked this a while back and have been running it for months. It works great. |
Proposed change
This resolves a feature request to add Z-wave JS services for retrieving lock usercodes.
A good use case for these services is creating a Z-wave lock management dashboard card like this:
In the 'Code Slot' section of this card, selecting a code slot index automatically updates the lock code to the current value for that slot (via an automation using
get_lock_usercode). This can help remind you of the code currently at that slot and possibly avoiding accidental overwrite/clear of a slot you forgot was in use.Further down the card in the 'All Codes' section, all usercodes from the main lock are shown (via
get_lock_usercodes) which is handy when reviewing all usercodes at a glance, especially when you have 10+ slots that may not be set sequentially.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..coveragerc.To help with the load of incoming pull requests: