Add reconfigure flow to Madvr (#122477)

* feat: add reconfigure

* feat: add reconfigure step

* fix: don't abort unique on reconfigure

* fix: add success string

* fix: improve reconfigure

* fix: entry will never be none

* fix: update ip in abort

* fix: check unique id on reconfigure

* feat: add test in case of new device

* fix: fail reconfigure if mac changes

* fix: abort instead of form

* feat: use is, dont mock config flow

* fix: implement comments

homeassistant/components/madvr/config_flow.py

@@ -8,8 +8,9 @@ import aiohttp
 from madvr.madvr import HeartBeatError, Madvr
 import voluptuous as vol
 
-from homeassistant.config_entries import ConfigFlow, ConfigFlowResult
+from homeassistant.config_entries import ConfigEntry, ConfigFlow, ConfigFlowResult
 from homeassistant.const import CONF_HOST, CONF_PORT
+from homeassistant.core import HomeAssistant
 
 from .const import DEFAULT_NAME, DEFAULT_PORT, DOMAIN
 from .errors import CannotConnect
@@ -18,13 +19,8 @@ _LOGGER = logging.getLogger(__name__)
 
 STEP_USER_DATA_SCHEMA = vol.Schema(
     {
-        vol.Required(
+        vol.Required(CONF_HOST): str,
-            CONF_HOST,
+        vol.Required(CONF_PORT, default=DEFAULT_PORT): int,
-        ): str,
-        vol.Required(
-            CONF_PORT,
-            default=DEFAULT_PORT,
-        ): int,
     }
 )
 
@@ -36,81 +32,120 @@ class MadVRConfigFlow(ConfigFlow, domain=DOMAIN):
 
     VERSION = 1
 
+    entry: ConfigEntry | None = None
+
     async def async_step_user(
         self, user_input: dict[str, Any] | None = None
     ) -> ConfigFlowResult:
         """Handle the initial step."""
+        return await self._handle_config_step(user_input)
+
+    async def async_step_reconfigure(
+        self, user_input: dict[str, Any] | None = None
+    ) -> ConfigFlowResult:
+        """Handle reconfiguration of the device."""
+        self.entry = self.hass.config_entries.async_get_entry(self.context["entry_id"])
+        return await self.async_step_reconfigure_confirm(user_input)
+
+    async def async_step_reconfigure_confirm(
+        self, user_input: dict[str, Any] | None = None
+    ) -> ConfigFlowResult:
+        """Handle a reconfiguration flow initialized by the user."""
+        return await self._handle_config_step(user_input, step_id="reconfigure")
+
+    async def _handle_config_step(
+        self, user_input: dict[str, Any] | None = None, step_id: str = "user"
+    ) -> ConfigFlowResult:
+        """Handle the configuration step for both initial setup and reconfiguration."""
         errors: dict[str, str] = {}
 
         if user_input is not None:
+            _LOGGER.debug("User input: %s", user_input)
             host = user_input[CONF_HOST]
             port = user_input[CONF_PORT]
 
             try:
-                # ensure we can connect and get the mac address from device
+                mac = await test_connection(self.hass, host, port)
-                mac = await self._test_connection(host, port)
             except CannotConnect:
                 _LOGGER.error("CannotConnect error caught")
                 errors["base"] = "cannot_connect"
             else:
                 if not mac:
                     errors["base"] = "no_mac"
-            if not errors:
+                else:
-                _LOGGER.debug("MAC address found: %s", mac)
+                    _LOGGER.debug("MAC address found: %s", mac)
-                # this will prevent the user from adding the same device twice and persist the mac address
+                    # abort if the detected mac differs from the one in the entry
-                await self.async_set_unique_id(mac)
+                    if self.entry:
-                self._abort_if_unique_id_configured()
+                        existing_mac = self.entry.unique_id
+                        if existing_mac != mac:
+                            _LOGGER.debug(
+                                "MAC address changed from %s to %s", existing_mac, mac
+                            )
+                            # abort
+                            return self.async_abort(reason="set_up_new_device")
 
-                # create the entry
+                        _LOGGER.debug("Reconfiguration done")
-                return self.async_create_entry(
+                        return self.async_update_reload_and_abort(
-                    title=DEFAULT_NAME,
+                            entry=self.entry,
-                    data=user_input,
+                            data={**user_input, CONF_HOST: host, CONF_PORT: port},
-                )
+                            reason="reconfigure_successful",
+                        )
+                    # abort if already configured with same mac
+                    await self.async_set_unique_id(mac)
+                    self._abort_if_unique_id_configured(updates={CONF_HOST: host})
 
-        # this will show the form or allow the user to retry if there was an error
+                    _LOGGER.debug("Configuration successful")
+                    return self.async_create_entry(
+                        title=DEFAULT_NAME,
+                        data=user_input,
+                    )
+        _LOGGER.debug("Showing form with errors: %s", errors)
         return self.async_show_form(
-            step_id="user",
+            step_id=step_id,
             data_schema=self.add_suggested_values_to_schema(
                 STEP_USER_DATA_SCHEMA, user_input
             ),
             errors=errors,
         )
 
-    async def _test_connection(self, host: str, port: int) -> str:
-        """Test if we can connect to the device and grab the mac."""
-        madvr_client = Madvr(host=host, port=port, loop=self.hass.loop)
-        _LOGGER.debug("Testing connection to madVR at %s:%s", host, port)
-        # try to connect
-        try:
-            await asyncio.wait_for(madvr_client.open_connection(), timeout=15)
-        # connection can raise HeartBeatError if the device is not available or connection does not work
-        except (TimeoutError, aiohttp.ClientError, OSError, HeartBeatError) as err:
-            _LOGGER.error("Error connecting to madVR: %s", err)
-            raise CannotConnect from err
 
-        # check if we are connected
+async def test_connection(hass: HomeAssistant, host: str, port: int) -> str:
-        if not madvr_client.connected:
+    """Test if we can connect to the device and grab the mac."""
-            raise CannotConnect("Connection failed")
+    madvr_client = Madvr(host=host, port=port, loop=hass.loop)
+    _LOGGER.debug("Testing connection to madVR at %s:%s", host, port)
+    # try to connect
+    try:
+        await asyncio.wait_for(madvr_client.open_connection(), timeout=15)
+    # connection can raise HeartBeatError if the device is not available or connection does not work
+    except (TimeoutError, aiohttp.ClientError, OSError, HeartBeatError) as err:
+        _LOGGER.error("Error connecting to madVR: %s", err)
+        raise CannotConnect from err
 
-        # background tasks needed to capture realtime info
+    # check if we are connected
-        await madvr_client.async_add_tasks()
+    if not madvr_client.connected:
+        raise CannotConnect("Connection failed")
 
-        # wait for client to capture device info
+    # background tasks needed to capture realtime info
-        retry_time = 15
+    await madvr_client.async_add_tasks()
-        while not madvr_client.mac_address and retry_time > 0:
-            await asyncio.sleep(RETRY_INTERVAL)
-            retry_time -= 1
 
-        mac_address = madvr_client.mac_address
+    # wait for client to capture device info
-        if mac_address:
+    retry_time = 15
-            _LOGGER.debug("Connected to madVR with MAC: %s", mac_address)
+    while not madvr_client.mac_address and retry_time > 0:
-        # close this connection because this client object will not be reused
+        await asyncio.sleep(RETRY_INTERVAL)
-        await self._close_test_connection(madvr_client)
+        retry_time -= 1
-        _LOGGER.debug("Connection test successful")
-        return mac_address
 
-    async def _close_test_connection(self, madvr_client: Madvr) -> None:
+    mac_address = madvr_client.mac_address
-        """Close the test connection."""
+    if mac_address:
-        madvr_client.stop()
+        _LOGGER.debug("Connected to madVR with MAC: %s", mac_address)
-        await madvr_client.async_cancel_tasks()
+    # close this connection because this client object will not be reused
-        await madvr_client.close_connection()
+    await close_test_connection(madvr_client)
+    _LOGGER.debug("Connection test successful")
+    return mac_address
+
+
+async def close_test_connection(madvr_client: Madvr) -> None:
+    """Close the test connection."""
+    _LOGGER.debug("Closing test connection")
+    madvr_client.stop()
+    await madvr_client.async_cancel_tasks()
+    await madvr_client.close_connection()

homeassistant/components/madvr/strings.json

@@ -3,7 +3,19 @@
     "step": {
       "user": {
         "title": "Setup madVR Envy",
-        "description": "Your device needs to be turned in order to add the integation. ",
+        "description": "Your device needs to be on in order to add the integation. ",
+        "data": {
+          "host": "[%key:common::config_flow::data::host%]",
+          "port": "[%key:common::config_flow::data::port%]"
+        },
+        "data_description": {
+          "host": "The hostname or IP address of your madVR Envy device.",
+          "port": "The port your madVR Envy is listening on. In 99% of cases, leave this as the default."
+        }
+      },
+      "reconfigure": {
+        "title": "Reconfigure madVR Envy",
+        "description": "Your device needs to be on in order to reconfigure the integation. ",
         "data": {
           "host": "[%key:common::config_flow::data::host%]",
           "port": "[%key:common::config_flow::data::port%]"
@@ -15,11 +27,13 @@
       }
     },
     "abort": {
-      "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%]"
     },
     "error": {
       "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]",
-      "no_mac": "A MAC address was not found. It required to identify the device. Please ensure your device is connectable."
+      "no_mac": "A MAC address was not found. It required to identify the device. Please ensure your device is connectable.",
+      "set_up_new_device": "A new device was detected. Please set it up as a new entity instead of reconfiguring."
     }
   },
   "entity": {

tests/components/madvr/const.py

@@ -8,6 +8,7 @@ MOCK_CONFIG = {
 }
 
 MOCK_MAC = "00:11:22:33:44:55"
+MOCK_MAC_NEW = "00:00:00:00:00:01"
 
 TEST_CON_ERROR = ConnectionError("Connection failed")
 TEST_IMP_ERROR = NotImplementedError("Not implemented")

tests/components/madvr/test_config_flow.py

@@ -6,12 +6,12 @@ from unittest.mock import AsyncMock, patch
 import pytest
 
 from homeassistant.components.madvr.const import DEFAULT_NAME, DOMAIN
-from homeassistant.config_entries import SOURCE_USER
+from homeassistant.config_entries import SOURCE_RECONFIGURE, SOURCE_USER
 from homeassistant.const import CONF_HOST, CONF_PORT
 from homeassistant.core import HomeAssistant
 from homeassistant.data_entry_flow import FlowResultType
 
-from .const import MOCK_CONFIG, MOCK_MAC
+from .const import MOCK_CONFIG, MOCK_MAC, MOCK_MAC_NEW
 
 from tests.common import MockConfigEntry
 
@@ -126,3 +126,120 @@ async def test_duplicate(
     )
     assert result["type"] is FlowResultType.ABORT
     assert result["reason"] == "already_configured"
+
+
+async def test_reconfigure_flow(
+    hass: HomeAssistant,
+    mock_madvr_client: AsyncMock,
+    mock_config_entry: MockConfigEntry,
+) -> None:
+    """Test reconfigure flow."""
+    mock_config_entry.add_to_hass(hass)
+    result = await hass.config_entries.flow.async_init(
+        DOMAIN,
+        context={"source": SOURCE_RECONFIGURE, "entry_id": mock_config_entry.entry_id},
+    )
+
+    assert result["type"] is FlowResultType.FORM
+    assert result["step_id"] == "reconfigure"
+    assert result["errors"] == {}
+
+    # define new host
+    new_host = "192.168.1.100"
+    # make sure setting port works
+    new_port = 44078
+
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"],
+        {CONF_HOST: new_host, CONF_PORT: new_port},
+    )
+
+    # should get the abort with success result
+    assert result["type"] is FlowResultType.ABORT
+    assert result["reason"] == "reconfigure_successful"
+
+    # Verify that the config entry was updated
+    assert mock_config_entry.data[CONF_HOST] == new_host
+    assert mock_config_entry.data[CONF_PORT] == new_port
+
+    # Verify that the connection was tested
+    mock_madvr_client.open_connection.assert_called()
+    mock_madvr_client.async_add_tasks.assert_called()
+    mock_madvr_client.async_cancel_tasks.assert_called()
+
+
+async def test_reconfigure_new_device(
+    hass: HomeAssistant,
+    mock_madvr_client: AsyncMock,
+    mock_config_entry: MockConfigEntry,
+) -> None:
+    """Test reconfigure flow."""
+    mock_config_entry.add_to_hass(hass)
+    # test reconfigure with a new device (should fail)
+    result = await hass.config_entries.flow.async_init(
+        DOMAIN,
+        context={"source": SOURCE_RECONFIGURE, "entry_id": mock_config_entry.entry_id},
+    )
+
+    # define new host
+    new_host = "192.168.1.100"
+    # make sure setting port works
+    new_port = 44078
+
+    # modify test_connection so it returns new_mac
+    mock_madvr_client.mac_address = MOCK_MAC_NEW
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"],
+        {CONF_HOST: new_host, CONF_PORT: new_port},
+    )
+
+    # unique id should remain unchanged with new device, should fail
+    assert mock_config_entry.unique_id == MOCK_MAC
+    assert result["type"] is FlowResultType.ABORT
+    assert result["reason"] == "set_up_new_device"
+
+
+async def test_reconfigure_flow_errors(
+    hass: HomeAssistant,
+    mock_madvr_client: AsyncMock,
+    mock_config_entry: MockConfigEntry,
+) -> None:
+    """Test error handling in reconfigure flow."""
+    mock_config_entry.add_to_hass(hass)
+
+    result = await hass.config_entries.flow.async_init(
+        DOMAIN,
+        context={"source": SOURCE_RECONFIGURE, "entry_id": mock_config_entry.entry_id},
+    )
+
+    assert result["type"] is FlowResultType.FORM
+    assert result["step_id"] == "reconfigure"
+
+    # Test CannotConnect error
+    mock_madvr_client.open_connection.side_effect = TimeoutError
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"],
+        {CONF_HOST: "192.168.1.100", CONF_PORT: 44077},
+    )
+    assert result["type"] is FlowResultType.FORM
+    assert result["errors"] == {"base": "cannot_connect"}
+
+    # Test no_mac error
+    mock_madvr_client.open_connection.side_effect = None
+    mock_madvr_client.connected = True
+    mock_madvr_client.mac_address = None
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"],
+        {CONF_HOST: "192.168.1.100", CONF_PORT: 44077},
+    )
+    assert result["type"] is FlowResultType.FORM
+    assert result["errors"] == {"base": "no_mac"}
+
+    # Ensure errors are recoverable
+    mock_madvr_client.mac_address = MOCK_MAC
+    result = await hass.config_entries.flow.async_configure(
+        result["flow_id"],
+        {CONF_HOST: "192.168.1.100", CONF_PORT: 44077},
+    )
+    assert result["type"] is FlowResultType.ABORT
+    assert result["reason"] == "reconfigure_successful"