Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scheduling and Appointments: Validations and other minor fixes #2711

Merged
merged 16 commits into from
Jan 8, 2025
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions care/emr/api/viewsets/scheduling/availability.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,7 @@ def get_slots_for_day_handler(cls, facility_external_id, request_data):
# Get list of all slots, create if missed
# Return slots

@classmethod
def create_appointment_handler(cls, obj, request_data, user):
def create_appointment_handler(self, obj, request_data, user):
request_data = AppointmentBookingSpec(**request_data)
patient = Patient.objects.filter(external_id=request_data.patient).first()
if not patient:
Expand Down
12 changes: 6 additions & 6 deletions care/emr/api/viewsets/scheduling/availability_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@
from rest_framework.generics import get_object_or_404

from care.emr.api.viewsets.base import EMRModelViewSet
from care.emr.models import AvailabilityException, SchedulableUserResource
from care.emr.models import AvailabilityException
from care.emr.resources.scheduling.availability_exception.spec import (
AvailabilityExceptionReadSpec,
AvailabilityExceptionWriteSpec,
)
from care.facility.models import Facility
from care.security.authorization import AuthorizationController
from care.users.models import User


class AvailabilityExceptionFilters(FilterSet):
Expand Down Expand Up @@ -38,14 +39,13 @@ def authorize_delete(self, instance):
self.authorize_update({}, instance)

def authorize_create(self, instance):
user_resource = get_object_or_404(
SchedulableUserResource, external_id=instance.resource
)
facility = self.get_facility_obj()
schedule_user = get_object_or_404(User, external_id=instance.user)
if not AuthorizationController.call(
"can_write_user_schedule",
self.request.user,
user_resource.facility,
user_resource.user,
facility,
schedule_user,
):
raise PermissionDenied("You do not have permission to view user schedule")

Expand Down
47 changes: 37 additions & 10 deletions care/emr/api/viewsets/scheduling/booking.py
Original file line number Diff line number Diff line change
@@ -1,27 +1,38 @@
from typing import Literal

from django.db import transaction
from django_filters import CharFilter, DateFilter, FilterSet, UUIDFilter
from django_filters.rest_framework import DjangoFilterBackend
from pydantic import BaseModel
from rest_framework.decorators import action
from rest_framework.exceptions import PermissionDenied
from rest_framework.generics import get_object_or_404
from rest_framework.response import Response

from care.emr.api.viewsets.base import (
EMRBaseViewSet,
EMRDeleteMixin,
EMRListMixin,
EMRRetrieveMixin,
EMRUpdateMixin,
)
from care.emr.models.scheduling import SchedulableUserResource, TokenBooking
from care.emr.resources.scheduling.slot.spec import (
CANCELLED_STATUS_CHOICES,
BookingStatusChoices,
TokenBookingReadSpec,
TokenBookingUpdateSpec,
TokenBookingWriteSpec,
)
from care.emr.resources.user.spec import UserSpec
from care.facility.models import Facility, FacilityOrganizationUser
from care.security.authorization import AuthorizationController


class CancelBookingSpec(BaseModel):
reason: Literal[
BookingStatusChoices.cancelled, BookingStatusChoices.entered_in_error
]


class TokenBookingFilters(FilterSet):
status = CharFilter(field_name="status")
date = DateFilter(field_name="token_slot__start_datetime__date")
Expand All @@ -41,12 +52,12 @@ def filter_by_user(self, queryset, name, value):


class TokenBookingViewSet(
EMRRetrieveMixin, EMRUpdateMixin, EMRListMixin, EMRDeleteMixin, EMRBaseViewSet
EMRRetrieveMixin, EMRUpdateMixin, EMRListMixin, EMRBaseViewSet
):
database_model = TokenBooking
pydantic_model = TokenBookingReadSpec
pydantic_model = TokenBookingWriteSpec
pydantic_read_model = TokenBookingReadSpec
pydantic_update_model = TokenBookingUpdateSpec
pydantic_update_model = TokenBookingWriteSpec

filterset_class = TokenBookingFilters
filter_backends = [DjangoFilterBackend]
Expand All @@ -57,18 +68,14 @@ def get_facility_obj(self):
Facility, external_id=self.kwargs["facility_external_id"]
)

def authorize_delete(self, instance):
# TODO, need more depth to handle this case
pass

def authorize_update(self, request_obj, model_instance):
if not AuthorizationController.call(
"can_write_user_booking",
self.request.user,
model_instance.token_slot.resource.facility,
model_instance.token_slot.resource.user,
):
raise PermissionDenied("You do not have permission to view user schedule")
raise PermissionDenied("You do not have permission to update bookings")

def get_queryset(self):
facility = self.get_facility_obj()
Expand All @@ -89,6 +96,26 @@ def get_queryset(self):
.order_by("-modified_date")
)

def cancel_appointment_handler(self, instance, request_data, user):
request_data = CancelBookingSpec(**request_data)
with transaction.atomic():
if instance.status not in CANCELLED_STATUS_CHOICES:
# Free up the slot if it is not cancelled already
instance.token_slot.allocated -= 1
instance.token_slot.save()
instance.status = request_data.reason
instance.updated_by = user
instance.save()
return Response(
TokenBookingReadSpec.serialize(instance).model_dump(exclude=["meta"])
)

@action(detail=True, methods=["POST"])
def cancel(self, request, *args, **kwargs):
instance = self.get_object()
self.authorize_update({}, instance)
return self.cancel_appointment_handler(instance, request.data, request.user)

@action(detail=False, methods=["GET"])
def available_users(self, request, *args, **kwargs):
facility = Facility.objects.get(external_id=self.kwargs["facility_external_id"])
Expand Down
30 changes: 29 additions & 1 deletion care/emr/api/viewsets/scheduling/schedule.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
from django.db import transaction
from django.utils import timezone
from django_filters import FilterSet, UUIDFilter
from django_filters.rest_framework import DjangoFilterBackend
from rest_framework.exceptions import PermissionDenied, ValidationError
from rest_framework.generics import get_object_or_404

from care.emr.api.viewsets.base import EMRModelViewSet
from care.emr.models.organization import FacilityOrganizationUser
from care.emr.models.scheduling.booking import TokenSlot
from care.emr.models.scheduling.schedule import Schedule
from care.emr.resources.scheduling.schedule.spec import (
ScheduleReadSpec,
Expand All @@ -15,6 +17,7 @@
from care.facility.models import Facility
from care.security.authorization import AuthorizationController
from care.users.models import User
from care.utils.lock import Lock


class ScheduleFilters(FilterSet):
Expand Down Expand Up @@ -43,13 +46,38 @@ def perform_create(self, instance):
availability_obj.schedule = instance
availability_obj.save()

def perform_update(self, instance):
with Lock(f"booking:resource:{instance.resource.id}"):
super().perform_update(instance)

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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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_datetime and TokenSlot.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


def authorize_delete(self, instance):
self.authorize_update({}, instance)

def validate_data(self, instance, model_obj=None):
# Validate user is part of the facility
facility = self.get_facility_obj()
schedule_user = get_object_or_404(User, external_id=instance.user)
schedule_user = (
model_obj.resource.user
if model_obj
else get_object_or_404(User, external_id=instance.user)
)
if not FacilityOrganizationUser.objects.filter(
user=schedule_user, organization__facility=facility
).exists():
Expand Down
23 changes: 23 additions & 0 deletions care/emr/migrations/0002_alter_availability_schedule.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Generated by Django 5.1.3 on 2025-01-07 05:22

import django.db.models.deletion
from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("emr", "0001_initial"),
]

operations = [
migrations.AlterField(
model_name="availability",
name="schedule",
field=models.ForeignKey(
on_delete=django.db.models.deletion.CASCADE,
related_name="availabilities",
to="emr.schedule",
),
),
]
4 changes: 3 additions & 1 deletion care/emr/models/scheduling/schedule.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ class Schedule(EMRBaseModel):


class Availability(EMRBaseModel):
schedule = models.ForeignKey(Schedule, on_delete=models.CASCADE)
schedule = models.ForeignKey(
Schedule, on_delete=models.CASCADE, related_name="availabilities"
)
name = models.CharField(max_length=255)
slot_type = models.CharField()
slot_size_in_minutes = models.IntegerField(null=False, blank=False, default=0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@ class AvailabilityExceptionBaseSpec(EMRResource):

class AvailabilityExceptionWriteSpec(AvailabilityExceptionBaseSpec):
facility: UUID4 | None = None
resource: UUID4
user: UUID4

def perform_extra_deserialization(self, is_update, obj):
if not is_update:
resource = None
try:
user = User.objects.get(external_id=self.resource)
user = User.objects.get(external_id=self.user)
resource = SchedulableUserResource.objects.get(
user=user,
facility=Facility.objects.get(external_id=self.facility),
Expand Down
46 changes: 41 additions & 5 deletions care/emr/resources/scheduling/schedule/spec.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import datetime
from enum import Enum

from pydantic import UUID4, Field
from django.db.models import Sum
from pydantic import UUID4, Field, model_validator
from rest_framework.exceptions import ValidationError
from rest_framework.generics import get_object_or_404

from care.emr.models.scheduling.booking import TokenSlot
from care.emr.models.scheduling.schedule import (
Availability,
SchedulableUserResource,
Expand All @@ -21,10 +24,6 @@ class SlotTypeOptions(str, Enum):
closed = "closed"


class ResourceTypeOptions(str, Enum):
user = "user"


class AvailabilityDateTimeSpec(EMRResource):
day_of_week: int = Field(le=6)
start_time: datetime.time
Expand Down Expand Up @@ -61,6 +60,11 @@ class ScheduleWriteSpec(ScheduleBaseSpec):
valid_to: datetime.datetime
availabilities: list[AvailabilityBaseSpec]

@model_validator(mode="after")
def validate_period(self):
if self.valid_from > self.valid_to:
raise ValidationError("Valid from cannot be greater than valid to")

def perform_extra_deserialization(self, is_update, obj):
if not is_update:
user = get_object_or_404(User, external_id=self.user)
Expand All @@ -82,6 +86,38 @@ class ScheduleUpdateSpec(ScheduleBaseSpec):
valid_from: datetime.datetime
valid_to: datetime.datetime

def perform_extra_deserialization(self, is_update, obj):
old_instance = Schedule.objects.get(id=obj.id)

# Get sum of allocated tokens in old date range
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
)

# Get sum of allocated tokens in new validity range
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
)

if old_allocated_sum != new_allocated_sum:
msg = (
"Cannot modify schedule validity as it would exclude some allocated slots. "
f"Old range has {old_allocated_sum} allocated slots while new range has {new_allocated_sum} allocated slots."
)
raise ValidationError(msg)


class ScheduleReadSpec(ScheduleBaseSpec):
name: str
Expand Down
7 changes: 6 additions & 1 deletion care/emr/resources/scheduling/slot/spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from enum import Enum

from pydantic import UUID4
from rest_framework.exceptions import ValidationError

from care.emr.models import TokenBooking
from care.emr.models.scheduling.booking import TokenSlot
Expand Down Expand Up @@ -64,9 +65,13 @@ class TokenBookingBaseSpec(EMRResource):
__exclude__ = ["token_slot", "patient"]


class TokenBookingUpdateSpec(TokenBookingBaseSpec):
class TokenBookingWriteSpec(TokenBookingBaseSpec):
status: BookingStatusChoices

def perform_extra_deserialization(self, is_update, obj):
if self.status in CANCELLED_STATUS_CHOICES:
raise ValidationError("Cannot cancel a booking. Use the cancel endpoint")


class TokenBookingReadSpec(TokenBookingBaseSpec):
id: UUID4 | None = None
Expand Down
Loading