fix(brain): paginate the patient list before building its rows (#97)

brain/tests_patient_list_upload.py

@@ -1,9 +1,11 @@
 from django.contrib.auth.models import User
+from django.db import connection
 from django.test import TestCase
+from django.test.utils import CaptureQueriesContext
 from django.urls import reverse
 
 from brain.models import Folder, Patient
-from common.models import FileRegistry, Modality, Project, ProjectAccess
+from common.models import FileRegistry, Job, Modality, Project, ProjectAccess
 
 
 class BrainPatientListTests(TestCase):
@@ -41,6 +43,76 @@ def test_defaults_to_ten_and_applies_modality_status_filter(self):
         self.assertEqual(len(response.context["page_obj"].object_list), 10)
         self.assertEqual([item["patient"] for item in filtered.context["page_obj"].object_list], [patients[0]])
 
+    def _add_patients(self, count, processed=False):
+        patients = []
+        for index in range(count):
+            patient = Patient.objects.create(name=f"Brain extra {index}", project=self.project, folder=self.folder)
+            if processed:
+                FileRegistry.objects.create(
+                    brain_patient=patient,
+                    domain="brain",
+                    modality=self.modality,
+                    file_type="braintumor_mri_t1_raw",
+                    file_path=f"brain/tests/{patient.pk}/t1.nii.gz",
+                    file_size=2,
+                    file_hash=f"{patient.pk:064d}",
+                )
+                Job.objects.create(
+                    domain="brain",
+                    brain_patient=patient,
+                    modality_slug=self.modality.slug,
+                    input_files={"input": f"brain/tests/{patient.pk}/t1.nii.gz"},
+                    status="completed",
+                )
+            patients.append(patient)
+        return patients
+
+    def _queries_for(self, params=None):
+        # The first request of a test also saves the session; keep that out of the count.
+        self.client.get(reverse("brain:patient_list"), params or {})
+        with CaptureQueriesContext(connection) as captured:
+            response = self.client.get(reverse("brain:patient_list"), params or {})
+        self.assertEqual(response.status_code, 200)
+        return len(captured.captured_queries)
+
+    def test_page_cost_does_not_grow_with_the_project(self):
+        """#97: rows used to be built for every patient before paginating."""
+        self._add_patients(10, processed=True)
+        small = self._queries_for()
+        self._add_patients(30, processed=True)
+        self.assertEqual(self._queries_for(), small)
+
+    def test_status_filter_page_cost_does_not_grow_with_the_project(self):
+        """The filtered path still reads every candidate, but builds rows for the page only."""
+        params = {"status_braintumor-mri-t1": "processed"}
+        self._add_patients(10, processed=True)
+        small = self._queries_for(params)
+        self._add_patients(30, processed=True)
+        self.assertEqual(self._queries_for(params), small)
+
+    def test_second_page_without_filters(self):
+        self._add_patients(11)
+        response = self.client.get(reverse("brain:patient_list"), {"page": 2})
+        page_obj = response.context["page_obj"]
+        self.assertEqual(page_obj.number, 2)
+        self.assertEqual(page_obj.paginator.num_pages, 2)
+        self.assertEqual(page_obj.paginator.count, 11)
+        self.assertEqual(len(page_obj.object_list), 1)
+        self.assertIn("modality_status_list", page_obj.object_list[0])
+
+    def test_status_filter_paginates_the_matches(self):
+        processed = self._add_patients(12, processed=True)
+        self._add_patients(5)
+        response = self.client.get(
+            reverse("brain:patient_list"), {"status_braintumor-mri-t1": "processed", "page": 2}
+        )
+        page_obj = response.context["page_obj"]
+        self.assertEqual(page_obj.paginator.count, 12)
+        self.assertEqual(len(page_obj.object_list), 2)
+        for item in page_obj.object_list:
+            self.assertIn(item["patient"], processed)
+            self.assertEqual(item["modality_statuses"]["braintumor-mri-t1"], "processed")
+
     def test_invalid_page_size_falls_back_to_ten(self):
         response = self.client.get(reverse("brain:patient_list"), {"per_page": "200"})
         self.assertEqual(response.context["per_page"], 10)

brain/views.py

@@ -17,6 +17,7 @@
 from common.export_share import is_share_expired
 from common.file_access import exists as artifact_exists
 from common.modality_config import (
+    modality_requires_processing,
     modality_status,
     rerun_step_labels,
     rerunnable_steps_for_patient,
@@ -253,9 +254,67 @@ def patient_detail(request, patient_id):
     return render_with_fallback(request, "patient_detail", context)
 
 
+def _modality_status_list(patient, allowed_modalities, requires_processing):
+    """Per-modality status for one list row, from the prefetched files and jobs only.
+
+    ``requires_processing`` maps slug -> `modality_requires_processing`, looked up
+    once per request rather than once per patient.
+    """
+    files_by_modality = {}
+    for file_obj in patient.files.all():
+        if file_obj.modality and file_obj.modality.slug:
+            files_by_modality.setdefault(file_obj.modality.slug, []).append(file_obj)
+    jobs_by_modality = {}
+    for job in patient.jobs.all():
+        jobs_by_modality.setdefault(job.modality_slug, []).append(job)
+    modality_status_list = []
+    for modality in allowed_modalities:
+        slug = modality.slug or ""
+        if slug in {"rawzip", "voice"}:
+            continue
+        status = modality_status(
+            slug,
+            jobs_by_modality.get(slug, []),
+            bool(files_by_modality.get(slug)),
+            requires_processing=requires_processing.get(slug),
+        )
+        modality_status_list.append({
+            "slug": slug,
+            "name": modality.name,
+            "icon": modality.icon or "",
+            "label": modality.label or "",
+            "status": status,
+        })
+    return modality_status_list
+
+
+def _patient_row(request, patient, modality_status_list):
+    """The template's row for one patient.
+
+    Unlike `_modality_status_list` this issues queries of its own (the rerun
+    picker's steps, the delete permission), so it is only ever run for the rows
+    on the page being shown -- see #97.
+    """
+    voice_captions = list(patient.voice_captions.all())
+    return {
+        "patient": patient,
+        "voice_caption_processing": any(vc.processing_status in ["pending", "processing"] for vc in voice_captions),
+        "voice_caption_processed": bool(voice_captions) and all(vc.processing_status == "completed" for vc in voice_captions),
+        "voice_caption_count": len(voice_captions),
+        "voice_annotators": list({vc.user.username for vc in voice_captions}),
+        "tags": patient.tag_names(),
+        "folder": patient.folder,
+        "available_modalities": [m.slug for m in patient.modalities.all()],
+        "modality_statuses": {item["slug"]: item["status"] for item in modality_status_list},
+        "modality_status_list": modality_status_list,
+        "rerunnable_steps": rerunnable_steps_for_patient(list(patient.files.all()), modality_status_list, patient=patient),
+        "can_delete": bool(user_is_project_admin(request.user, patient.project) or user_can_delete_single_patient(request.user, patient.folder, patient.project)),
+    }
+
+
 @login_required
 def patient_list(request):
-    patients = Patient.objects.select_related("uploaded_by").prefetch_related(
+    patients = Patient.objects.select_related("uploaded_by", "project", "folder").prefetch_related(
         "voice_captions",
         "voice_captions__user",
         "tags",
@@ -313,65 +372,41 @@ def patient_list(request):
             if value in {"processed", "processing", "failed"}:
                 status_filters[slug] = value
 
-    patients_with_status = []
     # UI flag for the project the list is showing; each row is checked on its own project.
     is_admin = user_is_project_admin(request.user, session_project(request))
-    for patient in patients:
-        voice_captions = list(patient.voice_captions.all())
-        patient_files = list(patient.files.all())
-        patient_jobs = list(patient.jobs.all()) if hasattr(patient, "jobs") else []
-        files_by_modality = {}
-        for file_obj in patient_files:
-            if file_obj.modality and file_obj.modality.slug:
-                files_by_modality.setdefault(file_obj.modality.slug, []).append(file_obj)
-        jobs_by_modality = {}
-        for job in patient_jobs:
-            jobs_by_modality.setdefault(job.modality_slug, []).append(job)
-        modality_status_list = []
-        for modality in allowed_modalities:
-            slug = modality.slug or ""
-            if slug in {"rawzip", "voice"}:
-                continue
-            status = modality_status(
-                slug,
-                jobs_by_modality.get(slug, []),
-                bool(files_by_modality.get(slug)),
-            )
-            modality_status_list.append({
-                "slug": slug,
-                "name": modality.name,
-                "icon": modality.icon or "",
-                "label": modality.label or "",
-                "status": status,
-            })
-        patients_with_status.append({
-            "patient": patient,
-            "voice_caption_processing": any(vc.processing_status in ["pending", "processing"] for vc in voice_captions),
-            "voice_caption_processed": bool(voice_captions) and all(vc.processing_status == "completed" for vc in voice_captions),
-            "voice_caption_count": len(voice_captions),
-            "voice_annotators": list({vc.user.username for vc in voice_captions}),
-            "tags": patient.tag_names(),
-            "folder": patient.folder,
-            "available_modalities": [m.slug for m in patient.modalities.all()],
-            "modality_statuses": {item["slug"]: item["status"] for item in modality_status_list},
-            "modality_status_list": modality_status_list,
-            "rerunnable_steps": rerunnable_steps_for_patient(patient_files, modality_status_list, patient=patient),
-            "can_delete": bool(user_is_project_admin(request.user, patient.project) or user_can_delete_single_patient(request.user, patient.folder, patient.project)),
-        })
-
-    if status_filters:
-        patients_with_status = [
-            item for item in patients_with_status
-            if all(item["modality_statuses"].get(slug, "absent") == value for slug, value in status_filters.items())
-        ]
 
     try:
         per_page = int(request.GET.get("per_page", 10))
     except (TypeError, ValueError):
         per_page = 10
     if per_page not in {10, 20, 50, 100}:
         per_page = 10
-    page_obj = Paginator(patients_with_status, per_page).get_page(request.GET.get("page"))
+    page_number = request.GET.get("page")
+    requires_processing = {m.slug: modality_requires_processing(m.slug) for m in allowed_modalities if m.slug}
+
+    # Paginate before building rows: a row costs queries of its own, so building
+    # one per patient in the project made every page as slow as the whole
+    # project (#97). A status filter is computed, not stored, so it still has to
+    # look at every matching patient -- but only through the in-memory status,
+    # and the full row is still built for the page alone.
+    if status_filters:
+        candidates = []
+        for patient in patients:
+            modality_status_list = _modality_status_list(patient, allowed_modalities, requires_processing)
+            statuses = {item["slug"]: item["status"] for item in modality_status_list}
+            if all(statuses.get(slug, "absent") == value for slug, value in status_filters.items()):
+                candidates.append((patient, modality_status_list))
+        page_obj = Paginator(candidates, per_page).get_page(page_number)
+        page_obj.object_list = [
+            _patient_row(request, patient, modality_status_list)
+            for patient, modality_status_list in page_obj.object_list
+        ]
+    else:
+        page_obj = Paginator(patients, per_page).get_page(page_number)
+        page_obj.object_list = [
+            _patient_row(request, patient, _modality_status_list(patient, allowed_modalities, requires_processing))
+            for patient in page_obj.object_list
+        ]
     project_id = current_project_id
     folders = filter_folders_for_user(
         request.user,

common/modality_config.py

@@ -88,7 +88,7 @@ def modality_is_blocking(modality_slug):
     return modality_requires_processing(modality_slug)
 
 
-def modality_status(modality_slug, jobs, has_files):
+def modality_status(modality_slug, jobs, has_files, requires_processing=None):
     """The patient-row status for one modality's pill.
 
     ``'failed' | 'processing' | 'pending' | 'processed' | 'absent'``, in that
@@ -100,8 +100,14 @@ def modality_status(modality_slug, jobs, has_files):
     makes it green. That is also what clears the debris -- a patient uploaded
     while a step-less modality still spawned a Job carries a 'failed' row that
     no rerun can ever complete, because there is no algo to run.
+
+    ``requires_processing`` is :func:`modality_requires_processing` for the slug,
+    for callers that status many patients at once: it is a query, and asking it
+    per patient made a list's cost grow with the project (#97).
     """
-    if modality_requires_processing(modality_slug):
+    if requires_processing is None:
+        requires_processing = modality_requires_processing(modality_slug)
+    if requires_processing:
         statuses = {getattr(job, "status", "") for job in jobs or ()}
         if "failed" in statuses:
             return "failed"