Refactor Scheduling Logic to Service Layer#3456
Refactor Scheduling Logic to Service Layer#3456MohamadJalal-1085 wants to merge 2 commits intoohcnetwork:developfrom
Conversation
Refactor availability logic
📝 WalkthroughWalkthroughThe PR introduces a slot and booking management system for appointment scheduling with data models, endpoints, and helper functions for fetching available slots, creating appointments, and computing availability statistics. Concurrently, viewset logic is refactored into a dedicated AvailabilityService for improved separation of concerns. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @availability.py:
- Around line 64-72: The model_validator method validate_period (mode="after")
performs checks but does not return the model; pydantic requires an explicit
return of the model instance for after-validators. Modify validate_period to
return self at the end (after performing the from_date/to_date checks and
potential ValidationError raises) so the validated model is returned properly.
- Around line 296-311: The code checks TokenBooking counts using the variable
patient before ensuring Patient.objects.filter(...).first() actually returned a
Patient; move the existence check for patient (the result of
Patient.objects.filter(...).first()) immediately after that lookup and raise
ValidationError("Patient not found") if falsy, then proceed into the
transaction.atomic block (or keep the transaction and perform the check inside)
to run the TokenBooking query and the MAX_APPOINTMENTS_PER_PATIENT comparison;
update references to TokenBooking.objects.filter(patient=patient,
token_slot__start_datetime__gte=care_now()) and the ValidationError about
maximum appointments to occur only after patient existence is confirmed.
- Around line 446-451: The type hint for calculate_slots is incorrect:
availabilities is annotated as list[Availability] but callers pass dict values
(QuerySet-like iterables via .values()), causing a mismatch; update the function
signature for calculate_slots to accept the actual structure (e.g.,
availabilities: dict[int, QuerySet] or availabilities: Iterable[QuerySet | dict]
/ Mapping[int, QuerySet]) and adjust any internal usage assuming list methods
accordingly so the parameter type reflects what's passed from the callers that
use .values().
In @care/emr/api/viewsets/scheduling/availability.py:
- Around line 24-28: The code passes request.data (a dict) to
AvailabilityService.get_available_slots which expects an object with attributes
(it accesses request_data.resource_type, resource_id); parse request.data into
the pydantic model SlotsForDayRequestSpec (e.g., instantiate/parse it from
request.data) and pass that model instance to
AvailabilityService.get_available_slots (or alternatively change the service to
accept a dict), and handle pydantic ValidationError by returning a 400 Response
with validation details so invalid input is reported gracefully.
In @care/emr/services/scheduling_service.py:
- Around line 8-24: get_available_slots currently treats request_data as an
object (using request_data.resource_type and request_data.resource_id) but
callers pass Django REST Framework request.data (a dict), causing
AttributeError; modify get_available_slots to accept and handle a dict by
reading keys with dict access (e.g., request_data["resource_type"],
request_data["resource_id"]) and validate missing keys (raise ValidationError)
before calling get_schedulable_resource, or alternatively update caller to
convert request.data to a validated object (pydantic/dataclass) and document
which approach is chosen; ensure the change touches the get_available_slots
signature/validation logic and uses the same key names expected by
get_schedulable_resource.
🧹 Nitpick comments (5)
care/emr/services/scheduling_service.py (2)
2-2: Unused import:AvailabilityThe
Availabilitymodel is imported but never used in this file. Might want to clean that up... unless you're planning to use it later, of course.🧹 Proposed fix
from rest_framework.exceptions import ValidationError -from care.emr.models.scheduling.schedule import Availability from care.emr.api.viewsets.scheduling.schedule import get_schedulable_resource
5-5: Remove development comment before merge.The comment referencing "your 'After' photo" appears to be a leftover from development/documentation. It doesn't add value for future readers of this code.
🧹 Proposed fix
-# This matches the top half of your "After" photo class AvailabilityService:care/emr/api/viewsets/scheduling/availability.py (2)
10-11: Remove development comment.This comment about "linking the two files together" is helpful during development but doesn't belong in production code.
🧹 Proposed fix
-# This import links the two files together from care.emr.services.scheduling_service import AvailabilityService
30-31: Consider using DRF's status constants.Using
status=400works, but DRF convention prefersstatus=status.HTTP_400_BAD_REQUESTfor clarity. Just a thought.♻️ Proposed fix
+from rest_framework import status + # ... except ValidationError as e: - return Response({"error": str(e)}, status=400) + return Response({"error": str(e)}, status=status.HTTP_400_BAD_REQUEST)availability.py (1)
200-205: TODO comment noted for authorization.The TODO at line 202 indicates authorization needs confirmation. Just making sure this is on someone's radar before this ships.
Would you like me to open an issue to track the authorization implementation for
get_slots_for_day?
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
availability.pycare/emr/api/viewsets/scheduling/availability.pycare/emr/services/scheduling_service.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/api/viewsets/scheduling/availability.pycare/emr/services/scheduling_service.pyavailability.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: rithviknishad
Repo: ohcnetwork/care PR: 3302
File: care/emr/resources/scheduling/slot/spec.py:156-161
Timestamp: 2025-10-08T08:07:56.493Z
Learning: In `care/emr/resources/scheduling/slot/spec.py`, token bookings must always have an associated patient. The maintainers prefer fail-fast behavior (allowing exceptions) over graceful degradation when a token booking lacks a patient, as it indicates a data integrity violation that should be caught immediately.
Learnt from: rithviknishad
Repo: ohcnetwork/care PR: 3302
File: care/emr/resources/scheduling/slot/spec.py:146-151
Timestamp: 2025-10-08T08:08:44.661Z
Learning: In care/emr/resources/scheduling/slot/spec.py and related token booking code, TokenBooking instances always have a patient associated. The codebase prefers a fail-fast approach where missing patients should cause errors rather than being handled gracefully with optional/nullable fields.
📚 Learning: 2025-10-08T08:07:56.493Z
Learnt from: rithviknishad
Repo: ohcnetwork/care PR: 3302
File: care/emr/resources/scheduling/slot/spec.py:156-161
Timestamp: 2025-10-08T08:07:56.493Z
Learning: In `care/emr/resources/scheduling/slot/spec.py`, token bookings must always have an associated patient. The maintainers prefer fail-fast behavior (allowing exceptions) over graceful degradation when a token booking lacks a patient, as it indicates a data integrity violation that should be caught immediately.
Applied to files:
care/emr/api/viewsets/scheduling/availability.pyavailability.py
📚 Learning: 2025-10-08T08:08:44.661Z
Learnt from: rithviknishad
Repo: ohcnetwork/care PR: 3302
File: care/emr/resources/scheduling/slot/spec.py:146-151
Timestamp: 2025-10-08T08:08:44.661Z
Learning: In care/emr/resources/scheduling/slot/spec.py and related token booking code, TokenBooking instances always have a patient associated. The codebase prefers a fail-fast approach where missing patients should cause errors rather than being handled gracefully with optional/nullable fields.
Applied to files:
care/emr/api/viewsets/scheduling/availability.pyavailability.py
🧬 Code graph analysis (3)
care/emr/api/viewsets/scheduling/availability.py (3)
care/utils/shortcuts.py (1)
get_object_or_404(6-15)care/emr/api/viewsets/base.py (1)
EMRRetrieveMixin(73-88)care/emr/services/scheduling_service.py (2)
AvailabilityService(6-24)get_available_slots(8-24)
care/emr/services/scheduling_service.py (2)
care/emr/models/scheduling/schedule.py (1)
Availability(60-68)care/emr/api/viewsets/scheduling/schedule.py (1)
get_schedulable_resource(88-110)
availability.py (9)
care/emr/api/viewsets/scheduling/schedule.py (1)
get_schedulable_resource(88-110)care/emr/models/scheduling/schedule.py (2)
Schedule(38-57)Availability(60-68)care/emr/models/scheduling/booking.py (2)
TokenBooking(22-50)TokenSlot(9-19)care/emr/resources/scheduling/schedule/spec.py (2)
SchedulableResourceTypeOptions(24-27)SlotTypeOptions(18-21)care/emr/tagging/base.py (1)
SingleFacilityTagManager(40-105)care/emr/api/viewsets/invoice.py (1)
lock(310-325)care/utils/lock.py (1)
Lock(12-30)care/utils/time_util.py (1)
care_now(4-5)care/emr/resources/base.py (1)
to_json(158-159)
🪛 Ruff (0.14.10)
care/emr/api/viewsets/scheduling/availability.py
20-20: Unused method argument: args
(ARG002)
20-20: Unused method argument: kwargs
(ARG002)
care/emr/services/scheduling_service.py
19-19: Avoid specifying long messages outside the exception class
(TRY003)
availability.py
68-68: Avoid specifying long messages outside the exception class
(TRY003)
129-129: Avoid specifying long messages outside the exception class
(TRY003)
131-131: Avoid specifying long messages outside the exception class
(TRY003)
137-137: Avoid specifying long messages outside the exception class
(TRY003)
201-201: Unused method argument: args
(ARG002)
201-201: Unused method argument: kwargs
(ARG002)
217-217: Avoid specifying long messages outside the exception class
(TRY003)
311-311: Avoid specifying long messages outside the exception class
(TRY003)
324-324: Unused method argument: request_obj
(ARG002)
328-328: Avoid specifying long messages outside the exception class
(TRY003)
331-331: Unused method argument: args
(ARG002)
331-331: Unused method argument: kwargs
(ARG002)
340-340: Unused method argument: args
(ARG002)
340-340: Unused method argument: kwargs
(ARG002)
356-356: Avoid specifying long messages outside the exception class
(TRY003)
440-440: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
availability.py (4)
74-123: Logic looks correct for slot generation with exception handling.The function properly handles slot generation with availability exceptions. The failsafe at line 93-95 prevents infinite loops, which is a nice defensive measure. The nested loop for exception checking (lines 98-109) could be optimized, but for typical workloads this should be fine.
126-148: Solid locking implementation for appointment creation.The use of
Lockcontext manager withtransaction.atomic()ensures thread-safe booking creation. The validation checks for past slots, full slots, and duplicate patient bookings are all appropriate. Based on learnings, the fail-fast approach for patient validation aligns with codebase conventions.
467-487: Slot calculation logic is sound.The while loop correctly iterates through time slots and checks for conflicts with exceptions. The conflict detection logic properly handles overlapping time ranges.
260-275: Creating slots on read request - verify this is intentional.The
get_slots_for_dayendpoint is a POST action that createsTokenSlotobjects (lines 268-275) as a side effect. While this lazy-creation pattern can be valid, it's worth confirming this behavior is intentional and documented, as it deviates from typical REST conventions where read operations don't mutate data.
| @model_validator(mode="after") | ||
| def validate_period(self): | ||
| max_period = 32 | ||
| if self.from_date > self.to_date: | ||
| raise ValidationError("From Date cannot be after To Date") | ||
| if self.to_date - self.from_date > datetime.timedelta(days=max_period): | ||
| msg = f"Period cannot be be greater than {max_period} days" | ||
| raise ValidationError(msg) | ||
|
|
There was a problem hiding this comment.
Critical: model_validator with mode="after" must return self.
The validate_period method doesn't return anything. Pydantic's model_validator with mode="after" requires the method to return the model instance, otherwise validation will silently fail or behave unexpectedly.
🐛 Proposed fix
@model_validator(mode="after")
def validate_period(self):
max_period = 32
if self.from_date > self.to_date:
raise ValidationError("From Date cannot be after To Date")
if self.to_date - self.from_date > datetime.timedelta(days=max_period):
msg = f"Period cannot be be greater than {max_period} days"
raise ValidationError(msg)
+ return self🧰 Tools
🪛 Ruff (0.14.10)
68-68: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @availability.py around lines 64 - 72, The model_validator method
validate_period (mode="after") performs checks but does not return the model;
pydantic requires an explicit return of the model instance for after-validators.
Modify validate_period to return self at the end (after performing the
from_date/to_date checks and potential ValidationError raises) so the validated
model is returned properly.
| patient = Patient.objects.filter(external_id=request_data.patient).first() | ||
| with transaction.atomic(): | ||
| if ( | ||
| TokenBooking.objects.filter( | ||
| patient=patient, | ||
| token_slot__start_datetime__gte=care_now(), | ||
| ) | ||
| .exclude(status__in=COMPLETED_STATUS_CHOICES) | ||
| .count() | ||
| >= settings.MAX_APPOINTMENTS_PER_PATIENT | ||
| ): | ||
| error = f"Patient already has maximum number of appointments ({settings.MAX_APPOINTMENTS_PER_PATIENT})" | ||
| raise ValidationError(error) | ||
|
|
||
| if not patient: | ||
| raise ValidationError("Patient not found") |
There was a problem hiding this comment.
Logic order issue: Patient lookup should occur before using patient in query.
The patient lookup on line 296 happens, then lines 298-308 use patient in a query filter before line 310-311 checks if patient is None. If the patient doesn't exist, the query at line 300 will use patient=None, which might not behave as intended.
Consider reordering to validate patient existence first.
🐛 Proposed fix
@classmethod
def create_appointment_handler(cls, obj, request_data, user):
request_data = AppointmentBookingSpec(**request_data)
patient = Patient.objects.filter(external_id=request_data.patient).first()
+ if not patient:
+ raise ValidationError("Patient not found")
with transaction.atomic():
if (
TokenBooking.objects.filter(
patient=patient,
token_slot__start_datetime__gte=care_now(),
)
.exclude(status__in=COMPLETED_STATUS_CHOICES)
.count()
>= settings.MAX_APPOINTMENTS_PER_PATIENT
):
error = f"Patient already has maximum number of appointments ({settings.MAX_APPOINTMENTS_PER_PATIENT})"
raise ValidationError(error)
- if not patient:
- raise ValidationError("Patient not found")
appointment = lock_create_appointment(obj, patient, user, request_data.note)🧰 Tools
🪛 Ruff (0.14.10)
311-311: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @availability.py around lines 296 - 311, The code checks TokenBooking counts
using the variable patient before ensuring Patient.objects.filter(...).first()
actually returned a Patient; move the existence check for patient (the result of
Patient.objects.filter(...).first()) immediately after that lookup and raise
ValidationError("Patient not found") if falsy, then proceed into the
transaction.atomic block (or keep the transaction and perform the check inside)
to run the TokenBooking query and the MAX_APPOINTMENTS_PER_PATIENT comparison;
update references to TokenBooking.objects.filter(patient=patient,
token_slot__start_datetime__gte=care_now()) and the ValidationError about
maximum appointments to occur only after patient existence is confirmed.
| def calculate_slots( | ||
| date: datetime.date, | ||
| availabilities: list[Availability], | ||
| schedules, | ||
| exceptions: list[AvailabilityException], | ||
| ): |
There was a problem hiding this comment.
Type hint mismatch: availabilities parameter.
The type hint says list[Availability], but the function receives dictionary values from .values() calls (see lines 367-372 and 456). The actual type is more like dict[int, QuerySet] where values are querysets of dicts.
📝 Proposed fix
def calculate_slots(
date: datetime.date,
- availabilities: list[Availability],
+ availabilities: dict,
schedules,
- exceptions: list[AvailabilityException],
+ exceptions,
):Or for more precision, consider using dict[int, QuerySet] or a TypedDict if you want to be explicit about the structure.
📝 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 calculate_slots( | |
| date: datetime.date, | |
| availabilities: list[Availability], | |
| schedules, | |
| exceptions: list[AvailabilityException], | |
| ): | |
| def calculate_slots( | |
| date: datetime.date, | |
| availabilities: dict, | |
| schedules, | |
| exceptions, | |
| ): |
🤖 Prompt for AI Agents
In @availability.py around lines 446 - 451, The type hint for calculate_slots is
incorrect: availabilities is annotated as list[Availability] but callers pass
dict values (QuerySet-like iterables via .values()), causing a mismatch; update
the function signature for calculate_slots to accept the actual structure (e.g.,
availabilities: dict[int, QuerySet] or availabilities: Iterable[QuerySet | dict]
/ Mapping[int, QuerySet]) and adjust any internal usage assuming list methods
accordingly so the parameter type reflects what's passed from the callers that
use .values().
| try: | ||
| facility = get_object_or_404(Facility, external_id=facility_external_id) | ||
| # CALLING THE SERVICE LAYER | ||
| slots = AvailabilityService.get_available_slots(facility, request.data) | ||
| return Response({"results": slots}) |
There was a problem hiding this comment.
Type mismatch: request.data is a dict, service expects object with attributes.
The service method accesses request_data.resource_type and request_data.resource_id as attributes, but request.data is a dictionary. This will raise an AttributeError at runtime.
You need to either:
- Parse
request.datainto a pydantic model (likeSlotsForDayRequestSpec) before passing to the service, or - Modify the service to accept a dict and use bracket notation.
🐛 Proposed fix (Option 1: Parse into pydantic model)
+from care.emr.resources.scheduling.slot.spec import SlotsForDayRequestSpec # or wherever it's defined
+
# ...
try:
facility = get_object_or_404(Facility, external_id=facility_external_id)
+ request_data = SlotsForDayRequestSpec(**request.data)
# CALLING THE SERVICE LAYER
- slots = AvailabilityService.get_available_slots(facility, request.data)
+ slots = AvailabilityService.get_available_slots(facility, request_data)
return Response({"results": slots})🤖 Prompt for AI Agents
In @care/emr/api/viewsets/scheduling/availability.py around lines 24 - 28, The
code passes request.data (a dict) to AvailabilityService.get_available_slots
which expects an object with attributes (it accesses request_data.resource_type,
resource_id); parse request.data into the pydantic model SlotsForDayRequestSpec
(e.g., instantiate/parse it from request.data) and pass that model instance to
AvailabilityService.get_available_slots (or alternatively change the service to
accept a dict), and handle pydantic ValidationError by returning a 400 Response
with validation details so invalid input is reported gracefully.
| def get_available_slots(facility, request_data): | ||
| """ | ||
| Pure business logic: Calculates slots without knowing about HTTP requests. | ||
| """ | ||
| # In the real code, we would call the helper function here | ||
| resource = get_schedulable_resource( | ||
| request_data.resource_type, | ||
| request_data.resource_id, | ||
| facility, | ||
| ) | ||
| if not resource: | ||
| raise ValidationError("No schedules found for this resource") | ||
|
|
||
| # This encapsulates the complex calculation logic shown in the "Before" code | ||
| # We return a simplified list for this architectural demo | ||
| slots = [] | ||
| return slots |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the file structure and verify the method exists
find . -type f -name "scheduling_service.py" | head -5Repository: ohcnetwork/care
Length of output: 100
🏁 Script executed:
#!/bin/bash
# Search for calls to get_available_slots (the method name in the code)
rg -n "get_available_slots" --type py -B2 -A3Repository: ohcnetwork/care
Length of output: 1127
🏁 Script executed:
#!/bin/bash
# Also search for AvailabilityService to understand the class structure
rg -n "class.*AvailabilityService\|def get_available_slots" --type py -B1 -A10Repository: ohcnetwork/care
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Let's examine the viewset file more closely to see how request.data is handled
cat -n care/emr/api/viewsets/scheduling/availability.py | head -40Repository: ohcnetwork/care
Length of output: 1588
🏁 Script executed:
#!/bin/bash
# Check if there's any serializer defined in this viewset
rg -n "Serializer\|serializer\|class.*ViewSet" care/emr/api/viewsets/scheduling/availability.py -B2 -A5Repository: ohcnetwork/care
Length of output: 41
Method will crash at runtime—request.data is a dictionary, not a structured object.
The service method accesses request_data.resource_type and request_data.resource_id as attributes, but the sole caller in care/emr/api/viewsets/scheduling/availability.py:27 passes request.data, which is a dictionary by default in Django REST Framework. This will raise an AttributeError when the code executes.
Either validate and convert request.data to a pydantic model before calling the service, or adjust the method to accept dictionary-style access (request_data["resource_type"]).
🧰 Tools
🪛 Ruff (0.14.10)
19-19: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @care/emr/services/scheduling_service.py around lines 8 - 24,
get_available_slots currently treats request_data as an object (using
request_data.resource_type and request_data.resource_id) but callers pass Django
REST Framework request.data (a dict), causing AttributeError; modify
get_available_slots to accept and handle a dict by reading keys with dict access
(e.g., request_data["resource_type"], request_data["resource_id"]) and validate
missing keys (raise ValidationError) before calling get_schedulable_resource, or
alternatively update caller to convert request.data to a validated object
(pydantic/dataclass) and document which approach is chosen; ensure the change
touches the get_available_slots signature/validation logic and uses the same key
names expected by get_schedulable_resource.
|
Hi, The Scheduling logic is being reworked on and has changed a lot from the current implementation. Merging this MR will add unnecessary conflicts to the new system, so we are forced to close this MR. ( TLDR; The care backend project does NOT accept AI generated MR's ) |
2 participants
Moved business logic from SlotViewSet to AvailabilityService to fix architectural coupling issues.
Summary by CodeRabbit
Release Notes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.