[NDR-363] Search by default will only return NON-DELETED docs

lambdas/services/document_reference_search_service.py

@@ -43,7 +43,7 @@ def get_document_references(
         :return: List of document references or FHIR DocumentReferences.
         """
         common_name = validate_common_name_in_mtls(
-            api_request_context=api_request_context
+            api_request_context=api_request_context,
         )
         try:
             list_of_table_names = self._get_table_names(common_name)
@@ -92,13 +92,16 @@ def _search_tables_for_documents(
 
         for table_name in table_names:
             logger.info(f"Searching for results in {table_name}")
-            filter_expression = self._get_filter_expression(
-                filters, upload_completed=check_upload_completed
+            filter_expression = self._build_filter_expression(
+                filters,
+                check_upload_completed,
             )
 
             if "coredocumentmetadata" not in table_name.lower():
                 documents = self.fetch_documents_from_table_with_nhs_number(
-                    nhs_number, table_name, query_filter=filter_expression
+                    nhs_number,
+                    table_name,
+                    query_filter=filter_expression,
                 )
             else:
                 documents = self.fetch_documents_from_table(
@@ -112,7 +115,8 @@ def _search_tables_for_documents(
                 self._validate_upload_status(documents)
 
             processed_documents = self._process_documents(
-                documents, return_fhir=return_fhir
+                documents,
+                return_fhir=return_fhir,
             )
             document_resources.extend(processed_documents)
 
@@ -123,16 +127,6 @@ def _search_tables_for_documents(
 
         return document_resources or None
 
-    def _get_filter_expression(
-        self, filters: dict[str, str | None] = None, upload_completed=False
-    ):
-        if filters:
-            return self._build_filter_expression(filters)
-        elif upload_completed:
-            return UploadCompleted
-        else:
-            return None
-
     def _create_fhir_bundle(self, document_resources: list[dict]) -> dict:
         entries = [
             BundleEntry(resource=doc_resource) for doc_resource in document_resources
@@ -155,7 +149,9 @@ def _validate_upload_status(self, documents: list[DocumentReference]):
             raise DocumentRefSearchException(423, LambdaError.UploadInProgressError)
 
     def _process_documents(
-        self, documents: list[DocumentReference], return_fhir: bool
+        self,
+        documents: list[DocumentReference],
+        return_fhir: bool,
     ) -> list[dict]:
         results = []
         for document in documents:
@@ -189,7 +185,16 @@ def _build_document_model(self, document: DocumentReference) -> dict:
         )
         return document_formatted
 
-    def _build_filter_expression(self, filter_values: dict[str, str]):
+    def _build_filter_expression(
+        self,
+        filter_values: dict[str, str] | None,
+        upload_completed=False,
+    ):
+        if not filter_values:
+            if not upload_completed:
+                return NotDeleted
+            return UploadCompleted
+
         filter_builder = DynamoQueryFilterBuilder()
         for filter_key, filter_value in filter_values.items():
             if filter_key == "custodian":
@@ -210,16 +215,13 @@ def _build_filter_expression(self, filter_values: dict[str, str]):
             elif filter_key == "document_snomed_code":
                 filter_builder.add_condition(
                     attribute=str(
-                        DocumentReferenceMetadataFields.DOCUMENT_SNOMED_CODE_TYPE.value
+                        DocumentReferenceMetadataFields.DOCUMENT_SNOMED_CODE_TYPE.value,
                     ),
                     attr_operator=AttributeOperator.EQUAL,
                     filter_value=filter_value,
                 )
-        if filter_values:
-            filter_expression = filter_builder.build() & NotDeleted
-        else:
-            filter_expression = NotDeleted
-        return filter_expression
+
+        return filter_builder.build() & NotDeleted
 
     def create_document_reference_fhir_response(
         self,
@@ -242,7 +244,7 @@ def create_document_reference_fhir_response(
                 attachment=document_details,
                 custodian=document_reference.current_gp_ods,
                 snomed_code_doc_type=SnomedCodes.find_by_code(
-                    document_reference.document_snomed_code_type
+                    document_reference.document_snomed_code_type,
                 ),
             )
             .create_fhir_document_reference_object(document_reference)

lambdas/tests/e2e/api/fhir/test_retrieve_document_fhir_api_failure.py

@@ -1,5 +1,5 @@
-from datetime import datetime, timezone
 import uuid
+from datetime import datetime, timezone
 
 import pytest
 from enums.document_retention import DocumentRetentionDays
@@ -20,11 +20,13 @@ def _assert_operation_outcome(body, code):
 @pytest.mark.parametrize(
     "doc_status, response_status",
     [
-        ("deprecated", 200),  # TODO Fix in NDR-363, this should return a 404
+        ("deprecated", 404),
     ],
 )
 def test_retrieval_of_deleted_document_reference(
-    test_data, doc_status, response_status
+    test_data,
+    doc_status,
+    response_status,
 ):
     deletion_date = datetime.now(timezone.utc)
     document_ttl_days = DocumentRetentionDays.SOFT_DELETE
@@ -33,15 +35,15 @@ def test_retrieval_of_deleted_document_reference(
     pdm_record = create_and_store_pdm_record(
         test_data,
         doc_status=doc_status,
-        deleted=deletion_date.strftime("%Y-%m-%dT%H:%M:%S.%fZ"),
+        Deleted=deletion_date.strftime("%Y-%m-%dT%H:%M:%S.%fZ"),
         ttl=document_reference_ttl,
     )
 
     response = get_pdm_document_reference(record_id=pdm_record["id"])
     assert response.status_code == response_status
 
     response_json = response.json()
-    assert response_json.get("docStatus") == doc_status
+    _assert_operation_outcome(body=response_json, code="RESOURCE_NOT_FOUND")
 
 
 @pytest.mark.parametrize(
@@ -51,7 +53,10 @@ def test_retrieval_of_deleted_document_reference(
     ],
 )
 def test_retrieve_non_existant_document_reference(
-    record_id, expected_status, expected_code, expected_diagnostics
+    record_id,
+    expected_status,
+    expected_code,
+    expected_diagnostics,
 ):
     response = get_pdm_document_reference(record_id)
     assert response.status_code == expected_status
@@ -73,7 +78,9 @@ def test_forbidden_with_invalid_cert(test_data, temp_cert_and_key):
     cert_path, key_path = temp_cert_and_key
 
     response = get_pdm_document_reference(
-        pdm_record["id"], client_cert_path=cert_path, client_key_path=key_path
+        pdm_record["id"],
+        client_cert_path=cert_path,
+        client_key_path=key_path,
     )
 
     body = response.json()
@@ -103,7 +110,10 @@ def test_retrieve_invalid_resource_type(test_data):
     ],
 )
 def test_incorrectly_formatted_path_param_id(
-    test_data, param, expected_status, expected_code
+    test_data,
+    param,
+    expected_status,
+    expected_code,
 ):
     response = get_pdm_document_reference(
         endpoint_override=param,

lambdas/tests/e2e/api/fhir/test_search_patient_fhir_api.py

@@ -1,4 +1,7 @@
+from datetime import datetime, timezone
+
 import pytest
+from enums.document_retention import DocumentRetentionDays
 from tests.e2e.api.fhir.conftest import (
     MTLS_ENDPOINT,
     PDM_SNOMED,
@@ -108,7 +111,10 @@ def test_multiple_cancelled_search_patient_details(test_data):
     ],
 )
 def test_search_edge_cases(
-    nhs_number, expected_status, expected_code, expected_diagnostics
+    nhs_number,
+    expected_status,
+    expected_code,
+    expected_diagnostics,
 ):
     response = search_document_reference(nhs_number)
     assert response.status_code == expected_status
@@ -129,7 +135,9 @@ def test_search_patient_unauthorized_mtls(test_data, temp_cert_and_key):
     cert_path, key_path = temp_cert_and_key
 
     response = search_document_reference(
-        "9912003071", client_cert_path=cert_path, client_key_path=key_path
+        "9912003071",
+        client_cert_path=cert_path,
+        client_key_path=key_path,
     )
 
     body = response.json()
@@ -142,3 +150,49 @@ def test_search_invalid_resource_type(test_data):
 
     response = search_document_reference("9912003071", resource_type="FooBar")
     assert response.status_code == 400
+
+
+def test_search_patient_details_deleted_are_not_returned(test_data):
+    created_record_1 = create_and_store_pdm_record(test_data)
+    expected_record_id_1 = created_record_1["id"]
+
+    deletion_date = datetime.now(timezone.utc)
+    document_ttl_days = DocumentRetentionDays.SOFT_DELETE
+    ttl_seconds = document_ttl_days * 24 * 60 * 60
+    document_reference_ttl = int(deletion_date.timestamp() + ttl_seconds)
+    created_record_2 = create_and_store_pdm_record(
+        test_data,
+        doc_status="deprecated",
+        Deleted=deletion_date.strftime("%Y-%m-%dT%H:%M:%S.%fZ"),
+        ttl=document_reference_ttl,
+    )
+    expected_record_id_2 = created_record_2["id"]
+
+    response = search_document_reference("9912003071")
+    assert response.status_code == 200
+
+    bundle = response.json()
+    assert bundle["total"] < 2
+    entries = bundle.get("entry", [])
+    assert entries
+
+    # Find the entry with the matching record_id
+    matching_entry = next(
+        (
+            e
+            for e in entries
+            if e["resource"].get("id") == f"{PDM_SNOMED}~{expected_record_id_1}"
+        ),
+        None,
+    )
+    assert matching_entry
+    # Assert deleted item doesn't exist
+    non_matching_entry = next(
+        (
+            e
+            for e in entries
+            if e["resource"].get("id") == f"{PDM_SNOMED}~{expected_record_id_2}"
+        ),
+        None,
+    )
+    assert non_matching_entry is None

lambdas/tests/e2e/api/test_search_patient_api.py

@@ -43,7 +43,7 @@ def test_search_patient_details(test_data, snapshot_json):
             "entry.0.resource.date",
             "entry.0.resource.content.0.attachment.url",
             "timestamp",
-        )
+        ),
     )
 
 
@@ -81,13 +81,17 @@ def test_multiple_cancelled_search_patient_details(test_data, snapshot_json):
 
     assert bundle["entry"][0] == snapshot_json(
         exclude=paths(
-            "resource.id", "resource.date", "resource.content.0.attachment.url"
-        )
+            "resource.id",
+            "resource.date",
+            "resource.content.0.attachment.url",
+        ),
     )
     assert bundle["entry"][1] == snapshot_json(
         exclude=paths(
-            "resource.id", "resource.date", "resource.content.0.attachment.url"
-        )
+            "resource.id",
+            "resource.date",
+            "resource.content.0.attachment.url",
+        ),
     )
 
 
@@ -110,7 +114,7 @@ def test_no_records(snapshot_json):
             "entry.0.resource.date",
             "entry.0.resource.content.0.attachment.url",
             "timestamp",
-        )
+        ),
     )
 
 
@@ -128,3 +132,70 @@ def test_invalid_patient(snapshot_json):
     bundle = response.json()
 
     assert bundle == snapshot_json
+
+
+def test_search_patient_details_deleted_are_not_returned(test_data):
+    lloyd_george_record = {}
+    test_data.append(lloyd_george_record)
+
+    lloyd_george_record["id"] = str(uuid.uuid4())
+    lloyd_george_record["nhs_number"] = "9449305943"
+    lloyd_george_record["data"] = io.BytesIO(b"Sample PDF Content")
+
+    data_helper.create_metadata(lloyd_george_record)
+    data_helper.create_resource(lloyd_george_record)
+
+    second_lloyd_george_record = {}
+    test_data.append(second_lloyd_george_record)
+
+    second_lloyd_george_record["id"] = str(uuid.uuid4())
+    second_lloyd_george_record["nhs_number"] = "9449305943"
+    second_lloyd_george_record["data"] = io.BytesIO(b"Sample PDF Content")
+
+    data_helper.create_metadata(second_lloyd_george_record)
+    data_helper.create_resource(second_lloyd_george_record)
+
+    url = f"https://{API_ENDPOINT}/FhirDocumentReference?subject:identifier=https://fhir.nhs.uk/Id/nhs-number|{lloyd_george_record['nhs_number']}&_id={second_lloyd_george_record['id']}"
+    headers = {
+        "Authorization": "Bearer 123",
+        "X-Api-Key": API_KEY,
+        "X-Correlation-Id": "1234",
+    }
+
+    delete_response = requests.request("DELETE", url, headers=headers)
+    assert delete_response.status_code == 204
+
+    url = f"https://{API_ENDPOINT}/FhirDocumentReference?subject:identifier=https://fhir.nhs.uk/Id/nhs-number|{lloyd_george_record['nhs_number']}"
+    headers = {
+        "Authorization": "Bearer 123",
+        "X-Api-Key": API_KEY,
+        "X-Correlation-Id": "1234",
+    }
+    response = requests.request("GET", url, headers=headers)
+    bundle = response.json()
+    assert bundle["total"] < 2
+    entries = bundle.get("entry", [])
+    assert entries
+
+    # Find the entry with the matching record_id
+    matching_entry = next(
+        (
+            e
+            for e in entries
+            if e["resource"].get("id")
+            == f"{LLOYD_GEORGE_SNOMED}~{lloyd_george_record['id']}"
+        ),
+        None,
+    )
+    assert matching_entry
+    # Assert deleted item doesn't exist
+    non_matching_entry = next(
+        (
+            e
+            for e in entries
+            if e["resource"].get("id")
+            == f"{LLOYD_GEORGE_SNOMED}~{second_lloyd_george_record['id']}"
+        ),
+        None,
+    )
+    assert non_matching_entry is None