Remove old Airthings devices #145914
Remove old Airthings devices #145914
Conversation
|
Hey there @Danielhiversen, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| def _remove_old_devices( | ||
| hass: HomeAssistant, | ||
| entry: AirthingsConfigEntry, | ||
| airthings_devices: dict[str, AirthingsDevice], | ||
| ) -> None: | ||
| device_registry = dr.async_get(hass) | ||
|
|
||
| for registered_device in device_registry.devices.get_devices_for_config_entry_id( | ||
| entry.entry_id | ||
| ): | ||
| device_id = next( | ||
| (i[1] for i in registered_device.identifiers if i[0] == DOMAIN), None | ||
| ) | ||
| if device_id and device_id not in airthings_devices: | ||
| device_registry.async_update_device( | ||
| registered_device.id, remove_config_entry_id=entry.entry_id | ||
| ) |
There was a problem hiding this comment.
Is the accumulation of old devices something that we can avoid? For example could they be removed on a case by case basis when they are unpaired?
If that is possible, then stray devices could be removed via a repair action where the user has to confirm it.
My concern is that if something goes wrong with the API, this code might delete a load of user settings that they might want.
There was a problem hiding this comment.
Automatically removing devices is fine and in theory is the behavior we want, BUT the api needs to be reliable. As in, if a device isn't returned when it's offline, then we should not do it.
We can't avoid API errors where the API would return zero devices, but I think that's very rare.
A good thing to keep in mind is that if a device is created again with the same unique id, the old name and such is used again.
There was a problem hiding this comment.
The API will never remove the device without the user removing it from the app. If the device is offline, we'll just get the latest values. If something fails, the API will return a 5xx error, not return an incomplete device list.
But of course, theoretically something could go wrong and the API could miss a device or something. So maybe I should just change the implementation?
A good thing to keep in mind is that if a device is created again with the same unique id, the old name and such is used again.
We use serial number as unique id, so the old one will be used.
There was a problem hiding this comment.
But of course, theoretically something could go wrong and the API could miss a device or something. So maybe I should just change the implementation?
Generally we don't take that into account, so I think this isn't as much of a concern
There was a problem hiding this comment.
I was thinking of that occasional problem where some elements fail to load on a web page and you have to refresh it.
For example if logging in and downloading the device list are done in two transactions.
But if that's impossible or a low enough risk then ok by me!
I would suggest that if there's no user confirmation we should put a message in the log when autodeleting something from the config.
There was a problem hiding this comment.
I think an integration should retry set up if it does not have all the information needed to set up :)
There was a problem hiding this comment.
pyAirthings will raise an exception if anything fails:
https://github.com/Danielhiversen/pyAirthings/blob/main/airthings/__init__.py#L184
We need locations (which is cached) + devices.
I would suggest that if there's no user confirmation we should put a message in the log when autodeleting something from the config.
That I can do.
And FYI: I more or less followed the first example here:
https://developers.home-assistant.io/docs/core/integration-quality-scale/rules/stale-devices
If this is not the "best" way of doing it, should have logs when doing it etc, this should probably be updated with some more details
Co-authored-by: Dave T <17680170+davet2001@users.noreply.github.com>
| ) | ||
| first_entry.add_to_hass(hass) | ||
|
|
||
| device_registry = dr.async_get(hass) |
There was a problem hiding this comment.
Device registry can be loaded as test parameter instead
|
|
||
| # Fetch new data with no devices | ||
| with patch( | ||
| "homeassistant.components.airthings.coordinator.AirthingsDataUpdateCoordinator._update_method", |
There was a problem hiding this comment.
Why do we patch an internal? I'd rather mock the library
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
@joostlek I’ve rewritten the test and added a few more to ensure I didn't break anything (these probably belonged in a separate PR). The new tests brings us to 100% test coverage. Let me know if you’d prefer I move the extra tests to a new PR or update the existing Airthings test coverage PR instead. |
Breaking change
Proposed change
I had a long list of old Airthings devices that had all been removed, but I wasn't able to remove them. This PR will make sure to remove old devices that are no longer included in the list of devices from the API. This will happen if you remove (unpair) a device.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: