[CDAPI-152]: Unhappy path integration tests#121
[CDAPI-152]: Unhappy path integration tests#121MohammadPatelNHS wants to merge 4 commits intomainfrom
Conversation
MohammadPatelNHS
force-pushed
the
feature/CDAPI-152
branch
from
cdd319d to
9d88422
Compare
MohammadPatelNHS
force-pushed
the
feature/CDAPI-152
branch
from
6285e34 to
32cbee5
Compare
|
|
Deployment Complete
|
nhsd-jack-wainwright
left a comment
nhsd-jack-wainwright
left a comment
There was a problem hiding this comment.
LGTM, just a few minor comments 👍
|
|
||
| patient = ( | ||
| resource["identifier"][0].get("value") | ||
| if type(resource["identifier"]) is list |
There was a problem hiding this comment.
Would we ever expect the patient resource's identifier value to not be a list here?
There was a problem hiding this comment.
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.
| return None | ||
|
|
||
| if len(patient_values) > 1: | ||
| if len(set(patient_values)) > 1: |
There was a problem hiding this comment.
Are you create a set from patient_values here to account for the same value being provided multiple times?
There was a problem hiding this comment.
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.
it does still return an error if the nhs numbers supplied in each resource is different.
| { | ||
| "resource": { | ||
| "resourceType": "Patient", | ||
| "identifier": [{"value": "PDM_SERVER_ERROR"}], |
There was a problem hiding this comment.
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.
|
|
||
| assert operation_outcome.issue == [issue] | ||
|
|
||
| def test_pdm_returns_internal_server_error( |
There was a problem hiding this comment.
Similar to above, is it worth including some scenarios here for if a BAD_GATEWAY or GATEWAY_TIMEOUT response is returned by PDM?
| request_method="POST", path="FHIR/R4/Bundle" | ||
| request_method="POST", | ||
| path="FHIR/R4/Bundle", | ||
| headers={"X-Correlation-ID": "bb038f9a-dc45-49e1-bcfd-3ab3c3de5e16"}, |
There was a problem hiding this comment.
Why do these test cases now need to include a correlation ID?
There was a problem hiding this comment.
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 server.py file to include a correlation id always instead
2 participants
Description
Context
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.