-
Notifications
You must be signed in to change notification settings - Fork 1
[CDAPI-152]: Unhappy path integration tests #121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
1af10fc
7126038
38e73b9
32cbee5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,21 +73,40 @@ def _raise_error(status_code: int, error_text: str, error_code: str) -> RequestH | |
| } | ||
|
|
||
|
|
||
| def _get_patient_id_from_resource(resource: dict[str, Any]) -> Any: | ||
| resource_type = resource.get("resourceType") | ||
| if resource_type != "Composition" and resource_type != "Patient": | ||
| return None | ||
|
|
||
| if resource_type == "Composition": | ||
| resource = resource.get("subject", {}) | ||
|
|
||
| if "identifier" not in resource or len(resource["identifier"]) == 0: | ||
| return None | ||
|
|
||
| patient = ( | ||
| resource["identifier"][0].get("value") | ||
| if type(resource["identifier"]) is list | ||
| else resource["identifier"].get("value") | ||
| ) | ||
| if not patient: | ||
| return None | ||
|
|
||
| return patient | ||
|
|
||
|
|
||
| def _fetch_patient_from_payload(payload: dict[str, Any]) -> str | None: | ||
| patient_values = [ | ||
| str(patient) | ||
| for entry in payload.get("entry", []) | ||
| if (resource := entry.get("resource")) | ||
| and resource.get("resourceType") == "Patient" | ||
| and "identifier" in resource | ||
| and len(resource["identifier"]) > 0 | ||
| and (patient := resource.get("identifier")[0].get("value")) | ||
| and (patient := _get_patient_id_from_resource(resource)) | ||
| ] | ||
|
|
||
| if not patient_values: | ||
| return None | ||
|
|
||
| if len(patient_values) > 1: | ||
| if len(set(patient_values)) > 1: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you create a set from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adding onto the above point, if a patient identifier is supplied as part of both composition subject and patient resource then its fine if they are the same value, pdm allows for that. |
||
| raise ValueError("Multiple patients referenced within the same bundle") | ||
|
|
||
| return str(patient_values[0]) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -193,6 +193,75 @@ def test_handle_post_request_multiple_patients( | |
|
|
||
| assert not boto3_resource_mock.called | ||
|
|
||
| @patch("common.storage_helper.StorageHelper.put_item") | ||
| @patch("pdm_mock.handler.uuid4") | ||
| @patch("pdm_mock.handler.datetime") | ||
| @pytest.mark.parametrize( | ||
| ("payload_entry", "expected_status_code"), | ||
| [ | ||
| pytest.param({"resource": {"resourceType": "Observation"}}, 201), | ||
| pytest.param( | ||
| { | ||
| "resource": { | ||
| "resourceType": "Composition", | ||
| "subject": {"identifier": {"value": "test_number"}}, | ||
| } | ||
| }, | ||
| 201, | ||
| ), | ||
| pytest.param({"resource": {"resourceType": "Patient"}}, 201), | ||
| pytest.param( | ||
| { | ||
| "resource": { | ||
| "resourceType": "Patient", | ||
| "identifier": [{"system": "https://fhir.nhs.uk/Id/nhs-number"}], | ||
| } | ||
| }, | ||
| 201, | ||
| ), | ||
| pytest.param( | ||
| { | ||
| "resource": { | ||
| "resourceType": "Composition", | ||
| "subject": {"identifier": {"value": "PDM_VALIDATION_ERROR"}}, | ||
| } | ||
| }, | ||
| 422, | ||
| ), | ||
| pytest.param( | ||
| { | ||
| "resource": { | ||
| "resourceType": "Patient", | ||
| "identifier": [{"value": "PDM_SERVER_ERROR"}], | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there some other scenarios here covered by CDAPI-72 that need to be included here? I think the BAD_GATEWAY and GATEWAY_TIMEOUT scenarios should be included for example as well as perhaps the authentication errors. |
||
| } | ||
| }, | ||
| 500, | ||
| ), | ||
| ], | ||
| ) | ||
| def test_magic_patient_id_in_payload( | ||
| self, | ||
| mock_datetime: MagicMock, | ||
| mock_uuid: MagicMock, | ||
| mock_storage_helper_put_item: MagicMock, | ||
| basic_document_payload: dict[str, Any], | ||
| payload_entry: dict[str, Any], | ||
| expected_status_code: int, | ||
| handler: ModuleType, | ||
| ) -> None: | ||
|
|
||
| mock_datetime.now.return_value = datetime.datetime( | ||
| 2024, 6, 1, 0, 0, 0, tzinfo=datetime.timezone.utc | ||
| ) | ||
| mock_uuid.return_value = "uuid4" | ||
| payload = {**basic_document_payload, "entry": [payload_entry]} | ||
|
|
||
| response = handler.handle_post_request(payload) | ||
|
|
||
| assert response["status_code"] == expected_status_code | ||
| if expected_status_code == 201: | ||
| assert mock_storage_helper_put_item.called | ||
|
|
||
| @patch("boto3.resource") | ||
| @patch("pdm_mock.handler.uuid4") | ||
| @patch("pdm_mock.handler.datetime") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ def test_bundle_returns_200( | |
| response.headers["X-Correlation-ID"] | ||
| == "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16" | ||
| ) | ||
| assert response.headers["etag"] == 'W/"1"' | ||
|
|
||
| response_data = response.json() | ||
| response_bundle = Bundle.model_validate(response_data, by_alias=True) | ||
|
|
@@ -46,16 +47,14 @@ def test_bundle_returns_200( | |
| assert response_bundle.meta is not None | ||
| response_meta = response_bundle.meta | ||
|
|
||
| print(f"Response meta: {response_meta}") | ||
|
|
||
| assert response_meta.last_updated is not None | ||
| assert response_meta.version_id == "1" | ||
|
|
||
| assert response.headers["etag"] == 'W/"1"' | ||
|
|
||
| def test_no_payload_returns_error(self, client: Client) -> None: | ||
| response = client.send_without_payload( | ||
| request_method="POST", path="FHIR/R4/Bundle" | ||
| request_method="POST", | ||
| path="FHIR/R4/Bundle", | ||
| headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"}, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do these test cases now need to include a correlation ID?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was running the integration tests locally, which doesn't supply a base correlation id like it does on remote because its not using proxygen. I could update the the api gateway mock |
||
| ) | ||
| assert response.status_code == 400 | ||
|
|
||
|
|
@@ -76,7 +75,12 @@ def test_no_payload_returns_error(self, client: Client) -> None: | |
| assert response.status_code == 400 | ||
|
|
||
| def test_empty_payload_returns_error(self, client: Client) -> None: | ||
| response = client.send(data="", request_method="POST", path="FHIR/R4/Bundle") | ||
| response = client.send( | ||
| data="", | ||
| request_method="POST", | ||
| path="FHIR/R4/Bundle", | ||
| headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"}, | ||
| ) | ||
| assert response.status_code == 400 | ||
|
|
||
| response_data = response.json() | ||
|
|
@@ -346,7 +350,10 @@ def test_invalid_payload_returns_error( | |
| self, client: Client, payload: dict[str, Any], expected_diagnostic: str | ||
| ) -> None: | ||
| response = client.send( | ||
| data=json.dumps(payload), request_method="POST", path="FHIR/R4/Bundle" | ||
| data=json.dumps(payload), | ||
| request_method="POST", | ||
| path="FHIR/R4/Bundle", | ||
| headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"}, | ||
| ) | ||
| assert response.status_code == 400 | ||
|
|
||
|
|
@@ -548,7 +555,10 @@ def test_invalid_composition_resource( | |
| } | ||
|
|
||
| response = client.send( | ||
| data=json.dumps(bundle), request_method="POST", path="FHIR/R4/Bundle" | ||
| data=json.dumps(bundle), | ||
| request_method="POST", | ||
| path="FHIR/R4/Bundle", | ||
| headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"}, | ||
| ) | ||
| assert response.status_code == 400 | ||
|
|
||
|
|
@@ -653,7 +663,10 @@ def test_invalid_service_request_resource( | |
| } | ||
|
|
||
| response = client.send( | ||
| data=json.dumps(bundle), request_method="POST", path="FHIR/R4/Bundle" | ||
| data=json.dumps(bundle), | ||
| request_method="POST", | ||
| path="FHIR/R4/Bundle", | ||
| headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"}, | ||
| ) | ||
| assert response.status_code == 400 | ||
|
|
||
|
|
@@ -759,7 +772,10 @@ def test_invalid_practitioner_role_resource( | |
| } | ||
|
|
||
| response = client.send( | ||
| data=json.dumps(bundle), request_method="POST", path="FHIR/R4/Bundle" | ||
| data=json.dumps(bundle), | ||
| request_method="POST", | ||
| path="FHIR/R4/Bundle", | ||
| headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"}, | ||
| ) | ||
| assert response.status_code == 400 | ||
|
|
||
|
|
@@ -870,7 +886,10 @@ def test_invalid_organization_resource( | |
| } | ||
|
|
||
| response = client.send( | ||
| data=json.dumps(bundle), request_method="POST", path="FHIR/R4/Bundle" | ||
| data=json.dumps(bundle), | ||
| request_method="POST", | ||
| path="FHIR/R4/Bundle", | ||
| headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"}, | ||
| ) | ||
| assert response.status_code == 400 | ||
|
|
||
|
|
@@ -886,6 +905,89 @@ def test_invalid_organization_resource( | |
| ], | ||
| } | ||
|
|
||
| def test_pdm_returns_validation_error( | ||
| self, client: Client, build_valid_test_result: Callable[[str, str], Bundle] | ||
| ) -> None: | ||
| bundle = build_valid_test_result("PDM_VALIDATION_ERROR", "ods_code") | ||
|
|
||
| response = client.send( | ||
| data=bundle.model_dump_json(by_alias=True), | ||
| path="FHIR/R4/Bundle", | ||
| request_method="POST", | ||
| headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"}, | ||
| ) | ||
|
|
||
| assert response.status_code == 500 | ||
| assert response.headers["Content-Type"] == "application/fhir+json" | ||
| assert ( | ||
| response.headers["X-Correlation-ID"] | ||
| == "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16" | ||
| ) | ||
|
|
||
| response_data = response.json() | ||
| operation_outcome = OperationOutcome.model_validate(response_data) | ||
|
|
||
| pdm_diagnostic = { | ||
| "resourceType": "OperationOutcome", | ||
| "issue": [ | ||
| { | ||
| "severity": "error", | ||
| "code": "invariant", | ||
| "details": { | ||
| "text": "Bundle size exceeds maximum allowed size or" | ||
| " number of entries." | ||
| }, | ||
| } | ||
| ], | ||
| } | ||
| issue: OperationOutcome.Issue = { | ||
| "severity": "error", | ||
| "code": "invalid", | ||
| "diagnostics": f"Failed to store document: {json.dumps(pdm_diagnostic)}", | ||
| } | ||
|
|
||
| assert operation_outcome.issue == [issue] | ||
|
|
||
| def test_pdm_returns_internal_server_error( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to above, is it worth including some scenarios here for if a BAD_GATEWAY or GATEWAY_TIMEOUT response is returned by PDM? |
||
| self, client: Client, build_valid_test_result: Callable[[str, str], Bundle] | ||
| ) -> None: | ||
| bundle = build_valid_test_result("PDM_SERVER_ERROR", "ods_code") | ||
|
|
||
| response = client.send( | ||
| data=bundle.model_dump_json(by_alias=True), | ||
| path="FHIR/R4/Bundle", | ||
| request_method="POST", | ||
| headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"}, | ||
| ) | ||
|
|
||
| assert response.status_code == 500 | ||
| assert response.headers["Content-Type"] == "application/fhir+json" | ||
| assert ( | ||
| response.headers["X-Correlation-ID"] | ||
| == "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16" | ||
| ) | ||
|
|
||
| response_data = response.json() | ||
| operation_outcome = OperationOutcome.model_validate(response_data) | ||
|
|
||
| pdm_diagnostic = { | ||
| "resourceType": "OperationOutcome", | ||
| "issue": [ | ||
| { | ||
| "severity": "error", | ||
| "code": "exception", | ||
| "details": {"text": "Internal server error"}, | ||
| } | ||
| ], | ||
| } | ||
| issue: OperationOutcome.Issue = { | ||
| "severity": "error", | ||
| "code": "invalid", | ||
| "diagnostics": f"Failed to store document: {json.dumps(pdm_diagnostic)}", | ||
| } | ||
|
|
||
| assert operation_outcome.issue == [issue] | ||
|
|
||
| @pytest.mark.parametrize( | ||
| ("subject"), | ||
| [ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we ever expect the patient resource's
identifiervalue to not be a list here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the exact structure of the document we get from middleware will be but I've seen an example where a patient identifier could be supplied in both a patient resource and as part of the subject in the composition resource. when supplied as part of the composition subject, identifier is not a list.