From c9779a209544ea03213eb96aa65b350162c73a96 Mon Sep 17 00:00:00 2001 From: NogaNHS Date: Wed, 18 Oct 2023 13:47:53 +0100 Subject: [PATCH 1/6] prmdr-327 access token fixed --- lambdas/services/pds_api_service.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lambdas/services/pds_api_service.py b/lambdas/services/pds_api_service.py index ee2092e84..9127825f6 100644 --- a/lambdas/services/pds_api_service.py +++ b/lambdas/services/pds_api_service.py @@ -59,10 +59,10 @@ def pds_request(self, nshNumber: str, retry_on_expired: bool): access_token_response = json.loads(access_token_response) access_token = access_token_response["access_token"] access_token_expiration = int(access_token_response["expires_in"]) + int( - access_token_response["issued_at"] + access_token_response["issued_at"]/1000 ) time_safety_margin_seconds = 10 - if time.time() - access_token_expiration < time_safety_margin_seconds: + if time.time() - access_token_expiration > time_safety_margin_seconds: access_token = self.get_new_access_token() x_request_id = str(uuid.uuid4()) From 2ed39d9835ba1a31abca9b407b62531ba01f7117 Mon Sep 17 00:00:00 2001 From: NogaNHS Date: Wed, 18 Oct 2023 17:33:44 +0100 Subject: [PATCH 2/6] refactor pds services to have parent class --- .../search_patient_details_handler.py | 5 +- lambdas/services/mock_pds_service.py | 31 +--- lambdas/services/patient_search_service.py | 44 ++++++ lambdas/services/pds_api_service.py | 38 +---- .../test_search_patient_details_handler.py | 18 +-- .../helpers/data/pds/pds_patient_response.py | 146 +++++++++--------- .../services/test_mock_pds_api_service.py | 13 +- .../unit/services/test_pds_api_service.py | 7 +- 8 files changed, 144 insertions(+), 158 deletions(-) create mode 100644 lambdas/services/patient_search_service.py diff --git a/lambdas/handlers/search_patient_details_handler.py b/lambdas/handlers/search_patient_details_handler.py index b0cf298de..1b3d0c6cd 100644 --- a/lambdas/handlers/search_patient_details_handler.py +++ b/lambdas/handlers/search_patient_details_handler.py @@ -3,7 +3,6 @@ from json import JSONDecodeError from pydantic import ValidationError -from requests import HTTPError from services.pds_api_service import PdsApiService from utils.exceptions import ( InvalidResourceIdException, @@ -16,6 +15,8 @@ from services.mock_pds_service import MockPdsApiService +from utils.decorators.validate_patient_id import validate_patient_id + logger = logging.getLogger() logger.setLevel(logging.INFO) @@ -27,7 +28,7 @@ def get_pds_service(): else MockPdsApiService ) - +@validate_patient_id def lambda_handler(event, context): logger.info("API Gateway event received - processing starts") logger.info(event) diff --git a/lambdas/services/mock_pds_service.py b/lambdas/services/mock_pds_service.py index 2355cf9e2..41f796ccf 100644 --- a/lambdas/services/mock_pds_service.py +++ b/lambdas/services/mock_pds_service.py @@ -1,41 +1,20 @@ import json -from models.pds_models import PatientDetails from requests import Response -from utils.utilities import validate_id from utils.exceptions import ( PdsErrorException, PatientNotFoundException, InvalidResourceIdException, ) -from models.pds_models import Patient +from services.patient_search_service import PatientSearch -class MockPdsApiService: + +class MockPdsApiService(PatientSearch): def __init__(self, *args, **kwargs): pass - def fetch_patient_details(self, nhs_number: str) -> PatientDetails: - validate_id(nhs_number) - - response = self.fake_pds_request(nhs_number) - - if response.status_code == 200: - patient = Patient.model_validate(response.content) - patient_details = patient.get_patient_details(nhs_number) - return patient_details - - if response.status_code == 404: - raise PatientNotFoundException( - "Patient does not exist for given NHS number" - ) - - if response.status_code == 400: - raise InvalidResourceIdException("Invalid NHS number") - - raise PdsErrorException("Error when requesting patient from PDS") - - def fake_pds_request(self, nhsNumber: str) -> Response: + def pds_request(self, nhsNumber: str, *args, **kwargs) -> Response: mock_pds_results: list[dict] = [] try: @@ -59,7 +38,7 @@ def fake_pds_request(self, nhsNumber: str) -> Response: if bool(pds_patient): response.status_code = 200 - response._content = pds_patient + response._content = json.dumps(pds_patient).encode('utf-8') else: response.status_code = 404 diff --git a/lambdas/services/patient_search_service.py b/lambdas/services/patient_search_service.py new file mode 100644 index 000000000..8627959da --- /dev/null +++ b/lambdas/services/patient_search_service.py @@ -0,0 +1,44 @@ +import logging + +from botocore.exceptions import ClientError + +from models.pds_models import PatientDetails, Patient +from requests import Response + +from utils.exceptions import PdsErrorException, PatientNotFoundException, InvalidResourceIdException + +logger = logging.getLogger() +logger.setLevel(logging.INFO) + + +class PatientSearch: + + def fetch_patient_details( + self, + nhs_number: str, + ) -> PatientDetails: + try: + response = self.pds_request(nhs_number, retry_on_expired=True) + return self.handle_response(response, nhs_number) + except ClientError as e: + logger.error(f"Error when getting ssm parameters {e}") + raise PdsErrorException("Failed to preform patient search") + + def handle_response(self, response: Response, nhs_number: str) -> PatientDetails: + if response.status_code == 200: + patient = Patient.model_validate(response.json()) + patient_details = patient.get_patient_details(nhs_number) + return patient_details + + if response.status_code == 404: + raise PatientNotFoundException( + "Patient does not exist for given NHS number" + ) + + if response.status_code == 400: + raise InvalidResourceIdException("Invalid NHS number") + + raise PdsErrorException("Error when requesting patient from PDS") + + def pds_request(self, nhsNumber: str, *args, **kwargs): + pass diff --git a/lambdas/services/pds_api_service.py b/lambdas/services/pds_api_service.py index 9127825f6..7c1b21cd3 100644 --- a/lambdas/services/pds_api_service.py +++ b/lambdas/services/pds_api_service.py @@ -5,55 +5,25 @@ import jwt import requests -from botocore.exceptions import ClientError -from models.pds_models import Patient, PatientDetails -from requests.models import Response, HTTPError +from requests.models import HTTPError from utils.exceptions import ( InvalidResourceIdException, PatientNotFoundException, PdsErrorException, ) -from utils.utilities import validate_id from enums.pds_ssm_parameters import SSMParameter +from services.patient_search_service import PatientSearch + logger = logging.getLogger() logger.setLevel(logging.INFO) -class PdsApiService: +class PdsApiService(PatientSearch): def __init__(self, ssm_service): self.ssm_service = ssm_service - def fetch_patient_details( - self, - nhs_number: str, - ) -> PatientDetails: - try: - logger.info("Using real pds service") - validate_id(nhs_number) - response = self.pds_request(nhs_number, retry_on_expired=True) - return self.handle_response(response, nhs_number) - except ClientError as e: - logger.error(f"Error when getting ssm parameters {e}") - raise PdsErrorException("Failed to preform patient search") - - def handle_response(self, response: Response, nhs_number: str) -> PatientDetails: - if response.status_code == 200: - patient = Patient.model_validate(response.json()) - patient_details = patient.get_patient_details(nhs_number) - return patient_details - - if response.status_code == 404: - raise PatientNotFoundException( - "Patient does not exist for given NHS number" - ) - - if response.status_code == 400: - raise InvalidResourceIdException("Invalid NHS number") - - raise PdsErrorException("Error when requesting patient from PDS") - def pds_request(self, nshNumber: str, retry_on_expired: bool): endpoint, access_token_response = self.get_parameters_for_pds_api_request() access_token_response = json.loads(access_token_response) diff --git a/lambdas/tests/unit/handlers/test_search_patient_details_handler.py b/lambdas/tests/unit/handlers/test_search_patient_details_handler.py index 4de7c7210..8522d7936 100644 --- a/lambdas/tests/unit/handlers/test_search_patient_details_handler.py +++ b/lambdas/tests/unit/handlers/test_search_patient_details_handler.py @@ -1,3 +1,4 @@ +import json import os from unittest.mock import patch @@ -21,10 +22,10 @@ def test_lambda_handler_valid_id_returns_200( ): response = Response() response.status_code = 200 - response._content = PDS_PATIENT + response._content = json.dumps(PDS_PATIENT).encode('utf-8') mocker.patch( - "services.mock_pds_service.MockPdsApiService.fake_pds_request", + "services.mock_pds_service.MockPdsApiService.pds_request", return_value=response, ) @@ -53,7 +54,7 @@ def test_lambda_handler_invalid_id_returns_400( response.status_code = 400 mocker.patch( - "services.mock_pds_service.MockPdsApiService.fake_pds_request", + "services.mock_pds_service.MockPdsApiService.pds_request", return_value=response, ) @@ -80,7 +81,7 @@ def test_lambda_handler_valid_id_not_in_pds_returns_404( response.status_code = 404 mocker.patch( - "services.mock_pds_service.MockPdsApiService.fake_pds_request", + "services.mock_pds_service.MockPdsApiService.pds_request", return_value=response, ) @@ -103,18 +104,11 @@ def test_lambda_handler_valid_id_not_in_pds_returns_404( def test_lambda_handler_missing_id_in_query_params_returns_400( missing_id_event, context, mocker, patch_env_vars ): - response = Response() - response.status_code = 400 - - mocker.patch( - "services.mock_pds_service.MockPdsApiService.fake_pds_request", - return_value=response, - ) actual = lambda_handler(missing_id_event, context) expected = { - "body": "No NHS number found in request parameters.", + "body": "An error occurred due to missing key: 'patientId'", "headers": { "Content-Type": "application/fhir+json", "Access-Control-Allow-Origin": "*", diff --git a/lambdas/tests/unit/helpers/data/pds/pds_patient_response.py b/lambdas/tests/unit/helpers/data/pds/pds_patient_response.py index 5275a461e..773b795a4 100644 --- a/lambdas/tests/unit/helpers/data/pds/pds_patient_response.py +++ b/lambdas/tests/unit/helpers/data/pds/pds_patient_response.py @@ -14,12 +14,12 @@ "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-NHSNumberVerificationStatus", "version": "1.0.0", "code": "01", - "display": "Number present and verified", + "display": "Number present and verified" } ] - }, + } } - ], + ] } ], "meta": { @@ -28,9 +28,9 @@ { "system": "http://terminology.hl7.org/CodeSystem/v3-Confidentiality", "code": "U", - "display": "unrestricted", + "display": "unrestricted" } - ], + ] }, "name": [ { @@ -40,7 +40,7 @@ "given": ["Jane"], "family": "Smith", "prefix": ["Mrs"], - "suffix": ["MBE"], + "suffix": ["MBE"] }, { "id": "1234", @@ -49,8 +49,8 @@ "given": ["Jim"], "family": "Stevens", "prefix": ["Mr"], - "suffix": ["MBE"], - }, + "suffix": ["MBE"] + } ], "gender": "female", "birthDate": "2010-10-22", @@ -63,8 +63,8 @@ "identifier": { "system": "https://fhir.nhs.uk/Id/ods-organization-code", "value": "Y12345", - "period": {"start": "2020-01-01", "end": "2021-12-31"}, - }, + "period": {"start": "2020-01-01", "end": "2021-12-31"} + } } ], "managingOrganization": { @@ -72,8 +72,8 @@ "identifier": { "system": "https://fhir.nhs.uk/Id/ods-organization-code", "value": "Y12345", - "period": {"start": "2020-01-01", "end": "2021-12-31"}, - }, + "period": {"start": "2020-01-01", "end": "2021-12-31"} + } }, "extension": [ { @@ -81,27 +81,27 @@ "valueReference": { "identifier": { "system": "https://fhir.nhs.uk/Id/ods-organization-code", - "value": "Y12345", + "value": "Y12345" } - }, + } }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-PreferredDispenserOrganization", "valueReference": { "identifier": { "system": "https://fhir.nhs.uk/Id/ods-organization-code", - "value": "Y23456", + "value": "Y23456" } - }, + } }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-MedicalApplianceSupplier", "valueReference": { "identifier": { "system": "https://fhir.nhs.uk/Id/ods-organization-code", - "value": "Y34567", + "value": "Y34567" } - }, + } }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-DeathNotificationStatus", @@ -114,16 +114,16 @@ "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-DeathNotificationStatus", "version": "1.0.0", "code": "2", - "display": "Formal - death notice received from Registrar of Deaths", + "display": "Formal - death notice received from Registrar of Deaths" } ] - }, + } }, { "url": "systemEffectiveDate", - "valueDateTime": "2010-10-22T00:00:00+00:00", - }, - ], + "valueDateTime": "2010-10-22T00:00:00+00:00" + } + ] }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-NHSCommunication", @@ -136,13 +136,13 @@ "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-HumanLanguage", "version": "1.0.0", "code": "fr", - "display": "French", + "display": "French" } ] - }, + } }, - {"url": "interpreterRequired", "valueBoolean": True}, - ], + {"url": "interpreterRequired", "valueBoolean": "True"} + ] }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-ContactPreference", @@ -154,10 +154,10 @@ { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-PreferredWrittenCommunicationFormat", "code": "12", - "display": "Braille", + "display": "Braille" } ] - }, + } }, { "url": "PreferredContactMethod", @@ -166,21 +166,21 @@ { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-PreferredContactMethod", "code": "1", - "display": "Letter", + "display": "Letter" } ] - }, + } }, - {"url": "PreferredContactTimes", "valueString": "Not after 7pm"}, - ], + {"url": "PreferredContactTimes", "valueString": "Not after 7pm"} + ] }, { "url": "http://hl7.org/fhir/StructureDefinition/patient-birthPlace", "valueAddress": { "city": "Manchester", "district": "Greater Manchester", - "country": "GBR", - }, + "country": "GBR" + } }, { "url": "https://fhir.nhs.uk/StructureDefinition/Extension-PDS-RemovalFromRegistration", @@ -192,20 +192,20 @@ { "system": "https://fhir.nhs.uk/CodeSystem/PDS-RemovalReasonExitCode", "code": "SCT", - "display": "Transferred to Scotland", + "display": "Transferred to Scotland" } ] - }, + } }, { "url": "effectiveTime", "valuePeriod": { "start": "2020-01-01T00:00:00+00:00", - "end": "2021-12-31T00:00:00+00:00", - }, - }, - ], - }, + "end": "2021-12-31T00:00:00+00:00" + } + } + ] + } ], "telecom": [ { @@ -213,14 +213,14 @@ "period": {"start": "2020-01-01", "end": "2021-12-31"}, "system": "phone", "value": "01632960587", - "use": "home", + "use": "home" }, { "id": "790", "period": {"start": "2019-01-01", "end": "2022-12-31"}, "system": "email", "value": "jane.smith@example.com", - "use": "home", + "use": "home" }, { "id": "OC789", @@ -234,11 +234,11 @@ "valueCoding": { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-OtherContactSystem", "code": "textphone", - "display": "Minicom (Textphone)", - }, + "display": "Minicom (Textphone)" + } } - ], - }, + ] + } ], "contact": [ { @@ -250,12 +250,12 @@ { "system": "http://terminology.hl7.org/CodeSystem/v2-0131", "code": "C", - "display": "Emergency Contact", + "display": "Emergency Contact" } ] } ], - "telecom": [{"system": "phone", "value": "01632960587"}], + "telecom": [{"system": "phone", "value": "01632960587"}] } ], "address": [ @@ -268,7 +268,7 @@ "Boar Lane", "City Centre", "Leeds", - "West Yorkshire", + "West Yorkshire" ], "postalCode": "LS1 6AE", "extension": [ @@ -279,11 +279,11 @@ "url": "type", "valueCoding": { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-AddressKeyType", - "code": "PAF", - }, + "code": "PAF" + } }, - {"url": "value", "valueString": "12345678"}, - ], + {"url": "value", "valueString": "12345678"} + ] }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-AddressKey", @@ -292,13 +292,13 @@ "url": "type", "valueCoding": { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-AddressKeyType", - "code": "UPRN", - }, + "code": "UPRN" + } }, - {"url": "value", "valueString": "123456789012"}, - ], - }, - ], + {"url": "value", "valueString": "123456789012"} + ] + } + ] }, { "id": "T456", @@ -310,7 +310,7 @@ "Boar Lane", "City Centre", "Leeds", - "West Yorkshire", + "West Yorkshire" ], "postalCode": "LS1 6AE", "extension": [ @@ -321,11 +321,11 @@ "url": "type", "valueCoding": { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-AddressKeyType", - "code": "PAF", - }, + "code": "PAF" + } }, {"url": "value", "valueString": "12345678"}, - ], + ] }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-AddressKey", @@ -334,15 +334,15 @@ "url": "type", "valueCoding": { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-AddressKeyType", - "code": "UPRN", - }, + "code": "UPRN" + } }, - {"url": "value", "valueString": "123456789012"}, - ], - }, - ], - }, - ], + {"url": "value", "valueString": "123456789012"} + ] + } + ] + } + ] } PDS_PATIENT_RESTRICTED = { diff --git a/lambdas/tests/unit/services/test_mock_pds_api_service.py b/lambdas/tests/unit/services/test_mock_pds_api_service.py index 41643dfb0..37092d058 100644 --- a/lambdas/tests/unit/services/test_mock_pds_api_service.py +++ b/lambdas/tests/unit/services/test_mock_pds_api_service.py @@ -1,3 +1,5 @@ +import json + import pytest from models.pds_models import PatientDetails from requests.models import Response @@ -18,10 +20,10 @@ def test_fetch_patient_details_valid_returns_PatientDetails(mocker): response = Response() response.status_code = 200 - response._content = PDS_PATIENT + response._content = json.dumps(PDS_PATIENT).encode('utf-8') mocker.patch( - "services.mock_pds_service.MockPdsApiService.fake_pds_request", + "services.mock_pds_service.MockPdsApiService.pds_request", return_value=response, ) @@ -38,10 +40,3 @@ def test_fetch_patient_details_valid_returns_PatientDetails(mocker): ) assert actual == expected - - -def test_fetch_patient_details_invalid_nhs_number_raises_InvalidResourceIdException(): - nhs_number = "000000000" - - with pytest.raises(InvalidResourceIdException): - pds_service.fetch_patient_details(nhs_number) diff --git a/lambdas/tests/unit/services/test_pds_api_service.py b/lambdas/tests/unit/services/test_pds_api_service.py index a9882173d..dea84a421 100644 --- a/lambdas/tests/unit/services/test_pds_api_service.py +++ b/lambdas/tests/unit/services/test_pds_api_service.py @@ -1,5 +1,8 @@ +import json + import pytest from models.pds_models import PatientDetails +from requests import Response from tests.unit.helpers.data.pds.pds_patient_response import PDS_PATIENT from utils.exceptions import ( InvalidResourceIdException, @@ -29,9 +32,9 @@ def update_ssm_parameter(self, *arg, **kwargs): def test_handle_response_200_returns_PatientDetails(mocker): nhs_number = "9000000025" - response = mocker.MagicMock() + response = Response() response.status_code = 200 - response.json.return_value = PDS_PATIENT + response._content = json.dumps(PDS_PATIENT).encode('utf-8') actual = pds_service.handle_response(response, nhs_number) From 58326da7c487ca127ff57e8637419b8810697754 Mon Sep 17 00:00:00 2001 From: NogaNHS Date: Thu, 19 Oct 2023 13:15:34 +0100 Subject: [PATCH 3/6] prmdr-327 unit tests --- lambdas/services/patient_search_service.py | 20 +-- lambdas/services/pds_api_service.py | 49 +++--- .../helpers/data/pds/access_token_response.py | 6 + .../services/test_mock_pds_api_service.py | 6 - .../unit/services/test_pds_api_service.py | 160 +++++++++++++++++- 5 files changed, 195 insertions(+), 46 deletions(-) create mode 100644 lambdas/tests/unit/helpers/data/pds/access_token_response.py diff --git a/lambdas/services/patient_search_service.py b/lambdas/services/patient_search_service.py index 8627959da..fd1da42d3 100644 --- a/lambdas/services/patient_search_service.py +++ b/lambdas/services/patient_search_service.py @@ -1,28 +1,16 @@ -import logging - -from botocore.exceptions import ClientError - from models.pds_models import PatientDetails, Patient from requests import Response from utils.exceptions import PdsErrorException, PatientNotFoundException, InvalidResourceIdException -logger = logging.getLogger() -logger.setLevel(logging.INFO) - - class PatientSearch: - def fetch_patient_details( self, nhs_number: str, ) -> PatientDetails: - try: - response = self.pds_request(nhs_number, retry_on_expired=True) - return self.handle_response(response, nhs_number) - except ClientError as e: - logger.error(f"Error when getting ssm parameters {e}") - raise PdsErrorException("Failed to preform patient search") + response = self.pds_request(nhs_number, retry_on_expired=True) + return self.handle_response(response, nhs_number) + def handle_response(self, response: Response, nhs_number: str) -> PatientDetails: if response.status_code == 200: @@ -40,5 +28,5 @@ def handle_response(self, response: Response, nhs_number: str) -> PatientDetails raise PdsErrorException("Error when requesting patient from PDS") - def pds_request(self, nhsNumber: str, *args, **kwargs): + def pds_request(self, nhsNumber: str, *args, **kwargs) -> Response: pass diff --git a/lambdas/services/pds_api_service.py b/lambdas/services/pds_api_service.py index 7c1b21cd3..43cd3eba6 100644 --- a/lambdas/services/pds_api_service.py +++ b/lambdas/services/pds_api_service.py @@ -5,6 +5,7 @@ import jwt import requests +from botocore.exceptions import ClientError from requests.models import HTTPError from utils.exceptions import ( InvalidResourceIdException, @@ -25,28 +26,32 @@ def __init__(self, ssm_service): self.ssm_service = ssm_service def pds_request(self, nshNumber: str, retry_on_expired: bool): - endpoint, access_token_response = self.get_parameters_for_pds_api_request() - access_token_response = json.loads(access_token_response) - access_token = access_token_response["access_token"] - access_token_expiration = int(access_token_response["expires_in"]) + int( - access_token_response["issued_at"]/1000 - ) - time_safety_margin_seconds = 10 - if time.time() - access_token_expiration > time_safety_margin_seconds: - access_token = self.get_new_access_token() - - x_request_id = str(uuid.uuid4()) - - authorization_header = { - "Authorization": f"Bearer {access_token}", - "X-Request-ID": x_request_id, - } - - url_endpoint = endpoint + "Patient/" + nshNumber - pds_response = requests.get(url=url_endpoint, headers=authorization_header) - if pds_response.status_code == 401 & retry_on_expired: - return self.pds_request(nshNumber, retry_on_expired=False) - return pds_response + try: + endpoint, access_token_response = self.get_parameters_for_pds_api_request() + access_token_response = json.loads(access_token_response) + access_token = access_token_response["access_token"] + access_token_expiration = int(access_token_response["expires_in"]) + int( + access_token_response["issued_at"])/1000 + time_safety_margin_seconds = 10 + if time.time() - access_token_expiration > time_safety_margin_seconds: + access_token = self.get_new_access_token() + + x_request_id = str(uuid.uuid4()) + + authorization_header = { + "Authorization": f"Bearer {access_token}", + "X-Request-ID": x_request_id, + } + + url_endpoint = endpoint + "Patient/" + nshNumber + pds_response = requests.get(url=url_endpoint, headers=authorization_header) + if pds_response.status_code == 401 and retry_on_expired: + return self.pds_request(nshNumber, retry_on_expired=False) + return pds_response + + except ClientError as e: + logger.error(f"Error when getting ssm parameters {e}") + raise PdsErrorException("Failed to preform patient search") def get_new_access_token(self): logger.info("Getting new PDS access token") diff --git a/lambdas/tests/unit/helpers/data/pds/access_token_response.py b/lambdas/tests/unit/helpers/data/pds/access_token_response.py new file mode 100644 index 000000000..989337bde --- /dev/null +++ b/lambdas/tests/unit/helpers/data/pds/access_token_response.py @@ -0,0 +1,6 @@ +RESPONSE_TOKEN = { + "access_token": "Sr5PGv19wTEHJdDr2wx2f7IGd0cw", + "expires_in": "599", + "token_type": "Bearer", + "issued_at": "1650000006000" + } \ No newline at end of file diff --git a/lambdas/tests/unit/services/test_mock_pds_api_service.py b/lambdas/tests/unit/services/test_mock_pds_api_service.py index 37092d058..1e6e506c4 100644 --- a/lambdas/tests/unit/services/test_mock_pds_api_service.py +++ b/lambdas/tests/unit/services/test_mock_pds_api_service.py @@ -1,14 +1,8 @@ import json -import pytest from models.pds_models import PatientDetails from requests.models import Response from tests.unit.helpers.data.pds.pds_patient_response import PDS_PATIENT -from utils.exceptions import ( - InvalidResourceIdException, - PatientNotFoundException, - PdsErrorException, -) from services.mock_pds_service import MockPdsApiService diff --git a/lambdas/tests/unit/services/test_pds_api_service.py b/lambdas/tests/unit/services/test_pds_api_service.py index dea84a421..86cfd9334 100644 --- a/lambdas/tests/unit/services/test_pds_api_service.py +++ b/lambdas/tests/unit/services/test_pds_api_service.py @@ -1,6 +1,7 @@ import json import pytest +from botocore.exceptions import ClientError from models.pds_models import PatientDetails from requests import Response from tests.unit.helpers.data.pds.pds_patient_response import PDS_PATIENT @@ -14,6 +15,8 @@ from enums.pds_ssm_parameters import SSMParameter +from tests.unit.helpers.data.pds.access_token_response import RESPONSE_TOKEN + class FakeSSMService: def __init__(self, *arg, **kwargs): @@ -146,5 +149,158 @@ def test_get_parameters_for_new_access_token(mocker): pds_service.get_parameters_for_new_access_token() fake_ssm_service.get_ssm_parameters.assert_called_with(parameters, with_decryption=True) -def test_get_new_access_token(mocker): - pass \ No newline at end of file +def test_get_new_access_token_return_200(mocker): + response = Response() + response.status_code = 200 + response._content = json.dumps(RESPONSE_TOKEN).encode('utf-8') + mock_nhs_oauth_endpoint = "api.test/endpoint" + mock_token = "test_token" + mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_new_access_token", return_value={SSMParameter.NHS_OAUTH_ENDPOINT.value: mock_nhs_oauth_endpoint}) + mock_create_jwt = mocker.patch("services.pds_api_service.PdsApiService.create_jwt_token_for_new_access_token_request", return_value=mock_token) + mock_api_call_oauth = mocker.patch("services.pds_api_service.PdsApiService.request_new_access_token", return_value=response) + mock_update_ssm = mocker.patch("services.pds_api_service.PdsApiService.update_access_token_ssm") + expected = RESPONSE_TOKEN['access_token'] + + actual = pds_service.get_new_access_token() + + mock_create_jwt.assert_called_with({SSMParameter.NHS_OAUTH_ENDPOINT.value: mock_nhs_oauth_endpoint}) + mock_api_call_oauth.assert_called_with(mock_token, mock_nhs_oauth_endpoint) + mock_update_ssm.assert_called_with(json.dumps(RESPONSE_TOKEN)) + assert expected == actual + +def test_get_new_access_token_raise_PdsErrorException(mocker): + with pytest.raises(PdsErrorException): + + response = Response() + response.status_code = 400 + mock_nhs_oauth_endpoint = "api.test/endpoint" + mock_token = "test_token" + mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_new_access_token", return_value={SSMParameter.NHS_OAUTH_ENDPOINT.value: mock_nhs_oauth_endpoint}) + mock_create_jwt = mocker.patch("services.pds_api_service.PdsApiService.create_jwt_token_for_new_access_token_request", return_value=mock_token) + mock_api_call_oauth = mocker.patch("services.pds_api_service.PdsApiService.request_new_access_token", return_value=response) + mock_update_ssm = mocker.patch("services.pds_api_service.PdsApiService.update_access_token_ssm") + + pds_service.get_new_access_token() + + mock_create_jwt.assert_called_with({SSMParameter.NHS_OAUTH_ENDPOINT.value: mock_nhs_oauth_endpoint}) + mock_api_call_oauth.assert_called_with(mock_token, mock_nhs_oauth_endpoint) + mock_update_ssm.assert_not_called() + +def test_pds_request_valid_token(mocker): + response = Response() + response.status_code = 200 + response._content = json.dumps(PDS_PATIENT).encode('utf-8') + mock_api_request_parameters = ('api.test/endpoint/', json.dumps(RESPONSE_TOKEN)) + nhs_number = "1111111111" + mock_url_endpoint = 'api.test/endpoint/Patient/' + nhs_number + mock_authorization_header = { + "Authorization": f"Bearer {RESPONSE_TOKEN['access_token']}", + "X-Request-ID": "123412342", + } + + mock_get_parameters = mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", return_value=mock_api_request_parameters) + mocker.patch("time.time", return_value=1600000000.953031) + mock_new_access_token = mocker.patch("services.pds_api_service.PdsApiService.get_new_access_token") + mocker.patch("uuid.uuid4", return_value="123412342") + mock_post = mocker.patch("requests.get", return_value=response) + + pds_service.pds_request(nshNumber="1111111111", retry_on_expired=True) + + mock_get_parameters.assert_called_once() + mock_new_access_token.assert_not_called() + mock_post.assert_called_with(url=mock_url_endpoint, headers=mock_authorization_header) + +def test_pds_request_expired_token(mocker): + response = Response() + response.status_code = 200 + response._content = json.dumps(PDS_PATIENT).encode('utf-8') + mock_api_request_parameters = ('api.test/endpoint/', json.dumps(RESPONSE_TOKEN)) + nhs_number = "1111111111" + mock_url_endpoint = 'api.test/endpoint/Patient/' + nhs_number + new_mock_access_token = "mock_access_token" + + mock_authorization_header = { + "Authorization": f"Bearer {new_mock_access_token}", + "X-Request-ID": "123412342", + } + + mock_get_parameters = mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", return_value=mock_api_request_parameters) + mocker.patch("time.time", return_value=1700000000.953031) + mock_new_access_token = mocker.patch("services.pds_api_service.PdsApiService.get_new_access_token", return_value=new_mock_access_token) + mocker.patch("uuid.uuid4", return_value="123412342") + mock_post = mocker.patch("requests.get", return_value=response) + + actual = pds_service.pds_request(nshNumber="1111111111", retry_on_expired=True) + + assert actual == response + mock_get_parameters.assert_called_once() + mock_new_access_token.assert_called_once() + mock_post.assert_called_with(url=mock_url_endpoint, headers=mock_authorization_header) + +def test_pds_request_valid_token_expired_response(mocker): + first_response = Response() + first_response.status_code = 401 + second_response = Response() + second_response.status_code = 200 + second_response._content = json.dumps(PDS_PATIENT).encode('utf-8') + mock_api_request_parameters = ('api.test/endpoint/', json.dumps(RESPONSE_TOKEN)) + nhs_number = "1111111111" + mock_url_endpoint = 'api.test/endpoint/Patient/' + nhs_number + new_mock_access_token = "mock_access_token" + + mock_authorization_header = { + "Authorization": f"Bearer {new_mock_access_token}", + "X-Request-ID": "123412342", + } + + mock_get_parameters = mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", return_value=mock_api_request_parameters) + mocker.patch("time.time", side_effect=[1600000000.953031, 1700000000.953031]) + mock_new_access_token = mocker.patch("services.pds_api_service.PdsApiService.get_new_access_token", return_value=new_mock_access_token) + mocker.patch("uuid.uuid4", return_value="123412342") + mock_post = mocker.patch("requests.get", side_effect=[first_response, second_response]) + + actual = pds_service.pds_request(nshNumber="1111111111", retry_on_expired=True) + + assert actual == second_response + assert mock_get_parameters.call_count == 2 + mock_new_access_token.assert_called_once() + + mock_post.assert_called_with(url=mock_url_endpoint, headers=mock_authorization_header) + +def test_pds_request_valid_token_expired_response_no_retry(mocker): + response = Response() + response.status_code = 401 + mock_api_request_parameters = ('api.test/endpoint/', json.dumps(RESPONSE_TOKEN)) + nhs_number = "1111111111" + mock_url_endpoint = 'api.test/endpoint/Patient/' + nhs_number + mock_authorization_header = { + "Authorization": f"Bearer {RESPONSE_TOKEN['access_token']}", + "X-Request-ID": "123412342", + } + mock_get_parameters = mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", return_value=mock_api_request_parameters) + mocker.patch("time.time", return_value=1600000000.953031) + mock_new_access_token = mocker.patch("services.pds_api_service.PdsApiService.get_new_access_token") + mocker.patch("uuid.uuid4", return_value="123412342") + mock_post = mocker.patch("requests.get", return_value= response) + + actual = pds_service.pds_request(nshNumber="1111111111", retry_on_expired=False) + + assert actual == response + mock_get_parameters.assert_called_once() + mock_new_access_token.assert_not_called() + mock_post.assert_called_with(url=mock_url_endpoint, headers=mock_authorization_header) + +def test_pds_request_raise_pds_error_exception(mocker): + with pytest.raises(PdsErrorException): + + mock_get_parameters = mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", side_effect=ClientError( + {"Error": {"Code": "500", "Message": "mocked error"}}, "test" + )) + mock_new_access_token = mocker.patch("services.pds_api_service.PdsApiService.get_new_access_token") + mock_post = mocker.patch("requests.get") + + pds_service.pds_request(nshNumber="1111111111", retry_on_expired=True) + + mock_get_parameters.assert_called_once() + mock_new_access_token.assert_not_called() + mock_post.assert_not_called() From e1dff01b67795b22f82d99d8503d9cfda5ff1a0c Mon Sep 17 00:00:00 2001 From: NogaNHS Date: Thu, 19 Oct 2023 13:24:09 +0100 Subject: [PATCH 4/6] prmdr-327 add logs --- lambdas/handlers/search_patient_details_handler.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lambdas/handlers/search_patient_details_handler.py b/lambdas/handlers/search_patient_details_handler.py index 1b3d0c6cd..22dc9d274 100644 --- a/lambdas/handlers/search_patient_details_handler.py +++ b/lambdas/handlers/search_patient_details_handler.py @@ -44,23 +44,27 @@ def lambda_handler(event, context): return ApiGatewayResponse(200, response, "GET").create_api_gateway_response() except PatientNotFoundException as e: - return ApiGatewayResponse(404, f"{str(e)}", "GET").create_api_gateway_response() + logger.error(f"PDS not found: {str(e)}") + return ApiGatewayResponse(404, f"Patient does not exist for given NHS number", "GET").create_api_gateway_response() except (InvalidResourceIdException, PdsErrorException) as e: - return ApiGatewayResponse(400, f"{str(e)}", "GET").create_api_gateway_response() + logger.error(f"PDS Error: {str(e)}") + return ApiGatewayResponse(400, f"An error occurred while searching for patient", "GET").create_api_gateway_response() except ValidationError as e: + logger.error(f"Failed to parse PDS data:{str(e)}") return ApiGatewayResponse( - 400, f"Failed to parse PDS data: {str(e)}", "GET" + 400, f"Failed to parse PDS data", "GET" ).create_api_gateway_response() except JSONDecodeError as e: + logger.error(f"Error while decoding Json:{str(e)}") return ApiGatewayResponse( - 400, f"Invalid json in body: {str(e)}", "GET" + 400, f"Invalid json in body", "GET" ).create_api_gateway_response() except KeyError as e: - logger.info(f"Error parsing patientId from json: {str(e)}") + logger.error(f"Error parsing patientId from json: {str(e)}") return ApiGatewayResponse( 400, "No NHS number found in request parameters.", "GET" ).create_api_gateway_response() From 3aa9087c58d5d29894045ce7aef79e166bf803be Mon Sep 17 00:00:00 2001 From: NogaNHS Date: Thu, 19 Oct 2023 13:27:01 +0100 Subject: [PATCH 5/6] prmdr-327 format --- .../search_patient_details_handler.py | 11 +- lambdas/services/mock_pds_service.py | 2 +- lambdas/services/patient_search_service.py | 12 +- lambdas/services/pds_api_service.py | 6 +- .../test_search_patient_details_handler.py | 3 +- .../helpers/data/pds/access_token_response.py | 4 +- .../helpers/data/pds/pds_patient_response.py | 146 +++++++------- .../services/test_mock_pds_api_service.py | 2 +- .../unit/services/test_pds_api_service.py | 179 +++++++++++++----- 9 files changed, 229 insertions(+), 136 deletions(-) diff --git a/lambdas/handlers/search_patient_details_handler.py b/lambdas/handlers/search_patient_details_handler.py index 22dc9d274..8d6707c19 100644 --- a/lambdas/handlers/search_patient_details_handler.py +++ b/lambdas/handlers/search_patient_details_handler.py @@ -24,10 +24,11 @@ def get_pds_service(): return ( PdsApiService - if (os.getenv("PDS_FHIR_IS_STUBBED") == 'false') + if (os.getenv("PDS_FHIR_IS_STUBBED") == "false") else MockPdsApiService ) + @validate_patient_id def lambda_handler(event, context): logger.info("API Gateway event received - processing starts") @@ -45,11 +46,15 @@ def lambda_handler(event, context): except PatientNotFoundException as e: logger.error(f"PDS not found: {str(e)}") - return ApiGatewayResponse(404, f"Patient does not exist for given NHS number", "GET").create_api_gateway_response() + return ApiGatewayResponse( + 404, f"Patient does not exist for given NHS number", "GET" + ).create_api_gateway_response() except (InvalidResourceIdException, PdsErrorException) as e: logger.error(f"PDS Error: {str(e)}") - return ApiGatewayResponse(400, f"An error occurred while searching for patient", "GET").create_api_gateway_response() + return ApiGatewayResponse( + 400, f"An error occurred while searching for patient", "GET" + ).create_api_gateway_response() except ValidationError as e: logger.error(f"Failed to parse PDS data:{str(e)}") diff --git a/lambdas/services/mock_pds_service.py b/lambdas/services/mock_pds_service.py index 41f796ccf..f464aa7e5 100644 --- a/lambdas/services/mock_pds_service.py +++ b/lambdas/services/mock_pds_service.py @@ -38,7 +38,7 @@ def pds_request(self, nhsNumber: str, *args, **kwargs) -> Response: if bool(pds_patient): response.status_code = 200 - response._content = json.dumps(pds_patient).encode('utf-8') + response._content = json.dumps(pds_patient).encode("utf-8") else: response.status_code = 404 diff --git a/lambdas/services/patient_search_service.py b/lambdas/services/patient_search_service.py index fd1da42d3..d5a3c0fee 100644 --- a/lambdas/services/patient_search_service.py +++ b/lambdas/services/patient_search_service.py @@ -1,17 +1,21 @@ from models.pds_models import PatientDetails, Patient from requests import Response -from utils.exceptions import PdsErrorException, PatientNotFoundException, InvalidResourceIdException +from utils.exceptions import ( + PdsErrorException, + PatientNotFoundException, + InvalidResourceIdException, +) + class PatientSearch: def fetch_patient_details( - self, - nhs_number: str, + self, + nhs_number: str, ) -> PatientDetails: response = self.pds_request(nhs_number, retry_on_expired=True) return self.handle_response(response, nhs_number) - def handle_response(self, response: Response, nhs_number: str) -> PatientDetails: if response.status_code == 200: patient = Patient.model_validate(response.json()) diff --git a/lambdas/services/pds_api_service.py b/lambdas/services/pds_api_service.py index 43cd3eba6..de0146d2b 100644 --- a/lambdas/services/pds_api_service.py +++ b/lambdas/services/pds_api_service.py @@ -30,8 +30,10 @@ def pds_request(self, nshNumber: str, retry_on_expired: bool): endpoint, access_token_response = self.get_parameters_for_pds_api_request() access_token_response = json.loads(access_token_response) access_token = access_token_response["access_token"] - access_token_expiration = int(access_token_response["expires_in"]) + int( - access_token_response["issued_at"])/1000 + access_token_expiration = ( + int(access_token_response["expires_in"]) + + int(access_token_response["issued_at"]) / 1000 + ) time_safety_margin_seconds = 10 if time.time() - access_token_expiration > time_safety_margin_seconds: access_token = self.get_new_access_token() diff --git a/lambdas/tests/unit/handlers/test_search_patient_details_handler.py b/lambdas/tests/unit/handlers/test_search_patient_details_handler.py index 8522d7936..13223469f 100644 --- a/lambdas/tests/unit/handlers/test_search_patient_details_handler.py +++ b/lambdas/tests/unit/handlers/test_search_patient_details_handler.py @@ -22,7 +22,7 @@ def test_lambda_handler_valid_id_returns_200( ): response = Response() response.status_code = 200 - response._content = json.dumps(PDS_PATIENT).encode('utf-8') + response._content = json.dumps(PDS_PATIENT).encode("utf-8") mocker.patch( "services.mock_pds_service.MockPdsApiService.pds_request", @@ -104,7 +104,6 @@ def test_lambda_handler_valid_id_not_in_pds_returns_404( def test_lambda_handler_missing_id_in_query_params_returns_400( missing_id_event, context, mocker, patch_env_vars ): - actual = lambda_handler(missing_id_event, context) expected = { diff --git a/lambdas/tests/unit/helpers/data/pds/access_token_response.py b/lambdas/tests/unit/helpers/data/pds/access_token_response.py index 989337bde..aa86095cd 100644 --- a/lambdas/tests/unit/helpers/data/pds/access_token_response.py +++ b/lambdas/tests/unit/helpers/data/pds/access_token_response.py @@ -2,5 +2,5 @@ "access_token": "Sr5PGv19wTEHJdDr2wx2f7IGd0cw", "expires_in": "599", "token_type": "Bearer", - "issued_at": "1650000006000" - } \ No newline at end of file + "issued_at": "1650000006000", +} diff --git a/lambdas/tests/unit/helpers/data/pds/pds_patient_response.py b/lambdas/tests/unit/helpers/data/pds/pds_patient_response.py index 773b795a4..3d92888f2 100644 --- a/lambdas/tests/unit/helpers/data/pds/pds_patient_response.py +++ b/lambdas/tests/unit/helpers/data/pds/pds_patient_response.py @@ -14,12 +14,12 @@ "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-NHSNumberVerificationStatus", "version": "1.0.0", "code": "01", - "display": "Number present and verified" + "display": "Number present and verified", } ] - } + }, } - ] + ], } ], "meta": { @@ -28,9 +28,9 @@ { "system": "http://terminology.hl7.org/CodeSystem/v3-Confidentiality", "code": "U", - "display": "unrestricted" + "display": "unrestricted", } - ] + ], }, "name": [ { @@ -40,7 +40,7 @@ "given": ["Jane"], "family": "Smith", "prefix": ["Mrs"], - "suffix": ["MBE"] + "suffix": ["MBE"], }, { "id": "1234", @@ -49,8 +49,8 @@ "given": ["Jim"], "family": "Stevens", "prefix": ["Mr"], - "suffix": ["MBE"] - } + "suffix": ["MBE"], + }, ], "gender": "female", "birthDate": "2010-10-22", @@ -63,8 +63,8 @@ "identifier": { "system": "https://fhir.nhs.uk/Id/ods-organization-code", "value": "Y12345", - "period": {"start": "2020-01-01", "end": "2021-12-31"} - } + "period": {"start": "2020-01-01", "end": "2021-12-31"}, + }, } ], "managingOrganization": { @@ -72,8 +72,8 @@ "identifier": { "system": "https://fhir.nhs.uk/Id/ods-organization-code", "value": "Y12345", - "period": {"start": "2020-01-01", "end": "2021-12-31"} - } + "period": {"start": "2020-01-01", "end": "2021-12-31"}, + }, }, "extension": [ { @@ -81,27 +81,27 @@ "valueReference": { "identifier": { "system": "https://fhir.nhs.uk/Id/ods-organization-code", - "value": "Y12345" + "value": "Y12345", } - } + }, }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-PreferredDispenserOrganization", "valueReference": { "identifier": { "system": "https://fhir.nhs.uk/Id/ods-organization-code", - "value": "Y23456" + "value": "Y23456", } - } + }, }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-MedicalApplianceSupplier", "valueReference": { "identifier": { "system": "https://fhir.nhs.uk/Id/ods-organization-code", - "value": "Y34567" + "value": "Y34567", } - } + }, }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-DeathNotificationStatus", @@ -114,16 +114,16 @@ "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-DeathNotificationStatus", "version": "1.0.0", "code": "2", - "display": "Formal - death notice received from Registrar of Deaths" + "display": "Formal - death notice received from Registrar of Deaths", } ] - } + }, }, { "url": "systemEffectiveDate", - "valueDateTime": "2010-10-22T00:00:00+00:00" - } - ] + "valueDateTime": "2010-10-22T00:00:00+00:00", + }, + ], }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-NHSCommunication", @@ -136,13 +136,13 @@ "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-HumanLanguage", "version": "1.0.0", "code": "fr", - "display": "French" + "display": "French", } ] - } + }, }, - {"url": "interpreterRequired", "valueBoolean": "True"} - ] + {"url": "interpreterRequired", "valueBoolean": "True"}, + ], }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-ContactPreference", @@ -154,10 +154,10 @@ { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-PreferredWrittenCommunicationFormat", "code": "12", - "display": "Braille" + "display": "Braille", } ] - } + }, }, { "url": "PreferredContactMethod", @@ -166,21 +166,21 @@ { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-PreferredContactMethod", "code": "1", - "display": "Letter" + "display": "Letter", } ] - } + }, }, - {"url": "PreferredContactTimes", "valueString": "Not after 7pm"} - ] + {"url": "PreferredContactTimes", "valueString": "Not after 7pm"}, + ], }, { "url": "http://hl7.org/fhir/StructureDefinition/patient-birthPlace", "valueAddress": { "city": "Manchester", "district": "Greater Manchester", - "country": "GBR" - } + "country": "GBR", + }, }, { "url": "https://fhir.nhs.uk/StructureDefinition/Extension-PDS-RemovalFromRegistration", @@ -192,20 +192,20 @@ { "system": "https://fhir.nhs.uk/CodeSystem/PDS-RemovalReasonExitCode", "code": "SCT", - "display": "Transferred to Scotland" + "display": "Transferred to Scotland", } ] - } + }, }, { "url": "effectiveTime", "valuePeriod": { "start": "2020-01-01T00:00:00+00:00", - "end": "2021-12-31T00:00:00+00:00" - } - } - ] - } + "end": "2021-12-31T00:00:00+00:00", + }, + }, + ], + }, ], "telecom": [ { @@ -213,14 +213,14 @@ "period": {"start": "2020-01-01", "end": "2021-12-31"}, "system": "phone", "value": "01632960587", - "use": "home" + "use": "home", }, { "id": "790", "period": {"start": "2019-01-01", "end": "2022-12-31"}, "system": "email", "value": "jane.smith@example.com", - "use": "home" + "use": "home", }, { "id": "OC789", @@ -234,11 +234,11 @@ "valueCoding": { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-OtherContactSystem", "code": "textphone", - "display": "Minicom (Textphone)" - } + "display": "Minicom (Textphone)", + }, } - ] - } + ], + }, ], "contact": [ { @@ -250,12 +250,12 @@ { "system": "http://terminology.hl7.org/CodeSystem/v2-0131", "code": "C", - "display": "Emergency Contact" + "display": "Emergency Contact", } ] } ], - "telecom": [{"system": "phone", "value": "01632960587"}] + "telecom": [{"system": "phone", "value": "01632960587"}], } ], "address": [ @@ -268,7 +268,7 @@ "Boar Lane", "City Centre", "Leeds", - "West Yorkshire" + "West Yorkshire", ], "postalCode": "LS1 6AE", "extension": [ @@ -279,11 +279,11 @@ "url": "type", "valueCoding": { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-AddressKeyType", - "code": "PAF" - } + "code": "PAF", + }, }, - {"url": "value", "valueString": "12345678"} - ] + {"url": "value", "valueString": "12345678"}, + ], }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-AddressKey", @@ -292,13 +292,13 @@ "url": "type", "valueCoding": { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-AddressKeyType", - "code": "UPRN" - } + "code": "UPRN", + }, }, - {"url": "value", "valueString": "123456789012"} - ] - } - ] + {"url": "value", "valueString": "123456789012"}, + ], + }, + ], }, { "id": "T456", @@ -310,7 +310,7 @@ "Boar Lane", "City Centre", "Leeds", - "West Yorkshire" + "West Yorkshire", ], "postalCode": "LS1 6AE", "extension": [ @@ -321,11 +321,11 @@ "url": "type", "valueCoding": { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-AddressKeyType", - "code": "PAF" - } + "code": "PAF", + }, }, {"url": "value", "valueString": "12345678"}, - ] + ], }, { "url": "https://fhir.hl7.org.uk/StructureDefinition/Extension-UKCore-AddressKey", @@ -334,15 +334,15 @@ "url": "type", "valueCoding": { "system": "https://fhir.hl7.org.uk/CodeSystem/UKCore-AddressKeyType", - "code": "UPRN" - } + "code": "UPRN", + }, }, - {"url": "value", "valueString": "123456789012"} - ] - } - ] - } - ] + {"url": "value", "valueString": "123456789012"}, + ], + }, + ], + }, + ], } PDS_PATIENT_RESTRICTED = { diff --git a/lambdas/tests/unit/services/test_mock_pds_api_service.py b/lambdas/tests/unit/services/test_mock_pds_api_service.py index 1e6e506c4..f418d157a 100644 --- a/lambdas/tests/unit/services/test_mock_pds_api_service.py +++ b/lambdas/tests/unit/services/test_mock_pds_api_service.py @@ -14,7 +14,7 @@ def test_fetch_patient_details_valid_returns_PatientDetails(mocker): response = Response() response.status_code = 200 - response._content = json.dumps(PDS_PATIENT).encode('utf-8') + response._content = json.dumps(PDS_PATIENT).encode("utf-8") mocker.patch( "services.mock_pds_service.MockPdsApiService.pds_request", diff --git a/lambdas/tests/unit/services/test_pds_api_service.py b/lambdas/tests/unit/services/test_pds_api_service.py index 86cfd9334..7a47243f5 100644 --- a/lambdas/tests/unit/services/test_pds_api_service.py +++ b/lambdas/tests/unit/services/test_pds_api_service.py @@ -23,11 +23,12 @@ def __init__(self, *arg, **kwargs): pass def get_ssm_parameters(self, parameters_keys, *arg, **kwargs): - return {parameter : f"test_value_{parameter}" for parameter in parameters_keys} + return {parameter: f"test_value_{parameter}" for parameter in parameters_keys} def update_ssm_parameter(self, *arg, **kwargs): pass + fake_ssm_service = FakeSSMService() pds_service = PdsApiService(fake_ssm_service) @@ -37,7 +38,7 @@ def test_handle_response_200_returns_PatientDetails(mocker): response = Response() response.status_code = 200 - response._content = json.dumps(PDS_PATIENT).encode('utf-8') + response._content = json.dumps(PDS_PATIENT).encode("utf-8") actual = pds_service.handle_response(response, nhs_number) @@ -126,17 +127,27 @@ def test_create_jwt_for_new_access_token(mocker): headers={"kid": "test_string_pds_kid"}, ) + def test_get_parameters_for_pds_api_request(): - ssm_parameters_expected = (f"test_value_{SSMParameter.PDS_API_ENDPOINT.value}", f"test_value_{SSMParameter.PDS_API_ACCESS_TOKEN.value}") + ssm_parameters_expected = ( + f"test_value_{SSMParameter.PDS_API_ENDPOINT.value}", + f"test_value_{SSMParameter.PDS_API_ACCESS_TOKEN.value}", + ) actual = pds_service.get_parameters_for_pds_api_request() assert ssm_parameters_expected == actual + def test_update_access_token_ssm(mocker): fake_ssm_service.update_ssm_parameter = mocker.MagicMock() pds_service.update_access_token_ssm("test_string") - fake_ssm_service.update_ssm_parameter.assert_called_with(parameter_key=SSMParameter.PDS_API_ACCESS_TOKEN.value, parameter_value="test_string", parameter_type="SecureString") + fake_ssm_service.update_ssm_parameter.assert_called_with( + parameter_key=SSMParameter.PDS_API_ACCESS_TOKEN.value, + parameter_value="test_string", + parameter_type="SecureString", + ) + def test_get_parameters_for_new_access_token(mocker): parameters = [ @@ -147,60 +158,97 @@ def test_get_parameters_for_new_access_token(mocker): ] fake_ssm_service.get_ssm_parameters = mocker.MagicMock() pds_service.get_parameters_for_new_access_token() - fake_ssm_service.get_ssm_parameters.assert_called_with(parameters, with_decryption=True) + fake_ssm_service.get_ssm_parameters.assert_called_with( + parameters, with_decryption=True + ) + def test_get_new_access_token_return_200(mocker): response = Response() response.status_code = 200 - response._content = json.dumps(RESPONSE_TOKEN).encode('utf-8') + response._content = json.dumps(RESPONSE_TOKEN).encode("utf-8") mock_nhs_oauth_endpoint = "api.test/endpoint" mock_token = "test_token" - mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_new_access_token", return_value={SSMParameter.NHS_OAUTH_ENDPOINT.value: mock_nhs_oauth_endpoint}) - mock_create_jwt = mocker.patch("services.pds_api_service.PdsApiService.create_jwt_token_for_new_access_token_request", return_value=mock_token) - mock_api_call_oauth = mocker.patch("services.pds_api_service.PdsApiService.request_new_access_token", return_value=response) - mock_update_ssm = mocker.patch("services.pds_api_service.PdsApiService.update_access_token_ssm") - expected = RESPONSE_TOKEN['access_token'] + mocker.patch( + "services.pds_api_service.PdsApiService.get_parameters_for_new_access_token", + return_value={SSMParameter.NHS_OAUTH_ENDPOINT.value: mock_nhs_oauth_endpoint}, + ) + mock_create_jwt = mocker.patch( + "services.pds_api_service.PdsApiService.create_jwt_token_for_new_access_token_request", + return_value=mock_token, + ) + mock_api_call_oauth = mocker.patch( + "services.pds_api_service.PdsApiService.request_new_access_token", + return_value=response, + ) + mock_update_ssm = mocker.patch( + "services.pds_api_service.PdsApiService.update_access_token_ssm" + ) + expected = RESPONSE_TOKEN["access_token"] actual = pds_service.get_new_access_token() - mock_create_jwt.assert_called_with({SSMParameter.NHS_OAUTH_ENDPOINT.value: mock_nhs_oauth_endpoint}) + mock_create_jwt.assert_called_with( + {SSMParameter.NHS_OAUTH_ENDPOINT.value: mock_nhs_oauth_endpoint} + ) mock_api_call_oauth.assert_called_with(mock_token, mock_nhs_oauth_endpoint) mock_update_ssm.assert_called_with(json.dumps(RESPONSE_TOKEN)) assert expected == actual + def test_get_new_access_token_raise_PdsErrorException(mocker): with pytest.raises(PdsErrorException): - response = Response() response.status_code = 400 mock_nhs_oauth_endpoint = "api.test/endpoint" mock_token = "test_token" - mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_new_access_token", return_value={SSMParameter.NHS_OAUTH_ENDPOINT.value: mock_nhs_oauth_endpoint}) - mock_create_jwt = mocker.patch("services.pds_api_service.PdsApiService.create_jwt_token_for_new_access_token_request", return_value=mock_token) - mock_api_call_oauth = mocker.patch("services.pds_api_service.PdsApiService.request_new_access_token", return_value=response) - mock_update_ssm = mocker.patch("services.pds_api_service.PdsApiService.update_access_token_ssm") + mocker.patch( + "services.pds_api_service.PdsApiService.get_parameters_for_new_access_token", + return_value={ + SSMParameter.NHS_OAUTH_ENDPOINT.value: mock_nhs_oauth_endpoint + }, + ) + mock_create_jwt = mocker.patch( + "services.pds_api_service.PdsApiService.create_jwt_token_for_new_access_token_request", + return_value=mock_token, + ) + mock_api_call_oauth = mocker.patch( + "services.pds_api_service.PdsApiService.request_new_access_token", + return_value=response, + ) + mock_update_ssm = mocker.patch( + "services.pds_api_service.PdsApiService.update_access_token_ssm" + ) pds_service.get_new_access_token() - mock_create_jwt.assert_called_with({SSMParameter.NHS_OAUTH_ENDPOINT.value: mock_nhs_oauth_endpoint}) + mock_create_jwt.assert_called_with( + {SSMParameter.NHS_OAUTH_ENDPOINT.value: mock_nhs_oauth_endpoint} + ) mock_api_call_oauth.assert_called_with(mock_token, mock_nhs_oauth_endpoint) mock_update_ssm.assert_not_called() + def test_pds_request_valid_token(mocker): response = Response() response.status_code = 200 - response._content = json.dumps(PDS_PATIENT).encode('utf-8') - mock_api_request_parameters = ('api.test/endpoint/', json.dumps(RESPONSE_TOKEN)) + response._content = json.dumps(PDS_PATIENT).encode("utf-8") + mock_api_request_parameters = ("api.test/endpoint/", json.dumps(RESPONSE_TOKEN)) nhs_number = "1111111111" - mock_url_endpoint = 'api.test/endpoint/Patient/' + nhs_number + mock_url_endpoint = "api.test/endpoint/Patient/" + nhs_number mock_authorization_header = { "Authorization": f"Bearer {RESPONSE_TOKEN['access_token']}", "X-Request-ID": "123412342", } - mock_get_parameters = mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", return_value=mock_api_request_parameters) + mock_get_parameters = mocker.patch( + "services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", + return_value=mock_api_request_parameters, + ) mocker.patch("time.time", return_value=1600000000.953031) - mock_new_access_token = mocker.patch("services.pds_api_service.PdsApiService.get_new_access_token") + mock_new_access_token = mocker.patch( + "services.pds_api_service.PdsApiService.get_new_access_token" + ) mocker.patch("uuid.uuid4", return_value="123412342") mock_post = mocker.patch("requests.get", return_value=response) @@ -208,15 +256,18 @@ def test_pds_request_valid_token(mocker): mock_get_parameters.assert_called_once() mock_new_access_token.assert_not_called() - mock_post.assert_called_with(url=mock_url_endpoint, headers=mock_authorization_header) + mock_post.assert_called_with( + url=mock_url_endpoint, headers=mock_authorization_header + ) + def test_pds_request_expired_token(mocker): response = Response() response.status_code = 200 - response._content = json.dumps(PDS_PATIENT).encode('utf-8') - mock_api_request_parameters = ('api.test/endpoint/', json.dumps(RESPONSE_TOKEN)) + response._content = json.dumps(PDS_PATIENT).encode("utf-8") + mock_api_request_parameters = ("api.test/endpoint/", json.dumps(RESPONSE_TOKEN)) nhs_number = "1111111111" - mock_url_endpoint = 'api.test/endpoint/Patient/' + nhs_number + mock_url_endpoint = "api.test/endpoint/Patient/" + nhs_number new_mock_access_token = "mock_access_token" mock_authorization_header = { @@ -224,9 +275,15 @@ def test_pds_request_expired_token(mocker): "X-Request-ID": "123412342", } - mock_get_parameters = mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", return_value=mock_api_request_parameters) + mock_get_parameters = mocker.patch( + "services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", + return_value=mock_api_request_parameters, + ) mocker.patch("time.time", return_value=1700000000.953031) - mock_new_access_token = mocker.patch("services.pds_api_service.PdsApiService.get_new_access_token", return_value=new_mock_access_token) + mock_new_access_token = mocker.patch( + "services.pds_api_service.PdsApiService.get_new_access_token", + return_value=new_mock_access_token, + ) mocker.patch("uuid.uuid4", return_value="123412342") mock_post = mocker.patch("requests.get", return_value=response) @@ -235,17 +292,20 @@ def test_pds_request_expired_token(mocker): assert actual == response mock_get_parameters.assert_called_once() mock_new_access_token.assert_called_once() - mock_post.assert_called_with(url=mock_url_endpoint, headers=mock_authorization_header) + mock_post.assert_called_with( + url=mock_url_endpoint, headers=mock_authorization_header + ) + def test_pds_request_valid_token_expired_response(mocker): first_response = Response() first_response.status_code = 401 second_response = Response() second_response.status_code = 200 - second_response._content = json.dumps(PDS_PATIENT).encode('utf-8') - mock_api_request_parameters = ('api.test/endpoint/', json.dumps(RESPONSE_TOKEN)) + second_response._content = json.dumps(PDS_PATIENT).encode("utf-8") + mock_api_request_parameters = ("api.test/endpoint/", json.dumps(RESPONSE_TOKEN)) nhs_number = "1111111111" - mock_url_endpoint = 'api.test/endpoint/Patient/' + nhs_number + mock_url_endpoint = "api.test/endpoint/Patient/" + nhs_number new_mock_access_token = "mock_access_token" mock_authorization_header = { @@ -253,11 +313,19 @@ def test_pds_request_valid_token_expired_response(mocker): "X-Request-ID": "123412342", } - mock_get_parameters = mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", return_value=mock_api_request_parameters) + mock_get_parameters = mocker.patch( + "services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", + return_value=mock_api_request_parameters, + ) mocker.patch("time.time", side_effect=[1600000000.953031, 1700000000.953031]) - mock_new_access_token = mocker.patch("services.pds_api_service.PdsApiService.get_new_access_token", return_value=new_mock_access_token) + mock_new_access_token = mocker.patch( + "services.pds_api_service.PdsApiService.get_new_access_token", + return_value=new_mock_access_token, + ) mocker.patch("uuid.uuid4", return_value="123412342") - mock_post = mocker.patch("requests.get", side_effect=[first_response, second_response]) + mock_post = mocker.patch( + "requests.get", side_effect=[first_response, second_response] + ) actual = pds_service.pds_request(nshNumber="1111111111", retry_on_expired=True) @@ -265,38 +333,53 @@ def test_pds_request_valid_token_expired_response(mocker): assert mock_get_parameters.call_count == 2 mock_new_access_token.assert_called_once() - mock_post.assert_called_with(url=mock_url_endpoint, headers=mock_authorization_header) + mock_post.assert_called_with( + url=mock_url_endpoint, headers=mock_authorization_header + ) + def test_pds_request_valid_token_expired_response_no_retry(mocker): response = Response() response.status_code = 401 - mock_api_request_parameters = ('api.test/endpoint/', json.dumps(RESPONSE_TOKEN)) + mock_api_request_parameters = ("api.test/endpoint/", json.dumps(RESPONSE_TOKEN)) nhs_number = "1111111111" - mock_url_endpoint = 'api.test/endpoint/Patient/' + nhs_number + mock_url_endpoint = "api.test/endpoint/Patient/" + nhs_number mock_authorization_header = { "Authorization": f"Bearer {RESPONSE_TOKEN['access_token']}", "X-Request-ID": "123412342", } - mock_get_parameters = mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", return_value=mock_api_request_parameters) + mock_get_parameters = mocker.patch( + "services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", + return_value=mock_api_request_parameters, + ) mocker.patch("time.time", return_value=1600000000.953031) - mock_new_access_token = mocker.patch("services.pds_api_service.PdsApiService.get_new_access_token") + mock_new_access_token = mocker.patch( + "services.pds_api_service.PdsApiService.get_new_access_token" + ) mocker.patch("uuid.uuid4", return_value="123412342") - mock_post = mocker.patch("requests.get", return_value= response) + mock_post = mocker.patch("requests.get", return_value=response) actual = pds_service.pds_request(nshNumber="1111111111", retry_on_expired=False) assert actual == response mock_get_parameters.assert_called_once() mock_new_access_token.assert_not_called() - mock_post.assert_called_with(url=mock_url_endpoint, headers=mock_authorization_header) + mock_post.assert_called_with( + url=mock_url_endpoint, headers=mock_authorization_header + ) + def test_pds_request_raise_pds_error_exception(mocker): with pytest.raises(PdsErrorException): - - mock_get_parameters = mocker.patch("services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", side_effect=ClientError( - {"Error": {"Code": "500", "Message": "mocked error"}}, "test" - )) - mock_new_access_token = mocker.patch("services.pds_api_service.PdsApiService.get_new_access_token") + mock_get_parameters = mocker.patch( + "services.pds_api_service.PdsApiService.get_parameters_for_pds_api_request", + side_effect=ClientError( + {"Error": {"Code": "500", "Message": "mocked error"}}, "test" + ), + ) + mock_new_access_token = mocker.patch( + "services.pds_api_service.PdsApiService.get_new_access_token" + ) mock_post = mocker.patch("requests.get") pds_service.pds_request(nshNumber="1111111111", retry_on_expired=True) From d169a1cdac512b1f50dde138483e2b86d8dfd071 Mon Sep 17 00:00:00 2001 From: NogaNHS Date: Thu, 19 Oct 2023 13:33:34 +0100 Subject: [PATCH 6/6] prmdr-327 move handle response test to new file --- .../services/test_patient_search_service.py | 67 +++++++++++++++++++ .../unit/services/test_pds_api_service.py | 53 --------------- 2 files changed, 67 insertions(+), 53 deletions(-) create mode 100644 lambdas/tests/unit/services/test_patient_search_service.py diff --git a/lambdas/tests/unit/services/test_patient_search_service.py b/lambdas/tests/unit/services/test_patient_search_service.py new file mode 100644 index 000000000..67b7b47ac --- /dev/null +++ b/lambdas/tests/unit/services/test_patient_search_service.py @@ -0,0 +1,67 @@ +import json + +import pytest +from requests import Response +from services.patient_search_service import PatientSearch + +from models.pds_models import PatientDetails +from tests.unit.helpers.data.pds.pds_patient_response import PDS_PATIENT +from utils.exceptions import ( + PatientNotFoundException, + InvalidResourceIdException, + PdsErrorException, +) + +search_service = PatientSearch() + + +def test_handle_response_200_returns_PatientDetails(mocker): + nhs_number = "9000000025" + + response = Response() + response.status_code = 200 + response._content = json.dumps(PDS_PATIENT).encode("utf-8") + + actual = search_service.handle_response(response, nhs_number) + + expected = PatientDetails( + givenName=["Jane"], + familyName="Smith", + birthDate="2010-10-22", + postalCode="LS1 6AE", + nhsNumber="9000000009", + superseded=False, + restricted=False, + ) + + assert actual == expected + + +def test_handle_response_404_raises_PatientNotFoundException(mocker): + nhs_number = "9000000025" + + response = mocker.MagicMock() + response.status_code = 404 + + with pytest.raises(PatientNotFoundException): + search_service.handle_response(response, nhs_number) + + +def test_handle_response_400_raises_InvalidResourceIdException(mocker): + nhs_number = "9000000025" + + response = mocker.MagicMock() + response.status_code = 400 + + with pytest.raises(InvalidResourceIdException): + search_service.handle_response(response, nhs_number) + + +def test_handle_response_catch_all_raises_PdsErrorException(mocker): + nhs_number = "9000000025" + + response = mocker.MagicMock() + response.status_code = 500 + + with pytest.raises(PdsErrorException): + search_service.handle_response(response, nhs_number) diff --git a/lambdas/tests/unit/services/test_pds_api_service.py b/lambdas/tests/unit/services/test_pds_api_service.py index 7a47243f5..7cd880fcc 100644 --- a/lambdas/tests/unit/services/test_pds_api_service.py +++ b/lambdas/tests/unit/services/test_pds_api_service.py @@ -2,7 +2,6 @@ import pytest from botocore.exceptions import ClientError -from models.pds_models import PatientDetails from requests import Response from tests.unit.helpers.data.pds.pds_patient_response import PDS_PATIENT from utils.exceptions import ( @@ -33,58 +32,6 @@ def update_ssm_parameter(self, *arg, **kwargs): pds_service = PdsApiService(fake_ssm_service) -def test_handle_response_200_returns_PatientDetails(mocker): - nhs_number = "9000000025" - - response = Response() - response.status_code = 200 - response._content = json.dumps(PDS_PATIENT).encode("utf-8") - - actual = pds_service.handle_response(response, nhs_number) - - expected = PatientDetails( - givenName=["Jane"], - familyName="Smith", - birthDate="2010-10-22", - postalCode="LS1 6AE", - nhsNumber="9000000009", - superseded=False, - restricted=False, - ) - - assert actual == expected - - -def test_handle_response_404_raises_PatientNotFoundException(mocker): - nhs_number = "9000000025" - - response = mocker.MagicMock() - response.status_code = 404 - - with pytest.raises(PatientNotFoundException): - pds_service.handle_response(response, nhs_number) - - -def test_handle_response_400_raises_InvalidResourceIdException(mocker): - nhs_number = "9000000025" - - response = mocker.MagicMock() - response.status_code = 400 - - with pytest.raises(InvalidResourceIdException): - pds_service.handle_response(response, nhs_number) - - -def test_handle_response_catch_all_raises_PdsErrorException(mocker): - nhs_number = "9000000025" - - response = mocker.MagicMock() - response.status_code = 500 - - with pytest.raises(PdsErrorException): - pds_service.handle_response(response, nhs_number) - - def test_request_new_token_is_call_with_correct_data(mocker): mock_jwt_token = "testtest" mock_endpoint = "api.endpoint/mock"