[GPCAPIM-255] Controller module #49
Conversation
Vox-Ben
force-pushed
the
feature/GPCAPIM-255_controller_module
branch
from
1ab29e7 to
fc5b194
Compare
|
ALB Target: |
1 similar comment
|
ALB Target: |
|
ALB Target: |
2 similar comments
|
ALB Target: |
|
ALB Target: |
|
|
||
| return consumer_asid, provider_asid, provider_endpoint | ||
|
|
||
| def call_gp_provider( |
There was a problem hiding this comment.
I think this does more than call the provider (the provider module does only that): perhaps orchestrate_gp_provider-call?
There was a problem hiding this comment.
That feels a bit verbose for what is a main in all but name. How about just run? So using the controller looks like:
controller = Controller(pds_url, sds_url, nhsd_urid, timeout)
patient_details = controller.run(body, headers, auth_token)
davidhamill1-nhs
left a comment
davidhamill1-nhs
left a comment
There was a problem hiding this comment.
1329 lines of code is not a small PR.
| c = _make_controller() | ||
|
|
||
| # PDS returns None by default | ||
| body = make_request_body("9434765919") | ||
| headers = make_headers() | ||
|
|
||
| r = c.call_gp_provider(body, headers, "token-abc") | ||
|
|
||
| assert r.status_code == 404 | ||
| assert "No PDS patient found for NHS number" in (r.data or "") |
There was a problem hiding this comment.
Given how much is mocked, it feels like this is testing the mocks more than the actual controller.
It is not clear from the test setup why this should return a 404/No patient found.
There was a problem hiding this comment.
It's mocking out PDS, SDS and the GP provider. I don't want to call any of those in the unit test for the controller - that's for the integration tests (which don't exist, because we weren't going to stand up the server to run them against for steel thread). In this case, no patient with the given NHS number has been added to the PDS stub, and therefore when the controller calls the PDS stub it gets nothing back and returns a 404 with the given error message.
Does that make sense? I'll add a comment to explain that a bit more.
There was a problem hiding this comment.
These feel less like unit tests, testing a unit of code, and more like integration tests.
You could break call_gp_prodvier in to multiple private method calls, eg
Put
try:
nhs_number = self._get_details_from_body(request_body)
except RequestError as err:
return FlaskResponse(
status_code=err.status_code,
data=str(err),
)
# Extract consumer ODS from headers
consumer_ods = headers.get("Ods-from", "").strip()
if not consumer_ods:
return FlaskResponse(
status_code=400,
data='Missing required header "Ods-from"',
)
trace_id = headers.get("X-Request-ID")
if trace_id is None:
return FlaskResponse(
status_code=400, data="Missing required header: X-Request-ID"
)
try:
provider_ods = self._get_pds_details(auth_token, consumer_ods, nhs_number)
except RequestError as err:
return FlaskResponse(status_code=err.status_code, data=str(err))
in its own method and unit test that. And similarly with the calls to ODS for the provider and the consumer.
There was a problem hiding this comment.
There are lots of tests for handling error conditions here, but I couldn't actually see a 'happy path' 200 with expected response test.
There was a problem hiding this comment.
Yeah. Um. Whoops. Failing to see the wood for the trees there. I've added a happy-path test.
There was a problem hiding this comment.
On the point about breaking it up, I actually did break it up, because Ruff was complaining about cognitive complexity. So what was call_gp_provider (now run) now calls _get_details_from_body, _get_pds_details and _get_sds_details, but overall control is still within that public function. I left the unit tests just testing run though because a) they were already written and b) it seemed to make more sense to me to test the public function, which is where the functionality that we care about (ie. that it calls each in turn and returns the result) lives, than to test the private functions, which exist only for purposes of abstracting out some of the complexity and could be easily changed in future.
Vox-Ben
force-pushed
the
feature/GPCAPIM-255_controller_module
branch
from
c39e48c to
58c7e73
Compare
Vox-Ben
force-pushed
the
feature/GPCAPIM-255_controller_module
branch
from
58c7e73 to
5966ab0
Compare
|
|
Deployment Complete
|
|
This PR superseded by #59 |
3 participants
Description
Creates the controller class that orchestrates calls to the other gateway components and to the GP provider
Context
This module is needed so that we have something that actually calls all the components that we need to call and manages their responses.
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.