Merge pull request #153 from NHSDigital/dtos-9252-optimize-fetching

MatMoore · web-flow · commit b322560f10e3 · 2025-06-25T16:12:23.000+01:00
[DTOS-9252] Optimise queries a bit
diff --git a/manage_breast_screening/clinics/models.py b/manage_breast_screening/clinics/models.py
@@ -97,8 +97,9 @@ class TimeOfDay:
 
     objects = ClinicQuerySet.as_manager()
 
+    @property
     def current_status(self):
-        return self.statuses.first()
+        return self.statuses.order_by("-created_at").first()
 
     def session_type(self):
         start_hour = self.starts_at.hour
diff --git a/manage_breast_screening/clinics/presenters.py b/manage_breast_screening/clinics/presenters.py
@@ -1,3 +1,5 @@
+from functools import cached_property
+
 from django.urls import reverse
 
 from ..core.utils.date_formatting import format_date, format_time_range
@@ -12,7 +14,7 @@ def __init__(self, filtered_clinics, filter, counts_by_filter):
         self.counts_by_filter = counts_by_filter
         self.filter = filter
 
-    @property
+    @cached_property
     def heading(self):
         if self.filter == "today":
             return "Today’s clinics"
@@ -42,9 +44,9 @@ def __init__(self, clinic):
         self.type = clinic.get_type_display()
         self.risk_type = clinic.get_risk_type_display()
 
-    @property
+    @cached_property
     def state(self):
-        status = self._clinic.current_status()
+        status = self._clinic.current_status
         value = status.state
         text = status.get_state_display()
 
@@ -53,7 +55,7 @@ def state(self):
             "classes": "nhsuk-tag--" + self.STATUS_COLORS[value],
         }
 
-    @property
+    @cached_property
     def setting_name(self):
         return self._clinic.setting.name
 
@@ -67,7 +69,7 @@ def __init__(self, clinic_id, appointments, filter, counts_by_filter):
         self.counts_by_filter = counts_by_filter
         self.clinic_id = clinic_id
 
-    @property
+    @cached_property
     def secondary_nav_data(self):
         filters = [
             {
diff --git a/manage_breast_screening/clinics/tests/test_models.py b/manage_breast_screening/clinics/tests/test_models.py
@@ -13,7 +13,9 @@
 @pytest.mark.django_db
 def test_clinic_current_status():
     clinic = ClinicFactory.create(current_status=models.ClinicStatus.SCHEDULED)
-    assert clinic.current_status().state == models.ClinicStatus.SCHEDULED
+    clinic.statuses.create(state=models.ClinicStatus.CANCELLED)
+    assert clinic.statuses.first().state == models.ClinicStatus.CANCELLED
+    assert clinic.current_status.state == models.ClinicStatus.CANCELLED
 
 
 @pytest.mark.django_db
diff --git a/manage_breast_screening/clinics/tests/test_presenters.py b/manage_breast_screening/clinics/tests/test_presenters.py
@@ -24,7 +24,7 @@ def mock_clinic():
     }
     mock.get_type_display.return_value = "Screening"
     mock.get_risk_type_display.return_value = "Routine"
-    mock.current_status.return_value = ClinicStatusFactory.build()
+    mock.current_status = ClinicStatusFactory.build()
 
     return mock
 
diff --git a/manage_breast_screening/clinics/views.py b/manage_breast_screening/clinics/views.py
@@ -18,11 +18,12 @@ def clinic_list(request, filter="today"):
 
 
 def clinic(request, id, filter="remaining"):
-    clinic = Clinic.objects.prefetch_related("setting").get(id=id)
+    clinic = Clinic.objects.select_related("setting").get(id=id)
     presented_clinic = ClinicPresenter(clinic)
     appointments = (
         Appointment.objects.for_clinic_and_filter(clinic, filter)
-        .select_related("clinic_slot", "screening_episode__participant")
+        .prefetch_related("statuses")
+        .select_related("clinic_slot__clinic", "screening_episode__participant")
         .order_by("clinic_slot__starts_at")
     )
     counts_by_filter = Appointment.objects.filter_counts_for_clinic(clinic)
diff --git a/manage_breast_screening/config/settings.py b/manage_breast_screening/config/settings.py
@@ -74,6 +74,14 @@ def boolean_env(key, default=None):
     "django.middleware.clickjacking.XFrameOptionsMiddleware",
 ]
 
+if DEBUG:
+    INTERNAL_IPS = ["127.0.0.1"]
+
+DEBUG_TOOLBAR = DEBUG
+if DEBUG_TOOLBAR:
+    INSTALLED_APPS.append("debug_toolbar")
+    MIDDLEWARE.insert(0, "debug_toolbar.middleware.DebugToolbarMiddleware")
+
 ROOT_URLCONF = "manage_breast_screening.core.urls"
 
 TEMPLATES = [
diff --git a/manage_breast_screening/config/settings_test.py b/manage_breast_screening/config/settings_test.py
@@ -10,6 +10,12 @@
     },
 }
 
+
 MIDDLEWARE.remove(
     "whitenoise.middleware.WhiteNoiseMiddleware",
 )
+
+if DEBUG:
+    INSTALLED_APPS.remove("debug_toolbar")
+    MIDDLEWARE.remove("debug_toolbar.middleware.DebugToolbarMiddleware")
+    DEBUG_TOOLBAR = False
diff --git a/manage_breast_screening/core/urls.py b/manage_breast_screening/core/urls.py
@@ -15,6 +15,7 @@
     2. Add a URL to urlpatterns:  path('blog/', include('blog.urls'))
 """
 
+from django.conf import settings
 from django.contrib import admin
 from django.urls import include, path
 from django.views.generic.base import RedirectView
@@ -37,3 +38,10 @@
     ),
     path("", RedirectView.as_view(pattern_name="clinics:index"), name="home"),
 ]
+
+if settings.DEBUG_TOOLBAR:
+    from debug_toolbar.toolbar import debug_toolbar_urls
+
+    urlpatterns = [
+        *urlpatterns,
+    ] + debug_toolbar_urls()
diff --git a/manage_breast_screening/mammograms/presenters.py b/manage_breast_screening/mammograms/presenters.py
@@ -1,3 +1,5 @@
+from functools import cached_property
+
 from django.urls import reverse
 
 from ..core.utils.date_formatting import format_date, format_relative_date, format_time
@@ -42,9 +44,9 @@ def present_secondary_nav(id):
 
 
 class AppointmentPresenter:
-    def __init__(self, appointment):
+    def __init__(self, appointment, last_known_screening=None):
         self._appointment = appointment
-        self._last_known_screening = appointment.screening_episode.previous()
+        self._last_known_screening = last_known_screening
 
         self.allStatuses = AppointmentStatus
         self.id = appointment.id
@@ -53,17 +55,18 @@ def __init__(self, appointment):
             appointment.screening_episode.participant
         )
 
-    @property
+    @cached_property
     def participant_url(self):
         return self.participant.url
 
-    @property
+    @cached_property
     def start_time(self):
         return self.clinic_slot.starts_at
 
-    @property
+    @cached_property
     def current_status(self):
         current_status = self._appointment.current_status
+
         colour = status_colour(current_status.state)
 
         return {
@@ -73,7 +76,7 @@ def current_status(self):
             "is_confirmed": current_status.state == AppointmentStatus.CONFIRMED,
         }
 
-    @property
+    @cached_property
     def last_known_screening(self):
         return (
             {
@@ -98,17 +101,17 @@ def __init__(self, clinic_slot):
 
         self.clinic_id = self._clinic.id
 
-    @property
+    @cached_property
     def clinic_type(self):
         return self._clinic.get_type_display().capitalize()
 
-    @property
+    @cached_property
     def slot_time_and_clinic_date(self):
         clinic_slot = self._clinic_slot
         clinic = self._clinic
 
         return f"{format_time(clinic_slot.starts_at)} ({clinic_slot.duration_in_minutes} minutes) - {format_date(clinic.starts_at)} ({format_relative_date(clinic.starts_at)})"
 
-    @property
+    @cached_property
     def starts_at(self):
         return format_time(self._clinic_slot.starts_at)
diff --git a/manage_breast_screening/mammograms/tests/test_presenters.py b/manage_breast_screening/mammograms/tests/test_presenters.py
@@ -72,9 +72,9 @@ def test_status(
     def test_last_known_screening(self, mock_appointment):
         mock_screening = MagicMock(spec=ScreeningEpisode)
         mock_screening.created_at = datetime(2015, 1, 1)
-        mock_appointment.screening_episode.previous.return_value = mock_screening
+        last_known_screening = mock_screening
 
-        result = AppointmentPresenter(mock_appointment)
+        result = AppointmentPresenter(mock_appointment, last_known_screening)
 
         assert result.last_known_screening == {
             "date": "1 January 2015",
diff --git a/manage_breast_screening/mammograms/views.py b/manage_breast_screening/mammograms/views.py
@@ -43,7 +43,8 @@ def get_context_data(self, **kwargs):
         context = super().get_context_data(**kwargs)
 
         appointment = self.get_appointment()
-        presenter = AppointmentPresenter(appointment)
+        last_known_screening = appointment.screening_episode.previous()
+        presenter = AppointmentPresenter(appointment, last_known_screening)
 
         context.update(
             {
diff --git a/manage_breast_screening/participants/models.py b/manage_breast_screening/participants/models.py
@@ -1,13 +1,15 @@
 import uuid
 from datetime import date
+from logging import getLogger
 
 from django.contrib.postgres.fields import ArrayField
 from django.db import models
 from django.db.models import OuterRef, Subquery
-from django.utils.functional import cached_property
 
 from ..core.models import BaseModel
 
+logger = getLogger(__name__)
+
 # List of ethnic groups from
 # https://design-system.service.gov.uk/patterns/equality-information/
 # This list is specific to England.
@@ -178,9 +180,24 @@ class Appointment(BaseModel):
     reinvite = models.BooleanField(default=False)
     stopped_reasons = models.JSONField(null=True, blank=True)
 
-    @cached_property
-    def current_status(self):
-        return self.statuses.first()
+    @property
+    def current_status(self) -> "AppointmentStatus":
+        """
+        Fetch the most recent status associated with this appointment.
+        If there are no statuses for any reason, assume the default one.
+        """
+        # avoid `first()` here so that `statuses` can be prefetched
+        # when fetching many appointments
+        statuses = list(self.statuses.order_by("-created_at").all())
+
+        if not statuses:
+            status = AppointmentStatus()
+            logger.info(
+                f"Appointment {self.pk} has no statuses. Assuming {status.state}"
+            )
+            return status
+
+        return statuses[0]
 
 
 class AppointmentStatus(models.Model):
@@ -208,3 +225,6 @@ class AppointmentStatus(models.Model):
     appointment = models.ForeignKey(
         Appointment, on_delete=models.PROTECT, related_name="statuses"
     )
+
+    class Meta:
+        ordering = ["-created_at"]
diff --git a/manage_breast_screening/participants/tests/test_models.py b/manage_breast_screening/participants/tests/test_models.py
@@ -167,3 +167,14 @@ def test_filter_counts_for_clinic(self):
         assert counts["checked_in"] == 1
         assert counts["complete"] == 2
         assert counts["all"] == 5
+
+
+@pytest.mark.django_db
+def test_appointment_current_status():
+    appointment = AppointmentFactory.create(
+        current_status=models.AppointmentStatus.CONFIRMED
+    )
+    appointment.statuses.create(state=models.AppointmentStatus.CHECKED_IN)
+
+    assert appointment.statuses.first().state == models.AppointmentStatus.CHECKED_IN
+    assert appointment.current_status.state == models.AppointmentStatus.CHECKED_IN
diff --git a/poetry.lock b/poetry.lock
diff --git a/pyproject.toml b/pyproject.toml
@@ -30,6 +30,7 @@ pytest-playwright = "^0.7.0"
 ipdb = "^0.13.13"
 axe-playwright-python = "^0.1.5"
 ruff = "^0.12.0"
+django-debug-toolbar = "^5.2.0"
 
 [build-system]
 requires = ["poetry-core>=2.0.0,<3.0.0"]
@@ -46,4 +47,4 @@ markers = ["system: mark a test as a system test"]
 exclude = ["*/migrations/*.py"]
 
 # On top of the defaults (`E4`, E7`, `E9`, and `F`), enable isort (`I`)
-select = ["E4", "E7", "E9", "F", "I"]
+lint.select = ["E4", "E7", "E9", "F", "I"]

Original file line number	Diff line number	Diff line change
`@@ -24,7 +24,7 @@ def mock_clinic():`
`24`	`24`	`}`
`25`	`25`	`mock.get_type_display.return_value = "Screening"`
`26`	`26`	`mock.get_risk_type_display.return_value = "Routine"`
`27`		`- mock.current_status.return_value = ClinicStatusFactory.build()`
	`27`	`+ mock.current_status = ClinicStatusFactory.build()`
`28`	`28`
`29`	`29`	`return mock`
`30`	`30`
Original file line number	Diff line number	Diff line change
`@@ -10,6 +10,12 @@`
`10`	`10`	`},`
`11`	`11`	`}`
`12`	`12`
	`13`	`+`
`13`	`14`	`MIDDLEWARE.remove(`
`14`	`15`	`"whitenoise.middleware.WhiteNoiseMiddleware",`
`15`	`16`	`)`
	`17`	`+`
	`18`	`+if DEBUG:`
	`19`	`+ INSTALLED_APPS.remove("debug_toolbar")`
	`20`	`+ MIDDLEWARE.remove("debug_toolbar.middleware.DebugToolbarMiddleware")`
	`21`	`+ DEBUG_TOOLBAR = False`