feat: add keyring and ulp-user #299
Conversation
WalkthroughThe changes primarily enhance the functionality of the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…d ulpusers retrieval
…ect dict_from_unifi_list calls
… and improve message processing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
src/uiprotect/data/convert.py (1)
91-91: Specify correct type forreturn_dictThe variable
return_dictis annotated asdict[str, Any], but it stores values of typeProtectModelWithId. Update the type annotation to reflect this for better type clarity and consistency.Apply this diff to correct the type annotation:
- return_dict: dict[str, Any] = {} + return_dict: dict[str, ProtectModelWithId] = {}src/uiprotect/data/bootstrap.py (2)
414-417: Add unit tests for the 'remove' action handlingThe code handling the 'remove' action in
_process_ws_keyring_or_ulp_user_messageis not covered by unit tests, as indicated by the static analysis hints. Consider adding tests to ensure this code path is validated.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 416-416: src/uiprotect/data/bootstrap.py#L416
Added line #L416 was not covered by tests
424-427: Add unit tests for the 'update' action handlingSimilar to the 'remove' action, the 'update' action code path lacks test coverage. Adding unit tests for this path will enhance reliability and catch potential issues early.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 426-426: src/uiprotect/data/bootstrap.py#L426
Added line #L426 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/uiprotect/api.py(3 hunks)src/uiprotect/data/bootstrap.py(5 hunks)src/uiprotect/data/convert.py(4 hunks)tests/data/test_common.py(1 hunks)tests/test_api.py(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/uiprotect/data/bootstrap.py
[warning] 416-416: src/uiprotect/data/bootstrap.py#L416
Added line #L416 was not covered by tests
[warning] 426-426: src/uiprotect/data/bootstrap.py#L426
Added line #L426 was not covered by tests
[warning] 439-439: src/uiprotect/data/bootstrap.py#L439
Added line #L439 was not covered by tests
[warning] 604-604: src/uiprotect/data/bootstrap.py#L604
Added line #L604 was not covered by tests
[warning] 606-606: src/uiprotect/data/bootstrap.py#L606
Added line #L606 was not covered by tests
🔇 Additional comments (1)
tests/data/test_common.py (1)
314-317: Appropriate handling of new keyrings and ulpUsers keys
The removal of the keyrings and ulpUsers keys from obj_dict in the test_bootstrap function ensures the test remains valid after the introduction of these new attributes. This prevents test failures due to unexpected keys.
…nce tests for user removal and updates
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/uiprotect/data/bootstrap.py (1)
389-441: LGTM! Well-structured websocket message handler with proper type safety.The implementation is robust and follows established patterns. Consider enhancing the error logging in the unexpected action case to include the actual action type for better debugging.
- _LOGGER.debug("Unexpected ws action for %s: %s", model_type, action_type) + _LOGGER.debug("Unexpected ws action '%s' for model type %s", action_type, model_type)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/uiprotect/data/bootstrap.py(5 hunks)tests/test_api_ws.py(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/uiprotect/data/bootstrap.py (1)
Learnt from: RaHehl
PR: uilibs/uiprotect#299
File: src/uiprotect/data/bootstrap.py:191-192
Timestamp: 2024-12-07T12:12:11.857Z
Learning: In `src/uiprotect/data/bootstrap.py`, it's acceptable to initialize `keyrings` and `ulp_users` to `None` in the `Bootstrap` class because it's impossible to receive websocket updates for them when they are `None`.
🪛 GitHub Check: codecov/patch
src/uiprotect/data/bootstrap.py
[warning] 605-605: src/uiprotect/data/bootstrap.py#L605
Added line #L605 was not covered by tests
[warning] 607-607: src/uiprotect/data/bootstrap.py#L607
Added line #L607 was not covered by tests
🔇 Additional comments (4)
src/uiprotect/data/bootstrap.py (3)
191-192: LGTM! Attribute declarations are well-typed and properly initialized.
The initialization of keyrings and ulp_users to None is intentional and correct, as it's impossible to receive websocket updates for them when they are None.
600-603: LGTM! Clean integration of keyring and ULP user message handling.
The changes properly integrate with the existing message handling logic and maintain consistent error handling patterns.
605-607: Add test coverage for edge cases.
The following lines lack test coverage:
- Line 605: Processing remove packet
- Line 607: Handling empty data packet
Consider adding test cases to cover these scenarios.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 605-605: src/uiprotect/data/bootstrap.py#L605
Added line #L605 was not covered by tests
[warning] 607-607: src/uiprotect/data/bootstrap.py#L607
Added line #L607 was not covered by tests
tests/test_api_ws.py (1)
741-1298: LGTM! Comprehensive test coverage for keyring and ULP user functionality.
The test suite is well-structured and covers:
- All CRUD operations for both keyrings and ULP users
- Edge cases like non-existent users and unknown actions
- Different types of keyrings (NFC, fingerprint)
- State verification before and after operations
The tests follow established patterns and provide good coverage of the new functionality.
…pdate return type in dict_from_unifi_list
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/uiprotect/data/bootstrap.py (2)
401-406: Simplify type casting logic.The type casting can be simplified since
create_from_unifi_dictalready returns the correct type.- add_obj = create_from_unifi_dict(data, api=self._api, model_type=model_type) - if TYPE_CHECKING: - model_class = MODEL_TO_CLASS.get(model_type) - assert model_class is not None and isinstance(add_obj, model_class) - add_obj = cast(ProtectModelWithId, add_obj) + add_obj = create_from_unifi_dict(data, api=self._api, model_type=model_type)
428-431: Optimize update logic.The update logic creates an unnecessary copy when the object is updated in place.
- old_obj = updated_obj.copy() - updated_data = {to_snake_case(k): v for k, v in data.items()} - updated_obj.update_from_dict(updated_data) + updated_data = {to_snake_case(k): v for k, v in data.items()} + old_obj = updated_obj.copy() + updated_obj.update_from_dict(updated_data)src/uiprotect/data/convert.py (2)
88-95: Add docstring documentation.The function would benefit from documentation explaining its purpose, parameters, return value, and any exceptions it might raise.
def dict_from_unifi_list( api: ProtectApiClient, unifi_list: list[dict[str, ProtectModelWithId]] ) -> dict[str, ProtectModelWithId]: + """Convert a list of UniFi objects to a dictionary keyed by their IDs. + + Args: + api: The ProtectApiClient instance for object creation + unifi_list: List of UniFi object dictionaries to convert + + Returns: + Dictionary mapping object IDs to their corresponding ProtectModelWithId instances + + Raises: + DataDecodeError: If any object in the list has an invalid format + """ return_dict: dict[str, ProtectModelWithId] = {}
91-94: Consider adding error handling for malformed input.The function should gracefully handle cases where the input list contains malformed dictionaries or when casting fails.
return_dict: dict[str, ProtectModelWithId] = {} for obj_dict in unifi_list: - obj = create_from_unifi_dict(obj_dict, api) - return_dict[cast(ProtectModelWithId, obj).id] = obj + try: + obj = create_from_unifi_dict(obj_dict, api) + if not isinstance(obj, ProtectModelWithId): + raise DataDecodeError(f"Object {obj} does not have an ID") + return_dict[obj.id] = obj + except (DataDecodeError, KeyError) as e: + raise DataDecodeError(f"Failed to process object: {e}") from e
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/uiprotect/data/bootstrap.py(5 hunks)src/uiprotect/data/convert.py(4 hunks)
🧰 Additional context used
📓 Learnings (2)
src/uiprotect/data/convert.py (1)
Learnt from: RaHehl
PR: uilibs/uiprotect#299
File: src/uiprotect/data/convert.py:89-89
Timestamp: 2024-12-07T12:11:32.535Z
Learning: In `src/uiprotect/data/convert.py`, the parameter `unifi_list` in `dict_from_unifi_list` should be annotated as `list[dict[str, ProtectModelWithId]]` to maintain precise typing.
src/uiprotect/data/bootstrap.py (1)
Learnt from: RaHehl
PR: uilibs/uiprotect#299
File: src/uiprotect/data/bootstrap.py:191-192
Timestamp: 2024-12-07T12:12:11.857Z
Learning: In `src/uiprotect/data/bootstrap.py`, it's acceptable to initialize `keyrings` and `ulp_users` to `None` in the `Bootstrap` class because it's impossible to receive websocket updates for them when they are `None`.
🔇 Additional comments (4)
src/uiprotect/data/bootstrap.py (2)
191-192: LGTM!
The initialization of keyrings and ulp_users as empty dictionaries is appropriate. As per the learning context, it's safe since websocket updates cannot occur when these are None.
600-603: LGTM!
The addition of keyring and ULP user handling in the message processor is well-integrated with the existing pattern.
src/uiprotect/data/convert.py (2)
5-5: LGTM! Import changes are appropriate.
The new imports for cast, Keyring, and UlpUser are necessary for the added functionality.
Also applies to: 21-21
43-44: LGTM! Model mappings added correctly.
The new mappings for KEYRING and ULP_USER follow the existing pattern and maintain the dictionary's organization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/uiprotect/data/convert.py (1)
88-95: Add docstring to improve code documentationThe function logic is correct and the type annotation is properly set. However, a docstring would help document the purpose and return type of this utility function.
Add a docstring to improve documentation:
def dict_from_unifi_list( api: ProtectApiClient, unifi_list: list[dict[str, ProtectModelWithId]] ) -> dict[str, ProtectModelWithId]: + """Convert a list of UniFi objects to a dictionary keyed by their IDs. + + Args: + api: The ProtectApiClient instance + unifi_list: List of UniFi object dictionaries + + Returns: + Dictionary mapping object IDs to their corresponding ProtectModelWithId instances + """ return_dict: dict[str, ProtectModelWithId] = {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/uiprotect/data/convert.py(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/uiprotect/data/convert.py (1)
Learnt from: RaHehl
PR: uilibs/uiprotect#299
File: src/uiprotect/data/convert.py:89-89
Timestamp: 2024-12-07T12:11:32.535Z
Learning: In `src/uiprotect/data/convert.py`, the parameter `unifi_list` in `dict_from_unifi_list` should be annotated as `list[dict[str, ProtectModelWithId]]` to maintain precise typing.
🔇 Additional comments (2)
src/uiprotect/data/convert.py (2)
5-5: LGTM: Import changes are well-organized
The new imports for cast, Keyring, and UlpUser are properly organized and necessary for the new functionality.
Also applies to: 21-21
43-44: LGTM: Model mappings added correctly
The new mappings for KEYRING and ULP_USER are consistent with the existing pattern.
Let's verify that these model types are properly defined in the ModelType enum:
✅ Verification successful
Let me check the complete enum definition to see all available types:
✅ Model types are properly defined
Both KEYRING and ULP_USER model types are correctly defined in the ModelType enum, confirming that the mappings in convert.py are valid and properly aligned with the available types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ModelType enum contains the new types
rg -A 1 "class ModelType" src/uiprotect/data/types.py
Length of output: 134
Script:
#!/bin/bash
# Get the full ModelType enum definition
rg -A 20 "class ModelType" src/uiprotect/data/types.py
Length of output: 618
Description of change
Pull-Request Checklist
mainbranchFixes #0000Summary by CodeRabbit
Release Notes
New Features
KeyringandUlpUserto better encapsulate user and device data.Bug Fixes
Tests