Add new integration Loqed#70080
Conversation
|
Hi @frenck Thanks for reviewing. Yes I see, i removed the changes from my PR. Only the Codeowners-file gives errors now, but I don't know why. |
This comment was marked as duplicate.
This comment was marked as duplicate.
frenck
left a comment
frenck
left a comment
There was a problem hiding this comment.
Hi there @cpolhout 👋
Some initial comments to get this started. Additionally:
- Missing tests. We require configuration flows to have 100% test coverage.
- What if the lock was not on my internal network? E.g., a remote lock at my parents?
- Consider moving data handling using a
DataUpdateCoordinator - How would it handle multiple locks?
../Frenck
mikewoudenberg
force-pushed
the
loqed-integration
branch
from
3ed449c to
d171456
Compare
|
Hi @frenck now updated this PR. Will remove the other one!. Thanks! |
mikewoudenberg
force-pushed
the
loqed-integration
branch
from
6401ecb to
09e81cb
Compare
|
Hi @frenck , did you have time to review the changes we made? |
This comment was marked as duplicate.
This comment was marked as duplicate.
mikewoudenberg
force-pushed
the
loqed-integration
branch
2 times, most recently
from
3734c7e to
554552d
Compare
mikewoudenberg
force-pushed
the
loqed-integration
branch
from
554552d to
e310e05
Compare
mikewoudenberg
force-pushed
the
loqed-integration
branch
from
e310e05 to
eb0d0e2
Compare
|
I don’t know if it is mentioned somewhere. But if you grab the api key from the website (Login there) it logs you out of the mobile app. (Yes it logs you out everywhere) it’s bad but that should be very clear in order to not get an issue bomb. |
|
@mikewoudenberg Do you know what that is? Can that be prevented? |
Yes that is known. It's mentioned on the webpage of the app: |
|
Same reason Frenck mentioned to login to get the api json. People do not read/lesser user friendly. A warning on the HA side would be good too. |
|
I don't think this should be a warning it at all, in my opinion, it should not log people out. There is no reason for such behavior in modern-day applications. It almost feels like deliberately torturing people doing so. Don't frustrate end-users, that is not needed at all. |
|
Totally agree with that, but that is currently not in our control. Perhaps Mike has closer contacts to get the word out. |
|
The logging out after logging in on the webapp is being looked in to fortunately |
|
The logging out issue has been resolved. The user will no longer be logged out of the native app when logging into the Webapp to copy the setup config. Supporting other types of logging in will take considerable more effort so I don't expect that anytime soon. So I was wondering what kind of next steps you expect from my side to land this pr. I'll start by rebasing it on current develop obviously |
mikewoudenberg
force-pushed
the
loqed-integration
branch
from
36447a4 to
9daa3fe
Compare
mikewoudenberg
force-pushed
the
loqed-integration
branch
from
c13d649 to
6967347
Compare
|
CI failing on one of the tests, otherwise fine |
marcelveldt
left a comment
marcelveldt
left a comment
There was a problem hiding this comment.
Very nice, thanks!
Let's try to improve the setup flow later once this is possible upstream, for now this is totally fine.
Only change left is fix the test so that CI passes. Once that is done this can be merged imo.
Fixed them (forgot to push the updated tests). |
I pushed the fixed tests, but now CI is still failing? Seems to be running into a timeout. |
|
kicked CI as it was indeed failing on something unrelated. |
|
Thanks @mikewoudenberg ! |
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
Please address the comments in a new PR. Thanks!
| lock_data = await cloud_client.async_get_locks() | ||
| except aiohttp.ClientError: | ||
| _LOGGER.error("HTTP Connection error to loqed API") | ||
| raise CannotConnect from aiohttp.ClientError |
There was a problem hiding this comment.
Please raise from the exception instance in this scope instead.
except SomeException as err:
raise AnotherException from err| except InvalidAuth: | ||
| errors["base"] = "invalid_auth" | ||
| else: | ||
| await self.async_set_unique_id( |
There was a problem hiding this comment.
Pass raise_on_progress=False to make the user flow always take precedence over the discovered flow.
|
|
||
|
|
||
| DOMAIN = "loqed" | ||
| OAUTH2_AUTHORIZE = "https://app.loqed.com/API/integration_oauth3/login.php" |
There was a problem hiding this comment.
These constants don't appear to be used.
Does the api support oauth2?
| ) -> None: | ||
| """Initialize the Loqed Data Update coordinator.""" | ||
| super().__init__(hass, _LOGGER, name="Loqed sensors") | ||
| self._hass = hass |
There was a problem hiding this comment.
hass is set in the parent.
| async with async_timeout.timeout(10): | ||
| return await self._api.async_get_lock_details() | ||
|
|
||
| @callback |
There was a problem hiding this comment.
A coroutine function is not a callback.
| @property | ||
| def changed_by(self) -> str: | ||
| """Return internal ID of last used key.""" | ||
| return "KeyID " + str(self._lock.last_key_id) |
There was a problem hiding this comment.
Please use f-strings instead of string concatenation.
| "error": { | ||
| "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]", | ||
| "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]", | ||
| "unknown": "[%key:common::config_flow::error::unknown%]" |
There was a problem hiding this comment.
Unknown string is never used.
| @@ -0,0 +1,22 @@ | |||
| { | |||
There was a problem hiding this comment.
We don't source translations anymore. Please remove this file.
| coordinator = hass.data[DOMAIN][integration.entry_id] | ||
| lock.receiveWebhook = AsyncMock(return_value={"error": "invalid hash"}) | ||
|
|
||
| with patch.object(coordinator, "async_set_updated_data") as mock: |
There was a problem hiding this comment.
Don't patch the coordinator. That's an integration detail. Only assert some core state, eg an entity state in the state machine, or a patched library instance.
https://developers.home-assistant.io/docs/development_testing#writing-tests-for-integrations
| """Tests the lock responding to updates.""" | ||
| coordinator: LoqedDataCoordinator = hass.data[DOMAIN][integration.entry_id] | ||
| lock.bolt_state = "night_lock" | ||
| coordinator.async_update_listeners() |
There was a problem hiding this comment.
Move time forward to trigger an update instead.
Lines 394 to 397 in 98a94fe
|
Thanks for the feedback I'll create a follow-up PR to address these issues. |
15 participants
Breaking change
Proposed change
Type of change
Additional information
Checklist
black --fast 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.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: