palazzem / econnect-python

API adapter used to control programmatically an Elmo alarm system
BSD 3-Clause "New" or "Revised" License
8 stars 5 forks source link

Add support for selective exclusion and readmission of inputs #55

Closed davidecavestro closed 2 years ago

palazzem commented 4 years ago

Thanks @davidecavestro for your contribution! I will go through the code to give you a feedback, but thanks for adding tests and documentation for the new API.

Just one question before I start the review: can you describe me here the goal of the new API and what are you trying to achieve? If you want, it's even fine if you write me a snippet about how you use the API. I'd like to keep the API smallest as possible, but I'm also happy to add everything that is needed.

It will help me to understand better the use case. Thank you very much!

davidecavestro commented 4 years ago

I'm writing a Kivy app to control my home alarm.

I can share the whole project as FOSS (I'd do it anyway sooner or later). It is basically a stripped down KivyMD KitchenSink demo: I just need the time to do some cleanup before pushing it to github.

I'd like to achieve something similar to what you can do on alarm inputs from the web UI: image

The kivy counterpart (still an early work in progress) at the moment is image

It reacts to a click on the checkbox calling the exclude command this way

    def exclude_sensor(self, sensor_id):
        def job():
            self.auth()

            pin = self.get_pin()
            with self.client.lock(pin) as c:
                try:
                    print('excluding sensor ', sensor_id)
                    c.exclude([sensor_id])
                finally:
                    self.client.unlock()
                vibrate(2)

        self.executor.submit(job)

Some notes:

davidecavestro commented 4 years ago

I'm still investigating on how the bypass/exclusion status is exposed (in order to set the initial checkbox value)

Could it be useful adding something like raw query methods to expose the data as returned by the server?

To this purpose this is an excerpt of the whole set of attributes exposed by the server api respectively for sectors and inputs.

SECTORS

'Active' = {bool} False
'ActivePartial' = {bool} False
'Max' = {bool} False
'Activable' = {bool} True
'ActivablePartial' = {bool} False
'InUse' = {bool} True
'Id' = {int} 7010
'Index' = {int} 0
'Element' = {int} 1
'CommandId' = {int} 0
'InProgress' = {bool} False

INPUTS

'Alarm' = {bool} False
'MemoryAlarm' = {bool} False
'Excluded' = {bool} False
'InUse' = {bool} True
'IsVideo' = {bool} False
'Id' = {int} 26114
'Index' = {int} 0
'Element' = {int} 1
'CommandId' = {int} 0
'InProgress' = {bool} False
palazzem commented 4 years ago

Side note: I've also reconfigured CircleCI. It should run tests from your fork now.

codecov-commenter commented 4 years ago

Codecov Report

Merging #55 (61c32e3) into master (c6c5257) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #55   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          240       276   +36     
=========================================
+ Hits           240       276   +36     
Impacted Files Coverage Δ
elmo/api/client.py 100.00% <100.00%> (ø)
elmo/api/exceptions.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c6c5257...61c32e3. Read the comment docs.

palazzem commented 2 years ago

@davidecavestro checking this PR again. I'll take care of the merge so no action items on you!

palazzem commented 2 years ago

@davidecavestro can you allow me to edit and push changes to your fork? It should be something along these lines: https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

davidecavestro commented 2 years ago

Changes from mantainers seems already enabled, I've unchecked and rechecked image I also invited you as a collaborator on my fork, just in case...

palazzem commented 2 years ago

Weird, it tells me:

remote: Permission to davidecavestro/econnect-python.git denied to palazzem.
fatal: unable to access 'https://github.com/davidecavestro/econnect-python.git/': The requested URL returned error: 403

Can you try to give me admin access to your fork? So I can override any settings and update your master. Thanks!

davidecavestro commented 2 years ago

Weird, it tells me:

remote: Permission to davidecavestro/econnect-python.git denied to palazzem. fatal: unable to access 'https://github.com/davidecavestro/econnect-python.git/': The requested URL returned error: 403

I still see the invitation as pending

Can you try to give me admin access to your fork? So I can override any settings and update your master

I guess GH doesn't let me give administrator access from a personal namespace (not an organisation) I could try to transfer the fork to you (no prob for me, it was meant just to propose patches to you)

Or if you prefer I can somewhat extract a patch from git.

EDIT: just reformatted... I replied via email but it was a mess...

palazzem commented 2 years ago

@davidecavestro here we go. Permissions weren't updated for an access token I was using, all good now!

palazzem commented 2 years ago

@davidecavestro contribution merged! thank you very much for pushing toward this direction. I'm moving forward to integrate the change we discussed. Thanks again!