Fix swallowed exceptions in action handlers for Slack (#177049)

homeassistant/components/slack/notify.py

@@ -20,6 +20,7 @@ from homeassistant.components.notify import (
 )
 from homeassistant.const import ATTR_ICON, CONF_PATH
 from homeassistant.core import HomeAssistant, callback
+from homeassistant.exceptions import ServiceValidationError
 from homeassistant.helpers import aiohttp_client, config_validation as cv, template
 from homeassistant.helpers.typing import ConfigType, DiscoveryInfoType
 
@@ -34,6 +35,7 @@ from .const import (
     ATTR_USERNAME,
     CONF_DEFAULT_CHANNEL,
     DATA_CLIENT,
+    DOMAIN,
     SLACK_DATA,
 )
 from .utils import upload_file_to_slack
@@ -278,10 +280,12 @@ class SlackNotificationService(BaseNotificationService):
 
         try:
             DATA_SCHEMA(data)
-        # pylint: disable-next=home-assistant-action-swallowed-exception
         except vol.Invalid as err:
-            _LOGGER.error("Invalid message data: %s", err)
-            data = {}
+            raise ServiceValidationError(
+                translation_domain=DOMAIN,
+                translation_key="invalid_message_data",
+                translation_placeholders={"error": str(err)},
+            ) from err
 
         title = kwargs.get(ATTR_TITLE)
         targets = _async_sanitize_channel_names(

homeassistant/components/slack/strings.json

@@ -32,5 +32,10 @@
         "name": "Do not disturb until"
       }
     }
+  },
+  "exceptions": {
+    "invalid_message_data": {
+      "message": "Invalid message data: {error}"
+    }
   }
 }

tests/components/slack/test_notify.py

@@ -1,110 +1,138 @@
 """Test slack notifications."""
 
-from unittest.mock import AsyncMock, Mock
+from unittest.mock import AsyncMock
+
+import pytest
 
 from homeassistant.components import notify
 from homeassistant.components.slack import DOMAIN
-from homeassistant.components.slack.notify import (
-    ATTR_THREAD_TS,
-    CONF_DEFAULT_CHANNEL,
-    SlackNotificationService,
+from homeassistant.components.slack.notify import ATTR_THREAD_TS
+from homeassistant.const import ATTR_ICON
+from homeassistant.core import HomeAssistant
+from homeassistant.exceptions import ServiceValidationError
+
+from . import CONF_DATA, TEAM_ID, mock_connection
+
+from tests.common import MockConfigEntry
+from tests.test_util.aiohttp import AiohttpClientMocker
+
+SERVICE_NAME = "test_team"
+
+
+async def _async_setup_notify_service(
+    hass: HomeAssistant,
+    aioclient_mock: AiohttpClientMocker,
+    entry_data: dict[str, str],
+) -> AsyncMock:
+    """Set up the slack integration and mock the message client."""
+    entry = MockConfigEntry(domain=DOMAIN, data=entry_data, unique_id=TEAM_ID)
+    entry.add_to_hass(hass)
+    mock_connection(aioclient_mock)
+    await hass.config_entries.async_setup(entry.entry_id)
+    await hass.async_block_till_done()
+    assert hass.services.has_service(notify.DOMAIN, SERVICE_NAME)
+    mock_fn = AsyncMock()
+    entry.runtime_data.client.chat_postMessage = mock_fn
+    return mock_fn
+
+
+@pytest.mark.parametrize(
+    ("entry_icon", "service_data", "expected_key", "expected_icon"),
+    [
+        pytest.param(
+            ":robot_face:",
+            {notify.ATTR_MESSAGE: "test"},
+            "icon_emoji",
+            ":robot_face:",
+            id="default_emoji",
+        ),
+        pytest.param(
+            "default_icon",
+            {notify.ATTR_MESSAGE: "test", notify.ATTR_DATA: {ATTR_ICON: ":new:"}},
+            "icon_emoji",
+            ":new:",
+            id="emoji_overrides_default",
+        ),
+        pytest.param(
+            "https://example.com/hass.png",
+            {notify.ATTR_MESSAGE: "test"},
+            "icon_url",
+            "https://example.com/hass.png",
+            id="default_icon_url",
+        ),
+        pytest.param(
+            "default_icon",
+            {
+                notify.ATTR_MESSAGE: "test",
+                notify.ATTR_DATA: {ATTR_ICON: "https://example.com/hass.png"},
+            },
+            "icon_url",
+            "https://example.com/hass.png",
+            id="icon_url_overrides_default",
+        ),
+    ],
 )
-from homeassistant.const import ATTR_ICON, CONF_API_KEY, CONF_NAME, CONF_PLATFORM
-
-from . import CONF_DATA
-
-SERVICE_NAME = f"notify_{DOMAIN}"
-
-DEFAULT_CONFIG = {
-    notify.DOMAIN: [
-        {
-            CONF_PLATFORM: DOMAIN,
-            CONF_NAME: SERVICE_NAME,
-            CONF_API_KEY: "12345",
-            CONF_DEFAULT_CHANNEL: "channel",
-        }
-    ]
-}
-
-
-async def test_message_includes_default_emoji() -> None:
-    """Tests that default icon is used when no message icon is given."""
-    mock_client = Mock()
-    mock_client.chat_postMessage = AsyncMock()
-    expected_icon = ":robot_face:"
-    service = SlackNotificationService(
-        None, mock_client, CONF_DATA | {ATTR_ICON: expected_icon}
+async def test_message_icon(
+    hass: HomeAssistant,
+    aioclient_mock: AiohttpClientMocker,
+    entry_icon: str,
+    service_data: dict[str, str],
+    expected_key: str,
+    expected_icon: str,
+) -> None:
+    """Test that the message icon comes from the entry data or the service data."""
+    mock_fn = await _async_setup_notify_service(
+        hass, aioclient_mock, CONF_DATA | {ATTR_ICON: entry_icon}
     )
 
-    await service.async_send_message("test")
-
-    mock_fn = mock_client.chat_postMessage
-    mock_fn.assert_called_once()
-    _, kwargs = mock_fn.call_args
-    assert kwargs["icon_emoji"] == expected_icon
-
-
-async def test_message_emoji_overrides_default() -> None:
-    """Tests that overriding the default icon emoji when sending a message works."""
-    mock_client = Mock()
-    mock_client.chat_postMessage = AsyncMock()
-    service = SlackNotificationService(
-        None, mock_client, CONF_DATA | {ATTR_ICON: "default_icon"}
+    await hass.services.async_call(
+        notify.DOMAIN, SERVICE_NAME, service_data, blocking=True
     )
 
-    expected_icon = ":new:"
-    await service.async_send_message("test", data={"icon": expected_icon})
-
-    mock_fn = mock_client.chat_postMessage
     mock_fn.assert_called_once()
     _, kwargs = mock_fn.call_args
-    assert kwargs["icon_emoji"] == expected_icon
+    assert kwargs[expected_key] == expected_icon
 
 
-async def test_message_includes_default_icon_url() -> None:
-    """Tests that overriding the default icon url when sending a message works."""
-    mock_client = Mock()
-    mock_client.chat_postMessage = AsyncMock()
-    expected_icon = "https://example.com/hass.png"
-    service = SlackNotificationService(
-        None, mock_client, CONF_DATA | {ATTR_ICON: expected_icon}
-    )
-
-    await service.async_send_message("test")
-
-    mock_fn = mock_client.chat_postMessage
-    mock_fn.assert_called_once()
-    _, kwargs = mock_fn.call_args
-    assert kwargs["icon_url"] == expected_icon
-
-
-async def test_message_icon_url_overrides_default() -> None:
-    """Tests that overriding the default icon url when sending a message works."""
-    mock_client = Mock()
-    mock_client.chat_postMessage = AsyncMock()
-    service = SlackNotificationService(
-        None, mock_client, CONF_DATA | {ATTR_ICON: "default_icon"}
-    )
-
-    expected_icon = "https://example.com/hass.png"
-    await service.async_send_message("test", data={ATTR_ICON: expected_icon})
-
-    mock_fn = mock_client.chat_postMessage
-    mock_fn.assert_called_once()
-    _, kwargs = mock_fn.call_args
-    assert kwargs["icon_url"] == expected_icon
-
-
-async def test_message_as_reply() -> None:
+async def test_message_as_reply(
+    hass: HomeAssistant, aioclient_mock: AiohttpClientMocker
+) -> None:
     """Tests that a message pointer will be passed to Slack if specified."""
-    mock_client = Mock()
-    mock_client.chat_postMessage = AsyncMock()
-    service = SlackNotificationService(None, mock_client, CONF_DATA)
+    mock_fn = await _async_setup_notify_service(hass, aioclient_mock, CONF_DATA)
 
     expected_ts = "1624146685.064129"
-    await service.async_send_message("test", data={ATTR_THREAD_TS: expected_ts})
+    await hass.services.async_call(
+        notify.DOMAIN,
+        SERVICE_NAME,
+        {
+            notify.ATTR_MESSAGE: "test",
+            notify.ATTR_DATA: {ATTR_THREAD_TS: expected_ts},
+        },
+        blocking=True,
+    )
 
-    mock_fn = mock_client.chat_postMessage
     mock_fn.assert_called_once()
     _, kwargs = mock_fn.call_args
     assert kwargs["thread_ts"] == expected_ts
+
+
+async def test_invalid_message_data(
+    hass: HomeAssistant, aioclient_mock: AiohttpClientMocker
+) -> None:
+    """Tests that invalid message data raises an error and sends no message."""
+    mock_fn = await _async_setup_notify_service(hass, aioclient_mock, CONF_DATA)
+
+    with pytest.raises(ServiceValidationError) as exc_info:
+        await hass.services.async_call(
+            notify.DOMAIN,
+            SERVICE_NAME,
+            {
+                notify.ATTR_MESSAGE: "test",
+                notify.ATTR_DATA: {"not_a_valid_key": "value"},
+            },
+            blocking=True,
+        )
+
+    assert exc_info.value.translation_domain == DOMAIN
+    assert exc_info.value.translation_key == "invalid_message_data"
+    mock_fn.assert_not_called()