core icon indicating copy to clipboard operation
core copied to clipboard

Add august open action

Open m10x opened this issue 1 year ago • 23 comments

Breaking change

Proposed change

Added support for opening locks

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [x] 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 #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • [ ] The code change is tested and works locally.
  • [ ] Local tests pass. Your PR cannot be merged unless tests pass
  • [ ] There is no commented out code in this PR.
  • [ ] I have followed the development checklist
  • [ ] I have followed the perfect PR recommendations
  • [ ] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [ ] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

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 running python3 -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:

m10x avatar Mar 19 '24 07:03 m10x

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Mar 19 '24 07:03 home-assistant[bot]

Hey there @bdraco, mind taking a look at this pull request as it has been labeled with an integration (august) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of august can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign august Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar Mar 19 '24 07:03 home-assistant[bot]

Works flawlessly in my fresh dev docker and my yale linus L2. The question is, what if a smart lock only has lock/unlock but no open action

m10x avatar Mar 19 '24 12:03 m10x

Hi, when will this be included in the stable version? Thank you!

mbay0r avatar Apr 24 '24 12:04 mbay0r

Hi, when will this be included in the stable version? Thank you!

There are 2 problems afaik that need to be solved before that:

  1. Coverage for one method is missing (https://github.com/home-assistant/core/pull/113795#discussion_r1530762028) I added a PR for that, but one of the checks is failing and I don't get why.

  2. It should only be set for devices that support the open action (https://github.com/home-assistant/core/pull/113795#discussion_r1530761482) However, I have no idea what the best way is to solve this.

I'm waiting for feedback on these 2 problems

m10x avatar Apr 24 '24 14:04 m10x

  1. missing

Can I somehow integrate this into my system now? Do you perhaps have a hint? I only have devices that support open

It still seems to take a while.

Thank you!

mbay0r avatar Apr 29 '24 13:04 mbay0r

I'm eagerly waiting for it to be merged, too. I can see if it's possible to create a new repo and add the required files to turn It into a HACS compatible Integration. That way it might be easy to add it as custom Repository in HACS, so we can use it that way until this PR gets merged. However, I've never done this before, so not sure if it works.

m10x avatar Apr 29 '24 19:04 m10x

2. It should only be set for devices that support the open action (Add august open action #113795 (comment)) However, I have no idea what the best way is to solve this.

Its likely going to be a PR to yalexs to add a property if it has open or not.

Hopefully the device details from the api can tell us if it supports open or not

bdraco avatar Apr 30 '24 13:04 bdraco

seeing whether the door is open or not would of course also be great. is this already being worked on?

thanks!

mbay0r avatar Apr 30 '24 13:04 mbay0r

2. It should only be set for devices that support the open action (Add august open action #113795 (comment)) However, I have no idea what the best way is to solve this.

Its likely going to be a PR to yalexs to add a property if it has open or not.

Hopefully the device details from the api can tell us if it supports open or not

I looked at the device Details and other API Request, however I could not see any info about whether opening is supported or not. We could go by the lock model and compare it to a list of models that Support opening.

m10x avatar Apr 30 '24 17:04 m10x

We could go by the lock model and compare it to a list of models that Support opening.

If we hide that behind a property or function in yalexs we can always switch it out later to something better if we find a way to get it from the device details

bdraco avatar Apr 30 '24 17:04 bdraco

We could go by the lock model and compare it to a list of models that Support opening.

If we hide that behind a property or function in yalexs we can always switch it out later to something better if we find a way to get it from the device details

Created a PR for this https://github.com/bdraco/yalexs/pull/105 (the new assertion in test_async_get_lock_with_unlatch is failing. I don't get why atm)

m10x avatar Apr 30 '24 19:04 m10x

I've updated my 2 PRs to yalexs https://github.com/bdraco/yalexs/pull/105 https://github.com/bdraco/yalexs/pull/103 and I've already prepared the update to this PR (PR should be finished then). If the yalexs PRs are merged and a new version is published, I can test and push my update here.

m10x avatar May 01 '24 10:05 m10x

lib bump in https://github.com/home-assistant/core/pull/116511

bdraco avatar May 01 '24 13:05 bdraco

I'm not sure yet how to access the new unlatch_supported attribute. I though that I can do something like

def __init__(self, data: AugustData, device: Lock) -> None:
[...]
     if self._unlatch_supported:
          self._attr_supported_features = LockEntityFeature.OPEN

but the attribute _unlatch_supported is unknown

Edit: oh, it's if self._detail.unlatch_supported:

m10x avatar May 01 '24 16:05 m10x

Code looks good. Should be good to merge once tests are added

bdraco avatar May 01 '24 17:05 bdraco

Code looks good. Should be good to merge once tests are added

Ok great. I'll Look into that tomorrow

m10x avatar May 01 '24 18:05 m10x

I've added a test for the hass lock.open action. Is this sufficient or are more tests needed?

m10x avatar May 02 '24 07:05 m10x

It looks like you might be manually trying to fix ruff issues.

I'd recommend installing pre-commit

https://developers.home-assistant.io/docs/development_testing/

bdraco avatar May 04 '24 12:05 bdraco

The one check is failing because homeassistant.exceptions.HomeAssistantError: Entity lock.online_with_doorsense_name does not support this service.. That's correct because currently only devices with the Type 17 get the Unlock Support Flag set. So I guess we need a new fixture (like https://github.com/bdraco/yalexs/blob/master/tests/fixtures/lock_with_unlatch.online.json) and use that. However I currently don't get how the fixtures are loaded. It test_lock uses lock.online_with_doorsense_name however there is no fixture with that exact name. Also it would be good to assert that the current fixture lock does not have the unlock support flag set. However, that's another thing I currently don't get

m10x avatar May 04 '24 12:05 m10x

You'll need to copy the fixture and than add a new one in https://github.com/home-assistant/core/blob/63484fdaddaca7ec92ed95376a94b8fb2b1daa97/tests/components/august/mocks.py#L365

bdraco avatar May 04 '24 13:05 bdraco

Well, I just found out that I can run the tests with pytest locally... Should have read the whole Development Workflow in the docs earlier, sorry

m10x avatar May 04 '24 19:05 m10x

Wow, thank you a lot, I was already frustrated, as the L1 could open the door..

dbara avatar May 09 '24 18:05 dbara

Wow, thank you a lot, I was already frustrated, as the L1 could open the door..

You're welcome! Currently only the L2 will be getting the open feature, because I created a whitelist of lock models that support unlatching in the yalexs api (https://github.com/bdraco/yalexs) this component is using. Currently only model 17 (Yale Linus L2) is included. So, if you would like the L1 to have this feature too, I'll need the model number in order to add it to yalexs (or you can create a PR yourself). The easiest way to get the model number is probably to turn on debug logging for the august component. Then you can search the logs for "Type":. You can use e.g. Studio Code Server to open /homeassistant/home-assistant.log.

m10x avatar May 10 '24 07:05 m10x

If the model id is not in diagnostics, it would be nice to add it there as well

bdraco avatar May 10 '24 07:05 bdraco

If the model id is not in diagnostics, it would be nice to add it there as well

It is already. One can search for "Type": in the debug logs, I just found out. I've updated the command above for the easier way

m10x avatar May 10 '24 09:05 m10x

@bdraco The negative test is working. A lock that does not support unlatching (according to yalexs) does not have the open feature flag set. However, the positive test (https://github.com/home-assistant/core/pull/113795/commits/5996c5445f9b00bb2857f1872a5bfa17bbdf22c1) leads to the error message FAILED tests/components/august/test_lock.py::test_open_lock_operation - TypeError: object MagicMock can't be used in 'await' expression. I'm not able to find out why. Is there a mock missing in mocks.py? I guess the positive test is the only missing thing until this can be merged?

m10x avatar May 10 '24 10:05 m10x

@bdraco The negative test is working. A lock that does not support unlatching (according to yalexs) does not have the open feature flag set. However, the positive test (5996c54) leads to the error message FAILED tests/components/august/test_lock.py::test_open_lock_operation - TypeError: object MagicMock can't be used in 'await' expression. I'm not able to find out why. Is there a mock missing in mocks.py? I guess the positive test is the only missing thing until this can be merged?

@bdraco sorry for tagging you again :) The test for devices which don't support unlatching works. The test for devices which support it does not work and I don't know how to fix that.

m10x avatar May 23 '24 08:05 m10x

could someone please be so kind and help @m10x ? it has been dragging on for two months now and no end in sight :( thanks a lot!

mbay0r avatar May 27 '24 13:05 mbay0r

It's on my review list. There are still ~30 PRs ahead of it. Please be patient; there are a lot of people who need help. If the test gets solved before I have a large enough time block to be able to analyze whats needed, I will come back to it sooner as I have much more small time blocks than large ones.

bdraco avatar May 27 '24 22:05 bdraco