diff --git a/lambdas/services/document_reference_search_service.py b/lambdas/services/document_reference_search_service.py index a4046bb45..b40fa8b54 100644 --- a/lambdas/services/document_reference_search_service.py +++ b/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) diff --git a/lambdas/tests/e2e/api/fhir/test_retrieve_document_fhir_api_failure.py b/lambdas/tests/e2e/api/fhir/test_retrieve_document_fhir_api_failure.py index 0640a412b..e1cfebe33 100644 --- a/lambdas/tests/e2e/api/fhir/test_retrieve_document_fhir_api_failure.py +++ b/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,7 +35,7 @@ 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, ) @@ -41,7 +43,7 @@ def test_retrieval_of_deleted_document_reference( 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, diff --git a/lambdas/tests/e2e/api/fhir/test_search_patient_fhir_api.py b/lambdas/tests/e2e/api/fhir/test_search_patient_fhir_api.py index b2022432e..92783b115 100644 --- a/lambdas/tests/e2e/api/fhir/test_search_patient_fhir_api.py +++ b/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 diff --git a/lambdas/tests/e2e/api/test_search_patient_api.py b/lambdas/tests/e2e/api/test_search_patient_api.py index ad7c7c79f..2a28349c6 100644 --- a/lambdas/tests/e2e/api/test_search_patient_api.py +++ b/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