NO TICKET: removed admin and old schemas, added observation/water-chemistry#66
Conversation
feat: added observation/water-chemistry
jirhiker
changed the title
# Conflicts: # schemas/__init__.py
|
|
||
|
|
||
| @router.post("/water-chemistry", status_code=HTTP_201_CREATED) | ||
| def add_water_chemistry_observation( |
There was a problem hiding this comment.
Is it necessary to specify a specific type of observation vs adding a general observation? I think the sample_matrix field in the Sample table would indirectly identify the type of observation (groundwater, surface water, soil, etc.)
There was a problem hiding this comment.
By limiting to a type of observation, we can perform better data validations, i.e, if all the necessary fields are provided. This would not be as easy or explicit with a general add_observation endpoint.
There was a problem hiding this comment.
So this is something that's necessary at the validation level? Additional fields are not needed in the sample or observation tables, correct?
| if "field_sample_id" in data: | ||
| field_sample_id = data.pop("field_sample_id") | ||
| data.pop( | ||
| "sample_id", None | ||
| ) # Ensure sample_id is not set if field_sample_id is used | ||
|
|
||
| sql = select(Sample).where(Sample.field_sample_id == field_sample_id) | ||
| sample = session.scalar(sql) | ||
| if not sample: | ||
| raise ValueError(f"Sample with id {field_sample_id} does not exist") | ||
| data["sample"] = sample |
There was a problem hiding this comment.
I think that this if block can be removed because
- The
observationmodel and schema don't have afield_sample_idfield (this may be an artifact from a previous iteration of the schema?) sample_idis also a non-nullable field with a foreign key constraint, so it should be in the payload (not popped).- If the corresponding
sampledoes not exist the database will raise an error and that can be handled at the endpoint (raise a 409 error with an appropriate error message).
- If the corresponding
There was a problem hiding this comment.
This is necessary for how the frontend works to upload the csv.
The way the frontend current works, when adding an Observation, it needs to refer to the Sample via the field_sample_id not the Sample.id
There was a problem hiding this comment.
is that be cause Sample.id is not defined until Sample has been created? If that's the case, could you use the sample_id from the POST /sample response on the frontend? Or, would that not be feasible given how the frontend is setup.
There was a problem hiding this comment.
"is that be cause Sample.id is not defined until Sample has been created?" correct
Thats probably the correct way to do it eventually, e.g. using the sample_id from POST response.
It wasn't feasible with the current structure, but I have some ideas on how to better implement this in the future and we can remove this if block
There was a problem hiding this comment.
Sounds good! We can make a note of this and then when the new implementation is done on the frontend this if block can be removed. If a ticket is made for that implementation this could be a subtask. We can also add a # REFACTOR TODO note here as well.
ksmuczynski
left a comment
ksmuczynski
left a comment
There was a problem hiding this comment.
Looks good overall. Left a few comments questioning the need to ad a specific type of observation as opposed to a general observation. Open to discussion.
jacob-a-brown
left a comment
jacob-a-brown
left a comment
There was a problem hiding this comment.
See previous comments
Codecov Report❌ Patch coverage is
|
jacob-a-brown
left a comment
jacob-a-brown
left a comment
There was a problem hiding this comment.
I noted down the future removal of the if field_sample_id in data in my backlog notes too
4 participants
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?