feat: add processing for NFC scan events #265 #266
Conversation
WalkthroughThe changes in this pull request introduce functionality to handle NFC card scanning events within the existing event processing framework. A new event type, Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
tests/test_api_ws.py (2)
319-337: Consider enhancing test data coverage.While the WebSocket frame construction is correct, consider improving the test data:
- Add test cases for non-empty
ramDescriptionandramClassificationsto ensure proper handling of these fields.- Consider adding test cases for missing or malformed NFC metadata to validate error handling.
359-361: Consider adding edge case tests.While the happy path is well tested, consider adding test cases for:
- Invalid NFC IDs
- Missing user IDs
- Multiple rapid NFC scans
src/uiprotect/data/nvr.py (1)
203-204: Consider adding version validation and documentation.The field looks good, but consider these improvements:
# requires 5.1.34+ - nfc: NfcMetadata | None = None + nfc: NfcMetadata | None = Field( + None, + description="NFC card scan metadata. Available in version 5.1.34+", + validate_version=Version("5.1.34"), + )tests/sample_data/sample_raw_events.json (1)
30001-30008: Consider enhancing test data quality.The metadata structure is well-formed, but consider:
- Adding a test case with missing or malformed NFC data
- Including meaningful
ramDescriptionandramClassificationsto test integration with RAM system"metadata": { "nfc": { "nfcId": "64B2A621", "userId": "672b573764f79603e400031d" }, - "ramDescription": "", - "ramClassifications": [] + "ramDescription": "NFC Card Access Event", + "ramClassifications": ["access_control", "nfc_scan"] },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/uiprotect/data/bootstrap.py(1 hunks)src/uiprotect/data/devices.py(4 hunks)src/uiprotect/data/nvr.py(2 hunks)src/uiprotect/data/types.py(2 hunks)tests/conftest.py(0 hunks)tests/sample_data/sample_constants.json(2 hunks)tests/sample_data/sample_raw_events.json(1 hunks)tests/test_api_ws.py(1 hunks)
🔥 Files not summarized due to errors (1)
- tests/sample_data/sample_raw_events.json: Error: Server error: no LLM provider could handle the message
💤 Files with no reviewable changes (1)
- tests/conftest.py
🔇 Additional comments (14)
tests/sample_data/sample_constants.json (2)
24-24: LGTM! Event count increment aligns with NFC functionality.
The event count increment from 1374 to 1375 is consistent with adding support for the new NFC card scan event type.
10-10: Verify the camera count increment.
The camera count has been increased from 11 to 12. While this is test data, please verify if this increment is intentional and related to the NFC functionality being added.
src/uiprotect/data/types.py (3)
204-204: LGTM! New event type follows conventions.
The addition of NFC_CARD_SCANNED enum value follows the established naming conventions and is placed logically within the event types.
204-204: Consider adding NFC metadata models.
Given the test failures with Pydantic ValidationError mentioned in the PR objectives, it might be necessary to define Pydantic models for NFC metadata (e.g., card ID, timestamp, etc.) to properly validate NFC scan events.
Let's check if NFC metadata models exist elsewhere:
#!/bin/bash
# Description: Check for existing NFC-related models
# Test 1: Look for NFC-related class definitions
echo "Checking for NFC-related classes..."
ast-grep --pattern 'class $nfc_class($_):
$$$' --lang python | rg -i "nfc"
# Test 2: Look for NFC-related Pydantic models
echo "Checking for NFC-related Pydantic models..."
ast-grep --pattern 'class $nfc_model(BaseModel):
$$$' --lang python | rg -i "nfc"278-278: LGTM! Event type correctly added to device events.
The new event type is properly integrated into the device events list, maintaining the caching behavior.
Let's verify that all necessary handlers for this new event type are in place:
✅ Verification successful
Let me try a different approach to verify the handlers for this new event type.
Let me try one more time with corrected command syntax:
Based on the search results, I can now generate the final response:
NFC_CARD_SCANNED event type is properly handled throughout the codebase
The verification shows that the NFC card scanning event is fully integrated:
- Defined in
EventTypeenum intypes.py - Added to device events list as verified in the original review
- Has proper bootstrap handling in
bootstrap.pywith last event tracking - Covered by test cases in
test_api_ws.py - No missing handlers found in event processing logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential missing handlers for NFC_CARD_SCANNED events
# Test 1: Look for event handlers or processors
echo "Checking for NFC event handlers..."
rg -l "nfcCardScanned" --type python
# Test 2: Look for any TODO comments related to NFC
echo "Checking for TODO comments related to NFC handling..."
rg -i "todo.*nfc" --type python
# Test 3: Check for potential switch/match statements that might need updating
echo "Checking for switch/match statements on event types..."
ast-grep --pattern 'match $expr:
case EventType.$_:
$$$' --lang python
Length of output: 637
Script:
#!/bin/bash
# Look for EventType.NFC_CARD_SCANNED usage
echo "Checking for NFC_CARD_SCANNED event type usage..."
rg "NFC_CARD_SCANNED" -A 3 --type python
# Look for event handling patterns
echo "Checking for event handling patterns..."
rg "def.*handle.*event" -A 3 --type python
# Look for match/if statements with EventType
echo "Checking conditional blocks with EventType..."
rg "if.*event.*type.*==" -A 3 --type python
rg "match.*event.*type" -A 5 --type python
Length of output: 599
Script:
#!/bin/bash
# Look for NFC_CARD_SCANNED event type usage
echo "Checking for NFC_CARD_SCANNED event type usage..."
rg "NFC_CARD_SCANNED"
# Look for event handling patterns in Python files
echo "Checking for event handling patterns..."
rg "def.*handle.*event"
# Look for event type conditionals
echo "Checking event type conditionals..."
rg "EventType\." -A 3
Length of output: 14045
tests/test_api_ws.py (1)
296-315: LGTM! Test setup follows established patterns.
The test setup is well-structured and consistent with other event tests in the file. It properly uses async test markers, datetime mocking, and clear variable naming conventions.
src/uiprotect/data/bootstrap.py (1)
101-104: LGTM! The NFC event mapping follows the established pattern.
The addition of NFC_CARD_SCANNED to CAMERA_EVENT_ATTR_MAP correctly maps to the corresponding camera attributes and integrates well with the existing event processing framework.
Let's verify the integration points:
✅ Verification successful
✅ NFC event type integration is properly implemented
All integration points are verified successfully:
NFC_CARD_SCANNEDevent type is defined intypes.pywith value"nfcCardScanned"- Camera class in
devices.pyhas both required attributes and a helper method:last_nfc_card_scanned: datetime | Nonelast_nfc_card_scanned_event_id: str | Nonelast_nfc_card_scanned_event()method
- Test coverage exists in
tests/test_api_ws.pyfor NFC card scan events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the NFC event type integration
# Test 1: Verify NFC_CARD_SCANNED is defined in types.py
echo "Checking EventType definition..."
rg "NFC_CARD_SCANNED.*=.*" src/uiprotect/data/types.py
# Test 2: Verify Camera class has the new attributes
echo "Checking Camera class attributes..."
rg "last_nfc_card_scanned(_event_id)?" src/uiprotect/data/devices.py
# Test 3: Verify test coverage for NFC events
echo "Checking test coverage..."
rg "test.*nfc.*card.*scan" tests/
Length of output: 889
src/uiprotect/data/devices.py (4)
1001-1002: LGTM! New properties for NFC card scanning.
The new properties follow the established pattern for event tracking and are properly typed.
1023-1024: LGTM! Properly excluded NFC card scan fields from change tracking.
The new fields are correctly added to the excluded fields set, consistent with how other event-related fields are handled.
1157-1164: LGTM! Well-implemented event retrieval method.
The new property method follows the established pattern for event retrieval and properly handles all cases.
1094-1095: LGTM! Proper dictionary conversion handling for NFC fields.
The new fields are correctly added to the dictionary conversion logic, maintaining consistency with other event-related fields.
tests/sample_data/sample_raw_events.json (3)
29990-29992: LGTM! Event type naming follows convention.
The new event type "nfcCardScanned" follows camelCase convention consistent with other event types.
29995-29997: LGTM! Empty arrays for smart detect fields.
Correctly initialized empty arrays for smartDetectTypes and smartDetectEvents as NFC events don't involve smart detection.
29993-29994: Verify timestamp handling in the codebase.
The identical start and end timestamps (1731245487257) correctly represent an instantaneous event, but the timestamp appears to be set in the future (around 2024-11-10). While this might be intentional for testing, ensure the codebase properly handles future timestamps.
✅ Verification successful
Let me check the timestamp validation logic in the relevant files.
Let me check how these timestamps are used in the test data context.
Based on the code analysis, I can now provide a final response:
Future timestamps in test data are properly handled by the codebase
The codebase has robust timestamp handling through utility functions in src/uiprotect/utils.py:
from_js_time()properly converts JavaScript timestamps to Python datetime objectsto_js_time()handles conversion back to JavaScript timestamps- Type hints and null checks are in place for timestamp fields in the data models
The future timestamp in the test data is safe as:
- The conversion functions are timezone-aware (using UTC)
- No validation restrictions are placed on timestamp ranges, allowing for testing with future dates
- The data models properly type timestamps as
datetimeobjects
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for timestamp validation in the codebase
# Expected: Find timestamp validation logic in event processing
# Search for timestamp validation patterns
rg -l 'timestamp|start.*end' --type py
# Look for datetime validation in models
ast-grep --pattern 'class $_Event {
$$$
timestamp: $_
$$$
}'
Length of output: 320
Script:
#!/bin/bash
# Check timestamp handling in event-related files
rg -A 5 "timestamp|start.*end" src/uiprotect/data/nvr.py src/uiprotect/utils.py
# Look for any timestamp validation or normalization
ast-grep --pattern 'def $_($$$) {
$$$
timestamp$_
$$$
}'
Length of output: 2662
Script:
#!/bin/bash
# Check test data and event handling
rg -A 5 "sample_raw_events.json" tests/
rg -A 10 "1731245487257" tests/
Length of output: 71
tests/test_api_ws.py
Outdated
| assert camera.dict_with_excludes() == camera_before.dict_with_excludes() | ||
| assert event.id == expected_event_id | ||
| assert event.type == EventType.NFC_CARD_SCANNED | ||
| assert event.metadata.nfc.get("nfcId") == expected_nfc_id | ||
| assert event.metadata.nfc.get("userId") == expected_user_id | ||
| assert event.thumbnail_id == f"e-{expected_event_id}" | ||
| assert event.heatmap_id == f"e-{expected_event_id}" | ||
| assert event.start == (now - timedelta(seconds=30)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent assertion methods across tests.
The test uses dict_with_excludes() while other similar tests use dict(). Consider:
- Using the same assertion method across all event tests for consistency
- Adding validation for the camera's
last_nfc_card_scannedproperty - Adding assertions for the event's end time, similar to the smart detect test
Codecov ReportAttention: Patch coverage is
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/uiprotect/data/nvr.py(3 hunks)
🔇 Additional comments (2)
src/uiprotect/data/nvr.py (2)
204-205: LGTM!
The addition of the optional nfc field to EventMetadata follows the established patterns in the codebase and is properly documented with version requirements.
Line range hint 140-205: Verify NFC functionality implementation completeness.
Since this PR adds NFC functionality, please verify:
- Are there any other NFC-related changes needed in the codebase?
- Does this require updates to the documentation?
Run the following script to check for potential missing changes:
✅ Verification successful
Based on the results, I can now generate the final response since I have sufficient information about the NFC implementation across the codebase.
NFC functionality implementation appears complete
The codebase shows comprehensive NFC support:
- Event type and metadata models are properly defined
- Camera device properties track NFC card scanning
- WebSocket events handle NFC card scanning
- Test coverage exists for NFC functionality
No additional implementation changes are needed. The only suggested action would be to document the new NFC functionality in the changelog.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for NFC-related code and documentation that might need updates
# Check for NFC-related code
echo "Checking for NFC-related code..."
rg -i "nfc|card.*scan" --type py
# Check for documentation files
echo "Checking for documentation files that might need updates..."
fd -e md -e rst
Length of output: 2435
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/test_api_ws.py (1)
296-361: Consider refactoring event tests to reduce duplicationThe test follows the same pattern as other event tests (ring, motion, smart). Consider extracting common test logic into a shared helper function or fixture to reduce code duplication.
Example refactor:
@pytest.fixture def event_test_data(): def _make_data(event_type, metadata=None): return { "expected_updated_id": "0441ecc6-f0fa-4b03-b071-7987c143138a", "expected_event_id": "6730b5af01029603e4003bdb", "event_type": event_type, "metadata": metadata or {}, } return _make_data async def _test_ws_event( protect_client, now, camera, packet, event_data, event_assertions, ): # Common test setup and execution logic ... @pytest.mark.asyncio @patch("uiprotect.api.datetime", MockDatetime) async def test_ws_event_nfc_card_scanned( protect_client_no_debug, now, camera, packet, event_test_data, ): nfc_metadata = { "nfc": { "nfcId": "66B2A649", "userId": "672b570000f79603e400049d" } } event_data = event_test_data("nfcCardScanned", nfc_metadata) await _test_ws_event( protect_client_no_debug, now, camera, packet, event_data, lambda event: assert_nfc_event(event), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/uiprotect/data/nvr.py(2 hunks)tests/test_api_ws.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/uiprotect/data/nvr.py
🔇 Additional comments (1)
tests/test_api_ws.py (1)
350-350: Use consistent assertion method across tests
The test uses dict_with_excludes() while other event tests use dict(). Consider using the same assertion method for consistency.
|
@bdraco From my perspective, everything looks good now, and all the tests are passing. Could you please take a look when you have a moment? |
|
replaced by #275 |
Description of change
Fixes #263
Pull-Request Checklist
mainbranchFixes #0000Summary by CodeRabbit
Release Notes
New Features
NfcMetadataclass added to encapsulate NFC-related information.Bug Fixes
Tests
Documentation