Added support for enabling boundaries for assetbed cameras as plugin #2689
Conversation
…ding-camera-plugin
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive refactoring of the asset management system, focusing on enhancing flexibility and dynamic handling of asset classes. The changes span multiple files, removing hardcoded asset class references and introducing more dynamic registration and validation mechanisms. Key modifications include updating serializers, models, and integration classes to support a more adaptable approach to asset management, with a particular emphasis on removing ONVIF-specific logic and creating a more generalized asset handling framework. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AssetSerializer
participant AssetClasses
participant Asset
Client->>AssetSerializer: Create Asset
AssetSerializer->>AssetClasses: Validate Asset Class
AssetClasses-->>AssetSerializer: Validate Result
AssetSerializer->>Asset: Save Asset
Asset->>AssetClasses: Verify Asset Class
AssetClasses-->>Asset: Validation Confirmation
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (20)
care/facility/models/json_schema/asset.py (1)
42-68: Potential performance considerations.
Every invocation ofget_dynamic_asset_meta()rebuilds the entire schema, which is fine for moderate usage. However, if this is called frequently, consider caching the results or at least partial results to avoid repeated dictionary assembly.care/facility/models/asset.py (1)
207-213: Method for retrieving valid asset class choices.
get_asset_class_choices()is a straightforward solution, though it might be beneficial to memoize if usage grows.care/utils/models/validators.py (2)
54-75: Flexible design for dynamic schema validation.
DynamicJSONFieldSchemaValidatoris a brilliant way to handle multiple plugin schemas. Minor note: consider adding doc examples for future developers.
76-98: Graceful error handling when generating or applying schemas.
Catching exceptions in__call__and re-raising asValueErrororValidationErroris helpful for debugging. Maybe, for truly critical failures, a bit more logging detail would go a long way.care/facility/migrations/0469_alter_asset_asset_class_alter_asset_meta.py (2)
1-2: Consider removing version/comment lines if unconventionally executed
While Django often includes a signature comment in migration files, if your project does not strictly require it, you may want to remove or shorten it for clarity.
11-11: Check consistency of quote styles
Static analysis suggests using double quotes instead of single quotes. Although not a deal-breaker, it’s best to maintain consistency across the codebase.-('facility', '0468_alter_facilitypatientstatshistory_unique_together_and_more'), +("facility", "0468_alter_facilitypatientstatshistory_unique_together_and_more"),Also applies to: 16-16, 17-17, 21-21, 22-22
🧰 Tools
🪛 Ruff (0.8.2)
11-11: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
11-11: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/utils/assetintegration/ventilator.py (2)
54-56:can_be_linked_to_consultation_bed()
ReturningTruemight allow these assets to directly attach to a consultation bed. Ensure that logical flows (bed assignment, usage info) are updated accordingly to avoid accidental linking.
61-62:get_asset_statusmethod
Fetching a “devices/status” endpoint to retrieve status is straightforward. Be mindful of timeout handling and error scenarios, potentially logging issues for easier troubleshooting.care/utils/assetintegration/hl7monitor.py (2)
54-56:can_be_linked_to_asset_bed()
It’sTruehere. Cross-check whether there's a mechanism preventing the same bed from linking to assets that logically conflict (like a mismatch with a ventilator or multiple HL7 monitors).
61-62:get_asset_statusfor HL7Monitor
Implementation parallels the ventilator’s approach. Perfect for uniformity. Keep an eye on any subtle differences in HL7 endpoints vs. ventilator endpoints.care/utils/assetintegration/utils.py (1)
41-70:MutableEnumbase class
The combination of_registryand dynamic registration is clever. Confirm that large-scale use won’t cause performance bottlenecks if your application grows.care/utils/assetintegration/base.py (3)
64-64: Consider surfacing more context for request timeouts.While raising a
504error is valid, you might provide additional info or even logic to retry once before failing (for large timeouts).
82-86: Specify your action choices or remove the stub.
get_action_choicesis currently not implemented, which might delay usage in integrating classes.
102-104: Clarify the advanced status retrieval.
get_asset_statususage remains unclear. A docstring or a reference to the needed implementation would help.care/facility/tests/test_assetbed_api.py (2)
91-91: Test coverage fortest_list_assetbed.The new expected count of 2 might reflect the resource removal. Kindly ensure the old references for 3 are fully removed in docstrings or comments.
Line range hint
278-291: Successful update flow.Your coverage of updated bed and meta fields is thorough. Possibly consider additional checks for concurrency or race conditions if your environment is multi-tenant.
care/facility/api/serializers/bed.py (1)
124-133: Adaptability for new asset classes.You’re narrowing valid asset classes to HL7MONITOR. That’s one approach. However, you might expand or configure allowed classes if needed.
care/facility/api/serializers/asset.py (1)
241-243: Suggest clarifying the helper method name.
This code snippet dynamically filters asset classes viacan_be_linked_to_asset_bed(). The inline comment hints at needing a better name. Perhaps you could rename it for clearer intent, although that’s obviously your call.care/facility/api/viewsets/asset.py (2)
204-208: Naming mismatch could be improved.
Here,movable_assetsis the collection of asset classes that failis_movable(), making them effectively “non-movable.” It’s a minor naming quirk, but clarifying might save confusion down the line.- movable_assets = [...] + non_movable_assets = [...]Also applies to: 210-210, 212-212
395-403: Double-check local variable naming.
You collect[asset.name for asset in AssetClasses.all()], but naming the loop variable asmemberorasset_classmight be clearer, especially sinceassetis already a significant entity in this context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
care/facility/api/serializers/asset.py(4 hunks)care/facility/api/serializers/bed.py(3 hunks)care/facility/api/serializers/camera_preset.py(0 hunks)care/facility/api/viewsets/asset.py(3 hunks)care/facility/api/viewsets/camera_preset.py(0 hunks)care/facility/migrations/0469_alter_asset_asset_class_alter_asset_meta.py(1 hunks)care/facility/models/asset.py(4 hunks)care/facility/models/bed.py(0 hunks)care/facility/models/json_schema/asset.py(1 hunks)care/facility/tasks/asset_monitor.py(2 hunks)care/facility/tests/test_asset_api.py(1 hunks)care/facility/tests/test_assetbed_api.py(10 hunks)care/utils/assetintegration/asset_classes.py(1 hunks)care/utils/assetintegration/base.py(2 hunks)care/utils/assetintegration/hl7monitor.py(1 hunks)care/utils/assetintegration/onvif.py(0 hunks)care/utils/assetintegration/utils.py(1 hunks)care/utils/assetintegration/ventilator.py(1 hunks)care/utils/models/validators.py(2 hunks)config/api_router.py(1 hunks)plug_config.py(1 hunks)
💤 Files with no reviewable changes (4)
- care/facility/models/bed.py
- care/utils/assetintegration/onvif.py
- care/facility/api/serializers/camera_preset.py
- care/facility/api/viewsets/camera_preset.py
🧰 Additional context used
📓 Learnings (4)
plug_config.py (1)
Learnt from: rithviknishad
PR: ohcnetwork/care#2610
File: plug_config.py:25-25
Timestamp: 2024-11-25T11:39:05.352Z
Learning: In `plug_config.py`, when moving camera-related code from the main backend to a plugin, adding `camera_plugin` to the plugs list is appropriate because the plugin will handle the camera functionality.
care/facility/api/serializers/asset.py (2)
Learnt from: DraKen0009
PR: ohcnetwork/care#2610
File: care/facility/models/asset.py:207-223
Timestamp: 2024-11-22T19:08:58.399Z
Learning: In `care/facility/models/asset.py`, for the `Asset` model, validation of `asset_class` is handled at the serializer level, so additional model-level validation in `save()` or `clean()` methods is not necessary.
Learnt from: DraKen0009
PR: ohcnetwork/care#2610
File: care/facility/models/asset.py:87-90
Timestamp: 2024-11-22T19:10:46.229Z
Learning: In the `Asset` model (`care/facility/models/asset.py`), the `asset_class` field cannot include the `choices` parameter because the choices are dynamic and are handled through the serializers and the `save` method.
care/facility/models/json_schema/asset.py (1)
Learnt from: DraKen0009
PR: ohcnetwork/care#2610
File: care/facility/models/json_schema/asset.py:66-68
Timestamp: 2024-11-22T19:13:40.516Z
Learning: In `care/facility/models/json_schema/asset.py`, when using `AssetMetaRegistry.register_meta`, the project ensures that plugin schema names are unique, so adding a duplicate schema name check in `register_meta` is unnecessary.
care/facility/models/asset.py (1)
Learnt from: DraKen0009
PR: ohcnetwork/care#2610
File: care/facility/models/asset.py:87-90
Timestamp: 2024-11-22T19:10:46.229Z
Learning: In the `Asset` model (`care/facility/models/asset.py`), the `asset_class` field cannot include the `choices` parameter because the choices are dynamic and are handled through the serializers and the `save` method.
🪛 Ruff (0.8.2)
care/facility/migrations/0469_alter_asset_asset_class_alter_asset_meta.py
11-11: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
11-11: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🔇 Additional comments (42)
plug_config.py (2)
18-23: Consider locking plugin to a stable release instead of “@main”.
While pulling from the main branch is definitely convenient, it makes your code susceptible to upstream changes—especially if someone merges something questionable. It might be safer to specify a stable version or tag you can trust.
25-25: Add camera plugin to all relevant references.
This addition looks good. Double-check any references or docs are updated, given that the new plugin is included now.
care/facility/models/json_schema/asset.py (2)
1-9: Sensible use of a central registry for asset schemas.
Good job introducing AssetMetaRegistry to store plugin-specific schemas. Notably, there's no direct check for duplicate schema names within register_meta, which aligns with the retrieved learnings indicating that such checks are already handled elsewhere.
26-41: Dynamic schema assembly is well-structured.
The approach of aggregating base definitions and registry contents in get_dynamic_asset_meta() is a neat way to allow plugins to hook in without major code modifications. The layering of the definitions block is quite thorough.
care/facility/tasks/asset_monitor.py (1)
Line range hint 20-74: Clean removal of ONVIF-specific code.
The shift toward a single result = asset_class.get_asset_status() call is more extensible. A subtle caution: if a plugin fails to load or define get_asset_status(), you might want more defensive error handling or logging.
care/facility/models/asset.py (4)
10-20: Using get_dynamic_asset_meta and DynamicJSONFieldSchemaValidator
This is a tidy replacement for the previous static import of ASSET_META. It elegantly permits future expansions without code duplication.
87-90: asset_class field no longer has strict choices.
Dropping predefined choices is logical, given the new plugin-based approach. Just be mindful that invalid or misspelled classes may go unnoticed during form entry unless validated thoroughly in the serializer or via the save() method.
101-103: Dynamic JSON validation integration.
Great to see the meta field seamlessly utilize DynamicJSONFieldSchemaValidator. This ensures that newly registered schemas are always enforced.
214-223: Runtime check on asset class validity before save.
This extra safeguard is a smart fallback. One tiny concern: if AssetClasses changes frequently, you might see race conditions in ephemeral environments (likely a small risk). Not a blocker, just keep it in mind.
care/utils/assetintegration/asset_classes.py (2)
6-8: Transition to MutableEnum.
Allowing dynamic registrations is a pragmatic choice. This design fosters plugin-based expansions. Nicely done.
10-11: Well-defined registrations for HL7Monitor and Ventilator.
These references keep the code easy to maintain. If more classes arise, the centralized approach should scale effortlessly.
care/facility/migrations/0469_alter_asset_asset_class_alter_asset_meta.py (2)
15-19: Ensure alignment with backend references for asset_class
The new CharField(blank=True, default=None, max_length=20, null=True) looks good. Just confirm that other parts of the code handling asset_class can handle None gracefully, especially if calls assume a string non-null.
🧰 Tools
🪛 Ruff (0.8.2)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
20-24: Validate JSON schema references
Using models.JSONField with a dynamic schema validator is a solid approach. Double-check that care.facility.models.json_schema.asset.get_dynamic_asset_meta is correct and that the validator properly handles blank dictionaries.
🧰 Tools
🪛 Ruff (0.8.2)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/utils/assetintegration/ventilator.py (3)
44-47: get_action_choices method looks fine
Returning a list of (value, name) tuples from the enum is a neat approach, ensuring easy usage in forms or dropdowns. Just confirm any external calls handle these tuples properly.
50-52: Confirm the rationale for is_movable()
The method returns True. Check that other parts of the code or UI do interpret and handle the concept of movable ventilator assets accordingly.
58-60: can_be_linked_to_asset_bed()
Setting this to False is consistent if your domain logic says a ventilator can’t be assigned to a dedicated “asset bed.” Just verify it doesn’t break any generic bed-linking processes.
care/utils/assetintegration/hl7monitor.py (3)
44-47: get_action_choices enumeration
Similar to the ventilator’s implementation, returning these tuples is a nice pattern. Confirm that the enum references remain consistent with the actual HL7 monitor actions.
50-52: is_movable()
Returning False is consistent with the idea that HL7 monitors typically aren’t considered mobile. Just verify the UI or any scheduling logic expecting False.
58-60: can_be_linked_to_consultation_bed()
The approach is reversed from ventilators. This fosters clarity around the specialized usage of different assets. Continue with thorough tests to ensure bed types are all well-defined.
care/utils/assetintegration/utils.py (2)
1-15: MutableEnumMember class
This effectively encapsulates name/value pairs for a dynamic enum. Very readable approach. Might want to confirm no circular references occur if using these objects in logs or debug prints.
17-39: MutableEnumMeta metaclass
Good job capturing the essence of an enum while permitting dynamic extension. Just ensure that dynamically initialized entries are not accidentally re-registered.
care/utils/assetintegration/base.py (3)
57-60: Nicely refined exception handling for status codes.
Separating out 400 Bad Request from other statuses makes the error reporting more accurate.
68-68: Protect the code from unexpected content types gracefully.
Catching JSONDecodeError is good, but you may want to handle other errant content, e.g., HTML error pages, to avoid confusion for the caller.
87-101: Keep them unimplemented or provide a default.
The new static methods properly highlight future extension points. If these are placeholders, ensure your dev folks know to override them.
config/api_router.py (1)
196-196: Camera preset removal recognized.
You've removed the camera preset routes. Presumably, the plugin integration now handles these. Ensure your team updates any references that might still expect these endpoints.
care/facility/tests/test_assetbed_api.py (7)
80-80: Minor docstring update.
Stating “Constructs the url for assetbed api” clarifies usage. Thanks for the thoroughness.
214-217: Duplicate assetbed scenario.
Good job preventing duplicates. This test ensures that your system handles 409-like conflicts properly.
255-255: Asset mismatch test is valuable.
Checking for invalid asset usage aligns with ensuring correct facility associations.
266-266: Confirm bed mismatch logic.
Yet another essential check to ensure bed assignment integrity. Fine job.
307-307: Partial update check.
Extensive checks for bed-asset mismatch continue to keep your data consistent. Approved.
330-330: Prevent bed-asset conflict via PATCH.
Your updated data validation preserves data integrity. Looking good.
343-343: Post-update verification.
Verifying updated fields after a partial update is crucial to avoid hidden regressions.
care/facility/api/serializers/bed.py (2)
323-327: Excluding non-consultation-friendly assets.
Creating a dynamic exclude_asset_classes list is a great step toward flexible usage patterns. Just ensure the integration teams are aware of new classes that require overrides.
343-343: Filtering out excluded asset classes.
The .exclude(asset_class__in=exclude_asset_classes) logic is consistent. A top-notch final check before assignment.
care/facility/api/serializers/asset.py (5)
144-144: New optional field introduced.
This newly added asset_class field is well aligned with your dynamic validation approach. It might be prudent to test scenarios where it is omitted, ensuring robust coverage since it’s declared as optional.
167-173: Good dynamic validation; consider test coverage.
Your check against Asset.get_asset_class_choices() is an excellent step for preventing invalid classes. It might be worthwhile to confirm that upstream usage is thoroughly tested, particularly for boundary edge cases (e.g., empty string).
420-424: Dynamic action choices are well designed.
Your approach uses a list comprehension to gather all action choices from the configured asset classes. Looks good! Keep in mind that if AssetClasses.all() grows large, caching might spare you repeated iterations down the line.
428-428: Flexible structure for additional data.
Defining options = JSONField(required=False) strengthens extensibility. No issues spotted.
431-431: Field registration is consistent.
You’ve included the newly introduced options in the serializer’s fields. This ensures the end-user sees the field as intended.
care/facility/tests/test_asset_api.py (2)
133-133: Nice inclusion of asset_class in creation tests.
Including "asset_class": "VENTILATOR" in your creation payload ensures that new validation logic is effectively tested.
138-147: Great negative test coverage.
The test_create_asset_with_invalid_asset_class helps confirm that your serializer properly rejects unknown classes. You could explore additional corner cases (e.g., lowercase vs. uppercase mismatch), but this is already robust.
care/facility/api/viewsets/asset.py (1)
477-479: Consistent approach for bed linkage.
Again, the snippet uses can_be_linked_to_asset_bed() for filtering. If you decide to rename it in the serializers, do it here for consistency. Otherwise, this is fine.
Proposed Changes
Updated #2557 to migrate it changes to the camera asset plugin.
changes in camera plugin - ohcnetwork/care_camera_asset#5
Note : This PR should be merged only after #2610
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactoring
Chores