Conversation
FimranNHS
requested review from
JamesW1-NHS,
Thomas-Boyle,
dlzhry2nhs and
krupasindhubaral
|
This branch is working on a ticket in the NHS England VED JIRA Project. Here's a handy link to the ticket: VED-1106 |
|
FimranNHS
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
FimranNHS
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
FimranNHS
temporarily deployed
to
internal-dev-sandbox
GitHub Actions
Inactive
dlzhry2nhs
left a comment
dlzhry2nhs
left a comment
There was a problem hiding this comment.
Noice. Some optional comments.
| } | ||
| print(f"\n Search Post request (mixed valid/invalid target-disease) - \n {context.request}") | ||
| context.response = http_requests_session.post(context.url, headers=context.headers, data=context.request) | ||
| trigger_search_request_by_httpMethod(context, httpMethod=httpMethod) |
There was a problem hiding this comment.
Technically in python and per coding standards we're looking for Pep 8 ideally variables and method names would be lower case with underscores separating the words.
i.e http_method. Not major, as it would be something to rework in the automation tests anyway. So can leave as optional to do later if you'd prefer.
|
|
||
| def validate_inf_ack_file(context, success: bool = True) -> bool: | ||
| content = context.fileContent | ||
| content = content.replace("\r\n", "\n") |
There was a problem hiding this comment.
Is this issue just being seen on certain Windows laptops?
| elapsed = 0 | ||
|
|
||
| if folderName == "forwardedFile": | ||
| expected_extensions = {".csv", ".json"} |
There was a problem hiding this comment.
Could link this line to VED-1071. E.g. put a # VED-1071 TODO comment about restricting to JSON only once DPS has done the required development work.
More widely it does beg the question why the INF Acks are still CSV. That is a design question for the architect. Ultimately does not matter too much if no one is using them yet, but if we want clients other than DPS to use it in future, it does seem weird that one is CSV and one is JSON!
|
|
||
| if expected_extensions == {".csv"}: | ||
| return {"csv": found_files[".csv"]} | ||
| if expected_extensions.issubset(found_files.keys()): |
There was a problem hiding this comment.
Subset could mean that even more keys were found. This check is fine, but it could be tightened up with:
len(expected_extensions.difference(found_files.keys())) == 0. There might be a neater more Pythonic way to do it, but that will be more precise at least.
2 participants
Summary
Add any other relevant notes or explanations here. Remove this line if you have nothing to add.
Reviews Required
Review Checklist
ℹ️ This section is to be filled in by the reviewer.