diff --git a/brain/tests_patient_list_upload.py b/brain/tests_patient_list_upload.py index 47063820..ba42d8af 100644 --- a/brain/tests_patient_list_upload.py +++ b/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) diff --git a/brain/views.py b/brain/views.py index 38720656..8e6a5532 100644 --- a/brain/views.py +++ b/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,57 +372,8 @@ 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)) @@ -371,7 +381,32 @@ def patient_list(request): 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, diff --git a/common/modality_config.py b/common/modality_config.py index f050cc85..a9518c13 100644 --- a/common/modality_config.py +++ b/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"