From bb5f0529e6acc11d4bc28832713b60a65d7d5d56 Mon Sep 17 00:00:00 2001 From: Timothy <6560631+TimoPtr@users.noreply.github.com> Date: Thu, 20 Nov 2025 13:13:07 +0100 Subject: [PATCH 01/11] Cleanup cloudhook when not connected and don't send it over get_config --- .../components/mobile_app/__init__.py | 14 ++- .../components/mobile_app/webhook.py | 5 +- tests/components/mobile_app/test_init.py | 99 +++++++++++++++++++ tests/components/mobile_app/test_webhook.py | 67 +++++++++++++ 4 files changed, 179 insertions(+), 6 deletions(-) diff --git a/homeassistant/components/mobile_app/__init__.py b/homeassistant/components/mobile_app/__init__.py index 64b367112efd9..7b4583d76659d 100644 --- a/homeassistant/components/mobile_app/__init__.py +++ b/homeassistant/components/mobile_app/__init__.py @@ -131,12 +131,22 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: registration_name = f"Mobile App: {registration[ATTR_DEVICE_NAME]}" webhook_register(hass, DOMAIN, registration_name, webhook_id, handle_webhook) + def clean_cloudhook() -> None: + """Clean up cloudhook from config entry.""" + if CONF_CLOUDHOOK_URL in entry.data: + data = dict(entry.data) + data.pop(CONF_CLOUDHOOK_URL) + hass.config_entries.async_update_entry(entry, data=data) async def manage_cloudhook(state: cloud.CloudConnectionState) -> None: if ( state is cloud.CloudConnectionState.CLOUD_CONNECTED and CONF_CLOUDHOOK_URL not in entry.data ): await async_create_cloud_hook(hass, webhook_id, entry) + elif (state is cloud.CloudConnectionState.CLOUD_DISCONNECTED + and not cloud.async_is_logged_in(hass) + ): + clean_cloudhook() if cloud.async_is_logged_in(hass): if ( @@ -147,9 +157,7 @@ async def manage_cloudhook(state: cloud.CloudConnectionState) -> None: await async_create_cloud_hook(hass, webhook_id, entry) elif CONF_CLOUDHOOK_URL in entry.data: # If we have a cloudhook but no longer logged in to the cloud, remove it from the entry - data = dict(entry.data) - data.pop(CONF_CLOUDHOOK_URL) - hass.config_entries.async_update_entry(entry, data=data) + clean_cloudhook() entry.async_on_unload(cloud.async_listen_connection_change(hass, manage_cloudhook)) diff --git a/homeassistant/components/mobile_app/webhook.py b/homeassistant/components/mobile_app/webhook.py index 102182026683d..da6c6676b4f35 100644 --- a/homeassistant/components/mobile_app/webhook.py +++ b/homeassistant/components/mobile_app/webhook.py @@ -756,10 +756,9 @@ async def webhook_get_config( "theme_color": MANIFEST_JSON["theme_color"], } - if CONF_CLOUDHOOK_URL in config_entry.data: - resp[CONF_CLOUDHOOK_URL] = config_entry.data[CONF_CLOUDHOOK_URL] - if cloud.async_active_subscription(hass): + if CONF_CLOUDHOOK_URL in config_entry.data: + resp[CONF_CLOUDHOOK_URL] = config_entry.data[CONF_CLOUDHOOK_URL] with suppress(cloud.CloudNotAvailable): resp[CONF_REMOTE_UI_URL] = cloud.async_remote_ui_url(hass) diff --git a/tests/components/mobile_app/test_init.py b/tests/components/mobile_app/test_init.py index a4edbea6ecf8b..dd0e2d0d09ac1 100644 --- a/tests/components/mobile_app/test_init.py +++ b/tests/components/mobile_app/test_init.py @@ -260,3 +260,102 @@ async def test_remove_entry_on_user_remove( entries = hass.config_entries.async_entries(DOMAIN) assert len(entries) == 0 + + +async def test_cloudhook_cleanup_on_disconnect_and_logout( + hass: HomeAssistant, + hass_admin_user: MockUser, +) -> None: + """Test cloudhook is cleaned up when cloud disconnects and user is logged out.""" + config_entry = MockConfigEntry( + data={ + **REGISTER_CLEARTEXT, + CONF_WEBHOOK_ID: "test-webhook-id", + ATTR_DEVICE_NAME: "Test", + ATTR_DEVICE_ID: "Test", + CONF_USER_ID: hass_admin_user.id, + CONF_CLOUDHOOK_URL: "https://hook-url", + }, + domain=DOMAIN, + title="Test", + ) + config_entry.add_to_hass(hass) + + with ( + patch( + "homeassistant.components.cloud.async_is_logged_in", + return_value=True, + ), + patch( + "homeassistant.components.cloud.async_active_subscription", + return_value=True, + ), + patch( + "homeassistant.components.cloud.async_is_connected", + return_value=True, + ), + ): + assert await hass.config_entries.async_setup(config_entry.entry_id) + await hass.async_block_till_done() + assert config_entry.state is ConfigEntryState.LOADED + # Cloudhook should still exist + assert CONF_CLOUDHOOK_URL in config_entry.data + + # Simulate cloud disconnect and logout + with patch( + "homeassistant.components.cloud.async_is_logged_in", + return_value=False, + ): + async_mock_cloud_connection_status(hass, False) + await hass.async_block_till_done() + + # Cloudhook should be removed from config entry + assert CONF_CLOUDHOOK_URL not in config_entry.data + + +async def test_cloudhook_persists_on_disconnect_when_logged_in( + hass: HomeAssistant, + hass_admin_user: MockUser, +) -> None: + """Test cloudhook persists when cloud disconnects but user is still logged in.""" + config_entry = MockConfigEntry( + data={ + **REGISTER_CLEARTEXT, + CONF_WEBHOOK_ID: "test-webhook-id", + ATTR_DEVICE_NAME: "Test", + ATTR_DEVICE_ID: "Test", + CONF_USER_ID: hass_admin_user.id, + CONF_CLOUDHOOK_URL: "https://hook-url", + }, + domain=DOMAIN, + title="Test", + ) + config_entry.add_to_hass(hass) + + with ( + patch( + "homeassistant.components.cloud.async_is_logged_in", + return_value=True, + ), + patch( + "homeassistant.components.cloud.async_active_subscription", + return_value=True, + ), + patch( + "homeassistant.components.cloud.async_is_connected", + return_value=True, + ), + ): + assert await hass.config_entries.async_setup(config_entry.entry_id) + await hass.async_block_till_done() + assert config_entry.state is ConfigEntryState.LOADED + # Cloudhook should exist + assert CONF_CLOUDHOOK_URL in config_entry.data + + # Simulate cloud disconnect while still logged in + async_mock_cloud_connection_status(hass, False) + await hass.async_block_till_done() + + # Cloudhook should still exist because user is still logged in + assert CONF_CLOUDHOOK_URL in config_entry.data + diff --git a/tests/components/mobile_app/test_webhook.py b/tests/components/mobile_app/test_webhook.py index b071caebd16e2..e0c5a1cf77cd4 100644 --- a/tests/components/mobile_app/test_webhook.py +++ b/tests/components/mobile_app/test_webhook.py @@ -310,6 +310,73 @@ async def test_webhook_handle_get_config( assert expected_dict == json +async def test_webhook_handle_get_config_with_cloudhook_and_active_subscription( + hass: HomeAssistant, + create_registrations: tuple[dict[str, Any], dict[str, Any]], + webhook_client: TestClient, +) -> None: + """Test get_config returns cloudhook_url when there's an active subscription.""" + webhook_id = create_registrations[1]["webhook_id"] + webhook_url = f"/api/webhook/{webhook_id}" + + # Get the config entry and add cloudhook_url to it + config_entry = hass.config_entries.async_entries(DOMAIN)[1] + hass.config_entries.async_update_entry( + config_entry, + data={**config_entry.data, "cloudhook_url": "https://hooks.nabu.casa/test"}, + ) + + with ( + patch( + "homeassistant.components.cloud.async_active_subscription", + return_value=True, + ), + patch( + "homeassistant.components.cloud.async_remote_ui_url", + return_value="https://remote.ui.url", + ), + ): + resp = await webhook_client.post(webhook_url, json={"type": "get_config"}) + assert resp.status == HTTPStatus.OK + json_resp = await resp.json() + + # Cloudhook should be in response + assert "cloudhook_url" in json_resp + assert json_resp["cloudhook_url"] == "https://hooks.nabu.casa/test" + # Remote UI should also be in response + assert "remote_ui_url" in json_resp + + +async def test_webhook_handle_get_config_with_cloudhook_no_subscription( + hass: HomeAssistant, + create_registrations: tuple[dict[str, Any], dict[str, Any]], + webhook_client: TestClient, +) -> None: + """Test get_config doesn't return cloudhook_url without active subscription.""" + webhook_id = create_registrations[1]["webhook_id"] + webhook_url = f"/api/webhook/{webhook_id}" + + # Get the config entry and add cloudhook_url to it + config_entry = hass.config_entries.async_entries(DOMAIN)[1] + hass.config_entries.async_update_entry( + config_entry, + data={**config_entry.data, "cloudhook_url": "https://hooks.nabu.casa/test"}, + ) + + with patch( + "homeassistant.components.cloud.async_active_subscription", + return_value=False, + ): + resp = await webhook_client.post(webhook_url, json={"type": "get_config"}) + assert resp.status == HTTPStatus.OK + json_resp = await resp.json() + + # Cloudhook should NOT be in response even though it exists in config entry + assert "cloudhook_url" not in json_resp + # Remote UI should also not be in response + assert "remote_ui_url" not in json_resp + + async def test_webhook_returns_error_incorrect_json( create_registrations: tuple[dict[str, Any], dict[str, Any]], webhook_client: TestClient, From 219b734556f5a5f67ea75cf0a0e0d9d546b628ad Mon Sep 17 00:00:00 2001 From: Timothy <6560631+TimoPtr@users.noreply.github.com> Date: Thu, 20 Nov 2025 13:14:51 +0100 Subject: [PATCH 02/11] Watch Cloudhook delection and remove it from entry --- homeassistant/components/cloud/__init__.py | 34 ++++++ .../components/mobile_app/__init__.py | 23 +++- tests/components/cloud/test_init.py | 106 ++++++++++++++++++ tests/components/mobile_app/test_init.py | 61 ++++++++++ 4 files changed, 220 insertions(+), 4 deletions(-) diff --git a/homeassistant/components/cloud/__init__.py b/homeassistant/components/cloud/__init__.py index 05e420cb4143a..7b61d434b41f4 100644 --- a/homeassistant/components/cloud/__init__.py +++ b/homeassistant/components/cloud/__init__.py @@ -69,12 +69,15 @@ DOMAIN, MODE_DEV, MODE_PROD, + PREF_CLOUDHOOKS, ) from .helpers import FixedSizeQueueLogHandler from .prefs import CloudPreferences from .repairs import async_manage_legacy_subscription_issue from .subscription import async_subscription_info +_LOGGER = logging.getLogger(__name__) + DEFAULT_MODE = MODE_PROD PLATFORMS = [Platform.BINARY_SENSOR, Platform.STT, Platform.TTS] @@ -242,6 +245,37 @@ async def async_delete_cloudhook(hass: HomeAssistant, webhook_id: str) -> None: await hass.data[DATA_CLOUD].cloudhooks.async_delete(webhook_id) +@callback +def async_listen_cloudhook_deletion( + hass: HomeAssistant, + webhook_id: str, + on_deletion: Callable, +) -> Callable: + """Listen for cloudhook deletion and call on_deletion when deleted. + + Args: + hass: Home Assistant instance + webhook_id: The webhook ID to monitor + on_deletion: Callback to call when the cloudhook is deleted + + Returns: + Unsubscribe function to stop listening + """ + _LOGGER.debug("Setting up cloudhook deletion listener for %s", webhook_id) + + if DATA_CLOUD not in hass.data: + # If cloud is not available, return a no-op unsubscribe + return lambda: None + + async def _async_prefs_updated(prefs: CloudPreferences) -> None: + """Handle cloud preferences update.""" + if PREF_CLOUDHOOKS in prefs.last_updated: + if webhook_id not in prefs.cloudhooks: + on_deletion() + + return hass.data[DATA_CLOUD].client.prefs.async_listen_updates(_async_prefs_updated) + + @bind_hass @callback def async_remote_ui_url(hass: HomeAssistant) -> str: diff --git a/homeassistant/components/mobile_app/__init__.py b/homeassistant/components/mobile_app/__init__.py index 7b4583d76659d..91b56d3526d50 100644 --- a/homeassistant/components/mobile_app/__init__.py +++ b/homeassistant/components/mobile_app/__init__.py @@ -2,6 +2,7 @@ from contextlib import suppress from functools import partial +import logging from typing import Any from homeassistant.auth import EVENT_USER_REMOVED @@ -55,6 +56,8 @@ from .util import async_create_cloud_hook, supports_push from .webhook import handle_webhook +_LOGGER = logging.getLogger(__name__) + PLATFORMS = [Platform.BINARY_SENSOR, Platform.DEVICE_TRACKER, Platform.SENSOR] CONFIG_SCHEMA = cv.empty_config_schema(DOMAIN) @@ -134,16 +137,25 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: def clean_cloudhook() -> None: """Clean up cloudhook from config entry.""" if CONF_CLOUDHOOK_URL in entry.data: - data = dict(entry.data) - data.pop(CONF_CLOUDHOOK_URL) - hass.config_entries.async_update_entry(entry, data=data) + data = dict(entry.data) + data.pop(CONF_CLOUDHOOK_URL) + hass.config_entries.async_update_entry(entry, data=data) + + def on_cloudhook_deletion() -> None: + """Handle cloudhook deletion.""" + _LOGGER.debug("Webhook deleted %s", webhook_id) + data = dict(entry.data) + data.pop(CONF_CLOUDHOOK_URL, None) + hass.config_entries.async_update_entry(entry, data=data) + async def manage_cloudhook(state: cloud.CloudConnectionState) -> None: if ( state is cloud.CloudConnectionState.CLOUD_CONNECTED and CONF_CLOUDHOOK_URL not in entry.data ): await async_create_cloud_hook(hass, webhook_id, entry) - elif (state is cloud.CloudConnectionState.CLOUD_DISCONNECTED + elif ( + state is cloud.CloudConnectionState.CLOUD_DISCONNECTED and not cloud.async_is_logged_in(hass) ): clean_cloudhook() @@ -160,6 +172,9 @@ async def manage_cloudhook(state: cloud.CloudConnectionState) -> None: clean_cloudhook() entry.async_on_unload(cloud.async_listen_connection_change(hass, manage_cloudhook)) + entry.async_on_unload( + cloud.async_listen_cloudhook_deletion(hass, webhook_id, on_cloudhook_deletion) + ) await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS) diff --git a/tests/components/cloud/test_init.py b/tests/components/cloud/test_init.py index 087634c5173eb..f7cb796a389a1 100644 --- a/tests/components/cloud/test_init.py +++ b/tests/components/cloud/test_init.py @@ -11,6 +11,7 @@ CloudNotAvailable, CloudNotConnected, async_get_or_create_cloudhook, + async_listen_cloudhook_deletion, async_listen_connection_change, async_remote_ui_url, ) @@ -311,3 +312,108 @@ async def test_cloud_logout( await hass.async_block_till_done() assert cloud.is_logged_in is False + + +async def test_async_listen_cloudhook_deletion( + hass: HomeAssistant, + cloud: MagicMock, + set_cloud_prefs: Callable[[dict[str, Any]], Coroutine[Any, Any, None]], +) -> None: + """Test async_listen_cloudhook_deletion.""" + assert await async_setup_component(hass, "cloud", {"cloud": {}}) + await hass.async_block_till_done() + await cloud.login("test-user", "test-pass") + + webhook_id = "mock-webhook-id" + cloudhook_url = "https://cloudhook.nabu.casa/abcdefg" + + # Set up initial cloudhooks state + await set_cloud_prefs( + { + PREF_CLOUDHOOKS: { + webhook_id: { + "webhook_id": webhook_id, + "cloudhook_id": "random-id", + "cloudhook_url": cloudhook_url, + "managed": True, + } + } + } + ) + + # Track if deletion callback was called + deletion_called = False + + def on_deletion() -> None: + """Handle cloudhook deletion.""" + nonlocal deletion_called + deletion_called = True + + # Register the deletion listener + unsubscribe = async_listen_cloudhook_deletion(hass, webhook_id, on_deletion) + + # Verify deletion callback not called yet + assert not deletion_called + + # Delete the cloudhook by updating prefs + await set_cloud_prefs({PREF_CLOUDHOOKS: {}}) + await hass.async_block_till_done() + + # Verify deletion callback was called + assert deletion_called + + # Reset for next test + deletion_called = False + + # Add cloudhook back + await set_cloud_prefs( + { + PREF_CLOUDHOOKS: { + webhook_id: { + "webhook_id": webhook_id, + "cloudhook_id": "random-id", + "cloudhook_url": cloudhook_url, + "managed": True, + } + } + } + ) + await hass.async_block_till_done() + + # Verify callback not called when cloudhook is added + assert not deletion_called + + # Unsubscribe from listener + unsubscribe() + + # Delete cloudhook again + await set_cloud_prefs({PREF_CLOUDHOOKS: {}}) + await hass.async_block_till_done() + + # Verify deletion callback was NOT called after unsubscribe + assert not deletion_called + + +async def test_async_listen_cloudhook_deletion_no_cloud( + hass: HomeAssistant, +) -> None: + """Test async_listen_cloudhook_deletion when cloud is not available.""" + webhook_id = "mock-webhook-id" + deletion_called = False + + def on_deletion() -> None: + """Handle cloudhook deletion.""" + nonlocal deletion_called + deletion_called = True + + # Should return a no-op unsubscribe when cloud is not available + unsubscribe = async_listen_cloudhook_deletion(hass, webhook_id, on_deletion) + + # Verify it returns a callable + assert callable(unsubscribe) + + # Call unsubscribe (should not raise) + unsubscribe() + + # Verify deletion callback was never called + assert not deletion_called diff --git a/tests/components/mobile_app/test_init.py b/tests/components/mobile_app/test_init.py index dd0e2d0d09ac1..0ddec943ac6dd 100644 --- a/tests/components/mobile_app/test_init.py +++ b/tests/components/mobile_app/test_init.py @@ -359,3 +359,64 @@ async def test_cloudhook_persists_on_disconnect_when_logged_in( # Cloudhook should still exist because user is still logged in assert CONF_CLOUDHOOK_URL in config_entry.data + +async def test_cloudhook_deletion_listener( + hass: HomeAssistant, + hass_admin_user: MockUser, +) -> None: + """Test cloudhook deletion listener removes cloudhook from config entry.""" + webhook_id = "test-webhook-id" + config_entry = MockConfigEntry( + data={ + **REGISTER_CLEARTEXT, + CONF_WEBHOOK_ID: webhook_id, + ATTR_DEVICE_NAME: "Test", + ATTR_DEVICE_ID: "Test", + CONF_USER_ID: hass_admin_user.id, + CONF_CLOUDHOOK_URL: "https://hook-url", + }, + domain=DOMAIN, + title="Test", + ) + config_entry.add_to_hass(hass) + + cloudhook_deleted_callback = None + + def mock_listen_cloudhook_deletion(hass_instance, wh_id: str, callback): + """Mock the cloudhook deletion listener.""" + nonlocal cloudhook_deleted_callback + cloudhook_deleted_callback = callback + return lambda: None # Return unsubscribe function + + with ( + patch( + "homeassistant.components.cloud.async_is_logged_in", + return_value=True, + ), + patch( + "homeassistant.components.cloud.async_active_subscription", + return_value=True, + ), + patch( + "homeassistant.components.cloud.async_is_connected", + return_value=True, + ), + patch( + "homeassistant.components.cloud.async_listen_cloudhook_deletion", + side_effect=mock_listen_cloudhook_deletion, + ), + ): + assert await hass.config_entries.async_setup(config_entry.entry_id) + await hass.async_block_till_done() + assert config_entry.state is ConfigEntryState.LOADED + # Cloudhook should exist + assert CONF_CLOUDHOOK_URL in config_entry.data + # Deletion listener should have been registered + assert cloudhook_deleted_callback is not None + + # Simulate cloudhook deletion by calling the callback + cloudhook_deleted_callback() + await hass.async_block_till_done() + + # Cloudhook should be removed from config entry + assert CONF_CLOUDHOOK_URL not in config_entry.data From df7cacbfb4ba52cdf4bad1d70b551c825def6fe7 Mon Sep 17 00:00:00 2001 From: Timothy <6560631+TimoPtr@users.noreply.github.com> Date: Thu, 20 Nov 2025 15:20:51 +0100 Subject: [PATCH 03/11] Change how we get the cloudhook --- homeassistant/components/cloud/__init__.py | 18 +-- .../components/mobile_app/__init__.py | 32 +++-- .../components/mobile_app/http_api.py | 4 +- homeassistant/components/mobile_app/util.py | 13 +- tests/components/cloud/test_init.py | 81 +++++------ tests/components/mobile_app/test_init.py | 135 ++++++++++++++++-- 6 files changed, 192 insertions(+), 91 deletions(-) diff --git a/homeassistant/components/cloud/__init__.py b/homeassistant/components/cloud/__init__.py index 7b61d434b41f4..df3510246fe8c 100644 --- a/homeassistant/components/cloud/__init__.py +++ b/homeassistant/components/cloud/__init__.py @@ -7,7 +7,7 @@ from datetime import datetime, timedelta from enum import Enum import logging -from typing import cast +from typing import Any, cast from hass_nabucasa import Cloud import voluptuous as vol @@ -246,20 +246,21 @@ async def async_delete_cloudhook(hass: HomeAssistant, webhook_id: str) -> None: @callback -def async_listen_cloudhook_deletion( +def async_listen_cloudhook_change( hass: HomeAssistant, webhook_id: str, - on_deletion: Callable, + on_change: Callable[[dict[str, Any] | None], None], ) -> Callable: - """Listen for cloudhook deletion and call on_deletion when deleted. + """Listen for cloudhook changes for the given webhook and notify when modified or deleted. Args: hass: Home Assistant instance - webhook_id: The webhook ID to monitor - on_deletion: Callback to call when the cloudhook is deleted + webhook_id: The webhook ID to monitor for changes + on_change: Callback invoked when the cloudhook changes. Receives the + cloudhook when updated, or None when deleted. Returns: - Unsubscribe function to stop listening + Callable that unsubscribes from cloudhook change notifications """ _LOGGER.debug("Setting up cloudhook deletion listener for %s", webhook_id) @@ -270,8 +271,7 @@ def async_listen_cloudhook_deletion( async def _async_prefs_updated(prefs: CloudPreferences) -> None: """Handle cloud preferences update.""" if PREF_CLOUDHOOKS in prefs.last_updated: - if webhook_id not in prefs.cloudhooks: - on_deletion() + on_change(prefs.cloudhooks.get(webhook_id)) return hass.data[DATA_CLOUD].client.prefs.async_listen_updates(_async_prefs_updated) diff --git a/homeassistant/components/mobile_app/__init__.py b/homeassistant/components/mobile_app/__init__.py index 91b56d3526d50..030315165396c 100644 --- a/homeassistant/components/mobile_app/__init__.py +++ b/homeassistant/components/mobile_app/__init__.py @@ -141,40 +141,50 @@ def clean_cloudhook() -> None: data.pop(CONF_CLOUDHOOK_URL) hass.config_entries.async_update_entry(entry, data=data) - def on_cloudhook_deletion() -> None: - """Handle cloudhook deletion.""" - _LOGGER.debug("Webhook deleted %s", webhook_id) - data = dict(entry.data) - data.pop(CONF_CLOUDHOOK_URL, None) - hass.config_entries.async_update_entry(entry, data=data) + def on_cloudhook_change(cloudhook: dict[str, Any] | None) -> None: + """Handle cloudhook changes.""" + if cloudhook: + if ( + CONF_CLOUDHOOK_URL in entry.data + and entry.data[CONF_CLOUDHOOK_URL] == cloudhook[CONF_CLOUDHOOK_URL] + ): + return + + hass.config_entries.async_update_entry( + entry, + data={**entry.data, CONF_CLOUDHOOK_URL: cloudhook[CONF_CLOUDHOOK_URL]}, + ) + else: + clean_cloudhook() async def manage_cloudhook(state: cloud.CloudConnectionState) -> None: if ( state is cloud.CloudConnectionState.CLOUD_CONNECTED and CONF_CLOUDHOOK_URL not in entry.data ): - await async_create_cloud_hook(hass, webhook_id, entry) + await async_create_cloud_hook(hass, webhook_id) elif ( state is cloud.CloudConnectionState.CLOUD_DISCONNECTED and not cloud.async_is_logged_in(hass) ): clean_cloudhook() + entry.async_on_unload( + cloud.async_listen_cloudhook_change(hass, webhook_id, on_cloudhook_change) + ) + if cloud.async_is_logged_in(hass): if ( CONF_CLOUDHOOK_URL not in entry.data and cloud.async_active_subscription(hass) and cloud.async_is_connected(hass) ): - await async_create_cloud_hook(hass, webhook_id, entry) + await async_create_cloud_hook(hass, webhook_id) elif CONF_CLOUDHOOK_URL in entry.data: # If we have a cloudhook but no longer logged in to the cloud, remove it from the entry clean_cloudhook() entry.async_on_unload(cloud.async_listen_connection_change(hass, manage_cloudhook)) - entry.async_on_unload( - cloud.async_listen_cloudhook_deletion(hass, webhook_id, on_cloudhook_deletion) - ) await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS) diff --git a/homeassistant/components/mobile_app/http_api.py b/homeassistant/components/mobile_app/http_api.py index f4786b2914cb4..24b0b75e77e6a 100644 --- a/homeassistant/components/mobile_app/http_api.py +++ b/homeassistant/components/mobile_app/http_api.py @@ -70,9 +70,7 @@ async def post(self, request: Request, data: dict) -> Response: webhook_id = secrets.token_hex() if cloud.async_active_subscription(hass): - data[CONF_CLOUDHOOK_URL] = await async_create_cloud_hook( - hass, webhook_id, None - ) + data[CONF_CLOUDHOOK_URL] = await async_create_cloud_hook(hass, webhook_id) data[CONF_WEBHOOK_ID] = webhook_id diff --git a/homeassistant/components/mobile_app/util.py b/homeassistant/components/mobile_app/util.py index f139a203c345b..3d8be941c6a13 100644 --- a/homeassistant/components/mobile_app/util.py +++ b/homeassistant/components/mobile_app/util.py @@ -6,7 +6,6 @@ from typing import TYPE_CHECKING from homeassistant.components import cloud -from homeassistant.config_entries import ConfigEntry from homeassistant.core import HomeAssistant, callback from .const import ( @@ -14,7 +13,6 @@ ATTR_PUSH_TOKEN, ATTR_PUSH_URL, ATTR_PUSH_WEBSOCKET_CHANNEL, - CONF_CLOUDHOOK_URL, DATA_CONFIG_ENTRIES, DATA_DEVICES, DATA_NOTIFY, @@ -63,14 +61,7 @@ def get_notify_service(hass: HomeAssistant, webhook_id: str) -> str | None: _CLOUD_HOOK_LOCK = asyncio.Lock() -async def async_create_cloud_hook( - hass: HomeAssistant, webhook_id: str, entry: ConfigEntry | None -) -> str: +async def async_create_cloud_hook(hass: HomeAssistant, webhook_id: str) -> str: """Create a cloud hook.""" async with _CLOUD_HOOK_LOCK: - hook = await cloud.async_get_or_create_cloudhook(hass, webhook_id) - if entry: - hass.config_entries.async_update_entry( - entry, data={**entry.data, CONF_CLOUDHOOK_URL: hook} - ) - return hook + return await cloud.async_get_or_create_cloudhook(hass, webhook_id) diff --git a/tests/components/cloud/test_init.py b/tests/components/cloud/test_init.py index f7cb796a389a1..37d24b3e7b901 100644 --- a/tests/components/cloud/test_init.py +++ b/tests/components/cloud/test_init.py @@ -11,7 +11,7 @@ CloudNotAvailable, CloudNotConnected, async_get_or_create_cloudhook, - async_listen_cloudhook_deletion, + async_listen_cloudhook_change, async_listen_connection_change, async_remote_ui_url, ) @@ -314,12 +314,12 @@ async def test_cloud_logout( assert cloud.is_logged_in is False -async def test_async_listen_cloudhook_deletion( +async def test_async_listen_cloudhook_change( hass: HomeAssistant, cloud: MagicMock, set_cloud_prefs: Callable[[dict[str, Any]], Coroutine[Any, Any, None]], ) -> None: - """Test async_listen_cloudhook_deletion.""" + """Test async_listen_cloudhook_change.""" assert await async_setup_component(hass, "cloud", {"cloud": {}}) await hass.async_block_till_done() await cloud.login("test-user", "test-pass") @@ -341,47 +341,40 @@ async def test_async_listen_cloudhook_deletion( } ) - # Track if deletion callback was called - deletion_called = False + # Track cloudhook changes + changes = [] - def on_deletion() -> None: - """Handle cloudhook deletion.""" - nonlocal deletion_called - deletion_called = True + def on_change(cloudhook: dict[str, Any] | None) -> None: + """Handle cloudhook change.""" + changes.append(cloudhook) - # Register the deletion listener - unsubscribe = async_listen_cloudhook_deletion(hass, webhook_id, on_deletion) + # Register the change listener + unsubscribe = async_listen_cloudhook_change(hass, webhook_id, on_change) - # Verify deletion callback not called yet - assert not deletion_called + # Verify no changes yet + assert len(changes) == 0 # Delete the cloudhook by updating prefs await set_cloud_prefs({PREF_CLOUDHOOKS: {}}) await hass.async_block_till_done() - # Verify deletion callback was called - assert deletion_called - - # Reset for next test - deletion_called = False + # Verify deletion callback was called with None + assert len(changes) == 1 + assert changes[-1] is None # Add cloudhook back - await set_cloud_prefs( - { - PREF_CLOUDHOOKS: { - webhook_id: { - "webhook_id": webhook_id, - "cloudhook_id": "random-id", - "cloudhook_url": cloudhook_url, - "managed": True, - } - } - } - ) + cloudhook_data = { + "webhook_id": webhook_id, + "cloudhook_id": "random-id", + "cloudhook_url": cloudhook_url, + "managed": True, + } + await set_cloud_prefs({PREF_CLOUDHOOKS: {webhook_id: cloudhook_data}}) await hass.async_block_till_done() - # Verify callback not called when cloudhook is added - assert not deletion_called + # Verify callback called with cloudhook data + assert len(changes) == 2 + assert changes[-1] == cloudhook_data # Unsubscribe from listener unsubscribe() @@ -390,24 +383,24 @@ def on_deletion() -> None: await set_cloud_prefs({PREF_CLOUDHOOKS: {}}) await hass.async_block_till_done() - # Verify deletion callback was NOT called after unsubscribe - assert not deletion_called + # Verify change callback was NOT called after unsubscribe + assert len(changes) == 2 -async def test_async_listen_cloudhook_deletion_no_cloud( +async def test_async_listen_cloudhook_change_no_cloud( hass: HomeAssistant, ) -> None: - """Test async_listen_cloudhook_deletion when cloud is not available.""" + """Test async_listen_cloudhook_change when cloud is not available.""" webhook_id = "mock-webhook-id" - deletion_called = False + change_called = False - def on_deletion() -> None: - """Handle cloudhook deletion.""" - nonlocal deletion_called - deletion_called = True + def on_change(cloudhook: dict[str, Any] | None) -> None: + """Handle cloudhook change.""" + nonlocal change_called + change_called = True # Should return a no-op unsubscribe when cloud is not available - unsubscribe = async_listen_cloudhook_deletion(hass, webhook_id, on_deletion) + unsubscribe = async_listen_cloudhook_change(hass, webhook_id, on_change) # Verify it returns a callable assert callable(unsubscribe) @@ -415,5 +408,5 @@ def on_deletion() -> None: # Call unsubscribe (should not raise) unsubscribe() - # Verify deletion callback was never called - assert not deletion_called + # Verify change callback was never called + assert not change_called diff --git a/tests/components/mobile_app/test_init.py b/tests/components/mobile_app/test_init.py index 0ddec943ac6dd..173f619aecbd4 100644 --- a/tests/components/mobile_app/test_init.py +++ b/tests/components/mobile_app/test_init.py @@ -84,6 +84,14 @@ async def _test_create_cloud_hook( ) config_entry.add_to_hass(hass) + cloudhook_change_callback = None + + def mock_listen_cloudhook_change(hass_instance, wh_id: str, callback): + """Mock the cloudhook change listener.""" + nonlocal cloudhook_change_callback + cloudhook_change_callback = callback + return lambda: None # Return unsubscribe function + with ( patch( "homeassistant.components.cloud.async_active_subscription", @@ -95,6 +103,10 @@ async def _test_create_cloud_hook( "homeassistant.components.cloud.async_get_or_create_cloudhook", autospec=True, ) as mock_async_get_or_create_cloudhook, + patch( + "homeassistant.components.cloud.async_listen_cloudhook_change", + side_effect=mock_listen_cloudhook_change, + ), ): cloud_hook = "https://hook-url" mock_async_get_or_create_cloudhook.return_value = cloud_hook @@ -102,6 +114,21 @@ async def _test_create_cloud_hook( assert await hass.config_entries.async_setup(config_entry.entry_id) await hass.async_block_till_done() assert config_entry.state is ConfigEntryState.LOADED + + # Simulate cloudhook creation by calling the callback, but only if there's no existing cloudhook + if ( + cloudhook_change_callback + and async_active_subscription_return_value + and CONF_CLOUDHOOK_URL not in config_entry.data + ): + cloudhook_change_callback({CONF_CLOUDHOOK_URL: cloud_hook}) + await hass.async_block_till_done() + + # Store the callback for use in additional_steps + hass.data.setdefault("_test", {})["cloudhook_change_callback"] = ( + cloudhook_change_callback + ) + await additional_steps( config_entry, mock_async_get_or_create_cloudhook, cloud_hook ) @@ -180,8 +207,19 @@ async def additional_steps( assert CONF_CLOUDHOOK_URL not in config_entry.data mock_create_cloudhook.assert_not_called() + # Get the callback from test data + cloudhook_change_callback = hass.data.get("_test", {}).get( + "cloudhook_change_callback" + ) + async_mock_cloud_connection_status(hass, True) await hass.async_block_till_done() + + # Simulate cloudhook creation by calling the callback + if cloudhook_change_callback: + cloudhook_change_callback({CONF_CLOUDHOOK_URL: cloud_hook}) + await hass.async_block_till_done() + assert config_entry.data[CONF_CLOUDHOOK_URL] == cloud_hook mock_create_cloudhook.assert_called_once_with( hass, config_entry.data[CONF_WEBHOOK_ID] @@ -360,11 +398,11 @@ async def test_cloudhook_persists_on_disconnect_when_logged_in( assert CONF_CLOUDHOOK_URL in config_entry.data -async def test_cloudhook_deletion_listener( +async def test_cloudhook_change_listener_deletion( hass: HomeAssistant, hass_admin_user: MockUser, ) -> None: - """Test cloudhook deletion listener removes cloudhook from config entry.""" + """Test cloudhook change listener removes cloudhook from config entry on deletion.""" webhook_id = "test-webhook-id" config_entry = MockConfigEntry( data={ @@ -380,12 +418,12 @@ async def test_cloudhook_deletion_listener( ) config_entry.add_to_hass(hass) - cloudhook_deleted_callback = None + cloudhook_change_callback = None - def mock_listen_cloudhook_deletion(hass_instance, wh_id: str, callback): - """Mock the cloudhook deletion listener.""" - nonlocal cloudhook_deleted_callback - cloudhook_deleted_callback = callback + def mock_listen_cloudhook_change(hass_instance, wh_id: str, callback): + """Mock the cloudhook change listener.""" + nonlocal cloudhook_change_callback + cloudhook_change_callback = callback return lambda: None # Return unsubscribe function with ( @@ -402,8 +440,8 @@ def mock_listen_cloudhook_deletion(hass_instance, wh_id: str, callback): return_value=True, ), patch( - "homeassistant.components.cloud.async_listen_cloudhook_deletion", - side_effect=mock_listen_cloudhook_deletion, + "homeassistant.components.cloud.async_listen_cloudhook_change", + side_effect=mock_listen_cloudhook_change, ), ): assert await hass.config_entries.async_setup(config_entry.entry_id) @@ -411,12 +449,83 @@ def mock_listen_cloudhook_deletion(hass_instance, wh_id: str, callback): assert config_entry.state is ConfigEntryState.LOADED # Cloudhook should exist assert CONF_CLOUDHOOK_URL in config_entry.data - # Deletion listener should have been registered - assert cloudhook_deleted_callback is not None + # Change listener should have been registered + assert cloudhook_change_callback is not None - # Simulate cloudhook deletion by calling the callback - cloudhook_deleted_callback() + # Simulate cloudhook deletion by calling the callback with None + cloudhook_change_callback(None) await hass.async_block_till_done() # Cloudhook should be removed from config entry assert CONF_CLOUDHOOK_URL not in config_entry.data + + +async def test_cloudhook_change_listener_update( + hass: HomeAssistant, + hass_admin_user: MockUser, +) -> None: + """Test cloudhook change listener updates cloudhook URL in config entry.""" + webhook_id = "test-webhook-id" + original_url = "https://hook-url" + config_entry = MockConfigEntry( + data={ + **REGISTER_CLEARTEXT, + CONF_WEBHOOK_ID: webhook_id, + ATTR_DEVICE_NAME: "Test", + ATTR_DEVICE_ID: "Test", + CONF_USER_ID: hass_admin_user.id, + CONF_CLOUDHOOK_URL: original_url, + }, + domain=DOMAIN, + title="Test", + ) + config_entry.add_to_hass(hass) + + cloudhook_change_callback = None + + def mock_listen_cloudhook_change(hass_instance, wh_id: str, callback): + """Mock the cloudhook change listener.""" + nonlocal cloudhook_change_callback + cloudhook_change_callback = callback + return lambda: None # Return unsubscribe function + + with ( + patch( + "homeassistant.components.cloud.async_is_logged_in", + return_value=True, + ), + patch( + "homeassistant.components.cloud.async_active_subscription", + return_value=True, + ), + patch( + "homeassistant.components.cloud.async_is_connected", + return_value=True, + ), + patch( + "homeassistant.components.cloud.async_listen_cloudhook_change", + side_effect=mock_listen_cloudhook_change, + ), + ): + assert await hass.config_entries.async_setup(config_entry.entry_id) + await hass.async_block_till_done() + assert config_entry.state is ConfigEntryState.LOADED + # Cloudhook should exist with original URL + assert config_entry.data[CONF_CLOUDHOOK_URL] == original_url + # Change listener should have been registered + assert cloudhook_change_callback is not None + + # Simulate cloudhook URL change + new_url = "https://new-hook-url" + cloudhook_change_callback({CONF_CLOUDHOOK_URL: new_url}) + await hass.async_block_till_done() + + # Cloudhook URL should be updated in config entry + assert config_entry.data[CONF_CLOUDHOOK_URL] == new_url + + # Simulate same URL update (should not trigger update) + cloudhook_change_callback({CONF_CLOUDHOOK_URL: new_url}) + await hass.async_block_till_done() + + # URL should remain the same + assert config_entry.data[CONF_CLOUDHOOK_URL] == new_url From a1ce21437e4f12239c4093d21934fde43771e0a1 Mon Sep 17 00:00:00 2001 From: Timothy <6560631+TimoPtr@users.noreply.github.com> Date: Fri, 21 Nov 2025 10:40:48 +0100 Subject: [PATCH 04/11] Use signal to receive notification of cloudhook changes --- homeassistant/components/cloud/__init__.py | 44 +++++++---------- .../components/mobile_app/__init__.py | 7 +-- .../components/mobile_app/http_api.py | 4 +- homeassistant/components/mobile_app/util.py | 13 ++++- tests/components/cloud/test_init.py | 47 +++++++++++++++---- 5 files changed, 70 insertions(+), 45 deletions(-) diff --git a/homeassistant/components/cloud/__init__.py b/homeassistant/components/cloud/__init__.py index df3510246fe8c..0b790affe6ade 100644 --- a/homeassistant/components/cloud/__init__.py +++ b/homeassistant/components/cloud/__init__.py @@ -69,15 +69,12 @@ DOMAIN, MODE_DEV, MODE_PROD, - PREF_CLOUDHOOKS, ) from .helpers import FixedSizeQueueLogHandler from .prefs import CloudPreferences from .repairs import async_manage_legacy_subscription_issue from .subscription import async_subscription_info -_LOGGER = logging.getLogger(__name__) - DEFAULT_MODE = MODE_PROD PLATFORMS = [Platform.BINARY_SENSOR, Platform.STT, Platform.TTS] @@ -89,6 +86,8 @@ "CLOUD_CONNECTION_STATE" ) +SIGNAL_CLOUDHOOKS_UPDATED: SignalType[dict[str, Any]] = SignalType("CLOUDHOOKS_UPDATED") + STARTUP_REPAIR_DELAY = 1 # 1 hour ALEXA_ENTITY_SCHEMA = vol.Schema( @@ -250,30 +249,17 @@ def async_listen_cloudhook_change( hass: HomeAssistant, webhook_id: str, on_change: Callable[[dict[str, Any] | None], None], -) -> Callable: - """Listen for cloudhook changes for the given webhook and notify when modified or deleted. - - Args: - hass: Home Assistant instance - webhook_id: The webhook ID to monitor for changes - on_change: Callback invoked when the cloudhook changes. Receives the - cloudhook when updated, or None when deleted. - - Returns: - Callable that unsubscribes from cloudhook change notifications - """ - _LOGGER.debug("Setting up cloudhook deletion listener for %s", webhook_id) - - if DATA_CLOUD not in hass.data: - # If cloud is not available, return a no-op unsubscribe - return lambda: None +) -> Callable[[], None]: + """Listen for cloudhook changes for the given webhook and notify when modified or deleted.""" - async def _async_prefs_updated(prefs: CloudPreferences) -> None: - """Handle cloud preferences update.""" - if PREF_CLOUDHOOKS in prefs.last_updated: - on_change(prefs.cloudhooks.get(webhook_id)) + @callback + def _handle_cloudhooks_updated(cloudhooks: dict[str, Any]) -> None: + """Handle cloudhooks updated signal.""" + on_change(cloudhooks.get(webhook_id)) - return hass.data[DATA_CLOUD].client.prefs.async_listen_updates(_async_prefs_updated) + return async_dispatcher_connect( + hass, SIGNAL_CLOUDHOOKS_UPDATED, _handle_cloudhooks_updated + ) @bind_hass @@ -323,7 +309,7 @@ async def _shutdown(event: Event) -> None: hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, _shutdown) - _remote_handle_prefs_updated(cloud) + _handle_prefs_updated(hass, cloud) _setup_services(hass, prefs) async def async_startup_repairs(_: datetime) -> None: @@ -407,8 +393,8 @@ async def _on_initialized() -> None: @callback -def _remote_handle_prefs_updated(cloud: Cloud[CloudClient]) -> None: - """Handle remote preferences updated.""" +def _handle_prefs_updated(hass: HomeAssistant, cloud: Cloud[CloudClient]) -> None: + """Handle preferences updated.""" cur_pref = cloud.client.prefs.remote_enabled lock = asyncio.Lock() @@ -417,6 +403,8 @@ async def remote_prefs_updated(prefs: CloudPreferences) -> None: """Update remote status.""" nonlocal cur_pref + async_dispatcher_send(hass, SIGNAL_CLOUDHOOKS_UPDATED, prefs.cloudhooks) + async with lock: if prefs.remote_enabled == cur_pref: return diff --git a/homeassistant/components/mobile_app/__init__.py b/homeassistant/components/mobile_app/__init__.py index 030315165396c..8683fbda7b3f9 100644 --- a/homeassistant/components/mobile_app/__init__.py +++ b/homeassistant/components/mobile_app/__init__.py @@ -2,7 +2,6 @@ from contextlib import suppress from functools import partial -import logging from typing import Any from homeassistant.auth import EVENT_USER_REMOVED @@ -56,8 +55,6 @@ from .util import async_create_cloud_hook, supports_push from .webhook import handle_webhook -_LOGGER = logging.getLogger(__name__) - PLATFORMS = [Platform.BINARY_SENSOR, Platform.DEVICE_TRACKER, Platform.SENSOR] CONFIG_SCHEMA = cv.empty_config_schema(DOMAIN) @@ -162,7 +159,7 @@ async def manage_cloudhook(state: cloud.CloudConnectionState) -> None: state is cloud.CloudConnectionState.CLOUD_CONNECTED and CONF_CLOUDHOOK_URL not in entry.data ): - await async_create_cloud_hook(hass, webhook_id) + await async_create_cloud_hook(hass, webhook_id, entry) elif ( state is cloud.CloudConnectionState.CLOUD_DISCONNECTED and not cloud.async_is_logged_in(hass) @@ -179,7 +176,7 @@ async def manage_cloudhook(state: cloud.CloudConnectionState) -> None: and cloud.async_active_subscription(hass) and cloud.async_is_connected(hass) ): - await async_create_cloud_hook(hass, webhook_id) + await async_create_cloud_hook(hass, webhook_id, entry) elif CONF_CLOUDHOOK_URL in entry.data: # If we have a cloudhook but no longer logged in to the cloud, remove it from the entry clean_cloudhook() diff --git a/homeassistant/components/mobile_app/http_api.py b/homeassistant/components/mobile_app/http_api.py index 24b0b75e77e6a..f4786b2914cb4 100644 --- a/homeassistant/components/mobile_app/http_api.py +++ b/homeassistant/components/mobile_app/http_api.py @@ -70,7 +70,9 @@ async def post(self, request: Request, data: dict) -> Response: webhook_id = secrets.token_hex() if cloud.async_active_subscription(hass): - data[CONF_CLOUDHOOK_URL] = await async_create_cloud_hook(hass, webhook_id) + data[CONF_CLOUDHOOK_URL] = await async_create_cloud_hook( + hass, webhook_id, None + ) data[CONF_WEBHOOK_ID] = webhook_id diff --git a/homeassistant/components/mobile_app/util.py b/homeassistant/components/mobile_app/util.py index 3d8be941c6a13..f139a203c345b 100644 --- a/homeassistant/components/mobile_app/util.py +++ b/homeassistant/components/mobile_app/util.py @@ -6,6 +6,7 @@ from typing import TYPE_CHECKING from homeassistant.components import cloud +from homeassistant.config_entries import ConfigEntry from homeassistant.core import HomeAssistant, callback from .const import ( @@ -13,6 +14,7 @@ ATTR_PUSH_TOKEN, ATTR_PUSH_URL, ATTR_PUSH_WEBSOCKET_CHANNEL, + CONF_CLOUDHOOK_URL, DATA_CONFIG_ENTRIES, DATA_DEVICES, DATA_NOTIFY, @@ -61,7 +63,14 @@ def get_notify_service(hass: HomeAssistant, webhook_id: str) -> str | None: _CLOUD_HOOK_LOCK = asyncio.Lock() -async def async_create_cloud_hook(hass: HomeAssistant, webhook_id: str) -> str: +async def async_create_cloud_hook( + hass: HomeAssistant, webhook_id: str, entry: ConfigEntry | None +) -> str: """Create a cloud hook.""" async with _CLOUD_HOOK_LOCK: - return await cloud.async_get_or_create_cloudhook(hass, webhook_id) + hook = await cloud.async_get_or_create_cloudhook(hass, webhook_id) + if entry: + hass.config_entries.async_update_entry( + entry, data={**entry.data, CONF_CLOUDHOOK_URL: hook} + ) + return hook diff --git a/tests/components/cloud/test_init.py b/tests/components/cloud/test_init.py index 37d24b3e7b901..6203cddfffb6b 100644 --- a/tests/components/cloud/test_init.py +++ b/tests/components/cloud/test_init.py @@ -387,26 +387,55 @@ def on_change(cloudhook: dict[str, Any] | None) -> None: assert len(changes) == 2 -async def test_async_listen_cloudhook_change_no_cloud( +async def test_async_listen_cloudhook_change_cloud_setup_later( hass: HomeAssistant, + cloud: MagicMock, + set_cloud_prefs: Callable[[dict[str, Any]], Coroutine[Any, Any, None]], ) -> None: - """Test async_listen_cloudhook_change when cloud is not available.""" + """Test async_listen_cloudhook_change works when cloud is set up after listener registration.""" webhook_id = "mock-webhook-id" - change_called = False + cloudhook_url = "https://cloudhook.nabu.casa/abcdefg" + + # Track cloudhook changes + changes: list[dict[str, Any] | None] = [] def on_change(cloudhook: dict[str, Any] | None) -> None: """Handle cloudhook change.""" - nonlocal change_called - change_called = True + changes.append(cloudhook) - # Should return a no-op unsubscribe when cloud is not available + # Register listener BEFORE cloud is set up unsubscribe = async_listen_cloudhook_change(hass, webhook_id, on_change) # Verify it returns a callable assert callable(unsubscribe) - # Call unsubscribe (should not raise) + # No changes yet since cloud isn't set up + assert len(changes) == 0 + + # Now set up cloud + assert await async_setup_component(hass, "cloud", {"cloud": {}}) + await hass.async_block_till_done() + await cloud.login("test-user", "test-pass") + + # Add a cloudhook - this should trigger the listener + cloudhook_data = { + "webhook_id": webhook_id, + "cloudhook_id": "random-id", + "cloudhook_url": cloudhook_url, + "managed": True, + } + await set_cloud_prefs({PREF_CLOUDHOOKS: {webhook_id: cloudhook_data}}) + await hass.async_block_till_done() + + # Verify the listener received the update + assert len(changes) == 1 + assert changes[-1] == cloudhook_data + + # Unsubscribe and verify no more updates unsubscribe() - # Verify change callback was never called - assert not change_called + await set_cloud_prefs({PREF_CLOUDHOOKS: {}}) + await hass.async_block_till_done() + + # Should not receive update after unsubscribe + assert len(changes) == 1 From 12d16c3006414637fe1e261ad2a462e605cfcbc9 Mon Sep 17 00:00:00 2001 From: Timothy <6560631+TimoPtr@users.noreply.github.com> Date: Fri, 21 Nov 2025 12:40:01 +0100 Subject: [PATCH 05/11] Apply small comments --- homeassistant/components/cloud/__init__.py | 8 +++++--- homeassistant/components/mobile_app/__init__.py | 5 +---- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/homeassistant/components/cloud/__init__.py b/homeassistant/components/cloud/__init__.py index 0b790affe6ade..1a2cc323ec295 100644 --- a/homeassistant/components/cloud/__init__.py +++ b/homeassistant/components/cloud/__init__.py @@ -86,7 +86,9 @@ "CLOUD_CONNECTION_STATE" ) -SIGNAL_CLOUDHOOKS_UPDATED: SignalType[dict[str, Any]] = SignalType("CLOUDHOOKS_UPDATED") +_SIGNAL_CLOUDHOOKS_UPDATED: SignalType[dict[str, Any]] = SignalType( + "CLOUDHOOKS_UPDATED" +) STARTUP_REPAIR_DELAY = 1 # 1 hour @@ -258,7 +260,7 @@ def _handle_cloudhooks_updated(cloudhooks: dict[str, Any]) -> None: on_change(cloudhooks.get(webhook_id)) return async_dispatcher_connect( - hass, SIGNAL_CLOUDHOOKS_UPDATED, _handle_cloudhooks_updated + hass, _SIGNAL_CLOUDHOOKS_UPDATED, _handle_cloudhooks_updated ) @@ -403,7 +405,7 @@ async def remote_prefs_updated(prefs: CloudPreferences) -> None: """Update remote status.""" nonlocal cur_pref - async_dispatcher_send(hass, SIGNAL_CLOUDHOOKS_UPDATED, prefs.cloudhooks) + async_dispatcher_send(hass, _SIGNAL_CLOUDHOOKS_UPDATED, prefs.cloudhooks) async with lock: if prefs.remote_enabled == cur_pref: diff --git a/homeassistant/components/mobile_app/__init__.py b/homeassistant/components/mobile_app/__init__.py index 8683fbda7b3f9..65c8296264a53 100644 --- a/homeassistant/components/mobile_app/__init__.py +++ b/homeassistant/components/mobile_app/__init__.py @@ -141,10 +141,7 @@ def clean_cloudhook() -> None: def on_cloudhook_change(cloudhook: dict[str, Any] | None) -> None: """Handle cloudhook changes.""" if cloudhook: - if ( - CONF_CLOUDHOOK_URL in entry.data - and entry.data[CONF_CLOUDHOOK_URL] == cloudhook[CONF_CLOUDHOOK_URL] - ): + if entry.data.get(CONF_CLOUDHOOK_URL) == cloudhook[CONF_CLOUDHOOK_URL]: return hass.config_entries.async_update_entry( From 2f5f9b52cc9e8fc11c04cf1a4b50a4030b69a8b9 Mon Sep 17 00:00:00 2001 From: Timothy <6560631+TimoPtr@users.noreply.github.com> Date: Fri, 21 Nov 2025 12:50:37 +0100 Subject: [PATCH 06/11] Only update when the cloudhooks changes --- homeassistant/components/cloud/__init__.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/homeassistant/components/cloud/__init__.py b/homeassistant/components/cloud/__init__.py index 1a2cc323ec295..549557e5d1570 100644 --- a/homeassistant/components/cloud/__init__.py +++ b/homeassistant/components/cloud/__init__.py @@ -398,16 +398,21 @@ async def _on_initialized() -> None: def _handle_prefs_updated(hass: HomeAssistant, cloud: Cloud[CloudClient]) -> None: """Handle preferences updated.""" cur_pref = cloud.client.prefs.remote_enabled + cur_cloudhooks = cloud.client.prefs.cloudhooks lock = asyncio.Lock() # Sync remote connection with prefs async def remote_prefs_updated(prefs: CloudPreferences) -> None: """Update remote status.""" nonlocal cur_pref - - async_dispatcher_send(hass, _SIGNAL_CLOUDHOOKS_UPDATED, prefs.cloudhooks) + nonlocal cur_cloudhooks async with lock: + if cur_cloudhooks != prefs.cloudhooks: + async_dispatcher_send( + hass, _SIGNAL_CLOUDHOOKS_UPDATED, prefs.cloudhooks + ) + if prefs.remote_enabled == cur_pref: return From 5aaf07be932411976558503f1d5bd546237477ee Mon Sep 17 00:00:00 2001 From: Timothy <6560631+TimoPtr@users.noreply.github.com> Date: Fri, 21 Nov 2025 13:29:59 +0100 Subject: [PATCH 07/11] Apply comments --- tests/components/mobile_app/test_init.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/tests/components/mobile_app/test_init.py b/tests/components/mobile_app/test_init.py index 173f619aecbd4..81086b45003c1 100644 --- a/tests/components/mobile_app/test_init.py +++ b/tests/components/mobile_app/test_init.py @@ -86,7 +86,9 @@ async def _test_create_cloud_hook( cloudhook_change_callback = None - def mock_listen_cloudhook_change(hass_instance, wh_id: str, callback): + def mock_listen_cloudhook_change( + _: HomeAssistant, _webhook_id: str, callback: Callable[[Any], None] + ): """Mock the cloudhook change listener.""" nonlocal cloudhook_change_callback cloudhook_change_callback = callback @@ -115,10 +117,11 @@ def mock_listen_cloudhook_change(hass_instance, wh_id: str, callback): await hass.async_block_till_done() assert config_entry.state is ConfigEntryState.LOADED - # Simulate cloudhook creation by calling the callback, but only if there's no existing cloudhook + assert cloudhook_change_callback is not None + + # Simulate cloudhook creation by calling the callback, but only if there's an acive subscription and not already a cloudhook if ( - cloudhook_change_callback - and async_active_subscription_return_value + async_active_subscription_return_value and CONF_CLOUDHOOK_URL not in config_entry.data ): cloudhook_change_callback({CONF_CLOUDHOOK_URL: cloud_hook}) @@ -216,9 +219,9 @@ async def additional_steps( await hass.async_block_till_done() # Simulate cloudhook creation by calling the callback - if cloudhook_change_callback: - cloudhook_change_callback({CONF_CLOUDHOOK_URL: cloud_hook}) - await hass.async_block_till_done() + assert cloudhook_change_callback is not None + cloudhook_change_callback({CONF_CLOUDHOOK_URL: cloud_hook}) + await hass.async_block_till_done() assert config_entry.data[CONF_CLOUDHOOK_URL] == cloud_hook mock_create_cloudhook.assert_called_once_with( @@ -420,7 +423,9 @@ async def test_cloudhook_change_listener_deletion( cloudhook_change_callback = None - def mock_listen_cloudhook_change(hass_instance, wh_id: str, callback): + def mock_listen_cloudhook_change( + _: HomeAssistant, _webhook_id: str, callback: Callable[[Any], None] + ): """Mock the cloudhook change listener.""" nonlocal cloudhook_change_callback cloudhook_change_callback = callback From 5cf9354f4a31ab94926133bf2c3601be85857f26 Mon Sep 17 00:00:00 2001 From: Timothy <6560631+TimoPtr@users.noreply.github.com> Date: Fri, 21 Nov 2025 13:33:54 +0100 Subject: [PATCH 08/11] Fix spelling --- tests/components/mobile_app/test_init.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/components/mobile_app/test_init.py b/tests/components/mobile_app/test_init.py index 81086b45003c1..f379d44834465 100644 --- a/tests/components/mobile_app/test_init.py +++ b/tests/components/mobile_app/test_init.py @@ -119,7 +119,7 @@ def mock_listen_cloudhook_change( assert cloudhook_change_callback is not None - # Simulate cloudhook creation by calling the callback, but only if there's an acive subscription and not already a cloudhook + # Simulate cloudhook creation by calling the callback, but only if there's an active subscription and not already a cloudhook if ( async_active_subscription_return_value and CONF_CLOUDHOOK_URL not in config_entry.data From 72010288a7e398962ef75e2414fac24e88f4014c Mon Sep 17 00:00:00 2001 From: Timothy <6560631+TimoPtr@users.noreply.github.com> Date: Fri, 21 Nov 2025 13:38:24 +0100 Subject: [PATCH 09/11] Remove the if statement using the side_effect --- tests/components/mobile_app/test_init.py | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/tests/components/mobile_app/test_init.py b/tests/components/mobile_app/test_init.py index f379d44834465..e490044ff8947 100644 --- a/tests/components/mobile_app/test_init.py +++ b/tests/components/mobile_app/test_init.py @@ -94,6 +94,14 @@ def mock_listen_cloudhook_change( cloudhook_change_callback = callback return lambda: None # Return unsubscribe function + cloud_hook = "https://hook-url" + + async def mock_get_or_create_cloudhook(_hass: HomeAssistant, _webhook_id: str): + """Mock creating a cloudhook and trigger the change callback.""" + assert cloudhook_change_callback is not None + cloudhook_change_callback({CONF_CLOUDHOOK_URL: cloud_hook}) + return cloud_hook + with ( patch( "homeassistant.components.cloud.async_active_subscription", @@ -103,30 +111,19 @@ def mock_listen_cloudhook_change( patch("homeassistant.components.cloud.async_is_connected", return_value=True), patch( "homeassistant.components.cloud.async_get_or_create_cloudhook", - autospec=True, + side_effect=mock_get_or_create_cloudhook, ) as mock_async_get_or_create_cloudhook, patch( "homeassistant.components.cloud.async_listen_cloudhook_change", side_effect=mock_listen_cloudhook_change, ), ): - cloud_hook = "https://hook-url" - mock_async_get_or_create_cloudhook.return_value = cloud_hook - assert await hass.config_entries.async_setup(config_entry.entry_id) await hass.async_block_till_done() assert config_entry.state is ConfigEntryState.LOADED assert cloudhook_change_callback is not None - # Simulate cloudhook creation by calling the callback, but only if there's an active subscription and not already a cloudhook - if ( - async_active_subscription_return_value - and CONF_CLOUDHOOK_URL not in config_entry.data - ): - cloudhook_change_callback({CONF_CLOUDHOOK_URL: cloud_hook}) - await hass.async_block_till_done() - # Store the callback for use in additional_steps hass.data.setdefault("_test", {})["cloudhook_change_callback"] = ( cloudhook_change_callback From d6fcf01f453190217582b5398fd12f8313d70a20 Mon Sep 17 00:00:00 2001 From: Timothy <6560631+TimoPtr@users.noreply.github.com> Date: Fri, 21 Nov 2025 14:30:30 +0100 Subject: [PATCH 10/11] Fix and update tests --- homeassistant/components/cloud/__init__.py | 14 ++++++-------- tests/components/cloud/test_init.py | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/homeassistant/components/cloud/__init__.py b/homeassistant/components/cloud/__init__.py index 549557e5d1570..c8683ce1a344e 100644 --- a/homeassistant/components/cloud/__init__.py +++ b/homeassistant/components/cloud/__init__.py @@ -396,22 +396,20 @@ async def _on_initialized() -> None: @callback def _handle_prefs_updated(hass: HomeAssistant, cloud: Cloud[CloudClient]) -> None: - """Handle preferences updated.""" + """Register handler for cloud preferences updates.""" cur_pref = cloud.client.prefs.remote_enabled cur_cloudhooks = cloud.client.prefs.cloudhooks lock = asyncio.Lock() - # Sync remote connection with prefs - async def remote_prefs_updated(prefs: CloudPreferences) -> None: - """Update remote status.""" + async def on_prefs_updated(prefs: CloudPreferences) -> None: + """Handle cloud preferences updates.""" nonlocal cur_pref nonlocal cur_cloudhooks async with lock: if cur_cloudhooks != prefs.cloudhooks: - async_dispatcher_send( - hass, _SIGNAL_CLOUDHOOKS_UPDATED, prefs.cloudhooks - ) + cur_cloudhooks = prefs.cloudhooks + async_dispatcher_send(hass, _SIGNAL_CLOUDHOOKS_UPDATED, cur_cloudhooks) if prefs.remote_enabled == cur_pref: return @@ -421,7 +419,7 @@ async def remote_prefs_updated(prefs: CloudPreferences) -> None: else: await cloud.remote.disconnect() - cloud.client.prefs.async_listen_updates(remote_prefs_updated) + cloud.client.prefs.async_listen_updates(on_prefs_updated) async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: diff --git a/tests/components/cloud/test_init.py b/tests/components/cloud/test_init.py index 6203cddfffb6b..71ff04d9f3a70 100644 --- a/tests/components/cloud/test_init.py +++ b/tests/components/cloud/test_init.py @@ -343,16 +343,20 @@ async def test_async_listen_cloudhook_change( # Track cloudhook changes changes = [] + changeInvoked = False def on_change(cloudhook: dict[str, Any] | None) -> None: """Handle cloudhook change.""" + nonlocal changeInvoked changes.append(cloudhook) + changeInvoked = True # Register the change listener unsubscribe = async_listen_cloudhook_change(hass, webhook_id, on_change) # Verify no changes yet assert len(changes) == 0 + assert changeInvoked is False # Delete the cloudhook by updating prefs await set_cloud_prefs({PREF_CLOUDHOOKS: {}}) @@ -361,6 +365,10 @@ def on_change(cloudhook: dict[str, Any] | None) -> None: # Verify deletion callback was called with None assert len(changes) == 1 assert changes[-1] is None + assert changeInvoked is True + + # Reset changeInvoked to detect next change + changeInvoked = False # Add cloudhook back cloudhook_data = { @@ -375,6 +383,16 @@ def on_change(cloudhook: dict[str, Any] | None) -> None: # Verify callback called with cloudhook data assert len(changes) == 2 assert changes[-1] == cloudhook_data + assert changeInvoked is True + + # Reset changeInvoked to detect next change + changeInvoked = False + + # Update cloudhook data with same cloudhook should not trigger callback + await set_cloud_prefs({PREF_CLOUDHOOKS: {webhook_id: cloudhook_data}}) + await hass.async_block_till_done() + + assert changeInvoked is False # Unsubscribe from listener unsubscribe() @@ -385,6 +403,7 @@ def on_change(cloudhook: dict[str, Any] | None) -> None: # Verify change callback was NOT called after unsubscribe assert len(changes) == 2 + assert changeInvoked is False async def test_async_listen_cloudhook_change_cloud_setup_later( From eb0419e0d482e43d3dfbc811b797ccc605308a7c Mon Sep 17 00:00:00 2001 From: Timothy <6560631+TimoPtr@users.noreply.github.com> Date: Fri, 21 Nov 2025 14:54:57 +0100 Subject: [PATCH 11/11] Apply PR comments --- homeassistant/components/cloud/__init__.py | 9 ++--- tests/components/mobile_app/test_init.py | 40 +++++++++++++--------- 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/homeassistant/components/cloud/__init__.py b/homeassistant/components/cloud/__init__.py index c8683ce1a344e..5377075fe545e 100644 --- a/homeassistant/components/cloud/__init__.py +++ b/homeassistant/components/cloud/__init__.py @@ -397,24 +397,25 @@ async def _on_initialized() -> None: @callback def _handle_prefs_updated(hass: HomeAssistant, cloud: Cloud[CloudClient]) -> None: """Register handler for cloud preferences updates.""" - cur_pref = cloud.client.prefs.remote_enabled + cur_remote_enabled = cloud.client.prefs.remote_enabled cur_cloudhooks = cloud.client.prefs.cloudhooks lock = asyncio.Lock() async def on_prefs_updated(prefs: CloudPreferences) -> None: """Handle cloud preferences updates.""" - nonlocal cur_pref + nonlocal cur_remote_enabled nonlocal cur_cloudhooks + # Lock protects cur_ state variables from concurrent updates async with lock: if cur_cloudhooks != prefs.cloudhooks: cur_cloudhooks = prefs.cloudhooks async_dispatcher_send(hass, _SIGNAL_CLOUDHOOKS_UPDATED, cur_cloudhooks) - if prefs.remote_enabled == cur_pref: + if prefs.remote_enabled == cur_remote_enabled: return - if cur_pref := prefs.remote_enabled: + if cur_remote_enabled := prefs.remote_enabled: await cloud.remote.connect() else: await cloud.remote.disconnect() diff --git a/tests/components/mobile_app/test_init.py b/tests/components/mobile_app/test_init.py index e490044ff8947..7b541f3d27680 100644 --- a/tests/components/mobile_app/test_init.py +++ b/tests/components/mobile_app/test_init.py @@ -68,7 +68,9 @@ async def _test_create_cloud_hook( hass_admin_user: MockUser, additional_config: dict[str, Any], async_active_subscription_return_value: bool, - additional_steps: Callable[[ConfigEntry, Mock, str], Awaitable[None]], + additional_steps: Callable[ + [ConfigEntry, Mock, str, Callable[[Any], None]], Awaitable[None] + ], ) -> None: config_entry = MockConfigEntry( data={ @@ -124,13 +126,11 @@ async def mock_get_or_create_cloudhook(_hass: HomeAssistant, _webhook_id: str): assert cloudhook_change_callback is not None - # Store the callback for use in additional_steps - hass.data.setdefault("_test", {})["cloudhook_change_callback"] = ( - cloudhook_change_callback - ) - await additional_steps( - config_entry, mock_async_get_or_create_cloudhook, cloud_hook + config_entry, + mock_async_get_or_create_cloudhook, + cloud_hook, + cloudhook_change_callback, ) @@ -141,7 +141,10 @@ async def test_create_cloud_hook_on_setup( """Test creating a cloud hook during setup.""" async def additional_steps( - config_entry: ConfigEntry, mock_create_cloudhook: Mock, cloud_hook: str + config_entry: ConfigEntry, + mock_create_cloudhook: Mock, + cloud_hook: str, + cloudhook_change_callback: Callable[[Any], None], ) -> None: assert config_entry.data[CONF_CLOUDHOOK_URL] == cloud_hook mock_create_cloudhook.assert_called_once_with( @@ -161,7 +164,10 @@ async def test_remove_cloudhook( """Test removing a cloud hook when config entry is removed.""" async def additional_steps( - config_entry: ConfigEntry, mock_create_cloudhook: Mock, cloud_hook: str + config_entry: ConfigEntry, + mock_create_cloudhook: Mock, + cloud_hook: str, + cloudhook_change_callback: Callable[[Any], None], ) -> None: webhook_id = config_entry.data[CONF_WEBHOOK_ID] assert config_entry.data[CONF_CLOUDHOOK_URL] == cloud_hook @@ -185,7 +191,10 @@ async def test_create_cloud_hook_aleady_exists( cloud_hook = "https://hook-url-already-exists" async def additional_steps( - config_entry: ConfigEntry, mock_create_cloudhook: Mock, _: str + config_entry: ConfigEntry, + mock_create_cloudhook: Mock, + _: str, + cloudhook_change_callback: Callable[[Any], None], ) -> None: assert config_entry.data[CONF_CLOUDHOOK_URL] == cloud_hook mock_create_cloudhook.assert_not_called() @@ -202,21 +211,18 @@ async def test_create_cloud_hook_after_connection( """Test creating a cloud hook when connected to the cloud.""" async def additional_steps( - config_entry: ConfigEntry, mock_create_cloudhook: Mock, cloud_hook: str + config_entry: ConfigEntry, + mock_create_cloudhook: Mock, + cloud_hook: str, + cloudhook_change_callback: Callable[[Any], None], ) -> None: assert CONF_CLOUDHOOK_URL not in config_entry.data mock_create_cloudhook.assert_not_called() - # Get the callback from test data - cloudhook_change_callback = hass.data.get("_test", {}).get( - "cloudhook_change_callback" - ) - async_mock_cloud_connection_status(hass, True) await hass.async_block_till_done() # Simulate cloudhook creation by calling the callback - assert cloudhook_change_callback is not None cloudhook_change_callback({CONF_CLOUDHOOK_URL: cloud_hook}) await hass.async_block_till_done()