From cd928d5571b60312e2e899a50a0acf61cff6ec5a Mon Sep 17 00:00:00 2001 From: Maciej Bieniek Date: Wed, 12 Jun 2024 17:39:44 +0200 Subject: [PATCH] Support reconfigure flow in Brother integration (#117298) * Add reconfigure flow * Improve config flow * Check if it is the same printer * Improve description * Add tests * Improve strings * Add missing reconfigure_successful string * Improve test names and comments * Format * Mock unload entry * Use add_suggested_values_to_schema() * Do not abort when another device's IP has been used * Remove unnecessary code * Suggested changes --------- Co-authored-by: Maciej Bieniek <478555+bieniu@users.noreply.github.com> --- .../components/brother/config_flow.py | 104 ++++++++++-- homeassistant/components/brother/strings.json | 15 +- tests/components/brother/conftest.py | 11 +- tests/components/brother/test_config_flow.py | 159 +++++++++++++++++- 4 files changed, 266 insertions(+), 23 deletions(-) diff --git a/homeassistant/components/brother/config_flow.py b/homeassistant/components/brother/config_flow.py index 2b711186fff..4536cb9c4d5 100644 --- a/homeassistant/components/brother/config_flow.py +++ b/homeassistant/components/brother/config_flow.py @@ -2,15 +2,16 @@ from __future__ import annotations -from typing import Any +from typing import TYPE_CHECKING, Any from brother import Brother, SnmpError, UnsupportedModelError import voluptuous as vol from homeassistant.components import zeroconf from homeassistant.components.snmp import async_get_snmp_engine -from homeassistant.config_entries import ConfigFlow, ConfigFlowResult +from homeassistant.config_entries import ConfigEntry, ConfigFlow, ConfigFlowResult from homeassistant.const import CONF_HOST, CONF_TYPE +from homeassistant.core import HomeAssistant from homeassistant.exceptions import HomeAssistantError from homeassistant.util.network import is_host_valid @@ -18,10 +19,29 @@ from .const import DOMAIN, PRINTER_TYPES DATA_SCHEMA = vol.Schema( { - vol.Required(CONF_HOST, default=""): str, + vol.Required(CONF_HOST): str, vol.Optional(CONF_TYPE, default="laser"): vol.In(PRINTER_TYPES), } ) +RECONFIGURE_SCHEMA = vol.Schema({vol.Required(CONF_HOST): str}) + + +async def validate_input( + hass: HomeAssistant, user_input: dict[str, Any], expected_mac: str | None = None +) -> tuple[str, str]: + """Validate the user input.""" + if not is_host_valid(user_input[CONF_HOST]): + raise InvalidHost + + snmp_engine = await async_get_snmp_engine(hass) + + brother = await Brother.create(user_input[CONF_HOST], snmp_engine=snmp_engine) + await brother.async_update() + + if expected_mac is not None and brother.serial.lower() != expected_mac: + raise AnotherDevice + + return (brother.model, brother.serial) class BrotherConfigFlow(ConfigFlow, domain=DOMAIN): @@ -33,6 +53,7 @@ class BrotherConfigFlow(ConfigFlow, domain=DOMAIN): """Initialize.""" self.brother: Brother self.host: str | None = None + self.entry: ConfigEntry | None = None async def async_step_user( self, user_input: dict[str, Any] | None = None @@ -42,21 +63,7 @@ class BrotherConfigFlow(ConfigFlow, domain=DOMAIN): if user_input is not None: try: - if not is_host_valid(user_input[CONF_HOST]): - raise InvalidHost - - snmp_engine = await async_get_snmp_engine(self.hass) - - brother = await Brother.create( - user_input[CONF_HOST], snmp_engine=snmp_engine - ) - await brother.async_update() - - await self.async_set_unique_id(brother.serial.lower()) - self._abort_if_unique_id_configured() - - title = f"{brother.model} {brother.serial}" - return self.async_create_entry(title=title, data=user_input) + model, serial = await validate_input(self.hass, user_input) except InvalidHost: errors[CONF_HOST] = "wrong_host" except (ConnectionError, TimeoutError): @@ -65,6 +72,12 @@ class BrotherConfigFlow(ConfigFlow, domain=DOMAIN): errors["base"] = "snmp_error" except UnsupportedModelError: return self.async_abort(reason="unsupported_model") + else: + await self.async_set_unique_id(serial.lower()) + self._abort_if_unique_id_configured() + + title = f"{model} {serial}" + return self.async_create_entry(title=title, data=user_input) return self.async_show_form( step_id="user", data_schema=DATA_SCHEMA, errors=errors @@ -127,6 +140,61 @@ class BrotherConfigFlow(ConfigFlow, domain=DOMAIN): }, ) + async def async_step_reconfigure( + self, _: dict[str, Any] | None = None + ) -> ConfigFlowResult: + """Handle a reconfiguration flow initialized by the user.""" + entry = self.hass.config_entries.async_get_entry(self.context["entry_id"]) + + if TYPE_CHECKING: + assert entry is not None + + self.entry = entry + + return await self.async_step_reconfigure_confirm() + + async def async_step_reconfigure_confirm( + self, user_input: dict[str, Any] | None = None + ) -> ConfigFlowResult: + """Handle a reconfiguration flow initialized by the user.""" + errors = {} + + if TYPE_CHECKING: + assert self.entry is not None + + if user_input is not None: + try: + await validate_input(self.hass, user_input, self.entry.unique_id) + except InvalidHost: + errors[CONF_HOST] = "wrong_host" + except (ConnectionError, TimeoutError): + errors["base"] = "cannot_connect" + except SnmpError: + errors["base"] = "snmp_error" + except AnotherDevice: + errors["base"] = "another_device" + else: + self.hass.config_entries.async_update_entry( + self.entry, + data=self.entry.data | {CONF_HOST: user_input[CONF_HOST]}, + ) + await self.hass.config_entries.async_reload(self.entry.entry_id) + return self.async_abort(reason="reconfigure_successful") + + return self.async_show_form( + step_id="reconfigure_confirm", + data_schema=self.add_suggested_values_to_schema( + data_schema=RECONFIGURE_SCHEMA, + suggested_values=self.entry.data | (user_input or {}), + ), + description_placeholders={"printer_name": self.entry.title}, + errors=errors, + ) + class InvalidHost(HomeAssistantError): """Error to indicate that hostname/IP address is invalid.""" + + +class AnotherDevice(HomeAssistantError): + """Error to indicate that hostname/IP address belongs to another device.""" diff --git a/homeassistant/components/brother/strings.json b/homeassistant/components/brother/strings.json index 0d8f4f4eedf..d7f8f4a1b89 100644 --- a/homeassistant/components/brother/strings.json +++ b/homeassistant/components/brother/strings.json @@ -17,16 +17,27 @@ "data": { "type": "[%key:component::brother::config::step::user::data::type%]" } + }, + "reconfigure_confirm": { + "description": "Update configuration for {printer_name}.", + "data": { + "host": "[%key:common::config_flow::data::host%]" + }, + "data_description": { + "host": "[%key:component::brother::config::step::user::data_description::host%]" + } } }, "error": { "wrong_host": "Invalid hostname or IP address.", "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]", - "snmp_error": "SNMP server turned off or printer not supported." + "snmp_error": "SNMP server turned off or printer not supported.", + "another_device": "The IP address or hostname of another Brother printer was used." }, "abort": { "unsupported_model": "This printer model is not supported.", - "already_configured": "[%key:common::config_flow::abort::already_configured_device%]" + "already_configured": "[%key:common::config_flow::abort::already_configured_device%]", + "reconfigure_successful": "[%key:common::config_flow::abort::reconfigure_successful%]" } }, "entity": { diff --git a/tests/components/brother/conftest.py b/tests/components/brother/conftest.py index 66f92f5907d..5fadca5314d 100644 --- a/tests/components/brother/conftest.py +++ b/tests/components/brother/conftest.py @@ -87,7 +87,16 @@ def mock_setup_entry() -> Generator[AsyncMock]: @pytest.fixture -def mock_brother_client() -> Generator[AsyncMock]: +def mock_unload_entry() -> Generator[AsyncMock, None, None]: + """Override async_unload_entry.""" + with patch( + "homeassistant.components.brother.async_unload_entry", return_value=True + ) as mock_unload_entry: + yield mock_unload_entry + + +@pytest.fixture +def mock_brother_client() -> Generator[AsyncMock, None, None]: """Mock Brother client.""" with ( patch("homeassistant.components.brother.Brother", autospec=True) as mock_client, diff --git a/tests/components/brother/test_config_flow.py b/tests/components/brother/test_config_flow.py index 3a9aff48e90..ac7af4cc912 100644 --- a/tests/components/brother/test_config_flow.py +++ b/tests/components/brother/test_config_flow.py @@ -8,7 +8,11 @@ import pytest from homeassistant.components import zeroconf from homeassistant.components.brother.const import DOMAIN -from homeassistant.config_entries import SOURCE_USER, SOURCE_ZEROCONF +from homeassistant.config_entries import ( + SOURCE_RECONFIGURE, + SOURCE_USER, + SOURCE_ZEROCONF, +) from homeassistant.const import CONF_HOST, CONF_TYPE from homeassistant.core import HomeAssistant from homeassistant.data_entry_flow import FlowResultType @@ -19,7 +23,7 @@ from tests.common import MockConfigEntry CONFIG = {CONF_HOST: "127.0.0.1", CONF_TYPE: "laser"} -pytestmark = pytest.mark.usefixtures("mock_setup_entry") +pytestmark = pytest.mark.usefixtures("mock_setup_entry", "mock_unload_entry") async def test_show_form(hass: HomeAssistant) -> None: @@ -248,3 +252,154 @@ async def test_zeroconf_confirm_create_entry( assert result["title"] == "HL-L2340DW 0123456789" assert result["data"][CONF_HOST] == "127.0.0.1" assert result["data"][CONF_TYPE] == "laser" + + +async def test_reconfigure_successful( + hass: HomeAssistant, + mock_brother_client: AsyncMock, + mock_config_entry: MockConfigEntry, +) -> None: + """Test starting a reconfigure flow.""" + await init_integration(hass, mock_config_entry) + + result = await hass.config_entries.flow.async_init( + DOMAIN, + context={ + "source": SOURCE_RECONFIGURE, + "entry_id": mock_config_entry.entry_id, + }, + data=mock_config_entry.data, + ) + + assert result["type"] is FlowResultType.FORM + assert result["step_id"] == "reconfigure_confirm" + + result = await hass.config_entries.flow.async_configure( + result["flow_id"], + user_input={CONF_HOST: "10.10.10.10"}, + ) + + assert result["type"] is FlowResultType.ABORT + assert result["reason"] == "reconfigure_successful" + assert mock_config_entry.data == { + CONF_HOST: "10.10.10.10", + CONF_TYPE: "laser", + } + + +@pytest.mark.parametrize( + ("exc", "base_error"), + [ + (ConnectionError, "cannot_connect"), + (TimeoutError, "cannot_connect"), + (SnmpError("error"), "snmp_error"), + ], +) +async def test_reconfigure_not_successful( + hass: HomeAssistant, + exc: Exception, + base_error: str, + mock_brother_client: AsyncMock, + mock_config_entry: MockConfigEntry, +) -> None: + """Test starting a reconfigure flow but no connection found.""" + await init_integration(hass, mock_config_entry) + + result = await hass.config_entries.flow.async_init( + DOMAIN, + context={ + "source": SOURCE_RECONFIGURE, + "entry_id": mock_config_entry.entry_id, + }, + data=mock_config_entry.data, + ) + + assert result["type"] is FlowResultType.FORM + assert result["step_id"] == "reconfigure_confirm" + + mock_brother_client.async_update.side_effect = exc + + result = await hass.config_entries.flow.async_configure( + result["flow_id"], + user_input={CONF_HOST: "10.10.10.10"}, + ) + + assert result["type"] is FlowResultType.FORM + assert result["step_id"] == "reconfigure_confirm" + assert result["errors"] == {"base": base_error} + + mock_brother_client.async_update.side_effect = None + + result = await hass.config_entries.flow.async_configure( + result["flow_id"], + user_input={CONF_HOST: "10.10.10.10"}, + ) + + assert result["type"] is FlowResultType.ABORT + assert result["reason"] == "reconfigure_successful" + assert mock_config_entry.data == { + CONF_HOST: "10.10.10.10", + CONF_TYPE: "laser", + } + + +async def test_reconfigure_invalid_hostname( + hass: HomeAssistant, + mock_brother_client: AsyncMock, + mock_config_entry: MockConfigEntry, +) -> None: + """Test starting a reconfigure flow but no connection found.""" + await init_integration(hass, mock_config_entry) + + result = await hass.config_entries.flow.async_init( + DOMAIN, + context={ + "source": SOURCE_RECONFIGURE, + "entry_id": mock_config_entry.entry_id, + }, + data=mock_config_entry.data, + ) + + assert result["type"] is FlowResultType.FORM + assert result["step_id"] == "reconfigure_confirm" + + result = await hass.config_entries.flow.async_configure( + result["flow_id"], + user_input={CONF_HOST: "invalid/hostname"}, + ) + + assert result["type"] is FlowResultType.FORM + assert result["step_id"] == "reconfigure_confirm" + assert result["errors"] == {CONF_HOST: "wrong_host"} + + +async def test_reconfigure_not_the_same_device( + hass: HomeAssistant, + mock_brother_client: AsyncMock, + mock_config_entry: MockConfigEntry, +) -> None: + """Test starting the reconfiguration process, but with a different printer.""" + await init_integration(hass, mock_config_entry) + + result = await hass.config_entries.flow.async_init( + DOMAIN, + context={ + "source": SOURCE_RECONFIGURE, + "entry_id": mock_config_entry.entry_id, + }, + data=mock_config_entry.data, + ) + + assert result["type"] is FlowResultType.FORM + assert result["step_id"] == "reconfigure_confirm" + + mock_brother_client.serial = "9876543210" + + result = await hass.config_entries.flow.async_configure( + result["flow_id"], + user_input={CONF_HOST: "10.10.10.10"}, + ) + + assert result["type"] is FlowResultType.FORM + assert result["step_id"] == "reconfigure_confirm" + assert result["errors"] == {"base": "another_device"}