-
Notifications
You must be signed in to change notification settings - Fork 94
feat: add shared rooms endpoint #826
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.
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
Changes from 1 commit
d375eb8
c2eb1e8
5c1800c
1a7c4d9
a37c53a
072d0a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import logging | ||
| from dataclasses import dataclass | ||
| from functools import cached_property | ||
|
|
||
| from roborock.data import HomeData, HomeDataRoom, NamedRoomMapping, RoborockBase | ||
| from roborock.devices.traits.v1 import common | ||
|
|
@@ -84,12 +85,22 @@ class RoomsTrait(Rooms, common.V1TraitMixin): | |
| command = RoborockCommand.GET_ROOM_MAPPING | ||
| converter = RoomsConverter() | ||
|
|
||
| def __init__(self, home_data: HomeData, web_api: UserWebApiClient) -> None: | ||
| def __init__(self, home_data: HomeData, device_uid: str, web_api: UserWebApiClient) -> None: | ||
| """Initialize the RoomsTrait.""" | ||
| super().__init__() | ||
| self._home_data = home_data | ||
| self._device_uid = device_uid | ||
| self._web_api = web_api | ||
| self._discovered_iot_ids: set[str] = set() | ||
| self._shared_room_names: dict[str, str] = {} | ||
|
|
||
| @cached_property | ||
| def _is_shared(self) -> bool: | ||
| return any(d.duid == self._device_uid for d in self._home_data.received_devices) | ||
|
|
||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's a pretty big edge case. It's very unlikely that shared devices changes mid run and we don't even really support that right now But defer to your thoughts @allenporter |
||
| @property | ||
| def _room_name_map(self) -> dict[str, str]: | ||
| return {**self._home_data.rooms_name_map, **self._shared_room_names} | ||
|
|
||
| async def refresh(self) -> None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| """Refresh room mappings and backfill unknown room names from the web API.""" | ||
|
|
@@ -104,12 +115,15 @@ async def refresh(self) -> None: | |
|
|
||
| segment_map = RoomsConverter.extract_segment_map(response) | ||
| # Track all iot ids seen before. Refresh the room list when new ids are found. | ||
| new_iot_ids = set(segment_map.values()) - set(self._home_data.rooms_map.keys()) | ||
| new_iot_ids = set(segment_map.values()) - set(self._room_name_map.keys()) | ||
| if new_iot_ids - self._discovered_iot_ids: | ||
| _LOGGER.debug("Refreshing room list to discover new room names") | ||
| if updated_rooms := await self._refresh_rooms(): | ||
| _LOGGER.debug("Updating rooms: %s", list(updated_rooms)) | ||
| self._home_data.rooms = updated_rooms | ||
| if self._is_shared: | ||
| self._shared_room_names = {room.iot_id: room.name for room in updated_rooms} | ||
| else: | ||
| self._home_data.rooms = updated_rooms | ||
| self._discovered_iot_ids.update(new_iot_ids) | ||
| try: | ||
| rooms = self.converter.convert(response) | ||
|
|
@@ -121,12 +135,14 @@ async def refresh(self) -> None: | |
| inner_error=err, | ||
| ) from err | ||
|
|
||
| rooms = rooms.with_room_names(self._home_data.rooms_name_map) | ||
| rooms = rooms.with_room_names(self._room_name_map) | ||
| common.merge_trait_values(self, rooms) | ||
|
|
||
| async def _refresh_rooms(self) -> list[HomeDataRoom]: | ||
| """Fetch the latest rooms from the web API.""" | ||
| try: | ||
| if self._is_shared: | ||
| return await self._web_api.get_shared_device_rooms(self._device_uid) | ||
| return await self._web_api.get_rooms() | ||
| except Exception: | ||
| _LOGGER.debug("Failed to fetch rooms from web API", exc_info=True) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -207,3 +207,34 @@ async def test_refresh_unknown_room_names_failure_falls_back_to_room_segment_id( | |||||||||||||||
| assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=16, iot_id="9999401") | ||||||||||||||||
| assert rooms_trait.rooms[0].name == "Room 16" | ||||||||||||||||
| web_api_client.get_rooms.assert_called_once() | ||||||||||||||||
|
|
||||||||||||||||
|
|
||||||||||||||||
| async def test_refresh_shared_room_names_use_shared_device_rooms_without_mutating_home_data( | ||||||||||||||||
| rooms_trait: RoomsTrait, | ||||||||||||||||
| web_api_client: AsyncMock, | ||||||||||||||||
| mock_rpc_channel: AsyncMock, | ||||||||||||||||
| ) -> None: | ||||||||||||||||
| """Test shared devices resolve room names via the shared-device room list.""" | ||||||||||||||||
| original_rooms = list(rooms_trait._home_data.rooms or ()) | ||||||||||||||||
| try: | ||||||||||||||||
| # Mark the device as shared by adding it to received_devices | ||||||||||||||||
| device = next(d for d in rooms_trait._home_data.devices if d.duid == rooms_trait._device_uid) | ||||||||||||||||
| rooms_trait._home_data.received_devices = [device] | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of mutating internal state of the trait, add a new home data fixture input to (Keep in mind there is other wiring like the product information etc that needs to be matched up) |
||||||||||||||||
| rooms_trait._home_data.devices = [] | ||||||||||||||||
|
|
||||||||||||||||
| web_api_client.get_shared_device_rooms.return_value = [ | ||||||||||||||||
| HomeDataRoom(id=9999999, name="Office"), | ||||||||||||||||
| ] | ||||||||||||||||
| room_mapping_data = [[16, "2362048"], [17, "9999999"]] | ||||||||||||||||
| mock_rpc_channel.send_command.side_effect = [room_mapping_data] | ||||||||||||||||
|
|
||||||||||||||||
| await rooms_trait.refresh() | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. worth asserting on the state before this is run? To verify these rooms weren't already set |
||||||||||||||||
|
|
||||||||||||||||
| assert rooms_trait.rooms | ||||||||||||||||
| assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=16, iot_id="2362048", raw_name="Example room 1") | ||||||||||||||||
| assert rooms_trait.rooms[1] == NamedRoomMapping(segment_id=17, iot_id="9999999", raw_name="Office") | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth asserting that there are two?
Suggested change
|
||||||||||||||||
| assert rooms_trait._home_data.rooms == original_rooms | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't think we need to assert on this |
||||||||||||||||
| web_api_client.get_shared_device_rooms.assert_called_once_with(rooms_trait._device_uid) | ||||||||||||||||
| web_api_client.get_rooms.assert_not_called() | ||||||||||||||||
| finally: | ||||||||||||||||
| rooms_trait._home_data.rooms = original_rooms | ||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need this try/finally for this test? |
||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can check this up front in the constructor and if true, then set
self._shared_device_uidthen directly use that below when checking whether or not to check for shared device rooms