diff --git a/custom_components/lock_code_manager/__init__.py b/custom_components/lock_code_manager/__init__.py index f70ccd104..46aeb36f6 100644 --- a/custom_components/lock_code_manager/__init__.py +++ b/custom_components/lock_code_manager/__init__.py @@ -128,7 +128,7 @@ MIN_PIN_LENGTH, generate_pin, ) -from .domain.queries import get_entry_config +from .domain.queries import get_entry_config, subentry_id_for_slot from .domain.references import async_notify_moved from .domain.services import ( async_add_users, @@ -1359,8 +1359,14 @@ def _is_ours(device: dr.DeviceEntry) -> bool: slot_num = parse_slot_unique_id(entry_id, entity.unique_id) if slot_num is None: continue + # Created under the user who holds the slot, like every other + # device this integration makes. Created bare, the first + # `async_add_entities` would pull it into that subentry implicitly -- + # which Home Assistant deprecates, and on the very upgrade path this + # function exists to serve. slot_device = dev_reg.async_get_or_create( config_entry_id=entry_id, + config_subentry_id=subentry_id_for_slot(config_entry, slot_num), **build_slot_device_info(config_entry, slot_num), ) _LOGGER.debug( @@ -1828,6 +1834,36 @@ async def async_update_listener( config_entry.runtime_data.settled.set() +@callback +def _async_settle_options( + hass: HomeAssistant, + config_entry: LockCodeManagerConfigEntry, + config: EntryConfig, +) -> None: + """ + Fold a staged options submission into the entry's own data. + + `data` is where the configuration lives; `options` is a staging area, + because an options flow cannot write `data` itself. Settling is what keeps + that true, so it happens on every path out of the update listener. + + Only when something is actually staged. Settling unconditionally would + corrupt the direct writes: `async_write_entry_config` reconciles the + subentries and the entry in separate calls, each of which wakes the + listener, and a pass that woke between them would write back the half of + the configuration it had read -- undoing the half that had already landed. + Those callers clear `options` themselves, so this is a no-op for them. + + ``to_dict()`` is what makes the stored data plain dicts rather than the + read-only ``MappingProxyType`` wrappers ``EntryConfig`` uses internally, + which Home Assistant's storage layer cannot serialize. + """ + if config_entry.options: + hass.config_entries.async_update_entry( + config_entry, data=config.to_dict(), options={} + ) + + async def _async_apply_entry_update( hass: HomeAssistant, config_entry: LockCodeManagerConfigEntry, @@ -1867,6 +1903,11 @@ async def _async_apply_entry_update( # produces no diff when it re-enters. diff = EntryConfigDiff(old=old_config, new=new_config) if not diff.has_changes: + # Settled here too. An options submission that changes nothing reaches + # this return, and leaving it staged would strand it in `options` for + # good -- read in preference to `data` for as long as the entry + # exists, then cleared by the next write that touches the entry. + _async_settle_options(hass, config_entry, new_config) return ent_reg = er.async_get(hass) @@ -1940,11 +1981,16 @@ async def _async_apply_entry_update( # anchored the slot; slot-only providers leave the default no-op in # place. This runs before ``locks_to_remove`` processing so providers # in ``runtime_data.locks`` are still usable. - # Drained, not read: a hand-off applies to the write that requested it, - # and leaving the pair behind would spare the next occupant of that slot - # number the cleanup it does need. + # Consumed pair by pair, not drained wholesale. A hand-off applies to the + # write that requested it and must not outlive it -- leaving a pair behind + # would spare the next occupant of that number the cleanup it does need -- + # but one `delete_user` call is several entry writes, one per departing + # user, and each wakes this listener. Taking the whole set on the first + # pass left every departure after the first one unprotected, and their + # credential was wiped off every lock despite the caller asking for it to + # be left programmed. retained_pairs = runtime_data.retained_pairs - runtime_data.retained_pairs = set() + runtime_data.retained_pairs = retained_pairs - diff.pairs_removed for lock_entity_id, slot_num in diff.pairs_removed: release_lock = runtime_data.locks.get(lock_entity_id) if release_lock is None: @@ -2021,31 +2067,7 @@ async def _async_apply_entry_update( _LOGGER.info( "%s (%s): Done creating and/or updating entities", entry_id, entry_title ) - # Only when there is something staged to settle. The options flow leaves - # its submission in `options` for this listener to fold into `data`; - # everything else writes `data` directly and clears `options` itself. - # - # An options submission that changes nothing does not reach here at all -- - # the diff above is empty and returns first -- so `options` is left - # standing. Harmless while it holds what the options flow writes, which is - # `data`'s own contents; a field that lived in `options` WITHOUT also being - # in `data` would go stale there, so do not add one. - # - # Settling unconditionally corrupts those direct writes. - # `async_write_entry_config` reconciles the subentries and the entry in - # separate calls, each of which wakes this listener -- and a pass that - # woke between them would write back the half of the configuration it - # read, undoing the half that had already landed. - # - # to_dict() is what makes the stored data plain dicts rather than the - # read-only MappingProxyType wrappers EntryConfig uses internally, which - # Home Assistant's storage layer cannot serialize. - if config_entry.options: - hass.config_entries.async_update_entry( - config_entry, data=new_config.to_dict(), options={} - ) - # The async_update_entry above re-triggers this listener, which - # refreshes runtime_data.config at the top before the early-return. + _async_settle_options(hass, config_entry, new_config) # Notify Lovelace dashboards to re-render when structure changes # (slots or locks added/removed), so strategy-generated cards update diff --git a/custom_components/lock_code_manager/config_flow.py b/custom_components/lock_code_manager/config_flow.py index 827c8547a..fc8dd227b 100644 --- a/custom_components/lock_code_manager/config_flow.py +++ b/custom_components/lock_code_manager/config_flow.py @@ -795,11 +795,17 @@ async def async_step_reconfigure( description_placeholders: dict[str, Any] = {} held = normalize_name(subentry.title) + # Everybody except the person being edited. Both the name check and, + # below, the count allocation is asked for are about the OTHERS: the + # edited user is already in `config.users`, so counting them again + # refuses on a lock with exactly enough room, and reconciling against + # a dict that still holds their old name issues their new one a + # second number. + others = {other: user for other, user in config.users.items() if other != held} + if user_input is not None: name, fields, errors, placeholders = _validate_user_form( - self.hass, - user_input, - [other for other in config.users if other != held], + self.hass, user_input, others ) description_placeholders.update(placeholders) @@ -822,7 +828,7 @@ async def async_step_reconfigure( allocation_errors, allocation_placeholders, ) = await _allocate_for( - self.hass, entry, config.locks, len(config.users) + 1 + self.hass, entry, config.locks, len(others) + 1 ) if unavailable is None: errors.update(allocation_errors) @@ -831,7 +837,7 @@ async def async_step_reconfigure( subentry, user_input, errors, description_placeholders ) held_slot = config.assignment.reconcile( - {**config.users, name: fields}, + {**others, name: fields}, start=1, unavailable=unavailable, ).slot(name) diff --git a/custom_components/lock_code_manager/providers/zha.py b/custom_components/lock_code_manager/providers/zha.py index dfe223d4b..f92b44e97 100644 --- a/custom_components/lock_code_manager/providers/zha.py +++ b/custom_components/lock_code_manager/providers/zha.py @@ -54,14 +54,6 @@ DoorLock.OperationEvent.ScheduleUnlock: False, } -OPERATION_SOURCE_NAMES: dict[int, str] = { - DoorLock.OperationEventSource.Keypad: "Keypad", - DoorLock.OperationEventSource.RF: "RF", - DoorLock.OperationEventSource.Manual: "Manual", - DoorLock.OperationEventSource.RFID: "RFID", - DoorLock.OperationEventSource.Indeterminate: "Unknown", -} - @dataclass(repr=False, eq=False) class ZHALock(BaseLock): diff --git a/tests/test_config_flow.py b/tests/test_config_flow.py index 5f713b003..0fe5566ff 100644 --- a/tests/test_config_flow.py +++ b/tests/test_config_flow.py @@ -806,6 +806,85 @@ async def test_editing_a_user_who_has_no_number_issues_one( assert entry.subentries[subentry.subentry_id].data[CONF_SLOT] == 1 +async def test_repairing_an_unnumbered_user_fits_on_a_lock_with_exactly_enough_room( + hass: HomeAssistant, mock_lock_config_entry +): + """ + The repair asks for room for the OTHERS plus this one, not for one extra. + + The user being edited is already in the entry's users, so counting them + again refuses on a lock with exactly enough room -- naming a user count + that does not exist -- and the dialog that exists to repair the record + becomes one that can only fail. + """ + entry = MockConfigEntry( + domain=DOMAIN, + title="test", + data={CONF_LOCKS: [LOCK_1_ENTITY_ID]}, + subentries_data=[ + *user_subentries({1: {CONF_NAME: "Alice", CONF_PIN: "1111"}}), + unnumbered_user_subentry("Nomad", **{CONF_ENABLED: False}), + ], + unique_id="unnumbered-exact-fit", + ) + entry.add_to_hass(hass) + subentry = next(s for s in entry.subentries.values() if s.title == "Nomad") + + result = await hass.config_entries.subentries.async_init( + (entry.entry_id, SUBENTRY_TYPE_USER), + context={"source": "reconfigure", "subentry_id": subentry.subentry_id}, + ) + + # Two slots on the lock, Alice holds one, and two users all told. + with ( + patch.object(MockLCMLock, "async_get_max_slot", AsyncMock(return_value=2)), + _holding(), + ): + result = await hass.config_entries.subentries.async_configure( + result["flow_id"], + {CONF_NAME: "Nomad", CONF_ENABLED: True, CONF_PIN: "4242"}, + ) + + assert result["type"] == "abort" + assert entry.subentries[subentry.subentry_id].data[CONF_SLOT] == 2 + + +async def test_repairing_an_unnumbered_user_while_renaming_them_wastes_no_slot( + hass: HomeAssistant, mock_lock_config_entry +): + """ + A repair that also renames issues one number, from the bottom. + + Reconciling against a dict that still holds the old name puts the same + person in twice under two identities, so the new one is numbered around + the old one and the lowest free position is burned for good. + """ + entry = MockConfigEntry( + domain=DOMAIN, + title="test", + data={CONF_LOCKS: [LOCK_1_ENTITY_ID]}, + subentries_data=[unnumbered_user_subentry("Nomad", **{CONF_ENABLED: False})], + unique_id="unnumbered-renamed", + ) + entry.add_to_hass(hass) + subentry = next(iter(entry.subentries.values())) + + result = await hass.config_entries.subentries.async_init( + (entry.entry_id, SUBENTRY_TYPE_USER), + context={"source": "reconfigure", "subentry_id": subentry.subentry_id}, + ) + + with _holding(): + result = await hass.config_entries.subentries.async_configure( + result["flow_id"], + {CONF_NAME: "Zed", CONF_ENABLED: True, CONF_PIN: "4242"}, + ) + + assert result["type"] == "abort" + assert entry.subentries[subentry.subentry_id].title == "Zed" + assert entry.subentries[subentry.subentry_id].data[CONF_SLOT] == 1 + + async def test_editing_an_unnumbered_user_refuses_when_the_lock_cannot_be_read( hass: HomeAssistant, mock_lock_config_entry ): diff --git a/tests/test_init.py b/tests/test_init.py index 059532660..8b4dd41c5 100644 --- a/tests/test_init.py +++ b/tests/test_init.py @@ -2113,6 +2113,33 @@ async def _fail_lock_2(self: MockLCMLock, config_entry) -> None: await hass.config_entries.async_unload(entry.entry_id) +async def test_an_options_submission_that_changes_nothing_still_settles( + hass: HomeAssistant, mock_lock_config_entry, lock_code_manager_config_entry +) -> None: + """ + `options` is a staging area, and it is emptied even when nothing changed. + + The update listener returns early when the submission produces no diff, + and that was the one path out of it that skipped settling. What was left + behind read fine -- `options` holds what `data` holds -- right up until + the next write cleared it, which would take anything that lived only + there with it. + """ + entry = lock_code_manager_config_entry + locks = list(get_entry_config(entry).locks) + assert entry.options == {} + + result = await hass.config_entries.options.async_init(entry.entry_id) + result = await hass.config_entries.options.async_configure( + result["flow_id"], user_input={CONF_LOCKS: locks} + ) + await hass.async_block_till_done() + + assert result["type"] == "create_entry" + assert entry.options == {} + assert entry.data[CONF_LOCKS] == locks + + async def test_migration_v5_moves_an_existing_slot_device_to_its_user( hass: HomeAssistant, mock_lock_config_entry ) -> None: diff --git a/tests/test_services.py b/tests/test_services.py index ef34c5532..24de33470 100644 --- a/tests/test_services.py +++ b/tests/test_services.py @@ -596,6 +596,43 @@ async def test_delete_user_service_can_hand_the_credential_over( assert not entry.runtime_data.retained_pairs +async def test_delete_user_hands_over_every_credential_not_just_the_first( + hass: HomeAssistant, + mock_lock_config_entry, + lock_code_manager_config_entry, +) -> None: + """ + A batch hand-off applies to everybody in it. + + One `delete_user` call is several entry writes -- a subentry removal per + departing user -- and each one wakes the update listener. The listener + drained the whole hand-off set on its first pass, so from the second user + onward the credential the caller explicitly asked to leave programmed was + wiped off every lock instead. + """ + entry = lock_code_manager_config_entry + for lock in entry.runtime_data.locks.values(): + lock.async_release_managed_slot = AsyncMock() + + await hass.services.async_call( + DOMAIN, + SERVICE_DELETE_USER, + { + "config_entry_id": entry.entry_id, + CONF_NAME: ["test1", "test2"], + ATTR_CLEAR_CREDENTIALS: False, + }, + blocking=True, + ) + await hass.async_block_till_done() + + config = get_entry_config(hass.config_entries.async_get_entry(entry.entry_id)) + assert not config.users + for lock in entry.runtime_data.locks.values(): + lock.async_release_managed_slot.assert_not_called() + assert not entry.runtime_data.retained_pairs + + async def test_delete_user_service_unknown_name( hass: HomeAssistant, mock_lock_config_entry,