adding a bit of error handling, cleaning up code, and updating docs #213
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
guerrerotook
requested changes
Feb 27, 2024
README.md
Outdated
|  | ||
|
|
||
| - Enter the username and password for your Securitas account. | ||
| - Use 2FA (default: no). It is not necessary to use two-factor authentication to use this integration, but we're leaving that option in case Securitas changes that in the future. |
There was a problem hiding this comment.
Hey @cantupaz I think that the default should be yes, enabled by default. I think all countries are actually enabling 2FA right now. That how it was in the past, and the API stopped working. I don't see Securitas not enabling the 2FA or even disabling this. I don't know where you are getting this information from, but this should be enabled by default.
| @@ -50,41 +50,19 @@ | |||
| CONF_DEVICE_INDIGITALL = "idDeviceIndigitall" | |||
| CONF_ENTRY_ID = "entry_id" | |||
| CONF_INSTALLATION_KEY = "instalation" | |||
| CONF_ENABLE_CODE = "enable_code" | |||
| CONF_DELAY_CHECK_OPERATION = "delay_check_operation" | |||
|
|
|||
| DEFAULT_USE_2FA = False | |||
There was a problem hiding this comment.
This should be true, look at my previous comment.
|
I can try to get to this tomorrow, but at least in Spain the 2FA doesn’t do anything. Of course, in the official app, it won’t let you proceed without 2FA, but the API returns the authorization token just after login. You get the exact same token after the 2FA.
From: Luis Guerrero ***@***.***>
Sent: Tuesday, February 27, 2024 11:18 AM
To: guerrerotook/securitas-direct-new-api ***@***.***>
Cc: Erick Cantú Paz ***@***.***>; Mention ***@***.***>
Subject: Re: [guerrerotook/securitas-direct-new-api] adding a bit of error handling, cleaning up code, and updating docs (PR #213)
@guerrerotook requested changes on this pull request.
_____
In README.md <#213 (comment)> :
@@ -14,13 +14,27 @@ securitas:
## Features
-- List all your installations and add a panel into Home Assistant.
-- Support Sentinel and add two sensor for each Sentinel in each installation you have. The sensor are temperature and humidity.
-- If the option is set to False, the check_alarm will only check the last status that securitas have in their server instead of checking in the alarm itself. This will decrease the number of request that show in your account. In this is set to true and you arm or disarm the alarm not throught Home Assistant, this will likely show a different state. The default value is True.
-- Added a new configuration panel to change some of the options that you set during the setup. Here is an screenshot.
+- Lists all your installations and add a panel into Home Assistant.
+- Supports Sentinel and adds temperature and humidity sensors for each Sentinel in each installation you have.
+- Supports installations with Perimetral (external) alarms.
+
+## Setup
+
+
+- Enter the username and password for your Securitas account.
+- Use 2FA (default: no). It is not necessary to use two-factor authentication to use this integration, but we're leaving that option in case Securitas changes that in the future.
Hey @cantupaz <https://github.com/cantupaz> I think that the default should be yes, enabled by default. I think all countries are actually enabling 2FA right now. That how it was in the past, and the API stopped working. I don't see Securitas not enabling the 2FA or even disabling this. I don't know where you are getting this information from, but this should be enabled by default.
_____
In custom_components/securitas/__init__.py <#213 (comment)> :
@@ -50,41 +50,19 @@
CONF_DEVICE_INDIGITALL = "idDeviceIndigitall"
CONF_ENTRY_ID = "entry_id"
CONF_INSTALLATION_KEY = "instalation"
-CONF_ENABLE_CODE = "enable_code"
CONF_DELAY_CHECK_OPERATION = "delay_check_operation"
DEFAULT_USE_2FA = False
This should be true, look at my previous comment.
—
Reply to this email directly, view it on GitHub <#213 (review)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AT44HH2D5W7Q74TRHO7B6VTYVWXG5AVCNFSM6AAAAABD3YA5MSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMBTGA4TMNRWGU> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AT44HH3H4DSNH7C5PFXHNTLYVWXG5A5CNFSM6AAAAABD3YA5MSWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTTRN3ZVS.gif> Message ID: ***@***.*** ***@***.***> >
|
|
That is not true @cantupaz, there are a lot of issues people complaining that it logged but then after some time the integration stop working, #52 here in Spain, France and other countries as well. Again, I don't know exactly where you got that it doesn't do anything. My recommendation, for everyone should be to enable this and as you said, it this fails for you have the option to disable, but not the other way around. This works for you without the 2FA fine, but we have had a lot of issues in the past with this and I believe that not enabling this will likely result in more people opening issues because weird errors that all down to not be authenticated. |
|
Just to be clear, I only want you to change the default value to true, that's it. The rest of the PR is great! |
|
Easy. I said I’ll change it, but probably tomorrow because I got something to do today. Btw, I changed that months ago 😊 I agree it’s safer to switch the default in case one day Securitas stops sending the token after login.
From: Luis Guerrero ***@***.***>
Sent: Tuesday, February 27, 2024 12:05 PM
To: guerrerotook/securitas-direct-new-api ***@***.***>
Cc: Erick Cantú Paz ***@***.***>; Mention ***@***.***>
Subject: Re: [guerrerotook/securitas-direct-new-api] adding a bit of error handling, cleaning up code, and updating docs (PR #213)
That is not true @cantupaz <https://github.com/cantupaz> , there are a lot of issues people complaining that it logged but then after some time the integration stop working, #52 <#52> here in Spain, France and other countries as well.
Again, I don't know exactly where you got that it doesn't do anything. My recommendation, for everyone should be to enable this and as you said, it this fails for you have the option to disable, but not the other way around. This works for you without the 2FA fine, but we have had a lot of issues in the past with this and I believe that not enabling this will likely result in more people opening issues because weird errors that all down to not be authenticated.
—
Reply to this email directly, <#213 (comment)> view it on GitHub, or <https://github.com/notifications/unsubscribe-auth/AT44HHY3SX4QXZMMFOWWF2LYVW4XLAVCNFSM6AAAAABD3YA5MSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRWGMYDKNZSGY> unsubscribe.
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AT44HH7PM7BACTXSVSICYY3YVW4XLA5CNFSM6AAAAABD3YA5MSWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTVGNY34.gif> Message ID: < ***@***.***> ***@***.***>
|
|
Thanks very much!, don't worry about when, take your time. |
|
I made the changes, please take a look. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi. I added a little bit of error handling when arming/disarming. I got rid of the ENABLE_CODE flag that was probably confusing users in the config window (I found it confusing that I could leave the code empty and I also had to uncheck the box to get rid of the PIN).
I added a few missing type hints and updated the readme file.