feat: add feature flags for nfc and fingerprint #276
Conversation
WalkthroughThis pull request introduces several enhancements to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
tests/conftest.py (1)
747-749: Align indentation of new feature flags with existing entriesThe indentation of
"supportNfc"and"hasFingerprintSensor"is inconsistent with the other entries inNEW_CAMERA_FEATURE_FLAGS. For consistency and readability, please adjust the indentation.Apply this diff to fix the indentation:
# 4.73.71+ - "supportNfc", - "hasFingerprintSensor", + "supportNfc", + "hasFingerprintSensor",tests/sample_data/sample_camera.json (2)
471-471: Maintain consistent naming convention for feature flagsWhile the addition of NFC and fingerprint sensor flags is appropriate, the naming convention should be consistent with other hardware capability flags in the configuration.
Consider updating the NFC flag to follow the "has" prefix convention:
"hasFingerprintSensor": true, "mountPositions": [], "smartDetectTypes": [ "person", "vehicle" ], - "supportNfc": true, + "hasNfc": true, "motionAlgorithms": [Also applies to: 477-477
471-477: Consider grouping related feature flagsFor better maintainability and readability, consider grouping related hardware capability flags together (e.g., authentication-related features like NFC, fingerprint sensor, and biometrics).
Consider moving these flags next to other hardware-related flags like
hasBluetoothorhasLcdScreento maintain a logical grouping.src/uiprotect/data/devices.py (2)
862-864: LGTM! Consider adding docstrings for the new feature flags.The new feature flags follow the established naming conventions and type patterns. Consider adding docstrings to describe their purpose and usage.
# 4.73.71+ - support_nfc: bool | None = None - has_fingerprint_sensor: bool | None = None + # Whether the camera supports NFC card scanning capabilities + support_nfc: bool | None = None + # Whether the camera has a fingerprint sensor for biometric authentication + has_fingerprint_sensor: bool | None = None
862-864: Consider adding helper properties for the new feature flags.For consistency with other feature flags in the codebase, consider adding helper properties to the Camera class to check these capabilities.
Add these properties to the Camera class:
@property def is_nfc_supported(self) -> bool: """Whether the camera supports NFC card scanning.""" return self.feature_flags.support_nfc is True @property def has_fingerprint(self) -> bool: """Whether the camera has a fingerprint sensor.""" return self.feature_flags.has_fingerprint_sensor is Truetests/sample_data/sample_bootstrap.json (1)
Line range hint
1-2024: Consider adding documentation for new feature flags.The new feature flags have been added but there's no documentation explaining their purpose or expected behavior. Consider adding comments or documentation to describe:
- When these features are enabled/disabled
- How they interact with other settings
- Any dependencies or requirements
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/uiprotect/data/devices.py(1 hunks)tests/conftest.py(1 hunks)tests/sample_data/sample_bootstrap.json(1 hunks)tests/sample_data/sample_camera.json(1 hunks)tests/test_api_ws.py(2 hunks)
🔇 Additional comments (4)
tests/test_api_ws.py (2)
342-342: LGTM! Good feature validation.
The assertion correctly validates that the camera has NFC support before testing NFC card scanning events.
416-416: LGTM! Good feature validation.
The assertion correctly validates that the camera has fingerprint sensor support before testing fingerprint identification events.
tests/sample_data/sample_bootstrap.json (2)
2003-2003: LGTM: Feature flags properly added to G4 Doorbell Pro device.
The new feature flags hasFingerprintSensor and supportNfc are correctly added to the G4 Doorbell Pro device configuration, which is the appropriate device type for these capabilities.
Also applies to: 2009-2009
2003-2009: Verify feature flags are enabled only for supported devices.
The feature flags are currently only added to the G4 Doorbell Pro device. Let's verify this is intentional and no other device types should have these capabilities.
|
Thanks @RaHehl |
|
6.6.0 cooking |
|
Thanks @bdraco, created a pr in HA home-assistant/core#130872 |
Description of change
add missing featureflags for nfc and fingerprint
Pull-Request Checklist
mainbranchFixes #0000Summary by CodeRabbit
New Features
Bug Fixes
Documentation