Conversation
| res = query.count() | ||
| if res == 0: | ||
| raise HTTPException( | ||
| status_code=403, detail="User is not authorized for proposal" |
There was a problem hiding this comment.
Here i would return 404 Proposal not found and log that the user was attempting to access a proposal for which they didn't have access
| old_sample.Crystal = crystal | ||
|
|
||
| proposal = crystal.Protein.Proposal | ||
| authorize_for_proposal(proposal.proposalId) |
There was a problem hiding this comment.
I dont think you should need this.
old_sample = get_samples(blSampleId=blSampleId, skip=0, limit=1).first
will check the authorization of the current user vs the sample and return an empty list if they dont have access, thus throwing IndexError which the route function will catch
| ) | ||
|
|
||
|
|
||
| @router.get("/components/types", response_model=list[schema.ComponentType]) |
There was a problem hiding this comment.
Returning a list is technically not a valid REST response. It should be encapsulated in a dict. Could be solved by simply paginating this response
| return Paged(total=total, results=results, skip=skip, limit=limit) | ||
|
|
||
|
|
||
| def get_component_types() -> list[models.ComponentType]: |
There was a problem hiding this comment.
Maybe paginate these responses, if only for consistency?
| return results | ||
|
|
||
|
|
||
| def get_concentration_types() -> list[models.ConcentrationType]: |
There was a problem hiding this comment.
Same as above, maybe paginate these responses, if only for consistency?
| sample = crud.update_sample(blSampleId=blSampleId, sample=sample) | ||
| return sample | ||
| except IndexError: | ||
| raise HTTPException(status_code=404, detail="Sample not found") |
There was a problem hiding this comment.
You might like a final except Exception to catch anything else that might go wrong as is done on the shipment route:
except Exception:
logger.exception(
f"Could not update shipping `{shippingId}` with payload `{shipping}`"
)
raise HTTPException(status_code=400, detail="Could not update shipment")
| ) -> models.BLSample: | ||
| """create a sample""" | ||
| sample = crud.create_sample(sample=sample) | ||
| return sample |
There was a problem hiding this comment.
you can concat this onto a single line
stufisher
left a comment
stufisher
left a comment
There was a problem hiding this comment.
Looks good, thanks.
Would you like to return the sample_compositions with the sample? The query is a bit involved and some parsing faff, but its doable
|
Probably also need a couple of checks for |
2 participants
Routes for ssx sample definition: