From aa1304e93acfb4009126588ebec7ced176ac691f Mon Sep 17 00:00:00 2001 From: Alberto Geniola Date: Tue, 25 Apr 2023 15:40:46 +0200 Subject: [PATCH] Elmax/sensors improvements (#74323) * Address suggestions in PR #64090 * Remove redundant force-refresh at discovery time * Improve re-auth UX * Resolve linting warning * Cover reauth case when no entry is in place * Add missing reauth_successful message * Use ad-hoc schema for reauth flow * Align test cases with latest modifications * Improve re-authentication flow, align tests * Remove None check (as it can never happen) * Remove panel_id from reauth dialog * Remove panel_id from reauth dialog * Simplify try-catch block * Address suggestions in PR #64090 * Remove redundant force-refresh at discovery time * Improve re-auth UX * Resolve linting warning * Cover reauth case when no entry is in place * Add missing reauth_successful message * Use ad-hoc schema for reauth flow * Align test cases with latest modifications * Improve re-authentication flow, align tests * Remove None check (as it can never happen) * Remove panel_id from reauth dialog * Remove panel_id from reauth dialog * Simplify try-catch block * Improve type handling * Remove translations * Add unique-id attribute within context object * Optimize local variable usage --- .../components/elmax/binary_sensor.py | 9 +- homeassistant/components/elmax/config_flow.py | 87 ++++++++----------- homeassistant/components/elmax/strings.json | 12 ++- homeassistant/components/elmax/switch.py | 15 ++-- tests/components/elmax/test_config_flow.py | 66 ++++++++++---- 5 files changed, 114 insertions(+), 75 deletions(-) diff --git a/homeassistant/components/elmax/binary_sensor.py b/homeassistant/components/elmax/binary_sensor.py index 71588b4687f..6eb4cd654c5 100644 --- a/homeassistant/components/elmax/binary_sensor.py +++ b/homeassistant/components/elmax/binary_sensor.py @@ -44,11 +44,14 @@ async def async_setup_entry( coordinator=coordinator, ) entities.append(entity) - async_add_entities(entities, True) - known_devices.update([e.unique_id for e in entities]) + + if entities: + async_add_entities(entities) + known_devices.update([e.unique_id for e in entities]) # Register a listener for the discovery of new devices - coordinator.async_add_listener(_discover_new_devices) + remove_handle = coordinator.async_add_listener(_discover_new_devices) + config_entry.async_on_unload(remove_handle) # Immediately run a discovery, so we don't need to wait for the next update _discover_new_devices() diff --git a/homeassistant/components/elmax/config_flow.py b/homeassistant/components/elmax/config_flow.py index 0c1a0148205..5b9bb3b1085 100644 --- a/homeassistant/components/elmax/config_flow.py +++ b/homeassistant/components/elmax/config_flow.py @@ -32,6 +32,14 @@ LOGIN_FORM_SCHEMA = vol.Schema( } ) +REAUTH_FORM_SCHEMA = vol.Schema( + { + vol.Required(CONF_ELMAX_USERNAME): str, + vol.Required(CONF_ELMAX_PASSWORD): str, + vol.Required(CONF_ELMAX_PANEL_PIN): str, + } +) + def _store_panel_by_name( panel: PanelEntry, username: str, panel_names: dict[str, str] @@ -56,8 +64,7 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): _password: str _panels_schema: vol.Schema _panel_names: dict - _reauth_username: str | None - _reauth_panelid: str | None + _entry: config_entries.ConfigEntry | None async def async_step_user( self, user_input: dict[str, Any] | None = None @@ -170,82 +177,64 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): async def async_step_reauth(self, entry_data: Mapping[str, Any]) -> FlowResult: """Perform reauth upon an API authentication error.""" - self._reauth_username = entry_data.get(CONF_ELMAX_USERNAME) - self._reauth_panelid = entry_data.get(CONF_ELMAX_PANEL_ID) + self._entry = self.hass.config_entries.async_get_entry(self.context["entry_id"]) return await self.async_step_reauth_confirm() - async def async_step_reauth_confirm(self, user_input=None): + async def async_step_reauth_confirm( + self, user_input: dict[str, Any] | None = None + ) -> FlowResult: """Handle reauthorization flow.""" errors = {} if user_input is not None: - panel_pin = user_input.get(CONF_ELMAX_PANEL_PIN) - password = user_input.get(CONF_ELMAX_PASSWORD) - entry = await self.async_set_unique_id(self._reauth_panelid) + username = user_input[CONF_ELMAX_USERNAME] + password = user_input[CONF_ELMAX_PASSWORD] + panel_pin = user_input[CONF_ELMAX_PANEL_PIN] # Handle authentication, make sure the panel we are re-authenticating against is listed among results # and verify its pin is correct. + assert self._entry is not None try: # Test login. - client = await self._async_login( - username=self._reauth_username, password=password - ) - + client = await self._async_login(username=username, password=password) # Make sure the panel we are authenticating to is still available. panels = [ p for p in await client.list_control_panels() - if p.hash == self._reauth_panelid + if p.hash == self._entry.data[CONF_ELMAX_PANEL_ID] ] if len(panels) < 1: raise NoOnlinePanelsError() - # Verify the pin is still valid.from + # Verify the pin is still valid. await client.get_panel_status( - control_panel_id=self._reauth_panelid, pin=panel_pin + control_panel_id=self._entry.data[CONF_ELMAX_PANEL_ID], + pin=panel_pin, ) - # If it is, proceed with configuration update. - self.hass.config_entries.async_update_entry( - entry, - data={ - CONF_ELMAX_PANEL_ID: self._reauth_panelid, - CONF_ELMAX_PANEL_PIN: panel_pin, - CONF_ELMAX_USERNAME: self._reauth_username, - CONF_ELMAX_PASSWORD: password, - }, - ) - await self.hass.config_entries.async_reload(entry.entry_id) - self._reauth_username = None - self._reauth_panelid = None - return self.async_abort(reason="reauth_successful") - except ElmaxBadLoginError: - _LOGGER.error( - "Wrong credentials or failed login while re-authenticating" - ) errors["base"] = "invalid_auth" except NoOnlinePanelsError: - _LOGGER.warning( - "Panel ID %s is no longer associated to this user", - self._reauth_panelid, - ) errors["base"] = "reauth_panel_disappeared" except ElmaxBadPinError: errors["base"] = "invalid_pin" - # We want the user to re-authenticate only for the given panel id using the same login. - # We pin them to the UI, so the user realizes she must log in with the appropriate credentials - # for the that specific panel. - schema = vol.Schema( - { - vol.Required(CONF_ELMAX_USERNAME): self._reauth_username, - vol.Required(CONF_ELMAX_PASSWORD): str, - vol.Required(CONF_ELMAX_PANEL_ID): self._reauth_panelid, - vol.Required(CONF_ELMAX_PANEL_PIN): str, - } - ) + # If all went right, update the config entry + if not errors: + self.hass.config_entries.async_update_entry( + self._entry, + data={ + CONF_ELMAX_PANEL_ID: self._entry.data[CONF_ELMAX_PANEL_ID], + CONF_ELMAX_PANEL_PIN: panel_pin, + CONF_ELMAX_USERNAME: username, + CONF_ELMAX_PASSWORD: password, + }, + ) + await self.hass.config_entries.async_reload(self._entry.entry_id) + return self.async_abort(reason="reauth_successful") + + # Otherwise start over and show the relative error message return self.async_show_form( - step_id="reauth_confirm", data_schema=schema, errors=errors + step_id="reauth_confirm", data_schema=REAUTH_FORM_SCHEMA, errors=errors ) @staticmethod diff --git a/homeassistant/components/elmax/strings.json b/homeassistant/components/elmax/strings.json index a9c823f3a1a..e8cdbe23a5c 100644 --- a/homeassistant/components/elmax/strings.json +++ b/homeassistant/components/elmax/strings.json @@ -15,6 +15,14 @@ "panel_id": "Panel ID", "panel_pin": "PIN Code" } + }, + "reauth_confirm": { + "description": "Please re-authenticate with the panel.", + "data": { + "password": "[%key:common::config_flow::data::password%]", + "username": "[%key:common::config_flow::data::username%]", + "panel_pin": "Panel Pin" + } } }, "error": { @@ -22,10 +30,12 @@ "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]", "network_error": "A network error occurred", "invalid_pin": "The provided pin is invalid", + "reauth_panel_disappeared": "The given panel is no longer associated to this user. Please log in using an account associated to this panel.", "unknown": "[%key:common::config_flow::error::unknown%]" }, "abort": { - "already_configured": "[%key:common::config_flow::abort::already_configured_device%]" + "already_configured": "[%key:common::config_flow::abort::already_configured_device%]", + "reauth_successful": "[%key:common::config_flow::abort::reauth_successful%]" } } } diff --git a/homeassistant/components/elmax/switch.py b/homeassistant/components/elmax/switch.py index e8de2986b95..431e75a0883 100644 --- a/homeassistant/components/elmax/switch.py +++ b/homeassistant/components/elmax/switch.py @@ -36,19 +36,24 @@ async def async_setup_entry( # Otherwise, add all the entities we found entities = [] for actuator in panel_status.actuators: + # Skip already handled devices + if actuator.endpoint_id in known_devices: + continue entity = ElmaxSwitch( panel=coordinator.panel_entry, elmax_device=actuator, panel_version=panel_status.release, coordinator=coordinator, ) - if entity.unique_id not in known_devices: - entities.append(entity) - async_add_entities(entities, True) - known_devices.update([entity.unique_id for entity in entities]) + entities.append(entity) + + if entities: + async_add_entities(entities) + known_devices.update([entity.unique_id for entity in entities]) # Register a listener for the discovery of new devices - coordinator.async_add_listener(_discover_new_devices) + remove_handle = coordinator.async_add_listener(_discover_new_devices) + config_entry.async_on_unload(remove_handle) # Immediately run a discovery, so we don't need to wait for the next update _discover_new_devices() diff --git a/tests/components/elmax/test_config_flow.py b/tests/components/elmax/test_config_flow.py index e626a095e14..d2f8d9841d4 100644 --- a/tests/components/elmax/test_config_flow.py +++ b/tests/components/elmax/test_config_flow.py @@ -223,9 +223,25 @@ async def test_no_online_panel(hass: HomeAssistant) -> None: async def test_show_reauth(hass: HomeAssistant) -> None: """Test that the reauth form shows.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={ + CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, + CONF_ELMAX_USERNAME: MOCK_USERNAME, + CONF_ELMAX_PASSWORD: MOCK_PASSWORD, + CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, + }, + unique_id=MOCK_PANEL_ID, + ) + entry.add_to_hass(hass) + result = await hass.config_entries.flow.async_init( DOMAIN, - context={"source": SOURCE_REAUTH}, + context={ + "source": SOURCE_REAUTH, + "unique_id": entry.unique_id, + "entry_id": entry.entry_id, + }, data={ CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, @@ -239,7 +255,7 @@ async def test_show_reauth(hass: HomeAssistant) -> None: async def test_reauth_flow(hass: HomeAssistant) -> None: """Test that the reauth flow works.""" - MockConfigEntry( + entry = MockConfigEntry( domain=DOMAIN, data={ CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, @@ -248,7 +264,8 @@ async def test_reauth_flow(hass: HomeAssistant) -> None: CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, }, unique_id=MOCK_PANEL_ID, - ).add_to_hass(hass) + ) + entry.add_to_hass(hass) # Trigger reauth with patch( @@ -257,7 +274,11 @@ async def test_reauth_flow(hass: HomeAssistant) -> None: ): reauth_result = await hass.config_entries.flow.async_init( DOMAIN, - context={"source": SOURCE_REAUTH}, + context={ + "source": SOURCE_REAUTH, + "unique_id": entry.unique_id, + "entry_id": entry.entry_id, + }, data={ CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, @@ -268,7 +289,6 @@ async def test_reauth_flow(hass: HomeAssistant) -> None: result = await hass.config_entries.flow.async_configure( reauth_result["flow_id"], { - CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, CONF_ELMAX_USERNAME: MOCK_USERNAME, CONF_ELMAX_PASSWORD: MOCK_PASSWORD, @@ -282,7 +302,7 @@ async def test_reauth_flow(hass: HomeAssistant) -> None: async def test_reauth_panel_disappeared(hass: HomeAssistant) -> None: """Test that the case where panel is no longer associated with the user.""" # Simulate a first setup - MockConfigEntry( + entry = MockConfigEntry( domain=DOMAIN, data={ CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, @@ -291,7 +311,8 @@ async def test_reauth_panel_disappeared(hass: HomeAssistant) -> None: CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, }, unique_id=MOCK_PANEL_ID, - ).add_to_hass(hass) + ) + entry.add_to_hass(hass) # Trigger reauth with patch( @@ -300,7 +321,11 @@ async def test_reauth_panel_disappeared(hass: HomeAssistant) -> None: ): reauth_result = await hass.config_entries.flow.async_init( DOMAIN, - context={"source": SOURCE_REAUTH}, + context={ + "source": SOURCE_REAUTH, + "unique_id": entry.unique_id, + "entry_id": entry.entry_id, + }, data={ CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, @@ -311,7 +336,6 @@ async def test_reauth_panel_disappeared(hass: HomeAssistant) -> None: result = await hass.config_entries.flow.async_configure( reauth_result["flow_id"], { - CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, CONF_ELMAX_USERNAME: MOCK_USERNAME, CONF_ELMAX_PASSWORD: MOCK_PASSWORD, @@ -324,7 +348,7 @@ async def test_reauth_panel_disappeared(hass: HomeAssistant) -> None: async def test_reauth_invalid_pin(hass: HomeAssistant) -> None: """Test that the case where panel is no longer associated with the user.""" - MockConfigEntry( + entry = MockConfigEntry( domain=DOMAIN, data={ CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, @@ -333,7 +357,8 @@ async def test_reauth_invalid_pin(hass: HomeAssistant) -> None: CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, }, unique_id=MOCK_PANEL_ID, - ).add_to_hass(hass) + ) + entry.add_to_hass(hass) # Trigger reauth with patch( @@ -342,7 +367,11 @@ async def test_reauth_invalid_pin(hass: HomeAssistant) -> None: ): reauth_result = await hass.config_entries.flow.async_init( DOMAIN, - context={"source": SOURCE_REAUTH}, + context={ + "source": SOURCE_REAUTH, + "unique_id": entry.unique_id, + "entry_id": entry.entry_id, + }, data={ CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, @@ -353,7 +382,6 @@ async def test_reauth_invalid_pin(hass: HomeAssistant) -> None: result = await hass.config_entries.flow.async_configure( reauth_result["flow_id"], { - CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, CONF_ELMAX_USERNAME: MOCK_USERNAME, CONF_ELMAX_PASSWORD: MOCK_PASSWORD, @@ -366,7 +394,7 @@ async def test_reauth_invalid_pin(hass: HomeAssistant) -> None: async def test_reauth_bad_login(hass: HomeAssistant) -> None: """Test bad login attempt at reauth time.""" - MockConfigEntry( + entry = MockConfigEntry( domain=DOMAIN, data={ CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, @@ -375,7 +403,8 @@ async def test_reauth_bad_login(hass: HomeAssistant) -> None: CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, }, unique_id=MOCK_PANEL_ID, - ).add_to_hass(hass) + ) + entry.add_to_hass(hass) # Trigger reauth with patch( @@ -384,7 +413,11 @@ async def test_reauth_bad_login(hass: HomeAssistant) -> None: ): reauth_result = await hass.config_entries.flow.async_init( DOMAIN, - context={"source": SOURCE_REAUTH}, + context={ + "source": SOURCE_REAUTH, + "unique_id": entry.unique_id, + "entry_id": entry.entry_id, + }, data={ CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, @@ -395,7 +428,6 @@ async def test_reauth_bad_login(hass: HomeAssistant) -> None: result = await hass.config_entries.flow.async_configure( reauth_result["flow_id"], { - CONF_ELMAX_PANEL_ID: MOCK_PANEL_ID, CONF_ELMAX_PANEL_PIN: MOCK_PANEL_PIN, CONF_ELMAX_USERNAME: MOCK_USERNAME, CONF_ELMAX_PASSWORD: MOCK_PASSWORD,