Conversation
Coverage Report
|
tests: Run #53
🎉 All tests passed!Github Test Reporter by CTRF 💚 🔄 This comment has been updated |
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a device migration system for the InPost Air integration to clean up legacy device identifiers left from versions ≤ 1.4.x. The migration logic removes old 3-tuple device identifiers while preserving user settings by transferring them to corresponding new devices with 2-tuple identifiers.
- Added migration logic to identify and remove legacy devices with 3-tuple identifiers
- Implemented settings transfer from old devices to new devices when they exist
- Created comprehensive test coverage for migration scenarios including edge cases
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| custom_components/inpost_air/init.py | Added device migration logic and fixed type annotation typo |
| tests/test_migration.py | Unit tests for migration functionality with mocked dependencies |
| tests/test_migration_integration.py | Integration tests for migration using real Home Assistant device registry |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| new_disabled_by = new_dev.disabled_by | ||
| if isinstance(new_disabled_by, str): | ||
| new_disabled_by = dr.DeviceEntryDisabler(new_disabled_by) |
There was a problem hiding this comment.
The string-to-enum conversion logic for disabled_by is duplicated for both old_disabled_by and new_disabled_by. Consider extracting this into a helper function to reduce code duplication.
| new_disabled_by = dr.DeviceEntryDisabler(new_disabled_by) | |
| def _normalize_disabled_by(value): | |
| return dr.DeviceEntryDisabler(value) if isinstance(value, str) else value | |
| old_disabled_by = _normalize_disabled_by(device.disabled_by) | |
| new_disabled_by = _normalize_disabled_by(new_dev.disabled_by) |
| new_dev.id, | ||
| area_id=device.area_id or new_dev.area_id, | ||
| name_by_user=device.name_by_user or new_dev.name_by_user, | ||
| disabled_by=old_disabled_by or new_disabled_by, |
There was a problem hiding this comment.
Using 'or' operator with DeviceEntryDisabler enums may not work as expected. If old_disabled_by is DeviceEntryDisabler.USER (which is truthy), it will be used correctly. However, if it's None, new_disabled_by will be used. But if old_disabled_by is a falsy enum value like DeviceEntryDisabler.INTEGRATION (if it exists), the logic might not behave as intended. Consider using explicit None checks instead.
| disabled_by=old_disabled_by or new_disabled_by, | |
| disabled_by=old_disabled_by if old_disabled_by is not None else new_disabled_by, |
| _LOGGER.debug( | ||
| "Migrating %s from version %s", config_entry.title, config_entry.version | ||
| ) |
There was a problem hiding this comment.
let's keep that for debugging purposes
| "Migrating %s to version %s completed", config_entry.title, config_entry.version | ||
| ) | ||
|
|
||
| if entry.version == 1: # 1.5.0 → 1.5.1 |
There was a problem hiding this comment.
version 2 was introduced in 1.5.0
| if entry.version == 1: # 1.5.0 → 1.5.1 | |
| if entry.version == 1: # < 1.5.0 |
| hass.config_entries.async_update_entry( | ||
| config_entry, | ||
| data={"parcel_locker": from_dict(InPostAirPoint, config_entry.data)}, | ||
| version=2, | ||
| ) |
There was a problem hiding this comment.
what happened to the original migration logic? VERSION 2 introduced storage of runtime data in config entry. I don't see logic that updates config_entry to handle that
| dev_reg = dr.async_get(hass) | ||
| ent_reg = er.async_get(hass) | ||
|
|
||
| for device in list(dev_reg.devices.values()): |
There was a problem hiding this comment.
wouldn't be easier to just find device with 3-tuple id in registry instead of iterating over all devices? (especially since async_migrate_entry will be invoked for each configured parcel locker).
If there is such device then update it's ID to new format, else delete it
| "Migrating %s from version %s", config_entry.title, config_entry.version | ||
| ) | ||
| async def async_migrate_entry(hass: HomeAssistant, entry: InPostAirConfigEntry) -> bool: | ||
| """Remove legacy 3-tuple devices left by ≤ 1.4.x.""" |
There was a problem hiding this comment.
this method will be invoked each time we will bump config entry version, not just for 3-tuple ID change. we should revert old comment
This pull request introduces a migration for legacy device identifiers in the
inpost_airHome Assistant integration, ensuring that old 3-tuple devices are properly cleaned up and their user settings are preserved. The migration logic is thoroughly tested with new integration tests to guarantee correct behavior and idempotency.Migration logic for legacy devices:
async_migrate_entryfunction incustom_components/inpost_air/__init__.pynow removes legacy devices with 3-tuple identifiers left by versions ≤ 1.4.x. If a corresponding new device exists, user settings (area, name, disabled status) are transferred. The migration marks the config entry as upgraded to version 2.entity_registryandDOMAINare added in__init__.pyto support the migration process.Integration tests for migration:
tests/test_migration_integration.pyverify that legacy devices are removed, settings are transferred to new devices, orphaned devices are cleaned up, and the migration is idempotent (safe to run multiple times).