Adding Test cases for Location Viewset #2818
Conversation
📝 WalkthroughWalkthroughThis pull request enhances the encounter validation logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant V as FacilityLocationEncounterViewSet
participant L as _validate_data Logic
C->>V: Submit encounter data
V->>L: Process data validation
alt instance.end_datetime exists
L-->>V: Use instance.end_datetime
else
L-->>V: Use model_obj.end_datetime
end
V->>L: Evaluate status value
alt status is "active"
L-->>V: Enforce end_datetime is None
end
V-->>C: Return validated response
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 (2)
care/emr/tests/test_location_api.py (2)
30-44: Consider returning 201 for resource creation
The method generate_data_for_facility_location() is well structured to randomize essential fields for testing. However, as many test flows rely on this data for POST requests, consider standardizing your expectations (e.g., returning status code 201) to reflect typical RESTful conventions for creation. This would yield clearer semantics and might highlight discrepancies in your code if 201 is not returned.
45-65: Verify status code for creation
While this function successfully creates a location and checks for a 200 response, returning 201 (Created) is a common HTTP standard for new resource creation. Confirm whether the underlying API endpoint intentionally returns 200 or if it should ideally return 201.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/api/viewsets/location.py(1 hunks)care/emr/tests/test_location_api.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test / Test
- GitHub Check: Analyze (python)
🔇 Additional comments (11)
care/emr/tests/test_location_api.py (10)
78-90: Good setup approach
The setUp() method effectively prepares test dependencies (super_user, user, facility). It’s succinct and ensures each test starts with a consistent state.
92-106: Properly testing authentication flow
The test_list_facility_locations() method correctly verifies that unauthenticated or unauthorized requests receive 403 responses, then validates that authenticated users see 200 responses. This well covers the typical permission behavior for listing resources.
116-132: Permission checks are on point
test_create_facility_location_without_permission thoroughly checks multiple permission scenarios and ensures the correct error messages appear. This coverage is commendable.
160-202: Comprehensive parent-location validation
The test_create_with_parent_location() method covers various edge cases (wrong mode, invalid parent UUID, different facility, etc.). This helps prevent subtle logic leaks in hierarchical location creation.
248-270: Extra check for partial role update
test_update_facility_location_with_permissions properly checks the shift from “can_list_facility_locations” to “can_write_facility_locations.” The incremental permission approach ensures roles are managed accurately, preventing accidental privilege escalations.
325-350: Smart check on child dependencies
test_deleting_parent_location ensures that any parent location with active children cannot be removed, returning a 400 with a proper error message. This is a good guard against orphaning location data.
379-430: Organization handling logic
test_organizations_add_to_facility_location verifies permission checks and attempts to add a duplicate organization. This robust approach, combined with your “Organization already exists” message, prevents duplicate associations slipping in.
487-508: Sensible fixture setup for encounters
setUp() in TestFacilityLocationEncounterViewSet sets up a location, an encounter, and a base URL. The approach is consistent with the location tests, ensuring clarity and maintainability across different test scenarios.
608-631: Excellent scheduling conflict tests
test_create_encounter_with_conflicting_schedule confirms that new encounters with overlapping times cannot be created. This is crucial for date/time-based logic, preventing concurrency or scheduling breaches that can create contradictory data.
882-926: Validation logic on completed status
test_update_with_permission ensures a location encounter cannot revert from completed status. This upholds the domain rule that once an encounter is completed, it must remain final. This prevents data integrity issues and aligns with typical healthcare workflows.care/emr/api/viewsets/location.py (1)
358-371: Refined handling of status and end_datetime
The updated _validate_data method logically enforces that active encounters have no end_datetime and that completed encounters do have an end_datetime. This is critical to prevent invalid states. However, do note a tiny corner scenario where a transition from “active” to “planned” or “reserved” also mandates an end_datetime. Ensure callers handle that gracefully. Otherwise, this looks solid in verifying date/time constraints and blocking multiple active encounters.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2818 +/- ##
===========================================
- Coverage 56.24% 56.20% -0.05%
===========================================
Files 215 215
Lines 10237 10245 +8
Branches 1038 1042 +4
===========================================
Hits 5758 5758
- Misses 4463 4471 +8
Partials 16 16 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
care/emr/api/viewsets/location.py (1)
345-415: This method could use some refactoring to reduce complexity.The method has high cyclomatic complexity and handles multiple responsibilities. Consider breaking it down into smaller, focused methods.
Here's a suggested refactoring:
- def _validate_data(self, instance, model_obj=None): # noqa PLR0912 + def _validate_data(self, instance, model_obj=None): + self._validate_location_mode(instance) + self._validate_dates_and_status(instance, model_obj) + self._validate_schedule_conflicts(instance, model_obj) + self._validate_active_encounter_conflicts(instance, model_obj) + self._validate_status_transitions(instance, model_obj) + + def _validate_location_mode(self, instance): + location = self.get_location_obj() + if location.mode == FacilityLocationModeChoices.instance.value: + raise ValidationError("Cannot assign encounters for Location instances") + + def _validate_dates_and_status(self, instance, model_obj): + status = instance.status or model_obj.status if model_obj else instance.status + end_datetime = self._get_end_datetime(instance, model_obj) + + if status == LocationEncounterAvailabilityStatusChoices.active.value: + end_datetime = None + + if end_datetime and instance.start_datetime > end_datetime: + raise ValidationError("End Datetime should be greater than Start Datetime") + + if self._requires_end_datetime(status) and not end_datetime: + raise ValidationError("End Datetime is required for completed status") + + def _get_end_datetime(self, instance, model_obj): + if instance.end_datetime is not None: + return instance.end_datetime + return model_obj.end_datetime if model_obj else None + + def _requires_end_datetime(self, status): + return status in ( + LocationEncounterAvailabilityStatusChoices.completed.value, + LocationEncounterAvailabilityStatusChoices.reserved.value, + LocationEncounterAvailabilityStatusChoices.planned.value, + )
🧹 Nitpick comments (5)
care/emr/resources/location/spec.py (1)
89-97: Nice error handling improvements!The changes improve error handling by using try-except and chaining exceptions. The additional validation for parent location mode is also a good addition.
Consider adding more descriptive error messages that include the parent ID:
- raise ValueError("Parent not found") from e + raise ValueError(f"Parent location with ID {self.parent} not found") from e - raise ValueError("Instances cannot have children") + raise ValueError(f"Location instance with ID {self.parent} cannot have children")care/emr/api/viewsets/location.py (1)
387-394: The schedule conflict validation could be more descriptive.The error message doesn't provide enough context about the conflicting schedule.
Consider adding more details to the error message:
if base_qs.filter( start_datetime__lte=end_datetime, end_datetime__gte=start_datetime ).exists(): - raise ValidationError("Conflict in schedule") + raise ValidationError( + f"Schedule conflict: Another encounter is scheduled between " + f"{start_datetime} and {end_datetime}" + )care/emr/tests/test_location_api.py (3)
493-493: The RuntimeWarning ignore could be more specific.The ignore_warnings decorator suppresses datetime warnings, but it could be more targeted.
Consider using a more specific pattern:
-@ignore_warnings(category=RuntimeWarning, message=r".*received a naive datetime.*") +@ignore_warnings(category=RuntimeWarning, message=r"DateTimeField .* received a naive datetime .* while time zone support is active")
514-523: The test data generation could be more flexible.The generate_facility_location_encounter_data method has hardcoded defaults that might not suit all test cases.
Consider using an Enum or constants for the default values:
+class TestDefaults: + STATUS = LocationEncounterAvailabilityStatusChoices.active.value + START_DATETIME = timezone.now() + END_DATETIME = None def generate_facility_location_encounter_data(self, encounter_id, **kwargs): data = { - "status": LocationEncounterAvailabilityStatusChoices.active.value, - "encounter": encounter_id, - "start_datetime": timezone.now(), - "end_datetime": None, + "status": TestDefaults.STATUS, + "encounter": encounter_id, + "start_datetime": TestDefaults.START_DATETIME, + "end_datetime": TestDefaults.END_DATETIME, } data.update(kwargs) return data
859-863: The assertion could be more precise.The test is checking for non-existence using a filter, but there's a more direct way.
Consider using
assertRaisesto make the test more explicit:- self.assertFalse( - FacilityLocation.objects.filter( - external_id=facility_location_encounter["id"] - ).exists() - ) + with self.assertRaises(FacilityLocation.DoesNotExist): + FacilityLocation.objects.get(external_id=facility_location_encounter["id"])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/api/viewsets/location.py(2 hunks)care/emr/resources/location/spec.py(1 hunks)care/emr/tests/test_location_api.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test / Test
- GitHub Check: Analyze (python)
Proposed Changes
Associated Issue
Architecture changes
Merge 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