Update queryset references in views to avoid cached results #2582
Conversation
WalkthroughThe pull request introduces updates across several viewsets in the application, primarily focusing on 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2582 +/- ##
========================================
Coverage 69.20% 69.20%
========================================
Files 211 211
Lines 11942 11944 +2
Branches 1208 1208
========================================
+ Hits 8264 8266 +2
Misses 3302 3302
Partials 376 376 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
care/facility/api/viewsets/patient_otp_data.py (1)
Line range hint
21-26: Yo, solid upgrade on that queryset fetch! 🚀Using
super().get_queryset()instead of directly accessingself.querysetis totally the way to go! It ensures you're always getting fresh data instead of potentially stale cached results. Keep that data flow clean and crispy!Pro tip: You might want to add a quick docstring to explain the OTP filtering logic, something chill like:
def get_queryset(self): + """ + Returns patient data filtered by phone number for OTP users, + empty queryset for everyone else. Keeps it exclusive, you know what I mean? 😎 + """ is_otp_login = getattr(self.request.user, "is_alternative_login", False)care/facility/api/viewsets/mixins/access.py (1)
Line range hint
23-39: Hey fam, heads up about some filtering inconsistency! 🤔I noticed
filter_by_user_scopeandget_querysetare like twins but not identical - they're doing similar things but checking for slightly different attributes:
get_querysetchecks fordistrictfilter_by_user_scopechecks fordistrict_idThis could get confusing real quick! Maybe we should make these consistent?
Here's a chill way to refactor this:
def get_queryset(self): queryset = super().get_queryset() model = self.queryset.__dict__["model"] if not self.request.user.is_superuser: instance = model() if self.request.user.user_type >= User.TYPE_VALUE_MAP["DistrictAdmin"]: - if hasattr(instance, "district"): + if hasattr(instance, "district_id"): queryset = queryset.filter(district=self.request.user.district) - if hasattr(instance, "facility"): + if hasattr(instance, "facility_id"): queryset = queryset.filter( facility__district=self.request.user.district )This way, both methods will be checking for the same attributes. Much cleaner! 🧹
care/facility/api/viewsets/patient_external_test.py (2)
Line range hint
134-137: Hey, let's make this permission check a bit more user-friendly! 🤙The check is working fine, but we could make it more helpful for the users who get denied.
Here's a chill way to improve it:
def destroy(self, request, *args, **kwargs): if self.request.user.user_type < User.TYPE_VALUE_MAP["DistrictLabAdmin"]: - raise PermissionDenied + raise PermissionDenied("Sorry fam, only District Lab Admins and above can delete test records! 🛑") return super().destroy(request, *args, **kwargs)
Line range hint
154-189: Digging the validation vibes, but we can make it even smoother! 🌊The district validation is crucial, but we could make this whole flow more efficient and readable.
Here's a more streamlined approach:
@action(methods=["POST"], detail=False) def bulk_upsert(self, request, *args, **kwargs): if not self.check_upload_permission(): - msg = "Permission to Endpoint Denied" - raise PermissionDenied(msg) + raise PermissionDenied("No access to this endpoint, my friend! 🚫") if "sample_tests" not in request.data: raise ValidationError({"sample_tests": "No Data was provided"}) if not isinstance(request.data["sample_tests"], list): raise ValidationError({"sample_tests": "Data should be provided as a list"}) - # check if the user is from same district - for data in request.data["sample_tests"]: - if str(request.user.district) != data["district"]: - raise ValidationError({"Error": "User must belong to same district"}) + # Validate all districts match in one go + if any(str(request.user.district) != data["district"] for data in request.data["sample_tests"]): + raise ValidationError({"Error": "All samples must be from your district! 🗺️"}) errors = [] - ser_objects = [] - invalid = False - for sample in request.data["sample_tests"]: - serializer = PatientExternalTestSerializer(data=sample) - valid = serializer.is_valid() - current_error = pretty_errors(serializer._errors) # noqa: SLF001 - if current_error and (not valid): - errors.append(current_error) - invalid = True - ser_objects.append(serializer) - if invalid: + serializers = [PatientExternalTestSerializer(data=sample) for sample in request.data["sample_tests"]] + + if not all(serializer.is_valid() for serializer in serializers): + errors = [pretty_errors(s._errors) for s in serializers if not s.is_valid()] # noqa: SLF001 return Response(errors, status=status.HTTP_400_BAD_REQUEST) - for ser_object in ser_objects: - ser_object.save() + + for serializer in serializers: + serializer.save() return Response(status=status.HTTP_202_ACCEPTED)This refactor:
- Makes district validation more efficient with a single pass
- Simplifies serializer handling
- Keeps the same functionality but with cleaner code
Consider using bulk_create for better performance if you're dealing with large datasets!
care/facility/api/viewsets/shifting.py (2)
Line range hint
112-152: Yo, nice queryset optimization! 🚀The changes look solid! Using
super().get_queryset()is totally the way to go for fresh results. Love how you split theselect_relatedfields between list and detail views - that's some performance-conscious coding right there!Just one chill suggestion - maybe we could extract those select_related fields into class constants? Would make it easier to maintain those long lists.
class ShiftingViewSet: LIST_SELECT_RELATED = [ "origin_facility", "shifting_approving_facility", "assigned_facility", "patient", ] DETAIL_SELECT_RELATED = [ "origin_facility", "origin_facility__ward", # ... rest of the fields ]
Line range hint
249-285: Hey, let's keep the fresh queryset vibe going! 🌊While we're updating queryset patterns, might wanna give this method the same treatment. Adding
super().get_queryset()here would keep things consistent with the changes above.def get_queryset(self): - queryset = self.queryset.filter( + queryset = super().get_queryset().filter( request__external_id=self.kwargs.get("shift_external_id") )care/facility/api/viewsets/patient_consultation.py (1)
Line range hint
85-113: Yo, nice refactor on the get_queryset method! 🎯The changes look solid! Using
super().get_queryset()and a local variable is totally the way to go for avoiding those pesky cached results. The permission checks and filters are still doing their thing just right.Just one tiny suggestion to make it even more awesome - consider extracting those complex permission filters into a separate method for better readability. Something like this would be super clean:
def get_queryset(self): queryset = super().get_queryset() if self.serializer_class == PatientConsultationSerializer: queryset = queryset.prefetch_related(...) if self.request.user.is_superuser: return queryset if self.request.user.user_type >= User.TYPE_VALUE_MAP["StateLabAdmin"]: return queryset.filter(patient__facility__state=self.request.user.state) if self.request.user.user_type >= User.TYPE_VALUE_MAP["DistrictLabAdmin"]: return queryset.filter( patient__facility__district=self.request.user.district ) + return self._apply_facility_filters(queryset) + +def _apply_facility_filters(self, queryset): allowed_facilities = get_accessible_facilities(self.request.user) applied_filters = Q( Q(patient__is_active=True) & Q(patient__facility__id__in=allowed_facilities) ) applied_filters |= Q(facility=self.request.user.home_facility) return queryset.filter(applied_filters)But no pressure, the current implementation is totally working great too! 🚀
care/users/api/viewsets/users.py (1)
181-181: Nice security move, but we can make it even smoother! 🔒Using
self.get_queryset()is totally the right call for keeping those permissions tight. But check this out - we could make it a bit more efficient.Instead of getting the queryset and then filtering it again with username, we could do it all in one go:
- queryset = self.get_queryset() - username = kwargs["username"] - if request.user.is_superuser: + username = kwargs["username"] + if request.user.is_superuser: + queryset = self.get_queryset()Just a small optimization to keep things running smooth! 🏃♂️
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
care/facility/api/viewsets/mixins/access.py(1 hunks)care/facility/api/viewsets/patient_consultation.py(3 hunks)care/facility/api/viewsets/patient_external_test.py(1 hunks)care/facility/api/viewsets/patient_otp_data.py(1 hunks)care/facility/api/viewsets/shifting.py(2 hunks)care/users/api/viewsets/users.py(2 hunks)
🔇 Additional comments (5)
care/facility/api/viewsets/mixins/access.py (2)
8-8: Yo, solid move using super().get_queryset()! 🎯
This change is totally on point - it ensures we're always getting fresh data instead of potentially stale cached results. Perfect alignment with the PR's goals!
Line range hint 47-60: Quick security check on the asset auth flow! 🔒
The authentication setup looks solid, but let's make sure we're not accidentally skipping any important checks in the authentication chain.
✅ Verification successful
Let me check the authentication chain more thoroughly before making a final call.
Yo, the auth flow is totally solid! 🔒✌️
The security setup is actually pretty tight:
- Asset authentication is properly implemented with JWT validation and explicit asset_id checks
- DRYAssetPermissions adds an extra layer by prefixing asset-specific permissions
- Asset users are properly restricted from write operations in multiple places
- No sketchy direct asset assignments found outside of test files
Keep rockin' that secure code! 🎸
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the authentication chain and usage
# Look for any bypass of asset authentication or direct asset access
# Check for proper asset authentication usage
echo "Checking for proper asset authentication usage..."
rg -l "MiddlewareAssetAuthentication"
# Check for potential security bypasses
echo "Checking for potential security bypasses..."
rg "request.user.asset" -C 3
Length of output: 6596
Script:
#!/bin/bash
# Let's check the authentication implementation and permission classes
# Check the middleware asset authentication implementation
echo "Checking MiddlewareAssetAuthentication implementation..."
rg -A 10 "class MiddlewareAssetAuthentication"
# Check DRYAssetPermissions implementation
echo "Checking DRYAssetPermissions implementation..."
rg -A 10 "class DRYAssetPermissions"
# Check if there are any direct asset assignments or modifications
echo "Checking asset assignments..."
rg "\.asset\s*=" -C 3
Length of output: 11213
care/facility/api/viewsets/patient_external_test.py (2)
Line range hint 98-112: Yo, this queryset change is solid! 🎯
Switching to super().get_queryset() is totally the way to go - keeps things fresh and avoids those pesky cached results. The permission filtering is still on point too!
Line range hint 142-152: CSV handling looking clean! ✨
The CSV export logic is neat, but let's make sure those mapping keys are all good.
Let's double-check those CSV mappings real quick:
care/users/api/viewsets/users.py (1)
128-128: Yo, this change is solid! 🎯
Switching to super().get_queryset() is a sweet move - keeps things fresh and inherits all the good stuff from the parent class. No cached surprises here!
Refactor queryset retrieval in various views to ensure fresh data is fetched instead of getting unexpected cached results.
Summary by CodeRabbit
Release Notes
New Features
PatientExternalTestViewSetfor better data export functionality.Bug Fixes
Refactor
get_objectmethod of theUserViewSetto provide clearer feedback when a user is not found.