Add Reschedule API and serialize Facility in TokenBooking #2738
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the booking management system, focusing on appointment rescheduling functionality. The changes expand the booking status options by adding a Changes
Sequence DiagramsequenceDiagram
participant User
participant BookingViewSet
participant AppointmentHandler
participant SlotManager
User->>BookingViewSet: Request to reschedule
BookingViewSet->>BookingViewSet: Validate request
BookingViewSet->>AppointmentHandler: Cancel existing appointment
AppointmentHandler-->>BookingViewSet: Confirm cancellation
BookingViewSet->>SlotManager: Create new appointment
SlotManager-->>BookingViewSet: New appointment created
BookingViewSet-->>User: Rescheduling confirmed
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2738 +/- ##
===========================================
- Coverage 60.43% 60.41% -0.03%
===========================================
Files 252 252
Lines 12697 12719 +22
Branches 1111 1112 +1
===========================================
+ Hits 7674 7684 +10
- Misses 4954 4966 +12
Partials 69 69 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
care/emr/api/viewsets/scheduling/availability.py (1)
The review comment is spot-on, and here's why you should care about adding these validations
The current implementation allows for potentially problematic booking chains and circular references. Consider adding these validations:
- Prevent circular references by checking the booking chain
- Validate that the previous booking exists and is in a valid state
- Consider limiting the depth of booking chains to prevent infinite rescheduling
def lock_create_appointment( token_slot, patient, created_by, reason_for_visit, previous_booking=None ): with Lock(f"booking:resource:{token_slot.resource.id}"), transaction.atomic(): if previous_booking: # Prevent circular references if previous_booking.previous_booking: raise ValidationError("Nested rescheduling is not allowed") # Validate previous booking state if previous_booking.status not in ['cancelled', 'completed']: raise ValidationError("Previous booking must be cancelled or completed") if token_slot.allocated >= token_slot.availability.tokens_per_slot: raise ValidationError("Slot is already full") token_slot.allocated += 1 token_slot.save() return TokenBooking.objects.create( token_slot=token_slot, patient=patient, booked_by=created_by, reason_for_visit=reason_for_visit, status="booked", previous_booking=previous_booking, )🔗 Analysis chain
Line range hint
81-96: Consider adding validation for previous_booking reference.The function accepts a
previous_bookingparameter but doesn't validate against potential circular references or invalid states. You might want to add some basic validation, you know, just to be safe.Let's verify if there are any existing circular references or validation in the model:
Consider adding validation like:
def lock_create_appointment( token_slot, patient, created_by, reason_for_visit, previous_booking=None ): with Lock(f"booking:resource:{token_slot.resource.id}"), transaction.atomic(): + if previous_booking and previous_booking.previous_booking: + raise ValidationError("Nested rescheduling is not allowed") if token_slot.allocated >= token_slot.availability.tokens_per_slot: raise ValidationError("Slot is already full")🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any validation related to previous_booking in models and validators rg -l "previous_booking.*validate|validate.*previous_booking" # Search for any existing circular reference prevention ast-grep --pattern 'class TokenBooking: $$$ previous_booking = $$$ $$$ 'Length of output: 144
Script:
#!/bin/bash # Find TokenBooking model definition rg "class TokenBooking" -A 10 # Look for any clean or save methods in TokenBooking rg "def (clean|save).*TokenBooking" -A 5 # Find any file containing both TokenBooking and previous_booking rg "TokenBooking.*previous_booking|previous_booking.*TokenBooking" -A 5Length of output: 4516
Script:
#!/bin/bash # Get the complete TokenBooking model definition with more context cat care/emr/models/scheduling/booking.py # Backup search for any validation in the same file rg "previous_booking" care/emr/models/scheduling/booking.py -A 5Length of output: 1623
🧹 Nitpick comments (4)
care/emr/migrations/0007_tokenbooking_previous_booking.py (1)
14-23: Consider adding a database index for the previous_booking field.Since this field will likely be used in queries to track booking history, an index might improve query performance.
field=models.ForeignKey( blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to="emr.tokenbooking", + db_index=True, ),care/emr/models/scheduling/booking.py (1)
35-37: Field implementation looks good, but consider adding help_text.The previous_booking field is properly implemented, but it would be nice to have documentation about its purpose.
previous_booking = models.ForeignKey( "TokenBooking", on_delete=models.SET_NULL, null=True, blank=True, + help_text="Reference to the original booking if this is a rescheduled appointment", )care/emr/api/viewsets/scheduling/booking.py (1)
128-158: Consider adding rate limiting to prevent abuse.The reschedule endpoint could potentially be abused to continuously reschedule appointments.
Consider implementing rate limiting using Django's cache framework or a third-party package like django-ratelimit.
care/emr/api/viewsets/scheduling/availability.py (1)
81-83: Add docstring to explain the new parameter and its purpose.The function signature has been updated, but there's no documentation explaining the purpose of
previous_bookingor its relationship with rescheduling functionality. I mean, I'm sure you know what it does, but others might appreciate some clarity.def lock_create_appointment( token_slot, patient, created_by, reason_for_visit, previous_booking=None ): + """Create a new appointment with locking mechanism. + + Args: + token_slot: The slot to book + patient: Patient for whom the appointment is being booked + created_by: User creating the appointment + reason_for_visit: Reason for the appointment + previous_booking: Reference to the previous booking if this is a reschedule + + Returns: + TokenBooking: The created booking + + Raises: + ValidationError: If the slot is already full + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
care/emr/api/viewsets/scheduling/availability.py(2 hunks)care/emr/api/viewsets/scheduling/booking.py(4 hunks)care/emr/migrations/0007_tokenbooking_previous_booking.py(1 hunks)care/emr/models/scheduling/booking.py(1 hunks)care/emr/resources/scheduling/slot/spec.py(4 hunks)
🔇 Additional comments (5)
care/emr/migrations/0007_tokenbooking_previous_booking.py (1)
9-11: LGTM! Migration dependency looks correct.The migration properly depends on the blood_group alteration, maintaining a clean migration history.
care/emr/models/scheduling/booking.py (1)
Line range hint
7-7: That TODO comment has been sitting there for a while, hasn't it?The comment suggests propagating facility to avoid joins. Since we're touching this file, perhaps we should address this technical debt?
Would you like me to propose a solution for propagating the facility or create an issue to track this?
care/emr/resources/scheduling/slot/spec.py (1)
57-57: The rescheduled status addition looks good.The status is properly added to both enum and CANCELLED_STATUS_CHOICES.
Also applies to: 63-63
care/emr/api/viewsets/scheduling/booking.py (1)
138-142: Verify that the new slot belongs to the same facility.The code only checks the resource, but we should ensure the slot hasn't been moved to a different facility.
care/emr/api/viewsets/scheduling/availability.py (1)
Line range hint
81-96: Verify lock mechanism coverage for rescheduling scenarios.The current lock only covers the resource ID. For rescheduling scenarios, we might need to consider locking both the old and new slots to prevent race conditions.
Let's check if there are any existing race condition preventions:
✅ Verification successful
Actually, the current lock implementation is perfectly adequate 🎯
The rescheduling flow is already well-protected against race conditions through a combination of:
- Atomic transactions ensuring all-or-nothing execution
- Resource locking during new slot creation
- Previous booking tracking for audit purposes
While your concern was thoughtful, adding another lock would be a bit... redundant, if I may say so.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other lock usages in scheduling related code rg -B 5 -A 5 'Lock\(' --glob 'care/emr/**/*.py' # Search for any race condition related issues or comments rg -i 'race condition|concurrent|synchroniz' --glob 'care/emr/**/*.py'Length of output: 3480
Script:
#!/bin/bash # Search for reschedule-related code rg -B 5 -A 5 'reschedul|previous_booking' --glob 'care/emr/**/*.py' # Search for TokenBooking model usage ast-grep --pattern 'class TokenBooking($$$)'Length of output: 8447
| with transaction.atomic(): | ||
| self.cancel_appointment_handler( | ||
| existing_booking, | ||
| {"reason": BookingStatusChoices.rescheduled}, | ||
| request.user, | ||
| ) | ||
| appointment = lock_create_appointment( | ||
| new_slot, | ||
| existing_booking.patient, | ||
| request.user, | ||
| existing_booking.reason_for_visit, | ||
| previous_booking=existing_booking, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The transaction handling looks good, but consider adding capacity validation.
While the transaction ensures atomicity, we should verify that the new slot has available capacity before canceling the existing booking.
with transaction.atomic():
+ if new_slot.allocated >= new_slot.availability.tokens_per_slot:
+ raise ValidationError("Selected slot is already at capacity")
self.cancel_appointment_handler(
existing_booking,
{"reason": BookingStatusChoices.rescheduled},
request.user,
)📝 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.
| with transaction.atomic(): | |
| self.cancel_appointment_handler( | |
| existing_booking, | |
| {"reason": BookingStatusChoices.rescheduled}, | |
| request.user, | |
| ) | |
| appointment = lock_create_appointment( | |
| new_slot, | |
| existing_booking.patient, | |
| request.user, | |
| existing_booking.reason_for_visit, | |
| previous_booking=existing_booking, | |
| ) | |
| with transaction.atomic(): | |
| if new_slot.allocated >= new_slot.availability.tokens_per_slot: | |
| raise ValidationError("Selected slot is already at capacity") | |
| self.cancel_appointment_handler( | |
| existing_booking, | |
| {"reason": BookingStatusChoices.rescheduled}, | |
| request.user, | |
| ) | |
| appointment = lock_create_appointment( | |
| new_slot, | |
| existing_booking.patient, | |
| request.user, | |
| existing_booking.reason_for_visit, | |
| previous_booking=existing_booking, | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
care/emr/resources/scheduling/slot/spec.py (1)
104-106:⚠️ Potential issueThat get() call is still living dangerously, I see.
The direct
Facility.objects.get()call without error handling persists. This could lead to 500 errors if the facility doesn't exist.Consider implementing error handling:
- mapping["facility"] = FacilityBareMinimumSpec.serialize( - Facility.objects.get(id=obj.token_slot.resource.facility_id) - ).model_dump(exclude=["meta"]) + try: + facility = Facility.objects.get(id=obj.token_slot.resource.facility_id) + mapping["facility"] = FacilityBareMinimumSpec.serialize( + facility + ).model_dump(exclude=["meta"]) + except Facility.DoesNotExist: + mapping["facility"] = {}
🧹 Nitpick comments (1)
care/emr/resources/scheduling/slot/spec.py (1)
57-57: Would it kill you to add some documentation?Consider adding a docstring to the
BookingStatusChoicesenum explaining what each status means, particularly the newrescheduledstatus. This would make life slightly easier for future developers.class BookingStatusChoices(str, Enum): + """ + Enum defining all possible states of a token booking. + + - rescheduled: Indicates that the booking has been moved to a different time slot + [Add descriptions for other statuses as well] + """ proposed = "proposed"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/api/viewsets/scheduling/booking.py(4 hunks)care/emr/resources/scheduling/slot/spec.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- care/emr/api/viewsets/scheduling/booking.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
care/emr/resources/scheduling/slot/spec.py (2)
11-11: LGTM! The imports are properly organized.The new imports for facility-related functionality are well-placed and follow conventions.
Also applies to: 14-14
63-63: The addition to CANCELLED_STATUS_CHOICES makes perfect sense.Including
rescheduledin the cancelled statuses maintains logical consistency, as a rescheduled booking implies cancellation of the original slot.
Proposed 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
Release Notes
New Features
Improvements
The update provides users with more flexible appointment management and richer booking information.