Skip to content
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

PRMP-1242: Fix missing auth service bug #466

Merged
merged 6 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions lambdas/scripts/batch_update_ods_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@
from pydantic import BaseModel, TypeAdapter, ValidationError
from requests import HTTPError
from services.base.dynamo_service import DynamoDBService
from services.base.nhs_oauth_service import NhsOauthService
from services.base.ssm_service import SSMService
from utils.exceptions import PdsErrorException, PdsResponseValidationException
from utils.utilities import get_pds_service

Expand All @@ -33,10 +31,7 @@ def __init__(
):
self.progress_store = progress_store_file_path
self.table_name = os.getenv("table_name")
pds_service_class = get_pds_service()
ssm_service = SSMService()
auth_service = NhsOauthService(ssm_service)
self.pds_service = pds_service_class(ssm_service, auth_service)
self.pds_service = get_pds_service()
self.dynamo_service = DynamoDBService()
self.progress: Dict[str, ProgressForPatient] = {}

Expand Down
4 changes: 1 addition & 3 deletions lambdas/services/search_patient_details_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
from enums.repository_role import RepositoryRole
from pydantic import ValidationError
from pydantic_core import PydanticSerializationError
from services.base.nhs_oauth_service import NhsOauthService
from services.base.ssm_service import SSMService
from utils.audit_logging_setup import LoggingService
from utils.exceptions import (
Expand All @@ -26,8 +25,7 @@ def __init__(self, user_role, user_ods_code):

def handle_search_patient_request(self, nhs_number):
try:
auth_service = NhsOauthService(self.ssm_service)
pds_api_service = get_pds_service()(self.ssm_service, auth_service)
pds_api_service = get_pds_service()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of consistency, could we modify the name here to pds_service as set in other parts of the code? (e.g lloyd_george_validator.py line 215)

Suggested change
pds_api_service = get_pds_service()
pds_service = get_pds_service()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good shout! I like your thinking, pds_api_service is a pretty loaded term, being the name of the unstubbed service.

patient_details = pds_api_service.fetch_patient_details(nhs_number)

self.check_if_user_authorise(
Expand Down
25 changes: 25 additions & 0 deletions lambdas/tests/unit/utils/test_utilities.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import pytest
from services.mock_pds_service import MockPdsApiService
from services.pds_api_service import PdsApiService
from utils.exceptions import InvalidResourceIdException
from utils.utilities import (
camelize_dict,
flatten,
get_file_key_from_s3_url,
get_pds_service,
redact_id_to_last_4_chars,
validate_nhs_number,
)
Expand Down Expand Up @@ -34,6 +37,28 @@ def test_decapitalise_keys():
assert actual == expected


def test_get_pds_service_returns_stubbed_pds_when_true(monkeypatch):
monkeypatch.setenv("PDS_FHIR_IS_STUBBED", "True")

response = get_pds_service()

assert isinstance(response, MockPdsApiService)


def test_get_pds_service_returns_stubbed_pds_when_unset():
response = get_pds_service()

assert isinstance(response, MockPdsApiService)


def test_get_pds_service_returns_real_pds(monkeypatch):
monkeypatch.setenv("PDS_FHIR_IS_STUBBED", "False")

response = get_pds_service()

assert isinstance(response, PdsApiService)


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed that in lambdas/utils/utilities.py, get_pds_service() checks PDS_FHIR_IS_STUBBED against both "False" and "false" values.
Could we modify these tests to also take into account if the boolean value starts with a lowercase? As testing may require us to modify the PDS_FHIR_IS_STUBBED value manually on the AWS console, I could see ourselves interchangeably setting uppercase or lowercase values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will make these parameterized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point actually, the terraform contains() which determines this value will set to a lower case true/false our primary check should be for a lower case, but having both would be airtight

def test_redact_id():
mock_session_id = "1234532532432"
expected = "2432"
Expand Down
3 changes: 1 addition & 2 deletions lambdas/utils/lloyd_george_validator.py
Original file line number Diff line number Diff line change
Expand Up @@ -212,8 +212,7 @@ def validate_patient_date_of_birth(file_date_of_birth, pds_patient_details):


def getting_patient_info_from_pds(nhs_number: str) -> Patient:
pds_service_class = get_pds_service()
pds_service = pds_service_class(SSMService())
pds_service = get_pds_service()
pds_response = pds_service.pds_request(nhs_number=nhs_number, retry_on_expired=True)
check_pds_response_status(pds_response)
patient = parse_pds_response(pds_response)
Expand Down
16 changes: 10 additions & 6 deletions lambdas/utils/utilities.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
from urllib.parse import urlparse

from inflection import camelize
from services.base.nhs_oauth_service import NhsOauthService
from services.base.ssm_service import SSMService
from services.mock_pds_service import MockPdsApiService
from services.patient_search_service import PatientSearch
from services.pds_api_service import PdsApiService
from utils.exceptions import InvalidResourceIdException

Expand Down Expand Up @@ -42,12 +45,13 @@ def create_reference_id() -> str:
return str(uuid.uuid4())


def get_pds_service():
return (
PdsApiService
if (os.getenv("PDS_FHIR_IS_STUBBED") == "false")
else MockPdsApiService
)
def get_pds_service() -> PatientSearch:
if os.getenv("PDS_FHIR_IS_STUBBED") in ["False", "false"]:
ssm_service = SSMService()
auth_service = NhsOauthService(ssm_service)
return PdsApiService(ssm_service, auth_service)
else:
return MockPdsApiService()


def redact_id_to_last_4_chars(str_id: str) -> str:
Expand Down
Loading