From 6cebe14a69bf0ea19efeaf3ed5c20e5396087216 Mon Sep 17 00:00:00 2001 From: Roee Hendel Date: Tue, 17 Mar 2026 00:54:53 +0200 Subject: [PATCH] fix: merge last_service_data across split calls to fix detect_non_ha_changes with separate_turn_on_commands (#1426) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test: regression test — AL must not override manual brightness with separate_turn_on_commands End-to-end scenario: user adjusts brightness via a directly-bound Zigbee switch (e.g. IKEA RODRET). No HA service call is made; ZHA reports the new brightness via async_update_entity. On the next adaptation interval AL must detect the change and stop overriding the user's brightness. The test verifies the user-visible symptom: after two adaptation cycles following a simulated direct-Zigbee brightness change, the light's brightness must still be the manually set value — not AL's own target. NOTE: this test FAILS on the current code. It is committed here to document the bug before the fix is applied in the next commit. * fix: merge last_service_data across split calls to fix detect_non_ha_changes with separate_turn_on_commands When separate_turn_on_commands=True, each adaptation cycle makes two light.turn_on calls (brightness, then color_temp). Previously each call overwrote last_service_data[light], so after the cycle only the color_temp key remained. _attributes_have_changed() then saw old_brightness=None and silently skipped the brightness comparison, so a manually-set brightness was never detected and AL kept overriding it. Fix: merge instead of overwrite so all split-call attributes accumulate: self.manager.last_service_data[light] = { **self.manager.last_service_data.get(light, {}), **service_data, } * test: add intermediate assertions to regression test Two assertions were promised in the PR description but missing: 1. After the force-adapt, assert that last_service_data contains BOTH brightness AND color — directly proving the merge fix works. 2. After the first non-forced update, assert that BRIGHTNESS is in manual_control — proving detection fired, not just that the final state is right. Co-Authored-By: Claude Sonnet 4.6 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * refactor: remove spurious comments, trim test docstring and assertions Co-Authored-By: Claude Sonnet 4.6 * refactor: strip verbose comments from test, trim assert messages Co-Authored-By: Claude Sonnet 4.6 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * test: add message to bare assert --- custom_components/adaptive_lighting/switch.py | 5 +- tests/test_switch.py | 76 ++++++++++++++++++- 2 files changed, 78 insertions(+), 3 deletions(-) diff --git a/custom_components/adaptive_lighting/switch.py b/custom_components/adaptive_lighting/switch.py index 3923e056..91049970 100644 --- a/custom_components/adaptive_lighting/switch.py +++ b/custom_components/adaptive_lighting/switch.py @@ -1356,7 +1356,10 @@ class AdaptiveSwitch(SwitchEntity, RestoreEntity): data.context.id, ) light = service_data[ATTR_ENTITY_ID] - self.manager.last_service_data[light] = service_data + self.manager.last_service_data[light] = { + **self.manager.last_service_data.get(light, {}), + **service_data, + } await self.hass.services.async_call( LIGHT_DOMAIN, SERVICE_TURN_ON, diff --git a/tests/test_switch.py b/tests/test_switch.py index 5ba3cb60..3da5c967 100644 --- a/tests/test_switch.py +++ b/tests/test_switch.py @@ -9,7 +9,7 @@ from collections import OrderedDict from copy import deepcopy from random import randint from typing import Any -from unittest.mock import Mock, patch +from unittest.mock import AsyncMock, Mock, patch import homeassistant.util.dt as dt_util import pytest @@ -1696,7 +1696,7 @@ async def test_change_switch_settings_service(hass): async def test_cancellable_service_calls_task(hass): """Test the creation and execution of the task that wraps adaptation service calls.""" - (light, *_) = await setup_lights(hass) + light, *_ = await setup_lights(hass) _, switch = await setup_switch(hass, {CONF_SEPARATE_TURN_ON_COMMANDS: True}) context = switch.create_context("test") @@ -2914,3 +2914,75 @@ async def test_adapt_only_on_bare_turn_on_respects_pause_changed_mode(hass, inte f"With take_over_control_mode=PAUSE_CHANGED and only brightness marked " f"as manually controlled, color_temp should still be adapted." ) + + +async def test_detect_non_ha_changes_with_separate_turn_on_commands(hass): + """Regression test for detect_non_ha_changes with separate_turn_on_commands. + + With separate_turn_on_commands=True, each adaptation cycle makes two sequential + light.turn_on calls (brightness, then color). If the second call overwrites + last_service_data instead of merging, brightness is dropped — and + _attributes_have_changed silently skips the brightness comparison, so a direct + Zigbee brightness change is never detected as manual control. + """ + switch, (light, *_) = await setup_lights_and_switch( + hass, + { + CONF_SEPARATE_TURN_ON_COMMANDS: True, + CONF_DETECT_NON_HA_CHANGES: True, + CONF_TAKE_OVER_CONTROL: True, + }, + ) + + context = switch.create_context("test") + + async def update(force: bool = False): + await switch._update_attrs_and_maybe_adapt_lights( + context=context, + force=force, + transition=0, + ) + await hass.async_block_till_done() + + await update(force=True) + + last_sd = switch.manager.last_service_data.get(ENTITY_LIGHT_1) + assert last_sd is not None, "last_service_data not set after force adapt" + assert ( + ATTR_BRIGHTNESS in last_sd + ), f"brightness missing from last_service_data after split calls: {last_sd}" + assert ( + ATTR_COLOR_TEMP_KELVIN in last_sd or ATTR_RGB_COLOR in last_sd + ), f"color missing from last_service_data after split calls: {last_sd}" + + al_brightness = light._brightness + switch.manager.manual_control[ENTITY_LIGHT_1] = LightControlAttributes.NONE + + manual_brightness = ( + al_brightness - 120 if al_brightness >= 120 else al_brightness + 120 + ) + light._brightness = manual_brightness + + async def _flush_attr_state(hass, entity_id): + """Mimic a ZHA attribute report: write current hardware state to HA.""" + light.async_write_ha_state() + + with patch( + "homeassistant.components.adaptive_lighting.switch.async_update_entity", + new=AsyncMock(side_effect=_flush_attr_state), + ): + await update(force=False) + + assert LightControlAttributes.BRIGHTNESS in switch.manager.manual_control.get( + ENTITY_LIGHT_1, + LightControlAttributes.NONE, + ), ( + f"manual_control={switch.manager.manual_control.get(ENTITY_LIGHT_1)}, " + f"last_service_data={switch.manager.last_service_data.get(ENTITY_LIGHT_1)}" + ) + + await update(force=False) + + assert ( + light._brightness == manual_brightness + ), f"AL overrode manual brightness {manual_brightness} with {al_brightness}"