AMB-1769-Convert-MESH-CSV-records-to-FHIR-objects #47
Conversation
almas-liaqat
force-pushed
the
AMB-1769-Convert-MESH-CSV-records-to-FHIR-objects
branch
from
59c9aa7 to
195e743
Compare
backend/models/immunization.py
Outdated
| def validate_route_of_vaccination_code(cls, v, values): | ||
|
|
||
| if "NOT_GIVEN" in values: | ||
| if not Constants.if_vaccine_not_give(values["NOT_GIVEN"]): |
There was a problem hiding this comment.
I think Constant class and all its methods can be moved into ImmunizationModel as static method. Also ImmunizationModel is related only to CSV record. I think the name of the class should reflect that. If we call it ImmunisationModel then it'll be mixed with the FHIR Immunization.
backend/pyutils/csv_to_model.py
Outdated
| return immunisation_failure | ||
|
|
||
|
|
||
| def read_csv_to_model( |
There was a problem hiding this comment.
This function shouldn't expect a file. This code should perform under the assumption that it receives the content as string. You can read a file during test and pass it to this function. If this function assumes file then, we can't use it in the actual implementation because the content comes from S3 not reading a file.
| self.mesh_output = MeshOutputHandler(self.bucket, self.key) | ||
|
|
||
| @mock_s3 | ||
| def test_write_to_s3(self): |
There was a problem hiding this comment.
What's the purpose of this test? If we want to make sure that the content is written back, then we need to mock the MeshOutputHandler and assert if the write report has been called.
There was a problem hiding this comment.
I think this one is yours
lambda_code/tests/test_mesh.py
Outdated
| immunisation_records[0].dict(), indent=4, default=str | ||
| ) | ||
|
|
||
| assert json.loads(immunisation_record_json) == json.loads( |
There was a problem hiding this comment.
Please use unittest assertions. They'll give you diff and they're integrated with the test-runner. Like self.assertDictEqual() .
lambda_code/src/old_csv_to_model.py
Outdated
There was a problem hiding this comment.
Is this file still necessary? It's called old 😄
There was a problem hiding this comment.
Can you please remove all the TODOs if they are not relevant anymore. Only keep the one that still needs addressing. For example you create an Error class which means the one that's in this file is not used anymore.
nathanwall
deleted the
AMB-1769-Convert-MESH-CSV-records-to-FHIR-objects
branch
3 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.