Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 29 additions & 27 deletions custom_components/keymaster/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ async def async_setup_entry(hass: HomeAssistant, config_entry: ConfigEntry) -> b
updated_config[CONF_PARENT_ENTRY_ID] = None
elif updated_config.get(CONF_PARENT_ENTRY_ID) is None:
for entry in hass.config_entries.async_entries(DOMAIN):
if updated_config.get(CONF_PARENT) == entry.data.get(CONF_LOCK_NAME):
if updated_config.get(CONF_PARENT) in (entry.title, entry.data.get(CONF_LOCK_NAME)):
updated_config[CONF_PARENT_ENTRY_ID] = entry.entry_id
break

Expand All @@ -128,6 +128,10 @@ async def async_setup_entry(hass: HomeAssistant, config_entry: ConfigEntry) -> b
if updated_config != config_entry.data:
hass.config_entries.async_update_entry(config_entry, data=updated_config)

# Use normalized values for the rest of setup so runtime objects are created
# with resolved parent relationships in the same setup pass.
setup_data = updated_config

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: setup_data = updated_config creates an alias to the same dict object, not a copy. After async_update_entry is called above, config_entry.data already contains the updated values, so config_entry.data and updated_config point to the same data.

This means all the config_entry.datasetup_data substitutions below are functionally no-ops — they read the same values either way. The variable name setup_data implies it might be a separate snapshot, but it isn't.

This isn't a bug, but it adds 20+ line changes that don't change behavior, which makes the diff harder to review and inflates patch coverage requirements. Consider either:

  1. Removing the alias and keeping config_entry.data (fewer changes, same behavior), or
  2. If the intent is defensive (guard against async_update_entry not updating in-place in the future), add a comment explaining why

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Follow-up: This is still unaddressed. The setup_data = updated_config alias adds ~20 line changes with no behavioral change since async_update_entry updates config_entry.data in-place.

I'd like to understand the intent here — is this a defensive pattern against future HA API changes, a readability preference, or something else? If there's a good reason, a brief comment in the code explaining why would be helpful. Otherwise, removing it would simplify the diff significantly.

Please respond so we can resolve this thread one way or the other.

# _LOGGER.debug(f"[init async_setup_entry] updated config_entry.data: {config_entry.data}")

await async_setup_services(hass)
Expand All @@ -145,7 +149,7 @@ async def async_setup_entry(hass: HomeAssistant, config_entry: ConfigEntry) -> b
device_registry = dr.async_get(hass)

via_device: tuple[str, str] | None = None
if parent_entry_id := config_entry.data.get(CONF_PARENT_ENTRY_ID):
if parent_entry_id := setup_data.get(CONF_PARENT_ENTRY_ID):
via_device = (DOMAIN, parent_entry_id)

# _LOGGER.debug(
Expand All @@ -158,7 +162,7 @@ async def async_setup_entry(hass: HomeAssistant, config_entry: ConfigEntry) -> b
device_registry.async_get_or_create(
config_entry_id=config_entry.entry_id,
identifiers={(DOMAIN, config_entry.entry_id)},
name=config_entry.data.get(CONF_LOCK_NAME),
name=setup_data.get(CONF_LOCK_NAME),
configuration_url="https://github.com/FutureTense/keymaster",
via_device=via_device,
)
Expand All @@ -167,51 +171,49 @@ async def async_setup_entry(hass: HomeAssistant, config_entry: ConfigEntry) -> b

code_slots: MutableMapping[int, KeymasterCodeSlot] = {}
for x in range(
config_entry.data[CONF_START],
config_entry.data[CONF_START] + config_entry.data[CONF_SLOTS],
setup_data[CONF_START],
setup_data[CONF_START] + setup_data[CONF_SLOTS],
):
dow_slots: MutableMapping[int, KeymasterCodeSlotDayOfWeek] = {}
for i, dow in enumerate(DAY_NAMES):
dow_slots[i] = KeymasterCodeSlotDayOfWeek(day_of_week_num=i, day_of_week_name=dow)
code_slots[x] = KeymasterCodeSlot(number=x, accesslimit_day_of_week=dow_slots)

kmlock = KeymasterLock(
lock_name=config_entry.data[CONF_LOCK_NAME],
lock_entity_id=config_entry.data[CONF_LOCK_ENTITY_ID],
lock_name=setup_data[CONF_LOCK_NAME],
lock_entity_id=setup_data[CONF_LOCK_ENTITY_ID],
keymaster_config_entry_id=config_entry.entry_id,
alarm_level_or_user_code_entity_id=config_entry.data.get(
CONF_ALARM_LEVEL_OR_USER_CODE_ENTITY_ID
),
alarm_type_or_access_control_entity_id=config_entry.data.get(
alarm_level_or_user_code_entity_id=setup_data.get(CONF_ALARM_LEVEL_OR_USER_CODE_ENTITY_ID),
alarm_type_or_access_control_entity_id=setup_data.get(
CONF_ALARM_TYPE_OR_ACCESS_CONTROL_ENTITY_ID
),
door_sensor_entity_id=config_entry.data.get(CONF_DOOR_SENSOR_ENTITY_ID),
number_of_code_slots=config_entry.data[CONF_SLOTS],
starting_code_slot=config_entry.data[CONF_START],
door_sensor_entity_id=setup_data.get(CONF_DOOR_SENSOR_ENTITY_ID),
number_of_code_slots=setup_data[CONF_SLOTS],
starting_code_slot=setup_data[CONF_START],
code_slots=code_slots,
parent_name=config_entry.data.get(CONF_PARENT),
parent_config_entry_id=config_entry.data.get(CONF_PARENT_ENTRY_ID),
notify_script_name=config_entry.data.get(CONF_NOTIFY_SCRIPT_NAME),
parent_name=setup_data.get(CONF_PARENT),
parent_config_entry_id=setup_data.get(CONF_PARENT_ENTRY_ID),
notify_script_name=setup_data.get(CONF_NOTIFY_SCRIPT_NAME),
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This change from add_lock(kmlock=kmlock) to add_lock(kmlock=kmlock, update=True) makes every setup/reload unconditionally force-update the lock object via _update_lock().

This is the likely cause of the 12 "Lingering timer" test failures in CI — during setup, if a lock already exists in the coordinator (e.g. reload scenario), update=True triggers _update_lock() which may not properly clean up existing timers before replacing the lock's state.

Previously, add_lock without update=True would skip re-adding an already-known lock (unless it was pending_delete). Forcing update=True unconditionally changes the reload behavior for all locks, not just those with parent relationships.

Suggestion: Consider scoping this to only use update=True when the parent relationship has actually changed, or verify that _update_lock() properly handles timer cleanup. The broad unconditional update is risky for locks that don't need it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Follow-up: I see you've added test_setup_entry_calls_add_lock_with_update_true which validates the update=True call is made, and the mock_async_call_later fixture change in conftest.py likely resolves the lingering timer test failures.

However, my architectural concern remains unanswered: why does every lock need update=True unconditionally on every setup/reload? The previous behavior only skipped re-adding known locks (unless pending_delete). Now every reload forces _update_lock() on all locks regardless of whether their parent relationship changed.

Could you explain the rationale? Is this needed to handle a specific scenario, or would it be possible to scope this to only locks whose parent relationship has actually changed? Understanding your reasoning here would help move this forward.

try:
await coordinator.add_lock(kmlock=kmlock)
await coordinator.add_lock(kmlock=kmlock, update=True)
except asyncio.exceptions.CancelledError as e:
_LOGGER.error("Timeout on add_lock. %s: %s", e.__class__.__qualname__, e)

await hass.config_entries.async_forward_entry_setups(config_entry, PLATFORMS)
await async_generate_lovelace(
hass=hass,
kmlock_name=config_entry.data[CONF_LOCK_NAME],
kmlock_name=setup_data[CONF_LOCK_NAME],
keymaster_config_entry_id=config_entry.entry_id,
parent_config_entry_id=config_entry.data.get(CONF_PARENT_ENTRY_ID),
code_slot_start=config_entry.data[CONF_START],
code_slots=config_entry.data[CONF_SLOTS],
lock_entity=config_entry.data[CONF_LOCK_ENTITY_ID],
advanced_date_range=config_entry.data[CONF_ADVANCED_DATE_RANGE],
advanced_day_of_week=config_entry.data[CONF_ADVANCED_DAY_OF_WEEK],
door_sensor=config_entry.data.get(CONF_DOOR_SENSOR_ENTITY_ID),
hide_pins=config_entry.data.get(CONF_HIDE_PINS, False),
parent_config_entry_id=setup_data.get(CONF_PARENT_ENTRY_ID),
code_slot_start=setup_data[CONF_START],
code_slots=setup_data[CONF_SLOTS],
lock_entity=setup_data[CONF_LOCK_ENTITY_ID],
advanced_date_range=setup_data[CONF_ADVANCED_DATE_RANGE],
advanced_day_of_week=setup_data[CONF_ADVANCED_DAY_OF_WEEK],
door_sensor=setup_data.get(CONF_DOOR_SENSOR_ENTITY_ID),
hide_pins=setup_data.get(CONF_HIDE_PINS, False),
)

return True
Expand Down
9 changes: 8 additions & 1 deletion custom_components/keymaster/coordinator.py
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,14 @@ def _kmlocks_to_dict(self, instance: object) -> object:

async def _rebuild_lock_relationships(self) -> None:
for keymaster_config_entry_id, kmlock in self.kmlocks.items():
if kmlock.parent_name is not None:
if (
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good improvement — prioritizing an already-resolved parent_config_entry_id before falling back to name matching is the correct order of operations. This avoids redundant lookups and prevents stale name matches from overriding explicit relationships.

One edge case to consider: if parent_config_entry_id points to a config entry that was deleted/unloaded (i.e., it's not in self.kmlocks), the code correctly falls through to the elif branch for name-based matching. Worth adding a test for this scenario.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Follow-up: Thanks for adding test_rebuild_prefers_explicit_parent_id_over_parent_name — that covers the happy path well and validates the reordering logic.

The edge case I mentioned is still untested though: what happens when parent_config_entry_id is set but points to a config entry that has been deleted or unloaded (i.e., it's not present in self.kmlocks)? The code should fall through to name-based matching in that scenario. A test for this would give confidence that the fallback works correctly.

Could you add that test case, or explain why you think it's unnecessary?

kmlock.parent_config_entry_id is not None
and kmlock.parent_config_entry_id in self.kmlocks
):
parent_lock = self.kmlocks[kmlock.parent_config_entry_id]
if keymaster_config_entry_id not in parent_lock.child_config_entry_ids:
parent_lock.child_config_entry_ids.append(keymaster_config_entry_id)
elif kmlock.parent_name is not None:
for parent_config_entry_id, parent_lock in self.kmlocks.items():
if kmlock.parent_name == parent_lock.lock_name:
if kmlock.parent_config_entry_id is None:
Expand Down
11 changes: 7 additions & 4 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ async def disconnect():
client.connect = AsyncMock(side_effect=connect)
client.listen = AsyncMock(side_effect=listen)
client.disconnect = AsyncMock(side_effect=disconnect)
client.disable_server_logging = MagicMock()
client.disable_server_logging = AsyncMock(return_value=None)
client.driver = Driver(
client, copy.deepcopy(controller_state), copy.deepcopy(log_config_state)
)
Expand Down Expand Up @@ -381,9 +381,12 @@ def mock_async_call_later():
with patch("homeassistant.helpers.event.async_call_later") as mock:

def immediate_call(hass, delay, callback):
# Immediately call the callback with a mock `hass` object
del hass, delay # Parameters not used in immediate callback
return callback(None)
del hass, delay, callback # Parameters not used in the no-op mock

def _unsubscribe() -> None:
return None

return _unsubscribe

mock.side_effect = immediate_call
yield mock
Expand Down
35 changes: 35 additions & 0 deletions tests/test_coordinator.py
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,41 @@ async def test_rebuild_with_valid_parent_child_relationship(self, mock_coordinat
assert "child_id" in parent_lock.child_config_entry_ids
assert child_lock.parent_config_entry_id == "parent_id"

async def test_rebuild_prefers_explicit_parent_id_over_parent_name(self, mock_coordinator):
"""Test that explicit parent_config_entry_id wins over stale parent_name."""
parent_a = Mock(spec=KeymasterLock)
parent_a.keymaster_config_entry_id = "parent_a"
parent_a.lock_name = "Front Door"
parent_a.child_config_entry_ids = []
parent_a.parent_config_entry_id = None
parent_a.parent_name = None

parent_b = Mock(spec=KeymasterLock)
parent_b.keymaster_config_entry_id = "parent_b"
parent_b.lock_name = "Back Door"
parent_b.child_config_entry_ids = []
parent_b.parent_config_entry_id = None
parent_b.parent_name = None

child = Mock(spec=KeymasterLock)
child.keymaster_config_entry_id = "child"
child.lock_name = "Child"
child.child_config_entry_ids = []
child.parent_config_entry_id = "parent_b"
child.parent_name = "Front Door"

mock_coordinator.kmlocks = {
"parent_a": parent_a,
"parent_b": parent_b,
"child": child,
}

await mock_coordinator._rebuild_lock_relationships()

assert child.parent_config_entry_id == "parent_b"
assert "child" not in parent_a.child_config_entry_ids
assert "child" in parent_b.child_config_entry_ids

async def test_rebuild_with_mismatched_parent(self, mock_coordinator):
"""Test that child with mismatched parent is removed from old parent."""
# Arrange: Parent claims child, but child points to different parent
Expand Down
117 changes: 116 additions & 1 deletion tests/test_init.py
Original file line number Diff line number Diff line change
@@ -1,22 +1,28 @@
"""Test keymaster init."""

import logging
from unittest.mock import AsyncMock, patch
from unittest.mock import AsyncMock, Mock, patch

import pytest
from pytest_homeassistant_custom_component.common import MockConfigEntry

from custom_components.keymaster import async_setup_entry
from custom_components.keymaster.const import (
CONF_ADVANCED_DATE_RANGE,
CONF_ADVANCED_DAY_OF_WEEK,
CONF_ALARM_LEVEL_OR_USER_CODE_ENTITY_ID,
CONF_ALARM_TYPE_OR_ACCESS_CONTROL_ENTITY_ID,
CONF_DOOR_SENSOR_ENTITY_ID,
CONF_HIDE_PINS,
CONF_LOCK_ENTITY_ID,
CONF_LOCK_NAME,
CONF_NOTIFY_SCRIPT_NAME,
CONF_PARENT,
CONF_PARENT_ENTRY_ID,
CONF_SLOTS,
CONF_START,
DEFAULT_ADVANCED_DATE_RANGE,
DEFAULT_ADVANCED_DAY_OF_WEEK,
DOMAIN,
)
from homeassistant.components.sensor import DOMAIN as SENSOR_DOMAIN
Expand All @@ -35,6 +41,23 @@
KEYMASTER_SENSOR_COUNT = 8


def _build_entry_data(lock_name: str, lock_entity_id: str) -> dict:
"""Build minimal config entry data for async_setup_entry tests."""
return {
CONF_ALARM_LEVEL_OR_USER_CODE_ENTITY_ID: None,
CONF_ALARM_TYPE_OR_ACCESS_CONTROL_ENTITY_ID: None,
CONF_LOCK_ENTITY_ID: lock_entity_id,
CONF_LOCK_NAME: lock_name,
CONF_DOOR_SENSOR_ENTITY_ID: None,
CONF_SLOTS: 1,
CONF_START: 1,
CONF_NOTIFY_SCRIPT_NAME: None,
CONF_HIDE_PINS: False,
CONF_ADVANCED_DATE_RANGE: DEFAULT_ADVANCED_DATE_RANGE,
CONF_ADVANCED_DAY_OF_WEEK: DEFAULT_ADVANCED_DAY_OF_WEEK,
}


async def test_setup_entry(
hass,
lock_kwikset_910,
Expand Down Expand Up @@ -154,3 +177,95 @@ async def test_notify_script_name_slugified(hass):
await async_setup_entry(hass, entry)

assert entry.data[CONF_NOTIFY_SCRIPT_NAME] == "keymaster_akuvox_relay_a_manual_notify"


async def test_parent_title_resolves_to_parent_entry_id_and_setup_data_flow(hass):
"""Test parent title resolution and normalized setup_data propagation."""
parent_data = _build_entry_data("front_door", "lock.front_door")
parent_entry = MockConfigEntry(domain=DOMAIN, title="Front Door", data=parent_data, version=4)
parent_entry.add_to_hass(hass)

child_data = _build_entry_data("garage_door", "lock.garage_door")
child_data[CONF_PARENT] = "Front Door"
child_data[CONF_PARENT_ENTRY_ID] = None
child_entry = MockConfigEntry(domain=DOMAIN, title="Garage Door", data=child_data, version=4)
child_entry.add_to_hass(hass)

hass.data.setdefault(DOMAIN, {})

with (
patch("custom_components.keymaster.async_setup_services", new_callable=AsyncMock),
patch("custom_components.keymaster.KeymasterCoordinator") as mock_coordinator_class,
patch("custom_components.keymaster.dr.async_get") as mock_device_registry_get,
patch(
"custom_components.keymaster.async_generate_lovelace",
new_callable=AsyncMock,
) as mock_generate_lovelace,
patch.object(
hass.config_entries,
"async_forward_entry_setups",
new_callable=AsyncMock,
),
):
mock_coordinator = mock_coordinator_class.return_value
mock_coordinator.initial_setup = AsyncMock()
mock_coordinator.async_refresh = AsyncMock()
mock_coordinator.last_update_success = True
mock_coordinator.kmlocks = {}
mock_coordinator.add_lock = AsyncMock()

mock_device_registry = Mock()
mock_device_registry.async_get_or_create = Mock()
mock_device_registry_get.return_value = mock_device_registry

assert await async_setup_entry(hass, child_entry)

assert child_entry.data[CONF_PARENT_ENTRY_ID] == parent_entry.entry_id

add_lock_call = mock_coordinator.add_lock.await_args.kwargs
assert add_lock_call["update"] is True
assert add_lock_call["kmlock"].parent_name == "Front Door"
assert add_lock_call["kmlock"].parent_config_entry_id == parent_entry.entry_id

device_registry_call = mock_device_registry.async_get_or_create.call_args.kwargs
assert device_registry_call["via_device"] == (DOMAIN, parent_entry.entry_id)

lovelace_call = mock_generate_lovelace.await_args.kwargs
assert lovelace_call["parent_config_entry_id"] == parent_entry.entry_id


async def test_setup_entry_calls_add_lock_with_update_true(hass):
"""Test that setup always calls add_lock with update=True."""
entry_data = _build_entry_data("front_door", "lock.front_door")
entry = MockConfigEntry(domain=DOMAIN, title="Front Door", data=entry_data, version=4)
entry.add_to_hass(hass)

hass.data.setdefault(DOMAIN, {})

with (
patch("custom_components.keymaster.async_setup_services", new_callable=AsyncMock),
patch("custom_components.keymaster.KeymasterCoordinator") as mock_coordinator_class,
patch("custom_components.keymaster.dr.async_get") as mock_device_registry_get,
patch("custom_components.keymaster.async_generate_lovelace", new_callable=AsyncMock),
patch.object(
hass.config_entries,
"async_forward_entry_setups",
new_callable=AsyncMock,
),
):
mock_coordinator = mock_coordinator_class.return_value
mock_coordinator.initial_setup = AsyncMock()
mock_coordinator.async_refresh = AsyncMock()
mock_coordinator.last_update_success = True
mock_coordinator.add_lock = AsyncMock()
mock_coordinator.kmlocks = {entry.entry_id: Mock()}

mock_device_registry = Mock()
mock_device_registry.async_get_or_create = Mock()
mock_device_registry_get.return_value = mock_device_registry

assert await async_setup_entry(hass, entry)

add_lock_await_args = mock_coordinator.add_lock.await_args
assert add_lock_await_args is not None
assert add_lock_await_args.kwargs["update"] is True
Loading