Improve Control4 connection error logging (#159979)

Co-authored-by: Joostlek <joostlek@outlook.com>

homeassistant/components/control4/config_flow.py

@@ -3,12 +3,12 @@
 from __future__ import annotations
 
 import logging
-from typing import TYPE_CHECKING, Any
+from typing import Any
 
 from aiohttp.client_exceptions import ClientError
 from pyControl4.account import C4Account
 from pyControl4.director import C4Director
-from pyControl4.error_handling import NotFound, Unauthorized
+from pyControl4.error_handling import BadCredentials, NotFound, Unauthorized
 import voluptuous as vol
 
 from homeassistant.config_entries import (
@@ -22,8 +22,7 @@ from homeassistant.const import (
     CONF_SCAN_INTERVAL,
     CONF_USERNAME,
 )
-from homeassistant.core import HomeAssistant, callback
-from homeassistant.exceptions import HomeAssistantError
+from homeassistant.core import callback
 from homeassistant.helpers import aiohttp_client, config_validation as cv
 from homeassistant.helpers.device_registry import format_mac
 
@@ -46,106 +45,107 @@ DATA_SCHEMA = vol.Schema(
 )
 
 
-class Control4Validator:
-    """Validates that config details can be used to authenticate and communicate with Control4."""
-
-    def __init__(
-        self, host: str, username: str, password: str, hass: HomeAssistant
-    ) -> None:
-        """Initialize."""
-        self.host = host
-        self.username = username
-        self.password = password
-        self.controller_unique_id = None
-        self.director_bearer_token = None
-        self.hass = hass
-
-    async def authenticate(self) -> bool:
-        """Test if we can authenticate with the Control4 account API."""
-        try:
-            account_session = aiohttp_client.async_get_clientsession(self.hass)
-            account = C4Account(self.username, self.password, account_session)
-            # Authenticate with Control4 account
-            await account.getAccountBearerToken()
-
-            # Get controller name
-            account_controllers = await account.getAccountControllers()
-            self.controller_unique_id = account_controllers["controllerCommonName"]
-
-            # Get bearer token to communicate with controller locally
-            self.director_bearer_token = (
-                await account.getDirectorBearerToken(self.controller_unique_id)
-            )["token"]
-        except (Unauthorized, NotFound):
-            return False
-        return True
-
-    async def connect_to_director(self) -> bool:
-        """Test if we can connect to the local Control4 Director."""
-        try:
-            director_session = aiohttp_client.async_get_clientsession(
-                self.hass, verify_ssl=False
-            )
-            director = C4Director(
-                self.host, self.director_bearer_token, director_session
-            )
-            await director.getAllItemInfo()
-        except (Unauthorized, ClientError, TimeoutError):
-            _LOGGER.error("Failed to connect to the Control4 controller")
-            return False
-        return True
-
-
 class Control4ConfigFlow(ConfigFlow, domain=DOMAIN):
     """Handle a config flow for Control4."""
 
     VERSION = 1
 
+    async def _async_try_connect(
+        self, user_input: dict[str, Any]
+    ) -> tuple[dict[str, str], dict[str, Any] | None, dict[str, str]]:
+        """Try to connect to Control4 and return errors, data, and placeholders."""
+        errors: dict[str, str] = {}
+        description_placeholders: dict[str, str] = {}
+        data: dict[str, Any] | None = None
+
+        host = user_input[CONF_HOST]
+        username = user_input[CONF_USERNAME]
+        password = user_input[CONF_PASSWORD]
+
+        # Step 1: Authenticate with Control4 cloud API
+        account_session = aiohttp_client.async_get_clientsession(self.hass)
+        account = C4Account(username, password, account_session)
+        try:
+            await account.getAccountBearerToken()
+
+            account_controllers = await account.getAccountControllers()
+            controller_unique_id = account_controllers["controllerCommonName"]
+
+            director_bearer_token = (
+                await account.getDirectorBearerToken(controller_unique_id)
+            )["token"]
+        except (BadCredentials, Unauthorized):
+            errors["base"] = "invalid_auth"
+            return errors, data, description_placeholders
+        except NotFound:
+            errors["base"] = "controller_not_found"
+            return errors, data, description_placeholders
+        except Exception:
+            _LOGGER.exception(
+                "Unexpected exception during Control4 account authentication"
+            )
+            errors["base"] = "unknown"
+            return errors, data, description_placeholders
+
+        # Step 2: Connect to local Control4 Director
+        director_session = aiohttp_client.async_get_clientsession(
+            self.hass, verify_ssl=False
+        )
+        director = C4Director(host, director_bearer_token, director_session)
+        try:
+            await director.getAllItemInfo()
+        except Unauthorized:
+            errors["base"] = "director_auth_failed"
+            return errors, data, description_placeholders
+        except (ClientError, TimeoutError):
+            errors["base"] = "cannot_connect"
+            description_placeholders["host"] = host
+            return errors, data, description_placeholders
+        except Exception:
+            _LOGGER.exception(
+                "Unexpected exception during Control4 director connection"
+            )
+            errors["base"] = "unknown"
+            return errors, data, description_placeholders
+
+        # Success - return the data needed for entry creation
+        data = {
+            CONF_HOST: host,
+            CONF_USERNAME: username,
+            CONF_PASSWORD: password,
+            CONF_CONTROLLER_UNIQUE_ID: controller_unique_id,
+        }
+
+        return errors, data, description_placeholders
+
     async def async_step_user(
         self, user_input: dict[str, Any] | None = None
     ) -> ConfigFlowResult:
         """Handle the initial step."""
-        errors = {}
-        if user_input is not None:
-            hub = Control4Validator(
-                user_input[CONF_HOST],
-                user_input[CONF_USERNAME],
-                user_input[CONF_PASSWORD],
-                self.hass,
-            )
-            try:
-                if not await hub.authenticate():
-                    raise InvalidAuth  # noqa: TRY301
-                if not await hub.connect_to_director():
-                    raise CannotConnect  # noqa: TRY301
-            except InvalidAuth:
-                errors["base"] = "invalid_auth"
-            except CannotConnect:
-                errors["base"] = "cannot_connect"
-            except Exception:
-                _LOGGER.exception("Unexpected exception")
-                errors["base"] = "unknown"
+        errors: dict[str, str] = {}
+        description_placeholders: dict[str, str] = {}
 
-            if not errors:
-                controller_unique_id = hub.controller_unique_id
-                if TYPE_CHECKING:
-                    assert hub.controller_unique_id
+        if user_input is not None:
+            errors, data, description_placeholders = await self._async_try_connect(
+                user_input
+            )
+
+            if not errors and data is not None:
+                controller_unique_id = data[CONF_CONTROLLER_UNIQUE_ID]
                 mac = (controller_unique_id.split("_", 3))[2]
                 formatted_mac = format_mac(mac)
                 await self.async_set_unique_id(formatted_mac)
                 self._abort_if_unique_id_configured()
                 return self.async_create_entry(
                     title=controller_unique_id,
-                    data={
-                        CONF_HOST: user_input[CONF_HOST],
-                        CONF_USERNAME: user_input[CONF_USERNAME],
-                        CONF_PASSWORD: user_input[CONF_PASSWORD],
-                        CONF_CONTROLLER_UNIQUE_ID: controller_unique_id,
-                    },
+                    data=data,
                 )
 
         return self.async_show_form(
-            step_id="user", data_schema=DATA_SCHEMA, errors=errors
+            step_id="user",
+            data_schema=DATA_SCHEMA,
+            errors=errors,
+            description_placeholders=description_placeholders,
         )
 
     @staticmethod
@@ -178,11 +178,3 @@ class OptionsFlowHandler(OptionsFlowWithReload):
             }
         )
         return self.async_show_form(step_id="init", data_schema=data_schema)
-
-
-class CannotConnect(HomeAssistantError):
-    """Error to indicate we cannot connect."""
-
-
-class InvalidAuth(HomeAssistantError):
-    """Error to indicate there is invalid auth."""

homeassistant/components/control4/strings.json

@@ -4,7 +4,9 @@
       "already_configured": "[%key:common::config_flow::abort::already_configured_device%]"
     },
     "error": {
-      "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]",
+      "cannot_connect": "Failed to connect to the Control4 director at {host}",
+      "controller_not_found": "No Control4 controller found on this account",
+      "director_auth_failed": "The Control4 director rejected the authentication token",
       "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]",
       "unknown": "[%key:common::config_flow::error::unknown%]"
     },

tests/components/control4/conftest.py

@@ -32,16 +32,35 @@ def mock_config_entry() -> MockConfigEntry:
     )
 
 
+@pytest.fixture
+def mock_setup_entry() -> Generator[AsyncMock]:
+    """Mock control4 setup entry."""
+    with patch(
+        "homeassistant.components.control4.async_setup_entry", return_value=True
+    ) as mock_setup:
+        yield mock_setup
+
+
 @pytest.fixture
 def mock_c4_account() -> Generator[MagicMock]:
     """Mock a Control4 Account client."""
-    with patch(
-        "homeassistant.components.control4.C4Account", autospec=True
-    ) as mock_account_class:
+    with (
+        patch(
+            "homeassistant.components.control4.C4Account", autospec=True
+        ) as mock_account_class,
+        patch(
+            "homeassistant.components.control4.config_flow.C4Account",
+            new=mock_account_class,
+        ),
+    ):
         mock_account = mock_account_class.return_value
         mock_account.getAccountBearerToken = AsyncMock()
         mock_account.getAccountControllers = AsyncMock(
-            return_value={"href": "https://example.com"}
+            return_value={
+                "controllerCommonName": "control4_model_00AA00AA00AA",
+                "href": "https://apis.control4.com/account/v3/rest/accounts/000000",
+                "name": "Name",
+            }
         )
         mock_account.getDirectorBearerToken = AsyncMock(return_value={"token": "test"})
         mock_account.getControllerOSVersion = AsyncMock(return_value="3.2.0")
@@ -51,9 +70,15 @@ def mock_c4_account() -> Generator[MagicMock]:
 @pytest.fixture
 def mock_c4_director() -> Generator[MagicMock]:
     """Mock a Control4 Director client."""
-    with patch(
-        "homeassistant.components.control4.C4Director", autospec=True
-    ) as mock_director_class:
+    with (
+        patch(
+            "homeassistant.components.control4.C4Director", autospec=True
+        ) as mock_director_class,
+        patch(
+            "homeassistant.components.control4.config_flow.C4Director",
+            new=mock_director_class,
+        ),
+    ):
         mock_director = mock_director_class.return_value
         # Multi-platform setup: media room, climate room, shared devices
         # Note: The API returns JSON strings, so we load fixtures as strings

tests/components/control4/test_config_flow.py

@@ -1,13 +1,13 @@
 """Test the Control4 config flow."""
 
-from unittest.mock import AsyncMock, patch
+from unittest.mock import AsyncMock
 
-from pyControl4.account import C4Account
-from pyControl4.director import C4Director
-from pyControl4.error_handling import Unauthorized
+from aiohttp.client_exceptions import ClientError
+from pyControl4.error_handling import BadCredentials, NotFound, Unauthorized
+import pytest
 
-from homeassistant import config_entries
 from homeassistant.components.control4.const import DEFAULT_SCAN_INTERVAL, DOMAIN
+from homeassistant.config_entries import SOURCE_USER
 from homeassistant.const import (
     CONF_HOST,
     CONF_PASSWORD,
@@ -22,155 +22,178 @@ from .conftest import MOCK_HOST, MOCK_PASSWORD, MOCK_USERNAME
 from tests.common import MockConfigEntry
 
 
-def _get_mock_c4_account():
-    c4_account_mock = AsyncMock(C4Account)
-
-    c4_account_mock.getAccountControllers.return_value = {
-        "controllerCommonName": "control4_model_00AA00AA00AA",
-        "href": "https://apis.control4.com/account/v3/rest/accounts/000000",
-        "name": "Name",
-    }
-
-    c4_account_mock.getDirectorBearerToken.return_value = {"token": "token"}
-
-    return c4_account_mock
-
-
-def _get_mock_c4_director():
-    c4_director_mock = AsyncMock(C4Director)
-    c4_director_mock.getAllItemInfo.return_value = {}
-
-    return c4_director_mock
-
-
-async def test_form(hass: HomeAssistant) -> None:
-    """Test we get the form."""
+async def test_full_flow(
+    hass: HomeAssistant,
+    mock_c4_account: AsyncMock,
+    mock_c4_director: AsyncMock,
+    mock_setup_entry: AsyncMock,
+) -> None:
+    """Test full config flow."""
 
     result = await hass.config_entries.flow.async_init(
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
+        DOMAIN, context={"source": SOURCE_USER}
     )
     assert result["type"] is FlowResultType.FORM
+    assert result["step_id"] == "user"
     assert result["errors"] == {}
 
-    c4_account = _get_mock_c4_account()
-    c4_director = _get_mock_c4_director()
-    with (
-        patch(
-            "homeassistant.components.control4.config_flow.C4Account",
-            return_value=c4_account,
-        ),
-        patch(
-            "homeassistant.components.control4.config_flow.C4Director",
-            return_value=c4_director,
-        ),
-        patch(
-            "homeassistant.components.control4.async_setup_entry",
-            return_value=True,
-        ) as mock_setup_entry,
-    ):
-        result2 = await hass.config_entries.flow.async_configure(
-            result["flow_id"],
-            {
-                CONF_HOST: MOCK_HOST,
-                CONF_USERNAME: MOCK_USERNAME,
-                CONF_PASSWORD: MOCK_PASSWORD,
-            },
-        )
-        await hass.async_block_till_done()
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"],
+        {
+            CONF_HOST: MOCK_HOST,
+            CONF_USERNAME: MOCK_USERNAME,
+            CONF_PASSWORD: MOCK_PASSWORD,
+        },
+    )
 
-    assert result2["type"] is FlowResultType.CREATE_ENTRY
-    assert result2["title"] == "control4_model_00AA00AA00AA"
-    assert result2["data"] == {
+    assert result["type"] is FlowResultType.CREATE_ENTRY
+    assert result["title"] == "control4_model_00AA00AA00AA"
+    assert result["data"] == {
         CONF_HOST: MOCK_HOST,
         CONF_USERNAME: MOCK_USERNAME,
         CONF_PASSWORD: MOCK_PASSWORD,
         "controller_unique_id": "control4_model_00AA00AA00AA",
     }
-    assert result2["result"].unique_id == "00:aa:00:aa:00:aa"
+    assert result["result"].unique_id == "00:aa:00:aa:00:aa"
     assert len(mock_setup_entry.mock_calls) == 1
 
 
-async def test_form_invalid_auth(hass: HomeAssistant) -> None:
-    """Test we handle invalid auth."""
+@pytest.mark.parametrize(
+    ("exception", "error"),
+    [
+        (BadCredentials("Invalid username or password"), "invalid_auth"),
+        (Unauthorized("Permission denied"), "invalid_auth"),
+        (NotFound("something"), "controller_not_found"),
+        (Exception("Some other exception"), "unknown"),
+    ],
+)
+@pytest.mark.usefixtures("mock_setup_entry")
+async def test_user_flow_errors(
+    hass: HomeAssistant,
+    mock_c4_account: AsyncMock,
+    mock_c4_director: AsyncMock,
+    exception: Exception,
+    error: str,
+) -> None:
+    """Test we handle errors in the user flow."""
     result = await hass.config_entries.flow.async_init(
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
+        DOMAIN, context={"source": SOURCE_USER}
     )
 
-    with patch(
-        "homeassistant.components.control4.config_flow.C4Account",
-        side_effect=Unauthorized("message"),
-    ):
-        result2 = await hass.config_entries.flow.async_configure(
-            result["flow_id"],
-            {
-                CONF_HOST: MOCK_HOST,
-                CONF_USERNAME: MOCK_USERNAME,
-                CONF_PASSWORD: MOCK_PASSWORD,
-            },
-        )
+    mock_c4_account.getAccountBearerToken.side_effect = exception
 
-    assert result2["type"] is FlowResultType.FORM
-    assert result2["errors"] == {"base": "invalid_auth"}
-
-
-async def test_form_unexpected_exception(hass: HomeAssistant) -> None:
-    """Test we handle an unexpected exception."""
-    result = await hass.config_entries.flow.async_init(
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"],
+        {
+            CONF_HOST: MOCK_HOST,
+            CONF_USERNAME: MOCK_USERNAME,
+            CONF_PASSWORD: MOCK_PASSWORD,
+        },
     )
 
-    with patch(
-        "homeassistant.components.control4.config_flow.C4Account",
-        side_effect=ValueError("message"),
-    ):
-        result2 = await hass.config_entries.flow.async_configure(
-            result["flow_id"],
-            {
-                CONF_HOST: MOCK_HOST,
-                CONF_USERNAME: MOCK_USERNAME,
-                CONF_PASSWORD: MOCK_PASSWORD,
-            },
-        )
+    assert result["type"] is FlowResultType.FORM
+    assert result["errors"] == {"base": error}
 
-    assert result2["type"] is FlowResultType.FORM
-    assert result2["errors"] == {"base": "unknown"}
+    mock_c4_account.getAccountBearerToken.side_effect = None
 
-
-async def test_form_cannot_connect(hass: HomeAssistant) -> None:
-    """Test we handle cannot connect error."""
-    result = await hass.config_entries.flow.async_init(
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"],
+        {
+            CONF_HOST: MOCK_HOST,
+            CONF_USERNAME: MOCK_USERNAME,
+            CONF_PASSWORD: MOCK_PASSWORD,
+        },
     )
 
-    with (
-        patch(
-            "homeassistant.components.control4.config_flow.Control4Validator.authenticate",
-            return_value=True,
-        ),
-        patch(
-            "homeassistant.components.control4.config_flow.C4Director",
-            side_effect=Unauthorized("message"),
-        ),
-    ):
-        result2 = await hass.config_entries.flow.async_configure(
-            result["flow_id"],
-            {
-                CONF_HOST: MOCK_HOST,
-                CONF_USERNAME: MOCK_USERNAME,
-                CONF_PASSWORD: MOCK_PASSWORD,
-            },
-        )
-
-    assert result2["type"] is FlowResultType.FORM
-    assert result2["errors"] == {"base": "cannot_connect"}
+    assert result["type"] is FlowResultType.CREATE_ENTRY
 
 
-async def test_option_flow(hass: HomeAssistant) -> None:
+@pytest.mark.parametrize(
+    ("exception", "error"),
+    [
+        (Unauthorized("Permission denied"), "director_auth_failed"),
+        (ClientError, "cannot_connect"),
+        (TimeoutError, "cannot_connect"),
+        (Exception("Some other exception"), "unknown"),
+    ],
+)
+@pytest.mark.usefixtures("mock_setup_entry")
+async def test_user_flow_director_errors(
+    hass: HomeAssistant,
+    mock_c4_account: AsyncMock,
+    mock_c4_director: AsyncMock,
+    exception: Exception,
+    error: str,
+) -> None:
+    """Test we handle director auth failure."""
+    result = await hass.config_entries.flow.async_init(
+        DOMAIN, context={"source": SOURCE_USER}
+    )
+
+    mock_c4_director.getAllItemInfo.side_effect = exception
+
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"],
+        {
+            CONF_HOST: MOCK_HOST,
+            CONF_USERNAME: MOCK_USERNAME,
+            CONF_PASSWORD: MOCK_PASSWORD,
+        },
+    )
+
+    assert result["type"] is FlowResultType.FORM
+    assert result["errors"] == {"base": error}
+
+    mock_c4_director.getAllItemInfo.side_effect = None
+
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"],
+        {
+            CONF_HOST: MOCK_HOST,
+            CONF_USERNAME: MOCK_USERNAME,
+            CONF_PASSWORD: MOCK_PASSWORD,
+        },
+    )
+
+    assert result["type"] is FlowResultType.CREATE_ENTRY
+
+
+async def test_duplicate_entry(
+    hass: HomeAssistant,
+    mock_c4_account: AsyncMock,
+    mock_c4_director: AsyncMock,
+    mock_config_entry: MockConfigEntry,
+) -> None:
+    """Test that duplicate entries are not created."""
+    mock_config_entry.add_to_hass(hass)
+
+    result = await hass.config_entries.flow.async_init(
+        DOMAIN, context={"source": SOURCE_USER}
+    )
+    assert result["type"] is FlowResultType.FORM
+    assert result["step_id"] == "user"
+    assert result["errors"] == {}
+
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"],
+        {
+            CONF_HOST: MOCK_HOST,
+            CONF_USERNAME: MOCK_USERNAME,
+            CONF_PASSWORD: MOCK_PASSWORD,
+        },
+    )
+
+    assert result["type"] is FlowResultType.ABORT
+    assert result["reason"] == "already_configured"
+
+
+async def test_option_flow(
+    hass: HomeAssistant, mock_config_entry: MockConfigEntry
+) -> None:
     """Test config flow options."""
-    entry = MockConfigEntry(domain=DOMAIN, data={}, options=None)
-    entry.add_to_hass(hass)
+    mock_config_entry.add_to_hass(hass)
 
-    result = await hass.config_entries.options.async_init(entry.entry_id)
+    result = await hass.config_entries.options.async_init(mock_config_entry.entry_id)
 
     assert result["type"] is FlowResultType.FORM
     assert result["step_id"] == "init"
@@ -185,12 +208,13 @@ async def test_option_flow(hass: HomeAssistant) -> None:
     }
 
 
-async def test_option_flow_defaults(hass: HomeAssistant) -> None:
+async def test_option_flow_defaults(
+    hass: HomeAssistant, mock_config_entry: MockConfigEntry
+) -> None:
     """Test config flow options."""
-    entry = MockConfigEntry(domain=DOMAIN, data={}, options=None)
-    entry.add_to_hass(hass)
+    mock_config_entry.add_to_hass(hass)
 
-    result = await hass.config_entries.options.async_init(entry.entry_id)
+    result = await hass.config_entries.options.async_init(mock_config_entry.entry_id)
 
     assert result["type"] is FlowResultType.FORM
     assert result["step_id"] == "init"