Removing everything related to camera in favour of camera plugin #2610
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve significant updates to various serializers, viewsets, and models related to asset management within the application. Key modifications include the addition of an Changes
Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (22)
care/utils/assetintegration/asset_classes.py (1)
6-7: Add documentation to explain the empty class pattern.While I'm sure you had your reasons, this empty class could use a docstring explaining the dynamic registration pattern. It would help future maintainers understand why we're using this... interesting approach.
class AssetClasses(MutableEnum): + """ + Dynamic registry for asset classes. + Asset types are registered at runtime using MutableEnum.register() + instead of being defined as static enum members. + """ passplug_config.py (1)
18-25: Consider a phased transition approachIf there's a need to maintain backward compatibility while transitioning away from camera functionality, perhaps we should:
- Document the deprecation timeline
- Add migration guides for existing users
- Consider feature flags to gradually phase out the camera functionality
Would you like help creating a deprecation plan or implementing feature flags?
care/utils/assetintegration/ventilator.py (2)
43-47: I suppose we could make this more... efficientWhile the implementation works, we could skip the intermediate list creation and directly return the comprehension. Not that it matters much, but you know, every microsecond counts. 😊
@classmethod def get_action_choices(cls): - choices = [] - choices += [(e.value, e.name) for e in cls.VentilatorActions] - return choices + return [(e.value, e.name) for e in cls.VentilatorActions]
49-59: Would it kill you to add some docstrings?These static methods could really use some documentation explaining the reasoning behind these hardcoded values. Future maintainers might appreciate knowing why ventilators can be linked to consultation beds but not to asset beds. Just saying... 🤷
@staticmethod def is_movable(): + """ + Indicates whether ventilator assets can be physically relocated. + Returns True as ventilators are portable medical devices. + """ return True @staticmethod def can_be_linked_to_consultation_bed(): + """ + Indicates whether ventilator can be associated with consultation beds. + Returns True as ventilators are commonly used during consultations. + """ return True @staticmethod def can_be_linked_to_asset_bed(): + """ + Indicates whether ventilator can be associated with asset beds. + Returns False as ventilators have their own asset management workflow. + """ return Falsecare/utils/assetintegration/hl7monitor.py (1)
Line range hint
13-19: Would it kill you to add some documentation about required metadata?While the error handling is... adequate, it would be so nice if we could document what metadata keys are actually required. You know, for those of us who might want to use this class someday.
Consider adding a docstring like:
def __init__(self, meta): """Initialize HL7MonitorAsset with required metadata. Args: meta (dict): Required metadata including: - host: The device host address - id: The asset identifier [add other required keys...] Raises: ValidationError: If any required metadata keys are missing """care/utils/assetintegration/base.py (2)
85-99: Oh look, someone's discovered static methods!The implementation is mostly fine, but there are a few things that could make it even better:
- Those parentheses around NotImplementedError at line 98 are just showing off.
- These methods could really use some return type hints to help implementers.
@staticmethod - def can_be_linked_to_consultation_bed(): + def can_be_linked_to_consultation_bed() -> bool: error = "'can_be_linked_to_consultation_bed()' method is not implemented" raise NotImplementedError(error) @staticmethod - def can_be_linked_to_asset_bed(): + def can_be_linked_to_asset_bed() -> bool: error = "'can_be_linked_to_asset_bed()' method is not implemented" raise NotImplementedError(error) @staticmethod - def is_movable(): + def is_movable() -> bool: error = "'is_movable()' method is not implemented" - raise (NotImplementedError(error)) + raise NotImplementedError(error)
Camera-related code still exists and needs attention
It seems the camera functionality hasn't been completely removed as intended. There are several remaining references that should probably be addressed:
- Active camera-related code in:
plug_config.py: Still references camera plugin and its GitHub repositorycare/facility/models/camera_preset.py: Contains complete camera preset modelcare/utils/assetintegration/schema.py: Contains camera-specific schema validationscare/facility/api/serializers/asset.py: Has camera access key handlingcare/facility/api/viewsets/patient_consultation.py: Contains preset name logicWhile the base integration class has been cleaned up, these lingering camera-specific implementations suggest the migration to a plugin system isn't quite complete yet. Perhaps we should consider moving these to the camera plugin repository?
🔗 Analysis chain
Line range hint
1-102: Verify complete removal of camera-related codeThe changes align well with moving towards a dynamic plugin system. However, let's verify that all camera-related code has been properly removed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining camera-related code echo "Searching for camera-related code..." rg -i "camera|onvif|preset|ptz" --type py echo "Searching for camera-related imports..." ast-grep --pattern 'import $_ from "camera"' ast-grep --pattern 'from camera import $_'Length of output: 9863
care/facility/tasks/asset_monitor.py (3)
Line range hint
20-35: Consider filtering out inactive assets... if you want to be efficient.The query could benefit from excluding inactive or deleted assets (if such flags exist) to avoid unnecessary processing. Just a thought. 🤷
assets = ( Asset.objects.exclude(Q(asset_class=None) | Q(asset_class="")) + .filter(is_active=True) # assuming this field exists .select_related( "current_location", "current_location__facility", )
Line range hint
82-119: This nested logic is... interesting.The status processing logic could use some refactoring for clarity. Also, those dictionary accesses without
.get()are living dangerously, don't you think?Consider extracting the status determination logic:
+ def determine_asset_status(status_record, local_ip_address): + if not status_record.get("status"): + return AvailabilityStatus.DOWN + + asset_status = status_record.get("status", {}).get(local_ip_address, "down") + + status_map = { + "up": AvailabilityStatus.OPERATIONAL, + "maintenance": AvailabilityStatus.UNDER_MAINTENANCE, + "down": AvailabilityStatus.DOWN + } + return status_map.get(asset_status, AvailabilityStatus.DOWN) # In the main loop: - if asset.meta.get("local_ip_address") in status_record.get("status", {}): - asset_status = status_record["status"][asset.meta.get("local_ip_address")] - else: - asset_status = "down" - - if asset_status == "up": - new_status = AvailabilityStatus.OPERATIONAL - elif asset_status == "maintenance": - new_status = AvailabilityStatus.UNDER_MAINTENANCE + new_status = determine_asset_status( + status_record, + asset.meta.get("local_ip_address") + )
Camera-related code still exists and needs attention
It seems we've found quite a few camera-related components that are still lingering around, perhaps waiting for someone to notice them. Key areas requiring cleanup:
plug_config.py: Contains camera plugin configuration and GitHub repository referenceconfig/settings/base.py: Has a commented-out camera plugincare/facility/models/camera_preset.py: Complete file dedicated to camera presetscare/utils/assetintegration/schema.py: Contains ONVIF and camera access key schema definitionscare/facility/api/serializers/asset.py: Includes camera access key handling logiccare/facility/tasks/plausible_stats.py: Contains camera-related analytics eventsWhile some occurrences in migration files can be safely ignored, the active code files still contain camera functionality that should be removed as part of this cleanup.
🔗 Analysis chain
Line range hint
20-119: Verify complete removal of camera-related code.The ONVIF-specific handling has been removed, but let's ensure no other camera-related code remains.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining camera-related code rg -i "camera|onvif" --type pyLength of output: 9069
config/api_router.py (1)
Several camera-related files and code still need attention
I see you've been selectively thorough with the camera removal. There are still quite a few camera-related files and code snippets that need addressing:
care/facility/models/camera_preset.py- The entire camera preset model filecare/facility/migrations/0466_camera_presets.py- Migration for camera presetsplug_config.py- Contains camera plugin configurationcare/facility/tasks/plausible_stats.py- Has camera-related analytics eventscare/utils/assetintegration/schema.py- Contains camera access key schema definitionscare/facility/api/serializers/asset.py- Has camera access key handling logiccare/facility/api/viewsets/patient_consultation.py- Contains preset name filteringWould be lovely if we could clean these up to maintain consistency with the stated goal of "removing everything related to camera for plugin". 😊
🔗 Analysis chain
Line range hint
224-229: New routes look good, but let's verify camera removal is completeThe addition of availability and service record routes aligns well with the dynamic approach. However, I notice you've been quite thorough in removing the camera preset routes... almost too thorough. 😏
Let's make absolutely sure we haven't missed any camera-related code:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining camera-related code # Search for any remaining camera-related files echo "Checking for camera-related files..." fd -t f -e py camera # Search for any remaining camera-related code echo "Checking for camera-related code..." rg -i "camera|preset" --type py # Search for any orphaned camera-related migrations echo "Checking for camera-related migrations..." fd -t f -e py . "migrations" | xargs rg -i "camera|preset"Length of output: 18410
care/utils/models/validators.py (1)
54-60: The docstring could be a tiny bit more helpful, don't you think?While the docstring explains what the class does, it would be even better if it included:
- Example usage scenarios
- Sample code snippet
- Explanation of when to prefer this over the base class
""" A dynamic JSONField schema validator that generates the schema at runtime. Inherits from JSONFieldSchemaValidator for reusability. + + Example usage: + ```python + def generate_schema(): + return {"type": "object", "properties": {"dynamic_field": {"type": "string"}}} + + field = models.JSONField(validators=[DynamicJSONFieldSchemaValidator(generate_schema)]) + ``` + + Use this validator when the schema needs to be determined at validation time rather + than at class definition time. """care/facility/tests/test_assetbed_api.py (3)
80-80: Maybe consider capitalizing API for consistency?The docstring uses "api" while the rest of the codebase typically uses "API".
- Constructs the url for assetbed api + Constructs the URL for AssetBed API
91-91: Would be nice to add a comment explaining the assertion countThe test expects exactly 2 assetbeds, but it's not immediately clear which ones are being counted. A brief comment would help future maintainers understand the test's expectations.
- self.assertEqual(response.data["count"], 2) + # Expects 2 assetbeds: bed1-asset1 and bed3-asset3 + self.assertEqual(response.data["count"], 2)Also applies to: 214-217
Line range hint
255-291: Perhaps we could reduce some duplication in the test cases?The update and patch test cases share quite a bit of similar setup and validation logic. Consider extracting common test setup into helper methods.
For example:
def _assert_invalid_asset_update(self, method, data): """Helper method for testing invalid asset updates""" response = method( self.get_url(external_id=self.assetbed.external_id), data, format="json" ) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) def _verify_assetbed_state(self, bed, asset, meta=None): """Helper method to verify assetbed state""" self.assertEqual(self.assetbed.bed.external_id, bed.external_id) self.assertEqual(self.assetbed.asset.external_id, asset.external_id) if meta is not None: self.assertEqual(self.assetbed.meta, meta)This would help reduce code duplication between
test_update_assetbedandtest_patch_assetbed.Also applies to: 307-343
care/facility/tests/test_asset_api.py (1)
Line range hint
1-450: Consider adding migration test coverageSince we're removing camera functionality, it would be really nice if we had tests to verify that existing camera assets are handled gracefully during migration. You know, just to be thorough... 😊
Consider adding a test case like:
def test_migration_of_existing_camera_assets(self): # Create an asset with old camera configuration old_asset = self.create_asset( self.asset_location, meta={"legacy_camera_config": "some_value"} ) # Verify the asset is migrated correctly response = self.client.get(f"/api/v1/asset/{old_asset.external_id}/") self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertNotIn("legacy_camera_config", response.data["meta"])care/facility/api/viewsets/asset.py (2)
204-210: Consider caching the non-movable assets listThe list comprehension is executed on every filter call, which might not be the most efficient approach. Perhaps we could cache this list since it only changes when asset classes change?
+ # At module level + IMMOVABLE_ASSETS = [member.name for member in AssetClasses.all() if not member.is_movable] def filter_is_permanent(self, queryset, _, value): if value not in EMPTY_VALUES: - movable_assets = [ - member.name for member in AssetClasses.all() if not member.is_movable - ] if value: - queryset = queryset.filter(asset_class__in=movable_assets) + queryset = queryset.filter(asset_class__in=IMMOVABLE_ASSETS) else: - queryset = queryset.exclude(asset_class__in=movable_assets) + queryset = queryset.exclude(asset_class__in=IMMOVABLE_ASSETS)
475-477: Improve method naming as suggested by the commentThe comment suggests that the method name needs improvement. Perhaps we could help with that?
Would you like me to suggest some alternative names for
can_be_linked_to_asset_bed()? Here are a few options:
is_bed_linkable()supports_bed_association()is_bed_compatible()Let me know if you'd like me to create an issue to track this naming improvement.
care/utils/assetintegration/utils.py (2)
25-27: Ensure consistent error handling in__getitem__methodThe
__getitem__method currently raises aKeyErrorif the name is not found in_registry. For consistency with__getattr__, consider raising anAttributeErrorwith a descriptive message.Apply this diff to improve error handling:
def __getitem__(cls, name): - return cls._registry[name] + try: + return cls._registry[name] + except KeyError: + error = f"{name} not found in {cls.__name__}" + raise AttributeError(error)
49-56: Consider thread safety when modifying_registryIf
MutableEnumis used in a multithreaded context, concurrent modifications to_registrycould lead to race conditions. It might be wise to add thread-safe mechanisms, such as threading locks, to ensure safe member registration.Here's how you might implement it:
import threading class MutableEnum(metaclass=MutableEnumMeta): def __init_subclass__(cls, **kwargs): super().__init_subclass__(**kwargs) cls._registry = {} + cls._lock = threading.Lock() @classmethod def register(cls, name, value): + with cls._lock: if name in cls._registry: error = f"{name} is already registered." raise ValueError(error) cls._registry[name] = MutableEnumMember(name, value)care/facility/models/json_schema/asset.py (1)
1-23: Thread safety considerations forAssetMetaRegistryNot that it's a big concern, but using a mutable class-level attribute like
_registrymight cause issues in multi-threaded environments. You might want to ensure thread safety if concurrent access is expected.care/facility/api/serializers/asset.py (1)
236-238: Consider renamingcan_be_linked_to_asset_bedfor clarity.The inline comment suggests the current naming might not be ideal. Renaming it to something more descriptive could improve code readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (23)
care/facility/api/serializers/asset.py(3 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/0468_alter_asset_asset_class_alter_asset_meta.py(1 hunks)care/facility/models/__init__.py(0 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(1 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)config/settings/base.py(1 hunks)plug_config.py(1 hunks)
💤 Files with no reviewable changes (5)
- care/facility/api/serializers/camera_preset.py
- care/facility/api/viewsets/camera_preset.py
- care/facility/models/init.py
- care/facility/models/bed.py
- care/utils/assetintegration/onvif.py
✅ Files skipped from review due to trivial changes (1)
- config/settings/base.py
🧰 Additional context used
🪛 Ruff (0.7.0)
care/facility/migrations/0468_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 (16)
care/utils/assetintegration/asset_classes.py (1)
10-11: LGTM! Dynamic registration implemented correctly.
The dynamic registration approach aligns well with the PR objectives, making the system more flexible for future asset types.
care/facility/migrations/0468_alter_asset_asset_class_alter_asset_meta.py (3)
3-12: Dependencies and imports look fine... I suppose.
The migration chain and necessary imports are properly structured.
🧰 Tools
🪛 Ruff (0.7.0)
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)
15-19: Verify the impact on existing asset_class values
Making the asset_class field nullable could affect existing records. It would be so nice if we could verify there are no non-null values that might be affected.
✅ Verification successful
Making asset_class nullable is safe and aligned with code changes
The field modification is part of a larger effort to remove camera-related components, and the codebase analysis shows this change is well-handled:
- The migration is preceded by
0466_camera_presets.pywhich already handles cleanup of asset_class data - The field is already validated in
Asset.save()method against dynamic choices fromAssetClasses - The
asset_classfield is optional in serializers and properly handled in views with appropriate filters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing asset_class values that might be affected
echo "Checking for existing asset_class values..."
ast-grep --pattern 'class Asset($$$):
$$$
asset_class = $$$
$$$'
# Look for any references to asset_class in views/serializers
echo "Checking for asset_class references in views and serializers..."
rg "asset_class" --type py
Length of output: 20414
🧰 Tools
🪛 Ruff (0.7.0)
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:
Using dict() as default might not be the best idea...
Using a mutable default value (dict) in Django models can lead to unexpected behavior. While it might work in migrations, it would be slightly better to use a callable instead.
Consider this alternative:
- field=models.JSONField(blank=True, default=dict, validators=[care.utils.models.validators.DynamicJSONFieldSchemaValidator(care.facility.models.json_schema.asset.get_dynamic_asset_meta)]),
+ field=models.JSONField(blank=True, default=dict, validators=[care.utils.models.validators.DynamicJSONFieldSchemaValidator(care.facility.models.json_schema.asset.get_dynamic_asset_meta)]),Also, let's verify the schema validator won't break existing data.
🧰 Tools
🪛 Ruff (0.7.0)
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/hl7monitor.py (2)
43-47: Well, this is actually quite nice.
Clean implementation using enum values for dynamic action choices. I suppose we can't really complain about this one.
49-59: Oh, I see we're playing the "guess what these methods do" game.
While these capability methods are probably self-explanatory, it would be fantastic if we could verify that other asset types implement these consistently.
Consider adding docstrings to explain the purpose and impact of these capability flags:
@staticmethod
def is_movable():
"""
Indicates if this asset type can be physically relocated.
Used for asset management workflows.
"""
return Falsecare/utils/models/validators.py (1)
2-2: LGTM! The import statement looks perfectly fine.
The addition of Callable to the existing import statement from collections.abc is clean and well-organized.
care/facility/tests/test_assetbed_api.py (1)
Line range hint 1-364: Just checking - are we testing all asset class combinations?
The tests cover basic CRUD operations nicely, but it might be worth adding test cases for different asset class combinations to ensure we're handling all scenarios correctly.
Would you like me to help generate additional test cases for different asset class combinations?
care/facility/tests/test_asset_api.py (1)
Line range hint 1-450: Verify removal of all camera-related test cases
I notice we've removed some camera-related tests, but let's make absolutely sure we haven't missed any lingering camera references in the remaining test cases.
✅ Verification successful
Camera-related test cases have been properly handled... mostly
The only remaining camera references are in the negative test cases for VentilatorAsset and HL7MonitorAsset, which actually verify that camera_access_key is not allowed for these asset types. These test cases are serving their purpose by ensuring camera configurations don't sneak into non-camera assets, so they should stay.
• Lines 130-136: The test cases appropriately validate that camera_access_key is rejected for both ventilator and HL7 monitor assets
I suppose one could say the test coverage is quite thorough, even if it's just making sure things that shouldn't be there... aren't there.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining camera-related test cases or assertions
rg -i "camera|onvif" --type py
Length of output: 9069
Script:
#!/bin/bash
# Let's check if there are any camera-related test cases in the test file that we might have missed
ast-grep --pattern 'def test_$_camera$_$_($_) { $$$ }' care/facility/tests/test_asset_api.py
# Also check for any camera-related assertions or test data
rg -i "camera|onvif" care/facility/tests/test_asset_api.py -A 2 -B 2
Length of output: 991
care/facility/models/asset.py (3)
10-10: Import of get_dynamic_asset_meta is appropriate
Updating the import to get_dynamic_asset_meta correctly reflects the move towards dynamic asset metadata handling.
17-20: Validators are properly imported
Including DynamicJSONFieldSchemaValidator and PhoneNumberValidator ensures that the necessary validators are available for the model fields.
101-103: Dynamic JSON schema validation is well-implemented
Adding DynamicJSONFieldSchemaValidator(get_dynamic_asset_meta) to the meta field enhances validation by ensuring the JSON data conforms to the expected schema.
care/facility/api/serializers/bed.py (3)
124-133: Dynamic asset class validation enhances flexibility
This change allows for dynamic determination of valid asset classes when linking assets to beds, improving maintainability.
323-327: Dynamic exclusion of asset classes in consultation bed creation
By dynamically determining which asset classes cannot be linked to a consultation bed, the code becomes more flexible and maintainable.
343-343: Properly excluding invalid asset classes from assets query
This ensures that only appropriate assets are considered when assigning to a consultation bed.
care/facility/api/serializers/asset.py (1)
144-146: Adding 'asset_class' field aligns with dynamic asset class handling.
This addition correctly integrates asset classes into the serializer.
| def get_asset_status(self): | ||
| return self.api_get(self.get_url("devices/status")) |
There was a problem hiding this comment.
Interesting how we forgot about error handling and timeouts here
The get_asset_status method seems to be missing some rather important features that exist in handle_action:
- Error handling for API failures
- Timeout parameter to prevent hanging requests
- Documentation about the expected response format
Here's a slightly more robust implementation:
- def get_asset_status(self):
- return self.api_get(self.get_url("devices/status"))
+ def get_asset_status(self, timeout=None):
+ """
+ Retrieves the current status of the ventilator device.
+
+ Args:
+ timeout (float, optional): Request timeout in seconds
+
+ Returns:
+ dict: Device status information
+
+ Raises:
+ ValidationError: If the API request fails
+ """
+ try:
+ return self.api_get(self.get_url("devices/status"), timeout=timeout)
+ except Exception as e:
+ raise ValidationError({"status": str(e)}) from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_asset_status(self): | |
| return self.api_get(self.get_url("devices/status")) | |
| def get_asset_status(self, timeout=None): | |
| """ | |
| Retrieves the current status of the ventilator device. | |
| Args: | |
| timeout (float, optional): Request timeout in seconds | |
| Returns: | |
| dict: Device status information | |
| Raises: | |
| ValidationError: If the API request fails | |
| """ | |
| try: | |
| return self.api_get(self.get_url("devices/status"), timeout=timeout) | |
| except Exception as e: | |
| raise ValidationError({"status": str(e)}) from e |
| def get_asset_status(self): | ||
| return self.api_get(self.get_url("devices/status")) |
There was a problem hiding this comment.
I'm sure nothing could possibly go wrong with this implementation.
The get_asset_status method seems a bit... optimistic. It might be worth considering:
- Error handling for API failures
- Timeout parameter
- Documentation about the expected response format
Consider updating it like this:
def get_asset_status(self):
+ """
+ Fetch the current status of the asset from the device.
+
+ Returns:
+ dict: Status information containing:
+ - state: Current device state
+ - last_seen: Timestamp of last communication
+ [document other returned fields...]
+
+ Raises:
+ ValidationError: If the device is unreachable or returns invalid status
+ """
- return self.api_get(self.get_url("devices/status"))
+ try:
+ return self.api_get(self.get_url("devices/status"), timeout=5)
+ except Exception as e:
+ raise ValidationError({"status": f"Failed to fetch device status: {str(e)}"})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_asset_status(self): | |
| return self.api_get(self.get_url("devices/status")) | |
| def get_asset_status(self): | |
| """ | |
| Fetch the current status of the asset from the device. | |
| Returns: | |
| dict: Status information containing: | |
| - state: Current device state | |
| - last_seen: Timestamp of last communication | |
| [document other returned fields...] | |
| Raises: | |
| ValidationError: If the device is unreachable or returns invalid status | |
| """ | |
| try: | |
| return self.api_get(self.get_url("devices/status"), timeout=5) | |
| except Exception as e: | |
| raise ValidationError({"status": f"Failed to fetch device status: {str(e)}"}) |
| _registry = {} | ||
|
|
There was a problem hiding this comment.
Separate _registry per subclass to prevent unintended sharing
Currently, _registry is defined at the base class MutableEnum, meaning all subclasses share the same registry. This could lead to different enums unexpectedly sharing members. To ensure each enum class maintains its own members, initialize _registry in each subclass.
Apply this diff to fix the issue:
class MutableEnum(metaclass=MutableEnumMeta):
"""
A base class to mimic enum behavior with mutability.
"""
- _registry = {}
+ def __init_subclass__(cls, **kwargs):
+ super().__init_subclass__(**kwargs)
+ cls._registry = {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _registry = {} | |
| def __init_subclass__(cls, **kwargs): | |
| super().__init_subclass__(**kwargs) | |
| cls._registry = {} | |
There was a problem hiding this comment.
not needed according to me . Need more input though
There was a problem hiding this comment.
@DraKen0009, certainly. The issue arises because _registry is currently defined in the base class MutableEnum, causing all subclasses to share the same _registry dictionary. This means that if you have multiple subclasses of MutableEnum, their members could overlap unintentionally. By initializing _registry in each subclass using __init_subclass__, you ensure that each subclass maintains its own separate registry of members. This prevents any unintended sharing between different enums. Hope this helps clarify!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
care/facility/api/serializers/asset.py (3)
144-146: LGTM, though some documentation would be lovely.The dynamic asset class field implementation aligns well with the model's requirements. I see you've made it optional, which is... thoughtful.
A docstring explaining the field's purpose and valid choices would make this even better, but I suppose we can live without it. 🙃
236-238: That inline comment is practically begging for a refactor.The filtering logic works, but that inline comment about naming is... interesting. Let's make it more explicit.
Consider extracting this into a descriptive method:
-asset_class__in=[ - member.name - for member in AssetClasses.all() - if member.value.can_be_linked_to_asset_bed # need better naming -], +asset_class__in=AssetClasses.get_linkable_asset_classes(),And in AssetClasses:
@classmethod def get_linkable_asset_classes(cls): return [ member.name for member in cls.all() if member.value.can_be_linked_to_asset_bed ]
415-419: The flattening looks good, but have you considered caching?While the implementation correctly flattens the choices as suggested, we might want to avoid regenerating this list on every serializer instantiation.
Consider caching the choices:
+@classmethod +@cached_property +def get_flattened_action_choices(cls): + return [ + choice + for asset_class in AssetClasses.all() + for choice in asset_class.value.get_action_choices() + ] class AssetActionSerializer(Serializer): type = ChoiceField( - choices=[ - choice - for asset_class in AssetClasses.all() - for choice in asset_class.value.get_action_choices() - ], + choices=get_flattened_action_choices, required=True, )I mean, unless you enjoy regenerating the same list repeatedly... 😏
care/facility/tests/test_asset_api.py (1)
138-147: The invalid asset class test is good, but could be even better...While the test correctly validates rejection of invalid asset classes, it might be nice to add a few more edge cases. You know, just to be thorough... 😏
Consider adding these test variations:
def test_create_asset_with_invalid_asset_class(self): - sample_data = { - "name": "Test Asset", - "asset_type": 50, - "location": self.asset_location.external_id, - "asset_class": "INVALID_CLASS", - } - response = self.client.post("/api/v1/asset/", sample_data) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + invalid_classes = [ + "INVALID_CLASS", + "", # empty string + "camera", # lowercase + "CAMERA", # removed class + None, # null value + ] + + for invalid_class in invalid_classes: + with self.subTest(asset_class=invalid_class): + sample_data = { + "name": "Test Asset", + "asset_type": 50, + "location": self.asset_location.external_id, + "asset_class": invalid_class, + } + response = self.client.post("/api/v1/asset/", sample_data) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)care/facility/api/viewsets/asset.py (2)
393-401: Consider renaming loop variable for clarityUsing
assetas the loop variable inavailable_asset_classes = [asset.name for asset in AssetClasses.all()]might be a bit confusing sinceassetis also the variable representing the current asset. Maybe renaming it tomemberorasset_classcould enhance readability.
475-477: Improve method naming for clarityNot to nitpick, but since the comment mentions "better naming required," perhaps renaming
can_be_linked_to_asset_bed()to something likesupports_bed_linkage()might make its purpose clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
care/facility/api/serializers/asset.py(3 hunks)care/facility/api/viewsets/asset.py(3 hunks)care/facility/tests/test_asset_api.py(1 hunks)care/utils/models/validators.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- care/utils/models/validators.py
🧰 Additional context used
📓 Learnings (1)
care/facility/api/serializers/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.
🔇 Additional comments (1)
care/facility/tests/test_asset_api.py (1)
133-133: LGTM! The asset_class field addition looks good.
The explicit "VENTILATOR" asset class is a valid choice and aligns well with the move away from camera-specific implementations.
care/facility/api/viewsets/asset.py
Outdated
| movable_assets = [ | ||
| member.name for member in AssetClasses.all() if not member.is_movable | ||
| ] | ||
| if value: | ||
| queryset = queryset.filter( | ||
| asset_class__in=[ | ||
| AssetClasses.ONVIF.name, | ||
| AssetClasses.HL7MONITOR.name, | ||
| ] | ||
| ) | ||
| queryset = queryset.filter(asset_class__in=movable_assets) | ||
| else: | ||
| queryset = queryset.exclude( | ||
| asset_class__in=[ | ||
| AssetClasses.ONVIF.name, | ||
| AssetClasses.HL7MONITOR.name, | ||
| ] | ||
| ) | ||
| queryset = queryset.exclude(asset_class__in=movable_assets) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Variable naming inconsistency with 'movable_assets'
It appears that movable_assets actually contains asset classes where member.is_movable is False, meaning they are non-movable assets. Perhaps renaming the variable to non_movable_assets would improve clarity.
rithviknishad
changed the title
care/facility/models/__init__.py
Outdated
| @@ -4,7 +4,6 @@ | |||
| from .ambulance import * # noqa | |||
| from .asset import * # noqa | |||
| from .bed import * # noqa | |||
| from .camera_preset import * # noqa | |||
There was a problem hiding this comment.
removing this now will cause a pending make migration as 0 references are there to the old CameraPreset model now. lets remove it in the second part PR where we would be deleting the old model.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2610 +/- ##
===========================================
- Coverage 69.58% 69.43% -0.15%
===========================================
Files 211 209 -2
Lines 11875 11873 -2
Branches 1201 1197 -4
===========================================
- Hits 8263 8244 -19
- Misses 3246 3258 +12
- Partials 366 371 +5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
care/facility/api/serializers/asset.py (3)
144-144: Consider enhancing asset_class validation with explicit choicesWhile the validation works, it might be slightly more robust to define the choices directly on the field rather than just in the validate method. You know, just in case someone forgets to check the validation in the future.
- asset_class = serializers.CharField(required=False) + asset_class = serializers.CharField( + required=False, + allow_null=True, + allow_blank=True, + choices=Asset.get_asset_class_choices + )Also applies to: 167-173
241-243: That comment is practically begging for attentionThe comment
# need better namingis quite right. The predicatecan_be_linked_to_asset_bedcould be more descriptive of its actual purpose in IP address validation.Consider renaming to something more specific like
requires_unique_ip_addressoris_network_device.
Line range hint
359-371: Looks like we missed a spot with the camera removalThe
to_representationmethod still contains camera-specific logic for handlingcamera_access_key. Since we're removing everything related to cameras, shouldn't this be removed as well?def to_representation(self, instance: Asset): data = super().to_representation(instance) data["ip_address"] = instance.meta.get("local_ip_address") - if camera_access_key := instance.meta.get("camera_access_key"): - values = camera_access_key.split(":") - if len(values) == 3: # noqa: PLR2004 - data["username"], data["password"], data["access_key"] = values return data
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
care/facility/api/serializers/asset.py(4 hunks)
🔇 Additional comments (1)
care/facility/api/serializers/asset.py (1)
420-424: Nice implementation of the flattened choices
The list comprehension elegantly handles the flattening of choices from all asset classes. This addresses the previous feedback perfectly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
care/facility/api/serializers/asset.py (3)
144-144: Consider moving validation to field levelThe
asset_classvalidation could be more elegantly handled at the field level usingChoiceFieldinstead ofCharField. You know, just like how it's done for other choice fields in this file...- asset_class = serializers.CharField(required=False) + asset_class = ChoiceField( + choices=Asset.get_asset_class_choices, + required=False + )
167-173: Enhance error message with valid choicesThe error message could be more helpful by including the list of valid choices. I mean, if we're going to tell users they're wrong, we might as well tell them how to be right...
- error = f"{attrs['asset_class']} is not a valid asset class" + valid_choices = ", ".join(Asset.get_asset_class_choices()) + error = f"{attrs['asset_class']} is not a valid asset class. Valid choices are: {valid_choices}"
241-243: Improve method naming and readabilityThe comment "need better naming" is quite right. The method name
can_be_linked_to_asset_bedis a bit... mysterious. Also, this list comprehension could be more readable.Consider:
- Renaming the method to something more descriptive like
supports_ip_address_linking- Making the list comprehension more readable:
- member.name - for member in AssetClasses.all() - if member.value.can_be_linked_to_asset_bed() + member.name + for member in AssetClasses.all() + if member.value.supports_ip_address_linking() # renamed for clarity
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
care/facility/api/serializers/asset.py(4 hunks)care/facility/api/viewsets/asset.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- care/facility/api/viewsets/asset.py
🔇 Additional comments (1)
care/facility/api/serializers/asset.py (1)
420-424: Structure choices properly for ChoiceField
This looks suspiciously similar to an issue that was flagged before...
The current structure might cause issues with ChoiceField. Consider flattening the choices as previously suggested:
- choices=[
- choice
- for asset_class in AssetClasses.all()
- for choice in asset_class.value.get_action_choices()
- ],
+ choices=lambda: [
+ choice for asset_class in AssetClasses.all()
+ for choice in asset_class.value.get_action_choices()
+ ],
Proposed Changes
camerafrom care and using dynamic approaches to load plugin data if presentAssociated Issue
Merge Checklist
Only PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
Tests