Merge pull request #1115 from NHSDigital/fix/only-record-dicom-for-in-progress-appointments

Only process DICOM uploads for in progress appointments

Author: steventux

manage_breast_screening/dicom/dicom_recorder.py

@@ -8,16 +8,30 @@
 from django.core.files.uploadedfile import InMemoryUploadedFile
 from PIL import Image as PILImage
 
+from manage_breast_screening.gateway.models import GatewayAction
+
 from .models import Image, Series, Study
 
 logger = logging.getLogger(__name__)
 
 
+class DicomProcessingError(Exception):
+    """Custom exception for DICOM processing errors."""
+
+    pass
+
+
 class DicomRecorder:
     @staticmethod
     def get_or_create_records(
         source_message_id: str, dicom_file: File
     ) -> tuple[Study, Series, Image]:
+        if not __class__.appointment_in_progress(source_message_id):
+            raise DicomProcessingError(
+                f"Cannot process DICOM file for source_message_id={source_message_id} "
+                "because the associated appointment is not in progress."
+            )
+
         ds = pydicom.dcmread(dicom_file)
         study_uid = ds.StudyInstanceUID
         series_uid = ds.SeriesInstanceUID
@@ -102,3 +116,10 @@ def dataset_to_jpeg(sop_uid: str, ds: pydicom.Dataset) -> InMemoryUploadedFile:
             size=in_memory_file.getbuffer().nbytes,
             charset=None,
         )
+
+    @staticmethod
+    def appointment_in_progress(source_message_id: str) -> bool:
+        gateway_action = GatewayAction.objects.filter(id=source_message_id).first()
+        if not gateway_action or not gateway_action.appointment:
+            return False
+        return gateway_action.appointment.current_status.is_in_progress()

manage_breast_screening/dicom/tests/test_api.py

@@ -1,13 +1,14 @@
 import io
 import os
-from unittest.mock import MagicMock
+from unittest.mock import MagicMock, patch
 
 import pydicom
 import pytest
 from django.core.files.uploadedfile import SimpleUploadedFile
 from ninja.testing import TestClient
 
 from manage_breast_screening.core.api import api
+from manage_breast_screening.dicom.dicom_recorder import DicomRecorder
 from manage_breast_screening.dicom.models import Study
 
 os.environ["NINJA_SKIP_REGISTRY"] = "yes"
@@ -30,20 +31,21 @@ def test_upload_success(dataset, dicom_file, monkeypatch):
     monkeypatch.setenv("API_ENABLED", "true")
     monkeypatch.setenv("API_AUTH_TOKEN", "testtoken")
 
-    response = client.put(
-        "/dicom/abc123",
-        FILES={"file": dicom_file},
-        headers={"Authorization": "Bearer " + os.getenv("API_AUTH_TOKEN", "")},
-    )
+    with patch.object(DicomRecorder, "appointment_in_progress", return_value=True):
+        response = client.put(
+            "/dicom/abc123",
+            FILES={"file": dicom_file},
+            headers={"Authorization": "Bearer " + os.getenv("API_AUTH_TOKEN", "")},
+        )
 
-    assert response.status_code == 201
-    assert response.json() == {
-        "study_instance_uid": dataset.StudyInstanceUID,
-        "series_instance_uid": dataset.SeriesInstanceUID,
-        "sop_instance_uid": dataset.SOPInstanceUID,
-        "instance_id": 1,
-    }
-    assert Study.objects.last().source_message_id == "abc123"
+        assert response.status_code == 201
+        assert response.json() == {
+            "study_instance_uid": dataset.StudyInstanceUID,
+            "series_instance_uid": dataset.SeriesInstanceUID,
+            "sop_instance_uid": dataset.SOPInstanceUID,
+            "instance_id": 1,
+        }
+        assert Study.objects.last().source_message_id == "abc123"
 
 
 def test_upload_no_file(monkeypatch):
@@ -67,11 +69,12 @@ def test_upload_invalid_file(monkeypatch):
         "invalid.dcm", b"not a dicom file", content_type="application/dicom"
     )
 
-    response = client.put(
-        "/dicom/abc123",
-        FILES={"file": invalid_file},
-        headers={"Authorization": "Bearer " + os.getenv("API_AUTH_TOKEN", "")},
-    )
+    with patch.object(DicomRecorder, "appointment_in_progress", return_value=True):
+        response = client.put(
+            "/dicom/abc123",
+            FILES={"file": invalid_file},
+            headers={"Authorization": "Bearer " + os.getenv("API_AUTH_TOKEN", "")},
+        )
 
     assert response.status_code == 400
     assert response.json()["title"] == "Invalid DICOM file"
@@ -112,11 +115,12 @@ def test_upload_missing_uids(dataset, monkeypatch):
             "temp.dcm", buffer.read(), content_type="application/dicom"
         )
 
-    response = client.put(
-        "/dicom/abc123",
-        FILES={"file": dicom_file},
-        headers={"Authorization": "Bearer " + os.getenv("API_AUTH_TOKEN", "")},
-    )
+    with patch.object(DicomRecorder, "appointment_in_progress", return_value=True):
+        response = client.put(
+            "/dicom/abc123",
+            FILES={"file": dicom_file},
+            headers={"Authorization": "Bearer " + os.getenv("API_AUTH_TOKEN", "")},
+        )
 
     assert response.status_code == 400
     assert response.json()["title"] == "Missing DICOM attributes"
@@ -127,6 +131,21 @@ def test_upload_missing_uids(dataset, monkeypatch):
     )
 
 
+def test_upload_appointment_not_in_progress(dicom_file, monkeypatch):
+    monkeypatch.setenv("API_ENABLED", "true")
+    monkeypatch.setenv("API_AUTH_TOKEN", "testtoken")
+
+    with patch.object(DicomRecorder, "appointment_in_progress", return_value=False):
+        response = client.put(
+            "/dicom/abc123",
+            FILES={"file": dicom_file},
+            headers={"Authorization": "Bearer " + os.getenv("API_AUTH_TOKEN", "")},
+        )
+
+        assert response.status_code == 500
+        assert response.json()["title"] == "Internal Server Error"
+
+
 @pytest.mark.django_db
 def test_upload_when_api_disabled(dicom_file, monkeypatch):
     monkeypatch.setenv("API_ENABLED", "false")

manage_breast_screening/dicom/tests/test_dicom_recorder.py

@@ -1,22 +1,40 @@
 import tempfile
+import uuid
 from datetime import datetime
 from unittest.mock import patch
 
 import numpy as np
 import pydicom
 import pytest
 
-from manage_breast_screening.dicom.dicom_recorder import DicomRecorder
+from manage_breast_screening.dicom.dicom_recorder import (
+    DicomProcessingError,
+    DicomRecorder,
+)
 from manage_breast_screening.dicom.models import Image, Series, Study
+from manage_breast_screening.gateway.tests.factories import GatewayActionFactory
+from manage_breast_screening.participants.models.appointment import (
+    AppointmentStatusNames,
+)
+from manage_breast_screening.participants.tests.factories import AppointmentFactory
 
 
+@pytest.mark.django_db
 class TestDicomRecorder:
+    @pytest.fixture
+    def gateway_action(self, source_message_id):
+        return GatewayActionFactory(
+            id=source_message_id,
+            appointment=AppointmentFactory(
+                current_status=AppointmentStatusNames.IN_PROGRESS
+            ),
+        )
+
     @pytest.fixture
     def source_message_id(self):
-        return "test-source-message-id"
+        return str(uuid.uuid4())
 
-    @pytest.mark.django_db
-    def test_get_or_create_records(self, source_message_id, dataset):
+    def test_get_or_create_records(self, source_message_id, dataset, gateway_action):
         with tempfile.NamedTemporaryFile() as temp_file:
             pydicom.filewriter.dcmwrite(
                 temp_file.name, dataset, write_like_original=False
@@ -52,8 +70,9 @@ def test_get_or_create_records(self, source_message_id, dataset):
         assert image.image_file.size > 0
         assert image.image_file.storage.exists(image.image_file.name)
 
-    @pytest.mark.django_db
-    def test_get_or_create_records_duplicate(self, source_message_id, dataset):
+    def test_get_or_create_records_duplicate(
+        self, source_message_id, dataset, gateway_action
+    ):
         with tempfile.NamedTemporaryFile() as temp_file:
             pydicom.filewriter.dcmwrite(
                 temp_file.name, dataset, write_like_original=False
@@ -72,7 +91,9 @@ def test_get_or_create_records_duplicate(self, source_message_id, dataset):
                     image,
                 )
 
-    def test_get_or_create_records_invalid_dicom(self, source_message_id):
+    def test_get_or_create_records_invalid_dicom(
+        self, source_message_id, gateway_action
+    ):
         with tempfile.NamedTemporaryFile() as temp_file:
             invalid_ds = pydicom.Dataset()
             invalid_ds.transfer_syntax_uid = pydicom.uid.ExplicitVRLittleEndian
@@ -89,6 +110,25 @@ def test_get_or_create_records_invalid_dicom(self, source_message_id):
                 with pytest.raises(AttributeError):
                     DicomRecorder.get_or_create_records(source_message_id, dicom_file)
 
+    def test_get_or_create_records_appointment_not_in_progress(
+        self, source_message_id, dataset
+    ):
+        GatewayActionFactory(
+            id=source_message_id,
+            appointment=AppointmentFactory(
+                current_status=AppointmentStatusNames.SCREENED
+            ),
+        )
+        with tempfile.NamedTemporaryFile() as temp_file:
+            pydicom.filewriter.dcmwrite(
+                temp_file.name, dataset, write_like_original=False
+            )
+            with open(temp_file.name, "rb") as dicom_file:
+                with pytest.raises(DicomProcessingError):
+                    DicomRecorder.get_or_create_records(source_message_id, dicom_file)
+
+
+class TestJpegConversion:
     def test_dataset_to_jpeg(self, dataset):
         expected_pixel_array = self.expected_pixel_array(dataset)