Skip to content

fix: track lock _notifications_enabled state on enable#497

Draft
bluetoothbot wants to merge 2 commits into
sblibs:masterfrom
bluetoothbot:koan/lock-notifications-state-tracking
Draft

fix: track lock _notifications_enabled state on enable#497
bluetoothbot wants to merge 2 commits into
sblibs:masterfrom
bluetoothbot:koan/lock-notifications-state-tracking

Conversation

@bluetoothbot
Copy link
Copy Markdown
Collaborator

@bluetoothbot bluetoothbot commented May 15, 2026

What

Set _notifications_enabled = True on a successful _enable_notifications call (and skip the BLE write when already enabled). Tighten the existing test and add a failure-path test.

Why

SwitchbotLock._enable_notifications only returned the command result — it never updated self._notifications_enabled. The flag therefore stayed False for the lifetime of the device. That has two consequences:

  • _disable_notifications early-returns on if not self._notifications_enabled: return True, so the disable command would never actually be sent to the lock.
  • Every lock() / unlock() / unlock_without_unlatch() / half_lock() re-issues the enable command rather than recognising notifications are already on.

Surfaced while looking at #489 (lock event subscription). The notification-tracking gap is unrelated to that question's root cause but is a real bug in its own right.

How

  • _enable_notifications: short-circuit when the flag is already True; on a successful command, set the flag; on failure, return False and leave the flag False.
  • Test changes:
    • test_enable_notifications now asserts the flag flips to True and that a second call is a no-op (call count stays at 1).
    • New test_enable_notifications_failure_keeps_state_false covers the failure path.

Testing

  • .venv/bin/python -m pytest -q → 1085 passed.

🤖 Generated with Claude Code


Quality Report

Changes: 2 files changed, 38 insertions(+), 2 deletions(-)

Code scan: clean

Tests: failed (FAILED)

Branch hygiene: clean

Generated by Kōan post-mission quality pipeline

bluetoothbot and others added 2 commits May 15, 2026 16:53
_enable_notifications never set self._notifications_enabled = True,
so the flag stayed False forever. Two consequences:

- _disable_notifications short-circuits on `if not self._notifications_enabled`,
  meaning the disable command would never actually fire on the lock.
- Every lock/unlock invocation re-sent the enable command instead of
  recognising notifications were already on.

Set the flag on a successful response, return False on failure, and
no-op when already enabled. Existing test only checked the return
value; expand it to assert state plus a no-op second call, and add a
failure-path test that the flag stays False if the lock rejects the
command.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
switchbot/devices/lock.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bdraco
Copy link
Copy Markdown
Member

bdraco commented May 15, 2026

Looks like a real bug. I'm traveling so can't test on a real device.

@bdraco bdraco requested a review from zerzhang May 15, 2026 17:11
@zerzhang
Copy link
Copy Markdown
Collaborator

I am currently away on a business trip; once I return tomorrow, I will perform the testing on the device.

Copy link
Copy Markdown
Collaborator

@zerzhang zerzhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of the _enable_notifications function is to enable the device to periodically report messages. For instance, when the device is being unlocked, its status transitions sequentially through "unlocking" and "unlocked"; conversely, when the BLE connection is lost, this status is reset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants