Chime ringtones #346
Chime ringtones #346
Conversation
WalkthroughThe pull request introduces support for ringtones in the UI Protect system. A new Changes
Sequence DiagramsequenceDiagram
participant User
participant ProtectApiClient
participant ChimeDevice
User->>ProtectApiClient: play_speaker(device_id, ringtone_id, volume)
ProtectApiClient->>ChimeDevice: Send play request
ChimeDevice-->>ProtectApiClient: Confirm playback
ProtectApiClient-->>User: Playback completed
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 1
🧹 Nitpick comments (4)
src/uiprotect/api.py (1)
1809-1810: Include docstring updates for newly introduced parameters.
Theplay_speakermethod signature now includesringtone_idandtrack_no, but the method docstring doesn’t mention these new parameters. Document them for clarity and completeness.src/uiprotect/data/devices.py (1)
3392-3398: Implement unit tests for the newRingtoneclass.
Great addition of theRingtonemodel. Ensure you add tests for data serialization/deserialization, especially ifsizeormodel_keyare used for downstream logic or filtering.tests/sample_data/sample_bootstrap.json (2)
7279-7296: Review ringtone configuration structure.The ringtones section is well-structured with essential fields (id, name, size, isDefault, nvrMac). However, consider enhancing it with additional metadata.
Consider adding:
- Audio format metadata (e.g., format, bitrate, duration)
- Size limits validation
- Compatibility flags for different chime models
{ "id": "66a14fa502d44203e40003eb", "name": "Default", "size": 208, "isDefault": true, "nvrMac": "4B8290F6D7A3", - "modelKey": "ringtone" + "modelKey": "ringtone", + "format": "mp3", + "bitrate": 128, + "duration": 2, + "compatibleModels": ["UP Chime"] }
7289-7291: Fix typo in custom ringtone name.The custom ringtone name "custometest" appears to have a typo.
Apply this diff to fix the typo:
- "name": "custometest", + "name": "customtest",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/uiprotect/api.py(1 hunks)src/uiprotect/data/bootstrap.py(2 hunks)src/uiprotect/data/convert.py(2 hunks)src/uiprotect/data/devices.py(1 hunks)src/uiprotect/data/types.py(1 hunks)tests/sample_data/sample_bootstrap.json(1 hunks)tests/test_api.py(1 hunks)
🔇 Additional comments (11)
src/uiprotect/data/convert.py (2)
17-17: Great addition of Ringtone import.
No issues found. The import statement is clean and follows the existing pattern.
48-48: Mapping Ringtone to ModelType.RINGTONE looks correct.
This inclusion will enable JSON parsing for the new ringtone model.src/uiprotect/data/types.py (1)
131-131: Enum entry for RINGTONE.
Adding the new enumeration value is consistent. Ensure that external consumers or switch-case logic correctly handle this expanded set of model types.src/uiprotect/data/bootstrap.py (2)
33-33: New import for Ringtone recognized.
This is consistent with the companion changes inconvert.pyanddevices.py.
187-187: New ringtones list in Bootstrap.
Defining a dedicated list for ringtones is a straightforward way to integrate the new model. Consider verifying that your bootstrap loading code properly populates this list in practice.tests/test_api.py (5)
972-988: Test:test_play_speaker
This test checks the default parameters scenario for the newplay_speakermethod. It appears thorough and correctly invokes the API with no JSON payload. Nicely done.
989-1008: Test:test_play_speaker_with_volume
Good job verifying that a specified volume is passed in the JSON payload. This ensures volume override is handled.
1009-1029: Test:test_play_speaker_with_ringtone_id
Great addition checking that theringtoneIdparameter is included in the request payload.
1030-1039: Test:test_play_speaker_invalid_chime_id
Verifies exception handling for invalid chime IDs. Good negative test coverage.
1040-1067: Test:test_play_speaker_with_all_parameters
Excellent coverage by specifying all parameters: volume, repeat times, ringtone ID, and track number. This ensures the method can handle multiple simultaneous parameters.src/uiprotect/api.py (1)
1814-1814: 🛠️ Refactor suggestionConsider handling fallback logic for track number.
Currently, ifringtone_idis provided, the code removes"trackNo". This is fine if the system intends for these to be mutually exclusive. However, if the user might specify both (ringtone_idfor a certain ringtone but also a specific track?), add a guard or a clarifying comment to confirm that only one of the two is truly valid at a time.Also applies to: 1822-1826
Description of change
Resolves #342
Pull-Request Checklist
mainbranchFixes #0000Summary by CodeRabbit
New Features
Bug Fixes
Tests