Conversation
|
Hey there @iMicknl, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
This pull request adds placeOID-based device grouping for the Overkiz integration, enabling automatic detection and separation of sub-devices belonging to different physical locations. Previously, all sub-devices (e.g., thermostats, water heaters, sensors) sharing a base device URL were grouped under a single device. Now, when sub-devices have different placeOID values, they are split into separate devices with proper hierarchical linking to the main actuator device.
Changes:
- Introduced placeOID-based device grouping logic to automatically detect when sub-devices belong to different physical locations
- Modified device identifier generation to use
base_url#place_oidformat when grouping is enabled - Updated via_device linking so sub-devices link to the main actuator's device instead of directly to the gateway
- Added comprehensive unit tests for the new helper methods
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| homeassistant/components/overkiz/entity.py | Core logic changes: added 3 new helper methods (_get_sibling_devices, _has_siblings_with_different_place_oid, _is_main_device_for_place_oid, _get_via_device_id) and modified generate_device_info to support placeOID-based grouping |
| tests/components/overkiz/test_entity.py | New test file with 4 test functions covering the main scenarios for placeOID detection, main device identification, and via_device linking |
| def test_get_via_device_id_sub_device_links_to_main() -> None: | ||
| """Test sub-device links to main actuator with placeOID grouping.""" | ||
| devices = [ | ||
| _create_mock_device("io://gateway/123#1", "place-a", "Actuator"), | ||
| _create_mock_device("io://gateway/123#2", "place-b", "Zone"), | ||
| ] | ||
| entity = _create_mock_entity(devices[1], devices) | ||
| entity.executor = Mock() | ||
| entity.executor.get_gateway_id = Mock(return_value="gateway-id") | ||
|
|
||
| result = OverkizEntity._get_via_device_id(entity, use_place_oid_grouping=True) | ||
|
|
||
| assert result == "io://gateway/123#place-a" | ||
|
|
||
|
|
||
| def test_get_via_device_id_main_device_links_to_gateway() -> None: | ||
| """Test main device (#1) links to gateway.""" | ||
| devices = [ | ||
| _create_mock_device("io://gateway/123#1", "place-a", "Actuator"), | ||
| ] | ||
| entity = _create_mock_entity(devices[0], devices) | ||
| entity.executor = Mock() | ||
| entity.executor.get_gateway_id = Mock(return_value="gateway-id") | ||
|
|
||
| result = OverkizEntity._get_via_device_id(entity, use_place_oid_grouping=True) | ||
|
|
||
| assert result == "gateway-id" | ||
|
|
||
|
|
||
| def test_has_siblings_with_no_place_oid() -> None: | ||
| """Test device with no placeOID returns False.""" | ||
| devices = [ | ||
| _create_mock_device("io://gateway/123#1", None, "Device 1"), | ||
| _create_mock_device("io://gateway/123#2", "place-b", "Device 2"), | ||
| ] | ||
| entity = _create_mock_entity(devices[0], devices) | ||
|
|
||
| result = OverkizEntity._has_siblings_with_different_place_oid(entity) | ||
|
|
||
| assert result is False | ||
|
|
||
|
|
||
| def test_is_main_device_with_no_place_oid() -> None: | ||
| """Test device with no placeOID is always considered main.""" | ||
| devices = [ | ||
| _create_mock_device("io://gateway/123#2", None, "Device 2"), | ||
| _create_mock_device("io://gateway/123#1", "place-a", "Device 1"), | ||
| ] | ||
| entity = _create_mock_entity(devices[0], devices) | ||
|
|
||
| result = OverkizEntity._is_main_device_for_place_oid(entity) | ||
|
|
||
| assert result is True | ||
|
|
||
|
|
||
| def test_get_via_device_id_main_device_without_place_oid() -> None: | ||
| """Test fallback to gateway when #1 device has no placeOID.""" | ||
| devices = [ | ||
| _create_mock_device("io://gateway/123#1", None, "Actuator"), | ||
| _create_mock_device("io://gateway/123#2", "place-b", "Zone"), | ||
| ] | ||
| entity = _create_mock_entity(devices[1], devices) | ||
| entity.executor = Mock() | ||
| entity.executor.get_gateway_id = Mock(return_value="gateway-id") | ||
|
|
||
| result = OverkizEntity._get_via_device_id(entity, use_place_oid_grouping=True) | ||
|
|
||
| assert result == "gateway-id" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("device_url", "expected"), | ||
| [ | ||
| ("io://gateway/123#4", 4), | ||
| ("io://gateway/123#10", 10), | ||
| ("io://gateway/123#abc", None), | ||
| ("io://gateway/123#", None), | ||
| ], | ||
| ids=["single_digit", "multi_digit", "non_numeric", "empty_suffix"], | ||
| ) | ||
| def test_get_device_index(device_url: str, expected: int | None) -> None: | ||
| """Test extracting numeric index from device URL.""" | ||
| device = _create_mock_device(device_url, "place-a") | ||
| entity = _create_mock_entity(device, [device]) | ||
|
|
||
| result = OverkizEntity._get_device_index(entity, device_url) | ||
|
|
||
| assert result == expected |
There was a problem hiding this comment.
Consider adding a test case for when place_oid_grouping is enabled but there are more than two sub-devices with different placeOIDs. This would verify the device hierarchy is correctly constructed when there are multiple zones or sub-devices, ensuring that all sub-devices properly link to the main actuator device.
| def test_has_siblings_with_no_place_oid() -> None: | ||
| """Test device with no placeOID returns False.""" | ||
| devices = [ | ||
| _create_mock_device("io://gateway/123#1", None, "Device 1"), | ||
| _create_mock_device("io://gateway/123#2", "place-b", "Device 2"), | ||
| ] | ||
| entity = _create_mock_entity(devices[0], devices) | ||
|
|
||
| result = OverkizEntity._has_siblings_with_different_place_oid(entity) | ||
|
|
||
| assert result is False | ||
|
|
||
|
|
||
| def test_is_main_device_with_no_place_oid() -> None: | ||
| """Test device with no placeOID is always considered main.""" | ||
| devices = [ | ||
| _create_mock_device("io://gateway/123#2", None, "Device 2"), | ||
| _create_mock_device("io://gateway/123#1", "place-a", "Device 1"), | ||
| ] | ||
| entity = _create_mock_entity(devices[0], devices) | ||
|
|
||
| result = OverkizEntity._is_main_device_for_place_oid(entity) | ||
|
|
||
| assert result is True | ||
|
|
||
|
|
||
| def test_get_via_device_id_main_device_without_place_oid() -> None: | ||
| """Test fallback to gateway when #1 device has no placeOID.""" | ||
| devices = [ | ||
| _create_mock_device("io://gateway/123#1", None, "Actuator"), | ||
| _create_mock_device("io://gateway/123#2", "place-b", "Zone"), | ||
| ] | ||
| entity = _create_mock_entity(devices[1], devices) | ||
| entity.executor = Mock() | ||
| entity.executor.get_gateway_id = Mock(return_value="gateway-id") | ||
|
|
||
| result = OverkizEntity._get_via_device_id(entity, use_place_oid_grouping=True) | ||
|
|
||
| assert result == "gateway-id" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("device_url", "expected"), | ||
| [ | ||
| ("io://gateway/123#4", 4), | ||
| ("io://gateway/123#10", 10), | ||
| ("io://gateway/123#abc", None), | ||
| ("io://gateway/123#", None), | ||
| ], | ||
| ids=["single_digit", "multi_digit", "non_numeric", "empty_suffix"], | ||
| ) | ||
| def test_get_device_index(device_url: str, expected: int | None) -> None: | ||
| """Test extracting numeric index from device URL.""" | ||
| device = _create_mock_device(device_url, "place-a") | ||
| entity = _create_mock_entity(device, [device]) | ||
|
|
||
| result = OverkizEntity._get_device_index(entity, device_url) | ||
|
|
||
| assert result == expected |
There was a problem hiding this comment.
Consider adding a test case for a scenario with three or more devices where some have place_oid and others don't. For example:
- Device Unresolved TODO in /homeassistant/components/httpinterface/__init__.py : 575 #1 with place_oid "place-a"
- Device Added LuciDeviceScanner to scan OpenWrt Luci-RPC enabled router's state. #2 with place_oid None
- Device Added simple process state monitor source. #3 with place_oid "place-b"
This would verify that the device with no place_oid correctly falls back to the old behavior (inheriting from parent) while devices with different place_oids create separate device entries.
| def _create_mock_entity(device: Mock, all_devices: list[Mock]) -> Mock: | ||
| """Create a mock entity with the given device and coordinator data.""" | ||
| entity = Mock(spec=OverkizEntity) | ||
| entity.device = device | ||
| entity.device_url = device.device_url | ||
| entity.base_device_url = device.device_url.split("#")[0] | ||
| entity.coordinator = Mock() | ||
| entity.coordinator.data = {d.device_url: d for d in all_devices} | ||
|
|
||
| prefix = f"{entity.base_device_url}#" | ||
| entity._get_sibling_devices = lambda: [ | ||
| d | ||
| for d in all_devices | ||
| if d.device_url != device.device_url and d.device_url.startswith(prefix) | ||
| ] | ||
| entity._get_device_index = lambda url: ( | ||
| int(url.split("#")[-1]) if url.split("#")[-1].isdigit() else None | ||
| ) | ||
| return entity |
There was a problem hiding this comment.
The mock entity setup is testing private methods by calling them directly with 'self' passed as the first argument. While this works, consider creating full OverkizEntity instances with proper mocking of dependencies to test the public API instead. This would better match Home Assistant testing conventions and ensure the entity initialization and device_info generation work correctly as a whole.
For example, instead of:
OverkizEntity._has_siblings_with_different_place_oid(entity)
Consider:
entity = OverkizEntity(device_url, mock_coordinator)
result = entity._has_siblings_with_different_place_oid()
This approach would provide more robust integration testing and catch issues with the overall flow.
|
Thanks @piitaya, great catch. I did not realize this situation could occur with sub devices.
I will go through your PR in more detail over the next few days! |
|
I'm not sure if it was a different placeOID by default. I set up my heat pump years ago 😅. I also haven't tested a placeOID change. |
|
@iMicknl I just checked, when changing the place in Somfy app, the entities are moved to a new device... 😢 |
@piitaya, what would be your proposal here? I am not sure what is the best way forward. In general these climate devices are quite complex (also visible in the 25+ open issues) and the behavior between these devices differs a lot... |
Proposed change
Some Overkiz devices like Atlantic Pass APC heat pumps have multiple sub-devices (zones, water heater, sensors) that belong to different physical locations. Previously, all sub-devices were grouped under a single device.
This change automatically detects when sub-devices have different placeOIDs and creates separate devices for each location, while maintaining proper device hierarchy linking them to the main actuator.
For example, my Heat Pump device has been split into 5 different devices :
Devices list
Main device
Sub 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: