PRMP-1242: Fix missing auth service bug #466
Conversation
abbas-khan10
temporarily deployed
to
development
GitHub Actions
Inactive
…n instance of SSM Service
AndyFlintNHS
temporarily deployed
to
development
GitHub Actions
Inactive
AndyFlintNHS
temporarily deployed
to
development
GitHub Actions
Inactive
AndyFlintNHS
temporarily deployed
to
development
GitHub Actions
Inactive
AndyFlintNHS
temporarily deployed
to
development
GitHub Actions
Inactive
| @@ -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() | |||
There was a problem hiding this comment.
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)
| pds_api_service = get_pds_service() | |
| pds_service = get_pds_service() |
There was a problem hiding this comment.
Good shout! I like your thinking, pds_api_service is a pretty loaded term, being the name of the unstubbed service.
| 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) | ||
|
|
||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sure, will make these parameterized.
There was a problem hiding this comment.
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
AndyFlintNHS
temporarily deployed
to
development
GitHub Actions
Inactive
No description provided.