Add models for schedule and availability #2576
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2576 +/- ##
===========================================
+ Coverage 69.20% 69.85% +0.64%
===========================================
Files 211 216 +5
Lines 11944 12234 +290
Branches 1208 1227 +19
===========================================
+ Hits 8266 8546 +280
- Misses 3302 3307 +5
- Partials 376 381 +5 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
vigneshhari
changed the title
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a comprehensive suite of files focused on managing scheduling functionalities within a facility management system. It includes serializers, viewsets, models, service functions, and tests specifically designed for schedules and schedule exceptions. New routes are added to the API router to facilitate access to these features. The implementation emphasizes CRUD operations, data validation, and permission checks, ensuring robust handling of scheduling data and related exceptions. 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 (
|
![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 (15)
care/facility/svc/schedule.py (1)
17-23: The function works, but would it hurt to add some documentation?While the implementation is correct, adding docstring with return type and parameter descriptions would make it slightly more maintainable.
Consider adding database indexes on
valid_fromandvalid_tofields if they don't exist already, as this query will be frequently used. Here's a suggested improvement:def get_current_schedule(resource) -> QuerySet[Availability]: + """Get current availability schedule for a resource. + + Args: + resource: The resource to check availability for + + Returns: + QuerySet[Availability]: Currently valid availability schedules + """ now = timezone.now() return Availability.objects.filter( schedule__resource=resource, schedule__valid_from__lte=now, schedule__valid_to__gte=now, )care/facility/tests/schedule/test_schedule_exception_api.py (4)
20-21: Consider using a relative date instead of a hardcoded one.While the frozen date works for now, it might be worth using a relative date (like
today()ornow()) to prevent the tests from failing after November 2024. Not that I'm suggesting your tests will still be running unchanged by then, but you never know... 😏-@freeze_time("2024-11-01") +@freeze_time(datetime.now().replace(hour=0, minute=0, second=0, microsecond=0))
23-91: Add docstring to explain this rather elaborate setup.While I can almost follow what's happening here, a docstring explaining the test scenario would be really nice. You know, for those of us who like to understand things without having to piece together the story from comments like "he is on leave" and "he compensates".
Add something like:
@classmethod def setUpTestData(cls) -> None: """Sets up test data for schedule exceptions. Creates: - Facility hierarchy (state -> district -> local body -> facility) - Users (staff and doctor) - Schedule (Mon-Fri: 10-12 appointments, 14-16 open slots) - Schedule exceptions: 1. Leave period: Nov 5-7 2. Compensation slot: Nov 9, 10-12 """
112-117: Enhance error handling in schema validation.The current implementation swallows the validation error only to raise it again immediately. While I'm sure there's a perfectly good reason for this, perhaps we could add some context to make debugging easier?
def assert_response_schema(self, response: Response, schema: BaseModel): try: schema.validate(response.json()) except ValidationError as e: - raise e + raise ValidationError( + f"Response validation failed for {schema.__name__}: {str(e)}" + ) from e
139-166: Enhance update test assertions.While we're checking the slot size update, it might be slightly more thorough to verify:
- The name was updated correctly
- Other fields remained unchanged
- The response contains all expected fields with correct values
Add these assertions:
# Verify updated fields self.assertEqual(schedule_exception.name, "test schedule exception 2") # Verify unchanged fields remain intact self.assertEqual(schedule_exception.is_available, True) self.assertEqual(schedule_exception.tokens_per_slot, 10) self.assertEqual(schedule_exception.slot_type, SlotType.APPOINTMENT)care/facility/models/schedule.py (3)
22-24: Consider future-proofingRESOURCE_TO_MODEL.While mapping
ScheduleResourceType.DOCTORtoUserworks for now, you might want to handle potential future resource types to avoid hardcoding.
43-63: Permissions methods inAvailabilityensure proper access control.Delegating read and write permissions to the associated schedule maintains consistency. However, it might be beneficial to document this behavior.
75-76: Clarify the purpose ofis_availablefield.The
is_availableboolean field inScheduleExceptionmay need additional context or documentation to explain how it affects scheduling logic.care/facility/api/viewsets/schedule.py (2)
61-67: Reconsider the Order of Filter Backends for ConsistencyIn the
ScheduleViewSet, thefilter_backendslist placesScheduleDRYFilterSetbeforefilters.DjangoFilterBackend. While this may functionally work, it's generally advisable to place permission-related filters after standard filters to maintain consistency and potentially improve performance.Consider reordering the
filter_backends:filter_backends = [ - ScheduleDRYFilterSet, filters.DjangoFilterBackend, + ScheduleDRYFilterSet, ]This minor adjustment aligns the ordering with common practices, applying standard filters before permission checks.
104-107: Explicitly HandledestroyAction in Serializer SelectionIn
ScheduleExceptionViewSet, theget_serializer_classmethod doesn't explicitly handle thedestroyaction. While it may default to the read-only serializer, specifying it enhances code clarity.Add explicit handling for the
destroyaction:def get_serializer_class(self): if self.action in ("create", "update", "partial_update"): return ScheduleExceptionCreateSerializer + if self.action == "destroy": + return ScheduleExceptionReadOnlySerializer return ScheduleExceptionReadOnlySerializerThis explicit declaration makes the code more readable and maintainable.
care/facility/tests/schedule/test_schedule_api.py (2)
132-135: Redundant try-except block around schema validationCatching a
ValidationErroronly to re-raise it doesn't add much value. It might be cleaner to let the exception propagate naturally.Simplify the method like this:
- try: - schema.parse_obj(response.json()) - except ValidationError as e: - raise e + schema.parse_obj(response.json())
92-98: Refactor URL construction for better maintainabilityBuilding URLs by string concatenation might work, but using Django's
reversefunction or URL resolvers can make your code more reliable and easier to maintain, especially if the URL patterns change.Consider refactoring as follows:
+from django.urls import reverse ... def get_url(self, entry_id=None, action=None): - base_url = f"/api/v1/facility/{self.facility.external_id}/schedule/" + base_url = reverse('facility-schedule-list', kwargs={'facility_external_id': self.facility.external_id}) if entry_id is not None: - base_url += f"{entry_id}/" + base_url = reverse('facility-schedule-detail', kwargs={'facility_external_id': self.facility.external_id, 'pk': entry_id}) if action is not None: base_url += f"{action}/" return base_urlcare/facility/api/serializers/schedule.py (3)
71-77: Consider consolidating thevalidate_days_of_weekmethodThe
validate_days_of_weekmethod inAvailabilityCreateSerializerduplicates logic that also exists inAvailabilityUpdateSerializer. Refactoring this validation into a shared method or a mixin would reduce code redundancy and improve maintainability.
94-104: Refactor duplicatedvalidate_doctor_usernamelogicThe
validate_doctor_usernamemethod inScheduleCreateSerializeris identical to the one inScheduleExceptionCreateSerializer. It might be beneficial to extract this validation into a shared utility function or base class to avoid repetition.
105-130: Streamline resource creation logicThe
createmethod contains repetitive logic for handlingdoctor_usernameand creating aSchedulableResource, which is also present in other serializers. Consolidating this logic into a shared method could enhance code clarity and reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
care/facility/api/serializers/schedule.py(1 hunks)care/facility/api/viewsets/schedule.py(1 hunks)care/facility/models/schedule.py(1 hunks)care/facility/svc/schedule.py(1 hunks)care/facility/tests/schedule/test_schedule_api.py(1 hunks)care/facility/tests/schedule/test_schedule_exception_api.py(1 hunks)config/api_router.py(2 hunks)
🔇 Additional comments (12)
care/facility/svc/schedule.py (1)
1-10: LGTM! The imports are perfectly organized.
The imports are well-structured and necessary for the functionality. How refreshing to see proper import organization!
config/api_router.py (2)
86-89: How refreshing to see properly formatted imports!
The new imports follow the existing patterns and maintain alphabetical ordering. It's almost as if someone actually read the coding guidelines.
225-232: New routes look perfectly fine, but let's verify anyway.
The route registrations follow the established patterns and are properly nested under the facility router. The basenames are consistent with the naming convention.
Let's make absolutely sure these new routes don't conflict with any existing ones:
✅ Verification successful
The previous results weren't quite as comprehensive as one might hope. Let's get a better view of the routing context and any potential conflicts.
The previous results suggest these are new viewsets, but let's just take one final look at the implementation and routing context. Not that we don't trust the initial setup, but you know how these things can be... surprisingly interesting.
Well, someone did their homework with these routes.
After a thorough inspection (perhaps more thorough than strictly necessary), these new schedule routes are perfectly fine. They're properly nested under the facility router, have unique basenames, and the viewset implementations are quite... meticulously done, I must say. No conflicts with existing routes, and the permission classes are properly configured – not that we were worried.
- The
ScheduleViewSetandScheduleExceptionViewSetare properly implemented in their own file - The router registrations follow the established facility nested router pattern
- No conflicting routes found in the entire codebase
- Proper permission classes and filtering are in place (they really thought of everything)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential route conflicts
# Expected: No matches for similar route patterns
echo "Checking for potential route conflicts..."
rg -i "schedule.*basename.*facility" --type py
echo "Checking for similar route patterns..."
rg -i "register\(['\"]schedule" --type py
echo "Checking URL patterns..."
rg -i "path.*schedule" --type py
Length of output: 451
Script:
#!/bin/bash
# Let's do a more thorough check with context lines
echo "Checking router registrations with context..."
rg -B 5 -A 5 "register\(['\"]schedule" --type py
echo "Checking ViewSet imports and definitions..."
rg -l "class ScheduleViewSet|class ScheduleExceptionViewSet" --type py
echo "Checking all URL patterns in urls.py files..."
fd "urls\.py$" --exec rg -l "schedule" {}
Length of output: 507
Script:
#!/bin/bash
# One last check to be absolutely certain
echo "Checking the schedule viewset implementation..."
cat care/facility/api/viewsets/schedule.py
echo "Checking the entire router file for context..."
cat config/api_router.py
Length of output: 16878
care/facility/models/schedule.py (5)
13-16: Addition of SlotType choices is clear and well-defined.
The SlotType class correctly defines the slot types with appropriate integer choices, enhancing readability and maintainability.
18-20: ScheduleResourceType is appropriately set up for resource categorization.
Defining ScheduleResourceType with integer choices lays a solid foundation for future extensions of resource types.
27-34: SchedulableResource class is well-structured with generic relations.
Using GenericForeignKey allows flexibility in associating various resource types, which is a smart choice here.
36-41: Schedule model effectively links resources with scheduling details.
The fields are appropriately defined to capture the necessary scheduling information.
54-54:
Double-check the use of mutable default arguments.
Using default=list in days_of_week could lead to unexpected behavior due to the mutable default. It would be prudent to use default_factory=list instead.
Apply this diff to fix the issue:
- days_of_week = models.JSONField(default=list)
+ from django.db.models import JSONField
+ from django.contrib.postgres.fields import ArrayField
+
+ days_of_week = ArrayField(models.CharField(max_length=3), default=list)Alternatively, if using Django 3.1+, you can use default=list, but ensure that it's handling as expected.
Likely invalid or redundant comment.
care/facility/api/viewsets/schedule.py (1)
70-75: Confirm Serializer Selection for Custom Actions
In ScheduleViewSet, the get_serializer_class method includes "delete_availability" in its action check. Since delete_availability is a custom action, ensure that it correctly triggers the intended serializer.
Run the following script to confirm that the action name matches and the serializer is appropriate:
✅ Verification successful
Everything Seems Perfectly Fine Here
The delete_availability action is properly decorated and the serializer selection is actually quite sensible. The action is used to delete an availability record, and while it technically doesn't need a serializer for deletion, using ScheduleCreateSerializer is appropriate since it would have the necessary field validations for the schedule context. Not that anyone asked, but it's all properly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify action names and serializer classes in ScheduleViewSet.
# Expected: The 'delete_availability' action should be present and correctly named.
ast-grep --pattern $'@action(\n detail=$_,\n methods=$_,\n url_path=$_,\n)\ndef $FUNC($_):\n $$$' care/facility/api/viewsets/schedule.py
# Check the output to ensure 'delete_availability' is correctly referenced in get_serializer_class.
Length of output: 135
Script:
#!/bin/bash
# Let's try a different approach to find the delete_availability action
rg -A 5 "delete_availability" care/facility/api/viewsets/schedule.py
# Also check the serializer imports and class definition
rg "class Schedule.*Serializer" care/facility/api/viewsets/schedule.py
# And let's see the entire viewset class structure
rg -A 2 "class ScheduleViewSet" care/facility/api/viewsets/schedule.py
Length of output: 913
Script:
#!/bin/bash
# Let's see if delete_availability is properly decorated as an action
rg -B 2 "def delete_availability" care/facility/api/viewsets/schedule.py
# And check what the ScheduleCreateSerializer actually does
rg -A 5 "class ScheduleCreateSerializer" care/facility/api/viewsets/schedule.py
# Just to be thorough, let's see if there are any imports of these serializers
rg "from .*(Schedule.*Serializer)" care/facility/api/viewsets/schedule.py
Length of output: 391
Script:
#!/bin/bash
# One last attempt to find the action decorator details
rg -B 3 'url_path="availability' care/facility/api/viewsets/schedule.py
# And let's check the entire file content since we're missing some context
cat care/facility/api/viewsets/schedule.py
Length of output: 4056
care/facility/api/serializers/schedule.py (3)
151-157: Duplicate validation method detected
The validate_days_of_week method here is the same as previously mentioned in AvailabilityCreateSerializer. Refactoring it into a common location would promote reusability.
230-240: Repeated validate_doctor_username method
As noted earlier in ScheduleCreateSerializer, this validation logic is duplicated. Consider unifying this code to maintain consistency and simplify future updates.
28-35: Ensure super().to_representation returns expected data
The to_representation method in ScheduleResourceSerializer falls back to super().to_representation(instance) when the resource is not a User. Since ScheduleResourceSerializer is a Serializer, ensure that this fallback provides the intended serialization, or override it accordingly.
To confirm, run:
✅ Verification successful
The fallback to super().to_representation is perfectly fine
The to_representation method in ScheduleResourceSerializer falls back to super().to_representation(instance) for non-User resources, which is actually the correct behavior. Here's why:
- The code shows that
SchedulableResourceuses a generic foreign key withresource_typeandresource_idfields - The serializer specifically handles the User case, which is the only current use case as seen in
RESOURCE_TO_MODEL = { ScheduleResourceType.DOCTOR: User } - For any future resource types that might be added, they would need their own specific handling in the serializer, but for now the fallback is appropriate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the output of super().to_representation(instance) for non-User resources.
# Identify all SchedulableResource instances not linked to User models.
python -c "
from django.contrib.contenttypes.models import ContentType;
from care.facility.models.schedule import SchedulableResource;
non_user_ct = ContentType.objects.exclude(model='user');
resources = SchedulableResource.objects.filter(resource_type__in=non_user_ct);
print([r for r in resources]);
"
Length of output: 385
Script:
#!/bin/bash
# Let's examine the inheritance hierarchy and implementation details
# First, find the base class of ScheduleResourceSerializer
rg -A 5 "class ScheduleResourceSerializer"
# Then look for any other serializers that might handle non-User resources
rg "class.*Serializer.*Schedule" -A 5
# Also check for any resource-related models to understand the data structure
rg "class.*Resource.*\(models\.Model\)" -A 5
Length of output: 709
Script:
#!/bin/bash
# Let's check the model definition to understand what fields are available
rg -B 5 -A 10 "class SchedulableResource"
# And look for the base serializer implementation
fd -e py -x cat {} \; -p "serializers"
# Check for any related resource types or models
rg "resource_type = " -B 2 -A 2
Length of output: 235672
| def is_available(resource, from_datetime: datetime, to_datetime: datetime) -> bool: | ||
| msg = "Not implemented" | ||
| raise NotImplementedError(msg) |
There was a problem hiding this comment.
Oh, look who decided not to implement the availability check!
The is_available function is a critical component for checking resource availability, but it's currently just a placeholder. This could block dependent features from working correctly.
Would you like me to help implement this function? Here's what we could consider:
- Check for overlapping schedules in the given time range
- Verify against any exceptions
- Handle edge cases like timezone considerations
Let me know if you'd like me to open an issue to track this implementation.
| def get_exceptions(resource) -> QuerySet[AvailabilityException]: | ||
| return AvailabilityException.objects.filter(resource=resource) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Interesting choice to return all exceptions without any time filtering.
The function returns all exceptions for a resource, including potentially expired ones. Might want to consider adding date filtering unless there's a very good reason not to.
Consider this improvement:
def get_exceptions(resource) -> QuerySet[AvailabilityException]:
+ """Get active availability exceptions for a resource.
+
+ Args:
+ resource: The resource to get exceptions for
+
+ Returns:
+ QuerySet[AvailabilityException]: Active availability exceptions
+ """
+ now = timezone.now()
return AvailabilityException.objects.filter(
- resource=resource
+ resource=resource,
+ start_time__lte=now,
+ end_time__gte=now
)📝 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_exceptions(resource) -> QuerySet[AvailabilityException]: | |
| return AvailabilityException.objects.filter(resource=resource) | |
| def get_exceptions(resource) -> QuerySet[AvailabilityException]: | |
| """Get active availability exceptions for a resource. | |
| Args: | |
| resource: The resource to get exceptions for | |
| Returns: | |
| QuerySet[AvailabilityException]: Active availability exceptions | |
| """ | |
| now = timezone.now() | |
| return AvailabilityException.objects.filter( | |
| resource=resource, | |
| start_time__lte=now, | |
| end_time__gte=now | |
| ) |
| def test_create_schedule(self): | ||
| # test create schedule for doctor user in december month | ||
| data = { | ||
| "name": "test schedule", | ||
| "doctor_username": self.doctor_user.username, | ||
| "is_available": True, | ||
| "datetime_range": [ | ||
| "2024-12-09T10:00:00", | ||
| "2024-12-09T12:00:00", | ||
| ], | ||
| "slot_type": SlotType.APPOINTMENT, | ||
| "slot_size_in_minutes": 30, | ||
| "tokens_per_slot": 10, | ||
| } | ||
| response = self.client.post(self.get_url(), data, format="json") | ||
| self.assertEqual(response.status_code, status.HTTP_201_CREATED) | ||
|
|
||
| # check if the response is as per the spec | ||
| response_json_schema = self.get_response_schema() | ||
| self.assert_response_schema(response, response_json_schema) | ||
|
|
||
| def test_update_schedule_exception(self): | ||
| schedule_exception = ScheduleException.objects.create( | ||
| resource=self.schedulable_resource, | ||
| name="test schedule exception", | ||
| is_available=True, | ||
| datetime_range=( | ||
| datetime(2024, 12, 9, hour=10), | ||
| datetime(2024, 12, 9, hour=12), | ||
| ), | ||
| slot_size_in_minutes=30, | ||
| tokens_per_slot=10, | ||
| slot_type=SlotType.APPOINTMENT, | ||
| ) | ||
| data = { | ||
| "name": "test schedule exception 2", | ||
| "slot_size_in_minutes": 45, | ||
| } | ||
| url = self.get_url(schedule_exception.external_id) | ||
| response = self.client.patch(url, data, format="json") | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
|
|
||
| # check if the response is as per the spec | ||
| response_json_schema = self.get_response_schema() | ||
| self.assert_response_schema(response, response_json_schema) | ||
|
|
||
| # check if the availability is updated | ||
| schedule_exception.refresh_from_db() | ||
| self.assertEqual(schedule_exception.slot_size_in_minutes, 45) |
There was a problem hiding this comment.
💡 Codebase verification
Add test cases for validation and authentication scenarios
I see we have IsAuthenticated and DRYPermissions in place, yet somehow we're not testing them. How... interesting. 😊 Here are the specific test cases you should add:
-
Authentication scenarios:
- Unauthenticated access attempt
- Access with non-facility staff user
- Access with incorrect facility permissions
-
Validation scenarios:
- Invalid datetime_range (end before start)
- Missing required fields (the serializer seems quite... trusting)
- Invalid slot_size_in_minutes (negative or zero for appointment slots)
- Overlapping schedule exceptions (unless that's intentionally allowed?)
The serializer is surprisingly permissive with minimal validation, which might be fine if that's what you want, but it would be lovely to have tests confirming that's intentional.
🔗 Analysis chain
Add test cases for error scenarios and authentication.
I see we're testing the happy path, which is... nice. But what about:
- Invalid date ranges
- Overlapping schedule exceptions
- Unauthorized access attempts
- Invalid slot sizes
- Missing required fields
You know, all those pesky edge cases that users somehow always manage to find in production? 😊
Would you like me to help generate test cases for these scenarios?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other test files for schedule exceptions
# that might contain these test cases
echo "Searching for other schedule exception test files..."
fd "test.*schedule.*exception.*\.py$"
echo "Checking for existing error scenario tests..."
rg -i "test.*(invalid|error|fail|unauthorized|authentication)" \
-g "test_*schedule*.py"
Length of output: 404
Script:
#!/bin/bash
# Let's look at the actual test file content to see what we might be missing
echo "Checking the content of the test file..."
cat care/facility/tests/schedule/test_schedule_exception_api.py
# Let's also check for any validation logic in the related models/views
echo -e "\nChecking for validation logic in schedule exception related files..."
rg -A 5 "class ScheduleException"
# Let's see if there are any existing validation methods
echo -e "\nChecking for validation methods..."
rg "def (validate|clean)" -g "*schedule*exception*.py" -A 3
Length of output: 9435
Script:
#!/bin/bash
# Let's check the serializer implementation for any validation logic
echo "Checking serializer validation..."
rg -A 10 "class ScheduleExceptionCreateSerializer|class ScheduleExceptionUpdateSerializer" care/facility/api/serializers/schedule.py
# Let's also check if there are any existing test files for the serializer
echo -e "\nChecking for serializer test files..."
fd "test.*schedule.*serializer.*\.py$"
# And let's look for any authentication setup in the viewset
echo -e "\nChecking viewset authentication..."
rg -A 10 "class ScheduleExceptionViewSet" care/facility/api/viewsets/schedule.py
Length of output: 1626
care/facility/models/schedule.py
Outdated
| class ScheduleException(FacilityBaseModel, FacilityRelatedPermissionMixin): | ||
| resource = models.ForeignKey(SchedulableResource, on_delete=models.CASCADE) | ||
| name = models.CharField(max_length=255, null=False, blank=False) | ||
|
|
||
| slot_type = models.IntegerField( | ||
| choices=SlotType.choices, default=SlotType.OPEN, null=False, blank=False | ||
| ) | ||
| slot_size_in_minutes = models.IntegerField(null=False, blank=False, default=0) | ||
| tokens_per_slot = models.IntegerField(null=False, blank=False, default=0) | ||
|
|
||
| is_available = models.BooleanField(null=False, blank=False) | ||
| datetime_range = DateTimeRangeField(null=False, blank=False) |
There was a problem hiding this comment.
Missing permission methods in ScheduleException.
Unlike Availability, the ScheduleException model lacks has_object_read_permission and has_object_write_permission methods. This inconsistency might lead to unauthorized access or errors.
Consider adding the following methods to ensure consistent permission handling:
+ def has_object_read_permission(self, request):
+ return self.resource.has_object_read_permission(request)
+
+ def has_object_write_permission(self, request):
+ return self.resource.has_object_write_permission(request)📝 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.
| class ScheduleException(FacilityBaseModel, FacilityRelatedPermissionMixin): | |
| resource = models.ForeignKey(SchedulableResource, on_delete=models.CASCADE) | |
| name = models.CharField(max_length=255, null=False, blank=False) | |
| slot_type = models.IntegerField( | |
| choices=SlotType.choices, default=SlotType.OPEN, null=False, blank=False | |
| ) | |
| slot_size_in_minutes = models.IntegerField(null=False, blank=False, default=0) | |
| tokens_per_slot = models.IntegerField(null=False, blank=False, default=0) | |
| is_available = models.BooleanField(null=False, blank=False) | |
| datetime_range = DateTimeRangeField(null=False, blank=False) | |
| class ScheduleException(FacilityBaseModel, FacilityRelatedPermissionMixin): | |
| resource = models.ForeignKey(SchedulableResource, on_delete=models.CASCADE) | |
| name = models.CharField(max_length=255, null=False, blank=False) | |
| slot_type = models.IntegerField( | |
| choices=SlotType.choices, default=SlotType.OPEN, null=False, blank=False | |
| ) | |
| slot_size_in_minutes = models.IntegerField(null=False, blank=False, default=0) | |
| tokens_per_slot = models.IntegerField(null=False, blank=False, default=0) | |
| is_available = models.BooleanField(null=False, blank=False) | |
| datetime_range = DateTimeRangeField(null=False, blank=False) | |
| def has_object_read_permission(self, request): | |
| return self.resource.has_object_read_permission(request) | |
| def has_object_write_permission(self, request): | |
| return self.resource.has_object_write_permission(request) |
| def delete_availability(self, *args, availability_external_id=None, **kwargs): | ||
| try: | ||
| availability = Availability.objects.get( | ||
| external_id=availability_external_id, | ||
| schedule__external_id=kwargs["external_id"], | ||
| ) | ||
| availability.delete() | ||
| return Response(status=status.HTTP_204_NO_CONTENT) | ||
| except Availability.DoesNotExist: | ||
| return Response(status=status.HTTP_404_NOT_FOUND) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Exception Handling in delete_availability Method
The current exception handling in the delete_availability method only catches Availability.DoesNotExist, which might not differentiate between an availability that doesn't exist and one that doesn't belong to the specified schedule.
Enhance the exception handling to provide more informative responses:
def delete_availability(self, *args, availability_external_id=None, **kwargs):
try:
availability = Availability.objects.get(
external_id=availability_external_id,
schedule__external_id=kwargs["external_id"],
)
availability.delete()
return Response(status=status.HTTP_204_NO_CONTENT)
- except Availability.DoesNotExist:
+ except Availability.DoesNotExist as e:
+ if Availability.objects.filter(external_id=availability_external_id).exists():
+ return Response(
+ {"detail": "Availability does not belong to this schedule."},
+ status=status.HTTP_400_BAD_REQUEST,
+ )
return Response(status=status.HTTP_404_NOT_FOUND)This change provides clearer feedback to the client, distinguishing between a missing availability and an unauthorized deletion attempt.
📝 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 delete_availability(self, *args, availability_external_id=None, **kwargs): | |
| try: | |
| availability = Availability.objects.get( | |
| external_id=availability_external_id, | |
| schedule__external_id=kwargs["external_id"], | |
| ) | |
| availability.delete() | |
| return Response(status=status.HTTP_204_NO_CONTENT) | |
| except Availability.DoesNotExist: | |
| return Response(status=status.HTTP_404_NOT_FOUND) | |
| def delete_availability(self, *args, availability_external_id=None, **kwargs): | |
| try: | |
| availability = Availability.objects.get( | |
| external_id=availability_external_id, | |
| schedule__external_id=kwargs["external_id"], | |
| ) | |
| availability.delete() | |
| return Response(status=status.HTTP_204_NO_CONTENT) | |
| except Availability.DoesNotExist as e: | |
| if Availability.objects.filter(external_id=availability_external_id).exists(): | |
| return Response( | |
| {"detail": "Availability does not belong to this schedule."}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| return Response(status=status.HTTP_404_NOT_FOUND) |
| class ScheduleExceptionUpdateSerializer(serializers.ModelSerializer): | ||
| datetime_range = serializers.ListField(child=serializers.DateTimeField()) | ||
|
|
||
| class Meta: | ||
| model = ScheduleException | ||
| fields = ( | ||
| "name", | ||
| "is_available", | ||
| "datetime_range", | ||
| "slot_type", | ||
| "slot_size_in_minutes", | ||
| "tokens_per_slot", | ||
| ) | ||
|
|
||
| def to_representation(self, instance: ScheduleException) -> dict[str, any]: |
There was a problem hiding this comment.
Ensure datetime_range is properly converted during updates
Similar to the create serializer, the update process must convert the datetime_range list to a Range object to prevent issues when saving to the database.
Implement the conversion in the update method:
def update(self, instance, validated_data):
datetime_range_list = validated_data.pop('datetime_range', None)
if datetime_range_list:
validated_data['datetime_range'] = Range(*datetime_range_list)
return super().update(instance, validated_data)| class ScheduleCreateSerializer(serializers.ModelSerializer): | ||
| doctor_username = serializers.CharField(required=False) | ||
| availability = AvailabilityCreateSerializer(many=True) | ||
|
|
||
| class Meta: | ||
| model = Schedule | ||
| fields = ( | ||
| "doctor_username", | ||
| "name", | ||
| "valid_from", | ||
| "valid_to", | ||
| "availability", | ||
| ) | ||
|
|
There was a problem hiding this comment.
Validate that valid_from precedes valid_to
Currently, there is no check to ensure that valid_from is earlier than valid_to. This oversight might allow schedules with invalid date ranges.
Add a validate method to enforce this constraint:
def validate(self, data):
if data['valid_from'] >= data['valid_to']:
raise serializers.ValidationError("`valid_from` must be earlier than `valid_to`.")
return data| class ScheduleUpdateSerializer(serializers.ModelSerializer): | ||
| availability = AvailabilityUpdateSerializer(many=True) | ||
|
|
||
| class Meta: | ||
| model = Schedule | ||
| fields = ( | ||
| "name", | ||
| "valid_from", | ||
| "valid_to", | ||
| "availability", | ||
| ) | ||
|
|
There was a problem hiding this comment.
Add validation for date range consistency
As with the create serializer, ensure that in ScheduleUpdateSerializer, valid_from is earlier than valid_to to maintain valid schedule durations.
Include the validate method:
def validate(self, data):
if data['valid_from'] >= data['valid_to']:
raise serializers.ValidationError("`valid_from` must be earlier than `valid_to`.")
return data| def update(self, instance, validated_data): | ||
| availability_data = validated_data.pop("availability", []) | ||
| schedule = super().update(instance, validated_data) | ||
|
|
||
| for availability in availability_data: | ||
| if external_id := availability.pop("external_id", None): | ||
| Availability.objects.filter( | ||
| external_id=external_id, schedule=schedule | ||
| ).update(**availability) | ||
| else: | ||
| availability["schedule"] = schedule | ||
| Availability.objects.create(**availability) | ||
|
|
||
| return schedule |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap update operations in a transaction
The update method performs multiple database operations which could lead to partial updates if an error occurs. Wrapping these operations in a transaction ensures atomicity.
Implement transaction management:
from django.db import transaction
def update(self, instance, validated_data):
availability_data = validated_data.pop("availability", [])
with transaction.atomic():
schedule = super().update(instance, validated_data)
for availability in availability_data:
# Existing logic
return schedule| def create(self, validated_data: dict) -> Schedule: | ||
| if doctor_username := validated_data.pop("doctor_username", None): | ||
| facility = Facility.objects.get( | ||
| external_id=self.context["request"].parser_context["kwargs"][ | ||
| "facility_external_id" | ||
| ] | ||
| ) | ||
| doctor = FacilityUser.objects.get( | ||
| user__username=doctor_username, facility=facility | ||
| ).user | ||
|
|
||
| user_content_type = ContentType.objects.get_for_model(doctor) | ||
| resource, _ = SchedulableResource.objects.get_or_create( | ||
| facility=facility, | ||
| resource_id=doctor.id, | ||
| resource_type=user_content_type, | ||
| ) | ||
| validated_data["resource"] = resource | ||
|
|
||
| availability_data = validated_data.pop("availability") | ||
| schedule = super().create(validated_data) | ||
| for availability in availability_data: | ||
| availability["schedule"] = schedule | ||
| Availability.objects.create(**availability) | ||
| return schedule | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider transaction management in creation
Similarly, the create method could benefit from being wrapped in a transaction to ensure all related objects are created atomically.
Update the create method:
from django.db import transaction
def create(self, validated_data):
with transaction.atomic():
# Existing creation logic
return schedule
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
care/facility/models/schedule.py (4)
26-28: Perhaps consider a more... extensive resource mapping?The
RESOURCE_TO_MODELdictionary currently only mapsDOCTORtoUser. While this might work for now, it seems a bit... limiting. Future resource types would require code changes.Consider:
- Adding a comment explaining why only DOCTOR is mapped
- Or adding other potential resource types (nurses, equipment, rooms) if they're planned
RESOURCE_TO_MODEL = { ScheduleResourceType.DOCTOR: User, # Future resource types: # ScheduleResourceType.NURSE: Staff, # ScheduleResourceType.ROOM: Room, }
31-37: Your queries might appreciate some indexesThe
SchedulableResourcemodel usesGenericForeignKeywhich typically benefits from database indexes, but I couldn't help but notice they're missing.Consider adding indexes to improve query performance:
class SchedulableResource(FacilityBaseModel): facility = models.ForeignKey( "Facility", on_delete=models.CASCADE, null=False, blank=False ) - resource_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) - resource_id = models.PositiveIntegerField() + resource_type = models.ForeignKey(ContentType, on_delete=models.CASCADE, db_index=True) + resource_id = models.PositiveIntegerField(db_index=True) resource = GenericForeignKey("resource_type", "resource_id")
40-47: A tiny performance enhancement opportunityThe
filter_by_resourcemethod gets ContentType on every call. Not that I'm counting, but that's one database query per call that could be avoided.Consider caching the ContentType lookup:
class ScheduleBaseManager(models.Manager): + def get_content_type(self, model_class): + if not hasattr(self, '_content_type_cache'): + self._content_type_cache = {} + if model_class not in self._content_type_cache: + self._content_type_cache[model_class] = ContentType.objects.get_for_model(model_class) + return self._content_type_cache[model_class] + def filter_by_resource(self, resource): return self.filter( resource__resource_id=resource.id, - resource__resource_type=ContentType.objects.get_for_model( - resource.__class__ - ), + resource__resource_type=self.get_content_type(resource.__class__), )
81-92: Time ranges could use some boundariesThe
datetime_rangefield lacks validation for reasonable time ranges. We probably don't want exceptions scheduled for the year 2525.Consider adding validation for reasonable date ranges:
class ScheduleException(FacilityBaseModel, FacilityRelatedPermissionMixin): + MAX_EXCEPTION_DAYS = 30 # or whatever makes sense for your use case + # ... existing fields ... + + def clean(self): + super().clean() + if self.datetime_range: + duration = self.datetime_range.upper - self.datetime_range.lower + if duration.days > self.MAX_EXCEPTION_DAYS: + raise ValidationError({ + 'datetime_range': f'Exception duration cannot exceed {self.MAX_EXCEPTION_DAYS} days' + })care/facility/models/patient_base.py (1)
20-21: Remove the extra blank line... if it's not too much trouble.There are two consecutive blank lines where one would suffice.
def reverse_choices_with_label(choices): output = {} for choice in choices: output[choice[1].lower()] = choice[0] return output - SYMPTOM_CHOICES = [care/facility/migrations/0466_schedulableresource_schedule_availability_and_more.py (7)
13-15: Consistent Use of Double Quotes in DependenciesI noticed that single quotes are used in the
dependenciessection. To align with the project's coding standards, please replace single quotes with double quotes.Apply this diff to correct the quotes:
13- ('contenttypes', '0002_remove_content_type_name'), 14- ('facility', '0465_merge_20240923_1045'), + ("contenttypes", "0002_remove_content_type_name"), + ("facility", "0465_merge_20240923_1045"),🧰 Tools
🪛 Ruff
13-13: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
13-13: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-33: String Quoting Style inSchedulableResourceModel DefinitionSingle quotes are used in the model definition. For consistency with the preferred style, please use double quotes.
Here's the suggested change:
19- migrations.CreateModel( 20- name='SchedulableResource', + migrations.CreateModel( + name="SchedulableResource", ... 21- ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), ... 28- ('resource_type', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='contenttypes.contenttype')), + ("resource_type", models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="contenttypes.contenttype")),🧰 Tools
🪛 Ruff
19-19: 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)
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)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
31-31: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
35-51: Consistent Quoting inScheduleModel DefinitionSimilarly, the
Schedulemodel uses single quotes. Replacing them with double quotes will maintain code consistency.Apply this diff:
35- migrations.CreateModel( 36- name='Schedule', + migrations.CreateModel( + name="Schedule", ... 37- ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), ... 45- ('resource', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='facility.schedulableresource')), + ("resource", models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="facility.schedulableresource")),🧰 Tools
🪛 Ruff
35-35: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
37-37: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
37-37: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
38-38: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
39-39: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
40-40: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
41-41: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
42-42: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
43-43: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
44-44: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
53-73: Quoting Style inAvailabilityModel DefinitionThe
Availabilitymodel also contains single quotes. Please update them to double quotes for consistency.Suggested changes:
53- migrations.CreateModel( 54- name='Availability', + migrations.CreateModel( + name="Availability", ... 61- ('slot_type', models.IntegerField(choices=[(1, 'Open'), (2, 'Appointment')], default=1)), + ("slot_type", models.IntegerField(choices=[(1, "Open"), (2, "Appointment")], default=1)), ... 67- ('schedule', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='facility.schedule')), + ("schedule", models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="facility.schedule")),🧰 Tools
🪛 Ruff
53-53: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
55-55: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
55-55: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
56-56: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
57-57: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
58-58: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
59-59: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
60-60: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
61-61: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
61-61: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
61-61: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
62-62: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
63-63: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
64-64: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
65-65: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
66-66: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
67-67: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
67-67: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
70-70: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
75-94: Uniform Use of Double Quotes inScheduleExceptionModelTo keep the codebase consistent, please replace single quotes with double quotes in the
ScheduleExceptionmodel.Here's the diff:
75- migrations.CreateModel( 76- name='ScheduleException', + migrations.CreateModel( + name="ScheduleException", ... 83- ('slot_type', models.IntegerField(choices=[(1, 'Open'), (2, 'Appointment')], default=1)), + ("slot_type", models.IntegerField(choices=[(1, "Open"), (2, "Appointment")], default=1)), ... 88- ('resource', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='facility.schedulableresource')), + ("resource", models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="facility.schedulableresource")),🧰 Tools
🪛 Ruff
75-75: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
77-77: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
77-77: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
78-78: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
79-79: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
80-80: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
81-81: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
82-82: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
83-83: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
83-83: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
83-83: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
84-84: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
85-85: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
86-86: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
87-87: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
88-88: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
88-88: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
91-91: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
43-44: Consider Allowing Null Values forvalid_toField inScheduleModelCurrently, the
valid_tofield is mandatory. If there are schedules that don't have an end date, allowing null values might be beneficial.You might want to update the field definition:
44- ('valid_to', models.DateTimeField()), + ("valid_to", models.DateTimeField(null=True, blank=True)),This change permits schedules to have an indefinite validity period.
🧰 Tools
🪛 Ruff
43-43: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
44-44: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
86-86: Add Default Value tois_availableField inScheduleExceptionModelThe
is_availablefield doesn't have a default value. To prevent potential issues during record creation, consider adding a default value.Update the field as follows:
86- ('is_available', models.BooleanField()), + ("is_available", models.BooleanField(default=True)),🧰 Tools
🪛 Ruff
86-86: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
care/facility/api/serializers/schedule.py(1 hunks)care/facility/api/viewsets/schedule.py(1 hunks)care/facility/migrations/0466_schedulableresource_schedule_availability_and_more.py(1 hunks)care/facility/migrations/0468_merge_20241119_0746.py(1 hunks)care/facility/models/patient_base.py(1 hunks)care/facility/models/schedule.py(1 hunks)care/facility/tests/schedule/test_schedule_api.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- care/facility/api/serializers/schedule.py
- care/facility/api/viewsets/schedule.py
- care/facility/tests/schedule/test_schedule_api.py
🧰 Additional context used
🪛 Ruff
care/facility/migrations/0466_schedulableresource_schedule_availability_and_more.py
13-13: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
13-13: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: 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)
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)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
31-31: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
35-35: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
37-37: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
37-37: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
38-38: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
39-39: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
40-40: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
41-41: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
42-42: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
43-43: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
44-44: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
53-53: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
55-55: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
55-55: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
56-56: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
57-57: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
58-58: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
59-59: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
60-60: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
61-61: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
61-61: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
61-61: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
62-62: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
63-63: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
64-64: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
65-65: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
66-66: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
67-67: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
67-67: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
70-70: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
75-75: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
77-77: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
77-77: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
78-78: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
79-79: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
80-80: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
81-81: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
82-82: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
83-83: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
83-83: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
83-83: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
84-84: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
85-85: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
86-86: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
87-87: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
88-88: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
88-88: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
91-91: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/facility/migrations/0468_merge_20241119_0746.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🔇 Additional comments (2)
care/facility/models/patient_base.py (1)
15-20: Verify the usage of this new function across the codebase.
Since this function provides an alternative way to reverse choices, we should check where and how it's being used.
✅ Verification successful
Oh, look who's using a new choices reversal function... just schedule.py
The function is currently only used in schedule.py to create REVERSE_SLOT_TYPE, while the original reverse_choices is still being used across multiple files. The new function differs by using the choice label (lowercased) as the key instead of the choice value, which might be what you wanted... or not.
- The function is imported and used exactly once in
schedule.py - It serves a different purpose than the original
reverse_choicesfunction - Other files continue to use the original
reverse_choicesfunction, as they should
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the new function
echo "Searching for direct usage of reverse_choices_with_label:"
rg "reverse_choices_with_label" -l
echo -e "\nSearching for potential places where it could/should be used:"
rg "reverse_choices\(" -l
Length of output: 737
Script:
#!/bin/bash
# Let's see how reverse_choices_with_label is actually used in both files
echo "=== Usage in schedule.py ==="
rg "reverse_choices_with_label" care/facility/models/schedule.py -A 3 -B 3
echo -e "\n=== Usage in patient_base.py ==="
rg "reverse_choices_with_label" care/facility/models/patient_base.py -A 3 -B 3
# Let's also check the original reverse_choices implementation
echo -e "\n=== Original reverse_choices implementation ==="
rg "def reverse_choices" care/facility/models/patient_base.py -A 3 -B 3
Length of output: 1323
care/facility/migrations/0466_schedulableresource_schedule_availability_and_more.py (1)
64-64:
Validate the Default Value for days_of_week Field
Using default=list for the days_of_week field could lead to all instances sharing the same list. To ensure each instance has its own list, it's better to use default=list as a callable.
Modify the field definition:
64- ('days_of_week', models.JSONField(default=list)),
+ ("days_of_week", models.JSONField(default=list)),Wait, the field already uses default=list. Hmm, perhaps I misread that. Disregard this if default=list is correctly used.
🧰 Tools
🪛 Ruff
64-64: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
| class Schedule(FacilityBaseModel, FacilityRelatedPermissionMixin): | ||
| resource = models.ForeignKey(SchedulableResource, on_delete=models.CASCADE) | ||
| name = models.CharField(max_length=255, null=False, blank=False) | ||
| valid_from = models.DateTimeField(null=False, blank=False) | ||
| valid_to = models.DateTimeField(null=False, blank=False) | ||
|
|
||
| objects = ScheduleBaseManager() |
There was a problem hiding this comment.
Time travel not supported (yet)
The Schedule model doesn't validate that valid_from is before valid_to. I assume we're not building a time machine?
Add validation to ensure temporal sanity:
class Schedule(FacilityBaseModel, FacilityRelatedPermissionMixin):
resource = models.ForeignKey(SchedulableResource, on_delete=models.CASCADE)
name = models.CharField(max_length=255, null=False, blank=False)
valid_from = models.DateTimeField(null=False, blank=False)
valid_to = models.DateTimeField(null=False, blank=False)
objects = ScheduleBaseManager()
+
+ def clean(self):
+ super().clean()
+ if self.valid_from and self.valid_to and self.valid_from >= self.valid_to:
+ raise ValidationError({
+ 'valid_to': 'End time must be after start time'
+ })📝 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.
| class Schedule(FacilityBaseModel, FacilityRelatedPermissionMixin): | |
| resource = models.ForeignKey(SchedulableResource, on_delete=models.CASCADE) | |
| name = models.CharField(max_length=255, null=False, blank=False) | |
| valid_from = models.DateTimeField(null=False, blank=False) | |
| valid_to = models.DateTimeField(null=False, blank=False) | |
| objects = ScheduleBaseManager() | |
| class Schedule(FacilityBaseModel, FacilityRelatedPermissionMixin): | |
| resource = models.ForeignKey(SchedulableResource, on_delete=models.CASCADE) | |
| name = models.CharField(max_length=255, null=False, blank=False) | |
| valid_from = models.DateTimeField(null=False, blank=False) | |
| valid_to = models.DateTimeField(null=False, blank=False) | |
| objects = ScheduleBaseManager() | |
| def clean(self): | |
| super().clean() | |
| if self.valid_from and self.valid_to and self.valid_from >= self.valid_to: | |
| raise ValidationError({ | |
| 'valid_to': 'End time must be after start time' | |
| }) |
care/facility/models/schedule.py
Outdated
| class Availability(FacilityBaseModel, FacilityRelatedPermissionMixin): | ||
| schedule = models.ForeignKey( | ||
| "Schedule", on_delete=models.CASCADE, null=False, blank=False | ||
| ) | ||
| name = models.CharField(max_length=255, null=False, blank=False) | ||
| slot_type = models.IntegerField( | ||
| choices=SlotType.choices, default=SlotType.OPEN, null=False, blank=False | ||
| ) | ||
| slot_size_in_minutes = models.IntegerField(null=False, blank=False, default=0) | ||
| tokens_per_slot = models.IntegerField(null=False, blank=False, default=0) | ||
|
|
||
| days_of_week = models.JSONField(default=list) | ||
| start_time = models.TimeField(null=False, blank=False) | ||
| end_time = models.TimeField(null=False, blank=False) | ||
|
|
||
| def has_object_read_permission(self, request): | ||
| return self.schedule.has_object_read_permission(request) | ||
|
|
||
| def has_object_write_permission(self, request): | ||
| return self.schedule.has_object_write_permission(request) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apparently any day can be a Monday
The days_of_week JSONField accepts any value without validation. Not that anyone would intentionally put invalid days, but...
Add validation for the days_of_week field:
class Availability(FacilityBaseModel, FacilityRelatedPermissionMixin):
+ VALID_DAYS = range(0, 7) # 0 = Monday, 6 = Sunday
+
schedule = models.ForeignKey(
"Schedule", on_delete=models.CASCADE, null=False, blank=False
)
# ... other fields ...
days_of_week = models.JSONField(default=list)
+
+ def clean(self):
+ super().clean()
+ if not all(day in self.VALID_DAYS for day in self.days_of_week):
+ raise ValidationError({
+ 'days_of_week': 'Invalid day value. Must be between 0 (Monday) and 6 (Sunday)'
+ })📝 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.
| class Availability(FacilityBaseModel, FacilityRelatedPermissionMixin): | |
| schedule = models.ForeignKey( | |
| "Schedule", on_delete=models.CASCADE, null=False, blank=False | |
| ) | |
| name = models.CharField(max_length=255, null=False, blank=False) | |
| slot_type = models.IntegerField( | |
| choices=SlotType.choices, default=SlotType.OPEN, null=False, blank=False | |
| ) | |
| slot_size_in_minutes = models.IntegerField(null=False, blank=False, default=0) | |
| tokens_per_slot = models.IntegerField(null=False, blank=False, default=0) | |
| days_of_week = models.JSONField(default=list) | |
| start_time = models.TimeField(null=False, blank=False) | |
| end_time = models.TimeField(null=False, blank=False) | |
| def has_object_read_permission(self, request): | |
| return self.schedule.has_object_read_permission(request) | |
| def has_object_write_permission(self, request): | |
| return self.schedule.has_object_write_permission(request) | |
| class Availability(FacilityBaseModel, FacilityRelatedPermissionMixin): | |
| VALID_DAYS = range(0, 7) # 0 = Monday, 6 = Sunday | |
| schedule = models.ForeignKey( | |
| "Schedule", on_delete=models.CASCADE, null=False, blank=False | |
| ) | |
| name = models.CharField(max_length=255, null=False, blank=False) | |
| slot_type = models.IntegerField( | |
| choices=SlotType.choices, default=SlotType.OPEN, null=False, blank=False | |
| ) | |
| slot_size_in_minutes = models.IntegerField(null=False, blank=False, default=0) | |
| tokens_per_slot = models.IntegerField(null=False, blank=False, default=0) | |
| days_of_week = models.JSONField(default=list) | |
| start_time = models.TimeField(null=False, blank=False) | |
| end_time = models.TimeField(null=False, blank=False) | |
| def clean(self): | |
| super().clean() | |
| if not all(day in self.VALID_DAYS for day in self.days_of_week): | |
| raise ValidationError({ | |
| 'days_of_week': 'Invalid day value. Must be between 0 (Monday) and 6 (Sunday)' | |
| }) | |
| def has_object_read_permission(self, request): | |
| return self.schedule.has_object_read_permission(request) | |
| def has_object_write_permission(self, request): | |
| return self.schedule.has_object_write_permission(request) |
care/facility/models/patient_base.py
Outdated
| def reverse_choices_with_label(choices): | ||
| output = {} | ||
| for choice in choices: | ||
| output[choice[1].lower()] = choice[0] | ||
| return output | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
The new function could use some improvements... if you don't mind.
While the function works for basic cases, there are a few concerns that probably should be addressed:
- Potential key collisions if different labels map to the same lowercase string
- No handling of None/null values
- Inconsistent with Django's choice convention of using the first element as the key
Here's a slightly more robust implementation:
def reverse_choices_with_label(choices):
+ """
+ Creates a mapping of lowercase choice labels to their values.
+
+ Args:
+ choices: List of tuples (value, label)
+
+ Returns:
+ Dict mapping lowercase labels to their corresponding values
+
+ Raises:
+ ValueError: If multiple labels map to the same lowercase string
+ """
output = {}
for choice in choices:
+ if choice[1] is None:
+ continue
+ key = choice[1].lower()
+ if key in output:
+ raise ValueError(f"Multiple labels map to '{key}'")
output[choice[1].lower()] = choice[0]
return output📝 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 reverse_choices_with_label(choices): | |
| output = {} | |
| for choice in choices: | |
| output[choice[1].lower()] = choice[0] | |
| return output | |
| def reverse_choices_with_label(choices): | |
| """ | |
| Creates a mapping of lowercase choice labels to their values. | |
| Args: | |
| choices: List of tuples (value, label) | |
| Returns: | |
| Dict mapping lowercase labels to their corresponding values | |
| Raises: | |
| ValueError: If multiple labels map to the same lowercase string | |
| """ | |
| output = {} | |
| for choice in choices: | |
| if choice[1] is None: | |
| continue | |
| key = choice[1].lower() | |
| if key in output: | |
| raise ValueError(f"Multiple labels map to '{key}'") | |
| output[choice[1].lower()] = choice[0] | |
| return output |
| options={ | ||
| 'abstract': False, | ||
| }, | ||
| bases=(models.Model, care.facility.models.mixins.permissions.facility.FacilityRelatedPermissionMixin), |
There was a problem hiding this comment.
Avoid Specifying bases in Migration Model Definitions
Including the bases argument in migration model definitions is unconventional and might lead to migration issues. Django handles model inheritance, and specifying bases in migrations can cause inconsistencies.
Consider removing the bases argument from the model definitions in the migration file.
Suggested change:
50- bases=(models.Model, care.facility.models.mixins.permissions.facility.FacilityRelatedPermissionMixin),
+ # Removed 'bases' argument to allow Django to manage inheritance
72- bases=(models.Model, care.facility.models.mixins.permissions.facility.FacilityRelatedPermissionMixin),
+ # Removed 'bases' argument to allow Django to manage inheritance
93- bases=(models.Model, care.facility.models.mixins.permissions.facility.FacilityRelatedPermissionMixin),
+ # Removed 'bases' argument to allow Django to manage inheritanceAlso applies to: 72-72, 93-93
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
care/facility/tests/schedule/test_schedule_api.py (5)
20-21: A docstring would be lovely hereWhile the code is self-documenting (sort of), a class docstring explaining the test scenarios would make it even better. You know, for those days when we're not feeling particularly psychic about the test coverage.
170-171: Those assertions could use a bit more... personalityAdding assertion messages would make test failures more informative. Not that your code would ever fail, but you know... just in case.
- self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertEqual(response.status_code, status.HTTP_201_CREATED, "Schedule creation failed")
176-194: Might want to verify that deletion actually happenedThe status code check is nice, but wouldn't it be even nicer to verify that the availability actually doesn't exist anymore? Just a thought...
self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + self.assertFalse( + Availability.objects.filter(external_id=new_availability.external_id).exists(), + "Availability should not exist after deletion" + )
198-200: Those variable names could be a tad more... descriptive
existing_availability_external_idsis quite a mouthful. Maybe we could make it more... concise?- existing_availability_external_ids = list( + existing_ids = list( self.schedule.availability_set.all().values_list("external_id", flat=True) )
239-257: Maybe we could test a few more filter combinations?The current test covers the happy path, but what about testing:
- Invalid date ranges
- Non-existent doctor username
- Missing filter parameters
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
care/facility/api/serializers/schedule.py(1 hunks)care/facility/migrations/0469_remove_scheduleexception_datetime_range_and_more.py(1 hunks)care/facility/models/patient_base.py(1 hunks)care/facility/models/schedule.py(1 hunks)care/facility/tests/schedule/test_schedule_api.py(1 hunks)care/facility/tests/schedule/test_schedule_exception_api.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- care/facility/api/serializers/schedule.py
- care/facility/models/patient_base.py
- care/facility/models/schedule.py
- care/facility/tests/schedule/test_schedule_exception_api.py
🧰 Additional context used
🪛 Ruff
care/facility/migrations/0469_remove_scheduleexception_datetime_range_and_more.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
18-18: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
19-19: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
30-30: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
34-34: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
35-35: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
36-36: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
40-40: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
41-41: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
42-42: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
46-46: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
47-47: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
52-52: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
53-53: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
54-54: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
54-54: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
54-54: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
57-57: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
58-58: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
59-59: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
59-59: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
59-59: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🔇 Additional comments (3)
care/facility/migrations/0469_remove_scheduleexception_datetime_range_and_more.py (2)
13-16:
Verify data migration strategy for removed datetime_range field.
I couldn't help but notice that we're removing the datetime_range field without any apparent data migration strategy. It would be absolutely wonderful if you could clarify how existing data will be handled.
🧰 Tools
🪛 Ruff
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
51-60: Consider data migration for slot_type changes.
The slot type changes look mostly fine, but I couldn't help wondering about existing records. While the default of 1 ('Open') is reasonable, existing records might need a more nuanced migration strategy.
🧰 Tools
🪛 Ruff
52-52: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
53-53: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
54-54: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
54-54: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
54-54: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
57-57: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
58-58: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
59-59: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
59-59: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
59-59: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/facility/tests/schedule/test_schedule_api.py (1)
5-5:
Oh, interesting choice importing Response from 'requests'
I see you're importing Response from the requests library, but since we're in Django REST Framework land, we should probably use rest_framework.response.Response instead. Just saying... 😊
-from requests import Response
+from rest_framework.response import ResponseLikely invalid or redundant comment.
| model_name='scheduleexception', | ||
| name='end_time', | ||
| field=models.TimeField(default='00:00'), | ||
| preserve_default=False, | ||
| ), | ||
| migrations.AddField( | ||
| model_name='scheduleexception', | ||
| name='reason', | ||
| field=models.TextField(blank=True, null=True), | ||
| ), | ||
| migrations.AddField( | ||
| model_name='scheduleexception', | ||
| name='start_time', | ||
| field=models.TimeField(default='00:00'), | ||
| preserve_default=False, | ||
| ), |
There was a problem hiding this comment.
Reconsider default time values and add data migration.
Oh, those default values of '00:00' for time fields are... interesting. While they're removed with preserve_default=False, it might be worth considering:
- Adding a proper data migration strategy
- Using more meaningful default times for existing records
Would you mind adding a data migration to handle existing records more gracefully? Just a thought. 😊
🧰 Tools
🪛 Ruff
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
30-30: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
34-34: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
35-35: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
36-36: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
| migrations.AddField( | ||
| model_name='scheduleexception', | ||
| name='valid_from', | ||
| field=models.DateField(default='2024-11-01'), | ||
| preserve_default=False, | ||
| ), | ||
| migrations.AddField( | ||
| model_name='scheduleexception', | ||
| name='valid_to', | ||
| field=models.DateField(default='2024-11-01'), | ||
| preserve_default=False, | ||
| ), |
There was a problem hiding this comment.
Avoid hardcoding future dates in migrations.
I see we're using '2024-11-01' as a default date. While I'm sure there's a perfectly good reason for this specific date, it might be worth considering:
- Using a dynamic date (e.g., timezone.now())
- Adding a data migration instead of hardcoding dates
The current approach might cause interesting surprises when this migration runs in, say, 2025. Just saying.
🧰 Tools
🪛 Ruff
40-40: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
41-41: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
42-42: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
46-46: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
47-47: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
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
New Features
Bug Fixes
Tests
Documentation