Fix unique_id generation for AtwZoneSensors #51227
Conversation
probot-home-assistant
bot
added
bugfix
has-tests
integration: melcloud
small-pr
|
Please fix the broken import in the test. |
The existing unique_id generation generated the same ids for both zones in 2 zone setups. This change adds a zone-<index> suffix to the id breaking existing 1 zone setups.
vilppuvuorinen
force-pushed
the
bugfix/fix-atw-zone-sensor-aliasing
branch
from
1ce1408
to
c6528c9
Compare
|
I'm having issues running the tests locally in my WSL2 based dev environment. This could take a while. Edit: Nvm, I got it working. |
vilppuvuorinen
force-pushed
the
bugfix/fix-atw-zone-sensor-aliasing
branch
from
c6528c9
to
9e91b6f
Compare
| @@ -165,7 +165,8 @@ class AtwZoneSensor(MelDeviceSensor): | |||
|
|
|||
| def __init__(self, api: MelCloudDevice, zone: Zone, measurement, definition): | |||
| """Initialize the sensor.""" | |||
| super().__init__(api, measurement, definition) | |||
| full_measurement = f"{measurement}-zone-{zone.zone_index}" | |||
There was a problem hiding this comment.
Can we avoid adding the zone_index if the index is 0? That way it will minimize the impact of the breaking change.
There was a problem hiding this comment.
I'd rather check whether the device has more than 1 zones and add the suffix only in those cases. That way the 1 zone variant would not be impacted and the previously broken 2 zone setups would have consistent IDs. What do you think?
There was a problem hiding this comment.
The goal is to reduce the breaking changes. So if Home Assistant used to always receive index 0 first, that data is correct and it just ignored other zones. So by keeping index 0 in the old format, less will break.
There was a problem hiding this comment.
I implemented the zone_index == 1 exception.
The solution I proposed has similar goals and effects.
- If a device has a single zone, the
unique_idwould remain the same. - If a device has 2 zones, the sensors were unusable and there is nothing to break with the different
unique_idgeneration.
|
|
||
| def test_zone_unique_ids(mock_device, mock_zone_1, mock_zone_2): | ||
| """Test unique id generation correctness.""" | ||
| sensor_1 = AtwZoneSensor(mock_device, mock_zone_1, "room_temperature", {}) |
There was a problem hiding this comment.
We should set up the integration and assert the entity unique_id via the entity registry. We shouldn't interact with the entity instance directly.
https://developers.home-assistant.io/docs/development_testing#writing-tests-for-integrations
Breaking change
This change changes
unique_idgeneration for all ATW zone sensors and therefore it will require manual intervention to replace sensor references in existing 1 zone setups.The existing ID scheme (
{measurement}) is replaced by{measurement}-zone-{zone index}. It would be possible to keep the existing scheme for the first, currently functional zone, but the mixed scheme is just terrible. 2 zone setups would have sensor IDs like these:{measurement}{measurement}-zone-2Proposed change
Add zone index to ATW zone sensor
unique_ids. The current implementation does not generate uniqueunique_ids with 2 zone setups.Type of change
Additional information
Checklist
black --fast 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..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: