Skip to content
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

Add a service to list lock records #170

Merged
merged 16 commits into from
Jan 14, 2025

Conversation

samuellazea
Copy link
Contributor

Adding functionality for getting the lock events records history.
Basically adding TTLock: List lock records service that will print the history of the locks states

records: lock.lock_box: - id: 123456 lock_id: 15450395 record_type: KEYBOARD record_type_from_lock: 7 success: false username: 0*** keyboard_pwd: "0608" lock_date: "2025-01-06T15:01:58+02:00" server_date: "2025-01-06T15:00:42+02:00" lock.usa: - id: 965191616 lock_id: 123456 record_type: KEYBOARD record_type_from_lock: 4 success: true username: Test keyboard_pwd: "1234" lock_date: "2025-01-08T14:20:37+02:00" server_date: "2025-01-08T14:19:17+02:00"

Copy link

codecov bot commented Jan 8, 2025

Codecov Report

Attention: Patch coverage is 94.70199% with 8 lines in your changes missing coverage. Please review.

Project coverage is 85.50%. Comparing base (bbe0ef0) to head (be6958b).
Report is 17 commits behind head on develop.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
custom_components/ttlock/api.py 12.50% 7 Missing ⚠️
tests/conftest.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #170      +/-   ##
===========================================
+ Coverage    84.16%   85.50%   +1.33%     
===========================================
  Files           20       20              
  Lines         1017     1166     +149     
===========================================
+ Hits           856      997     +141     
- Misses         161      169       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samuellazea
Copy link
Contributor Author

I created this branch based on the list-passcodes branch so that there would be no merge conflicts.
If the lists-passcodes is ok and you decide to merge it into the main branch, then I can do a pull here and then if you are ok with this branch also, then the PR should be up to date with no effort of merging.

This branch is basically exposing the List lock records to be able to see an history of the locks. I also added the necessary tests and I also performed manual tests with multiple changes to confirm it's working including 2 locks in the same time.

image

I was thinking maybe to add this also to a sensor but I think it will be too heavy and then dropped the idea.
When you look at this please let me know if you see anything that doesn't fit or would need some change ;)

Copy link
Owner

@jbergler jbergler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty reasonable, I have a few questions - and it looks like you'll need to rebase/merge develop back into your branch to make it mergeable.

Comment on lines 212 to 213
success: bool
username: Optional[str] = None
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these be defined as Fields for consistency? (even if they don't need an alias)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :) thanks for pointing it out

custom_components/ttlock/models.py Outdated Show resolved Hide resolved
custom_components/ttlock/services.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
@jbergler jbergler changed the title Records list functionality Add lock record list functionality Jan 14, 2025
@jbergler jbergler changed the title Add lock record list functionality Add a service to list lock records Jan 14, 2025
@jbergler jbergler merged commit a32b633 into jbergler:develop Jan 14, 2025
7 checks passed
@jbergler
Copy link
Owner

Thanks again for the contribution - I'll cut a release soon (there's one other PR open that I might try and see if we can get it in at the same time)

@samuellazea
Copy link
Contributor Author

@jbergler I am just sitting here and thinking. Now that we have the lock records service, wouldn't it be an improvement to take the last_user and maybe the last_reason from the service on a certain interval and populate them in the sensor ( coordinator ) even if there is no webhook set up?

I am thinking this way we don't need to expose the HA anymore in order to get the last user and the reason. For me the webhook it's not the best option since I either have to expose HA to the internet ( which I don't want at all for lots of reasons ) or setup a cludflared tunnel just for this. All while we could fetch the data from the lock records on an interval.

Of course this is me fantasying here :) Please keep me honest here

@jbergler
Copy link
Owner

It's not a terrible idea, but unfortunately the API rate limits are pretty low so this doesn't work well if you have more than a few locks.
I know it probably works better for some peoples use cases than the current solution, but I don't really want to start trying to handle this path as well as the webhook event driven way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants