Fix tplink light effect behaviour when activating a scene (#121288)

homeassistant/components/tplink/light.py

@@ -382,7 +382,12 @@ def _async_update_attrs(self) -> None:
     async def async_turn_on(self, **kwargs: Any) -> None:
         """Turn the light on."""
         brightness, transition = self._async_extract_brightness_transition(**kwargs)
-        if ATTR_EFFECT in kwargs:
+        if (
+            (effect := kwargs.get(ATTR_EFFECT))
+            # Effect is unlikely to be LIGHT_EFFECTS_OFF but check for it anyway
+            and effect not in {LightEffect.LIGHT_EFFECTS_OFF, EFFECT_OFF}
+            and effect in self._effect_module.effect_list
+        ):
             await self._effect_module.set_effect(
                 kwargs[ATTR_EFFECT], brightness=brightness, transition=transition
             )

tests/components/tplink/__init__.py

@@ -210,7 +210,8 @@ def _mocked_device(
 
     if modules:
         device.modules = {
-            module_name: MODULE_TO_MOCK_GEN[module_name]() for module_name in modules
+            module_name: MODULE_TO_MOCK_GEN[module_name](device)
+            for module_name in modules
         }
 
     if features:
@@ -298,7 +299,7 @@ def _mocked_feature(
     return feature
 
 
-def _mocked_light_module() -> Light:
+def _mocked_light_module(device) -> Light:
     light = MagicMock(spec=Light, name="Mocked light module")
     light.update = AsyncMock()
     light.brightness = 50
@@ -314,26 +315,58 @@ def _mocked_light_module() -> Light:
     light.hsv = (10, 30, 5)
     light.valid_temperature_range = ColorTempRange(min=4000, max=9000)
     light.hw_info = {"sw_ver": "1.0.0", "hw_ver": "1.0.0"}
-    light.set_state = AsyncMock()
-    light.set_brightness = AsyncMock()
-    light.set_hsv = AsyncMock()
-    light.set_color_temp = AsyncMock()
+
+    async def _set_state(state, *_, **__):
+        light.state = state
+
+    light.set_state = AsyncMock(wraps=_set_state)
+
+    async def _set_brightness(brightness, *_, **__):
+        light.state.brightness = brightness
+        light.state.light_on = brightness > 0
+
+    light.set_brightness = AsyncMock(wraps=_set_brightness)
+
+    async def _set_hsv(h, s, v, *_, **__):
+        light.state.hue = h
+        light.state.saturation = s
+        light.state.brightness = v
+        light.state.light_on = True
+
+    light.set_hsv = AsyncMock(wraps=_set_hsv)
+
+    async def _set_color_temp(temp, *_, **__):
+        light.state.color_temp = temp
+        light.state.light_on = True
+
+    light.set_color_temp = AsyncMock(wraps=_set_color_temp)
     light.protocol = _mock_protocol()
     return light
 
 
-def _mocked_light_effect_module() -> LightEffect:
+def _mocked_light_effect_module(device) -> LightEffect:
     effect = MagicMock(spec=LightEffect, name="Mocked light effect")
     effect.has_effects = True
     effect.has_custom_effects = True
     effect.effect = "Effect1"
     effect.effect_list = ["Off", "Effect1", "Effect2"]
-    effect.set_effect = AsyncMock()
+
+    async def _set_effect(effect_name, *_, **__):
+        assert (
+            effect_name in effect.effect_list
+        ), f"set_effect '{effect_name}' not in {effect.effect_list}"
+        assert device.modules[
+            Module.Light
+        ], "Need a light module to test set_effect method"
+        device.modules[Module.Light].state.light_on = True
+        effect.effect = effect_name
+
+    effect.set_effect = AsyncMock(wraps=_set_effect)
     effect.set_custom_effect = AsyncMock()
     return effect
 
 
-def _mocked_fan_module() -> Fan:
+def _mocked_fan_module(effect) -> Fan:
     fan = MagicMock(auto_spec=Fan, name="Mocked fan")
     fan.fan_speed_level = 0
     fan.set_fan_speed_level = AsyncMock()

tests/components/tplink/test_light.py

@@ -5,6 +5,7 @@
 from datetime import timedelta
 from unittest.mock import MagicMock, PropertyMock
 
+from freezegun.api import FrozenDateTimeFactory
 from kasa import (
     AuthenticationError,
     DeviceType,
@@ -36,7 +37,13 @@
 )
 from homeassistant.components.tplink.const import DOMAIN
 from homeassistant.config_entries import SOURCE_REAUTH
-from homeassistant.const import ATTR_ENTITY_ID, CONF_HOST, STATE_OFF, STATE_ON
+from homeassistant.const import (
+    ATTR_ENTITY_ID,
+    CONF_HOST,
+    STATE_OFF,
+    STATE_ON,
+    STATE_UNKNOWN,
+)
 from homeassistant.core import HomeAssistant
 from homeassistant.exceptions import HomeAssistantError
 from homeassistant.helpers import entity_registry as er
@@ -920,3 +927,82 @@ async def test_light_child(
         assert child_entity
         assert child_entity.unique_id == f"{DEVICE_ID}0{light_id}"
         assert child_entity.device_id == entity.device_id
+
+
+async def test_scene_effect_light(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+) -> None:
+    """Test activating a scene works with effects.
+
+    i.e. doesn't try to set the effect to 'off'
+    """
+    already_migrated_config_entry = MockConfigEntry(
+        domain=DOMAIN, data={CONF_HOST: "127.0.0.1"}, unique_id=MAC_ADDRESS
+    )
+    already_migrated_config_entry.add_to_hass(hass)
+    device = _mocked_device(
+        modules=[Module.Light, Module.LightEffect], alias="my_light"
+    )
+    light_effect = device.modules[Module.LightEffect]
+    light_effect.effect = LightEffect.LIGHT_EFFECTS_OFF
+
+    with _patch_discovery(device=device), _patch_connect(device=device):
+        assert await async_setup_component(hass, tplink.DOMAIN, {tplink.DOMAIN: {}})
+        assert await async_setup_component(hass, "scene", {})
+        await hass.async_block_till_done()
+
+    entity_id = "light.my_light"
+
+    await hass.services.async_call(
+        LIGHT_DOMAIN, "turn_on", {ATTR_ENTITY_ID: entity_id}, blocking=True
+    )
+    await hass.async_block_till_done()
+    freezer.tick(5)
+    async_fire_time_changed(hass)
+    await hass.async_block_till_done()
+
+    state = hass.states.get(entity_id)
+    assert state.state is STATE_ON
+    assert state.attributes["effect"] is EFFECT_OFF
+
+    await hass.services.async_call(
+        "scene",
+        "create",
+        {"scene_id": "effect_off_scene", "snapshot_entities": [entity_id]},
+        blocking=True,
+    )
+    await hass.async_block_till_done()
+    scene_state = hass.states.get("scene.effect_off_scene")
+    assert scene_state.state is STATE_UNKNOWN
+
+    await hass.services.async_call(
+        LIGHT_DOMAIN, "turn_off", {ATTR_ENTITY_ID: entity_id}, blocking=True
+    )
+    await hass.async_block_till_done()
+    freezer.tick(5)
+    async_fire_time_changed(hass)
+    await hass.async_block_till_done()
+
+    state = hass.states.get(entity_id)
+    assert state.state is STATE_OFF
+
+    await hass.services.async_call(
+        "scene",
+        "turn_on",
+        {
+            "entity_id": "scene.effect_off_scene",
+        },
+        blocking=True,
+    )
+    await hass.async_block_till_done()
+    scene_state = hass.states.get("scene.effect_off_scene")
+    assert scene_state.state is not STATE_UNKNOWN
+
+    freezer.tick(5)
+    async_fire_time_changed(hass)
+    await hass.async_block_till_done()
+
+    state = hass.states.get(entity_id)
+    assert state.state is STATE_ON
+    assert state.attributes["effect"] is EFFECT_OFF