feat: incorporate api commons exception handlers #175
Conversation
lchen-2101
requested review from
hkeeler,
guffee23,
jcadam14 and
nargis-sultani
| status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
| content=f"The period ({period_code}) does not exist, therefore a Filing can not be created for this period.", | ||
| raise RegTechHttpException( | ||
| status_code=status.HTTP_404_NOT_FOUND, |
There was a problem hiding this comment.
@billhimmelsbach @shindigira @meissadia not sure how far along front end is with the status code; is it ok to switch most of the 422s in the filing API which are filing doesn't exist, submission doesn't exist, or filing period doesn't exist to 404s? It makes more sense in my mind; since in this example a filing period doesn't exist, therefore a not found error.
There was a problem hiding this comment.
I am going to take your updates here and build a wiki page with response codes based on when and we can discuss those during devchat?
| return JSONResponse( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
| content=f"The period ({period_code}) does not exist, therefore a Filing can not be created for this period.", | ||
| raise RegTechHttpException( |
There was a problem hiding this comment.
maybe more of a stylistic thing, but throwing an exception for non-200 responses, and let exception handlers deal with the rest looks better in my eyes; thoughts?
There was a problem hiding this comment.
I've seen both ways, and I don't have a preference but sticking with one way is ideal. I know there are a couple of places where we're raising and HttpException but others we're returning JSON responses. Philosophically non-200 responses are errors/exceptions/deviations from happy-path so raising versus returning makes a lot of sense. I say we go with that as a standard approach across the board.
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jcadam14
dismissed
their stale review
Merge conflict, fix that and it's good to go.
closes #162