Feedback from previous PR (#63022)

homeassistant/components/unifiprotect/media_player.py

@@ -1,7 +1,6 @@
 """Support for Ubiquiti's UniFi Protect NVR."""
 from __future__ import annotations
 
-from collections.abc import Callable, Sequence
 import logging
 from typing import Any
 
@@ -16,7 +15,6 @@ from homeassistant.components.media_player import (
 from homeassistant.components.media_player.const import (
     MEDIA_TYPE_MUSIC,
     SUPPORT_PLAY_MEDIA,
-    SUPPORT_SELECT_SOURCE,
     SUPPORT_STOP,
     SUPPORT_VOLUME_SET,
     SUPPORT_VOLUME_STEP,
@@ -24,7 +22,8 @@ from homeassistant.components.media_player.const import (
 from homeassistant.config_entries import ConfigEntry
 from homeassistant.const import STATE_IDLE, STATE_PLAYING
 from homeassistant.core import HomeAssistant, callback
-from homeassistant.helpers.entity import Entity
+from homeassistant.exceptions import HomeAssistantError
+from homeassistant.helpers.entity_platform import AddEntitiesCallback
 
 from .const import DOMAIN
 from .data import ProtectData
@@ -36,7 +35,7 @@ _LOGGER = logging.getLogger(__name__)
 async def async_setup_entry(
     hass: HomeAssistant,
     entry: ConfigEntry,
-    async_add_entities: Callable[[Sequence[Entity]], None],
+    async_add_entities: AddEntitiesCallback,
 ) -> None:
     """Discover cameras with speakers on a UniFi Protect NVR."""
     data: ProtectData = hass.data[DOMAIN][entry.entry_id]
@@ -71,11 +70,7 @@ class ProtectMediaPlayer(ProtectDeviceEntity, MediaPlayerEntity):
 
         self._attr_name = f"{self.device.name} Speaker"
         self._attr_supported_features = (
-            SUPPORT_PLAY_MEDIA
+            SUPPORT_PLAY_MEDIA | SUPPORT_VOLUME_SET | SUPPORT_VOLUME_STEP | SUPPORT_STOP
-            | SUPPORT_VOLUME_SET
-            | SUPPORT_VOLUME_STEP
-            | SUPPORT_STOP
-            | SUPPORT_SELECT_SOURCE
         )
         self._attr_media_content_type = MEDIA_TYPE_MUSIC
 
@@ -92,7 +87,6 @@ class ProtectMediaPlayer(ProtectDeviceEntity, MediaPlayerEntity):
         else:
             self._attr_state = STATE_IDLE
 
-    @callback
     async def async_set_volume_level(self, volume: float) -> None:
         """Set volume level, range 0..1."""
 
@@ -116,20 +110,14 @@ class ProtectMediaPlayer(ProtectDeviceEntity, MediaPlayerEntity):
         """Play a piece of media."""
 
         if media_type != MEDIA_TYPE_MUSIC:
-            _LOGGER.warning(
+            raise ValueError("Only music media type is supported")
-                "%s: Cannot play media type of %s, only `%s` supported",
-                self.device.name,
-                media_type,
-                MEDIA_TYPE_MUSIC,
-            )
-            return
 
         _LOGGER.debug("Playing Media %s for %s Speaker", media_id, self.device.name)
         await self.async_media_stop()
         try:
             await self.device.play_audio(media_id, blocking=False)
         except StreamError as err:
-            _LOGGER.error("Error while playing media: %s", err)
+            raise HomeAssistantError from err
         else:
             # update state after starting player
             self._async_updated_event()

tests/components/unifiprotect/test_media_player.py

@@ -1,4 +1,4 @@
-"""Test the UniFi Protect button platform."""
+"""Test the UniFi Protect media_player platform."""
 # pylint: disable=protected-access
 from __future__ import annotations
 
@@ -23,6 +23,7 @@ from homeassistant.const import (
     Platform,
 )
 from homeassistant.core import HomeAssistant
+from homeassistant.exceptions import HomeAssistantError
 from homeassistant.helpers import entity_registry as er
 
 from .conftest import MockEntityFixture
@@ -85,7 +86,7 @@ async def test_media_player_setup(
     assert state
     assert state.state == STATE_IDLE
     assert state.attributes[ATTR_ATTRIBUTION] == DEFAULT_ATTRIBUTION
-    assert state.attributes[ATTR_SUPPORTED_FEATURES] == 7684
+    assert state.attributes[ATTR_SUPPORTED_FEATURES] == 5636
     assert state.attributes[ATTR_MEDIA_CONTENT_TYPE] == "music"
     assert state.attributes[ATTR_MEDIA_VOLUME_LEVEL] == expected_volume
 
@@ -201,16 +202,17 @@ async def test_media_player_play_invalid(
     camera[0].__fields__["play_audio"] = Mock()
     camera[0].play_audio = AsyncMock()
 
-    await hass.services.async_call(
+    with pytest.raises(ValueError):
-        "media_player",
+        await hass.services.async_call(
-        "play_media",
+            "media_player",
-        {
+            "play_media",
-            ATTR_ENTITY_ID: camera[1],
+            {
-            "media_content_id": "/test.png",
+                ATTR_ENTITY_ID: camera[1],
-            "media_content_type": "image",
+                "media_content_id": "/test.png",
-        },
+                "media_content_type": "image",
-        blocking=True,
+            },
-    )
+            blocking=True,
+        )
 
     assert not camera[0].play_audio.called
 
@@ -226,16 +228,17 @@ async def test_media_player_play_error(
     camera[0].play_audio = AsyncMock(side_effect=StreamError)
     camera[0].wait_until_audio_completes = AsyncMock()
 
-    await hass.services.async_call(
+    with pytest.raises(HomeAssistantError):
-        "media_player",
+        await hass.services.async_call(
-        "play_media",
+            "media_player",
-        {
+            "play_media",
-            ATTR_ENTITY_ID: camera[1],
+            {
-            "media_content_id": "/test.mp3",
+                ATTR_ENTITY_ID: camera[1],
-            "media_content_type": "music",
+                "media_content_id": "/test.mp3",
-        },
+                "media_content_type": "music",
-        blocking=True,
+            },
-    )
+            blocking=True,
+        )
 
     assert camera[0].play_audio.called
     assert not camera[0].wait_until_audio_completes.called