Address review of Nanoleaf Config Flow (#55215)

homeassistant/components/nanoleaf/config_flow.py

@@ -9,7 +9,6 @@ from pynanoleaf import InvalidToken, Nanoleaf, NotAuthorizingNewTokens, Unavaila
 import voluptuous as vol
 
 from homeassistant import config_entries
-from homeassistant.components import persistent_notification
 from homeassistant.const import CONF_HOST, CONF_TOKEN
 from homeassistant.data_entry_flow import FlowResult
 from homeassistant.helpers.typing import DiscoveryInfoType
@@ -162,9 +161,7 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
             return self.async_abort(reason="invalid_token")
         except Exception:  # pylint: disable=broad-except
             _LOGGER.exception(
-                "Unknown error connecting with Nanoleaf at %s with token %s",
+                "Unknown error connecting with Nanoleaf at %s", self.nanoleaf.host
-                self.nanoleaf.host,
-                self.nanoleaf.token,
             )
             return self.async_abort(reason="unknown")
         name = info["name"]
@@ -189,11 +186,6 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
                 await self.hass.async_add_executor_job(
                     os.remove, self.hass.config.path(CONFIG_FILE)
                 )
-                persistent_notification.async_create(
-                    self.hass,
-                    "All Nanoleaf devices from the discovery integration are imported. If you used the discovery integration only for Nanoleaf you can remove it from your configuration.yaml",
-                    f"Imported Nanoleaf {name}",
-                )
 
         return self.async_create_entry(
             title=name,

homeassistant/components/nanoleaf/light.py

@@ -59,23 +59,25 @@ PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend(
 async def async_setup_platform(
     hass: HomeAssistant,
     config: ConfigType,
-    add_entities: AddEntitiesCallback,
+    async_add_entities: AddEntitiesCallback,
     discovery_info: DiscoveryInfoType | None = None,
 ) -> None:
     """Import Nanoleaf light platform."""
-    await hass.config_entries.flow.async_init(
+    hass.async_create_task(
-        DOMAIN,
+        hass.config_entries.flow.async_init(
-        context={"source": SOURCE_IMPORT},
+            DOMAIN,
-        data={CONF_HOST: config[CONF_HOST], CONF_TOKEN: config[CONF_TOKEN]},
+            context={"source": SOURCE_IMPORT},
+            data={CONF_HOST: config[CONF_HOST], CONF_TOKEN: config[CONF_TOKEN]},
+        )
     )
 
 
 async def async_setup_entry(
-    hass: HomeAssistant, entry: ConfigEntry, add_entities: AddEntitiesCallback
+    hass: HomeAssistant, entry: ConfigEntry, async_add_entities: AddEntitiesCallback
 ) -> None:
     """Set up the Nanoleaf light."""
     data = hass.data[DOMAIN][entry.entry_id]
-    add_entities([NanoleafLight(data[DEVICE], data[NAME], data[SERIAL_NO])], True)
+    async_add_entities([NanoleafLight(data[DEVICE], data[NAME], data[SERIAL_NO])], True)
 
 
 class NanoleafLight(LightEntity):

tests/components/nanoleaf/test_config_flow.py

@@ -1,7 +1,11 @@
 """Test the Nanoleaf config flow."""
+from __future__ import annotations
+
 from unittest.mock import patch
 
 from pynanoleaf import InvalidToken, NotAuthorizingNewTokens, Unavailable
+from pynanoleaf.pynanoleaf import NanoleafError
+import pytest
 
 from homeassistant import config_entries
 from homeassistant.components.nanoleaf.const import DOMAIN
@@ -10,14 +14,15 @@ from homeassistant.core import HomeAssistant
 
 TEST_NAME = "Canvas ADF9"
 TEST_HOST = "192.168.0.100"
+TEST_OTHER_HOST = "192.168.0.200"
 TEST_TOKEN = "R34F1c92FNv3pcZs4di17RxGqiLSwHM"
 TEST_OTHER_TOKEN = "Qs4dxGcHR34l29RF1c92FgiLQBt3pcM"
 TEST_DEVICE_ID = "5E:2E:EA:XX:XX:XX"
 TEST_OTHER_DEVICE_ID = "5E:2E:EA:YY:YY:YY"
 
 
-async def test_user_unavailable_user_step(hass: HomeAssistant) -> None:
+async def test_user_unavailable_user_step_link_step(hass: HomeAssistant) -> None:
-    """Test we handle Unavailable errors when host is not available in user step."""
+    """Test we handle Unavailable in user and link step."""
     result = await hass.config_entries.flow.async_init(
         DOMAIN, context={"source": config_entries.SOURCE_USER}
     )
@@ -36,12 +41,6 @@ async def test_user_unavailable_user_step(hass: HomeAssistant) -> None:
     assert result2["errors"] == {"base": "cannot_connect"}
     assert not result2["last_step"]
 
-
-async def test_user_unavailable_link_step(hass: HomeAssistant) -> None:
-    """Test we abort if the device becomes unavailable in the link step."""
-    result = await hass.config_entries.flow.async_init(
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
-    )
     with patch(
         "homeassistant.components.nanoleaf.config_flow.Nanoleaf.authorize",
         return_value=None,
@@ -67,8 +66,18 @@ async def test_user_unavailable_link_step(hass: HomeAssistant) -> None:
     assert result3["reason"] == "cannot_connect"
 
 
-async def test_user_unavailable_setup_finish(hass: HomeAssistant) -> None:
+@pytest.mark.parametrize(
-    """Test we abort if the device becomes unavailable during setup_finish."""
+    "error, reason",
+    [
+        (Unavailable("message"), "cannot_connect"),
+        (InvalidToken("message"), "invalid_token"),
+        (Exception, "unknown"),
+    ],
+)
+async def test_user_error_setup_finish(
+    hass: HomeAssistant, error: Exception, reason: str
+) -> None:
+    """Test abort flow if on error in setup_finish."""
     result = await hass.config_entries.flow.async_init(
         DOMAIN, context={"source": config_entries.SOURCE_USER}
     )
@@ -90,62 +99,84 @@ async def test_user_unavailable_setup_finish(hass: HomeAssistant) -> None:
         return_value=None,
     ), patch(
         "homeassistant.components.nanoleaf.config_flow.pynanoleaf_get_info",
-        side_effect=Unavailable("message"),
+        side_effect=error,
     ):
         result3 = await hass.config_entries.flow.async_configure(
             result["flow_id"],
             {},
         )
     assert result3["type"] == "abort"
-    assert result3["reason"] == "cannot_connect"
+    assert result3["reason"] == reason
 
 
-async def test_user_not_authorizing_new_tokens(hass: HomeAssistant) -> None:
+async def test_user_not_authorizing_new_tokens_user_step_link_step(
-    """Test we handle NotAuthorizingNewTokens errors."""
+    hass: HomeAssistant,
-    result = await hass.config_entries.flow.async_init(
+) -> None:
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
+    """Test we handle NotAuthorizingNewTokens in user step and link step."""
-    )
-    assert result["type"] == "form"
-    assert result["errors"] is None
-    assert not result["last_step"]
-    assert result["step_id"] == "user"
-
     with patch(
-        "homeassistant.components.nanoleaf.config_flow.Nanoleaf.authorize",
+        "homeassistant.components.nanoleaf.config_flow.Nanoleaf",
-        side_effect=NotAuthorizingNewTokens("message"),
+    ) as mock_nanoleaf, patch(
-    ):
+        "homeassistant.components.nanoleaf.config_flow.pynanoleaf_get_info",
+        return_value={"name": TEST_NAME},
+    ), patch(
+        "homeassistant.components.nanoleaf.async_setup_entry", return_value=True
+    ) as mock_setup_entry:
+        nanoleaf = mock_nanoleaf.return_value
+        nanoleaf.authorize.side_effect = NotAuthorizingNewTokens(
+            "Not authorizing new tokens"
+        )
+        nanoleaf.host = TEST_HOST
+        nanoleaf.token = TEST_TOKEN
+
+        result = await hass.config_entries.flow.async_init(
+            DOMAIN, context={"source": config_entries.SOURCE_USER}
+        )
+        assert result["type"] == "form"
+        assert result["errors"] is None
+        assert result["step_id"] == "user"
+        assert not result["last_step"]
+
         result2 = await hass.config_entries.flow.async_configure(
             result["flow_id"],
             {
                 CONF_HOST: TEST_HOST,
             },
         )
-    assert result2["type"] == "form"
+        assert result2["type"] == "form"
-    assert result2["errors"] is None
+        assert result2["errors"] is None
-    assert result2["step_id"] == "link"
+        assert result2["step_id"] == "link"
 
-    result3 = await hass.config_entries.flow.async_configure(
+        result3 = await hass.config_entries.flow.async_configure(
-        result["flow_id"],
+            result["flow_id"],
-    )
+        )
-    assert result3["type"] == "form"
+        assert result3["type"] == "form"
-    assert result3["errors"] is None
+        assert result3["errors"] is None
-    assert result3["step_id"] == "link"
+        assert result3["step_id"] == "link"
 
-    with patch(
+        result4 = await hass.config_entries.flow.async_configure(result["flow_id"], {})
-        "homeassistant.components.nanoleaf.config_flow.Nanoleaf.authorize",
+        assert result4["type"] == "form"
-        side_effect=NotAuthorizingNewTokens("message"),
+        assert result4["errors"] == {"base": "not_allowing_new_tokens"}
-    ):
+        assert result4["step_id"] == "link"
-        result4 = await hass.config_entries.flow.async_configure(
+
+        nanoleaf.authorize.side_effect = None
+        nanoleaf.authorize.return_value = None
+
+        result5 = await hass.config_entries.flow.async_configure(
             result["flow_id"],
             {},
         )
-    assert result4["type"] == "form"
+        assert result5["type"] == "create_entry"
-    assert result4["step_id"] == "link"
+        assert result5["title"] == TEST_NAME
-    assert result4["errors"] == {"base": "not_allowing_new_tokens"}
+        assert result5["data"] == {
+            CONF_HOST: TEST_HOST,
+            CONF_TOKEN: TEST_TOKEN,
+        }
+        await hass.async_block_till_done()
+        assert len(mock_setup_entry.mock_calls) == 1
 
 
-async def test_user_exception(hass: HomeAssistant) -> None:
+async def test_user_exception_user_step(hass: HomeAssistant) -> None:
-    """Test we handle Exception errors."""
+    """Test we handle Exception errors in user step."""
     result = await hass.config_entries.flow.async_init(
         DOMAIN, context={"source": config_entries.SOURCE_USER}
     )
@@ -203,35 +234,18 @@ async def test_user_exception(hass: HomeAssistant) -> None:
     assert result5["reason"] == "unknown"
 
 
-async def test_zeroconf_discovery(hass: HomeAssistant) -> None:
+@pytest.mark.parametrize(
-    """Test zeroconfig discovery flow init."""
+    "source, type_in_discovery_info",
-    zeroconf = "_nanoleafms._tcp.local"
+    [
-    with patch(
+        (config_entries.SOURCE_HOMEKIT, "_hap._tcp.local"),
-        "homeassistant.components.nanoleaf.config_flow.pynanoleaf_get_info",
+        (config_entries.SOURCE_ZEROCONF, "_nanoleafms._tcp.local"),
-        return_value={"name": TEST_NAME},
+        (config_entries.SOURCE_ZEROCONF, "_nanoleafapi._tcp.local."),
-    ), patch(
+    ],
-        "homeassistant.components.nanoleaf.config_flow.load_json",
+)
-        return_value={},
+async def test_discovery_link_unavailable(
-    ):
+    hass: HomeAssistant, source: type, type_in_discovery_info: str
-        result = await hass.config_entries.flow.async_init(
-            DOMAIN,
-            context={"source": config_entries.SOURCE_ZEROCONF},
-            data={
-                "host": TEST_HOST,
-                "name": f"{TEST_NAME}.{zeroconf}",
-                "type": zeroconf,
-                "properties": {"id": TEST_DEVICE_ID},
-            },
-        )
-    assert result["type"] == "form"
-    assert result["step_id"] == "link"
-
-
-async def test_homekit_discovery_link_unavailable(
-    hass: HomeAssistant,
 ) -> None:
-    """Test homekit discovery and abort if device is unavailable."""
+    """Test discovery and abort if device is unavailable."""
-    homekit = "_hap._tcp.local"
     with patch(
         "homeassistant.components.nanoleaf.config_flow.pynanoleaf_get_info",
         return_value={"name": TEST_NAME},
@@ -241,11 +255,11 @@ async def test_homekit_discovery_link_unavailable(
     ):
         result = await hass.config_entries.flow.async_init(
             DOMAIN,
-            context={"source": config_entries.SOURCE_HOMEKIT},
+            context={"source": source},
             data={
                 "host": TEST_HOST,
-                "name": f"{TEST_NAME}.{homekit}",
+                "name": f"{TEST_NAME}.{type_in_discovery_info}",
-                "type": homekit,
+                "type": type_in_discovery_info,
                 "properties": {"id": TEST_DEVICE_ID},
             },
         )
@@ -293,11 +307,21 @@ async def test_import_config(hass: HomeAssistant) -> None:
     assert len(mock_setup_entry.mock_calls) == 1
 
 
-async def test_import_config_invalid_token(hass: HomeAssistant) -> None:
+@pytest.mark.parametrize(
-    """Test configuration import with invalid token."""
+    "error, reason",
+    [
+        (Unavailable("message"), "cannot_connect"),
+        (InvalidToken("message"), "invalid_token"),
+        (Exception, "unknown"),
+    ],
+)
+async def test_import_config_error(
+    hass: HomeAssistant, error: NanoleafError, reason: str
+) -> None:
+    """Test configuration import with errors in setup_finish."""
     with patch(
         "homeassistant.components.nanoleaf.config_flow.pynanoleaf_get_info",
-        side_effect=InvalidToken("message"),
+        side_effect=error,
     ):
         result = await hass.config_entries.flow.async_init(
             DOMAIN,
@@ -305,25 +329,70 @@ async def test_import_config_invalid_token(hass: HomeAssistant) -> None:
             data={CONF_HOST: TEST_HOST, CONF_TOKEN: TEST_TOKEN},
         )
     assert result["type"] == "abort"
-    assert result["reason"] == "invalid_token"
+    assert result["reason"] == reason
 
 
-async def test_import_last_discovery_integration_host_zeroconf(
+@pytest.mark.parametrize(
+    "source, type_in_discovery",
+    [
+        (config_entries.SOURCE_HOMEKIT, "_hap._tcp.local"),
+        (config_entries.SOURCE_ZEROCONF, "_nanoleafms._tcp.local"),
+        (config_entries.SOURCE_ZEROCONF, "_nanoleafapi._tcp.local"),
+    ],
+)
+@pytest.mark.parametrize(
+    "nanoleaf_conf_file, remove_config",
+    [
+        ({TEST_DEVICE_ID: {"token": TEST_TOKEN}}, True),
+        ({TEST_HOST: {"token": TEST_TOKEN}}, True),
+        (
+            {
+                TEST_DEVICE_ID: {"token": TEST_TOKEN},
+                TEST_HOST: {"token": TEST_OTHER_TOKEN},
+            },
+            True,
+        ),
+        (
+            {
+                TEST_DEVICE_ID: {"token": TEST_TOKEN},
+                TEST_OTHER_HOST: {"token": TEST_OTHER_TOKEN},
+            },
+            False,
+        ),
+        (
+            {
+                TEST_OTHER_DEVICE_ID: {"token": TEST_OTHER_TOKEN},
+                TEST_HOST: {"token": TEST_TOKEN},
+            },
+            False,
+        ),
+    ],
+)
+async def test_import_discovery_integration(
     hass: HomeAssistant,
+    source: str,
+    type_in_discovery: str,
+    nanoleaf_conf_file: dict[str, dict[str, str]],
+    remove_config: bool,
 ) -> None:
     """
-    Test discovery integration import from < 2021.4 (host) with zeroconf.
+    Test discovery integration import.
 
-    Device is last in Nanoleaf config file.
+    Test with different discovery flow sources and corresponding types.
+    Test with different .nanoleaf_conf files with device_id (>= 2021.4), host (< 2021.4) and combination.
+    Test removing the .nanoleaf_conf file if it was the only device in the file.
+    Test updating the .nanoleaf_conf file if it was not the only device in the file.
     """
-    zeroconf = "_nanoleafapi._tcp.local"
     with patch(
         "homeassistant.components.nanoleaf.config_flow.load_json",
-        return_value={TEST_HOST: {"token": TEST_TOKEN}},
+        return_value=dict(nanoleaf_conf_file),
     ), patch(
         "homeassistant.components.nanoleaf.config_flow.pynanoleaf_get_info",
         return_value={"name": TEST_NAME},
     ), patch(
+        "homeassistant.components.nanoleaf.config_flow.save_json",
+        return_value=None,
+    ) as mock_save_json, patch(
         "homeassistant.components.nanoleaf.config_flow.os.remove",
         return_value=None,
     ) as mock_remove, patch(
@@ -332,68 +401,27 @@ async def test_import_last_discovery_integration_host_zeroconf(
     ) as mock_setup_entry:
         result = await hass.config_entries.flow.async_init(
             DOMAIN,
-            context={"source": config_entries.SOURCE_ZEROCONF},
+            context={"source": source},
             data={
                 "host": TEST_HOST,
-                "name": f"{TEST_NAME}.{zeroconf}",
+                "name": f"{TEST_NAME}.{type_in_discovery}",
-                "type": zeroconf,
+                "type": type_in_discovery,
                 "properties": {"id": TEST_DEVICE_ID},
             },
         )
-
     assert result["type"] == "create_entry"
     assert result["title"] == TEST_NAME
     assert result["data"] == {
         CONF_HOST: TEST_HOST,
         CONF_TOKEN: TEST_TOKEN,
     }
-    mock_remove.assert_called_once()
+
-    await hass.async_block_till_done()
+    if remove_config:
-    assert len(mock_setup_entry.mock_calls) == 1
+        mock_save_json.assert_not_called()
-
+        mock_remove.assert_called_once()
-
+    else:
-async def test_import_not_last_discovery_integration_device_id_homekit(
+        mock_save_json.assert_called_once()
-    hass: HomeAssistant,
+        mock_remove.assert_not_called()
-) -> None:
+
-    """
-    Test discovery integration import from >= 2021.4 (device_id) with homekit.
-
-    Device is not the only one in the Nanoleaf config file.
-    """
-    homekit = "_hap._tcp.local"
-    with patch(
-        "homeassistant.components.nanoleaf.config_flow.load_json",
-        return_value={
-            TEST_DEVICE_ID: {"token": TEST_TOKEN},
-            TEST_OTHER_DEVICE_ID: {"token": TEST_OTHER_TOKEN},
-        },
-    ), patch(
-        "homeassistant.components.nanoleaf.config_flow.pynanoleaf_get_info",
-        return_value={"name": TEST_NAME},
-    ), patch(
-        "homeassistant.components.nanoleaf.config_flow.save_json",
-        return_value=None,
-    ) as mock_save_json, patch(
-        "homeassistant.components.nanoleaf.async_setup_entry",
-        return_value=True,
-    ) as mock_setup_entry:
-        result = await hass.config_entries.flow.async_init(
-            DOMAIN,
-            context={"source": config_entries.SOURCE_HOMEKIT},
-            data={
-                "host": TEST_HOST,
-                "name": f"{TEST_NAME}.{homekit}",
-                "type": homekit,
-                "properties": {"id": TEST_DEVICE_ID},
-            },
-        )
-
-    assert result["type"] == "create_entry"
-    assert result["title"] == TEST_NAME
-    assert result["data"] == {
-        CONF_HOST: TEST_HOST,
-        CONF_TOKEN: TEST_TOKEN,
-    }
-    mock_save_json.assert_called_once()
     await hass.async_block_till_done()
     assert len(mock_setup_entry.mock_calls) == 1