Dedup and clarify imported konnected config flows #32138

kit-klein · 2020-02-24T13:30:38Z

Proposed change

This PR addresses some of the UX concerns brought up in item 1 and 2 of #32076 and should be included in the 0.106 release.

Previously importing from configuration.yaml and ssdp discovery would result in two config flows being started. Now the ssdp discovered flow is aborted and it's host info utilized by the import flow. This more accurately mimics historical konnected component behavior where host info was discovered if not included in the configuration.yaml.

This PR also includes a small change to utilize imported configuration.yaml options until the user goes thru the options flow. This has no impact on Konnected panels that are being setup completely thru the config flow but it does allow for configuration.yaml based setups to be imported and utilized without forcing the user to run thru the options flow.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example entry for `configuration.yaml`:

# Example configuration.yaml
konnected:
  access_token: arandomstringvalue
  api_host: http://192.168.1.223:8123 #IP address of Home Assistant
  devices:
    - id: a020a61b6233  
      binary_sensors:
        - zone: 4
          type: motion
          name: 'Hallway Motion'
        - zone: 5
          type: window
          name: 'Master Bedroom Window'
        - zone: 6
          type: window
          name: 'Downstairs Windows'
      switches:
        - zone: 1
          name: 'siren'

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue: Konnected Config Flow issues in 106 beta #32076
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

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

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

probot-home-assistant · 2020-02-24T13:30:44Z

This pull request needs to be manually signed off by @balloob before it can get merged.

probot-home-assistant · 2020-02-24T13:30:50Z

Hey there @heythisisnate, mind taking a look at this pull request as its been labeled with a integration (konnected) you are listed as a codeowner for? Thanks!

heythisisnate · 2020-02-24T14:05:37Z

LGTM

homeassistant/components/konnected/.translations/en.json

homeassistant/components/konnected/panel.py

codecov · 2020-02-24T14:27:29Z

Codecov Report

Merging #32138 into dev will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev   #32138      +/-   ##
==========================================
- Coverage   94.74%   94.73%   -0.01%     
==========================================
  Files         767      768       +1     
  Lines       55504    55619     +115     
==========================================
+ Hits        52587    52693     +106     
- Misses       2917     2926       +9

Impacted Files	Coverage Δ
homeassistant/util/dt.py	`95.36% <0%> (-4.12%)`	⬇️
homeassistant/components/uk_transport/sensor.py	`93.47% <0%> (-0.73%)`	⬇️
homeassistant/components/buienradar/camera.py	`96.34% <0%> (-0.05%)`	⬇️
homeassistant/components/twitch/sensor.py	`96.7% <0%> (-0.04%)`	⬇️
homeassistant/components/demo/media_player.py	`95.67% <0%> (-0.03%)`	⬇️
homeassistant/components/device_tracker/legacy.py	`99.14% <0%> (-0.01%)`	⬇️
homeassistant/components/rflink/light.py	`100% <0%> (ø)`	⬆️
homeassistant/components/darksky/sensor.py	`96.81% <0%> (ø)`	⬆️
homeassistant/components/mqtt/fan.py	`97.96% <0%> (ø)`	⬆️
homeassistant/components/withings/const.py	`100% <0%> (ø)`	⬆️
... and 43 more

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 df93636...9c355e4. Read the comment docs.

balloob · 2020-02-24T22:43:25Z

@frenck We need to make sure we update translations on the rc branch before release to pick up the changes from this PR.

MartinHjelmare · 2020-02-25T00:05:36Z

homeassistant/components/konnected/config_flow.py

+                description_placeholders={"id": self.unique_id},
+            )
+
+        # if we have ssdp discovered applicable host info use it


Do we really need this? If the user wants discovery help the user can use that approach instead of config yaml.

After receiving feedback on the beta build I think we do need to support this use case. The legacy konnected component automatically discovered host info and then applied the config.yaml.

The config.yaml can be extensive for this integration so we shouldn't require users to abandon what they have. This flow makes for a smooth upgrade path existing konnected users.

MartinHjelmare · 2020-02-25T00:14:38Z

homeassistant/components/konnected/config_flow.py

            if entry and (
                entry.data[CONF_HOST] != self.data[CONF_HOST]
                or entry.data[CONF_PORT] != self.data[CONF_PORT]
            ):
+                # update an existing config entry if host info changes
                entry_data = copy.deepcopy(entry.data)
                entry_data.update(self.data)
                self.hass.config_entries.async_update_entry(entry, data=entry_data)


We can now pass the updates directly to self._abort_if_unique_id_configured.

https://github.com/home-assistant/home-assistant/blob/b2d7bc40dc8c054430c6b8e0111be07060db5878/homeassistant/config_entries.py#L820-L833

cool - I'll swap over to this

MartinHjelmare · 2020-02-25T00:20:01Z

tests/components/konnected/test_panel.py

+        options={},
+    )
+    entry.add_to_hass(hass)
+    hass.data[panel.DOMAIN] = {


It's better to set up the integration using the mock entry than to mock integration code. Whenever we mock integration code in tests of the integration, the tests become less robust.

I understand. The panel module is well isolated from the rest of the integration and HA architecture - it just needs to parse and act upon passed in config entry data (which the integration also defines).

Bypassing the integration setup made it really clean to access, control, and assert behaviors based on the panel class methods. I think these tests might be an exception where directly mocking some workings of the integration shouldn't add maintenance overhead.

That we need to mock this is a sign that it's not isolated.

We don't really care about what the panel is doing. We just care about what the result in the core is upon different input that the panel forwards. It's better to set up the integration and assert core state.

Or if the info is flowing the other way, we assert that the library is called correctly.

ok - will fix

MartinHjelmare · 2020-02-25T03:00:40Z

tests/components/konnected/test_panel.py

+    }
+
+    # confirm the device settings are saved in hass.data
+    assert hass.data[panel.DOMAIN][panel.CONF_DEVICES] == {


Saving the data like this is just a detail which could change in the future. This isn't something we should assert.

Are you referring to asserting that konnected integration data resides in hass.data?

Got it - updating

Updated in latest commits. There is still one reference to hass.data in these tests to get the instance of the AlarmPanel created when the integration is setup. This is essential to confirm that the data store is formatted correctly - a check I want to keep to ensure that no future code changes in this module break that data structure since it is utilized when setting up the platforms.

kit-klein · 2020-02-25T04:46:46Z

The last commit prior to this comment (9c355e4) was a minor change to reflect a key change used to identify the soon to be released pro board. It has no impact on existing equipment.

MartinHjelmare

If we do these three types of changes, we'll no longer interact with the panel instance directly in the tests, making them more robust.

MartinHjelmare · 2020-02-25T09:46:21Z

tests/components/konnected/test_panel.py

+
+    # confirm panel instance was created and configured
+    # hass.data is the only mechanism to get a reference to the created panel instance
+    device = hass.data[panel.DOMAIN][panel.CONF_DEVICES]["112233445566"]["panel"]
    await device.update_switch("1", 0)


update_switch should be tested in konnected switch platform tests.

MartinHjelmare · 2020-02-25T09:49:08Z

tests/components/konnected/test_panel.py

-                    "state": None,
-                    "type": "door",
-                },
+    assert device.stored_configuration == {


Instead we should assert that mock_panel.put_settings is called with the correct configuration during device sync.

MartinHjelmare · 2020-02-25T09:57:05Z

tests/components/konnected/test_panel.py

@@ -92,9 +93,6 @@ def mock_constructor(host, port, websession):
        options=device_options,


Instead of creating the data and options first and using that to create a mock entry, just pass the configuration to async_setup_component. It will set up the integration and use the config yaml schemas of the integration before importing the config into a config entry.

kit-klein · 2020-02-25T12:26:57Z

If we do these three types of changes, we'll no longer interact with the panel instance directly in the tests, making them more robust.

If we had a single test_konnected.py module that tested the integration as a black box these suggestions would be the ideal strategy. In the current implementation they defeat the purpose of the attempted limited scope unit testing that is occurring here.

Directly accessing hass.data as part of a testing strategy is common throughout the codebase - including in the platinum integration examples like hue. It is a global data store but the content within each DOMAIN is defined/managed by the integration - ensuring robustness within the scope of the integration. If abstracting accessors existed for the data store it would be heinous to access it directly but as implemented it's not unreasonable to utilize it as a mechanism for testing.

Are these changes FYI/suggestions or requirements for PR approval? If they are required I will make them but I would like to understand why these items are blocking this PR but identical test code was approved without issue in the recent konnected PR. That would have been a much more ideal time to overhaul testing architecture vs now when attempting to fix beta release bugs.

MartinHjelmare · 2020-02-25T12:47:09Z

It's ok to improve the tests in a future PR. We shouldn't add any new tests that are not robust though.

As we said many times before, trying to get a huge PR like last PR merged is never a good idea since there will always be things that are missed. This is just confirmation.

kit-klein · 2020-02-25T14:33:36Z

@MartinHjelmare Understood. I greatly appreciate the high standards you and the team are implementing. Thank you for all the help.

* dedup config flows * use default (imported) options until user goes thru options flow * address pr feedback * correct key used to distinguish pro model

dedup config flows

64e7a45

homeassistant added the cla-signed label Feb 24, 2020

probot-home-assistant bot assigned balloob Feb 24, 2020

project-bot bot added this to Needs review in Dev Feb 24, 2020

probot-home-assistant bot added has-tests integration: konnected labels Feb 24, 2020

probot-home-assistant bot added the by-code-owner label Feb 24, 2020

project-bot bot moved this from Needs review to By Code Owner in Dev Feb 24, 2020

use default (imported) options until user goes thru options flow

89059f1

kit-klein mentioned this pull request Feb 24, 2020

Konnected Config Flow issues in 106 beta #32076

Closed

kit-klein commented Feb 24, 2020

View reviewed changes

homeassistant/components/konnected/.translations/en.json Show resolved Hide resolved

kit-klein commented Feb 24, 2020

View reviewed changes

homeassistant/components/konnected/panel.py Show resolved Hide resolved

springstan added this to the 0.106.0 milestone Feb 24, 2020

balloob requested a review from MartinHjelmare February 24, 2020 22:42

MartinHjelmare reviewed Feb 25, 2020

View reviewed changes

MartinHjelmare changed the title ~~0.106 Beta - dedup and clarify imported konnected config flows~~ Dedup and clarify imported konnected config flows Feb 25, 2020

MartinHjelmare reviewed Feb 25, 2020

View reviewed changes

kit-klein added 2 commits February 24, 2020 23:35

address pr feedback

d2b548c

correct key used to distinguish pro model

9c355e4

MartinHjelmare reviewed Feb 25, 2020

View reviewed changes

MartinHjelmare approved these changes Feb 25, 2020

View reviewed changes

Dev automation moved this from By Code Owner to Reviewer approved Feb 25, 2020

MartinHjelmare merged commit 5488389 into home-assistant:dev Feb 25, 2020

Dev automation moved this from Reviewer approved to Done Feb 25, 2020

balloob added the cherry-picked label Feb 25, 2020

balloob pushed a commit that referenced this pull request Feb 25, 2020

Dedup and clarify imported konnected config flows (#32138)

61e2ce5

* dedup config flows * use default (imported) options until user goes thru options flow * address pr feedback * correct key used to distinguish pro model

lock bot locked and limited conversation to collaborators Feb 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dedup and clarify imported konnected config flows #32138

Dedup and clarify imported konnected config flows #32138

kit-klein commented Feb 24, 2020 •

edited

Loading

probot-home-assistant bot commented Feb 24, 2020

probot-home-assistant bot commented Feb 24, 2020

heythisisnate commented Feb 24, 2020

codecov bot commented Feb 24, 2020 •

edited

Loading

balloob commented Feb 24, 2020

MartinHjelmare Feb 25, 2020

kit-klein Feb 25, 2020

MartinHjelmare Feb 25, 2020

MartinHjelmare Feb 25, 2020

kit-klein Feb 25, 2020

MartinHjelmare Feb 25, 2020

kit-klein Feb 25, 2020

MartinHjelmare Feb 25, 2020

MartinHjelmare Feb 25, 2020

kit-klein Feb 25, 2020

MartinHjelmare Feb 25, 2020

kit-klein Feb 25, 2020

MartinHjelmare Feb 25, 2020

kit-klein Feb 25, 2020

kit-klein Feb 25, 2020

kit-klein commented Feb 25, 2020

MartinHjelmare left a comment •

edited

Loading

MartinHjelmare Feb 25, 2020

MartinHjelmare Feb 25, 2020

MartinHjelmare Feb 25, 2020

kit-klein commented Feb 25, 2020

MartinHjelmare commented Feb 25, 2020

kit-klein commented Feb 25, 2020

		@@ -92,9 +93,6 @@ def mock_constructor(host, port, websession):
		options=device_options,

Dedup and clarify imported konnected config flows #32138

Dedup and clarify imported konnected config flows #32138

Conversation

kit-klein commented Feb 24, 2020 • edited Loading

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

probot-home-assistant bot commented Feb 24, 2020

probot-home-assistant bot commented Feb 24, 2020

heythisisnate commented Feb 24, 2020

codecov bot commented Feb 24, 2020 • edited Loading

Codecov Report

balloob commented Feb 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kit-klein commented Feb 25, 2020

MartinHjelmare left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kit-klein commented Feb 25, 2020

MartinHjelmare commented Feb 25, 2020

kit-klein commented Feb 25, 2020

kit-klein commented Feb 24, 2020 •

edited

Loading

Example entry for `configuration.yaml`:

codecov bot commented Feb 24, 2020 •

edited

Loading

MartinHjelmare left a comment •

edited

Loading