fix: subscribe to lock entity state changes in coordinator to fix autolock#633
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #633 +/- ##
==========================================
+ Coverage 84.14% 90.24% +6.09%
==========================================
Files 10 39 +29
Lines 801 4037 +3236
Branches 0 30 +30
==========================================
+ Hits 674 3643 +2969
- Misses 127 394 +267
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Line-by-Line Code Review — PR #633: fix: subscribe to lock entity state changesSummaryThis PR adds a state change listener to the lock entity so that manual or UI-initiated lock/unlock transitions immediately fire the autolock timer (fixing issue #632). The implementation is clean, test-covered, and focused.
|
| Line | Change | Review |
|---|---|---|
| +1 | _handle_lock_state_change signature |
Clean, matches existing patterns (e.g., _handle_door_state_change). |
| +5 | if not event: return |
Idempotency guard. Avoids errors if a real-time provider event fires first. |
| +7 | changed_entity = event.data["entity_id"] |
Direct access; safe since coordinator already validates event structure in async_setup(). |
| +9-10 | if changed_entity != kmlock.lock_entity_id: return |
Entity filtering. Prevents spurious triggers from neighboring locks. |
| +12-15 | Guarded old_state / new_state extraction |
Handles missing attributes gracefully. |
| +16-18 | Debug logging | Helpful for debugging state transitions. |
| +20 | if new_state == LockState.UNLOCKED: |
State transition handling. |
| +21 | if kmlock.lock_state != LockState.UNLOCKED: |
Idempotency guard. Avoids double-firing if a real-time provider event fires first. |
| +22-26 | await self._lock_unlocked(...) |
Calls existing method with explicit source="state_change" and event_label. |
| +28 | elif new_state == LockState.LOCKED: |
Symmetric handling for the locked state. |
| +29 | if kmlock.lock_state != LockState.LOCKED: |
Idempotency guard for locked state. |
| +30-34 | await self._lock_locked(...) |
Symmetric call to existing method. |
Overall: The method is minimal, readable, and properly guards against race conditions. The use of event_label="State Change Update Unlock/Lock" is consistent with existing telemetry conventions.
+ _LOGGER.debug(
+ "[create_listeners] %s: Creating handle_lock_state_change listener",
+ kmlock.lock_name,
+ )
+ kmlock.listeners.append(
+ async_track_state_change_event(
+ hass=self.hass,
+ entity_ids=kmlock.lock_entity_id,
+ action=functools.partial(self._handle_lock_state_change, kmlock),
+ ),
+ )| Line | Change | Review |
|---|---|---|
| +1 | Debug logging | Good for debugging listener registration. |
| +2-4 | async_track_state_change_event(...) |
Standard Home Assistant listener pattern. |
| +5 | entity_ids=kmlock.lock_entity_id |
Targets the specific lock entity. |
| +6 | action=functools.partial(...) |
Binds kmlock instance, matching how other listeners (door, sensor, etc.) are wired up. |
Overall: Clean, standard HA listener setup. The debug log ensures the registration step is observable during debugging sessions.
tests/test_coordinator.py — Diff Analysis
+ async def test_handle_lock_state_change_unlocked(self, mock_coordinator, mock_kmlock):
+ """Test _handle_lock_state_change triggers _lock_unlocked when lock transitions to UNLOCKED."""
+ mock_kmlock.lock_state = LockState.LOCKED
+ mock_coordinator._lock_unlocked = AsyncMock()
+ mock_coordinator._lock_locked = AsyncMock()
+
+ mock_old = Mock()
+ mock_old.state = LockState.LOCKED
+ mock_new = Mock()
+ mock_new.state = LockState.UNLOCKED
+ event = Mock()
+ event.data = {
+ "entity_id": "lock.front_door",
+ "old_state": mock_old,
+ "new_state": mock_new,
+ }
+
+ await mock_coordinator._handle_lock_state_change(mock_kmlock, event)
+
+ mock_coordinator._lock_unlocked.assert_called_once_with(
+ kmlock=mock_kmlock,
+ source="state_change",
+ event_label="State Change Update Unlock",
+ )
+ mock_coordinator._lock_locked.assert_not_called()| Line | Change | Review |
|---|---|---|
| +1 | test_handle_lock_state_change_unlocked |
Normal transition from LOCKED to UNLOCKED. |
| +3-5 | Setup mock_kmlock.lock_state = LockState.LOCKED |
Sets starting state to LOCKED so the transition is detected. |
| +6-8 | Mock _lock_unlocked and _lock_locked |
Isolates calls. |
| +10-12 | Build event with old_state=LOCKED, new_state=UNLOCKED |
Correct transition data. |
| +14 | Invoke handler | Tests the logic path. |
| +16-20 | Assert _lock_unlocked called with correct args |
Validates that the unlock handler fires with the right parameters. |
| +21 | Assert _lock_locked not called |
Prevents spurious calls. |
Overall: Test verifies the normal unlocking transition path with correct argument propagation.
+ async def test_handle_lock_state_change_locked(self, mock_coordinator, mock_kmlock):
+ """Test _handle_lock_state_change triggers _lock_locked when lock transitions to LOCKED."""
+ mock_kmlock.lock_state = LockState.UNLOCKED
+ mock_coordinator._lock_unlocked = AsyncMock()
+ mock_coordinator._lock_locked = AsyncMock()
+
+ mock_old = Mock()
+ mock_old.state = LockState.UNLOCKED
+ mock_new = Mock()
+ mock_new.state = LockState.LOCKED
+ event = Mock()
+ event.data = {
+ "entity_id": "lock.front_door",
+ "old_state": mock_old,
+ "new_state": mock_new,
+ }
+
+ await mock_coordinator._handle_lock_state_change(mock_kmlock, event)
+
+ mock_coordinator._lock_locked.assert_called_once_with(
+ kmlock=mock_kmlock,
+ source="state_change",
+ event_label="State Change Update Lock",
+ )
+ mock_coordinator._lock_unlocked.assert_not_called()| Line | Change | Review |
|---|---|---|
| +1 | test_handle_lock_state_change_locked |
Normal transition from UNLOCKED to LOCKED. |
| +3 | Set starting state to UNLOCKED |
Enables detection of the transition. |
| +5-6 | Mock handlers | Standard isolation. |
| +8-10 | Transition data | UNLOCKED → LOCKED is valid. |
| +12 | Invoke handler | Tests logic. |
| +14-18 | Assert _lock_locked called with correct args |
Validates lock handler. |
| +19 | Assert _lock_unlocked not called |
Prevents spurious calls. |
Overall: Complements the unlock test with the symmetric lock transition.
+ async def test_handle_lock_state_change_no_change(self, mock_coordinator, mock_kmlock):
+ """Test _handle_lock_state_change does nothing when state has not changed from kmlock's state."""
+ mock_kmlock.lock_state = LockState.UNLOCKED
+ mock_coordinator._lock_unlocked = AsyncMock()
+ mock_coordinator._lock_locked = AsyncMock()
+
+ mock_old = Mock()
+ mock_old.state = LockState.LOCKED
+ mock_new = Mock()
+ mock_new.state = LockState.UNLOCKED
+ event = Mock()
+ event.data = {
+ "entity_id": "lock.front_door",
+ "old_state": mock_old,
+ "new_state": mock_new,
+ }
+
+ await mock_coordinator._handle_lock_state_change(mock_kmlock, event)
+
+ mock_coordinator._lock_unlocked.assert_not_called()
+ mock_coordinator._lock_locked.assert_not_called()| Line | Change | Review |
|---|---|---|
| +1 | test_handle_lock_state_change_no_change |
Tests when state has not changed. |
| +3 | Sets starting state to UNLOCKED. |
|
| +5-6 | Mock handlers | Standard isolation. |
| +8-10 | Transition LOCKED → UNLOCKED |
|
| +12 | Invoke handler | Tests the logic. |
| +14-15 | Assert no calls | Validates idempotency guard against double-firing. |
Overall: Proves the idempotency guard works when a real-time provider event fires first. Excellent test.
+ async def test_handle_lock_state_change_wrong_entity(self, mock_coordinator, mock_kmlock):
+ """Test _handle_lock_state_change does nothing when the entity_id does not match."""
+ mock_kmlock.lock_state = LockState.LOCKED
+ mock_coordinator._lock_unlocked = AsyncMock()
+ mock_coordinator._lock_locked = AsyncMock()
+
+ mock_old = Mock()
+ mock_old.state = LockState.LOCKED
+ mock_new = Mock()
+ mock_new.state = LockState.UNLOCKED
+ event = Mock()
+ event.data = {
+ "entity_id": "lock.wrong_door",
+ "old_state": mock_old,
+ "new_state": mock_new,
+ }
+
+ await mock_coordinator._handle_lock_state_change(mock_kmlock, event)
+
+ mock_coordinator._lock_unlocked.assert_not_called()
+ mock_coordinator._lock_locked.assert_not_called()| Line | Change | Review |
|---|---|---|
| +1 | test_handle_lock_state_change_wrong_entity |
Tests entity filtering. |
| +3 | Sets starting state to LOCKED. |
|
| +5-6 | Mock handlers | Standard isolation. |
| +8-10 | Transition LOCKED → UNLOCKED |
|
| +12 | entity_id="lock.wrong_door" |
Tests that a different lock entity doesn't trigger. |
| +14 | Invoke handler | Tests the logic. |
| +16-17 | Assert no calls | Validates entity filtering. |
Overall: Prevents spurious triggers from neighboring locks. Important edge case covered.
+ async def test_handle_lock_state_change_no_event(self, mock_coordinator, mock_kmlock):
+ """Test _handle_lock_state_change does nothing when event is None."""
+ mock_coordinator._lock_unlocked = AsyncMock()
+ mock_coordinator._lock_locked = AsyncMock()
+
+ await mock_coordinator._handle_lock_state_change(mock_kmlock, None)
+
+ mock_coordinator._lock_unlocked.assert_not_called()
+ mock_coordinator._lock_locked.assert_not_called()| Line | Change | Review |
|---|---|---|
| +1 | test_handle_lock_state_change_no_event |
Tests None event handling. |
| +3-4 | Mock handlers | Standard isolation. |
| +6 | event = None |
Tests the early-exit guard. |
| +8 | Invoke handler | Tests the logic. |
| +10-11 | Assert no calls | Validates that None events are handled gracefully. |
Overall: Proves the if not event: return guard works. Good defensive test.
+ async def test_handle_lock_state_change_missing_states(self, mock_coordinator, mock_kmlock):
+ """Test _handle_lock_state_change handles missing old_state and new_state gracefully."""
+ mock_coordinator._lock_unlocked = AsyncMock()
+ mock_coordinator._lock_locked = AsyncMock()
+
+ event = Mock()
+ event.data = {
+ "entity_id": "lock.front_door",
+ }
+
+ await mock_coordinator._handle_lock_state_change(mock_kmlock, event)
+
+ mock_coordinator._lock_unlocked.assert_not_called()
+ mock_coordinator._lock_locked.assert_not_called()| Line | Change | Review |
|---|---|---|
| +1 | test_handle_lock_state_change_missing_states |
Tests missing state attributes. |
| +3-4 | Mock handlers | Standard isolation. |
| +6-9 | Build event with only entity_id |
|
| +11 | Invoke handler | Tests the logic. |
| +13-14 | Assert no calls | Validates that missing states don't cause errors. |
Overall: Proves guarded attribute extraction doesn't crash. Good defensive test.
+ async def test_handle_lock_state_change_no_change_locked(self, mock_coordinator, mock_kmlock):
+ """Test _handle_lock_state_change does nothing when lock is already locked."""
+ mock_kmlock.lock_state = LockState.LOCKED
+ mock_coordinator._lock_unlocked = AsyncMock()
+ mock_coordinator._lock_locked = AsyncMock()
+
+ mock_old = Mock()
+ mock_old.state = LockState.UNLOCKED
+ mock_new = Mock()
+ mock_new.state = LockState.LOCKED
+ event = Mock()
+ event.data = {
+ "entity_id": "lock.front_door",
+ "old_state": mock_old,
+ "new_state": mock_new,
+ }
+
+ await mock_coordinator._handle_lock_state_change(mock_kmlock, event)
+
+ mock_coordinator._lock_unlocked.assert_not_called()
+ mock_coordinator._lock_locked.assert_not_called()| Line | Change | Review |
|---|---|---|
| +1 | test_handle_lock_state_change_no_change_locked |
Tests idempotency guard when already locked. |
| +3 | Sets starting state to LOCKED. |
|
| +5-6 | Mock handlers | Standard isolation. |
| +8-10 | Transition UNLOCKED → LOCKED |
|
| +12 | Invoke handler | Tests the logic. |
| +14-15 | Assert no calls | Validates idempotency guard prevents double-firing. |
Overall: Proves the idempotency guard works when transitioning back to LOCKED. Complements test_handle_lock_state_change_no_change. Excellent test.
Overall Assessment
✅ LGTM
- The PR is clean, focused, and directly addresses issue ISSUE: Auto Lock not working #632.
- The implementation is minimal, readable, and well-tested.
- All edge cases are covered: normal transitions, idempotency guards, wrong-entity filtering,
Noneevents, missing attributes. - The code follows existing patterns and conventions.
No changes required. Ready to merge.
Summary
This PR fixes an issue where manual or Home Assistant UI-initiated lock/unlock transitions did not trigger the Auto Lock timer immediately (Issue #632).
Proposed change
kmlock.lock_entity_idstate change events in_create_listeners()._handle_lock_state_change()method in the coordinator to handle those transitions.UNLOCKEDorLOCKEDoccurs, it immediately fires_lock_unlocked()or_lock_locked()respectively._lock_unlocked()checksif kmlock.lock_state == LockState.UNLOCKED:and returns early, this handles deduplication cleanly if a real-time provider event fires first.tests/test_coordinator.pyvalidating correct invocation on unlocking, locking, no-change state transitions, and non-matching entity IDs.Type of change
Additional information