Skip to content

feat: fixing device migration #57

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mikart143 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: fixing device migration #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e68c849
eat: fixing device migration
mikart143 Aug 18, 2025
bc4c0c2
feat: fixing tests and config migration
mikart143 Aug 18, 2025
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
87 changes: 67 additions & 20 deletions custom_components/inpost_air/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
from homeassistant.const import Platform
from homeassistant.core import HomeAssistant
from homeassistant.helpers import device_registry as dr
from homeassistant.helpers import entity_registry as er

from custom_components.inpost_air.coordinator import InPostAirDataCoordinator
from custom_components.inpost_air.models import ParcelLocker
from custom_components.inpost_air.utils import get_device_info, get_parcel_locker_url
from custom_components.inpost_air.const import DOMAIN

from .api import InPostAirPoint, InPostApi

Expand All @@ -29,12 +31,12 @@ class InPostAirData:
coordinator: InPostAirDataCoordinator


type InPostAirConfiEntry = ConfigEntry[InPostAirData]
type InPostAirConfigEntry = ConfigEntry[InPostAirData]

PLATFORMS: list[Platform] = [Platform.SENSOR]


async def async_setup_entry(hass: HomeAssistant, entry: InPostAirConfiEntry) -> bool:
async def async_setup_entry(hass: HomeAssistant, entry: InPostAirConfigEntry) -> bool:
"""Set up InPost Air from a config entry."""
api_client = InPostApi(hass)
entry_data = entry.data.get("parcel_locker")
Expand Down Expand Up @@ -71,30 +73,75 @@ async def async_setup_entry(hass: HomeAssistant, entry: InPostAirConfiEntry) ->
return True


async def async_unload_entry(hass: HomeAssistant, entry: InPostAirConfiEntry) -> bool:
async def async_unload_entry(hass: HomeAssistant, entry: InPostAirConfigEntry) -> bool:
"""Unload a config entry."""
return await hass.config_entries.async_unload_platforms(entry, PLATFORMS)


async def async_migrate_entry(hass: HomeAssistant, config_entry: InPostAirConfiEntry):
"""Migrate old entry."""
_LOGGER.debug(
"Migrating %s from version %s", config_entry.title, config_entry.version
)
Comment on lines -81 to -83
Copy link
Copy Markdown
Collaborator

@ceski23 ceski23 Aug 19, 2025

Choose a reason for hiding this comment

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

let's keep that for debugging purposes

async def async_migrate_entry(hass: HomeAssistant, entry: InPostAirConfigEntry) -> bool:
"""Remove legacy 3-tuple devices left by ≤ 1.4.x."""
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

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

The migration only handles version 1 but should also handle the case where entry.version == 2 by returning True immediately without processing, as shown in the removed code.

Suggested change
"""Remove legacy 3-tuple devices left by ≤ 1.4.x."""
"""Remove legacy 3-tuple devices left by ≤ 1.4.x."""
if entry.version == 2:
return True

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

@ceski23 ceski23 Aug 19, 2025

Choose a reason for hiding this comment

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

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


if config_entry.version > 2:
if entry.version > 2:
# This means the user has downgraded from a future version
return False

if config_entry.version == 1:
hass.config_entries.async_update_entry(
config_entry,
data={"parcel_locker": from_dict(InPostAirPoint, config_entry.data)},
version=2,
)
Comment on lines -90 to -94
Copy link
Copy Markdown
Collaborator

@ceski23 ceski23 Aug 19, 2025

Choose a reason for hiding this comment

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

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


_LOGGER.debug(
"Migrating %s to version %s completed", config_entry.title, config_entry.version
)

if entry.version == 1: # 1.5.0 → 1.5.1
Copy link
Copy Markdown
Collaborator

@ceski23 ceski23 Aug 19, 2025

Choose a reason for hiding this comment

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

version 2 was introduced in 1.5.0

Suggested change
if entry.version == 1: # 1.5.0 → 1.5.1
if entry.version == 1: # < 1.5.0

dev_reg = dr.async_get(hass)
ent_reg = er.async_get(hass)

for device in list(dev_reg.devices.values()):
Copy link
Copy Markdown
Collaborator

@ceski23 ceski23 Aug 19, 2025

Choose a reason for hiding this comment

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

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

if entry.entry_id not in device.config_entries:
continue

# Detect an old device with a single 3-element identifier
old_id = next(
(i for i in device.identifiers if len(i) == 3 and i[0] == DOMAIN),
None,
)
if not old_id:
continue

# Skip if it still has entities (unexpected edge-case)
device_entities = er.async_entries_for_device(
registry=ent_reg, device_id=device.id
)
if device_entities:
_LOGGER.warning(
"Legacy device %s still has entities; skipping cleanup", device.id
)
continue

# Optionally carry over area/name/disabled flags to the new device
_, code, _ = old_id
new_dev = next(
(
d
for d in dev_reg.devices.values()
if (DOMAIN, code) in d.identifiers
and entry.entry_id in d.config_entries
and d.id != device.id
),
None,
)
if new_dev:
# Convert string disabled_by to enum if needed
old_disabled_by = device.disabled_by
if isinstance(old_disabled_by, str):
old_disabled_by = dr.DeviceEntryDisabler(old_disabled_by)

new_disabled_by = new_dev.disabled_by
if isinstance(new_disabled_by, str):
new_disabled_by = dr.DeviceEntryDisabler(new_disabled_by)
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
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)

Copilot uses AI. Check for mistakes.

dev_reg.async_update_device(
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,
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
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,

Copilot uses AI. Check for mistakes.
)

# Remove the orphaned 3-tuple device
dev_reg.async_remove_device(device.id)

entry.version = 2 # mark migration complete
return True
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

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

The migration function should check if entry.version > 2 and return False for unsupported downgrades, similar to the original implementation that was removed.

Copilot uses AI. Check for mistakes.
Loading