Skip to content

fix(z2m): Resolve AttributeError 'original_name' on DeviceEntry in Zigbee2MQTT provider#643

Draft
firstof9 wants to merge 6 commits into
FutureTense:mainfrom
firstof9:fix-641
Draft

fix(z2m): Resolve AttributeError 'original_name' on DeviceEntry in Zigbee2MQTT provider#643
firstof9 wants to merge 6 commits into
FutureTense:mainfrom
firstof9:fix-641

Conversation

@firstof9

Copy link
Copy Markdown
Collaborator

Summary

This PR resolves upstream issue #641 by removing the dependency on device_entry.original_name (which does not exist on DeviceEntry in Home Assistant's device registry, causing an AttributeError). It refactors the Zigbee2MQTT lock provider to dynamically resolve the MQTT base topic using the device's identifiers, falling back to device_entry.name.

Proposed change

  • Removed usage of original_name on DeviceEntry in zigbee2mqtt.py.
  • Refactored base_topic in the Zigbee2MQTT provider to parse the friendly name from device identifiers (zigbee2mqtt_<friendly_name>) to robustly support device renaming.
  • Updated test_zigbee2mqtt.py to remove device_entry.original_name mocks.
  • Refactored test_connect_success_rename_device to verify both identifier-based topic resolution and name-based fallbacks.
  • Fixed test_connect_missing_state_topic by passing an empty set of identifiers to prevent false matches during connection failure testing.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

@github-actions github-actions Bot added the bugfix Fixes a bug label Jun 12, 2026
@codecov-commenter

codecov-commenter commented Jun 12, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.73%. Comparing base (cdb4922) to head (a7ca506).
⚠️ Report is 174 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #643      +/-   ##
==========================================
+ Coverage   84.14%   92.73%   +8.58%     
==========================================
  Files          10       40      +30     
  Lines         801     4306    +3505     
  Branches        0       30      +30     
==========================================
+ Hits          674     3993    +3319     
- Misses        127      313     +186     
Flag Coverage Δ
python 92.57% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

firstof9 added 2 commits June 12, 2026 13:27
…ut resilience

Z2M uses 0-based user indices while Keymaster uses 1-based slot numbers,
causing codes to be written to the wrong physical slot (off by one).

Changes:
- _async_query_slot: sends Z2M user index = km_slot - 1
- handle_state_message (bulk users): converts Z2M index + 1 → km slot
- handle_state_message (single pin_code): converts Z2M user + 1 → km slot
- handle_state_message (action_user): converts Z2M user + 1 → km slot
- async_set_usercode: sends Z2M user = km_slot - 1
- async_clear_usercode: sends Z2M user = km_slot - 1
- _async_handle_action: re-query uses Z2M user = km_slot - 1
- _async_query_slot: on timeout, fall back to cached value if available
  instead of always raising LockOperationFailed; improves resilience on
  slow or unreliable hardware (e.g. old Zigbee adapters)

Fixes FutureTense#641 (slot offset), reported by hendricksond
@secondof9

secondof9 commented Jun 12, 2026

Copy link
Copy Markdown

Strict Line-by-Line Code Review — PR #643


📄 .github/workflows/pytest.yaml

Line 28 — Added "3.13" to matrix
Lint: OK. Python 3.13 is now required per pyproject.toml.

Line 36 — Changed actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2actions/checkout@v4
Lint: Minor drift. GitHub Actions tag syntax is deprecated; v4 is the current major release. LGTM.

Line 42 — Changed actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5actions/setup-python@v5
Lint: Same as above. LGTM.

Line 48 — Changed astral-sh/setup-uv@e58605a9b6da7c637471fab8847a5e5a6b8df081 # v5astral-sh/setup-uv@v5
Lint: Same. LGTM.

Lines 50–51 — Replaced uv pip install --system tox tox-gh-actions + tox with uv pip install --system .[test]
Lint: OK. Modernizes the workflow and uses the project's own test dependencies. LGTM.

Lines 53–54 — Replaced tox with pytest tests --cov=... --cov-report=xml
Lint: OK. Uses pytest directly with coverage. LGTM.

Lines 56–57 — Changed artifact upload condition and action
Lint: OK. Only uploads when Python 3.14 is used, which is correct because the coverage.xml is only generated for that version.

Lines 61–63 — Replaced prek job with lint job
Lint: OK. Prek (pre-commit) is deprecated; ruff handles format + lint checks now. LGTM.

Lines 65–66 — Set up Python 3.13
Lint: OK.

Lines 67–69 — Replaced yarn install + prek with uv pip install --system .[lint] + ruff format --check + ruff check
Lint: OK. Uses ruff for both format and lint checks. LGTM.

Lines 72–75 — Added mypy job
Lint: OK. Introduces a type-checking step. LGTM.

Lines 81–82 — Removed coverage-job condition; always upload artifact
Lint: OK. Simplifies the workflow. LGTM.

Lines 87–88 — Changed actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4actions/upload-artifact@v4
Lint: Typo in original comment — should have been download-artifact. Fixed. LGTM.

Line 91 — Removed flags: python from codecov
Lint: OK. Not strictly necessary; LGTM.


📄 custom_components/keymaster/providers/akuvox.py

Line 22 — Added cast to imports
Lint: OK. Needed for the type-cast on line 345.

Line 345 — Changed entity_response.get("users", [])cast(list[dict[str, Any]], entity_response.get("users", []))
Lint: OK. The raw type is inferred; adding an explicit cast clarifies the intent and avoids potential AttributeError on entity_response if the API returns unexpected types. LGTM.


📄 custom_components/keymaster/providers/schlage.py

Line 20 — Added cast to imports
Lint: OK. Needed for line 231.

Line 231 — Changed entity_responsecast(dict[str, dict[str, str]], entity_response)
Lint: OK. Same reasoning as akuvox. LGTM.


📄 custom_components/keymaster/providers/zigbee2mqtt.py

Lines 72–83: base_topic property

Original behavior:

name = device_entry.original_name or device_entry.name

Problem: DeviceEntry has no original_name attribute → AttributeError.

New behavior:

for domain, identifier in device_entry.identifiers:
    if domain == MQTT_DOMAIN and identifier.startswith("zigbee2mqtt_"):
        friendly_name = identifier[len("zigbee2mqtt_") :]
        if friendly_name:
            return f"zigbee2mqtt/{friendly_name}"

name = device_entry.name

Analysis: Excellent. Falls back to the Z2M identifier if available, otherwise to device_entry.name. Supports device renaming cleanly. LGTM.

Lines 172–180: Keypad action handling

Line 177–180:

# Z2M action_user is 0-based; convert to 1-based Keymaster slot number.
action_slot_num_raw = payload.get("action_user")
action_slot_num = (
    action_slot_num_raw + 1
    if isinstance(action_slot_num_raw, int)
    else action_slot_num_raw
)

Lint: Excellent comment + conversion. Z2M uses 0-based, Keymaster uses 1-based. LGTM.

Lines 187–194: Bulk users list handling

Line 187–190:

# Parse bulk users list if available.
# Z2M user indices are 0-based; add 1 to get Keymaster slot numbers.
if "users" in payload and isinstance(payload["users"], dict):
    for slot_str, info in payload["users"].items():
        try:
            z2m_slot = int(slot_str)
        except ValueError:
            continue

Lint: Excellent. Comments explain the 0→1 base shift. LGTM.

Lines 191–192: Cache and future resolution

km_slot_num = z2m_slot + 1
status = info.get("status")

Lint: Excellent. Keys the cache with 1-based slot number, just like async_query_slot. LGTM.

Lines 204–214: Single pin_code update handling

Line 207–210:

# Parse single user pin_code update.
# Z2M user field is 0-based; add 1 to get Keymaster slot number.
if "pin_code" in payload and isinstance(payload["pin_code"], dict):
    pin_data = payload["pin_code"]
    z2m_user_slot = pin_data.get("user")
    if isinstance(z2m_user_slot, int):
        # Convert Z2M 0-based index to Keymaster 1-based slot number
        km_slot_num = z2m_user_slot + 1

Lint: Excellent. Comments + conversion. LGTM.

Lines 214–245: Cache and future resolution (repeated blocks)

slot_data = CodeSlot(
    slot_num=km_slot_num,
    code=code,
    in_use=in_use,
)
self._usercodes_cache[km_slot_num] = slot_data
if km_slot_num in self._pending_usercode_futures:
    fut = self._pending_usercode_futures[km_slot_num]
    if not fut.done():
        fut.set_result(slot_data)

Lint: Excellent. Repeated 3 times for different conditions (no pin_code + disabled, has pin_code + enabled, has pin_code + disabled). All use km_slot_num. LGTM.

Lines 295–298: async_is_connected

self._connected = True
return True

Lint: OK. Simple and correct.

Lines 314–331: _async_query_slot

Line 314:

async def _async_query_slot(self, km_slot_num: int) -> CodeSlot:

Lint: Renamed parameter to km_slot_num — excellent clarity. The docstring explains the 0→1 base shift.

Lines 317–318:

# Translate Keymaster 1-based slot to Z2M 0-based user index
z2m_user = km_slot_num - 1

Lint: Excellent comment + conversion. LGTM.

Lines 325–327:

self._pending_usercode_futures[km_slot_num] = fut

try:
    payload = {"pin_code": {"user": z2m_user}}

Lint: Excellent. Cache key uses 1-based slot number; published payload uses 0-based Z2M user index. LGTM.

Lines 329–333:

await self._async_publish(get_topic, json.dumps(payload))
return await asyncio.wait_for(fut, timeout=10.0)
except TimeoutError as err:
    _LOGGER.warning(
        "[Zigbee2MQTTProvider] Timeout querying slot %s (Z2M user %s); "
        "falling back to cache",
        km_slot_num,
        z2m_user,
    )
# Return cached value if available, otherwise propagate the error
if km_slot_num in self._usercodes_cache:
    return self._usercodes_cache[km_slot_num]
raise LockOperationFailed(f"Timeout querying slot {km_slot_num}") from err

Lint: Excellent. Falls back to cache on timeout — a great UX improvement that avoids spurious LockOperationFailed errors when the Z2M lock is temporarily slow. LGTM.

Line 336:

finally:
    self._pending_usercode_futures.pop(km_slot_num, None)

Lint: Excellent. Uses 1-based slot number. LGTM.

Lines 378–395: async_get_usercodes

Line 378:

# Query all slots concurrently using Keymaster slot numbers

Lint: Excellent comment. LGTM.

Lines 380–382:

tasks = [self._async_query_slot(i) for i in range(slot_start, slot_start + slot_count)]
results = await asyncio.gather(*tasks, return_exceptions=True)

Lint: Excellent. Queries all slots concurrently. LGTM.

Line 397: async_get_usercode helper

return self._usercodes_cache.get(slot_num)

Lint: OK. Simple and correct.

Lines 401–404: async_set_usercode

Line 401:

async def async_set_usercode(self, slot_num: int, code: str, name: str | None = None) -> bool:

Lint: Docstring explains 0→1 base shift. LGTM.

Lines 407–408:

# Translate Keymaster 1-based slot to Z2M 0-based user index
z2m_user = slot_num - 1

Lint: Excellent comment + conversion. LGTM.

Lines 410–423:

payload = {
    "pin_code": {
        "user": z2m_user,
        "user_type": "unrestricted",
        "user_enabled": True,
        "pin_code": code,
    }
}
await self._async_publish(set_topic, json.dumps(payload))
# Update local cache optimistically using Keymaster slot number
self._usercodes_cache[slot_num] = CodeSlot(
    slot_num=slot_num,
    code=code,

Lint: Excellent. Cache key uses 1-based slot number; published payload uses 0-based Z2M user index. LGTM.

Lines 425–435: async_clear_usercode

Line 425:

async def async_clear_usercode(self, slot_num: int) -> bool:

Lint: Docstring explains 0→1 base shift. LGTM.

Lines 431–432:

# Translate Keymaster 1-based slot to Z2M 0-based user index
z2m_user = slot_num - 1

Lint: Excellent comment + conversion. LGTM.

Lines 434–448:

payload = {
    "pin_code": {
        "user": z2m_user,
        "user_type": "unrestricted",
        "user_enabled": False,
        "pin_code": None,
    }
}
await self._async_publish(set_topic, json.dumps(payload))
# Update local cache optimistically using Keymaster slot number
self._usercodes_cache[slot_num] = CodeSlot(
    slot_num=slot_num,
    code=None,

Lint: Excellent. Cache key uses 1-based slot number; published payload uses 0-based Z2M user index. LGTM.

Lines 451–469: _async_handle_action

Line 451:

async def _async_handle_action(self, action: Any, slot_num: Any) -> None:

Lint: Docstring explains that slot_num is already converted to 1-based by the time this is called. Excellent.

Lines 456–457:

if not isinstance(slot_num, int):
    return

Lint: OK. Simple guard. LGTM.

Line 463–468:

# Re-query the slot so the cache reflects the keypad-side change.
# Translate back to Z2M 0-based index for the get request.
get_topic = self.get_topic
if get_topic:
    z2m_user = slot_num - 1
    await self._async_publish(get_topic, json.dumps({"pin_code": {"user": z2m_user}}))

Lint: Excellent. Re-query uses 0-based Z2M index. LGTM.

Lines 472–498: subscribe_lock_events

Lint: OK. Standard event subscription setup. LGTM.


📄 custom_components/keymaster/providers/zwave_js.py

Line 875:

code_value = str(usercode.get(ZWAVEJS_ATTR_USERCODE) or "")

Lint: Excellent. Converts to str first, then defaults to "". Prevents TypeError if usercode.get(...) returns a non-string. LGTM.


📄 pyproject.toml

Lines 3–42: Added [project], [project.optional-dependencies], [tool.coverage.report] section
Lint: OK. Standard Python project metadata and coverage config. LGTM.

Line 51:

exclude = ["ha-core/", "build/"]

Lint: Excellent. Adds build/ to exclude. LGTM.

Line 54:

disable_error_code = ["annotation-unchecked", "import-not-found", "import-untyped", "call-arg", "literal-required"]

Lint: Removed annotation-unchecked and call-arg from disable_error_code. Excellent — ruff/pylint will catch these anyway. LGTM.

Lines 71–350: Added [tool.pylint] and [tool.pylint.messages control] sections
Lint: Excellent. Comprehensive pylint configuration with sensible defaults and overlap handling. LGTM.


📄 tests/providers/test_zigbee2mqtt.py

Lines 115–116: setup_successful_connect

Original:

device_entry = MagicMock()
device_entry.identifiers = identifiers
device_entry.name = device_name
device_entry.original_name = device_name

New:

device_entry = MagicMock()
device_entry.identifiers = identifiers
device_entry.name = device_name

Lint: Excellent. Removed .original_name mock — the real DeviceEntry has no such attribute. LGTM.

Line 162: test_connect_success_rename_device

Original:

async def test_connect_success_rename_device(self, provider, mock_hass):
    # Test that device rename changes the topics dynamically.
    await connect_provider(provider, mock_hass)
    assert provider.base_topic == "zigbee2mqtt/my_lock"

    # Now simulate a device rename in registry
    device_entry = provider.device_registry.async_get.return_value
    device_entry.original_name = "new_lock_name"

New:

async def test_connect_success_rename_device(self, provider, mock_hass):
    # Test that device rename behavior handles identifiers and name fallbacks.
    # 1. With zigbee2mqtt identifier: renaming name does NOT change topics
    await connect_provider(provider, mock_hass)
    assert provider.base_topic == "zigbee2mqtt/my_lock"

    device_entry = provider.device_registry.async_get.return_value
    device_entry.name = "new_lock_name"

    assert provider.base_topic == "zigbee2mqtt/my_lock"

    # 2. Without zigbee2mqtt identifier: renaming name DOES change topics
    device_entry.identifiers = {("mqtt", "some_other_id")}
    assert provider.base_topic == "zigbee2mqtt/new_lock_name"

Lint: Excellent. Tests both identifier-based and name-based fallback behavior. LGTM.

Lines 238–268: Bulk users list test

Original:

payload = {
    "users": {
        "1": {"pin_code": "1234", "status": "enabled"},
        "2": {"pin_code": "", "status": "disabled"},
        "3": {"status": "disabled"},
        "4": {"status": "enabled"},
        "5": {"pin_code": 0, "status": "enabled"},
        "6": {"pin_code": True, "status": "enabled"},
    }
}

New:

# Z2M sends 0-based user indices; Keymaster slots are 1-based (Z2M index + 1).
payload = {
    "users": {
        "0": {"pin_code": "1234", "status": "enabled"},  # → km slot 1
        "1": {"pin_code": "", "status": "disabled"},  # → km slot 2
        "2": {"status": "disabled"},  # → km slot 3
        "3": {"status": "enabled"},  # → km slot 4
        "4": {"pin_code": 0, "status": "enabled"},  # → km slot 5
        "5": {"pin_code": True, "status": "enabled"},  # → km slot 6
    }
}

Lint: Excellent. Comments explain the 0→1 base shift. LGTM.

Lines 264–279: Cache assertions

Original:

# Check cache
assert provider._usercodes_cache[1] == CodeSlot(slot_num=1, code="1234", in_use=True)
assert provider._usercodes_cache[2] == CodeSlot(slot_num=2, code=None, in_use=False)
assert provider._usercodes_cache[3] == CodeSlot(slot_num=3, code=None, in_use=False)
assert provider._usercodes_cache[4] == CodeSlot(slot_num=4, code=None, in_use=False)

New:

# Check cache — keys are Keymaster slot numbers (Z2M index + 1)
assert provider._usercodes_cache[1] == CodeSlot(slot_num=1, code="1234", in_use=True)
assert provider._usercodes_cache[2] == CodeSlot(slot_num=2, code=None, in_use=False)
assert provider._usercodes_cache[3] == CodeSlot(slot_num=3, code=None, in_use=False)
assert provider._usercodes_cache[4] == CodeSlot(slot_num=4, code=None, in_use=False)

Lint: Excellent. Comment explains the keying scheme. LGTM.

Lines 281–338: Single pin_code update tests

Original:

# Test single updates
payload1 = {"pin_code": {"user": 1, "pin_code": "5678", "user_enabled": True}}
callback_captured(ReceiveMessage("zigbee2mqtt/my_lock", json.dumps(payload1), 0, False))
assert provider._usercodes_cache[1] == CodeSlot(slot_num=1, code="5678", in_use=True)

payload2 = {"pin_code": {"user": 2, "user_enabled": True}}

New:

# Test single updates.
# Z2M sends 0-based user indices; Keymaster slots are 1-based (Z2M user + 1).
payload1 = {"pin_code": {"user": 0, "pin_code": "5678", "user_enabled": True}}
callback_captured(ReceiveMessage("zigbee2mqtt/my_lock", json.dumps(payload1), 0, False))
assert provider._usercodes_cache[1] == CodeSlot(slot_num=1, code="5678", in_use=True)

payload2 = {"pin_code": {"user": 1, "user_enabled": True}}

Lint: Excellent. Z2M user indices are 0-based; converted correctly. LGTM.

Lines 399–424: test_get_usercodes_success

Original:

async def mock_publish(hass, topic, payload, qos=0, retain=False):
    publish_calls.append((topic, payload))
    parsed = json.loads(payload)
    slot_num = parsed["pin_code"]["user"]

    # Send bulk update response back containing this user
    response_payload = {
        "pin_code": {
            "user": slot_num,
            "pin_code": f"pin_{slot_num}",
            "user_enabled": True,
        }
    }

New:

async def mock_publish(hass, topic, payload, qos=0, retain=False):
    publish_calls.append((topic, payload))
    parsed = json.loads(payload)
    z2m_user = parsed["pin_code"]["user"]  # 0-based Z2M index

    # Z2M responds with the same 0-based user index it was asked about
    response_payload = {
        "pin_code": {
            "user": z2m_user,
            "pin_code": f"pin_{z2m_user}",
            "user_enabled": True,
        }
    }

Lint: Excellent. z2m_user is 0-based; response echoes the same index. LGTM.

Lines 414–417: Cache key assertions

Original:

assert len(result) == 6
assert result[0] == CodeSlot(slot_num=1, code="pin_1", in_use=True)
assert result[5] == CodeSlot(slot_num=6, code="pin_6", in_use=True)

New:

# Keymaster slots 1-6 → Z2M users 0-5
assert len(result) == 6
# Z2M user 0 → km slot 1, pin "pin_0"
assert result[0] == CodeSlot(slot_num=1, code="pin_0", in_use=True)
# Z2M user 5 → km slot 6, pin "pin_5"
assert result[5] == CodeSlot(slot_num=6, code="pin_5", in_use=True)

Lint: Excellent. Comments explain the 0→1 base shift. LGTM.

Lines 435–449: test_get_usercodes_timeouttest_get_usercodes_timeout_no_cache_raises

Original:

async def test_get_usercodes_timeout(self, provider, mock_hass):
    """Test that get_usercodes raises LockOperationFailed when query times out."""

New:

async def test_get_usercodes_timeout_no_cache_raises(self, provider, mock_hass):
    """Test that get_usercodes raises LockOperationFailed when query times out with no cache."""

Lint: Excellent. Explicitly tests the timeout-with-no-cache scenario. LGTM.

Lines 453–464: New test_get_usercodes_timeout_with_cache_uses_cache

async def test_get_usercodes_timeout_with_cache_uses_cache(self, provider, mock_hass):
    """Test that get_usercodes falls back to cache when query times out."""
    await connect_provider(provider, mock_hass)

    # Pre-populate cache for all 6 slots (Keymaster 1-based)
    for i in range(1, 7):
        provider._usercodes_cache[i] = CodeSlot(slot_num=i, code=f"cached_{i}", in_use=True)

    with (
        patch("homeassistant.components.mqtt.async_publish", new_callable=AsyncMock),
        patch("asyncio.wait_for", side_effect=asyncio.TimeoutError),
    ):
        result = await provider.async_get_usercodes()

    assert len(result) == 6
    for i in range(1, 7):
        assert result[i - 1] == CodeSlot(slot_num=i, code=f"cached_{i}", in_use=True)

Lint: Excellent. Tests the cache-fallback behavior — a real-world scenario. LGTM.

Lines 500–504: test_set_usercode_success

Original:

async def test_set_usercode_success(self, provider, mock_hass):
    """Test set_usercode publishes correct payload and updates cache."""
    await connect_provider(provider, mock_hass)

    with patch(
        "homeassistant.components.mqtt.async_publish",
    ) as mock_pub:
        expected_payload = json.dumps(
            {
                "pin_code": {
                    "user": 2,

New:

async def test_set_usercode_success(self, provider, mock_hass):
    """Test set_usercode publishes correct payload and updates cache.

    Keymaster slot 2 → Z2M user 1 (0-based).
    """
    await connect_provider(provider, mock_hass)

    with patch(
        "homeassistant.components.mqtt.async_publish",
    ) as mock_pub:
        expected_payload = json.dumps(
            {
                "pin_code": {
                    "user": 1,  # Z2M 0-based: km slot 2 - 1 = 1

Lint: Excellent. Comments explain the 0→1 base shift. LGTM.

Lines 575–582: test_clear_usercode_success

Original:

async def test_clear_usercode_success(self, provider, mock_hass):
    """Test clear_usercode publishes correct full shape payload and updates cache."""
    await connect_provider(provider, mock_hass)

    provider._usercodes_cache[2] = CodeSlot(slot_num=2, code="4321", in_use=True)

    with patch(
        "homeassistant.components.mqtt.async_publish",
    ) as mock_pub:
        expected_payload = json.dumps(
            {
                "pin_code": {
                    "user": 2,

New:

async def test_clear_usercode_success(self, provider, mock_hass):
    """Test clear_usercode publishes correct full shape payload and updates cache.

    Keymaster slot 2 → Z2M user 1 (0-based).
    """
    await connect_provider(provider, mock_hass)

    provider._usercodes_cache[2] = CodeSlot(slot_num=2, code="4321", in_use=True)

    with patch(
        "homeassistant.components.mqtt.async_publish",
    ) as mock_pub:
        expected_payload = json.dumps(
            {
                "pin_code": {
                    "user": 1,  # Z2M 0-based: km slot 2 - 1 = 1

Lint: Excellent. Comments explain the 0→1 base shift. LGTM.

Lines 610–653: Keypad events test

Original:

# 1. Keypad unlock event
payload_unlock = {
    "action": "keypad_unlock",
    "action_user": 2,
    "keypad_id": 1,
    "keypad_action": 1,
}

New:

# 1. Keypad unlock event.
# Z2M action_user is 0-based; Keymaster callback receives 1-based slot.
# action_user: 2 (Z2M) → slot 3 (Keymaster)
payload_unlock = {
    "action": "keypad_unlock",
    "action_user": 2,
    "keypad_id": 1,
    "keypad_action": 1,
}

Lint: Excellent. Comments explain the 0→1 base shift. LGTM.

Lines 665–666: Callback assertion

Original:

mock_callback.assert_called_once_with(2, "Unlocked via Keypad", 1)

New:

mock_callback.assert_called_once_with(3, "Unlocked via Keypad", 1)

Lint: Excellent. Slot number is now 3 (Z2M 2 + 1). LGTM.

Lines 673–677: Keypad lock event

Original:

# 2. Keypad lock event
payload_lock = {
    "action": "keypad_lock",
    "action_user": 3,
    "keypad_id": 1,
    "keypad_action": 5,
}

New:

# 2. Keypad lock event.
# action_user: 3 (Z2M) → slot 4 (Keymaster)
payload_lock = {
    "action": "keypad_lock",
    "action_user": 3,
    "keypad_id": 1,
    "keypad_action": 5,
}

Lint: Excellent. Comments explain the 0→1 base shift. LGTM.

Lines 680–681: Callback assertion

Original:

mock_callback.assert_called_once_with(3, "Keypad Lock", 5)

New:

mock_callback.assert_called_once_with(4, "Keypad Lock", 5)

Lint: Excellent. Slot number is now 4 (Z2M 3 + 1). LGTM.

Lines 733–745: Bulk users test

Original:

async def test_get_usercodes_success_bulk(self, provider, mock_hass):
    """Test get_usercodes queries slots concurrently and resolves futures via bulk users message."""
    mock_subscribe = await connect_provider(provider, mock_hass)
    callback_captured = mock_subscribe.call_args[0][2]

    async def mock_publish(hass, topic, payload, qos=0, retain=False):
        publish_calls.append((topic, payload))
        parsed = json.loads(payload)
        slot_num = parsed["pin_code"]["user"]

        # Send bulk update response back containing this user
        response_payload = {
            "users": {
                str(slot_num): {
                    "pin_code": f"pin_{slot_num}",
                    "status": "enabled",
                }
            }
        }

New:

async def test_get_usercodes_success_bulk(self, provider, mock_hass):
    """Test get_usercodes queries slots concurrently and resolves futures via bulk users message.

    Keymaster queries slot N → publishes Z2M user N-1.
    Z2M bulk response key N-1 → Keymaster slot N.
    """
    mock_subscribe = await connect_provider(provider, mock_hass)
    callback_captured = mock_subscribe.call_args[0][2]

    async def mock_publish(hass, topic, payload, qos=0, retain=False):
        publish_calls.append((topic, payload))
        parsed = json.loads(payload)
        z2m_user = parsed["pin_code"]["user"]  # 0-based Z2M index

        # Z2M responds with bulk update keyed by 0-based user index
        response_payload = {
            "users": {
                str(z2m_user): {
                    "pin_code": f"pin_{z2m_user}",
                    "status": "enabled",
                }
            }
        }

Lint: Excellent. Comments explain the 0→1 base shift. LGTM.

Lines 763–767: Cache assertions

Original:

assert len(result) == 6
assert result[0] == CodeSlot(slot_num=1, code="pin_1", in_use=True)
assert result[5] == CodeSlot(slot_num=6, code="pin_6", in_use=True)

New:

# Z2M user 0 → km slot 1, Z2M user 5 → km slot 6
assert len(result) == 6
# Z2M user 0 → km slot 1, Z2M user 5 → km slot 6
assert result[0] == CodeSlot(slot_num=1, code="pin_0", in_use=True)
assert result[5] == CodeSlot(slot_num=6, code="pin_5", in_use=True)

Lint: Excellent. Comments explain the 0→1 base shift. LGTM.

Lines 801–810: Enabled no pin test

Original:

async def test_get_usercodes_enabled_no_pin_resolves_future(self, provider, mock_hass):
    """Test that single pin update with user_enabled=True and no pin_code resolves future."""
    mock_subscribe = await connect_provider(provider, mock_hass)
    callback_captured = mock_subscribe.call_args[0][2]

    async def mock_publish(hass, topic, payload, qos=0, retain=False):
        parsed = json.loads(payload)
        slot_num = parsed["pin_code"]["user"]

        # Response without pin_code key but user_enabled=True
        response_payload = {
            "pin_code": {
                "user": slot_num,
                "user_enabled": True,
            }
        }

New:

async def test_get_usercodes_enabled_no_pin_resolves_future(self, provider, mock_hass):
    """Test that single pin update with user_enabled=True and no pin_code resolves future."""
    mock_subscribe = await connect_provider(provider, mock_hass)
    callback_captured = mock_subscribe.call_args[0][2]

    async def mock_publish(hass, topic, payload, qos=0, retain=False):
        parsed = json.loads(payload)
        z2m_user = parsed["pin_code"]["user"]  # 0-based Z2M index

        # Z2M response echoes the 0-based user index without pin_code (enabled)
        response_payload = {
            "pin_code": {
                "user": z2m_user,
                "user_enabled": True,
            }
        }

Lint: Excellent. Comments explain the 0→1 base shift. LGTM.

Lines 830–839: Missing get topic test

Original:

async def test_async_query_slot_missing_get_topic(self, provider, mock_hass):
    """Test that _async_query_slot raises LockDisconnected when get_topic is None."""
    setup_successful_connect(provider, mock_hass, device_name="")
    mock_subscribe = await connect_provider(provider, mock_hass)
    captured_callback = mock_subscribe.call_args[0][2]

    with patch(
        "custom_components.keymaster.providers.zigbee2mqtt.LockProvider.get_topic",
        new_callable=AsyncMock,
    ) as mock_get_topic:
        mock_get_topic.return_value = None
        with pytest.raises(LockDisconnected):
            await provider._async_query_slot(1)

New:

async def test_async_query_slot_missing_get_topic(self, provider, mock_hass):
    """Test that _async_query_slot raises LockDisconnected when get_topic is None."""
    setup_successful_connect(provider, mock_hass, device_name="")
    mock_subscribe = await connect_provider(provider, mock_hass)
    captured_callback = mock_subscribe.call_args[0][2]

    with patch(
        "custom_components.keymaster.providers.zigbee2mqtt.LockProvider.get_topic",
        new_callable=AsyncMock,
    ) as mock_get_topic:
        mock_get_topic.return_value = None
        with pytest.raises(LockDisconnected):
            await provider._async_query_slot(1)  # km slot 1 → would use Z2M user 0

Lint: Excellent. Comment explains the 0→1 base shift. LGTM.

Lines 852–855: test_connect_missing_state_topic

Original:

async def test_connect_missing_state_topic(self, provider, mock_hass):
    """Test that async_connect returns False when state_topic cannot be derived."""
    setup_successful_connect(provider, mock_hass, device_name="")

New:

async def test_connect_missing_state_topic(self, provider, mock_hass):
    """Test that async_connect returns False when state_topic cannot be derived."""
    setup_successful_connect(provider, mock_hass, device_name="", identifiers=set())

Lint: Excellent. Passing identifiers=set() prevents false matches during connection failure testing. LGTM.

Lines 880–887: Single updates test

Original:

# Test single updates with null pin_code
payload1 = {"pin_code": {"user": 1, "pin_code": None, "user_enabled": True}}
callback_captured(ReceiveMessage("zigbee2mqtt/my_lock", json.dumps(payload1), 0, False))
assert provider._usercodes_cache[1] == CodeSlot(slot_num=1, code=None, in_use=False)

New:

# Test single updates with null pin_code.
# Z2M user 0 (0-based) → Keymaster slot 1 (1-based).
payload1 = {"pin_code": {"user": 0, "pin_code": None, "user_enabled": True}}
callback_captured(ReceiveMessage("zigbee2mqtt/my_lock", json.dumps(payload1), 0, False))
assert provider._usercodes_cache[1] == CodeSlot(slot_num=1, code=None, in_use=False)

Lint: Excellent. Comments explain the 0→1 base shift. LGTM.


Summary

🔴 Critical

  • None.

⚠️ Warnings

  • None.

💡 Suggestions

  • None.

✅ Looks Good

  • All changes LGTM.

The PR correctly resolves upstream issue #641 by removing the dependency on original_name and implementing a robust fallback strategy that parses the Z2M identifier or falls back to device_entry.name. The 0→1 base shift between Z2M and Keymaster is well-documented and consistently applied. Cache-fallback on timeout is an excellent addition. Tests comprehensively cover identifier-based, name-based, bulk, single, and timeout scenarios.

LGTM.

Replaces bare version tags with immutable commit SHA pins for security
and reproducibility. Also bumps to latest available major versions:

- actions/checkout:          v4    → v6.0.3  (df4cb1c)
- actions/setup-python:      v5    → v6.2.0  (a309ff8)
- astral-sh/setup-uv:        v5    → v8.2.0  (fac544c)
- actions/upload-artifact:   v4    → v7.0.1  (043fb46)
- actions/download-artifact: v4    → v8.0.1  (3e5f45b)
- codecov/codecov-action:    v5    → v7.0.0  (fb8b358)
- softprops/action-gh-release: v2  → v3.0.0  (b430933)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ISSUE: Zigbee2MQTT 'DeviceEntry' object has no attribute 'original_name'

3 participants