Fixing list asset bed preset and list bed preset #2650
Conversation
|
Warning Rate limit exceeded@DraKen0009 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve a modification to the Changes
Poem
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
care/facility/api/viewsets/camera_preset.py (1)
41-43: Have you considered a more... elegant solution?While filtering deleted assets works here, it might be worth considering a more centralized approach. Perhaps we could:
- Move this logic to a custom model manager
- Create a utility function in
care.utils.queryset.asset_bedThis would help maintain consistency across the entire codebase, but you know, no pressure or anything.
Example implementation:
# in models.py or managers.py class CameraPresetManager(models.Manager): def active(self): return self.filter(asset_bed__deleted=False) # in viewset queryset = CameraPreset.objects.active().select_related(...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/facility/api/viewsets/camera_preset.py(1 hunks)
🔇 Additional comments (1)
care/facility/api/viewsets/camera_preset.py (1)
41-43: The filter looks fine, I suppose... but let's verify the implementation.
The addition of asset_bed__deleted=False filter is a reasonable approach to exclude deleted asset beds. However, it would be lovely if we could verify this is the only place that needs this filter.
✅ Verification successful
Well, someone did their homework with the deleted filter...
The asset_bed__deleted=False filter in CameraPresetViewSet is perfectly fine and consistent with the codebase's patterns. I couldn't help but notice that:
AssetBedCameraPresetViewSetdoesn't need this filter as it usesget_asset_bed_queryset()which presumably handles deletion logic- Other viewsets dealing with related entities (like
PatientAssetBedViewSet) handle deletion through their own mechanisms - The codebase consistently filters deleted records across various viewsets
So, while I suppose we could nitpick about test coverage, the implementation itself is... actually quite proper.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other querysets that might need similar filtering
ast-grep --pattern 'CameraPreset.objects.$_($$$)'
# Look for any other asset_bed related querysets
rg -A 3 'asset_bed.*objects\.'
Length of output: 1586
Script:
#!/bin/bash
# Let's check if AssetBedCameraPresetViewSet has any filtering
ast-grep --pattern 'class AssetBedCameraPresetViewSet($$$)'
# Check if there are any other places using asset_bed with deleted field
rg -A 3 'asset_bed.*deleted'
# Let's also check the model definition to understand the relationship
ast-grep --pattern 'class CameraPreset($$$)'
Length of output: 487
Script:
#!/bin/bash
# Let's see what AssetBedCameraPresetViewSet actually does
rg -A 10 'class AssetBedCameraPresetViewSet'
# And check if there are any other viewsets dealing with asset_bed
rg -A 5 'class.*AssetBed.*ViewSet'
# One last check for any other deleted filters in viewsets
rg -A 3 'objects\.filter.*deleted'
Length of output: 11889
…009/care into fixing-list-viewset-of-camera-preset
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
care/facility/tests/test_assetbed_api.py (3)
430-430: Remove or complete the commented code.This line appears to be an incomplete comment. If it was meant to set up test data, it should be removed or completed.
- # self.asset_bed1.update(deleted=)
431-432: Inconsistent spacing around operators and after commas.The code would look slightly more polished with consistent spacing around operators and after commas. I mean, if you care about that sort of thing...
- res=self.client.post( + res = self.client.post(Also applies to: 444-445, 458-460, 465-467
429-469: Test could be more comprehensive.While the test covers the basic functionality, it might be worth adding a few more assertions to make it absolutely bulletproof:
- Verify the remaining preset belongs to
asset_bed2- Verify the deleted preset is actually not accessible
- Add assertions for the response status codes
Here's a suggested enhancement:
res = self.client.get( f"/api/v1/bed/{self.bed.external_id}/camera_presets/" ) + self.assertEqual(res.status_code, status.HTTP_200_OK) self.assertEqual(len(res.json()["results"]), 2) self.asset_bed1.delete() self.asset_bed1.refresh_from_db() res = self.client.get( f"/api/v1/bed/{self.bed.external_id}/camera_presets/" ) + self.assertEqual(res.status_code, status.HTTP_200_OK) self.assertEqual(len(res.json()["results"]), 1) + # Verify the remaining preset belongs to asset_bed2 + self.assertEqual(res.json()["results"][0]["asset_bed"], str(self.asset_bed2.external_id)) + + # Verify deleted preset is not accessible + res = self.client.get( + self.get_base_url(self.asset_bed1.external_id) + ) + self.assertEqual(res.status_code, status.HTTP_404_NOT_FOUND)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/facility/tests/test_assetbed_api.py(1 hunks)
🔇 Additional comments (1)
care/facility/tests/test_assetbed_api.py (1)
429-429: Consider adding edge cases for deleted asset beds.
The current test covers the basic scenario, but you might want to consider adding these slightly important edge cases:
- Attempting to create a preset for a deleted asset bed
- Attempting to update a preset after its asset bed is deleted
- Bulk deletion of asset beds and its effect on presets
Let's check if there are any existing tests covering these scenarios:
Proposed Changes
Fixed : #2619
deleted=Falsefor list bed preset, to fix listing deleted assetbed bedsMerge Checklist
/docsOnly 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