Improve PIN code handling and add code_arm_required option#335
Improve PIN code handling and add code_arm_required option#335clintongormley wants to merge 5 commits into
Conversation
Uses base AlarmControlPanel entity methods and attributes Allows to disable the PIN code to arm Correctly manage the code_format attribute if no PIN defined (for AndroidAuto)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use .get() with default for CONF_CODE_ARM_REQUIRED in async_step_import to avoid KeyError on older config entries - Default PIN field to existing configured value in options flow so submitting without re-entering doesn't wipe the PIN - Remove dead `code = ""` variable - Fix translation: "PIN code is required to arm ?" → "Require PIN code to arm?" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@poupounetjoyeux I've rebased your PR #326 against Question for you: why did you add the "Require PIN code to arm?" checkbox? This is a breaking change. Before it was enough for the user to fill in the PIN to make it required (and in fact the text in the PIN field still says this). I would remove the checkbox unless you have a good reason for keeping it? |
|
This is to allow arming without code (since its less sensitive than disarming) however in all cases disarming require the PIN |
There was a problem hiding this comment.
Pull request overview
This PR enhances PIN code handling for the Securitas Direct alarm integration by introducing better validation, auto-detection of code format, and a configurable option to require PIN for arming operations.
Changes:
- Improved PIN validation with ServiceValidationError for better user feedback
- Added
code_arm_requiredconfiguration option to control whether PIN is required for arming (disarm always requires PIN) - Auto-detection of code format (NUMBER vs TEXT) based on PIN content
- Options flow now preserves existing PIN value when not re-entered
- Backward compatibility maintained for older configuration entries
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| custom_components/securitas/translations/en.json | Added translations for code_arm_required option and invalid_pin_code exception message |
| custom_components/securitas/config_flow.py | Added code_arm_required option to config flow and options flow, with PIN preservation in options |
| custom_components/securitas/alarm_control_panel.py | Implemented PIN validation with ServiceValidationError, auto-detection of code format, and conditional PIN requirement for arm operations |
| custom_components/securitas/init.py | Added constants and defaults for code_arm_required, renamed DEFAULT_CODE to EMPTY_CODE for clarity |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._update_unsub = async_track_time_interval( | ||
| hass, self.async_update_status, self._update_interval | ||
| ) | ||
|
|
There was a problem hiding this comment.
There's trailing whitespace at the end of this line that should be removed for code cleanliness.
|
|
||
| self._code: str | None = client.config.get(CONF_CODE, None) | ||
| self._attr_code_format: CodeFormat | None = None | ||
| if self._code: | ||
| self._attr_code_format = CodeFormat.NUMBER if self._code.isdigit() else CodeFormat.TEXT | ||
|
|
There was a problem hiding this comment.
There's trailing whitespace at the end of this line that should be removed for code cleanliness.
| self._code: str | None = client.config.get(CONF_CODE, None) | |
| self._attr_code_format: CodeFormat | None = None | |
| if self._code: | |
| self._attr_code_format = CodeFormat.NUMBER if self._code.isdigit() else CodeFormat.TEXT | |
| self._code: str | None = client.config.get(CONF_CODE, None) | |
| self._attr_code_format: CodeFormat | None = None | |
| if self._code: | |
| self._attr_code_format = CodeFormat.NUMBER if self._code.isdigit() else CodeFormat.TEXT |
|
ok, then i'll close this PR in favour of yours |
@clintongormley to be clearer |
|
I'm not sure I understand - you added a checkbox that defaults to requiring a pin code in order to arm the alarm. But that doesn't work with the scenario you describe, wanting to arm the alarm from android without typing in a PIN. Also, you don't require a pin to arm the alarm today (that's why I flagged this as a breaking issue). Ahh I think I understand what is going on. You have two problems:
The way you deal with the non-numeric characters is to type the PIN into the PIN field and have that recognised as non-numeric, so that when you need to DISARM the PIN it allows you to type in whatever you want. But that forces you to use a PIN to ARM the alarm too, so you added the checkbox. I think that mixes concerns. A better way to do it would be to add a checkbox saying "Allow non-numeric PINs" and default it to FALSE. That way there is no-breaking change, but you still get your requirements met. |
|
Another thing to consider is that this pin is local to the Home Assistant installation, it's not related with securitas direct in any form. Actually you can leave your PIN blank and it will be more easy. |
|
Yes there is two different things (and so commits) The first one is dealing with AndroidAuto that requires to have no code at all to apply actions on alarms The second one is having a stronger code (local to HA I know 🙂) than just digits (in my case to disarming only but in classical case for all actions) Since I let the default comportment (PIN code digit or text always required when disarming and by default for arming) I don't see where you think about a breaking change. It will just add more flexibility for the user However I will recheck when rebasing if I missed something and retest again that migrating from an existing config keep the same comportement as the actual version 😉 |
I'm ok with the non-numeric fix, it's the In summary, I think you should add a checkbox for accepting non-numeric PINs (which would serve for both the PIN for arming and the PIN for disarming), but I wouldn't add the |
With the current code (I just checked) method There is no different PINs it's the same for disarming and (or not regarding my new flag) arming |
This is incorrect. The PIN field in the configuration is ONLY about arming, not about disarming. I have no PIN set in that field and I can arm without typing a PIN. I do, however, have to type a PIN to disarm. Also, they can be different PINs. The disarming PIN Is passed directly to Verisure. The arming PIN is only used within HA. See @guerrerotook's comment where he says the same thing: |
This is not what he's saying He is saying that the PIN is not linked to the Verisure one and is only for HA dashboard purpose (and this is what I want to manage/securize) So the options is also only for HA dashboard purpospe and allows to type the code only when disarming So regarding it's comment, my response is 'No because I want one for disarming so letting it empty doesn't fit my needs' I just rebased the PR #326, installed it locally and tested and can confirm it matches with what I'm saying By the same way I tested you mapping modification and don't undestand how I can map my arm_away option to do the same as an arm_night ? EDIT: |
I looked at the code now and my assumptions were incorrect. My apologies! The PIN is never requested for arming the alarm. And the PIN is always requested for disarming the alarm. The only difference is that, if the PIN is set, HA compares the input PIN to the field value before deciding whether to pass that on to Securitas or not. I had that completely wrong. I don't know how you managed to disarm the alarm without a PIN. In that situation what PIN would HA have passed to Securitas? So in that case I would keep your changes, but leave the "Require PIN for arming" checkbox unticked by default, as that is the breaking change I've been concerned about.
Ah OK. In Italy and Spain, all I have is Total/Partial/None. Yes sure, I can add it back! I'll make it Total/Partial Day/Partial Night/None, where Partial Night matches to |
|
@poupounetjoyeux I'm missing one status code that Verisure returns.
What status does it return for If you turn on debug logging, set the alarm to PARTIAL_NIGHT_PERI, look for the value in thanks |
|
No worry, I prefer someone that is challenging the code 😉 HA is passing the installation API token and not the PIN (this is what @guerrerotook said in its comment) Currently the PIN is always required for arming and disarming (and check before going further to the Verisure API) because the integration is not calling the base method So finally, having no PIN for disarming will just hides the keyboard on your HA dashboard but works as it is currently working meaning you can arm and disarm without typing any PIN and the code arm required will be ignored (useful if you already secured HA on your phone with digital print for example) |
I will try to debug but typically we have no perimeter on this kind of alarm 😅 |
If you can't find it, that's ok. That status just tells us what status to display on the alarm widget. If the right one is missing then it will just map to Custom. We can add a note in the docs that we need somebody to tell us what the right code is. Sooner or later somebody will run into this issue |
Will try and update if I find it Maybe that we could add a Verisure central version and allows mapping types regarding the version (clearly a nice to have but could be simpler for users to not make mistakes with the mapping 😁) |
Summary
Based on #326 by @poupounetjoyeux, rebased onto main with the
arm_away_as_arm_nightfeature removed and several fixes applied:check_code()with_check_code()that raisesServiceValidationErroron mismatch (shows error in UI instead of silently failing)code_arm_requiredoption: New config/options toggle to control whether a PIN is required to arm (disarm always requires PIN if configured)code_formatas NUMBER or TEXT based on PIN content.get()with defaults for new config keys inasync_step_importto avoidKeyErroron older entriescode_arm_requiredlabelChanges from #326
arm_away_as_arm_nightfeature (out of scope)async_step_importKeyError for older config entries missingcode_arm_requiredTest plan
code_arm_requiredin config data loads without errorcode_arm_required = falseallows arming without PIN🤖 Generated with Claude Code
Closes #326