Scheduling and Appointments: Validations and other minor fixes #2711
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files in the scheduling and resource management components. The changes primarily focus on simplifying user and booking-related logic, removing unnecessary model dependencies, and enhancing authorization and validation processes for scheduling operations. The modifications streamline how users, availability exceptions, and bookings are handled, with a particular emphasis on reducing complexity in user retrieval and booking cancellation mechanisms. It seems like someone finally decided to tidy up the code a bit—how refreshing! Changes
Sequence DiagramsequenceDiagram
participant User
participant BookingViewSet
participant CancelBookingSpec
participant TokenSlot
User->>BookingViewSet: Request Booking Cancellation
BookingViewSet->>CancelBookingSpec: Validate Cancellation Reason
CancelBookingSpec-->>BookingViewSet: Validation Result
BookingViewSet->>TokenSlot: Decrement Allocated Count
BookingViewSet->>BookingViewSet: Update Booking Status
Possibly Related PRs
Suggested Reviewers
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: 1
🧹 Nitpick comments (3)
care/emr/resources/scheduling/availability_exception/spec.py (1)
39-39: Validate user existence earlier or handle multiple matches.Retrieving the user object here is fine, but it might be gentler to validate earlier or provide a more detailed error message if the user is missing or ambiguous. Users sometimes claim confusion if they can’t figure out which user ID is invalid.
care/emr/api/viewsets/scheduling/availability_exceptions.py (1)
42-48: Streamline authorization and unify logic.Extracting the
facilityand user objects directly is a welcome simplification. However, verify that all references toSchedulableUserResourcein parallel code paths are cleanly removed to avert any partially leftover logic. Also, watch for potential duplication in your authorization code.care/emr/api/viewsets/scheduling/booking.py (1)
107-117:cancel_appointment_handler
Good concurrency flow. Subtly, if multiple cancellations target the same booking simultaneously, the lock ensures consistency. Just watch out for any negativeallocatedstates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
care/emr/api/viewsets/scheduling/availability_exceptions.py(2 hunks)care/emr/api/viewsets/scheduling/booking.py(4 hunks)care/emr/api/viewsets/scheduling/schedule.py(3 hunks)care/emr/resources/scheduling/availability_exception/spec.py(1 hunks)care/emr/resources/scheduling/schedule/spec.py(0 hunks)care/emr/resources/scheduling/slot/spec.py(2 hunks)
💤 Files with no reviewable changes (1)
- care/emr/resources/scheduling/schedule/spec.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (21)
care/emr/resources/scheduling/availability_exception/spec.py (1)
33-33: Confirm field name aligns with domain model.While changing from
resourcetouserimproves clarity, please ensure that this field's naming convention also reflects the broader domain model. In future, any references to "resource" will likely need rework.care/emr/api/viewsets/scheduling/availability_exceptions.py (2)
7-7: Remove unused import references or clarify usage.Since
AvailabilityExceptionis used, it’s acceptable here. Consider verifying if there are no stale references from your earlier refactor that might remain in other modules.
14-14: Confirm user import usage.Be absolutely sure the shift to a direct
Userimport is consistent across all references. A mismatch with the oldSchedulableUserResourcemight cause a subtle drift in code usage.care/emr/api/viewsets/scheduling/schedule.py (4)
2-2: Importtimezonelooks appropriate
This import is used properly below for comparisons with the current time.
10-10: TokenSlot import is used in future booking checks
No immediate concerns.
20-20: Lock import is consistent
It's nice to see concurrency concerns being addressed with a locking mechanism.
49-65: Verify concurrency and future booking checks
The methodology for preventing schedule deletions when future bookings exist appears sensible. While the lock usage helps avoid concurrent deletions, you might want to verify whether subsequent changes to availability can occur while this transaction is in progress. A thorough test would be wise to ensure no corner cases slip by.✅ Verification successful
Concurrency and booking checks are properly implemented
The combination of resource-level locking and database transactions ensures safe schedule deletion. The future booking check is comprehensive and properly atomic. No changes needed, though you might want to add a test if you're feeling particularly thorough about it.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references to perform_delete for concurrency usage rg -A 5 "perform_delete"Length of output: 1846
care/emr/api/viewsets/scheduling/booking.py (11)
1-2: Literal import
Typing clarity is always nice; no issues with addingLiteral.
6-6: BaseModel import
Using Pydantic's BaseModel is consistent with the rest of the code.
20-21: Cancellation references
These references to status choices align with the new cancellation flow.Also applies to: 23-23
28-28: Lock import ensures concurrency
Similar toschedule.py, using Lock for bookings is a thorough approach.
31-34:CancelBookingSpec
Requiring areasonfor cancellation is good for audit trails.
61-61: Blank line
No changes needed, but thanks for the spacing.
63-63: Removal of delete mixin
ExcludingEMRDeleteMixinis a safer design if you want to handle cancellations separately.
66-66: Switch toTokenBookingWriteSpec
By defaulting to the write spec, ensure that older usage patterns remain unaffected.
68-68: Unified write model
Using the same spec for create and update is consistent with the new flow, but carefully verify that no read-only fields get overwritten.
86-86: PermissionDenied message
Explicitly referencing "update bookings" clarifies the error. Good improvement.
118-123: Newcancelaction
A dedicated cancellation endpoint is a cleaner approach. This matches the PR objective of preventing cancellation via other methods.care/emr/resources/scheduling/slot/spec.py (3)
5-5: ValidationError import
This import is necessary for the new restriction logic below, so no issue here.
68-68:TokenBookingWriteSpec
Renaming fromTokenBookingUpdateSpecclarifies its unified role for writes.
71-74: Enforcing separate cancellation flow
Rejecting status changes to a canceled state here is a succinct way to force the official cancellation endpoint usage, though it might feel a bit strict if there's a corner case.
| def lock_cancel_appointment(token_booking): | ||
| token_slot = token_booking.token_slot | ||
| with Lock(f"booking:slot:{token_slot.id}"), transaction.atomic(): | ||
| token_slot.allocated -= 1 | ||
| token_slot.save() | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Slot allocation needs validation
The lock prevents race conditions, but there's nothing stopping the allocation from going negative. You might want to add a quick check before decrementing, like if token_slot.allocated > 0. I mean, unless negative appointments are your thing.
- Add validation in
lock_cancel_appointmentbefore decrementing - Consider adding a
CheckConstraintto theTokenSlotmodel to enforce non-negative values
🔗 Analysis chain
Slot lock and decrement
This correctly decrements allocation within a transaction. Keep in mind to confirm that the allocated count never goes below zero if multiple cancellations happen in quick succession.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for any references to 'allocated -= 1' to ensure there's no conflicting code
rg -A 5 "allocated -= 1"
Length of output: 446
Script:
#!/bin/bash
# Find TokenSlot model definition
ast-grep --pattern 'class TokenSlot($$$):'
# Check for any validation on allocated field
rg -B 5 "allocated = "
Length of output: 615
Script:
#!/bin/bash
# Check for allocation increments
rg -A 5 "allocated \+= 1"
# Look for validation methods on TokenSlot
ast-grep --pattern 'def $_(self$$$):
$$$
allocated$$$
$$$'
Length of output: 591
There was a problem hiding this comment.
If we add this check, that means somewhere an extra reduction is happening accidentally right, and the root cause of it would never see the day of light right? 🤔
There was a problem hiding this comment.
@rithviknishad, that’s a fair concern. Adding a check (or constraint) can indeed mask a deeper bug if the decrement is happening more often than it should. In some cases, though, a safeguard is still useful to prevent corrupted state, especially if the root cause isn’t trivial to pinpoint quickly. One possible compromise is to raise an exception (or log a warning) whenever allocated would go negative. That way, the data remains valid, but you also become aware (immediately!) if something is decrementing one time too many.
| @@ -89,6 +104,23 @@ def get_queryset(self): | |||
| .order_by("-modified_date") | |||
| ) | |||
|
|
|||
| @classmethod | |||
There was a problem hiding this comment.
i just tried to stick with similar pattern as of create appointment. will revert 😅
There was a problem hiding this comment.
These are classmethods because they are called from the OTP based viewsets as well.
|
Create the API from the OTP perspective as well. User should be able to cancel a |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
care/emr/api/viewsets/scheduling/booking.py (1)
99-111:⚠️ Potential issueThat allocated count decrement looks awfully brave
I see we're still living dangerously with unconditional decrements. While I appreciate the optimism in assuming nothing could go wrong, perhaps we could add a small check? You know, just in case reality doesn't match our expectations.
with transaction.atomic(): if instance.status not in CANCELLED_STATUS_CHOICES: # Free up the slot if it is not cancelled already + if instance.token_slot.allocated <= 0: + raise ValueError("Cannot decrement allocation: already at zero") instance.token_slot.allocated -= 1 instance.token_slot.save()
🧹 Nitpick comments (4)
care/emr/api/viewsets/scheduling/schedule.py (1)
49-51: Consider optimizing the lock scope.The lock acquisition spans the entire update operation. While this ensures thread safety, it might be holding the lock longer than strictly necessary. Perhaps we could narrow down the critical section?
care/emr/resources/scheduling/schedule/spec.py (1)
89-120: Consider optimizing the allocated tokens queries.While the validation logic is solid, we're making two separate queries that could potentially be combined into one. Just saying, databases have feelings too.
- old_allocated_sum = ( - TokenSlot.objects.filter( - resource=old_instance.resource, - availability__schedule__id=obj.id, - start_datetime__gte=old_instance.valid_from, - start_datetime__lte=old_instance.valid_to, - ).aggregate(total=Sum("allocated"))["total"] - or 0 - ) - - new_allocated_sum = ( - TokenSlot.objects.filter( - resource=old_instance.resource, - availability__schedule__id=obj.id, - start_datetime__gte=self.valid_from, - start_datetime__lte=self.valid_to, - ).aggregate(total=Sum("allocated"))["total"] - or 0 - ) + from django.db.models import Case, When, Value, IntegerField + allocated_sums = TokenSlot.objects.filter( + resource=old_instance.resource, + availability__schedule__id=obj.id, + ).aggregate( + old_sum=Sum(Case( + When( + start_datetime__gte=old_instance.valid_from, + start_datetime__lte=old_instance.valid_to, + then='allocated' + ), + default=Value(0), + output_field=IntegerField(), + )), + new_sum=Sum(Case( + When( + start_datetime__gte=self.valid_from, + start_datetime__lte=self.valid_to, + then='allocated' + ), + default=Value(0), + output_field=IntegerField(), + )) + ) + old_allocated_sum = allocated_sums['old_sum'] or 0 + new_allocated_sum = allocated_sums['new_sum'] or 0care/emr/api/viewsets/scheduling/availability.py (1)
182-182: Improve error message structure.The error message
{"Patient not found"}could be more specific. Perhaps something like{"error": "Patient with id {request_data.patient} not found"}?- raise ValidationError({"Patient not found"}) + raise ValidationError({ + "error": f"Patient with id {request_data.patient} not found" + })care/emr/api/viewsets/scheduling/booking.py (1)
77-78: Authorization message improved, but scope could be betterThe error message is more accurate now, but shouldn't we also check if the booking is in a cancellable state before allowing updates? Just a thought.
- raise PermissionDenied("You do not have permission to update bookings") + raise PermissionDenied( + "You do not have permission to update bookings in their current state" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
care/emr/api/viewsets/scheduling/availability.py(1 hunks)care/emr/api/viewsets/scheduling/booking.py(4 hunks)care/emr/api/viewsets/scheduling/schedule.py(3 hunks)care/emr/migrations/0002_alter_availability_schedule.py(1 hunks)care/emr/models/scheduling/schedule.py(1 hunks)care/emr/resources/scheduling/schedule/spec.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- care/emr/migrations/0002_alter_availability_schedule.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (6)
care/emr/api/viewsets/scheduling/schedule.py (1)
76-80: LGTM! Clean user retrieval logic.The conditional user retrieval looks good. Nice defensive programming there!
care/emr/models/scheduling/schedule.py (1)
23-25: Verify migration for related_name addition.Adding
related_nameis good for reverse lookups, but let's make sure we have the corresponding migration. You wouldn't want to leave your database in an inconsistent state, would you?✅ Verification successful
Migration verified, you're all set 🎯
I can see you've already taken care of the migration like a responsible developer. The migration file
0002_alter_availability_schedule.pyproperly handles therelated_nameaddition. How... unexpectedly thorough of you.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for migration file containing related_name change rg -l "related_name.*availabilities" care/emr/migrations/*.pyLength of output: 218
Script:
#!/bin/bash # Display content of the migration file cat care/emr/migrations/0002_alter_availability_schedule.pyLength of output: 617
care/emr/resources/scheduling/schedule/spec.py (1)
63-67: LGTM! Simple and effective validation.Clean period validation. Short, sweet, and to the point.
care/emr/api/viewsets/scheduling/booking.py (3)
1-34: Nice work with the Pydantic validation!The
CancelBookingSpecclass is well-structured and properly constrains the cancellation reasons. I suppose we're finally getting serious about input validation.
113-117: Clean implementation of the cancel endpointNice separation of concerns between authorization and handling. The implementation is refreshingly straightforward, which is... unusual for this codebase.
55-61: Consider separating create and update specificationsUsing the same
TokenBookingWriteSpecfor both creation and updates might be a bit... optimistic. Different validation rules typically apply for these operations, but I'm sure you've thought this through.Let's check if there are any validation differences:
| def perform_delete(self, instance): | ||
| with Lock(f"booking:resource:{instance.resource.id}"), transaction.atomic(): | ||
| # Check if there are any tokens allocated for this schedule in the future | ||
| availabilities = instance.availabilities.all() | ||
| has_future_bookings = TokenSlot.objects.filter( | ||
| resource=instance.resource, | ||
| availability_id__in=availabilities.values_list("id", flat=True), | ||
| start_datetime__gt=timezone.now(), | ||
| allocated__gt=0, | ||
| ).exists() | ||
| if has_future_bookings: | ||
| raise ValidationError( | ||
| "Cannot delete schedule as there are future bookings associated with it" | ||
| ) | ||
| availabilities.update(deleted=True) | ||
| super().perform_delete(instance) |
There was a problem hiding this comment.
💡 Codebase verification
Your database queries could use a little optimization boost.
The review comment about adding indexes is valid. The TokenSlot model lacks indexes on start_datetime and allocated fields, which could make your future booking queries slower than a sloth climbing a tree. However, the race condition concern is incorrect - the combination of resource-level locking and atomic transaction already handles this elegantly.
- Add a migration to create indexes on
TokenSlot.start_datetimeandTokenSlot.allocated
🔗 Analysis chain
Add index for future booking queries.
The query filtering future bookings would benefit from an index on start_datetime and allocated fields. I mean, unless you enjoy watching your database work harder than it needs to.
Potential race condition in future bookings check.
Even with the lock, there's a tiny window between checking for future bookings and actual deletion where new bookings could be created. Consider adding a status field to prevent new bookings during deletion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any indexes on start_datetime and allocated fields
rg -A 5 "class Migration|^ indexes = \[" care/emr/migrations/*.py
Length of output: 956
Script:
#!/bin/bash
# Find TokenSlot model definition and any related indexes
echo "=== TokenSlot Model Definition ==="
ast-grep --pattern 'class TokenSlot($$$):'
echo -e "\n=== Searching all migrations for TokenSlot indexes ==="
rg -l "TokenSlot" care/emr/migrations/*.py | xargs cat
Length of output: 60793
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2711 +/- ##
===========================================
- Coverage 45.97% 45.95% -0.02%
===========================================
Files 315 313 -2
Lines 17532 17565 +33
Branches 1682 1690 +8
===========================================
+ Hits 8060 8072 +12
- Misses 9427 9433 +6
- Partials 45 60 +15 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
care/emr/resources/scheduling/schedule/spec.py (2)
42-56: Consider optimizing the overlapping time validation.The nested loops for checking overlapping time ranges have O(n²) complexity. For larger schedules, this could become a performance bottleneck.
Consider using an interval tree data structure for more efficient overlap detection:
from intervaltree import IntervalTree @field_validator("availability", mode="after") def validate_availability(cls, availabilities: list[AvailabilityDateTimeSpec]): day_trees = {} for avail in availabilities: if avail.day_of_week not in day_trees: day_trees[avail.day_of_week] = IntervalTree() tree = day_trees[avail.day_of_week] if tree.overlaps(avail.start_time, avail.end_time): raise ValueError("Availability time ranges are overlapping") tree.addi(avail.start_time, avail.end_time) return availabilities
64-80: Robust token validation, but consider edge cases.The validation for preventing token modifications that would affect future bookings is well implemented. However, you might want to add a grace period to avoid race conditions with imminent bookings.
Consider adding a small buffer time:
- start_datetime__gte=timezone.now(), + start_datetime__gte=timezone.now() + timezone.timedelta(minutes=5),care/emr/api/viewsets/scheduling/schedule.py (2)
51-54: Consider adding timeout to the lock.While the locking mechanism is good, it might benefit from a timeout to prevent indefinite waiting.
- with Lock(f"booking:resource:{instance.resource.id}"): + with Lock(f"booking:resource:{instance.resource.id}", timeout=30):
185-189: Consider simplifying authorization chain.The authorization chain could be simplified since
authorize_updateandauthorize_deleteboth delegate toauthorize_create.- def authorize_update(self, request_obj, model_instance): - self.authorize_create({}, model_instance) - - def authorize_delete(self, instance): - self.authorize_update({}, instance) + def _authorize_common(self, instance): + facility = self.get_facility_obj() + schedule_user = self.get_schedule_obj().resource.user + if not AuthorizationController.call( + "can_write_user_schedule", self.request.user, facility, schedule_user + ): + raise PermissionDenied("You do not have permission to create schedule") + + authorize_update = authorize_delete = _authorize_common
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/api/viewsets/scheduling/schedule.py(5 hunks)care/emr/resources/scheduling/schedule/spec.py(5 hunks)config/api_router.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (3)
config/api_router.py (1)
43-47: LGTM! Clean routing setup.The nested router configuration for availability management is well-structured and follows REST conventions.
Also applies to: 175-180
care/emr/resources/scheduling/schedule/spec.py (1)
108-111: LGTM! Clear period validation.The validation ensures logical consistency in schedule periods.
care/emr/api/viewsets/scheduling/schedule.py (1)
59-64: Add index for future booking queries.The query filtering future bookings would benefit from an index on
start_datetimeandallocatedfields.
|
Is this ready for review ? |
Proposed Changes
Summary by CodeRabbit
Release Notes
New Features
AvailabilityViewSetfor managing availability instances.Bug Fixes
Refactoring