Add abode config entries and device registry

[shred86]

Breaking Changes:

This PR removes the following configuration variables: name, exclude and lights, which were all previously optional. Existing users of the abode integration that use these configuration variables will have to remove them from the configuration.yaml file. Entities that users wish to disable can be done from the Entity Registry in the Configuration UI.

Description:

Adds config entry and entity registry support for the Abode component. This implementation will create a config entry if a user has added the abode component configured via the configuration.yaml file. The entity registry unique ID is based on device_id from Abode (no devices in Abode should ever have the same device_id).

Tested on my Abode setup which includes sensors and lights.

Docs PR:

home-assistant/home-assistant.io#10467

Checklist:

• The code change is tested and works locally.
• Local tests pass with tox. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the [development checklist][dev-checklist]

If the code communicates with devices, web services, or third-party tools:

• [The manifest file][manifest-docs] has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
• Untested files have been added to .coveragerc.

[shred86]

Hey @frenck, what type of documentation would I need to create? The setup configuration settings if using the .yaml file hasn’t changed. I looked at a few other components that support config entires and didn’t really see anything I needed to add. Perhaps just a note it can be added via the Integrations UI now? Let me know and I’ll definitely get it taken care of! Thanks.

[frenck]

@shred86 The documentation page needs a frontmatter option: ha_config_flow which is set to true, furthermore, it would be helpful to explain to a user how to activate/add the integration using a config flow.

As for this PR, please add unit tests. Config flows require unit tests.

[shred86]

@shred86 The documentation page needs a frontmatter option: ha_config_flow which is set to true, furthermore, it would be helpful to explain to a user how to activate/add the integration using a config flow.

As for this PR, please add unit tests. Config flows require unit tests.

Ah okay, thanks. Just created a pull request with the updated documentation (home-assistant/home-assistant.io#10467). I’ll try to add unit tests soon. Unfortunately I’m fairly new to Python and will be fairly busy for the next two weeks.

[shred86]

Unfortunately I'm struggling with unit tests. If there's any guidance/documentation specific to writing unit tests for Home Assistant, please let me know. The basics of using the unittest module in python makes sense, but I'm not having much luck in understanding how they're written for Home Assistant, or more specifically how to execute async unit tests. I'll keep at it to try and figure it out, but any assistance would be appreciated.

[MartinHjelmare]

Read the pytest docs. Read a blog on unit testing eg this one:
https://realpython.com/python-testing/

We want all new tests to be standalone pytest test functions.

See existing tests eg:
https://github.com/home-assistant/home-assistant/blob/dev/tests/components/hue/test_config_flow.py

[shred86]

Added unit test for config flows. I used the SimpliSafe component as a reference. I think I covered all the test cases needed.

[shred86]

Just pushed a fix for this issue. Previously, when setup_hass_events was being called, it would execute hass.bus.listen_once(EVENT_HOMEASSISTANT_START, startup). This is why it was only working after a reboot since it was waiting for the EVENT_HOMEASSISTANT_STARTUP event to fire. I couldn't think of any reason not to simply call the startup function when setup_hass_events is called (except for the case where polling is enabled), so that's what the changes reflect in addition to making the code async. The function setup_hass_events should only be called during initial HA start or when the abode component is added, so there shouldn't ever be a situation where it's called twice.

[shred86]

@MartinHjelmare Thanks for review. Most of these make sense but there's a few I'll have to do some digging around to figure out. Thought I understood what CONFIG_SCHEMA was doing but apparently not. Once I get these changes/fixes implemented, should I just submit another push from my repo? I noticed there's an option on here to "commit suggestion" but I don't want my repo to get out of sync.

[MartinHjelmare]

Regarding suggestions, you can do either. If you commit the suggestion directly in github, you should pull from remote before you do local changes and push again.

[shred86]

Just made a new commit with fixes to all the suggestions. Marked everything as resolved that I verified was corrected.

[MartinHjelmare]

Please solve the merge conflict.

[shred86]

Another commit with fixes to all the suggestions. Also realized the unit test wasn't actually doing what I thought it was - should be fixed now and added another test case for the updated exception handling.

I'm not sure why there is a branch merge conflict. I haven't touched the manifest.json file but it looks like there's a mismatch between codeowners. The last time this branch merge conflict occurred, it resolved itself (or someone resolved it).

[MartinHjelmare]

I don't think the merge conflict will resolve itself.

[shred86]

Another commit to fix the code review suggestions. I also updated the manifest.json - I have no idea why it's flagging a branch conflict for me adding:

"config_flow": true

I took the latest manifest.json from the dev branch and only added the config_flow setting and updated the codeowners.

[shred86]

Forgot to delete this - corrected on the next commit.

Edit: Sorry, thought it would reference the line. It's line 67 in __init__.py: vol.Optional(CONF_NAME): cv.string,

And line 21: CONF_NAME, line 35: NOTIFICATION_ID = "abode_notification" and line 36 NOTIFICATION_ID = "abode_notification".

None of these constants are being used and will be removed:

EVENT_ABODE_ALARM = "abode_alarm"
EVENT_ABODE_ALARM_END = "abode_alarm_end"
EVENT_ABODE_AUTOMATION = "abode_automation"
EVENT_ABODE_FAULT = "abode_panel_fault"
EVENT_ABODE_RESTORE = "abode_panel_restore"

[MartinHjelmare]

If the dev branch changes the same file as we change here and git can't resolve, there will be a merge conflict. The manifest.json file on dev branch was changed three days ago. We need to solve that merge conflict by rebasing on latest dev branch and specify how to solve the conflict.

[shred86]

Rebased and committed some small changes I mentioned above. Additionally, updated the original post in this PR with the break changing and updated the PR for the documentation. I've also fixed the issue with switches being added as switches and lights from Abode which I'll include in the next commit along with some minor reorganizing of the imports to adhere to the style guidelines.

Edit: Re-working test_config_flow.py to fix some of the errors CI is finding and some others I've found.

[shred86]

Added option to enable polling to config flow UI. Personally, I think we can just get rid of polling since cloud push is obviously much faster and less demand on abode's servers, but I suppose it's worth keeping in the event someone needs to use it. I added the label "Polling (not recommended)" to make it fairly clear it shouldn't be used normally. I think a better option might be to remove the option from the UI and only allow enabling polling through the configuration.yaml. However, I tried reworking config_flow.py but ran into issues with getting exceptions to assert properly when moving the credential testing to a separate function. I'll continue trying to work it.

Additionally, pylint is complaining about from .const import DOMAIN in config_flow.py not being used when it is in fact being used:

class AbodeFlowHandler(config_entries.ConfigFlow, domain=DOMAIN):
    """Config flow for Abode."""

Not entirely sure how to clear that up as it's failing CI tests.

[shred86]

Hopefully that is the last commit. :)

[MartinHjelmare]

Looks good!

[MartinHjelmare]

Sorry, I noticed that the coverage isn't 100% for the config flow module.

https://codecov.io/gh/home-assistant/home-assistant/compare/a82ff4f7a94fc6f2c635f9ecc260211654fcc42d...9f01395f65fa894e8d88294e6836d3575e29f3d2/diff#D1-77

[shred86]

Added additional tests to the test_one_config_allowed function. Previously it was only covering the case where async_step_user was being called with an existing config entry. I simply added onto that same function and "mocked" an import config file and called async_step_import so both cases are covered now.

[MartinHjelmare]

Good!

[shred86]

Thanks for all the help @MartinHjelmare - I learned a ton in this process which I realize comes at the expense of your time fixing all my errors. I started learning python/programming primarily to help contribute to Home Assistant. I'm hoping to continue learn and contribute where I can. Cheers.

[shred86]

Noticed one minor issue - there is no .translations folder in the abode component folder. I was under the impression the strings.json file was used to create that translations folder, but not something I was supposed to do on my end.

Edit: Just found out on the discord channel that it will automatically be added at midnight.