Feature/minimal state assignment #25
Conversation
Signed-off-by: Adam Blanchet <[email protected]>
This way, we can personalise the data to the test. Signed-off-by: Adam Blanchet <[email protected]>
Signed-off-by: Adam Blanchet <[email protected]>
Signed-off-by: Adam Blanchet <[email protected]>
Signed-off-by: Adam Blanchet <[email protected]>
Signed-off-by: Adam Blanchet <[email protected]>
Signed-off-by: Adam Blanchet <[email protected]>
Signed-off-by: Adam Blanchet <[email protected]>
ghost
requested a review
from 
| @@ -15,6 +15,7 @@ uvicorn = { version = "^0.17", extras = ["standard"] } | |||
| ml-warehouse = { git = "https://github.com/wtsi-npg/ml-warehouse-python.git", tag="1.0.0" } | |||
| SQLAlchemy = { version = "^1.4.35", extras = ["pymysql"] } | |||
| pydantic = "^1.9.0" | |||
| product_id = { git = "https://github.com/absanger/product_id.git", branch="master"} | |||
There was a problem hiding this comment.
What is this for? Why is it in your personal space on github where it could disappear at a moment's notice?
There was a problem hiding this comment.
We've discussed this. I withdraw my objection
lang_qc/endpoints/qc_state.py
Outdated
| ).scalar() | ||
| if mlwh_well is None: | ||
| raise HTTPException( | ||
| status_code=400, detail="Well is not in the MLWH database." |
There was a problem hiding this comment.
It's nice if you can include what well was requested by the user so their own logs are informative. It can also improve the utility of server logs when there are lots of 400's being issued
There was a problem hiding this comment.
I think this is also a case for a 404, rather than a 400. The request is valid, but the target doesn't exist.
lang_qc/endpoints/qc_state.py
Outdated
| try: | ||
| update_qc_state(qc_status, qc_state, qcdb_session) | ||
| except Exception as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) |
There was a problem hiding this comment.
Catching all errors and declaring them a 400 is potentially a lie. If the database goes away or there is an integrity issue it would be a 500.
| ).one_or_none() | ||
| if desired_qc_state_dict is None: | ||
| raise Exception( | ||
| "Desired QC state is not in the QC database. It might not be allowed." |
There was a problem hiding this comment.
As above, you could put the qc_status_post.qc_state into this error and the user would be certain of what they asked for. It sounds self-evident but it's really useful when the client is confused.
lang_qc/util/qc_state_helpers.py
Outdated
| ).scalar_one_or_none() | ||
| if user is None: | ||
| raise Exception( | ||
| "User has not been found in the QC database. Has it been registered?" |
There was a problem hiding this comment.
"Have they been registered?" - Users don't like being objectified.
Signed-off-by: Adam Blanchet <[email protected]>
lang_qc/endpoints/qc_state.py
Outdated
| detail=f"An error occured: {str(e)}\nRequest body was: {request_body.json()}", | ||
| ) | ||
|
|
||
| qcdb_session.merge(qc_state) |
There was a problem hiding this comment.
I think this is a much more complex operation than you need here. Look into using .with_for_update or similar so that we can row-lock the already existing QC states and prevent two users altering the same well state.
There was a problem hiding this comment.
Opened #27 to not forget about this.
Merge didn't do anything in this specific case as objects were already associated with the session. Stopped using no_autoflush and passed an id rather than an ORM object already associated with a session. Wrap everything in a transaction, figure out how to properly deal with race conditions later. Signed-off-by: Adam Blanchet <[email protected]>
This is basically a stripped down version of #24.
There are tests and a working
qc_assignendpoint. But there are fewer enforced checks: it will change a QC state without thinking, it's up to the client to decide the rules for what should or should not be allowed to be changed in different situations. So we can have a single endpoint to do claiming, stealing wells and changing states.It does return an error in the following cases: