From ef90fe9aee592aedd76993194b982a9a96fb2680 Mon Sep 17 00:00:00 2001 From: Kevin Stillhammer Date: Sun, 16 Oct 2022 19:06:53 +0200 Subject: [PATCH] Display and log google_travel_time errors (#77604) * Display and log google_travel_time errors * Rename is_valid_config_entry to validate_config_entry Signed-off-by: Kevin Stillhammer Signed-off-by: Kevin Stillhammer --- .../google_travel_time/config_flow.py | 33 +++++---- .../components/google_travel_time/helpers.py | 46 ++++++++++--- .../google_travel_time/strings.json | 1 + .../google_travel_time/translations/en.json | 3 +- .../components/google_travel_time/conftest.py | 20 +++++- .../google_travel_time/test_config_flow.py | 67 +++++++++++++++++++ 6 files changed, 142 insertions(+), 28 deletions(-) diff --git a/homeassistant/components/google_travel_time/config_flow.py b/homeassistant/components/google_travel_time/config_flow.py index f2e23b02cc0..3a4c576f218 100644 --- a/homeassistant/components/google_travel_time/config_flow.py +++ b/homeassistant/components/google_travel_time/config_flow.py @@ -1,13 +1,12 @@ """Config flow for Google Maps Travel Time integration.""" from __future__ import annotations -import logging - import voluptuous as vol from homeassistant import config_entries from homeassistant.const import CONF_API_KEY, CONF_MODE, CONF_NAME from homeassistant.core import callback +from homeassistant.data_entry_flow import FlowResult import homeassistant.helpers.config_validation as cv from .const import ( @@ -36,9 +35,7 @@ from .const import ( TRAVEL_MODEL, UNITS, ) -from .helpers import is_valid_config_entry - -_LOGGER = logging.getLogger(__name__) +from .helpers import InvalidApiKeyException, UnknownException, validate_config_entry class GoogleOptionsFlow(config_entries.OptionsFlow): @@ -48,7 +45,7 @@ class GoogleOptionsFlow(config_entries.OptionsFlow): """Initialize google options flow.""" self.config_entry = config_entry - async def async_step_init(self, user_input=None): + async def async_step_init(self, user_input=None) -> FlowResult: """Handle the initial step.""" if user_input is not None: time_type = user_input.pop(CONF_TIME_TYPE) @@ -122,25 +119,27 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): """Get the options flow for this handler.""" return GoogleOptionsFlow(config_entry) - async def async_step_user(self, user_input=None): + async def async_step_user(self, user_input=None) -> FlowResult: """Handle the initial step.""" errors = {} user_input = user_input or {} if user_input: - if await self.hass.async_add_executor_job( - is_valid_config_entry, - self.hass, - user_input[CONF_API_KEY], - user_input[CONF_ORIGIN], - user_input[CONF_DESTINATION], - ): + try: + await self.hass.async_add_executor_job( + validate_config_entry, + self.hass, + user_input[CONF_API_KEY], + user_input[CONF_ORIGIN], + user_input[CONF_DESTINATION], + ) return self.async_create_entry( title=user_input.get(CONF_NAME, DEFAULT_NAME), data=user_input, ) - - # If we get here, it's because we couldn't connect - errors["base"] = "cannot_connect" + except InvalidApiKeyException: + errors["base"] = "invalid_auth" + except UnknownException: + errors["base"] = "cannot_connect" return self.async_show_form( step_id="user", diff --git a/homeassistant/components/google_travel_time/helpers.py b/homeassistant/components/google_travel_time/helpers.py index dc8fc32af7a..12394a23209 100644 --- a/homeassistant/components/google_travel_time/helpers.py +++ b/homeassistant/components/google_travel_time/helpers.py @@ -1,18 +1,46 @@ """Helpers for Google Time Travel integration.""" +import logging + from googlemaps import Client from googlemaps.distance_matrix import distance_matrix -from googlemaps.exceptions import ApiError +from googlemaps.exceptions import ApiError, Timeout, TransportError +from homeassistant.core import HomeAssistant from homeassistant.helpers.location import find_coordinates +_LOGGER = logging.getLogger(__name__) -def is_valid_config_entry(hass, api_key, origin, destination): + +def validate_config_entry( + hass: HomeAssistant, api_key: str, origin: str, destination: str +) -> None: """Return whether the config entry data is valid.""" - origin = find_coordinates(hass, origin) - destination = find_coordinates(hass, destination) - client = Client(api_key, timeout=10) + resolved_origin = find_coordinates(hass, origin) + resolved_destination = find_coordinates(hass, destination) try: - distance_matrix(client, origin, destination, mode="driving") - except ApiError: - return False - return True + client = Client(api_key, timeout=10) + except ValueError as value_error: + _LOGGER.error("Malformed API key") + raise InvalidApiKeyException from value_error + try: + distance_matrix(client, resolved_origin, resolved_destination, mode="driving") + except ApiError as api_error: + if api_error.status == "REQUEST_DENIED": + _LOGGER.error("Request denied: %s", api_error.message) + raise InvalidApiKeyException from api_error + _LOGGER.error("Unknown error: %s", api_error.message) + raise UnknownException() from api_error + except TransportError as transport_error: + _LOGGER.error("Unknown error: %s", transport_error) + raise UnknownException() from transport_error + except Timeout as timeout_error: + _LOGGER.error("Timeout error") + raise UnknownException() from timeout_error + + +class InvalidApiKeyException(Exception): + """Invalid API Key Error.""" + + +class UnknownException(Exception): + """Unknown API Error.""" diff --git a/homeassistant/components/google_travel_time/strings.json b/homeassistant/components/google_travel_time/strings.json index e0043a4b342..22a122b9a53 100644 --- a/homeassistant/components/google_travel_time/strings.json +++ b/homeassistant/components/google_travel_time/strings.json @@ -13,6 +13,7 @@ } }, "error": { + "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]", "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]" }, "abort": { diff --git a/homeassistant/components/google_travel_time/translations/en.json b/homeassistant/components/google_travel_time/translations/en.json index b0e08c1d63d..8e91fbf1df0 100644 --- a/homeassistant/components/google_travel_time/translations/en.json +++ b/homeassistant/components/google_travel_time/translations/en.json @@ -4,7 +4,8 @@ "already_configured": "Location is already configured" }, "error": { - "cannot_connect": "Failed to connect" + "cannot_connect": "Failed to connect", + "invalid_auth": "Invalid authentication" }, "step": { "user": { diff --git a/tests/components/google_travel_time/conftest.py b/tests/components/google_travel_time/conftest.py index 4ca7c5a9105..ec5a8f16917 100644 --- a/tests/components/google_travel_time/conftest.py +++ b/tests/components/google_travel_time/conftest.py @@ -1,7 +1,7 @@ """Fixtures for Google Time Travel tests.""" from unittest.mock import patch -from googlemaps.exceptions import ApiError +from googlemaps.exceptions import ApiError, Timeout, TransportError import pytest from homeassistant.components.google_travel_time.const import DOMAIN @@ -58,3 +58,21 @@ def validate_config_entry_fixture(): def invalidate_config_entry_fixture(validate_config_entry): """Return invalid config entry.""" validate_config_entry.side_effect = ApiError("test") + + +@pytest.fixture(name="invalid_api_key") +def invalid_api_key_fixture(validate_config_entry): + """Throw a REQUEST_DENIED ApiError.""" + validate_config_entry.side_effect = ApiError("REQUEST_DENIED", "Invalid API key.") + + +@pytest.fixture(name="timeout") +def timeout_fixture(validate_config_entry): + """Throw a Timeout exception.""" + validate_config_entry.side_effect = Timeout() + + +@pytest.fixture(name="transport_error") +def transport_error_fixture(validate_config_entry): + """Throw a TransportError exception.""" + validate_config_entry.side_effect = TransportError("Unknown.") diff --git a/tests/components/google_travel_time/test_config_flow.py b/tests/components/google_travel_time/test_config_flow.py index 06cc1851421..ff7e0768301 100644 --- a/tests/components/google_travel_time/test_config_flow.py +++ b/tests/components/google_travel_time/test_config_flow.py @@ -67,6 +67,73 @@ async def test_invalid_config_entry(hass): assert result2["errors"] == {"base": "cannot_connect"} +@pytest.mark.usefixtures("invalid_api_key") +async def test_invalid_api_key(hass): + """Test we get the form.""" + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + assert result["type"] == data_entry_flow.FlowResultType.FORM + assert result["errors"] == {} + result2 = await hass.config_entries.flow.async_configure( + result["flow_id"], + MOCK_CONFIG, + ) + + assert result2["type"] == data_entry_flow.FlowResultType.FORM + assert result2["errors"] == {"base": "invalid_auth"} + + +@pytest.mark.usefixtures("transport_error") +async def test_transport_error(hass): + """Test we get the form.""" + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + assert result["type"] == data_entry_flow.FlowResultType.FORM + assert result["errors"] == {} + result2 = await hass.config_entries.flow.async_configure( + result["flow_id"], + MOCK_CONFIG, + ) + + assert result2["type"] == data_entry_flow.FlowResultType.FORM + assert result2["errors"] == {"base": "cannot_connect"} + + +@pytest.mark.usefixtures("timeout") +async def test_timeout(hass): + """Test we get the form.""" + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + assert result["type"] == data_entry_flow.FlowResultType.FORM + assert result["errors"] == {} + result2 = await hass.config_entries.flow.async_configure( + result["flow_id"], + MOCK_CONFIG, + ) + + assert result2["type"] == data_entry_flow.FlowResultType.FORM + assert result2["errors"] == {"base": "cannot_connect"} + + +async def test_malformed_api_key(hass): + """Test we get the form.""" + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + assert result["type"] == data_entry_flow.FlowResultType.FORM + assert result["errors"] == {} + result2 = await hass.config_entries.flow.async_configure( + result["flow_id"], + MOCK_CONFIG, + ) + + assert result2["type"] == data_entry_flow.FlowResultType.FORM + assert result2["errors"] == {"base": "invalid_auth"} + + @pytest.mark.parametrize( "data,options", [